From 9e259f87c8be3d22a0b96f12ed7838830869c987 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:24:40 +0800 Subject: [PATCH] fix(native): retain signature roots until descriptor cleanup completes Give each signature snapshot a private child-resource scope. Its composite root lease closes those children before removing the directory and retains both stages across failed retries. Bind local cleanup failures to their exact owner so child release cannot clear a failed root removal. Replace the mixed-outcome regression that expected premature root removal. Cover real descriptor denial, primary/cancellation preservation, MCP close and same-connection recovery. Expose underlying real codesign test errors before asserting result fields. Fixes #5052 --- src/native/SignatureTargetBinding.test.ts | 68 ++++++++++- src/native/SignatureTargetBinding.ts | 34 ++++-- .../nativeSignatureTargetBindingMcp.test.ts | 111 +++++++++++++++++- .../nativeSignatureTargetBinding.test.ts | 7 +- 4 files changed, 200 insertions(+), 20 deletions(-) diff --git a/src/native/SignatureTargetBinding.test.ts b/src/native/SignatureTargetBinding.test.ts index 5bcf8ff971..ed019fafc6 100644 --- a/src/native/SignatureTargetBinding.test.ts +++ b/src/native/SignatureTargetBinding.test.ts @@ -223,7 +223,7 @@ it("waits for an in-flight signature binding before provider close", async () => await expect(stat(roots[0]!)).rejects.toMatchObject({ code: "ENOENT" }); }); -it("tracks mixed snapshot descriptor and root cleanup through partial release and retry", async () => { +it("releases retained descriptors before retrying their containing root", async () => { const target = await createTarget("observed", "expected"); const resources = new ArtifactResourceScope(); const original = OwnedFileHandle.prototype.close; @@ -265,15 +265,16 @@ it("tracks mixed snapshot descriptor and root cleanup through partial release an createRoot, ); expect(retried.ok).toBe(false); - if (retried.ok) throw new Error("Expected retained descriptor retry failure"); + if (retried.ok) throw new Error("Expected retained root retry failure"); expect(first.error.cleanupIncomplete).toBe(true); - expect(first.error.cleanupResources).toEqual([snapshot]); - expect(retried.error.cleanupResources).toEqual([snapshot]); + expect(first.error.cleanupResources).toEqual([roots[0]!]); + expect(retried.error.cleanupResources).toEqual([roots[0]!]); expect(roots).toHaveLength(1); expect(inspect).not.toHaveBeenCalled(); - await expect(stat(roots[0]!)).rejects.toMatchObject({ code: "ENOENT" }); + expect((await stat(roots[0]!)).isDirectory()).toBe(true); await resources.close(); expect(attempts).toBe(3); + await expect(stat(roots[0]!)).rejects.toMatchObject({ code: "ENOENT" }); for (const failure of [first.error, retried.error]) { expect(failure.cleanupIncomplete).toBe(false); expect(failure.cleanupResources).toEqual([]); @@ -283,6 +284,57 @@ it("tracks mixed snapshot descriptor and root cleanup through partial release an ); }); +it("retains the snapshot root until its failed descriptor owner confirms release", async () => { + const target = await createTarget("observed", "expected"); + const resources = new ArtifactResourceScope(); + const original = OwnedFileHandle.prototype.close; + let blocked: OwnedFileHandle | undefined; + let allowClose = false; + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(function (this: OwnedFileHandle) { + blocked ??= this; + if (this === blocked && !allowClose) + return Promise.reject(new Error("snapshot descriptor remains open")); + return original.call(this); + }); + const root = await createRoot(); + const removeRoot = vi.fn(root.close); + onTestFinished(async () => { + allowClose = true; + await resources.close(); + close.mockRestore(); + }); + const result = await withSignatureTarget( + target, + resources, + unusedInspection, + undefined, + async () => ({ path: root.path, close: removeRoot }), + ); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("Expected retained snapshot descriptor"); + expect(removeRoot).not.toHaveBeenCalled(); + expect((await stat(root.path)).isDirectory()).toBe(true); + const retry = await resources.close().then( + () => undefined, + (cause: unknown) => cause, + ); + expect(retry).toBeDefined(); + expect(removeRoot).not.toHaveBeenCalled(); + expect((await stat(root.path)).isDirectory()).toBe(true); + expect(result.error.cleanupIncomplete).toBe(true); + allowClose = true; + await resources.close(); + expect(removeRoot).toHaveBeenCalledTimes(1); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cleanupResources).toEqual([]); + expect(resolvedAnalysisError(result.error)._tag).toBe( + "AnalysisArtifactChangedError", + ); + await expect(stat(root.path)).rejects.toMatchObject({ code: "ENOENT" }); +}); + const opaqueReasons = [ null, 0, @@ -339,12 +391,15 @@ for (const ownerIndex of [0, 1]) const reason = { stage: "descriptor-close" }; const original = OwnedFileHandle.prototype.close; let calls = 0; + let blocked: OwnedFileHandle | undefined; + let allowClose = false; const close = vi .spyOn(OwnedFileHandle.prototype, "close") .mockImplementation(async function (this: OwnedFileHandle) { const cancels = calls++ === ownerIndex; + if (cancels && cleanupFails) blocked = this; try { - if (cancels && cleanupFails) + if (this === blocked && !allowClose) throw new Error("controlled close failure"); await original.call(this); } finally { @@ -368,6 +423,7 @@ for (const ownerIndex of [0, 1]) throw new Error("Expected descriptor finalization cancellation"); expect(inspect).not.toHaveBeenCalled(); expect(result.error.cleanupIncomplete).toBe(cleanupFails); + allowClose = true; await resources.close(); expect(resolvedAnalysisError(result.error)).toMatchObject({ _tag: "AnalysisCancelledError", diff --git a/src/native/SignatureTargetBinding.ts b/src/native/SignatureTargetBinding.ts index 2b2d6e062b..b16b2203d9 100644 --- a/src/native/SignatureTargetBinding.ts +++ b/src/native/SignatureTargetBinding.ts @@ -118,9 +118,20 @@ export const withSignatureTarget = async ( : snapshotFailure(target, "create", cause), ); } + const children = new ArtifactResourceScope(); const owner: SignatureOwner = { kind: "lease", - lease: new AnalysisCleanupLease(() => root.close(), [root.path]), + lease: new AnalysisCleanupLease( + async () => { + await children.close(); + await root.close(); + }, + { + get cleanupResources() { + return [...children.cleanupResources, root.path]; + }, + }, + ), resource: root.path, }; let result: Result; @@ -128,7 +139,7 @@ export const withSignatureTarget = async ( const binding = await bindSignatureTarget( target, join(root.path, basename(target.path)), - resources, + children, signal, ); if (!binding.ok) result = binding; @@ -173,11 +184,19 @@ export const withSignatureTarget = async ( cause instanceof ArtifactReaderFailure && cause.cleanup !== undefined ? cause.cleanup : undefined; - if (retained !== undefined) + if (retained !== undefined) { + const failure = signatureCleanupFailure( + target, + retained.resources, + cause, + "retry", + ); + bindArtifactFileCleanup(cause, failure); return combineCleanupFailure( err(signatureReadFailure(target, cause)), - signatureCleanupFailure(target, retained.resources, cause, "retry"), + failure, ); + } return err(signatureReadFailure(target, cause)); } }; @@ -202,8 +221,8 @@ const signatureCleanupFailure = ( resources: readonly string[], cause: unknown, phase: string, -): AnalysisError => { - const failure = new ProviderCleanupError( +): AnalysisError => + new ProviderCleanupError( NATIVE_MACOS_PROVIDER_IDENTITY.id, resources, { @@ -213,9 +232,6 @@ const signatureCleanupFailure = ( }, { operation: "inspect_signature", cause }, ); - bindArtifactFileCleanup(cause, failure); - return failure; -}; const snapshotFailure = ( target: BinaryTarget, diff --git a/tests/boundary/mcp/nativeSignatureTargetBindingMcp.test.ts b/tests/boundary/mcp/nativeSignatureTargetBindingMcp.test.ts index 5d8f9f8287..e8ea4d756d 100644 --- a/tests/boundary/mcp/nativeSignatureTargetBindingMcp.test.ts +++ b/tests/boundary/mcp/nativeSignatureTargetBindingMcp.test.ts @@ -1,8 +1,9 @@ import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; -import { writeFile } from "node:fs/promises"; +import { lstat, mkdir, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; -import { expect, it } from "vitest"; +import { expect, it, vi } from "vitest"; +import { OwnedFileHandle } from "../../../src/filesystem/OwnedFileHandle.js"; import { NativeMacOSProvider } from "../../../src/native/NativeMacOSProvider.js"; import { machoImage, @@ -16,6 +17,112 @@ import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { NativeFixtureRunner } from "../../fixtures/nativeCommands.js"; import { toolContract } from "../../../src/contracts/toolContracts.js"; +it("keeps a failed snapshot descriptor and root together through MCP close and recovery", async () => { + const root = await createTestTempDirectory("rea-signature-order-mcp-"); + const path = join(root, "program"); + await writeFile(path, machoImage({})); + const runner = new NativeFixtureRunner(); + const run = vi.spyOn(runner, "run"); + let created = 0; + let removed = 0; + const provider = new NativeMacOSProvider( + {}, + runner, + "darwin", + undefined, + async () => { + const path = join(root, `snapshot-${String(++created)}`); + await mkdir(path, { mode: 0o700 }); + return { + path, + close: async () => { + await rm(path, { recursive: true }); + removed++; + }, + }; + }, + ); + const server = createServer({ + kind: "session", + session: createTestBinarySession(provider), + }); + const client = new Client({ name: "signature-order-test", version: "1" }); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + const original = OwnedFileHandle.prototype.close; + let blocked: OwnedFileHandle | undefined; + let allowClose = false; + let close: ReturnType | undefined; + try { + await server.connect(serverTransport); + await client.connect(clientTransport); + expect( + (await client.callTool({ name: "open_binary", arguments: { path } })) + .isError, + ).not.toBe(true); + close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(function (this: OwnedFileHandle) { + blocked ??= this; + if (this === blocked && !allowClose) + return Promise.reject(new Error("snapshot descriptor close denied")); + return original.call(this); + }); + for (const name of [ + "inspect_signature", + "inspect_signature", + "close_binary", + ]) { + const reply = await client.callTool({ name, arguments: {} }); + const error = parseMcpToolError(reply).error; + expect(error.code).toBe("cleanup_incomplete"); + expect(error.details).toMatchObject({ + cleanup: "incomplete", + resources: expect.arrayContaining([ + join(root, "snapshot-1"), + join(root, "snapshot-1", "program"), + ]), + }); + expect(created).toBe(1); + expect(removed).toBe(0); + expect((await lstat(join(root, "snapshot-1"))).isDirectory()).toBe(true); + expect(run).not.toHaveBeenCalled(); + await client.ping(); + } + allowClose = true; + expect( + (await client.callTool({ name: "close_binary", arguments: {} })).isError, + ).not.toBe(true); + expect(removed).toBe(1); + await expect(lstat(join(root, "snapshot-1"))).rejects.toMatchObject({ + code: "ENOENT", + }); + expect( + (await client.callTool({ name: "open_binary", arguments: { path } })) + .isError, + ).not.toBe(true); + const recovered = await client.callTool({ + name: "inspect_signature", + arguments: {}, + }); + expect(recovered.isError, JSON.stringify(recovered.content)).not.toBe(true); + expect(recovered.structuredContent).toMatchObject({ + normalized_result: { signed: true, identifier: "com.example.fixture" }, + }); + expect(created).toBe(2); + expect(removed).toBe(2); + } finally { + allowClose = true; + try { + await client.close(); + await server.close(); + } finally { + close?.mockRestore(); + run.mockRestore(); + } + } +}); + it.each(["mach-o", "mach-o-universal"] as const)( "rejects changed %s signature targets through MCP and accepts reopening the new version", async (format) => { diff --git a/tests/boundary/providers/native/nativeSignatureTargetBinding.test.ts b/tests/boundary/providers/native/nativeSignatureTargetBinding.test.ts index 56f2892a73..66f00104e2 100644 --- a/tests/boundary/providers/native/nativeSignatureTargetBinding.test.ts +++ b/tests/boundary/providers/native/nativeSignatureTargetBinding.test.ts @@ -68,7 +68,8 @@ describe("native signature target identity", () => { const result = await new NativeMacOSProvider({}, new Captures(), "darwin") .createClient(target) .execute("inspect_signature", {}); - expect(result.ok && result.value.result).toMatchObject({ + if (!result.ok) throw result.error; + expect(result.value.result).toMatchObject({ signed: true, identifier: "com.example.fixture", }); @@ -477,11 +478,11 @@ it.skipIf(process.platform !== "darwin")( const result = await new NativeMacOSProvider({}, runner, "darwin") .createClient(target) .execute("inspect_signature", {}); - expect(result.ok && result.value.result).toMatchObject({ + if (!result.ok) throw result.error; + expect(result.value.result).toMatchObject({ signed: true, identifier: "rea.original", }); - if (!result.ok) throw result.error; expect( inspectSignatureSchema .parse(result.value.result)