From a721c42c07dc674c32ab939627355b8a1bee97a5 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Thu, 22 Oct 2026 04:17:14 +0800 Subject: [PATCH] fix(cancellation): bind retained errors to cleanup owners Carry the capture handle cleanup lease through retry results and bind composed export failures to it. Bind aborted binary retirement errors to the failed close so successful later cleanup reveals the original cancellation. Validation: 46 focused tests passed; npm run check passed. Four regression assertions fail on the original source. Full suite and real-provider workflows were not run. --- src/application/RegularFileRead.ts | 3 + src/application/WebScriptExportService.ts | 16 +++- src/application/binary/BinarySession.ts | 2 +- .../filesystem/webScriptExport.test.ts | 81 ++++++++++++------- .../binarySessionCleanupRetry.test.ts | 17 +++- 5 files changed, 88 insertions(+), 31 deletions(-) diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts index 31fc6bb432..cd4c4db1af 100644 --- a/src/application/RegularFileRead.ts +++ b/src/application/RegularFileRead.ts @@ -16,6 +16,7 @@ import { type RegularFileOperationOutcome, } from "../filesystem/OwnedRegularFile.js"; import { readBoundedFileBytes } from "../process/BoundedFileBytes.js"; +import type { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import type { AnalysisCleanupObservation } from "../domain/analysisErrorBase.js"; @@ -62,6 +63,7 @@ export const retryRegularFileCleanup = async ( signal?: AbortSignal, ): Promise<{ readonly outcome: RegularFileReadOutcome; + readonly cleanupLease?: AnalysisCleanupLease; readonly cleanup?: AnalysisCleanupObservation; }> => { const outcome: RegularFileReadOutcome = @@ -79,6 +81,7 @@ export const retryRegularFileCleanup = async ( if (released.kind === "released") return { outcome }; return { outcome, + cleanupLease: released.lease, cleanup: { reason: `${failureMessage(failure.cleanupCause)}; retry: ${failureMessage(released.cause)}`, resources: [failure.path], diff --git a/src/application/WebScriptExportService.ts b/src/application/WebScriptExportService.ts index df9201166a..67beeb213c 100644 --- a/src/application/WebScriptExportService.ts +++ b/src/application/WebScriptExportService.ts @@ -4,6 +4,10 @@ import { isAbsolute } from "node:path"; import { z } from "zod"; import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; +import { + AnalysisCleanupLeaseGroup, + type AnalysisCleanupLease, +} from "../artifacts/AnalysisCleanupLease.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { publishWebScripts } from "../browser/assets/PublishWebScripts.js"; import { selectScriptCapture } from "../browser/assets/ScriptCaptureAdapters.js"; @@ -48,6 +52,7 @@ interface CaptureRead { readonly reason: string; readonly incomplete: boolean; readonly cause: unknown; + readonly lease?: AnalysisCleanupLease; }; } @@ -259,6 +264,9 @@ const readCapture = async ( reason: retry.cleanup?.reason ?? errorMessage(cause.cleanupCause), incomplete: retry.cleanup !== undefined, cause: cause.cleanupCause, + ...(retry.cleanupLease === undefined + ? {} + : { lease: retry.cleanupLease }), }; if (retry.outcome.kind === "completed") { const cancellation = @@ -353,8 +361,8 @@ const incompleteCaptureCleanup = ( primary: AnalysisError, path: string, cleanup: NonNullable, -): AnalysisError => - analysisErrorWithCleanupFailure( +): AnalysisError => { + const error = analysisErrorWithCleanupFailure( primary, new ProviderCleanupError( PROVIDER.id, @@ -364,6 +372,10 @@ const incompleteCaptureCleanup = ( ), OPERATION, ); + if (cleanup.lease !== undefined) + new AnalysisCleanupLeaseGroup(error, [cleanup.lease]); + return error; +}; const errorMessage = (cause: unknown): string => cause instanceof Error ? cause.message : String(cause); diff --git a/src/application/binary/BinarySession.ts b/src/application/binary/BinarySession.ts index 3ea582b4d1..0ec43983f4 100644 --- a/src/application/binary/BinarySession.ts +++ b/src/application/binary/BinarySession.ts @@ -597,7 +597,7 @@ export class BinarySession }), closed.error, "open_binary", - ), + ).bindCleanupDependencies([closed.error]), ); return closed; } diff --git a/tests/boundary/filesystem/webScriptExport.test.ts b/tests/boundary/filesystem/webScriptExport.test.ts index dc39a5672f..45e178b76b 100644 --- a/tests/boundary/filesystem/webScriptExport.test.ts +++ b/tests/boundary/filesystem/webScriptExport.test.ts @@ -628,34 +628,52 @@ describe("captured script final commit cancellation", () => { } }); - it("returns cancellation with the caller reason when a retried capture close recovers", async () => { - const { input, resources } = await setup(); - const controller = new AbortController(); - const reason = { request: "stop export" }; - let closeCalls = 0; - openMock.mockImplementation(async (...args) => { - const handle = await actualFs.open(...args); - if (String(args[0]) !== input.capture_path) return handle; - const close = handle.close.bind(handle); - vi.spyOn(handle, "close").mockImplementation(async () => { - closeCalls += 1; - if (closeCalls === 1) { - controller.abort(reason); - throw new Error("injected capture close failure"); - } - await close(); + it.each([1, 2])( + "returns cancellation after %i failed capture close attempts recover", + async (failedCloses) => { + const { input, resources } = await setup(); + const controller = new AbortController(); + const reason = { request: "stop export" }; + let closeCalls = 0; + openMock.mockImplementation(async (...args) => { + const handle = await actualFs.open(...args); + if (String(args[0]) !== input.capture_path) return handle; + const close = handle.close.bind(handle); + vi.spyOn(handle, "close").mockImplementation(async () => { + closeCalls += 1; + if (closeCalls <= failedCloses) { + controller.abort(reason); + throw new Error("injected capture close failure"); + } + await close(); + }); + return handle; }); - return handle; - }); - const result = await exportWebScripts(input, resources, { - signal: controller.signal, - }); - if (result.ok) throw new Error("Expected cancellation"); - expect(closeCalls).toBe(2); - expect(result.error._tag).toBe("AnalysisCancelledError"); - expect(result.error.cause).toBe(reason); - expect(resources.hasPending).toBe(false); - }); + const result = await exportWebScripts(input, resources, { + signal: controller.signal, + }); + if (result.ok) throw new Error("Expected cancellation"); + expect(closeCalls).toBe(2); + expect(result.error.cleanupIncomplete).toBe(failedCloses === 2); + expect(resources.hasPending).toBe(failedCloses === 2); + if (failedCloses === 2) { + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cleanup_incomplete", + details: { resources: [input.capture_path] }, + }); + expect(result.error.cleanupPrimary?.cause).toBe(reason); + } else { + expect(result.error.cause).toBe(reason); + } + await resources.close(); + expect(closeCalls).toBe(failedCloses + 1); + expect(resources.hasPending).toBe(false); + expect(result.error.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cancelled", + }); + }, + ); it("identifies a missing capture file and rejected host path syntax", async () => { const { input, resources } = await setup(); @@ -925,6 +943,11 @@ describe("capture read and close failure", () => { await resources.close(); expect(closeCalls).toBe(4); expect(openedHandle.fd).toBe(-1); + expect(result.error.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "artifact_operation_failed", + details: { reason: "io" }, + }); } finally { allowClose = true; await resources.close().catch(() => undefined); @@ -984,6 +1007,10 @@ describe("capture handle ownership", () => { allowClose = true; await resources.close(); expect([...closeCalls.values()]).toEqual([4]); + expect(result.error.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "invalid_request", + }); } finally { allowClose = true; await resources.close().catch(() => undefined); diff --git a/tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts b/tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts index 519a8f7e43..6412922f15 100644 --- a/tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts +++ b/tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts @@ -2,6 +2,10 @@ import { access, mkdir, readFile, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { expect, it, onTestFinished } from "vitest"; +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../../../src/artifacts/AnalysisCleanupLease.js"; import type { AnalysisClient } from "../../../src/application/AnalysisProvider.js"; import { HopperStartError } from "../../../src/domain/hopperErrors.js"; import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; @@ -33,6 +37,11 @@ const retryFixture = async ( reason: "owned runtime removal denied", leftover_paths: [runtime], }); + const cleanupLease = new AnalysisCleanupLease( + () => rm(runtime, { recursive: true, force: true }), + [runtime], + ); + new AnalysisCleanupLeaseGroup(cleanupError, [cleanupLease]); const client: AnalysisClient = { execute, operationHealthSnapshot: () => ({ @@ -45,7 +54,7 @@ const retryFixture = async ( close: async (options) => { onClose?.(); if (!allowed) return err(cleanupError); - await rm(runtime, { recursive: true, force: true }); + await cleanupLease.release(); if (options?.retainDocument !== true) await rm(document, { force: true }); return resultOk(null); }, @@ -243,6 +252,12 @@ it("keeps cancellation as the primary when previous-client retirement fails afte }); fixture.allowCleanup(); expect((await fixture.session.close()).ok).toBe(true); + expect(fixture.cleanupError.cleanupIncomplete).toBe(false); + expect(result.error.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cancelled", + }); + expect(result.error.cleanupPrimary?.cause).toBe(reason); }); it("preserves caller-selected document retention when retrying cleanup", async () => {