From 62ccadef02dd52ed3ec5d4ae81ae81ec83a779eb Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Mon, 3 Aug 2026 00:40:31 -0600 Subject: [PATCH 1/2] fix(multiscan): keep scan outcomes when checkout cleanup fails The per-repository worker awaited checkout removal in a `finally` that ran before the attempt receipt was appended: } catch (error) { if (options.signal?.aborted === true) options.signal.throwIfAborted(); failure = redactedErrorMessage(error); } finally { await rm(checkout, { recursive: true, force: true }); } const status = failure === undefined ? "completed" : "failed"; await appendReceipt(...); `force: true` ignores a checkout that is already gone, but it does not suppress EACCES, EPERM or EBUSY. When the removal rejected, its filesystem error replaced the outcome the worker had just captured and execution never reached `appendReceipt`, so the real scan failure was lost, the campaign surfaced a confusing removal error instead, and the attempt was left unrecorded for resume. Capture the removal failure rather than throwing it. The attempt keeps the status its scan earned, always gets a receipt, and reports the removal failure alongside any scan failure so a leftover checkout is not silently dropped. The next attempt removes the checkout again inside the try block, so a leftover that outlives this run fails there rather than being scanned as if it were fresh. Cancellation is unaffected: the abort check stays in the catch, ahead of the cleanup. Fixes #211 --- sdk/typescript/src/multiscan.ts | 24 ++++- sdk/typescript/tests-ts/multiscan.test.ts | 113 +++++++++++++++++++++- 2 files changed, 133 insertions(+), 4 deletions(-) diff --git a/sdk/typescript/src/multiscan.ts b/sdk/typescript/src/multiscan.ts index 534715a6..914b2060 100644 --- a/sdk/typescript/src/multiscan.ts +++ b/sdk/typescript/src/multiscan.ts @@ -157,6 +157,7 @@ async function runCampaign( const progress = { repository: task.id, attempt }; options.onProgress?.({ ...progress, status: "started" }); let failure: string | undefined; + let cleanup: string | undefined; let cost: Readonly | null = null; try { await mkdir(dirname(scanDir), { recursive: true, mode: 0o700 }); @@ -193,9 +194,26 @@ async function runCampaign( if (options.signal?.aborted === true) options.signal.throwIfAborted(); failure = redactedErrorMessage(error); } finally { - await rm(checkout, { recursive: true, force: true }); + // Removing the checkout is best effort. `force` ignores a checkout that + // is already gone but not an EACCES, EPERM, or EBUSY removal, and a + // throw here would replace the outcome the try and catch just captured + // and skip the receipt below, leaving the attempt unrecorded for resume. + // The removal failure travels with that outcome instead, so the attempt + // keeps the status its scan earned and a leftover checkout is still + // reported. The next attempt removes the checkout again inside the try + // above, so a leftover that outlives this run fails there rather than + // being scanned as if it were fresh. + cleanup = await rm(checkout, { recursive: true, force: true }).then( + () => undefined, + (error: unknown) => + `Multiscan checkout cleanup failed: ${redactedErrorMessage(error)}`, + ); } const status = failure === undefined ? "completed" : "failed"; + const reported = [failure, cleanup].filter( + (message): message is string => message !== undefined, + ); + const error = reported.length === 0 ? undefined : reported.join("; "); await appendReceipt( ledger, `${JSON.stringify({ @@ -204,13 +222,13 @@ async function runCampaign( attempt, outputDir: scanDir, ...(cost === null ? {} : { cost }), - ...(failure === undefined ? {} : { error: failure }), + ...(error === undefined ? {} : { error }), })}\n`, ); options.onProgress?.({ ...progress, status, - ...(failure === undefined ? {} : { error: failure }), + ...(error === undefined ? {} : { error }), }); if (failure === undefined) { completed += 1; diff --git a/sdk/typescript/tests-ts/multiscan.test.ts b/sdk/typescript/tests-ts/multiscan.test.ts index f357fac9..dcb374b7 100644 --- a/sdk/typescript/tests-ts/multiscan.test.ts +++ b/sdk/typescript/tests-ts/multiscan.test.ts @@ -10,9 +10,10 @@ import { symlink, writeFile, } from "node:fs/promises"; +import * as fsPromises from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { afterEach, describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, mock, test } from "bun:test"; import type { ScanResult } from "../src/result.js"; import { buildGitHubCredentialArgs, runMultiscan } from "../src/multiscan.js"; import { resolveTrustedExecutable } from "../src/trusted-executable.js"; @@ -392,6 +393,116 @@ describe("multiscan", () => { expect(await results(summary.resultsPath)).toHaveLength(3); }); + // A checkout removal that fails once the scan is over used to escape the + // worker's finally, replacing the outcome the scan had already earned and + // skipping its receipt, so the attempt was never recorded for resume. + async function unremovableCheckout( + checkout: string, + scanned: () => boolean, + ): Promise<() => void> { + const originalRm = fsPromises.rm; + mock.module("node:fs/promises", () => ({ + ...fsPromises, + rm: async (...args: Parameters) => { + if (scanned() && String(args[0]) === checkout) { + throw Object.assign( + new Error(`EACCES: permission denied, rm '${checkout}'`), + { code: "EACCES" }, + ); + } + return await originalRm(...args); + }, + })); + return () => { + mock.module("node:fs/promises", () => ({ + ...fsPromises, + rm: originalRm, + })); + }; + } + + test("keeps a failed scan's outcome when its checkout cannot be removed", async () => { + const paths = await fixture(); + const source = await repository(paths.root, "stubborn"); + await writeFile( + paths.input, + `id,repository,revision\nstubborn,${source.path},${source.revision}\n`, + ); + let scanned = false; + const restore = await unremovableCheckout( + join(paths.output, "checkouts", "stubborn"), + () => scanned, + ); + + try { + const summary = await runMultiscan( + options( + paths, + client(async () => { + scanned = true; + throw new Error("ORIGINAL_SCAN_FAILURE"); + }), + { maxAttempts: 1 }, + ), + ); + + expect(summary).toMatchObject({ total: 1, completed: 0, failed: 1 }); + const receipts = await results(summary.resultsPath); + expect(receipts).toHaveLength(1); + expect(receipts[0]).toMatchObject({ status: "failed" }); + const error = String(receipts[0]!["error"]); + expect(error).toContain("ORIGINAL_SCAN_FAILURE"); + expect(error).toContain("Multiscan checkout cleanup failed"); + } finally { + restore(); + } + }); + + test("keeps a completed scan's outcome when its checkout cannot be removed", async () => { + const paths = await fixture(); + const source = await repository(paths.root, "stubborn"); + await writeFile( + paths.input, + `id,repository,revision\nstubborn,${source.path},${source.revision}\n`, + ); + let scanned = false; + const reported: string[] = []; + const restore = await unremovableCheckout( + join(paths.output, "checkouts", "stubborn"), + () => scanned, + ); + + try { + const summary = await runMultiscan( + options( + paths, + client(async (_repository, scanOptions = {}) => { + scanned = true; + return await completedScan(scanOptions.outputDir!); + }), + { + maxAttempts: 1, + onProgress: ({ status, error }) => { + if (error !== undefined) reported.push(`${status}: ${error}`); + }, + }, + ), + ); + + expect(summary).toMatchObject({ total: 1, completed: 1, failed: 0 }); + const receipts = await results(summary.resultsPath); + expect(receipts).toHaveLength(1); + expect(receipts[0]).toMatchObject({ status: "completed" }); + expect(String(receipts[0]!["error"])).toContain( + "Multiscan checkout cleanup failed", + ); + expect(reported).toHaveLength(1); + expect(reported[0]).toStartWith("completed: "); + } finally { + restore(); + } + }); + test("rejects another supervisor and recovers a crashed owner's checkout", async () => { const paths = await fixture(); const source = await repository(paths.root, "exclusive"); From 407cc99f2b4e8b42a41407c0054086a4f99e8753 Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Mon, 3 Aug 2026 17:57:36 -0600 Subject: [PATCH 2/2] fix(multiscan): retry a leftover checkout before skipping a completed task Keeping the scan outcome when the post-scan removal fails leaves the checkout on disk, and a completed receipt is counted as done during resume before the worker's pre-attempt removal ever runs. The task is never queued again, so the clone survived every later campaign. Retry the removal on the resume path, still best effort, for the tasks that are counted as already completed. The receipt and its recorded cleanup failure are untouched, so a completed or failed scan still keeps the status it earned, and the clone it left behind is reclaimed by the next run instead of lingering indefinitely. --- sdk/typescript/src/multiscan.ts | 13 +++++-- sdk/typescript/tests-ts/multiscan.test.ts | 42 +++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/sdk/typescript/src/multiscan.ts b/sdk/typescript/src/multiscan.ts index de7eba09..5be1381b 100644 --- a/sdk/typescript/src/multiscan.ts +++ b/sdk/typescript/src/multiscan.ts @@ -119,6 +119,14 @@ async function runCampaign( join(output, "artifacts", task.id, `attempt-${receipt.attempt}`) && (await hasArtifacts(receipt.outputDir)) ) { + // A skipped task never reaches the worker's removal below, so a checkout + // that survived a failed cleanup would linger for every later run. Retry + // the removal here, still best effort and without touching the receipt + // this task already earned, so resuming reclaims the clone it left. + await rm(join(output, "checkouts", task.id), { + recursive: true, + force: true, + }).catch(() => undefined); completed += 1; } else { pending.push(task); @@ -205,8 +213,9 @@ async function runCampaign( // The removal failure travels with that outcome instead, so the attempt // keeps the status its scan earned and a leftover checkout is still // reported. The next attempt removes the checkout again inside the try - // above, so a leftover that outlives this run fails there rather than - // being scanned as if it were fresh. + // above, and a resume that skips this task removes it before counting + // the task as done, so a leftover that outlives this run is retried + // rather than kept or scanned as if it were fresh. cleanup = await rm(checkout, { recursive: true, force: true }).then( () => undefined, (error: unknown) => diff --git a/sdk/typescript/tests-ts/multiscan.test.ts b/sdk/typescript/tests-ts/multiscan.test.ts index 730f9455..0509e34f 100644 --- a/sdk/typescript/tests-ts/multiscan.test.ts +++ b/sdk/typescript/tests-ts/multiscan.test.ts @@ -509,6 +509,48 @@ describe("multiscan", () => { } }); + test("removes a checkout left behind by a completed scan's failed cleanup", async () => { + const paths = await fixture(); + const source = await repository(paths.root, "leftover"); + await writeFile( + paths.input, + `id,repository,revision\nleftover,${source.path},${source.revision}\n`, + ); + let scanned = false; + let scans = 0; + const security = client(async (_repository, scanOptions = {}) => { + scans += 1; + scanned = true; + return await completedScan(scanOptions.outputDir!); + }); + const restore = await unremovableCheckout( + join(paths.output, "checkouts", "leftover"), + () => scanned, + ); + let first: Awaited>; + try { + first = await runMultiscan(options(paths, security, { maxAttempts: 1 })); + } finally { + restore(); + } + + expect(first).toMatchObject({ total: 1, completed: 1, failed: 0 }); + expect(await readdir(join(paths.output, "checkouts"))).toEqual([ + "leftover", + ]); + + const resumed = await runMultiscan( + options(paths, security, { maxAttempts: 1 }), + ); + + expect(resumed).toMatchObject({ total: 1, completed: 1, skipped: 1 }); + expect(scans).toBe(1); + expect(await readdir(join(paths.output, "checkouts"))).toEqual([]); + expect(await results(first.resultsPath)).toMatchObject([ + { id: "leftover", status: "completed", attempt: 1 }, + ]); + }); + test("rejects another supervisor and recovers a crashed owner's checkout", async () => { const paths = await fixture(); const source = await repository(paths.root, "exclusive");