diff --git a/src/native/NativeMacOSProvider.ts b/src/native/NativeMacOSProvider.ts index dabbab2fee..1a0d8a3ab5 100644 --- a/src/native/NativeMacOSProvider.ts +++ b/src/native/NativeMacOSProvider.ts @@ -210,7 +210,7 @@ export class NativeMacOSProvider implements AnalysisProvider { class NativeMacOSClient implements AnalysisClient { readonly #pendingNativeUiRuntimes = new Set(); - readonly #pendingCommands = new Set>(); + readonly #pendingCommandOperations = new Set>(); 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> { 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> { 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> { - 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> { - 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; - 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[]; diff --git a/tests/boundary/providers/native/nativePlistFileCleanup.test.ts b/tests/boundary/providers/native/nativePlistFileCleanup.test.ts index 57021b5d04..fa7aaccf63 100644 --- a/tests/boundary/providers/native/nativePlistFileCleanup.test.ts +++ b/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((resolve) => { + enter = resolve; + }); + const gate = new Promise((resolve) => { + resume = resolve; + }); + const validationEntered = new Promise((resolve) => { + enterValidation = resolve; + }); + const validationGate = new Promise((resolve) => { + resumeValidation = resolve; + }); + const original = prototype.read; + let reads = 0; + const read = vi.spyOn(prototype, "read").mockImplementation(async function ( + this: FileHandle, + ...args: Parameters + ) { + 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((resolve) => { - enter = resolve; - }); - const gate = new Promise((resolve) => { - resume = resolve; - }); - const original = prototype.read; - const read = vi - .spyOn(prototype, "read") - .mockImplementationOnce(async function ( - this: FileHandle, - ...args: Parameters - ) { - 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((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((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, + }); }, );