fix: do not delete active Jobs on SIGTERM — leave for orphan reattach (FAR-107)
Root cause of Nancy's k8s_job_deleted_externally false positive: the paperclip server itself receives SIGTERM during rolling deploys, evictions, scale-down, etc. The previous SIGTERM handler iterated activeJobs and deleted every Job before exiting, which surfaced in the in-flight heartbeat as "K8s Job was deleted externally" — even though nothing external touched it. With reattachOrphanedJobs=true (default), this is exactly the wrong behaviour: leaving the Jobs alive lets the next paperclip process discover them via the orphan-classification path and reattach their log streams. With reattachOrphanedJobs=false the operator opted into manual cleanup, so we still must not auto-delete. The Job's ownerReference (FAR-15) keeps the prompt Secret tied to the Job, so both survive together and TTL handles cleanup on natural completion. Test rewritten to assert the new contract: SIGTERM must not touch K8s Jobs. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
@@ -1771,7 +1771,7 @@ describe("execute: SIGTERM handler best-effort cleanup", () => {
|
|||||||
vi.useRealTimers();
|
vi.useRealTimers();
|
||||||
});
|
});
|
||||||
|
|
||||||
it("deletes the active Job when SIGTERM fires during execution", async () => {
|
it("does NOT delete active Jobs on SIGTERM — leaves them for orphan reattach (FAR-107)", async () => {
|
||||||
// Mock process.kill to prevent the test process from actually being killed.
|
// Mock process.kill to prevent the test process from actually being killed.
|
||||||
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
|
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
|
||||||
|
|
||||||
@@ -1782,17 +1782,19 @@ describe("execute: SIGTERM handler best-effort cleanup", () => {
|
|||||||
// Flush microtasks through the async setup chain: getSelfPodInfo, listJobs,
|
// Flush microtasks through the async setup chain: getSelfPodInfo, listJobs,
|
||||||
// readSkillEntries, prepareBundle, createJob, onLog, activeJobs.add(), and
|
// readSkillEntries, prepareBundle, createJob, onLog, activeJobs.add(), and
|
||||||
// ensureSigtermHandler() all complete before the try block enters streaming.
|
// ensureSigtermHandler() all complete before the try block enters streaming.
|
||||||
// 30 rounds is more than enough for the ~7 sequential await points.
|
|
||||||
for (let i = 0; i < 30; i++) await Promise.resolve();
|
for (let i = 0; i < 30; i++) await Promise.resolve();
|
||||||
|
|
||||||
// Emit SIGTERM — the process.once handler fires synchronously and kicks off
|
// Reset deleteJob spy after setup so we can detect any SIGTERM-driven calls.
|
||||||
// async cleanup (deleteNamespacedJob). The mock resolves immediately.
|
mockBatchDeleteJob.mockClear();
|
||||||
|
|
||||||
|
// Emit SIGTERM — the handler must re-raise to the default handler without
|
||||||
|
// touching the K8s Job. Deleting the Job here would surface as
|
||||||
|
// k8s_job_deleted_externally in the in-flight run (FAR-107).
|
||||||
process.emit("SIGTERM");
|
process.emit("SIGTERM");
|
||||||
|
|
||||||
// Flush microtasks for deleteJob to resolve and the .then(process.kill) to run.
|
|
||||||
for (let i = 0; i < 10; i++) await Promise.resolve();
|
for (let i = 0; i < 10; i++) await Promise.resolve();
|
||||||
|
|
||||||
expect(mockBatchDeleteJob).toHaveBeenCalled();
|
expect(mockBatchDeleteJob).not.toHaveBeenCalled();
|
||||||
expect(killSpy).toHaveBeenCalledWith(process.pid, "SIGTERM");
|
expect(killSpy).toHaveBeenCalledWith(process.pid, "SIGTERM");
|
||||||
|
|
||||||
killSpy.mockRestore();
|
killSpy.mockRestore();
|
||||||
|
|||||||
+14
-24
@@ -58,30 +58,20 @@ function ensureSigtermHandler(): void {
|
|||||||
if (sigtermHandlerRegistered) return;
|
if (sigtermHandlerRegistered) return;
|
||||||
sigtermHandlerRegistered = true;
|
sigtermHandlerRegistered = true;
|
||||||
process.once("SIGTERM", () => {
|
process.once("SIGTERM", () => {
|
||||||
const jobs = [...activeJobs];
|
// Do NOT delete active K8s Jobs on SIGTERM (FAR-107). Paperclip itself
|
||||||
void Promise.allSettled(
|
// receives SIGTERM during rolling deploys, evictions, scale-down, etc.
|
||||||
jobs.map(async (ref) => {
|
// Deleting the Jobs we own there causes the in-flight heartbeat to surface
|
||||||
try {
|
// a false-positive `k8s_job_deleted_externally` error and tears down work
|
||||||
const batchApi = getBatchApi(ref.kubeconfigPath);
|
// the user expected to keep running.
|
||||||
await batchApi.deleteNamespacedJob({
|
//
|
||||||
name: ref.jobName,
|
// The correct behaviour with `reattachOrphanedJobs=true` (default) is to
|
||||||
namespace: ref.namespace,
|
// leave the Jobs alive: the next paperclip process discovers them via the
|
||||||
body: { propagationPolicy: "Background" },
|
// orphan-classification path and reattaches their log streams. When
|
||||||
});
|
// `reattachOrphanedJobs=false` the operator explicitly opted into manual
|
||||||
} catch { /* best-effort */ }
|
// cleanup and should not have us auto-deleting either. The owning Job's
|
||||||
if (ref.promptSecretName && ref.promptSecretNamespace) {
|
// ownerReference (FAR-15) keeps the prompt Secret tied to the Job, so
|
||||||
try {
|
// both survive together and TTL cleans them up after natural completion.
|
||||||
const coreApi = getCoreApi(ref.kubeconfigPath);
|
process.kill(process.pid, "SIGTERM");
|
||||||
await coreApi.deleteNamespacedSecret({
|
|
||||||
name: ref.promptSecretName,
|
|
||||||
namespace: ref.promptSecretNamespace,
|
|
||||||
});
|
|
||||||
} catch { /* best-effort */ }
|
|
||||||
}
|
|
||||||
}),
|
|
||||||
).then(() => {
|
|
||||||
process.kill(process.pid, "SIGTERM");
|
|
||||||
});
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user