From e7c4a26316fdb3eb253b13684cd66d2e0836f5e7 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Wed, 11 Nov 2026 17:20:14 +0800 Subject: [PATCH] fix(native): retain command cleanup across source finalization (#10009) --- src/native/NativeMacOSProvider.ts | 83 +++++++- src/native/NativeMachoSourceProvider.test.ts | 167 ++++++++++++++++ src/native/SignatureTargetBinding.test.ts | 190 ++++++++++++++++++- 3 files changed, 432 insertions(+), 8 deletions(-) diff --git a/src/native/NativeMacOSProvider.ts b/src/native/NativeMacOSProvider.ts index 170240e488..cdc4fe9645 100644 --- a/src/native/NativeMacOSProvider.ts +++ b/src/native/NativeMacOSProvider.ts @@ -1055,6 +1055,19 @@ class NativeMacOSClient implements AnalysisClient { // Verify failed captures too: a disappearing target is not a codesign // execution failure, and later captures must never use another version. const after = await verifySignatureTarget(this.target, binding, signal); + if (!capture.ok && !after.ok) { + const primary = + after.error instanceof AnalysisCancelledError + ? analysisErrorWithCancellation(capture.error, after.error) + : after.error; + return err( + nativeFailureWithCommandCleanup( + primary, + capture.error, + "inspect_signature", + ), + ); + } return after.ok ? capture : after; } @@ -1817,6 +1830,7 @@ const nativeMachoCompletion = ( : new AnalysisCancelledError(operation, { cause: completion.cancellation.reason, ...(completedFailure === undefined || + completedFailure.cleanupIncomplete || (cause instanceof ArtifactReaderFailure && cause.cleanup !== undefined) ? {} @@ -1831,6 +1845,40 @@ const nativeMachoCompletion = ( return { observation, completedFailure, completionCancellation }; }; +/** Every native reclassification retains the command and independent file owners. */ +const nativeFailureWithCommandCleanup = ( + primary: AnalysisError, + producer: AnalysisError, + operation: string, + descriptorCleanup?: AnalysisError, +): AnalysisError => { + if (!producer.cleanupIncomplete) return primary; + const cleanup = descriptorCleanup ?? producer; + return cleanup === primary + ? primary + : analysisErrorWithCleanupFailure( + primary, + cleanup, + operation, + ).bindCleanupDependencies([ + primary, + producer, + ...(descriptorCleanup === undefined ? [] : [descriptorCleanup]), + ]); +}; + +/** The exact callback error remains an owner even if verification reclassifies it. */ +const nativeMachoProducerFailure = ( + cause: unknown, +): AnalysisError | undefined => { + if (cause instanceof AnalysisError) return cause; + if (!(cause instanceof NativeMachoSourceVerificationFailure)) + return undefined; + return cause.outcome.kind === "failed" + ? nativeMachoProducerFailure(cause.outcome.cause) + : nativeMachoCompletedFailure(cause.outcome.value); +}; + const nativeMachoSourceFailure = ( cause: unknown, target: BinaryTarget, @@ -1839,6 +1887,31 @@ const nativeMachoSourceFailure = ( ): AnalysisError => { const { observation, completedFailure, completionCancellation } = nativeMachoCompletion(cause, target, parameters, operation); + const source = artifactFilePrimaryFacts(cause); + const producerFailure = + completedFailure ?? nativeMachoProducerFailure(source.cause); + // Classification and ownership are independent: neither a changed source nor + // a released descriptor settles the command's retained cleanup capability. + const retainOwnedCleanup = ( + selected: AnalysisError, + descriptorCleanup?: AnalysisError, + ): AnalysisError => { + if (!producerFailure?.cleanupIncomplete) return selected; + const primary = + selected === descriptorCleanup + ? (descriptorCleanup.cleanupPrimary ?? producerFailure) + : selected === completionCancellation + ? producerFailure + : selected; + if (completionCancellation !== undefined) + analysisErrorWithCancellation(primary, completionCancellation); + return nativeFailureWithCommandCleanup( + primary, + producerFailure, + operation, + descriptorCleanup, + ); + }; if (cause instanceof ArtifactReaderFailure && cause.cleanup !== undefined) { const physical = cause instanceof ArtifactFileCleanupFailure @@ -1887,17 +1960,15 @@ const nativeMachoSourceFailure = ( if (observation !== undefined) completionCancellation?.retainPartialObservation(observation); bindArtifactFileCleanup(cause, failure); - return failure; + return retainOwnedCleanup(failure, failure); } if (completionCancellation !== undefined) { if (observation !== undefined) completionCancellation.retainPartialObservation(observation); - return completionCancellation; + return retainOwnedCleanup(completionCancellation); } - return nativeMachoSourcePrimary( - artifactFilePrimaryFacts(cause), - target.path, - operation, + return retainOwnedCleanup( + nativeMachoSourcePrimary(source, target.path, operation), ); }; diff --git a/src/native/NativeMachoSourceProvider.test.ts b/src/native/NativeMachoSourceProvider.test.ts index d2c833f052..e8f034cfd8 100644 --- a/src/native/NativeMachoSourceProvider.test.ts +++ b/src/native/NativeMachoSourceProvider.test.ts @@ -1,9 +1,14 @@ +import { EventEmitter } from "node:events"; import { open, writeFile, type FileHandle } from "node:fs/promises"; import { join } from "node:path"; import { expect, it, onTestFinished, vi } from "vitest"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { NativeMacOSProvider } from "./NativeMacOSProvider.js"; +import { NativeCommandFailure, XcrunCommandRunner } from "./CommandRunner.js"; +import { ProviderProcessSupervisor } from "../process/ProviderProcess.js"; +import { err } from "../domain/result.js"; +import { primaryAnalysisError } from "../domain/analysisErrorBase.js"; import { MachoCommandNamesLimitError } from "./MachoCommandNames.js"; import { NativeFixtureRunner, @@ -490,3 +495,165 @@ it.each([ } }, ); + +it.each( + (["inspect_macho", "list_architectures"] as const).flatMap((operation) => [ + ...[false, true].flatMap((cancelDuringClose) => + [false, true].map((failDescriptorClose) => ({ + operation, + cancelDuringClose, + failDescriptorClose, + changeTarget: false, + })), + ), + { + operation, + cancelDuringClose: false, + failDescriptorClose: false, + changeTarget: true, + }, + ]), +)( + "keeps $operation command cleanup live through finalization (cancel: $cancelDuringClose, failed close: $failDescriptorClose, changed target: $changeTarget)", + async ({ + operation, + cancelDuringClose, + failDescriptorClose, + changeTarget, + }) => { + const root = await createTestTempDirectory("rea-macho-command-owner-"); + const path = join(root, "fixture.macho"); + await writeFile(path, inspectionImage()); + const target = await nativeMachoTargetForFile(path); + // The exited event handle and cleanup callback are inert; no process starts. + const process = Object.assign(new EventEmitter(), { + exitCode: 1, + signalCode: null, + stdout: null, + stderr: null, + kill: () => { + throw new Error("Exited fixture must not be signalled"); + }, + }); + let mayClean = false; + let cleanupAttempts = 0; + const owner = new ProviderProcessSupervisor({ + process, + ownsProcessLifetime: true, + cleanup: async () => { + cleanupAttempts++; + return mayClean + ? { cleaned: true, signaled: false } + : { cleaned: false, reason: "worker remains owned" }; + }, + }); + process.emit("close", 1, null); + const physical = new Error("physical command read failure"); + const failure = new NativeCommandFailure( + "lipo", + "io", + 1, + { cause: physical, cleanupOwner: owner }, + { + stdout: "partial native output", + stderr: "read failed", + stdoutBytes: 21, + stderrBytes: 11, + exitCode: 1, + signal: null, + truncated: false, + }, + "worker remains owned", + ["native-command-process"], + ); + const runner = new XcrunCommandRunner({}, async () => { + if (changeTarget) await writeFile(path, "changed Mach-O source bytes"); + return err(failure); + }); + const client = new NativeMacOSProvider({}, runner, "darwin").createClient( + target, + ); + const controller = new AbortController(); + const reason = { request: "cancel during descriptor close" }; + let descriptorCloseFails = failDescriptorClose; + const probe = await open(path, "r"); + const prototype = Object.getPrototypeOf(probe) as FileHandle; + await probe.close(); + const original = prototype.stat; + const stat = vi + .spyOn(prototype, "stat") + .mockImplementationOnce(async function ( + this: FileHandle, + ...args: Parameters + ) { + const close = this.close.bind(this); + this.close = async () => { + if (cancelDuringClose) controller.abort(reason); + if (descriptorCloseFails) throw new Error("descriptor remains owned"); + await close(); + }; + return Reflect.apply(original, this, args); + }); + onTestFinished(async () => { + stat.mockRestore(); + descriptorCloseFails = false; + mayClean = true; + expect((await client.close()).ok).toBe(true); + }); + const result = await client.execute( + operation, + {}, + { signal: controller.signal }, + ); + if (result.ok) throw new Error("Expected command failure"); + expect(result.error._tag).toBe("ProviderAdapterError"); + const primary = primaryAnalysisError(result.error); + if (changeTarget) { + expect(primary._tag).toBe("AnalysisArtifactChangedError"); + expect(primary.diagnostics).toMatchObject({ + previous_error: { + details: { + captured_output: { + stdout: "partial native output", + stderr: "read failed", + }, + }, + }, + }); + } else { + expect(primary.cause).toBeInstanceOf(NativeCommandFailure); + if (!(primary.cause instanceof NativeCommandFailure)) + throw new Error("Expected the runner's physical command failure"); + expect(primary.cause.cause).toBe(failure); + expect(primary.capturedOutput).toMatchObject({ + stdout: "partial native output", + stderr: "read failed", + }); + } + expect(result.error.cleanupIncomplete).toBe(true); + expect(result.error.cleanupResources.toSorted()).toEqual( + [...(failDescriptorClose ? [path] : []), "native-command-process"].sort(), + ); + expect(primary.secondaryFailures).toEqual( + cancelDuringClose + ? [ + expect.objectContaining({ + _tag: "AnalysisCancelledError", + cause: reason, + }), + ] + : [], + ); + descriptorCloseFails = false; + const blocked = await client.close(); + expect(blocked.ok).toBe(false); + expect(result.error.cleanupIncomplete).toBe(true); + expect(result.error.cleanupResources).toEqual(["native-command-process"]); + expect(cleanupAttempts).toBe(1); + mayClean = true; + expect((await client.close()).ok).toBe(true); + expect(cleanupAttempts).toBe(2); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cleanupResources).toEqual([]); + }, +); diff --git a/src/native/SignatureTargetBinding.test.ts b/src/native/SignatureTargetBinding.test.ts index aa9db2a9e9..e6b87a77cd 100644 --- a/src/native/SignatureTargetBinding.test.ts +++ b/src/native/SignatureTargetBinding.test.ts @@ -1,3 +1,4 @@ +import { EventEmitter } from "node:events"; import { createHash } from "node:crypto"; import { chmod, @@ -19,7 +20,12 @@ import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import type { BinaryTarget } from "../domain/binaryTargetTypes.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { ok, err } from "../domain/result.js"; -import type { NativeCommandRunner } from "./CommandRunner.js"; +import { + NativeCommandFailure, + XcrunCommandRunner, + type NativeCommandRunner, +} from "./CommandRunner.js"; +import { ProviderProcessSupervisor } from "../process/ProviderProcess.js"; import type { NativeCallTracer } from "./LldbCallTracer.js"; import { NativeMacOSProvider } from "./NativeMacOSProvider.js"; import { @@ -27,7 +33,10 @@ import { withSignatureTarget, } from "./SignatureTargetBinding.js"; import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; -import { resolvedAnalysisError } from "../domain/analysisErrorBase.js"; +import { + primaryAnalysisError, + resolvedAnalysisError, +} from "../domain/analysisErrorBase.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { AnalysisOutputError } from "../domain/analysisErrorCore.js"; import { signatureInspectionFixture } from "../../tests/fixtures/nativeCommands.js"; @@ -645,3 +654,180 @@ it("does not adopt an inspection's arbitrary thrown cleanup marker", async () => expect(result.error.cleanupResources).toEqual([]); await resources.close(); }); + +it.each(["io", "cancelled"] as const)( + "preserves a codesign %s failure when later target verification observes cancellation", + async (kind) => { + const target = await createTarget("registered source bytes"); + const controller = new AbortController(); + const reason = { caller: "cancel after codesign result", kind }; + const output = { + stdout: "physical output", + stderr: "physical diagnostic", + stdoutBytes: 15, + stderrBytes: 19, + exitCode: null, + signal: null, + truncated: false, + }; + const failure = new NativeCommandFailure( + "codesign", + kind, + null, + { cancellation: { reason } }, + output, + "command cleanup remains unknown", + ["command-owner"], + ); + const runner: NativeCommandRunner = { + run: async () => { + controller.abort(reason); + return err(failure); + }, + }; + const client = new NativeMacOSProvider( + {}, + runner, + "darwin", + () => unusedTracer, + createRoot, + ).createClient(target); + try { + const result = await client.execute( + "inspect_signature", + {}, + { signal: controller.signal }, + ); + if (result.ok) throw new Error("Expected codesign failure"); + expect(result.error._tag).toBe( + kind === "cancelled" + ? "AnalysisCancelledError" + : "ProviderAdapterError", + ); + expect(result.error.capturedOutput).toMatchObject({ + stdout: output.stdout, + stderr: output.stderr, + }); + expect(result.error.cleanupIncomplete).toBe(true); + expect(result.error.cleanupResources).toEqual(["command-owner"]); + if (kind === "cancelled") expect(result.error.cause).toBe(reason); + else { + expect(result.error.cause).toBe(failure); + expect(result.error.secondaryFailures).toContainEqual( + expect.objectContaining({ + _tag: "AnalysisCancelledError", + cause: reason, + }), + ); + } + } finally { + await client.close(); + } + }, +); + +it.each(["unchanged", "cancelled", "changed"] as const)( + "keeps a codesign cleanup marker live after %s target verification", + async (outcome) => { + const target = await createTarget("registered source bytes"); + const controller = new AbortController(); + const reason = { request: "cancel after native command failure" }; + const process = Object.assign(new EventEmitter(), { + exitCode: 1, + signalCode: null, + stdout: null, + stderr: null, + kill: () => { + throw new Error("Exited fixture must not be signalled"); + }, + }); + let mayClean = false; + let cleanupAttempts = 0; + const owner = new ProviderProcessSupervisor({ + process, + ownsProcessLifetime: true, + cleanup: async () => { + cleanupAttempts++; + return mayClean + ? { cleaned: true, signaled: false } + : { cleaned: false, reason: "codesign worker remains owned" }; + }, + }); + process.emit("close", 1, null); + const failure = new NativeCommandFailure( + "codesign", + "io", + 1, + { cleanupOwner: owner }, + { + stdout: "physical output", + stderr: "physical diagnostic", + stdoutBytes: 15, + stderrBytes: 19, + exitCode: 1, + signal: null, + truncated: false, + }, + "codesign worker remains owned", + ["native-command-process"], + ); + const runner = new XcrunCommandRunner({}, async () => { + if (outcome === "cancelled") controller.abort(reason); + if (outcome === "changed") + await writeFile(target.path, "changed source bytes"); + return err(failure); + }); + const client = new NativeMacOSProvider( + {}, + runner, + "darwin", + () => unusedTracer, + createRoot, + ).createClient(target); + onTestFinished(async () => { + mayClean = true; + expect((await client.close()).ok).toBe(true); + }); + const result = await client.execute( + "inspect_signature", + {}, + { signal: controller.signal }, + ); + if (result.ok) throw new Error("Expected codesign failure"); + const primary = primaryAnalysisError(result.error); + expect(primary._tag).toBe( + outcome === "changed" + ? "AnalysisArtifactChangedError" + : "ProviderAdapterError", + ); + if (outcome !== "changed") { + if (!(primary.cause instanceof NativeCommandFailure)) + throw new Error("Expected physical codesign failure"); + expect(primary.cause.cause).toBe(failure); + expect(primary.capturedOutput).toMatchObject({ + stdout: "physical output", + stderr: "physical diagnostic", + }); + } + expect(result.error.cleanupIncomplete).toBe(true); + expect(result.error.cleanupResources).toEqual(["native-command-process"]); + expect(primary.secondaryFailures).toEqual( + outcome === "cancelled" + ? [ + expect.objectContaining({ + _tag: "AnalysisCancelledError", + cause: reason, + }), + ] + : [], + ); + expect((await client.close()).ok).toBe(false); + expect(result.error.cleanupIncomplete).toBe(true); + expect(cleanupAttempts).toBe(1); + mayClean = true; + expect((await client.close()).ok).toBe(true); + expect(cleanupAttempts).toBe(2); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cleanupResources).toEqual([]); + }, +);