diff --git a/src/application/ReferenceSourceImportPolicy.ts b/src/application/ReferenceSourceImportPolicy.ts index 30e683e0c..ca314bbd8 100644 --- a/src/application/ReferenceSourceImportPolicy.ts +++ b/src/application/ReferenceSourceImportPolicy.ts @@ -108,8 +108,9 @@ const buildIgnored = async ( try { text = await readRegularFileText(policyPath, { signal }); } catch (cause: unknown) { + if (signal?.aborted === true && cause === signal.reason) throw cause; if (!(cause instanceof RegularFileCleanupFailure)) throw cause; - const retry = await retryRegularFileCleanup(cause, resources); + const retry = await retryRegularFileCleanup(cause, resources, signal); if (retry.cleanup !== undefined) { const primary = retry.outcome.kind === "failed" diff --git a/src/application/ReferenceSourceVcsAdapter.ts b/src/application/ReferenceSourceVcsAdapter.ts index ac05ce0a4..c47cd60f3 100644 --- a/src/application/ReferenceSourceVcsAdapter.ts +++ b/src/application/ReferenceSourceVcsAdapter.ts @@ -88,8 +88,9 @@ const resolveSourceHead = async ( try { return await readRegularFile(path, { signal }); } catch (cause: unknown) { + if (signal?.aborted === true && cause === signal.reason) throw cause; if (!(cause instanceof RegularFileCleanupFailure)) throw cause; - const retry = await retryRegularFileCleanup(cause, resources); + const retry = await retryRegularFileCleanup(cause, resources, signal); if ( retry.outcome.kind === "failed" && errorCode(retry.outcome.cause) !== "ENOENT" diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts index ca420a2b7..da4585762 100644 --- a/src/application/RegularFileRead.ts +++ b/src/application/RegularFileRead.ts @@ -43,13 +43,15 @@ export type RegularFileReadOutcome = RegularFileOperationOutcome; export const retryRegularFileCleanup = async ( failure: RegularFileCleanupFailure, resources: ArtifactResourceScope, + signal?: AbortSignal, ): Promise<{ readonly outcome: RegularFileReadOutcome; readonly cleanup?: AnalysisCleanupObservation; }> => { const outcome: RegularFileReadOutcome = failure.outcome.kind === "failed" && - failure.outcome.cause instanceof RegularFileOperationCancelled + failure.outcome.cause instanceof RegularFileOperationCancelled && + !(signal?.aborted === true && failure.outcome.cause === signal.reason) ? { kind: "failed", cause: failure.outcome.cause.reason } : failure.outcome; const released = await resources.release({ @@ -106,6 +108,9 @@ export const withRegularFile = async ( }, ); } catch (cause: unknown) { + // Caller reasons are opaque, including objects shaped like file failures. + // They do not transfer ownership of any descriptor to this operation. + if (signal?.aborted === true && cause === signal.reason) throw cause; if (cause instanceof RegularFileOperationCancelled) throw cause.reason; if ( cause instanceof RegularFileCancelled && @@ -117,7 +122,7 @@ export const withRegularFile = async ( resources === undefined ) throw cause; - const retry = await retryRegularFileCleanup(cause, resources); + const retry = await retryRegularFileCleanup(cause, resources, signal); if (retry.cleanup !== undefined) throw cause; if (retry.outcome.kind === "failed") throw retry.outcome.cause; return retry.outcome.value; diff --git a/src/application/WebScriptExportService.ts b/src/application/WebScriptExportService.ts index a792706c2..c53c69802 100644 --- a/src/application/WebScriptExportService.ts +++ b/src/application/WebScriptExportService.ts @@ -236,8 +236,10 @@ const readCapture = async ( try { return ok({ bytes: await readRegularFile(path, { signal }) }); } catch (cause: unknown) { + if (signal?.aborted === true && cause === signal.reason) + return err(captureReadFailure(path, cause, signal)); if (cause instanceof RegularFileCleanupFailure) { - const retry = await retryRegularFileCleanup(cause, resources); + const retry = await retryRegularFileCleanup(cause, resources, signal); const cleanup = { reason: retry.cleanup?.reason ?? errorMessage(cause.cleanupCause), incomplete: retry.cleanup !== undefined, diff --git a/tests/boundary/filesystem/regularFileCancellation.test.ts b/tests/boundary/filesystem/regularFileCancellation.test.ts index 456714c03..265d2f3f6 100644 --- a/tests/boundary/filesystem/regularFileCancellation.test.ts +++ b/tests/boundary/filesystem/regularFileCancellation.test.ts @@ -8,6 +8,7 @@ import { retryRegularFileCleanup, } from "../../../src/application/RegularFileRead.js"; import { ArtifactResourceScope } from "../../../src/artifacts/ArtifactResourceScope.js"; +import { RegularFileOperationCancelled } from "../../../src/filesystem/RegularFile.js"; import { OwnedFileHandle } from "../../../src/filesystem/OwnedFileHandle.js"; import { RegularFileCleanupFailure, @@ -16,34 +17,76 @@ import { } from "../../../src/filesystem/OwnedRegularFile.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; -it("keeps a caller-selected cancellation reason opaque", async () => { - const root = await createTestTempDirectory("rea-file-caller-reason-"); - const path = join(root, "input.txt"); - await writeFile(path, "fixture bytes"); - const callerOwner = new OwnedFileHandle(await open(path, "r")); - await callerOwner.close(); - const reason = new RegularFileCancelled( - "/caller-selected-path", - callerOwner, - { - outcome: { kind: "completed", value: "caller-owned value" }, - reason: new Error("nested reason"), - }, - ); - const controller = new AbortController(); - controller.abort(reason); - const resources = new ArtifactResourceScope(); - const read = vi.fn(async () => "unexpected"); - try { - await expect( - withRegularFile(path, read, { signal: controller.signal, resources }), - ).rejects.toBe(reason); - expect(read).not.toHaveBeenCalled(); - expect(resources.hasPending).toBe(false); - } finally { - await resources.close(); - } -}); +it.each( + (["checkpoint", "completion", "cleanup"] as const).flatMap((kind) => + ["before admission", "during read"].flatMap((stage) => + [ + { scoped: false, retryClose: false }, + { scoped: true, retryClose: false }, + ...(stage === "during read" + ? [{ scoped: true, retryClose: true }] + : []), + ].map((scenario) => ({ kind, stage, ...scenario })), + ), + ), +)( + "keeps a $kind caller reason opaque $stage with scope=$scoped, close retry=$retryClose", + async ({ kind, stage, scoped, retryClose }) => { + const root = await createTestTempDirectory("rea-file-caller-reason-"); + const path = join(root, "input.txt"); + await writeFile(path, "fixture bytes"); + const callerOwner = new OwnedFileHandle(await open(path, "r")); + const close = vi.spyOn(callerOwner, "close"); + const nestedReason = new Error("nested reason"); + const outcome = { kind: "completed", value: "caller-owned value" } as const; + const reason = + kind === "checkpoint" + ? new RegularFileOperationCancelled(nestedReason) + : kind === "completion" + ? new RegularFileCancelled("/caller-selected-path", callerOwner, { + outcome, + reason: nestedReason, + }) + : new RegularFileCleanupFailure( + "/caller-selected-path", + callerOwner, + outcome, + nestedReason, + ); + const controller = new AbortController(); + if (stage === "before admission") controller.abort(reason); + const resources = scoped ? new ArtifactResourceScope() : undefined; + let admitted: FileHandle | undefined; + const read = vi.fn(async (handle: FileHandle) => { + admitted = handle; + await handle.readFile(); + if (retryClose) + vi.spyOn(handle, "close").mockRejectedValueOnce( + new Error("transient descriptor close failure"), + ); + controller.abort(reason); + controller.signal.throwIfAborted(); + return "unexpected"; + }); + try { + await expect( + withRegularFile(path, read, { + signal: controller.signal, + ...(resources === undefined ? {} : { resources }), + }), + ).rejects.toBe(reason); + expect(read).toHaveBeenCalledTimes(stage === "during read" ? 1 : 0); + if (admitted !== undefined) expect(admitted.fd).toBe(-1); + expect(close).not.toHaveBeenCalled(); + expect(callerOwner.handle.fd).toBeGreaterThanOrEqual(0); + expect(resources?.hasPending ?? false).toBe(false); + } finally { + await resources?.close(); + close.mockRestore(); + await callerOwner.close(); + } + }, +); it.each([false, true])( "preserves a nested read cancellation with the same signal=%s", @@ -236,7 +279,12 @@ it.each(["completed", "failed"] as const)( ); it.each( - [null, "caller stopped", new Error("caller stopped")].flatMap((reason) => + [ + null, + "caller stopped", + new Error("caller stopped"), + new RegularFileOperationCancelled(new Error("nested reason")), + ].flatMap((reason) => [false, true].map((retryFails) => ({ reason, retryFails })), ), )(