diff --git a/src/native/SignatureTargetBinding.test.ts b/src/native/SignatureTargetBinding.test.ts index a5d91f2531..171f0dd7b8 100644 --- a/src/native/SignatureTargetBinding.test.ts +++ b/src/native/SignatureTargetBinding.test.ts @@ -2,7 +2,7 @@ import { createHash } from "node:crypto"; import { mkdtemp, rm, stat, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { afterEach, expect, it } from "vitest"; +import { afterEach, expect, it, vi, onTestFinished } from "vitest"; import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import type { BinaryTarget } from "../domain/binaryTargetTypes.js"; @@ -12,6 +12,8 @@ import type { NativeCommandRunner } from "./CommandRunner.js"; import type { NativeCallTracer } from "./LldbCallTracer.js"; import { NativeMacOSProvider } from "./NativeMacOSProvider.js"; import { withSignatureTarget } from "./SignatureTargetBinding.js"; +import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; +import { resolvedAnalysisError } from "../domain/analysisErrorBase.js"; const roots: string[] = []; const targets: string[] = []; @@ -92,6 +94,11 @@ it("retains a snapshot root after cleanup fails alongside a digest mismatch", as }); await resources.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cleanupResources).toEqual([]); + expect(resolvedAnalysisError(result.error)._tag).toBe( + "AnalysisArtifactChangedError", + ); await expect(stat(roots[0]!)).rejects.toMatchObject({ code: "ENOENT" }); }); @@ -155,6 +162,12 @@ it("retries a retained signature root when the provider client closes", async () ok: true, value: null, }); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cleanupResources).toEqual([]); + if (!firstClose.ok) { + expect(firstClose.error.cleanupIncomplete).toBe(false); + expect(firstClose.error.cleanupResources).toEqual([]); + } await expect(stat(roots[0]!)).rejects.toMatchObject({ code: "ENOENT" }); }); @@ -196,3 +209,63 @@ it("waits for an in-flight signature binding before provider close", async () => expect(closeSettled).toBe(true); await expect(stat(roots[0]!)).rejects.toMatchObject({ code: "ENOENT" }); }); + +it("tracks mixed snapshot descriptor and root cleanup through partial release and retry", async () => { + const target = await createTarget("observed", "expected"); + const resources = new ArtifactResourceScope(); + const original = OwnedFileHandle.prototype.close; + let blocked: OwnedFileHandle | undefined; + let attempts = 0; + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(function (this: OwnedFileHandle) { + blocked ??= this; + if (this === blocked && ++attempts <= 2) + return Promise.reject( + new Error("controlled snapshot descriptor close failure"), + ); + return original.call(this); + }); + onTestFinished(async () => { + close.mockRestore(); + await resources.close(); + }); + const inspect = vi.fn(async () => ok("must not run")); + const first = await withSignatureTarget( + target, + resources, + inspect, + undefined, + () => createRoot(1), + ); + expect(first.ok).toBe(false); + if (first.ok) throw new Error("Expected both retained owners"); + const snapshot = join(roots[0]!, "target.bin"); + expect(new Set(first.error.cleanupResources)).toEqual( + new Set([snapshot, roots[0]!]), + ); + const retried = await withSignatureTarget( + target, + resources, + inspect, + undefined, + createRoot, + ); + expect(retried.ok).toBe(false); + if (retried.ok) throw new Error("Expected retained descriptor retry failure"); + expect(first.error.cleanupIncomplete).toBe(true); + expect(first.error.cleanupResources).toEqual([snapshot]); + expect(retried.error.cleanupResources).toEqual([snapshot]); + expect(roots).toHaveLength(1); + expect(inspect).not.toHaveBeenCalled(); + await expect(stat(roots[0]!)).rejects.toMatchObject({ code: "ENOENT" }); + await resources.close(); + expect(attempts).toBe(3); + for (const failure of [first.error, retried.error]) { + expect(failure.cleanupIncomplete).toBe(false); + expect(failure.cleanupResources).toEqual([]); + } + expect(resolvedAnalysisError(first.error)._tag).toBe( + "AnalysisArtifactChangedError", + ); +}); diff --git a/src/native/SignatureTargetBinding.ts b/src/native/SignatureTargetBinding.ts index 29c8651e61..97c5b67ceb 100644 --- a/src/native/SignatureTargetBinding.ts +++ b/src/native/SignatureTargetBinding.ts @@ -19,8 +19,22 @@ import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { PrivateRuntimeRoot } from "../process/PrivateRuntimeRoot.js"; import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../artifacts/AnalysisCleanupLease.js"; +import { bindArtifactFileCleanup } from "../artifacts/ArtifactFileOperation.js"; +import type { ArtifactResourceOwner } from "../artifacts/ArtifactResourceScope.js"; + import { NATIVE_MACOS_PROVIDER_IDENTITY } from "./NativeMacOSProviderMetadata.js"; +type SignatureOwner = Extract< + ArtifactResourceOwner, + { readonly kind: "lease" } +> & { + readonly lease: AnalysisCleanupLease; +}; + /** Binding of commands to the selected executable's registered content/version. */ export interface SignatureTargetBinding { readonly identity: string; @@ -95,9 +109,9 @@ export const withSignatureTarget = async ( : snapshotFailure(target, "create", cause), ); } - const owner = { - kind: "private-runtime-root" as const, - root, + const owner: SignatureOwner = { + kind: "lease", + lease: new AnalysisCleanupLease(() => root.close(), [root.path]), resource: root.path, }; let result: Result; @@ -132,14 +146,16 @@ export const withSignatureTarget = async ( ); } const cleanup = await resources.release(owner); - if (cleanup.kind === "failed") - return combineCleanupFailure( + if (cleanup.kind === "failed") { + const failure = signatureCleanupFailure( target, - result, [root.path], cleanup.cause, "cleanup", ); + new AnalysisCleanupLeaseGroup(failure, [owner.lease]); + return combineCleanupFailure(result, failure); + } return result; }); } catch (cause: unknown) { @@ -150,15 +166,12 @@ export const withSignatureTarget = async ( : undefined; if (retained !== undefined) return combineCleanupFailure( - target, err( signal?.aborted ? new AnalysisCancelledError("inspect_signature") : signatureReadFailure(target, cause), ), - retained.resources, - cause, - "retry", + signatureCleanupFailure(target, retained.resources, cause, "retry"), ); return err( signal?.aborted @@ -168,14 +181,28 @@ export const withSignatureTarget = async ( } }; +/** Each composed error follows the exact primary and secondary cleanup owners. */ const combineCleanupFailure = ( - target: BinaryTarget, primary: Result, + cleanup: AnalysisError, +): Result => + primary.ok + ? err(cleanup) + : err( + analysisErrorWithCleanupFailure( + primary.error, + cleanup, + "inspect_signature", + ).bindCleanupDependencies([primary.error, cleanup]), + ); + +const signatureCleanupFailure = ( + target: BinaryTarget, resources: readonly string[], cause: unknown, phase: string, -): Result => { - const cleanup = new ProviderCleanupError( +): AnalysisError => { + const failure = new ProviderCleanupError( NATIVE_MACOS_PROVIDER_IDENTITY.id, resources, { @@ -185,15 +212,8 @@ const combineCleanupFailure = ( }, { operation: "inspect_signature", cause }, ); - return primary.ok - ? err(cleanup) - : err( - analysisErrorWithCleanupFailure( - primary.error, - cleanup, - "inspect_signature", - ), - ); + bindArtifactFileCleanup(cause, failure); + return failure; }; const snapshotFailure = ( @@ -274,8 +294,8 @@ const bindSignatureTarget = async ( resources: ArtifactResourceScope, signal?: AbortSignal, ): Promise> => { - let sourceOwner: OwnedFileHandle | undefined; - let snapshotOwner: OwnedFileHandle | undefined; + let sourceOwner: SignatureOwner | undefined; + let snapshotOwner: SignatureOwner | undefined; const perform = async (): Promise< Result > => { @@ -292,7 +312,15 @@ const bindSignatureTarget = async ( target.path, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK, ); - sourceOwner = new OwnedFileHandle(file); + const sourceHandle = new OwnedFileHandle(file); + sourceOwner = { + kind: "lease", + resource: target.path, + lease: new AnalysisCleanupLease( + () => sourceHandle.close(), + [target.path], + ), + }; const opened = await file.stat({ bigint: true }); signal?.throwIfAborted(); if (identity(opened) !== identity(before)) @@ -303,7 +331,15 @@ const bindSignatureTarget = async ( const snapshot = await snapshotIO(target, "create-file", () => open(snapshotPath, "wx", 0o600), ); - snapshotOwner = new OwnedFileHandle(snapshot); + const snapshotHandle = new OwnedFileHandle(snapshot); + snapshotOwner = { + kind: "lease", + resource: snapshotPath, + lease: new AnalysisCleanupLease( + () => snapshotHandle.close(), + [snapshotPath], + ), + }; const observed = await copySignatureBytes( target, { file, stat: opened }, @@ -338,34 +374,31 @@ const bindSignatureTarget = async ( }; let result = await perform(); const cleanupFailures: { - readonly path: string; + readonly owner: SignatureOwner; readonly cause: unknown; }[] = []; - for (const [owner, path] of [ - [snapshotOwner, snapshotPath], - [sourceOwner, target.path], - ] as const) { + for (const owner of [snapshotOwner, sourceOwner]) { if (owner === undefined) continue; - const cleanup = await resources.release({ - kind: "file-handle", - handle: owner, - resource: path, - }); + const cleanup = await resources.release(owner); if (cleanup.kind === "failed") - cleanupFailures.push({ path, cause: cleanup.cause }); + cleanupFailures.push({ owner, cause: cleanup.cause }); } if (cleanupFailures.length > 0) { const cause = new AggregateError( cleanupFailures.map(({ cause: failure }) => failure), "Signature snapshot file handles could not be closed", ); - result = combineCleanupFailure( + const cleanup = signatureCleanupFailure( target, - result, - cleanupFailures.map(({ path }) => path), + cleanupFailures.map(({ owner }) => owner.resource), cause, "close", ); + new AnalysisCleanupLeaseGroup( + cleanup, + cleanupFailures.map(({ owner }) => owner.lease), + ); + result = combineCleanupFailure(result, cleanup); } return result; };