diff --git a/src/application/ReferenceSourceVcsAdapter.ts b/src/application/ReferenceSourceVcsAdapter.ts index 5bb6d0e35d..b2a21e2b50 100644 --- a/src/application/ReferenceSourceVcsAdapter.ts +++ b/src/application/ReferenceSourceVcsAdapter.ts @@ -4,10 +4,8 @@ import { join, resolve } from "node:path"; import { err, ok, type Result } from "../domain/result.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; -import type { AnalysisCleanupObservation } from "../domain/analysisErrorBase.js"; import { readRegularFile, - readRegularFileText, RegularFileCleanupFailure, retryRegularFileCleanup, } from "./RegularFileRead.js"; @@ -50,42 +48,6 @@ export const readReferenceSourceVcs = async ( return ok({ kind: "git", head, dirty: null }); } catch (cause: unknown) { if (cause instanceof ArtifactReaderFailure) throw cause; - if (cause instanceof RegularFileCleanupFailure) { - const retry = await retryRegularFileCleanup(cause, resources); - if (retry.cleanup !== undefined) { - const primary = - cause.outcome.kind === "failed" - ? cause.outcome.cause - : cause.cleanupCause; - const code = isAborted(signal) ? "cancelled" : "io"; - return err({ - tag: "reference-source-reader", - code, - message: `Git metadata could not be read safely: ${errorMessage(primary)}`, - cleanup: retry.cleanup, - cause: primary, - }); - } - if (retry.outcome.kind === "completed") - return err({ - tag: "reference-source-reader", - code: isAborted(signal) ? "cancelled" : "io", - message: isAborted(signal) - ? "Git metadata read cancelled" - : `Git metadata close failed: ${errorMessage(cause.cleanupCause)}`, - cause: cause.cleanupCause, - }); - if (retry.outcome.kind === "failed") { - return isAborted(signal) - ? err({ - tag: "reference-source-reader", - code: "cancelled", - message: "Git metadata read cancelled", - cause: retry.outcome.cause, - }) - : ok(unknownVcs()); - } - } return ok(unknownVcs()); } }); @@ -115,26 +77,65 @@ const resolveSourceHead = async ( // isomorphic-git is loaded on first use so CLI and MCP startup skip it. const { resolveRef } = await import("isomorphic-git"); let failedRead: { readonly cause: unknown } | undefined; - const cleanupFailures: { - readonly failure: RegularFileCleanupFailure; - readonly cleanup?: NonNullable; - }[] = []; + const cleanupFailures: ArtifactReaderFailure[] = []; + const readMetadataBytes = async (path: string): Promise => { + try { + return await readRegularFile(path, { signal }); + } catch (cause: unknown) { + if (!(cause instanceof RegularFileCleanupFailure)) throw cause; + const retry = await retryRegularFileCleanup(cause, resources); + if ( + retry.outcome.kind === "failed" && + errorCode(retry.outcome.cause) !== "ENOENT" + ) + failedRead ??= { cause: retry.outcome.cause }; + if ( + retry.cleanup === undefined && + retry.outcome.kind === "failed" && + !isAborted(signal) + ) + throw retry.outcome.cause; + const primary = + retry.outcome.kind === "failed" + ? retry.outcome.cause + : cause.cleanupCause; + throw new ArtifactReaderFailure( + isAborted(signal) ? "cancelled" : "io", + `Git metadata could not be read safely: ${errorMessage(primary)}`, + { + cause: primary, + ...(retry.cleanup === undefined ? {} : { cleanup: retry.cleanup }), + }, + ); + } + }; + const readMetadataText = async (path: string): Promise => + (await readMetadataBytes(path)).toString("utf8"); const throwCleanupFailures = (primary: unknown): never => { - const cleanup = mergeCleanup( - cleanupFailures.flatMap((failure) => - failure.cleanup === undefined ? [] : [failure.cleanup], - ), + const observations = cleanupFailures.flatMap((failure) => + failure.cleanup === undefined ? [] : [failure.cleanup], ); throw new ArtifactReaderFailure( isAborted(signal) ? "cancelled" : "io", `Git metadata could not be read safely: ${errorMessage(primary)}`, { cause: new AggregateError( - [primary, ...cleanupFailures.map((item) => item.failure)], + [primary, ...cleanupFailures], "Git metadata reads and cleanup failed", { cause: primary }, ), - ...(cleanup === undefined ? {} : { cleanup }), + ...(observations.length === 0 + ? {} + : { + cleanup: { + reason: observations.map(({ reason }) => reason).join("; "), + resources: [ + ...new Set( + observations.flatMap(({ resources }) => resources), + ), + ], + }, + }), }, ); }; @@ -157,26 +158,15 @@ const resolveSourceHead = async ( if (path === undefined) throw new TypeError("Git filesystem read requires a path"); try { - const bytes = await readRegularFile(path, { signal }); + const bytes = await readMetadataBytes(path); const encoding = typeof options === "string" ? options : options?.encoding; return encoding === undefined || encoding === null ? bytes : bytes.toString(encoding); } catch (cause: unknown) { - if (cause instanceof RegularFileCleanupFailure) { - const retry = await retryRegularFileCleanup(cause, resources); - cleanupFailures.push({ - failure: cause, - ...(retry.cleanup === undefined - ? {} - : { cleanup: retry.cleanup }), - }); - if ( - retry.outcome.kind === "failed" && - errorCode(retry.outcome.cause) !== "ENOENT" - ) - failedRead ??= { cause: retry.outcome.cause }; + if (cause instanceof ArtifactReaderFailure) { + cleanupFailures.push(cause); } else if (errorCode(cause) !== "ENOENT") failedRead ??= { cause }; throw cause; } @@ -197,19 +187,14 @@ const resolveSourceHead = async ( throw cause; } if (cleanupFailures.length > 0) - throwCleanupFailures( - failedRead?.cause ?? cleanupFailurePrimary(cleanupFailures[0]), - ); + throwCleanupFailures(failedRead?.cause ?? cleanupFailures[0]?.cause); signal?.throwIfAborted(); if (failedRead !== undefined) throw failedRead.cause; return head; }; const dotgit = join(root, ".git"); if (!(await lstat(dotgit)).isFile()) return readRef({ ref: "HEAD" }); - const pointer = (await readRegularFileText(dotgit, { signal })).replace( - /\r?\n$/u, - "", - ); + const pointer = (await readMetadataText(dotgit)).replace(/\r?\n$/u, ""); if (!pointer.startsWith("gitdir: ") || pointer.length === 8) throw new Error("Invalid Git directory pointer"); const gitdir = resolve(root, pointer.slice(8)); @@ -217,17 +202,16 @@ const resolveSourceHead = async ( try { commonDirectory = resolve( gitdir, - ( - await readRegularFileText(join(gitdir, "commondir"), { signal }) - ).replace(/\r?\n$/u, ""), + (await readMetadataText(join(gitdir, "commondir"))).replace( + /\r?\n$/u, + "", + ), ); } catch (cause: unknown) { if (errorCode(cause) !== "ENOENT") throw cause; return readRef({ gitdir, ref: "HEAD" }); } - const head = ( - await readRegularFileText(join(gitdir, "HEAD"), { signal }) - ).trim(); + const head = (await readMetadataText(join(gitdir, "HEAD"))).trim(); if (!head.startsWith("ref: ")) return readRef({ gitdir, ref: "HEAD" }); const ref = head.slice(5); const privateRef = ["refs/bisect/", "refs/rewritten/", "refs/worktree/"].some( @@ -239,32 +223,6 @@ const resolveSourceHead = async ( }); }; -const cleanupFailurePrimary = ( - failure: - | { - readonly failure: RegularFileCleanupFailure; - } - | undefined, -): unknown => - failure?.failure.outcome.kind === "failed" - ? failure.failure.outcome.cause - : failure?.failure.cleanupCause; - -const mergeCleanup = ( - observations: readonly AnalysisCleanupObservation[], -): AnalysisCleanupObservation | undefined => { - if (observations.length === 0) return undefined; - const first = observations[0]; - if (first === undefined) return undefined; - return observations.slice(1).reduce( - (merged, next) => ({ - reason: `${merged.reason}; ${next.reason}`, - resources: [...new Set([...merged.resources, ...next.resources])], - }), - first, - ); -}; - const errorCode = (cause: unknown): string | undefined => typeof cause === "object" && cause !== null && "code" in cause ? String(cause.code) diff --git a/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts b/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts index 615dbf6173..fd322996ca 100644 --- a/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts +++ b/tests/boundary/filesystem/referenceSourceMetadataCleanup.test.ts @@ -52,6 +52,41 @@ const failCloseUntilAllowed = ( }; }; +const createPackedRefRepository = async () => { + const root = await createTestTempDirectory("rea-reference-packed-owner-"); + await execFileOutput("git", ["init", "--initial-branch=main"], { cwd: root }); + await writeFile(join(root, "main.ts"), "export const value = 1;\n"); + await execFileOutput( + "git", + [ + "-c", + "user.name=REA fixture", + "-c", + "user.email=fixture@example.invalid", + "-c", + "commit.gpgsign=false", + "commit", + "--allow-empty", + "-m", + "fixture", + ], + { cwd: root }, + ); + const head = ( + await execFileOutput("git", ["rev-parse", "HEAD"], { cwd: root }) + ).stdout.trim(); + await execFileOutput("git", ["pack-refs", "--all", "--prune"], { + cwd: root, + }); + const looseRef = join(root, ".git", "refs", "heads", "main"); + const packedRefs = join(root, ".git", "packed-refs"); + await actual.mkdir(join(root, ".git", "refs", "heads"), { + recursive: true, + }); + await writeFile(looseRef, `${head}\n`); + return { root, looseRef, packedRefs }; +}; + it("retains failed .gitignore cleanup under the caller's scope", async () => { const root = await createTestTempDirectory("rea-reference-ignore-owner-"); const path = join(root, ".gitignore"); @@ -144,33 +179,7 @@ it("preserves Git metadata cleanup failure and the completed source read", async }); it("retains later packed-ref cleanup after an earlier loose-ref read failure", async () => { - const root = await createTestTempDirectory("rea-reference-packed-owner-"); - await execFileOutput("git", ["init", "--initial-branch=main"], { cwd: root }); - await writeFile(join(root, "main.ts"), "export const value = 1;\n"); - await execFileOutput( - "git", - [ - "-c", - "user.name=REA fixture", - "-c", - "user.email=fixture@example.invalid", - "-c", - "commit.gpgsign=false", - "commit", - "--allow-empty", - "-m", - "fixture", - ], - { cwd: root }, - ); - const head = ( - await execFileOutput("git", ["rev-parse", "HEAD"], { cwd: root }) - ).stdout.trim(); - await execFileOutput("git", ["pack-refs", "--all", "--prune"], { cwd: root }); - const looseRef = join(root, ".git", "refs", "heads", "main"); - const packedRefs = join(root, ".git", "packed-refs"); - await actual.mkdir(join(root, ".git", "refs", "heads"), { recursive: true }); - await writeFile(looseRef, `${head}\n`); + const { root, looseRef, packedRefs } = await createPackedRefRepository(); const resources = new ArtifactResourceScope(); let allowClose = false; @@ -224,3 +233,53 @@ it("retains later packed-ref cleanup after an earlier loose-ref read failure", a Array.from({ length: packedCloseCalls.size }, () => 3), ); }); + +it("keeps a later packed-ref read error primary over earlier loose-ref cleanup", async () => { + const { root, looseRef, packedRefs } = await createPackedRefRepository(); + const resources = new ArtifactResourceScope(); + let allowClose = false; + const looseCloseCalls = new Map(); + openMock.mockImplementation(async (...args) => { + const handle = await actual.open(...args); + const path = resolve(String(args[0])); + if (path === resolve(looseRef)) { + const close = handle.close.bind(handle); + vi.spyOn(handle, "close").mockImplementation(async () => { + looseCloseCalls.set(handle, (looseCloseCalls.get(handle) ?? 0) + 1); + if (!allowClose) throw new Error("injected loose ref close failure"); + await close(); + }); + } + if (path === resolve(packedRefs)) + vi.spyOn(handle, "read").mockImplementation(async () => { + throw Object.assign(new Error("injected packed-refs EIO"), { + code: "EIO", + }); + }); + return handle; + }); + onTestFinished(async () => { + allowClose = true; + await resources.close().catch(() => undefined); + }); + + const result = await readReferenceSourceVcs(root, resources); + if (result.ok) throw new Error("Expected Git metadata read to fail"); + expect(result.error.message).toContain("injected packed-refs EIO"); + expect(result.error.message).not.toContain( + "injected loose ref close failure", + ); + expect(result.error.cleanup).toMatchObject({ + resources: [looseRef], + reason: expect.stringContaining("injected loose ref close failure"), + }); + expect([...looseCloseCalls.values()]).toEqual( + Array.from({ length: looseCloseCalls.size }, () => 2), + ); + + allowClose = true; + await resources.close(); + expect([...looseCloseCalls.values()]).toEqual( + Array.from({ length: looseCloseCalls.size }, () => 3), + ); +});