From e8d1bbc98b2ef73f8e47e5694d250f493045c5e1 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 9 Nov 2026 19:42:00 +0800 Subject: [PATCH] fix(reference): preserve import cancellation provenance (#9597) * fix(reference): preserve import cancellation reason * test(reference): cover null cancellation during failed cleanup * test(reference): always release fixture directory --- src/application/ReferenceSourceImport.ts | 11 ++-- .../ReferenceSourceImportPolicy.ts | 26 ++++++++-- src/reference/ReferenceSourceReader.ts | 12 ++--- src/reference/ReferenceSourceReaderEntries.ts | 32 ++++++++---- src/reference/ReferenceSourceReaderErrors.ts | 8 ++- .../ReferenceSourceReaderValidate.ts | 6 +-- .../referenceSourceCleanupOwnership.test.ts | 50 +++++++++++++++++++ .../filesystem/referenceSourceImport.test.ts | 12 +++++ .../filesystem/referenceSourceReader.test.ts | 23 +++++++++ 9 files changed, 149 insertions(+), 31 deletions(-) diff --git a/src/application/ReferenceSourceImport.ts b/src/application/ReferenceSourceImport.ts index db90752bc0..9ff57563ec 100644 --- a/src/application/ReferenceSourceImport.ts +++ b/src/application/ReferenceSourceImport.ts @@ -39,9 +39,11 @@ const failure = ( }); const cancelled = ( + signal: AbortSignal | undefined, partial?: ReferenceSourceRead, ): ReferenceSourceImportError => ({ ...failure("cancelled", "Reference source import cancelled"), + ...(signal?.aborted === true ? { cause: signal.reason } : {}), ...(partial === undefined ? {} : { partial }), }); @@ -168,11 +170,11 @@ export const importReferenceSource = async ( options: ReferenceSourceImportOptions, resources: ArtifactResourceScope, ): Promise> => { - if (isAborted(options.signal)) return err(cancelled()); + if (isAborted(options.signal)) return err(cancelled(options.signal)); const prepared = await prepareReferenceSourceImport(options, resources); if (!prepared.ok) return prepared; const { ignored, root, secrets } = prepared.value; - if (isAborted(options.signal)) return err(cancelled()); + if (isAborted(options.signal)) return err(cancelled(options.signal)); const exclusions: HistoricalSourceGraphInput["exclusions"] = []; const shouldExclude = createShouldExclude(exclusions, secrets, ignored); @@ -201,13 +203,14 @@ export const importReferenceSource = async ( const vcs = vcsResult.value; - if (isAborted(options.signal)) return err(cancelled(readResult.value)); + if (isAborted(options.signal)) + return err(cancelled(options.signal, readResult.value)); const read = readResult.value; const { entries, relationships, parseFailures, limitations } = parseReferenceSourceEntries(read, exclusions, options.signal); - if (isAborted(options.signal)) return err(cancelled(read)); + if (isAborted(options.signal)) return err(cancelled(options.signal, read)); const uniqueRelationships = deduplicateRelationships(relationships); const uniqueFailures = normalizeHistoricalSourceParseFailures(parseFailures); diff --git a/src/application/ReferenceSourceImportPolicy.ts b/src/application/ReferenceSourceImportPolicy.ts index e9ceeaa97e..222356c934 100644 --- a/src/application/ReferenceSourceImportPolicy.ts +++ b/src/application/ReferenceSourceImportPolicy.ts @@ -80,11 +80,17 @@ const resolveRoot = async ( signal?: AbortSignal, ): Promise> => { if (isAborted(signal)) - return err(failure("cancelled", "Reference source import cancelled")); + return err({ + ...failure("cancelled", "Reference source import cancelled"), + cause: signal?.reason, + }); try { const metadata = await stat(requestedRoot); if (isAborted(signal)) - return err(failure("cancelled", "Reference source import cancelled")); + return err({ + ...failure("cancelled", "Reference source import cancelled"), + cause: signal?.reason, + }); if (!metadata.isDirectory()) return err( failure( @@ -94,11 +100,17 @@ const resolveRoot = async ( ); const canonicalRoot = await realpath(requestedRoot); if (isAborted(signal)) - return err(failure("cancelled", "Reference source import cancelled")); + return err({ + ...failure("cancelled", "Reference source import cancelled"), + cause: signal?.reason, + }); return ok(canonicalRoot); } catch (cause: unknown) { if (isAborted(signal)) - return err(failure("cancelled", "Reference source import cancelled")); + return err({ + ...failure("cancelled", "Reference source import cancelled"), + cause: signal?.reason, + }); const rootFailure = rootFilesystemFailure( cause, "Reference source root could not be resolved", @@ -242,7 +254,11 @@ const readIgnoreCase = async ( } catch (cause: unknown) { if (readFailure !== undefined) return err(readFailure); if (isAborted(signal) || isIgnorePolicyCancellation(cause)) - return err(failure("cancelled", "Reference source import cancelled")); + return err({ + ...failure("cancelled", "Reference source import cancelled"), + cause: + signal?.aborted === true ? signal.reason : ignorePolicyCause(cause), + }); return err({ ...failure( "io", diff --git a/src/reference/ReferenceSourceReader.ts b/src/reference/ReferenceSourceReader.ts index 3d6577e4d1..01d18a3357 100644 --- a/src/reference/ReferenceSourceReader.ts +++ b/src/reference/ReferenceSourceReader.ts @@ -19,7 +19,10 @@ import { noFollowOpenSupported, prepareRoot, } from "./ReferenceSourceReaderValidate.js"; -import { referenceSourceResourceFailure } from "./ReferenceSourceReaderErrors.js"; +import { + cancelled, + referenceSourceResourceFailure, +} from "./ReferenceSourceReaderErrors.js"; const PATH_RACE_LIMITATION = "Path identity is revalidated around operations; Node lacks portable descriptor-relative openat traversal, so a syscall-boundary pathname race remains."; @@ -199,12 +202,7 @@ const traverse = async ( state: TraversalState, ): Promise> => { while (state.pending.length > 0) { - if (isAborted(state.signal)) - return err({ - tag: "reference-source-reader", - code: "cancelled", - message: "Reference source traversal cancelled", - }); + if (isAborted(state.signal)) return err(cancelled(state.signal)); const current = state.pending.pop(); if (current === undefined) break; const result = await traverseDirectory(state, current); diff --git a/src/reference/ReferenceSourceReaderEntries.ts b/src/reference/ReferenceSourceReaderEntries.ts index 7e1f2fc3ae..163268f63c 100644 --- a/src/reference/ReferenceSourceReaderEntries.ts +++ b/src/reference/ReferenceSourceReaderEntries.ts @@ -69,7 +69,8 @@ export const traverseDirectory = async ( current.path, state.signal, ); - if (isAborted(state.signal)) return { ok: false, error: cancelled() }; + if (isAborted(state.signal)) + return { ok: false, error: cancelled(state.signal) }; if (!before.ok) { recordDirectoryFailure( state, @@ -103,11 +104,12 @@ export const traverseDirectory = async ( // A directory that cannot be opened retains its ordinary partial-entry // failure; only an opened directory needs policy before enumeration. result = isAborted(state.signal) - ? err(cancelled()) + ? err(cancelled(state.signal)) : ((await state.beforeDirectory?.( pathFromRoot(state.root, current.path), )) ?? ok(undefined)); - if (result.ok && isAborted(state.signal)) result = err(cancelled()); + if (result.ok && isAborted(state.signal)) + result = err(cancelled(state.signal)); while (result.ok) { let child: Dirent | null; try { @@ -154,7 +156,7 @@ export const traverseDirectory = async ( const previous = result.ok ? undefined : result.error.cleanup; const primary = result.ok ? isAborted(state.signal) - ? cancelled() + ? cancelled(state.signal) : failure( "io", readFailure?.localDiagnostic ?? @@ -173,12 +175,13 @@ export const traverseDirectory = async ( ...new Set([...previous.resources, ...observation.resources]), ], }, - cause: primary.cause ?? cleanup.cause, + cause: Object.hasOwn(primary, "cause") ? primary.cause : cleanup.cause, }); } if (!result.ok) return result; } - if (isAborted(state.signal)) return { ok: false, error: cancelled() }; + if (isAborted(state.signal)) + return { ok: false, error: cancelled(state.signal) }; if (readFailure !== undefined) { recordDirectoryFailure( state, @@ -196,7 +199,8 @@ export const traverseDirectory = async ( current.path, state.signal, ); - if (isAborted(state.signal)) return { ok: false, error: cancelled() }; + if (isAborted(state.signal)) + return { ok: false, error: cancelled(state.signal) }; if (!after.ok || !sameFile(before.stats, after.stats)) { recordDirectoryFailure( state, @@ -263,11 +267,13 @@ const processEntry = async ( name: string, directories: PendingDirectory[], ): Promise> => { - if (isAborted(state.signal)) return { ok: false, error: cancelled() }; + if (isAborted(state.signal)) + return { ok: false, error: cancelled(state.signal) }; const absolute = join(current.path, name); const path = pathFromRoot(state.root, absolute); const metadata = await readMetadata(absolute); - if (isAborted(state.signal)) return { ok: false, error: cancelled() }; + if (isAborted(state.signal)) + return { ok: false, error: cancelled(state.signal) }; if (!metadata.ok) { const excluded = applyExclusion(state.shouldExclude, path, "unknown"); if (!excluded.ok) @@ -312,7 +318,8 @@ const processEntry = async ( state.exclusions.push({ path, kind }); return { ok: true, value: undefined }; } - if (isAborted(state.signal)) return { ok: false, error: cancelled() }; + if (isAborted(state.signal)) + return { ok: false, error: cancelled(state.signal) }; if (kind === "symlink") state.entries.push( await readStableSymlinkTarget( @@ -370,6 +377,11 @@ const processFileEntry = async ( ? entry.message : `Reference file cleanup failed: ${path}`, cleanup: result.cleanup, + ...(entry.status === "failed" && + entry.code === "cancelled" && + state.signal?.aborted === true + ? { cause: state.signal.reason } + : {}), }); return ok(undefined); }; diff --git a/src/reference/ReferenceSourceReaderErrors.ts b/src/reference/ReferenceSourceReaderErrors.ts index 5f3515ca8a..14856e1c15 100644 --- a/src/reference/ReferenceSourceReaderErrors.ts +++ b/src/reference/ReferenceSourceReaderErrors.ts @@ -14,8 +14,12 @@ export const failure = ( message, }); -export const cancelled = (): ReferenceSourceReaderError => - failure("cancelled", "Reference source traversal cancelled"); +export const cancelled = ( + signal?: AbortSignal, +): ReferenceSourceReaderError => ({ + ...failure("cancelled", "Reference source traversal cancelled"), + ...(signal?.aborted === true ? { cause: signal.reason } : {}), +}); /** Project resource failures into the reference-source error contract. */ export const referenceSourceResourceFailure = ( diff --git a/src/reference/ReferenceSourceReaderValidate.ts b/src/reference/ReferenceSourceReaderValidate.ts index 0d95aab0c9..f69fb75081 100644 --- a/src/reference/ReferenceSourceReaderValidate.ts +++ b/src/reference/ReferenceSourceReaderValidate.ts @@ -99,7 +99,7 @@ export const prepareRoot = async ( readonly rootIdentity: BigIntStats; }> > => { - if (isAborted(signal)) return err(cancelled()); + if (isAborted(signal)) return err(cancelled(signal)); try { const metadata = await bigLstat(root); if (metadata.isSymbolicLink() || !metadata.isDirectory()) @@ -110,7 +110,7 @@ export const prepareRoot = async ( ), ); const canonicalRoot = await realpath(root); - if (isAborted(signal)) return err(cancelled()); + if (isAborted(signal)) return err(cancelled(signal)); const canonicalMetadata = await bigLstat(canonicalRoot); if (!sameFile(metadata, canonicalMetadata)) return err( @@ -121,7 +121,7 @@ export const prepareRoot = async ( ); return ok({ canonicalRoot, rootIdentity: canonicalMetadata }); } catch (cause: unknown) { - if (isAborted(signal)) return err(cancelled()); + if (isAborted(signal)) return err(cancelled(signal)); const rootFailure = rootFilesystemFailure( cause, "Reference source root could not be resolved", diff --git a/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts b/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts index 788c216b47..ea8a7e196e 100644 --- a/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts +++ b/tests/boundary/filesystem/referenceSourceCleanupOwnership.test.ts @@ -411,3 +411,53 @@ it("keeps cancelled traversal typed when native directory close rejects after in }); expect(closeCalls).toBe(1); }); + +it("preserves null cancellation and retained directory cleanup across retry", async () => { + const root = await createTestTempDirectory("rea-reference-null-close-"); + const resources = new ArtifactResourceScope(); + onTestFinished(() => resources.close().catch(() => undefined)); + const controller = new AbortController(); + const closeFailure = new Error("directory close temporarily denied"); + let closeCalls = 0; + let releaseFixture: (() => Promise) | undefined; + onTestFinished(() => releaseFixture?.()); + opendirMock.mockImplementation(async (...args) => { + const directory = await actual.opendir(...args); + const operations: DirectoryOperations = directory; + const close = operations.close.bind(directory); + releaseFixture = close; + vi.spyOn(operations, "close").mockImplementation(async () => { + closeCalls += 1; + if (closeCalls === 1) throw closeFailure; + await close(); + }); + return directory; + }); + const result = await readReferenceSource(root, resources, { + signal: controller.signal, + beforeDirectory: async () => { + controller.abort(null); + return ok(undefined); + }, + }); + if (result.ok) throw new Error("Expected cancellation and cleanup failure"); + expect(result.error.code).toBe("cancelled"); + expect(result.error.cause).toBeNull(); + expect(result.error.cleanup).toEqual({ + resources: [root], + reason: expect.stringContaining(closeFailure.message), + }); + expect(result.error.partial).toMatchObject({ entries: [], bytesRead: 0 }); + expect(resources.hasPending).toBe(true); + expect(resources.cleanupResources).toEqual([root]); + expect(closeCalls).toBe(1); + await expect(resources.close()).rejects.toMatchObject({ + cleanup: { + resources: [root], + reason: expect.stringContaining(closeFailure.message), + }, + }); + expect(resources.hasPending).toBe(true); + expect(resources.cleanupResources).toEqual([root]); + expect(closeCalls).toBe(1); +}); diff --git a/tests/boundary/filesystem/referenceSourceImport.test.ts b/tests/boundary/filesystem/referenceSourceImport.test.ts index 477606dd94..1eba01b6f6 100644 --- a/tests/boundary/filesystem/referenceSourceImport.test.ts +++ b/tests/boundary/filesystem/referenceSourceImport.test.ts @@ -801,6 +801,18 @@ describe("reference source import behavior", () => { } }); + it.each([null, "stop import", { requested: "stop import" }])( + "preserves the caller's pre-admission cancellation reason %j", + async (reason) => { + const controller = new AbortController(); + controller.abort(reason); + const result = await importTree("unopened-source", controller.signal); + if (result.ok) throw new Error("Expected cancellation"); + expect(result.error.code).toBe("cancelled"); + expect(result.error.cause).toBe(reason); + }, + ); + it("records bounded local Git state without invoking Git", async () => { const parent = await createTestTempDirectory("rea-reference-git-"); try { diff --git a/tests/boundary/filesystem/referenceSourceReader.test.ts b/tests/boundary/filesystem/referenceSourceReader.test.ts index c4a805585b..fccd4d9220 100644 --- a/tests/boundary/filesystem/referenceSourceReader.test.ts +++ b/tests/boundary/filesystem/referenceSourceReader.test.ts @@ -7,6 +7,28 @@ import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { readStableSymlinkTarget } from "../../../src/reference/ReferenceSourceReaderEntries.js"; import { readReferenceSource } from "../../support/referenceSourceResourceScope.js"; +import { ok } from "../../../src/domain/result.js"; + +it("retains the caller reason and observed entries when traversal is cancelled", async () => { + const root = await createTestTempDirectory("rea-reference-cancel-"); + await writeFile(join(root, "observed.js"), "observed"); + await mkdir(join(root, "nested")); + const controller = new AbortController(); + const reason = { requested: "stop traversal" }; + const result = await readReferenceSource(root, { + signal: controller.signal, + beforeDirectory: async (path) => { + if (path === "nested") controller.abort(reason); + return ok(undefined); + }, + }); + if (result.ok) throw new Error("Expected cancellation"); + expect(result.error.code).toBe("cancelled"); + expect(result.error.cause).toBe(reason); + expect(result.error.partial?.entries).toContainEqual( + expect.objectContaining({ path: "observed.js", status: "read" }), + ); +}); describe("readReferenceSource entries", () => { it("returns explicit entries in canonical code-point path order", async () => { @@ -231,6 +253,7 @@ describe("readReferenceSource failures and exclusions", () => { tag: "reference-source-reader", code: "cancelled", message: "Reference source traversal cancelled", + cause: controller.signal.reason, }, });