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 () => {