diff --git a/src/application/ReferenceSourceGitDirectory.ts b/src/application/ReferenceSourceGitDirectory.ts index 2804c9eabc..7f08564e2e 100644 --- a/src/application/ReferenceSourceGitDirectory.ts +++ b/src/application/ReferenceSourceGitDirectory.ts @@ -9,6 +9,13 @@ export interface ReferenceSourceGitDirectory { readonly commonDirectory: string; } +/** A cancellation observed by the Git directory locator itself. */ +export class ReferenceSourceGitDirectoryCancelled extends Error { + constructor(readonly reason: unknown) { + super("Git directory lookup cancelled", { cause: reason }); + } +} + /** Locate the nearest repository without running Git or following config includes. */ export const locateReferenceSourceGitDirectory = async ( root: string, @@ -17,7 +24,8 @@ export const locateReferenceSourceGitDirectory = async ( ): Promise => { let repository = root; for (;;) { - signal?.throwIfAborted(); + if (signal?.aborted === true) + throw new ReferenceSourceGitDirectoryCancelled(signal.reason); const dotgit = join(repository, ".git"); let metadata; try { diff --git a/src/application/ReferenceSourceImportPolicy.ts b/src/application/ReferenceSourceImportPolicy.ts index f8531c4fa8..f13cf1f75f 100644 --- a/src/application/ReferenceSourceImportPolicy.ts +++ b/src/application/ReferenceSourceImportPolicy.ts @@ -1,7 +1,10 @@ import { lstat, realpath, stat } from "node:fs/promises"; import fs from "node:fs"; import { join } from "node:path"; -import { locateReferenceSourceGitDirectory } from "./ReferenceSourceGitDirectory.js"; +import { + locateReferenceSourceGitDirectory, + ReferenceSourceGitDirectoryCancelled, +} from "./ReferenceSourceGitDirectory.js"; import type { ReferenceSourceReaderError } from "../reference/ReferenceSourceReaderTypes.js"; import ignore from "ignore"; @@ -106,11 +109,6 @@ const resolveRoot = async ( }); return ok(canonicalRoot); } catch (cause: unknown) { - if (signal?.aborted === true && cause === signal.reason) - return err({ - ...failure("cancelled", "Reference source import cancelled"), - cause: signal.reason, - }); const rootFailure = rootFilesystemFailure( cause, "Reference source root could not be resolved", @@ -267,12 +265,14 @@ const readIgnoreCase = async ( if (readFailure !== undefined) return err(readFailure); if ( isIgnorePolicyCancellation(cause) || - (signal?.aborted === true && cause === signal.reason) + cause instanceof ReferenceSourceGitDirectoryCancelled ) return err({ ...failure("cancelled", "Reference source import cancelled"), cause: - signal?.aborted === true ? signal.reason : ignorePolicyCause(cause), + cause instanceof ReferenceSourceGitDirectoryCancelled + ? cause.reason + : ignorePolicyCause(cause), }); return err({ ...failure( diff --git a/src/application/ReferenceSourceImportTypes.test.ts b/src/application/ReferenceSourceImportTypes.test.ts index ea7bc4dcfd..5b95d2b73b 100644 --- a/src/application/ReferenceSourceImportTypes.test.ts +++ b/src/application/ReferenceSourceImportTypes.test.ts @@ -38,14 +38,20 @@ describe("reference-source import error projection", () => { } }); - it("projects a later cancellation without replacing a physical failure", () => { + it("retains a physical failure and exact later cancellation before projection", () => { const reason = { caller: "stop after I/O failure" }; - const projected = projectReferenceSourceImportError({ + const physical = new Error("directory read failed"); + const typed = referenceSourceImportAnalysisError({ tag: "reference-source-import", code: "io", message: "directory read failed", + cause: physical, secondaryCancellation: { cause: reason }, }); + expect(typed.cause).toBe(physical); + expect(typed.secondaryFailures).toHaveLength(1); + expect(typed.secondaryFailures[0]?.cause).toBe(reason); + const projected = projectAnalysisError(typed); expect(projected.category).toBe("execution_failure"); expect(projected.details?.secondary_failures).toEqual([ expect.objectContaining({ category: "cancelled" }), diff --git a/src/application/ReferenceSourceVcsAdapter.ts b/src/application/ReferenceSourceVcsAdapter.ts index 17ea31210c..fd39d79221 100644 --- a/src/application/ReferenceSourceVcsAdapter.ts +++ b/src/application/ReferenceSourceVcsAdapter.ts @@ -1,6 +1,9 @@ import fs from "node:fs"; import { join } from "node:path"; -import { locateReferenceSourceGitDirectory } from "./ReferenceSourceGitDirectory.js"; +import { + locateReferenceSourceGitDirectory, + ReferenceSourceGitDirectoryCancelled, +} from "./ReferenceSourceGitDirectory.js"; import { err, ok, type Result } from "../domain/result.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; @@ -72,9 +75,19 @@ const resolveSourceVcs = async ( const cleanupFailures: ArtifactReaderFailure[] = []; const readMetadataBytes = async (path: string): Promise => { try { - return await readRegularFile(path, { signal }); + return await readRegularFile(path, { + signal, + cancellationFailure: (reason) => + new ArtifactReaderFailure( + "cancelled", + "Git metadata read cancelled", + { + cause: reason, + }, + ), + }); } catch (cause: unknown) { - if (signal?.aborted === true && cause === signal.reason) throw cause; + if (cause instanceof ArtifactReaderFailure) throw cause; if (!(cause instanceof RegularFileCleanupFailure)) { if ( signal?.aborted === true && @@ -92,11 +105,15 @@ const resolveSourceVcs = async ( { cause: physical }, ); } - throw cause; + throw cause instanceof RegularFileReadFailure + ? cause.operationCause + : cause; } + const cancellation = regularFileCancellation(cause); const retry = await retryRegularFileCleanup(cause, resources, signal); if ( retry.outcome.kind === "failed" && + cancellation === undefined && errorCode(retry.outcome.cause) !== "ENOENT" ) failedRead ??= { cause: retry.outcome.cause }; @@ -111,13 +128,10 @@ const resolveSourceVcs = async ( ? retry.outcome.cause : cause.cleanupCause; throw new ArtifactReaderFailure( - regularFileCancellation(cause) !== undefined || - (signal?.aborted === true && primary === signal.reason) - ? "cancelled" - : "io", + cancellation === undefined ? "io" : "cancelled", `Git metadata could not be read safely: ${errorMessage(primary)}`, { - cause: primary, + cause: cancellation === undefined ? primary : cancellation.reason, ...(retry.cleanup === undefined ? {} : { cleanup: retry.cleanup }), }, ); @@ -130,9 +144,7 @@ const resolveSourceVcs = async ( failure.cleanup === undefined ? [] : [failure.cleanup], ); throw new ArtifactReaderFailure( - (primary instanceof ArtifactReaderFailure && - primary.reason === "cancelled") || - (signal?.aborted === true && primary === signal.reason) + primary instanceof ArtifactReaderFailure && primary.reason === "cancelled" ? "cancelled" : "io", `Git metadata could not be read safely: ${errorMessage(primary)}`, @@ -161,9 +173,9 @@ const resolveSourceVcs = async ( ...fs, promises: { ...fs.promises, - // Let isomorphic-git continue from a failed loose ref to packed-refs, - // then report the original read failure after resolution. This preserves - // both observations when packed metadata also fails cleanup. + // Let isomorphic-git finish probing refs, then report original failures. + // A later resolved ref must not discard an earlier physical read or + // retained cleanup failure. readFile: async ( path: string | undefined, options: @@ -209,22 +221,38 @@ const resolveSourceVcs = async ( head = await resolveRef({ fs: referenceFs, dir: root, ...options }); } catch (cause: unknown) { if (cleanupFailures.length > 0) - throwCleanupFailures(failedRead?.cause ?? cause); + throwCleanupFailures(failedRead?.cause ?? cleanupFailures[0] ?? cause); if (failedRead !== undefined) throw failedRead.cause; throw cause; } if (cleanupFailures.length > 0) - throwCleanupFailures(failedRead?.cause ?? cleanupFailures[0]?.cause); + throwCleanupFailures(failedRead?.cause ?? cleanupFailures[0]); signal?.throwIfAborted(); if (failedRead !== undefined) throw failedRead.cause; return head; }; - const locations = await locateReferenceSourceGitDirectory( - root, - readMetadataText, - signal, - ); + let locations; + try { + locations = await locateReferenceSourceGitDirectory( + root, + readMetadataText, + signal, + ); + } catch (cause: unknown) { + if ( + signal?.aborted === true && + !(cause instanceof ReferenceSourceGitDirectoryCancelled) && + !(cause instanceof ArtifactReaderFailure) + ) + throw new ArtifactReaderFailure( + "io", + `Git directory could not be resolved: ${errorMessage(cause)}`, + { cause }, + ); + throw cause; + } if (locations === undefined) return { kind: "none", head: null, dirty: null }; + if (isAborted(signal)) return unknownVcs(); const { gitDirectory, commonDirectory } = locations; if (gitDirectory === commonDirectory) { const head = await readRef({ gitdir: gitDirectory, ref: "HEAD" }); diff --git a/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts b/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts index 3613f52d62..cfc4c9e694 100644 --- a/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts +++ b/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts @@ -1,11 +1,15 @@ import { mkdir, + lstat as fsLstat, open as fsOpen, opendir as fsOpendir, + realpath as fsRealpath, stat as fsStat, writeFile, + type FileHandle, } from "node:fs/promises"; import { join, resolve } from "node:path"; +import { constants } from "node:fs"; import { afterEach, beforeEach, expect, it, onTestFinished, vi } from "vitest"; @@ -19,15 +23,19 @@ vi.mock("node:fs/promises", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, + lstat: vi.fn(actual.lstat), open: vi.fn(actual.open), stat: vi.fn(actual.stat), + realpath: vi.fn(actual.realpath), opendir: vi.fn(actual.opendir), }; }); +const lstatMock = vi.mocked(fsLstat); const openMock = vi.mocked(fsOpen); const opendirMock = vi.mocked(fsOpendir); const statMock = vi.mocked(fsStat); +const realpathMock = vi.mocked(fsRealpath); let actual: typeof import("node:fs/promises"); beforeEach(async () => { @@ -35,15 +43,19 @@ beforeEach(async () => { await vi.importActual( "node:fs/promises", ); + lstatMock.mockImplementation((...args) => actual.lstat(...args)); openMock.mockImplementation((...args) => actual.open(...args)); opendirMock.mockImplementation((...args) => actual.opendir(...args)); statMock.mockImplementation((...args) => actual.stat(...args)); + realpathMock.mockImplementation((...args) => actual.realpath(...args)); }); afterEach(() => { + lstatMock.mockReset(); openMock.mockReset(); opendirMock.mockReset(); statMock.mockReset(); + realpathMock.mockReset(); }); const failCloseUntilAllowed = ( @@ -335,45 +347,65 @@ it("keeps a later packed-ref read error primary over earlier loose-ref cleanup", ); }); -it("preserves a physical root admission failure when cancellation arrives during stat", async () => { - const root = await createTestTempDirectory("rea-reference-root-eio-"); - const resources = new ArtifactResourceScope(); - const controller = new AbortController(); - const reason = { source: "caller" }; - const physical = Object.assign(new Error("root stat failed"), { - code: "EIO", - }); - statMock.mockImplementation(async (...args) => { - if (String(args[0]) !== root) return actual.stat(...args); - controller.abort(reason); - throw physical; - }); - try { - const result = await importReferenceSource( - { - root, - caller: "test", - policy: { secretPatterns: [] }, - signal: controller.signal, - }, - resources, - ); - expect(result).toMatchObject({ - ok: false, - error: { - code: "io", - cause: physical, - secondaryCancellation: { cause: reason }, - }, +it.each([ + ["stat", false], + ["stat", true], + ["realpath", false], + ["realpath", true], +] as const)( + "preserves a physical root %s failure when cancellation arrives (same reason: %s)", + async (stage, sameReason) => { + const root = await createTestTempDirectory("rea-reference-root-eio-"); + const resources = new ArtifactResourceScope(); + const controller = new AbortController(); + const physical = Object.assign(new Error(`root ${stage} failed`), { + code: "EIO", }); - } finally { - await resources.close(); - } -}); + const reason = sameReason ? physical : { source: "caller" }; + const reject = (): never => { + controller.abort(reason); + throw physical; + }; + if (stage === "stat") { + statMock.mockImplementation(async (...args) => { + if (String(args[0]) === root) return reject(); + return actual.stat(...args); + }); + } else { + realpathMock.mockImplementation(async (...args) => { + if (String(args[0]) === root) return reject(); + return actual.realpath(...args); + }); + } + try { + const result = await importReferenceSource( + { + root, + caller: "test", + policy: { secretPatterns: [] }, + signal: controller.signal, + }, + resources, + ); + if (result.ok) + throw new Error("Expected physical root admission failure"); + expect(result.error.code).toBe("io"); + expect(result.error.cause).toBe(physical); + expect(result.error.secondaryCancellation?.cause).toBe(reason); + } finally { + await resources.close(); + } + }, +); -it.each([false, true])( - "retains a physical Git metadata read failure and late cancellation through cleanup (close fails: %s)", - async (closeFails) => { +it.each([ + { closeFails: false, sameReason: false }, + { closeFails: false, sameReason: true }, + { closeFails: true, sameReason: false }, + { closeFails: true, sameReason: true }, +])( + "retains physical Git read failure through late cancellation (close fails: $closeFails, same reason: $sameReason)", + async ({ closeFails, sameReason }) => { const root = await createTestTempDirectory("rea-reference-vcs-eio-"); const gitdir = join(root, ".git"); await mkdir(gitdir); @@ -382,19 +414,34 @@ it.each([false, true])( const resources = new ArtifactResourceScope(); const controller = new AbortController(); const reason = { source: "caller" }; - const physical = Object.assign(new Error("metadata read failed"), { - code: "EIO", - }); + let physical: unknown; + const handles: FileHandle[] = []; let allowClose = false; openMock.mockImplementation(async (...args) => { - const handle = await actual.open(...args); - if (resolve(String(args[0])) !== resolve(head)) return handle; - vi.spyOn(handle, "read").mockImplementation(async () => { - throw physical; + if (resolve(String(args[0])) !== resolve(head)) + return actual.open(...args); + const flags = args[1]; + if (typeof flags !== "number") + throw new Error("Expected numeric regular-file admission flags"); + // Preserve admission flags; neither O_TRUNC nor a write is used. + const handle = await actual.open( + args[0], + flags | constants.O_WRONLY, + args[2], + ); + handles.push(handle); + const read = handle.read.bind(handle); + vi.spyOn(handle, "read").mockImplementation(async (...readArgs) => { + try { + return await read(...readArgs); + } catch (cause: unknown) { + physical = cause; + throw cause; + } }); const close = handle.close.bind(handle); vi.spyOn(handle, "close").mockImplementation(async () => { - controller.abort(reason); + controller.abort(sameReason ? physical : reason); if (closeFails && !allowClose) throw new Error("close failed"); await close(); }); @@ -406,14 +453,37 @@ it.each([false, true])( resources, controller.signal, ); + expect(physical).toMatchObject({ code: "EBADF" }); + const cancellation = sameReason ? physical : reason; + expect(controller.signal.reason).toBe(cancellation); expect(result).toMatchObject({ ok: false, error: { code: "io", - secondaryCancellation: { cause: reason }, + secondaryCancellation: { cause: cancellation }, ...(closeFails ? { cleanup: { resources: [head] } } : {}), }, }); + if (result.ok) throw new Error("Physical metadata failure was ignored"); + expect(result.error.secondaryCancellation?.cause).toBe(cancellation); + expect(resources.cleanupResources).toEqual(closeFails ? [head] : []); + let retained: unknown = result.error.cause; + while ( + retained !== physical && + retained instanceof Error && + retained.cause !== undefined + ) + retained = retained.cause; + expect(retained).toBe(physical); + expect( + handles.every((handle) => + closeFails ? handle.fd >= 0 : handle.fd === -1, + ), + ).toBe(true); + allowClose = true; + await resources.close(); + expect(handles.every((handle) => handle.fd === -1)).toBe(true); + expect(resources.hasPending).toBe(false); } finally { allowClose = true; await resources.close(); @@ -584,3 +654,124 @@ it.each(["metadata", "import"] as const)( } }, ); + +it.each([ + ["lstat", false], + ["lstat", true], + ["stat", false], + ["stat", true], + ["realpath", false], + ["realpath", true], +] as const)( + "retains physical git directory %s with late abort (same reason: %s)", + async (stage, sameReason) => { + const root = await createTestTempDirectory("rea-git-physical-"); + await mkdir(join(root, ".git")); + const resources = new ArtifactResourceScope(); + const controller = new AbortController(); + const physical = Object.assign(new Error("git lookup failed"), { + code: "EIO", + }); + const reason = sameReason ? physical : { caller: "stopped" }; + const reject = (): never => { + controller.abort(reason); + throw physical; + }; + if (stage === "lstat") + lstatMock.mockImplementation(async (...args) => { + if (resolve(String(args[0])) === resolve(root, ".git")) return reject(); + return actual.lstat(...args); + }); + else if (stage === "stat") + statMock.mockImplementation(async (...args) => { + if (resolve(String(args[0])) === resolve(root, ".git")) return reject(); + return actual.stat(...args); + }); + else + realpathMock.mockImplementation(async (...args) => { + if (resolve(String(args[0])) === resolve(root, ".git")) return reject(); + return actual.realpath(...args); + }); + try { + const result = await importReferenceSource( + { + root, + caller: "test", + policy: { secretPatterns: [] }, + signal: controller.signal, + }, + resources, + ); + expect(result).toMatchObject({ + ok: false, + error: { + code: "io", + cause: physical, + secondaryCancellation: { cause: reason }, + }, + }); + } finally { + await resources.close(); + } + }, +); + +it("preserves real cancellation between Git directory lookup checkpoints", async () => { + const root = await createTestTempDirectory("rea-git-checkpoint-"); + const resources = new ArtifactResourceScope(); + const controller = new AbortController(); + const reason = { caller: "stopped" }; + lstatMock.mockImplementation(async (...args) => { + if (resolve(String(args[0])) === resolve(root, ".git")) { + controller.abort(reason); + throw Object.assign(new Error("absent"), { code: "ENOENT" }); + } + return actual.lstat(...args); + }); + try { + const result = await importReferenceSource( + { + root, + caller: "test", + policy: { secretPatterns: [] }, + signal: controller.signal, + }, + resources, + ); + expect(result).toMatchObject({ + ok: false, + error: { code: "cancelled", cause: reason }, + }); + } finally { + await resources.close(); + } +}); + +it("retains physical Git discovery lstat failure with late abort through VCS", async () => { + const root = await createTestTempDirectory("rea-git-vcs-physical-"); + const resources = new ArtifactResourceScope(); + const controller = new AbortController(); + const physical = Object.assign(new Error("git discovery failed"), { + code: "EIO", + }); + lstatMock.mockImplementation(async (...args) => { + if (resolve(String(args[0])) === resolve(root, ".git")) { + controller.abort(physical); + throw physical; + } + return actual.lstat(...args); + }); + try { + const result = await readReferenceSourceVcs( + root, + resources, + controller.signal, + ); + expect(result).toMatchObject({ + ok: false, + error: { code: "io", secondaryCancellation: { cause: physical } }, + }); + } finally { + await resources.close(); + } +});