refactor(reference): consolidate Git metadata cleanup

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<ArtifactReaderFailure["cleanup"]>;
-  }[] = [];
+  const cleanupFailures: ArtifactReaderFailure[] = [];
+  const readMetadataBytes = async (path: string): Promise<Buffer> => {
+    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<string> =>
+    (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<AnalysisCleanupObservation>(
-    (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)

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<object, number>();
+  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),
+  );
+});