refactor(native): consolidate command operation admission

src/native/NativeMacOSProvider.ts

@@ -210,7 +210,7 @@ export class NativeMacOSProvider implements AnalysisProvider {
 
 class NativeMacOSClient implements AnalysisClient {
   readonly #pendingNativeUiRuntimes = new Set<NativeUiHelperRuntime>();
-  readonly #pendingCommands = new Set<Promise<unknown>>();
+  readonly #pendingCommandOperations = new Set<Promise<unknown>>();
   readonly #signatureResources = new ArtifactResourceScope();
   readonly #plistResources = new ArtifactResourceScope();
   readonly #callResources = new ArtifactResourceScope();
@@ -351,12 +351,12 @@ class NativeMacOSClient implements AnalysisClient {
     ) {
       return this.#observeNativeUi(operation, parameters, options?.signal);
     }
+    const pending = Promise.resolve().then(() =>
+      this.#dispatch(operation, parameters, options?.signal),
+    );
+    this.#pendingCommandOperations.add(pending);
     try {
-      const observation = await this.#dispatch(
-        operation,
-        parameters,
-        options?.signal,
-      );
+      const observation = await pending;
       return observation.ok
         ? ok(
             createAnalysisExecution(observation.value.result, IDENTITY, {
@@ -367,12 +367,14 @@ class NativeMacOSClient implements AnalysisClient {
         : err(resolvedAnalysisError(observation.error));
     } catch (cause: unknown) {
       return err(nativeOutputFailure(operation, cause));
+    } finally {
+      this.#pendingCommandOperations.delete(pending);
     }
   }
 
   async close(): Promise<Result<null, AnalysisError>> {
     this.#closed = true;
-    await Promise.allSettled(this.#pendingCommands);
+    await Promise.allSettled(this.#pendingCommandOperations);
     const uiCleanupFailures = await this.#serializeNativeUi(() =>
       this.#retryNativeUiCleanup(),
     );
@@ -1003,7 +1005,7 @@ class NativeMacOSClient implements AnalysisClient {
           };
           const selected = await requireRegularPlist(fileInput);
           if (!selected.ok) return selected;
-          const classified = await this.#runAdmitted(
+          const classified = await this.#run(
             "inspect_plist",
             "file",
             ["-b", plist.value.path],
@@ -1096,7 +1098,7 @@ class NativeMacOSClient implements AnalysisClient {
     if (format === "xml") {
       parsed = parsePlistXmlSource(source.value);
       if (parsed.ok) {
-        const validated = await this.#runAdmitted(
+        const validated = await this.#run(
           "inspect_plist",
           "plutil",
           ["-lint", "--", fileInput.selection.path],
@@ -1160,7 +1162,7 @@ class NativeMacOSClient implements AnalysisClient {
   ): Promise<Result<NativeObservation, AnalysisError>> {
     const { path } = fileInput.selection;
     const { signal, timeoutMs } = fileInput;
-    const json = await this.#runAdmitted(
+    const json = await this.#run(
       "inspect_plist",
       "plutil",
       ["-convert", "json", "-o", "-", "--", path],
@@ -1180,7 +1182,7 @@ class NativeMacOSClient implements AnalysisClient {
       AnalysisOutputError | AnalysisResourceConstraintError
     >;
     if (jsonAttempt === undefined || jsonAttempt.needsNumberTypes) {
-      const converted = await this.#runAdmitted(
+      const converted = await this.#run(
         "inspect_plist",
         "plutil",
         ["-convert", "xml1", "-o", "-", "--", path],
@@ -1244,39 +1246,22 @@ class NativeMacOSClient implements AnalysisClient {
     operation: NativeToolName,
     tool: string,
     arguments_: readonly string[],
-    options: NativeClientCommandOptions,
+    options: {
+      readonly signal?: AbortSignal | undefined;
+      readonly timeoutMs: number;
+      readonly acceptNonZero?: boolean;
+      readonly acceptNonZeroIf?:
+        | ((capture: NativeCommandCapture) => boolean)
+        | undefined;
+    },
   ): Promise<Result<NativeCommandCapture, AnalysisError>> {
-    if (this.#closed)
-      return err(
-        new AnalysisCapabilityUnavailableError(
-          IDENTITY.id,
-          operation,
-          "Native provider client is closed",
-        ),
-      );
-    return this.#runAdmitted(operation, tool, arguments_, options);
-  }
-
-  /** Child commands borrow the admitted file workflow's lifetime through close. */
-  async #runAdmitted(
-    operation: NativeToolName,
-    tool: string,
-    arguments_: readonly string[],
-    options: NativeClientCommandOptions,
-  ): Promise<Result<NativeCommandCapture, AnalysisError>> {
-    const command = this.runner.run(tool, arguments_, {
+    // Commands borrow their enclosing operation's admission through finalization.
+    const captured = await this.runner.run(tool, arguments_, {
       ...(options.signal === undefined ? {} : { signal: options.signal }),
       timeoutMs: options.timeoutMs,
       acceptNonZero:
         options.acceptNonZero ?? options.acceptNonZeroIf !== undefined,
     });
-    this.#pendingCommands.add(command);
-    let captured: Awaited<typeof command>;
-    try {
-      captured = await command;
-    } finally {
-      this.#pendingCommands.delete(command);
-    }
     if (captured.ok) {
       const hasFailedExit =
         captured.value.exitCode !== 0 || captured.value.signal !== null;
@@ -1378,15 +1363,6 @@ const nativeOutputFailure = (
 ): AnalysisError =>
   new AnalysisOutputError(operation, "Native output parsing failed", { cause });
 
-interface NativeClientCommandOptions {
-  readonly signal?: AbortSignal | undefined;
-  readonly timeoutMs: number;
-  readonly acceptNonZero?: boolean;
-  readonly acceptNonZeroIf?:
-    | ((capture: NativeCommandCapture) => boolean)
-    | undefined;
-}
-
 interface NativeObservation {
   readonly result: JsonValue;
   readonly limitations: readonly string[];

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

@@ -15,6 +15,7 @@ import { AnalysisCleanupLease } from "../../../../src/artifacts/AnalysisCleanupL
 import { ArtifactReaderFailure } from "../../../../src/artifacts/ArtifactReader.js";
 import { err, ok } from "../../../../src/domain/result.js";
 import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js";
+import { inspectPlistSchema } from "../../../../src/domain/native/nativeInspection.js";
 
 const fixture = async () => {
   const root = await createTestTempDirectory("rea-native-plist-lifetime-");
@@ -332,15 +333,55 @@ it("preserves an opaque cancellation observed during successful prefix close", a
   expect((await client.close()).ok).toBe(true);
 });
 
-it.for(["source", "json", "xml"] as const)(
-  "joins an admitted prefix read and its %s commands when client close starts concurrently",
-  async (route) => {
+it.for([
+  ["prefix", "source"],
+  ["source", "source"],
+  ["prefix", "json"],
+  ["prefix", "xml"],
+] as const)(
+  "joins an admitted %s read and final %s command when client close starts concurrently",
+  async ([phase, route]) => {
     const { client, path, prototype, runner } = await fixture();
     if (route !== "source") await writeFile(path, "{ name = value; }");
+    let enter!: () => void;
+    let resume!: () => void;
+    let enterValidation!: () => void;
+    let resumeValidation!: () => void;
+    const entered = new Promise<void>((resolve) => {
+      enter = resolve;
+    });
+    const gate = new Promise<void>((resolve) => {
+      resume = resolve;
+    });
+    const validationEntered = new Promise<void>((resolve) => {
+      enterValidation = resolve;
+    });
+    const validationGate = new Promise<void>((resolve) => {
+      resumeValidation = resolve;
+    });
+    const original = prototype.read;
+    let reads = 0;
+    const read = vi.spyOn(prototype, "read").mockImplementation(async function (
+      this: FileHandle,
+      ...args: Parameters<FileHandle["read"]>
+    ) {
+      reads += 1;
+      if (reads === (phase === "prefix" ? 1 : 2)) {
+        enter();
+        await gate;
+      }
+      return Reflect.apply(original, this, args);
+    });
+    let validationStarted = false;
     const run = runner.run.bind(runner);
     const commands = vi
       .spyOn(runner, "run")
       .mockImplementation(async (tool, args, options) => {
+        if (tool === "plutil" && (route !== "xml" || args[1] === "xml1")) {
+          validationStarted = true;
+          enterValidation();
+          await validationGate;
+        }
         const result = await run(tool, args, options);
         if (route !== "xml" || tool !== "plutil" || !result.ok) return result;
         const stdout =
@@ -352,31 +393,16 @@ it.for(["source", "json", "xml"] as const)(
           exitCode: args[1] === "json" ? 1 : 0,
         });
       });
-    onTestFinished(() => commands.mockRestore());
-    let enter!: () => void;
-    let resume!: () => void;
-    const entered = new Promise<void>((resolve) => {
-      enter = resolve;
-    });
-    const gate = new Promise<void>((resolve) => {
-      resume = resolve;
-    });
-    const original = prototype.read;
-    const read = vi
-      .spyOn(prototype, "read")
-      .mockImplementationOnce(async function (
-        this: FileHandle,
-        ...args: Parameters<FileHandle["read"]>
-      ) {
-        enter();
-        await gate;
-        return Reflect.apply(original, this, args);
-      });
     onTestFinished(() => {
       resume();
+      resumeValidation();
       read.mockRestore();
+      commands.mockRestore();
+    });
+    const inspecting = client.execute("inspect_plist", {
+      path,
+      native_command_timeout_ms: 1500,
     });
-    const inspecting = client.execute("inspect_plist", { path });
     await entered;
     let finished = false;
     const closing = client.close().then((result) => {
@@ -385,17 +411,43 @@ it.for(["source", "json", "xml"] as const)(
     });
     await new Promise<void>((resolve) => setImmediate(resolve));
     expect(finished).toBe(false);
+    expect(await client.execute("inspect_plist", { path })).toMatchObject({
+      ok: false,
+      error: { _tag: "AnalysisCapabilityUnavailableError" },
+    });
     resume();
+    await Promise.race([validationEntered, inspecting]);
+    expect(validationStarted).toBe(true);
+    await new Promise<void>((resolve) => setImmediate(resolve));
+    expect(finished).toBe(false);
+    resumeValidation();
     const inspected = await inspecting;
-    expect(
-      inspected.ok,
-      inspected.ok ? undefined : inspected.error.message,
-    ).toBe(true);
+    if (!inspected.ok) throw inspected.error;
     expect((await closing).ok).toBe(true);
-    expect(commands.mock.calls.map(([tool, args]) => [tool, args[0]])).toEqual([
-      ["file", "-b"],
-      ["plutil", route === "source" ? "-lint" : "-convert"],
-      ...(route === "xml" ? [["plutil", "-convert"]] : []),
-    ]);
+    const result = inspectPlistSchema.parse(inspected.value.result);
+    expect(result.value).toBe("value");
+    expect(result.source_path).toBe(path);
+    const expectedCommands = [
+      ["file", ["-b", path]],
+      [
+        "plutil",
+        route === "source"
+          ? ["-lint", "--", path]
+          : ["-convert", "json", "-o", "-", "--", path],
+      ],
+      ...(route === "xml"
+        ? [["plutil", ["-convert", "xml1", "-o", "-", "--", path]]]
+        : []),
+    ];
+    expect(commands.mock.calls.map(([tool, args]) => [tool, args])).toEqual(
+      expectedCommands,
+    );
+    expect(result.provenance.map(({ tool }) => tool)).toEqual(
+      expectedCommands.map(([tool]) => tool),
+    );
+    expect(commands.mock.calls.at(-1)?.[2]).toEqual({
+      timeoutMs: 1500,
+      acceptNonZero: true,
+    });
   },
 );