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)