fix(native): retain failed plist descriptor cleanup ownership

Native plist admission and the source-format prefix probe used raw
open/close, so a rejected close replaced the read outcome and left no
owner to retry. Both now go through the shared owned regular-file
operation in a client resource scope; client close retries retained
descriptors and a confirmed release clears the original error's cleanup
uncertainty.

Fixes #4723

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/native/NativeMacOSProvider.ts

@@ -8,7 +8,7 @@ import { observeNativeUi } from "./NativeUiObservation.js";
 import { LldbCallTracer, type NativeCallTracer } from "./LldbCallTracer.js";
 import { observeNativeCalls } from "./NativeCallObservation.js";
 import { basename, dirname, isAbsolute, resolve } from "node:path";
-import { open, realpath, stat } from "node:fs/promises";
+import { realpath, stat, type FileHandle } from "node:fs/promises";
 
 import { z } from "zod";
 
@@ -89,6 +89,13 @@ import {
 import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js";
 import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js";
 import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js";
+import {
+  artifactFileCancellationCause,
+  artifactFilePrimaryFailure,
+  bindArtifactFileCleanup,
+  withArtifactRegularFile,
+} from "../artifacts/ArtifactFileOperation.js";
+import { NonRegularFileReadError } from "../filesystem/RegularFile.js";
 import type { PrivateRuntimeRoot } from "../process/PrivateRuntimeRoot.js";
 import {
   createNativeUiHelperRuntime,
@@ -145,6 +152,7 @@ class NativeMacOSClient implements AnalysisClient {
   readonly #pendingNativeUiRuntimes = new Set<NativeUiHelperRuntime>();
   readonly #pendingCommands = new Set<Promise<unknown>>();
   readonly #signatureResources = new ArtifactResourceScope();
+  readonly #plistResources = new ArtifactResourceScope();
   #nativeUiTail: Promise<void> = Promise.resolve();
   #closed = false;
 
@@ -292,21 +300,14 @@ class NativeMacOSClient implements AnalysisClient {
     const uiCleanupFailures = await this.#serializeNativeUi(() =>
       this.#retryNativeUiCleanup(),
     );
-    let signatureCleanup: AnalysisError | undefined;
-    try {
-      await this.#signatureResources.close();
-    } catch (cause: unknown) {
-      const observation = ArtifactReaderFailure.cleanupObservation(
-        cause,
-        "signature-snapshot",
-      );
-      signatureCleanup = new ProviderCleanupError(
-        IDENTITY.id,
-        observation.resources,
-        { reason: observation.reason },
-        { operation: "close_binary", cause },
-      );
-    }
+    const signatureCleanup = await closeResourceScope(
+      this.#signatureResources,
+      "signature-snapshot",
+    );
+    const plistCleanup = await closeResourceScope(
+      this.#plistResources,
+      "plist",
+    );
     const commandCleanup = await this.#retryNativeCommandCleanup();
     let result = await this.tracer.close();
     const cleanupErrors = [
@@ -315,6 +316,7 @@ class NativeMacOSClient implements AnalysisClient {
         : [this.#nativeUiCleanupFailure(uiCleanupFailures, "close_binary")]),
       ...(commandCleanup === undefined ? [] : [commandCleanup]),
       ...(signatureCleanup === undefined ? [] : [signatureCleanup]),
+      ...(plistCleanup === undefined ? [] : [plistCleanup]),
     ];
     for (const cleanupError of cleanupErrors)
       result = result.ok
@@ -771,7 +773,8 @@ class NativeMacOSClient implements AnalysisClient {
       );
     const plist = await resolvePlistPath(this.target, requested);
     if (!plist.ok) return plist;
-    const selected = await requireRegularPlist(plist.value, requested);
+    const file = { resources: this.#plistResources, signal };
+    const selected = await requireRegularPlist(plist.value, requested, file);
     if (!selected.ok) return selected;
     const classified = await this.#run(
       "inspect_plist",
@@ -826,6 +829,7 @@ class NativeMacOSClient implements AnalysisClient {
       classified.value.stdout,
       plist.value,
       requested,
+      file,
     );
     if (!format.ok) return format;
     const result = inspectPlistSchema.parse({
@@ -1189,76 +1193,132 @@ const plistSourceFormat = async (
   description: string,
   path: string,
   requested: string | undefined,
+  file: PlistFileOwner,
 ): Promise<Result<"binary" | "xml" | "json" | "unknown", AnalysisError>> => {
   if (/binary property list/iu.test(description)) return ok("binary");
   if (/\bXML\b/iu.test(description)) return ok("xml");
   if (/\bJSON\b/iu.test(description)) return ok("json");
   if (!/\btext\b/iu.test(description)) return ok("unknown");
-  try {
-    const handle = await open(path, "r");
-    try {
-      const head = Buffer.alloc(PLIST_MARKUP_PROBE_BYTES);
-      const { bytesRead } = await handle.read(head, 0, head.length, 0);
-      const encoding =
-        head[0] === 0xff && head[1] === 0xfe
-          ? "utf-16le"
-          : head[0] === 0xfe && head[1] === 0xff
-            ? "utf-16be"
-            : "utf-8";
-      const text = new TextDecoder(encoding)
-        .decode(head.subarray(0, bytesRead))
-        .trimStart();
-      return ok(
-        /^<(?:[!?]|(?:plist|dict|array|string|integer|real|data|date|true|false)[\s/>])/u.test(
-          text,
-        )
-          ? "xml"
-          : "unknown",
-      );
-    } finally {
-      await handle.close();
-    }
-  } catch (cause: unknown) {
-    const code =
-      cause instanceof Error && "code" in cause ? cause.code : undefined;
-    if (code === "ENOENT" || code === "ENOTDIR")
-      return err(unreadablePlist(path, requested, "no file exists there"));
-    return err(
-      new ProviderAdapterError(IDENTITY.id, "inspect_plist", { cause }),
-    );
-  }
+  return withPlistFile(path, requested, file, async (handle) => {
+    const head = Buffer.alloc(PLIST_MARKUP_PROBE_BYTES);
+    const { bytesRead } = await handle.read(head, 0, head.length, 0);
+    const encoding =
+      head[0] === 0xff && head[1] === 0xfe
+        ? "utf-16le"
+        : head[0] === 0xfe && head[1] === 0xff
+          ? "utf-16be"
+          : "utf-8";
+    const text = new TextDecoder(encoding)
+      .decode(head.subarray(0, bytesRead))
+      .trimStart();
+    return /^<(?:[!?]|(?:plist|dict|array|string|integer|real|data|date|true|false)[\s/>])/u.test(
+      text,
+    )
+      ? "xml"
+      : "unknown";
+  });
 };
 
 /**
  * Admit a readable regular file before plutil runs, so a later plutil failure
  * reflects the bytes rather than a missing path or a host permission denial.
+ * Opening also observes ACL and macOS privacy denials that stat permits.
  */
-const requireRegularPlist = async (
+const requireRegularPlist = (
   path: string,
   requested: string | undefined,
-): Promise<Result<null, AnalysisError>> => {
+  file: PlistFileOwner,
+): Promise<Result<null, AnalysisError>> =>
+  withPlistFile(path, requested, file, () => Promise.resolve(null));
+
+/** The client scope that keeps a failed plist close owned until it is retried. */
+interface PlistFileOwner {
+  readonly resources: ArtifactResourceScope;
+  readonly signal: AbortSignal | undefined;
+}
+
+/** Own one plist descriptor from admission through its operation and final close. */
+const withPlistFile = async <Value>(
+  path: string,
+  requested: string | undefined,
+  { resources, signal }: PlistFileOwner,
+  operation: (handle: FileHandle) => Promise<Value>,
+): Promise<Result<Value, AnalysisError>> => {
   try {
-    if (!(await stat(path)).isFile())
-      return err(unreadablePlist(path, requested, "it is not a regular file"));
-    // Opening also observes ACL and macOS privacy denials that stat permits.
-    await (await open(path, "r")).close();
-    return ok(null);
-  } catch (cause: unknown) {
-    const code =
-      cause instanceof Error && "code" in cause ? cause.code : undefined;
-    if (code === "ENOENT" || code === "ENOTDIR")
-      return err(unreadablePlist(path, requested, "no file exists there"));
-    if (code === "EACCES" || code === "EPERM")
-      return err(
-        new BinaryTargetError(
-          path,
-          `permission denied while reading plist (${code})`,
-          { cause },
-        ),
-      );
-    return err(
-      new ProviderAdapterError(IDENTITY.id, "inspect_plist", { cause }),
+    return ok(
+      await resources.run(() =>
+        withArtifactRegularFile(path, resources, signal, operation, "follow"),
+      ),
     );
+  } catch (cause: unknown) {
+    return err(plistFileFailure(cause, path, requested));
+  }
+};
+
+/** Project one owned plist file outcome, keeping a failed close beside the physical failure. */
+const plistFileFailure = (
+  cause: unknown,
+  path: string,
+  requested: string | undefined,
+): AnalysisError => {
+  const physical = artifactFilePrimaryFailure(cause);
+  const code =
+    physical instanceof Error && "code" in physical ? physical.code : undefined;
+  const primary =
+    physical instanceof ArtifactReaderFailure && physical.reason === "cancelled"
+      ? new AnalysisCancelledError("inspect_plist", {
+          cause: artifactFileCancellationCause(physical),
+        })
+      : physical instanceof NonRegularFileReadError
+        ? unreadablePlist(path, requested, "it is not a regular file")
+        : code === "ENOENT" || code === "ENOTDIR"
+          ? unreadablePlist(path, requested, "no file exists there")
+          : code === "EACCES" || code === "EPERM"
+            ? new BinaryTargetError(
+                path,
+                `permission denied while reading plist (${code})`,
+                { cause: physical },
+              )
+            : new ProviderAdapterError(IDENTITY.id, "inspect_plist", {
+                cause: physical,
+              });
+  if (!(cause instanceof ArtifactReaderFailure) || cause.cleanup === undefined)
+    return primary;
+  const error = analysisErrorWithCleanupFailure(
+    primary,
+    new ProviderCleanupError(
+      IDENTITY.id,
+      cause.cleanup.resources,
+      { reason: cause.cleanup.reason },
+      { operation: "inspect_plist", cause },
+    ),
+    "inspect_plist",
+  );
+  bindArtifactFileCleanup(cause, error);
+  return error;
+};
+
+/** Retry one scope's retained owners at client close, bound to their exact leases. */
+const closeResourceScope = async (
+  resources: ArtifactResourceScope,
+  fallbackResource: string,
+): Promise<AnalysisError | undefined> => {
+  try {
+    await resources.close();
+    return undefined;
+  } catch (cause: unknown) {
+    const observation = ArtifactReaderFailure.cleanupObservation(
+      cause,
+      fallbackResource,
+    );
+    const error = new ProviderCleanupError(
+      IDENTITY.id,
+      observation.resources,
+      { reason: observation.reason },
+      { operation: "close_binary", cause },
+    );
+    bindArtifactFileCleanup(cause, error);
+    return error;
   }
 };
 

tests/boundary/providers/native/nativePlistFileCleanup.test.ts

@@ -0,0 +1,150 @@
+import { writeFile } from "node:fs/promises";
+import { join } from "node:path";
+
+import { expect, it, vi } from "vitest";
+
+const faults = vi.hoisted(() => ({
+  path: undefined as string | undefined,
+  failRead: false,
+  failClose: false,
+  closes: 0,
+}));
+
+vi.mock("node:fs/promises", async (importOriginal) => {
+  const fs = await importOriginal<typeof import("node:fs/promises")>();
+  return {
+    ...fs,
+    open: async (...args: Parameters<typeof fs.open>) => {
+      const handle = await fs.open(...args);
+      if (args[0] !== faults.path) return handle;
+      const read = handle.read.bind(handle) as typeof handle.read;
+      const close = handle.close.bind(handle);
+      handle.read = ((...readArgs: Parameters<typeof handle.read>) =>
+        faults.failRead
+          ? Promise.reject(new Error("injected plist prefix read failure"))
+          : read(...readArgs)) as typeof handle.read;
+      handle.close = async () => {
+        faults.closes++;
+        if (faults.failClose)
+          throw new Error("injected plist descriptor close failure");
+        return close();
+      };
+      return handle;
+    },
+  };
+});
+
+import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js";
+import { NativeMacOSProvider } from "../../../../src/native/NativeMacOSProvider.js";
+import {
+  NativeFixtureRunner,
+  nativeMachoTarget,
+} from "../../../fixtures/nativeCommands.js";
+import { createTestTempDirectory } from "../../../fixtures/temporaryDirectory.js";
+
+const setup = async (failure: {
+  read: boolean;
+  close: "admission" | "probe";
+}) => {
+  const root = await createTestTempDirectory("rea-plist-file-cleanup-");
+  const path = join(root, "selected.plist");
+  await writeFile(path, "<plist><string>value</string></plist>");
+  faults.path = path;
+  faults.failRead = false;
+  faults.failClose = false;
+  faults.closes = 0;
+  const client = new NativeMacOSProvider(
+    {},
+    new (class extends NativeFixtureRunner {
+      override async run(tool: string, arguments_: readonly string[]) {
+        // Arm the probe faults only after admission released its descriptor.
+        if (tool === "file" && failure.close === "probe") {
+          faults.failRead = failure.read;
+          faults.failClose = true;
+        }
+        return super.run(tool, arguments_);
+      }
+    })({ file: "ASCII text\n", plutil: JSON.stringify("value") }),
+    "darwin",
+  ).createClient(nativeMachoTarget(join(root, "target")));
+  if (failure.close === "admission") faults.failClose = true;
+  return { client, path };
+};
+
+const causeMessages = (error: unknown): string[] => {
+  const messages: string[] = [];
+  const pending = [error];
+  const seen = new Set<unknown>();
+  while (pending.length > 0) {
+    const next = pending.pop();
+    if (!(next instanceof Error) || seen.has(next)) continue;
+    seen.add(next);
+    messages.push(next.message);
+    pending.push(next.cause);
+    if (next instanceof AggregateError) pending.push(...next.errors);
+    for (const value of Object.values(next)) pending.push(value);
+  }
+  return messages;
+};
+
+it.for([
+  { name: "prefix read and close both reject", read: true },
+  { name: "prefix read succeeds and close rejects", read: false },
+])(
+  "retains the plist probe descriptor when $name",
+  async ({ read }, { onTestFinished }) => {
+    const { client, path } = await setup({ read, close: "probe" });
+    onTestFinished(async () => {
+      faults.failClose = false;
+      await client.close();
+    });
+
+    const inspected = await client.execute("inspect_plist", { path });
+    expect(inspected.ok).toBe(false);
+    if (inspected.ok)
+      throw new Error("Injected close failure was not returned");
+    expect(inspected.error.cleanupIncomplete).toBe(true);
+    expect(projectAnalysisError(inspected.error).code).toBe(
+      "cleanup_incomplete",
+    );
+    // The physical read failure stays primary beside the cleanup failure.
+    expect(causeMessages(inspected.error)).toEqual(
+      read
+        ? expect.arrayContaining(["injected plist prefix read failure"])
+        : expect.not.arrayContaining(["injected plist prefix read failure"]),
+    );
+    const attempted = faults.closes;
+
+    // Client close retries the exact retained descriptor and reports it.
+    const held = await client.close();
+    expect(held.ok).toBe(false);
+    expect(faults.closes).toBeGreaterThan(attempted);
+
+    faults.failClose = false;
+    faults.failRead = false;
+    const released = await client.close();
+    expect(released.ok).toBe(true);
+    // A confirmed release resolves the original error's cleanup uncertainty.
+    expect(inspected.error.cleanupIncomplete).toBe(false);
+  },
+);
+
+it("retains the admission descriptor when its close rejects", async ({
+  onTestFinished,
+}) => {
+  const { client, path } = await setup({ read: false, close: "admission" });
+  onTestFinished(async () => {
+    faults.failClose = false;
+    await client.close();
+  });
+
+  const inspected = await client.execute("inspect_plist", { path });
+  expect(inspected.ok).toBe(false);
+  if (inspected.ok) throw new Error("Injected close failure was not returned");
+  expect(inspected.error.cleanupIncomplete).toBe(true);
+
+  faults.failClose = false;
+  const released = await client.close();
+  expect(released.ok).toBe(true);
+  expect(inspected.error.cleanupIncomplete).toBe(false);
+});