From 1f23e977b50301e91f02a4a17c9f8d48bb7b60f3 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 18 Oct 2026 10:50:48 +0800 Subject: [PATCH] fix(pwntools): retain exact cleanup owners through finalization Give retained processes and runtime roots separate cleanup leases. Bind composed failures to their exact dependencies so process retirement removes only process labels and root deletion resolves the remaining markers. Retain local evidence while descriptor cleanup is unresolved. Preserve completed decoded facts and the original cancellation reason when final cleanup itself fails, then expose that primary after successful retry. Remove the superseded root-error composition branch after descriptor gating. Validation: 68 focused tests across nine files, including real retained file handles, late process handshakes, failed final cleanup, mixed child outcomes and cleanup retry. Static, docs (five checks) and discovery (1577 files) pass. The producer qualification scope is controlled Node/file fixtures with the real macOS inspector and a simulated availability gate, not native pwntools or GDB. Task tokens/CPU/process-family memory are unmeasured; no speed claim. Fixes #4945 --- src/native/pwntools/PwntoolsDecoder.ts | 111 ++++++++++------ tests/process-global/pwntoolsCleanup.test.ts | 29 ++++- .../pwntoolsCliFinalization.test.ts | 48 +++++++ .../pwntoolsStageRetirement.test.ts | 122 +++++++++++++++++- 4 files changed, 261 insertions(+), 49 deletions(-) diff --git a/src/native/pwntools/PwntoolsDecoder.ts b/src/native/pwntools/PwntoolsDecoder.ts index a0b9682487..489d9fc792 100644 --- a/src/native/pwntools/PwntoolsDecoder.ts +++ b/src/native/pwntools/PwntoolsDecoder.ts @@ -1,3 +1,7 @@ +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../../artifacts/AnalysisCleanupLease.js"; import { bindArtifactFileCleanup, artifactFileCancellationCause, @@ -113,7 +117,13 @@ export class PwntoolsDecoder< #pendingCleanup: | { root: Pick | undefined; - process: ProviderProcessSupervisor | undefined; + rootLease: AnalysisCleanupLease; + process: + | { + readonly supervisor: ProviderProcessSupervisor; + readonly lease: AnalysisCleanupLease; + } + | undefined; failure: AnalysisError | undefined; diagnostics: AnalysisCapturedOutput | undefined; observation: AnalysisPartialObservation | undefined; @@ -290,6 +300,7 @@ export class PwntoolsDecoder< root = await this.createRuntime(); this.#pendingCleanup = { root, + rootLease: AnalysisCleanupLease.forCloseOwner(root, root.path), process: undefined, failure: undefined, diagnostics: undefined, @@ -487,25 +498,27 @@ export class PwntoolsDecoder< : result.error.capturedOutput; } const cleanup = await this.#completeCleanup(() => this.#retire()); - if (cleanup !== undefined) - return err( - result.ok - ? cleanup - : analysisErrorWithCleanupFailure(result.error, cleanup, OPERATION), + if (result.ok && options?.signal?.aborted) + result = err( + new AnalysisCancelledError(OPERATION, { + cause: options.signal.reason, + partialObservation: result.value, + capturedOutput: result.value.diagnostics, + }), ); - return result.ok && options?.signal?.aborted - ? err( - new AnalysisCancelledError(OPERATION, { - cause: options.signal.reason, - partialObservation: result.value, - capturedOutput: { - stdout: result.value.diagnostics.stdout, - stderr: result.value.diagnostics.stderr, - truncated: result.value.diagnostics.truncated, - }, - }), - ) - : result; + if (cleanup !== undefined) { + const failure = result.ok + ? cleanup + : analysisErrorWithCleanupFailure( + result.error, + cleanup, + OPERATION, + ).bindCleanupDependencies([result.error, cleanup]); + if (this.#pendingCleanup !== undefined) + this.#pendingCleanup.failure = failure; + return err(failure); + } + return result; } async #runCommand( @@ -537,8 +550,21 @@ export class PwntoolsDecoder< ? {} : { signal: options.signal }), ...(this.launcher === undefined ? {} : { launcher: this.launcher }), - retainCleanup: (owner) => { - pending.process = owner; + retainCleanup: (supervisor) => { + const ownership = supervisor.launch.ownership; + pending.process = { + supervisor, + lease: new AnalysisCleanupLease( + async () => { + const stopped = await supervisor.stop(); + if (stopped.status === "incomplete") + throw new Error(stopped.reason, { cause: stopped }); + }, + ownership === undefined + ? ["owned-decoder"] + : ownedCommandResources(ownership), + ), + }; }, }, ), @@ -555,7 +581,7 @@ export class PwntoolsDecoder< primary, cleanup, this.profile.operation, - ); + ).bindCleanupDependencies([primary, cleanup]); } if (!outcome.ok) throw outcome.error; return outcome.value; @@ -590,6 +616,7 @@ export class PwntoolsDecoder< { reason: observation.reason }, { operation: this.profile.operation, cause }, ); + bindArtifactFileCleanup(cause, resourceFailure); failure = failure === undefined ? resourceFailure @@ -597,7 +624,7 @@ export class PwntoolsDecoder< failure, resourceFailure, this.profile.operation, - ); + ).bindCleanupDependencies([failure, resourceFailure]); } if (failure !== undefined) throw failure; }) @@ -626,10 +653,12 @@ export class PwntoolsDecoder< async #retireProcess(): Promise { const pending = this.#pendingCleanup; if (pending?.process === undefined) return undefined; - const stopped = await pending.process.stop(); - const ownership = pending.process.launch.ownership; - if (stopped.status === "incomplete") - return new ProviderCleanupError( + const process = pending.process; + const ownership = process.supervisor.launch.ownership; + try { + await process.lease.release(); + } catch (cause: unknown) { + const failure = new ProviderCleanupError( this.identity.id, ownership === undefined ? [ @@ -638,10 +667,10 @@ export class PwntoolsDecoder< ] : ownedCommandResources(ownership, pending.root?.path), { - reason: stopped.reason, + reason: cause instanceof Error ? cause.message : String(cause), captured_output: { ...(pending.diagnostics ?? - capturedPwntoolsOutput(pending.process.snapshot())), + capturedPwntoolsOutput(process.supervisor.snapshot())), }, previous_error: pending.failure === undefined @@ -650,11 +679,18 @@ export class PwntoolsDecoder< }, { operation: this.profile.operation, + cause, ...(pending.observation === undefined ? {} : { partialObservation: pending.observation }), }, ); + new AnalysisCleanupLeaseGroup(failure, [ + process.lease, + pending.rootLease, + ]); + return failure; + } pending.process = undefined; return undefined; } @@ -681,6 +717,7 @@ export class PwntoolsDecoder< }, ); } + if (failure !== undefined) bindArtifactFileCleanup(failure.cause, failure); const processFailure = await this.#retireProcess(); if (processFailure !== undefined) { return failure === undefined @@ -689,13 +726,15 @@ export class PwntoolsDecoder< failure, processFailure, this.profile.operation, - ); + ).bindCleanupDependencies([failure, processFailure]); } + // Retained descriptors still own local evidence under this root. + if (failure !== undefined) return failure; const pending = this.#pendingCleanup; if (pending?.root !== undefined) { const root = pending.root; try { - await root.close(); + await pending.rootLease.release(); pending.root = undefined; } catch (cause: unknown) { const rootFailure = new ProviderCleanupError( @@ -719,14 +758,8 @@ export class PwntoolsDecoder< : { partialObservation: pending.observation }), }, ); - failure = - failure === undefined - ? rootFailure - : analysisErrorWithCleanupFailure( - failure, - rootFailure, - this.profile.operation, - ); + new AnalysisCleanupLeaseGroup(rootFailure, [pending.rootLease]); + failure = rootFailure; } } if (!this.#resources.hasPending && pending?.root === undefined) diff --git a/tests/process-global/pwntoolsCleanup.test.ts b/tests/process-global/pwntoolsCleanup.test.ts index f976c20832..6260e65f0d 100644 --- a/tests/process-global/pwntoolsCleanup.test.ts +++ b/tests/process-global/pwntoolsCleanup.test.ts @@ -59,6 +59,7 @@ it.runIf(supportedHost)( let pid: number | undefined; let launches = 0; let cleanupAllowed = false; + let rootRemovalAllowed = false; let releaseFixture: (() => Promise) | undefined; const provider = new PwntoolsLayoutProvider( { REA_PWNTOOLS_PYTHON: process.execPath }, @@ -85,11 +86,24 @@ it.runIf(supportedHost)( }), }; }, - () => - PrivateRuntimeRoot.create({ parent: workspace.root, prefix: "owned-" }), + async () => { + const root = await PrivateRuntimeRoot.create({ + parent: workspace.root, + prefix: "owned-", + }); + return { + path: root.path, + close: async () => { + if (!rootRemovalAllowed) + throw new Error("root removal refused after process retirement"); + await root.close(); + }, + }; + }, ); onTestFinished(async () => { cleanupAllowed = true; + rootRemovalAllowed = true; await provider.close(); await releaseFixture?.(); }); @@ -125,8 +139,16 @@ it.runIf(supportedHost)( await expect(access(ownedPath)).resolves.toBeUndefined(); cleanupAllowed = true; - await provider.close(); + await expect(provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); expect(() => process.kill(workerPid, 0)).toThrow(); + expect(result.error.cleanupResources).toEqual([ownedPath]); + expect(result.error.cleanupIncomplete).toBe(true); + rootRemovalAllowed = true; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(refused.error.cleanupIncomplete).toBe(false); await expect(access(ownedPath)).rejects.toMatchObject({ code: "ENOENT" }); }, ); @@ -189,6 +211,7 @@ it.runIf(supportedHost)( expect(releases).toBe(2); removalAllowed = true; await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); }, ); diff --git a/tests/process-global/pwntoolsCliFinalization.test.ts b/tests/process-global/pwntoolsCliFinalization.test.ts index 8728e68bc2..97c0c09f0f 100644 --- a/tests/process-global/pwntoolsCliFinalization.test.ts +++ b/tests/process-global/pwntoolsCliFinalization.test.ts @@ -14,6 +14,7 @@ import { import { BinaryLayoutService } from "../../src/application/binaryDiagnostics/BinaryLayoutService.js"; import { createCli } from "../../src/cli.js"; import * as composition from "../../src/composition/binaryDiagnostics.js"; +import { resolvedAnalysisError } from "../../src/domain/analysisErrorBase.js"; import { projectAnalysisError } from "../../src/domain/analysisErrorProjection.js"; import { PwntoolsLayoutProvider } from "../../src/native/pwntools/PwntoolsLayoutProvider.js"; import type { PwntoolsLauncher } from "../../src/native/pwntools/PwntoolsDecoder.js"; @@ -437,3 +438,50 @@ it.runIf(supportedHost)( await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); }, ); + +it.runIf(supportedHost)( + "retains cancellation and decoded facts until failed final cleanup is retried", + async () => { + const { path, root, launch } = await fixture(); + const controller = new AbortController(); + const reason = { stage: "failed final cleanup" }; + let removalAllowed = false; + const provider = new PwntoolsLayoutProvider( + { REA_PWNTOOLS_PYTHON: process.execPath }, + launch, + async () => ({ + path: root.path, + close: async () => { + if (!removalAllowed) { + controller.abort(reason); + throw new Error("root removal refused"); + } + await root.close(); + }, + }), + ); + onTestFinished(async () => { + removalAllowed = true; + await provider.close(); + }); + const result = await provider.inspectAndClose( + { path }, + { signal: controller.signal }, + ); + if (result.ok) + throw new Error("Expected cancelled finalization with retained cleanup"); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cleanup_incomplete", + details: { diagnostics: { primary_error: { code: "cancelled" } } }, + }); + expect(result.error.cleanupPrimary?.cause).toBe(reason); + expect(result.error.partialObservation).toBeDefined(); + expect(result.error.capturedOutput?.stdout).toBe("decoder output"); + expect(result.error.cleanupResources).toEqual([root.path]); + removalAllowed = true; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(resolvedAnalysisError(result.error).cause).toBe(reason); + await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); + }, +); diff --git a/tests/process-global/pwntoolsStageRetirement.test.ts b/tests/process-global/pwntoolsStageRetirement.test.ts index 7cdf0b48aa..3e24b47cd1 100644 --- a/tests/process-global/pwntoolsStageRetirement.test.ts +++ b/tests/process-global/pwntoolsStageRetirement.test.ts @@ -1,4 +1,4 @@ -import { access, mkdir, readFile, writeFile } from "node:fs/promises"; +import { access, mkdir, open, readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { @@ -25,6 +25,10 @@ import { import { prepareProcessOwnershipInspection } from "../../src/process/ProcessOwnershipObservation.js"; import { spawnOwnedProviderProcess } from "../../src/process/ProviderProcess.js"; import { PrivateRuntimeRoot } from "../../src/process/PrivateRuntimeRoot.js"; +import { + withArtifactFileHandle, + ArtifactFileCleanupFailure, +} from "../../src/artifacts/ArtifactFileOperation.js"; import * as artifacts from "../../src/artifacts/readStableArtifact.js"; import { recordedCrashFixture, @@ -269,12 +273,15 @@ it.runIf(supportedHost).each([ expect(projected.code).toBe("cleanup_incomplete"); if (retainedOwnership === undefined) throw new Error("Expected retained process ownership"); - expect(projected.details?.resources).toEqual([ - retainedOwnership.runId, - root.path, - `pid:${retainedOwnership.leaderPid}`, - `process-group:${retainedOwnership.processGroupId}`, - ]); + expect(projected.details?.resources).toEqual( + expect.arrayContaining([ + retainedOwnership.runId, + root.path, + `pid:${retainedOwnership.leaderPid}`, + `process-group:${retainedOwnership.processGroupId}`, + ]), + ); + expect(projected.details?.resources).toHaveLength(4); if (stage === "debugger") expect(projected).toMatchObject({ details: { @@ -368,3 +375,104 @@ it.runIf(supportedHost)( await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); }, ); + +it.runIf(supportedHost).each(["decoder", "debugger"])( + "does not reinterpret a completed %s read when retention cancels after failed close", + async (stage) => { + const { path, root, environment, basic } = await fixture(); + const controller = new AbortController(); + const reason = { stage: "retention callback after owner completion" }; + const reply = join( + root.path, + ...(stage === "debugger" ? ["debugger"] : []), + "reply.json", + ); + const read = artifacts.readStableArtifact; + let injected = false; + let releaseAllowed = false; + const emergencyCloses: (() => Promise)[] = []; + vi.spyOn(artifacts, "readStableArtifact").mockImplementation( + async (...args) => { + if (args[0] !== reply || injected) return read(...args); + injected = true; + const resources = args[4]; + if (resources === undefined) + throw new Error("Reply must have an artifact resource owner"); + const file = await open(reply, "r"); + const close = file.close.bind(file); + emergencyCloses.push(close); + file.close = async () => { + if (!releaseAllowed) throw new Error("reply close refused"); + await close(); + }; + const retain = resources.retain.bind(resources); + const spy = vi + .spyOn(resources, "retain") + .mockImplementation((owner) => { + retain(owner); + controller.abort(reason); + }); + try { + return await withArtifactFileHandle( + reply, + file, + resources, + controller.signal, + () => read(...args), + ); + } finally { + spy.mockRestore(); + } + }, + ); + let launches = 0; + const provider = new PwntoolsRecordedCrashProvider( + environment, + async (spawn) => { + if (++launches === 1) return basic(spawn); + await writeFile( + reply, + JSON.stringify({ ok: true, value: recordedCrashDebuggerFixture() }), + ); + return spawnOwnedProviderProcess({ + ...spawn, + arguments: ["-e", "process.stdout.write('debugger diagnostic')"], + }); + }, + async () => root, + ); + onTestFinished(async () => { + releaseAllowed = true; + try { + await provider.close(); + } finally { + await Promise.all(emergencyCloses.map((close) => close())); + } + }); + const result = await provider.inspectAndClose( + { path, include_debugger_context: true }, + { signal: controller.signal }, + ); + if (result.ok) throw new Error("Expected the physical reply-close failure"); + expect(result.error._tag).toBe("AnalysisOutputError"); + expect(result.error.cause).toBeInstanceOf(ArtifactFileCleanupFailure); + if (!(result.error.cause instanceof ArtifactFileCleanupFailure)) + throw new Error("Expected exact file owner"); + expect(result.error.cause.fileCleanupFailure.outcome.kind).toBe( + "completed", + ); + expect( + result.error.cause.fileCleanupFailure.completionCancellation, + ).toBeUndefined(); + expect(result.error.cleanupIncomplete).toBe(true); + await expect(provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); + await expect(access(root.path)).resolves.toBeUndefined(); + releaseAllowed = true; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.capturedOutput?.stdout).toContain("basic diagnostic"); + await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); + }, +);