fix(multiscan): keep scan outcomes when checkout cleanup fails

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<ScanCost> | 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;

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<typeof originalRm>) => {
+        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<ReturnType<typeof runMultiscan>>;
+    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");