diff --git a/sdk/typescript/src/multiscan.ts b/sdk/typescript/src/multiscan.ts index 4795bcfd..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); @@ -158,6 +166,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 }); @@ -197,9 +206,27 @@ 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, 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) => + `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({ @@ -208,13 +235,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 429bb793..0509e34f 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"; @@ -398,6 +399,158 @@ 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("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");