From 67ce671d66876da51c03220b2d4a16e6e02f4f00 Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Sat, 14 Nov 2026 09:56:18 +0100 Subject: [PATCH] fix(verifiers): release fixture roots only after their owners finish (#10491) * fix(verifiers): release fixture roots only after their owners finish Several real-tool verifiers released a fixture root even when a cleanup that still used it had failed, so a surviving MCP server, session or descendant kept running against a deleted directory and the failure looked like ordinary cleanup. - scripts/lib/verifier-cleanup.mjs: new releaseAfter(dependents, release, retained) task. It attempts every dependent cleanup and releases the resource only if all succeed; otherwise it keeps the resource and reports it with the dependent failures. - verify-signal-recording: the root is released after the MCP client and transport close, and the host-limits read moves before the root is acquired so its failure cannot leak the root. - verify-recorded-crash: sessions, the sentinel and the process-lineage check must all succeed before the root is released. A transport close failure after a failed connect keeps the connect failure. - verify-wasm-artifact: both roots are released after the MCP client and transport close; the residue check and the two directories stay independent after that. - verify-native-ui: `open -W` exits only after the app exits, so the root is removed only once that exit is observed; otherwise it is retained and reported. Fixes #8561 Fixes #8562 Fixes #8564 Refs #8559 Co-Authored-By: gensokyo-dev Co-Authored-By: Claude Opus 5.5 * fix(verifiers): treat a signaled open as unproven fixture exit A terminated `open` launcher does not prove the LaunchServices app it started has exited, so only an `open -W` that exits on its own counts. Also clear the grace timer so a prompt exit does not hold the process for the rest of the grace period. Refs #8559 Co-Authored-By: gensokyo-dev Co-Authored-By: Claude Opus 5.5 * test(verifiers): model open -W exiting after the fixture is signaled The native UI verifier now removes its root only after `open -W` exits on its own. The authored recording's `open` never exited when the fixture was signaled and had no signalCode, so the complete case waited out the grace period and kept the root. The double now behaves like `open -W`: it exits once the fixture process is signaled. Refs #8559 Co-Authored-By: gensokyo-dev Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: gensokyo-dev Co-authored-by: Claude Opus 5.5 --- scripts/lib/verifier-cleanup.d.mts | 10 ++++ scripts/lib/verifier-cleanup.mjs | 20 +++++++ scripts/verify-native-ui.mjs | 43 ++++++++++++-- scripts/verify-recorded-crash.mjs | 42 ++++++++------ scripts/verify-signal-recording.mjs | 36 +++++++----- scripts/verify-wasm-artifact.mjs | 51 ++++++++--------- scripts/verify/recorded-crash/public.mjs | 11 +++- .../filesystem/verifierCleanup.test.ts | 57 +++++++++++++++++++ .../process/nativeUiVerifierActions.test.ts | 4 +- 9 files changed, 207 insertions(+), 67 deletions(-) create mode 100644 tests/boundary/filesystem/verifierCleanup.test.ts diff --git a/scripts/lib/verifier-cleanup.d.mts b/scripts/lib/verifier-cleanup.d.mts index 9458277128..ccc2fcddb8 100644 --- a/scripts/lib/verifier-cleanup.d.mts +++ b/scripts/lib/verifier-cleanup.d.mts @@ -8,3 +8,13 @@ export function withVerifierCleanup( operation: () => T | Promise, cleanupTasks: readonly (() => void | Promise)[], ): Promise; + +/** + * A cleanup task that releases one resource only after every cleanup that may + * still use it has succeeded; otherwise the resource is retained and reported. + */ +export function releaseAfter( + dependents: readonly (() => void | Promise)[], + release: () => void | Promise, + retained: string, +): () => Promise; diff --git a/scripts/lib/verifier-cleanup.mjs b/scripts/lib/verifier-cleanup.mjs index 979106d1d4..67552be2c7 100644 --- a/scripts/lib/verifier-cleanup.mjs +++ b/scripts/lib/verifier-cleanup.mjs @@ -11,6 +11,26 @@ async function collectCleanupFailures(tasks) { return failures; } +/** + * A cleanup task that releases one resource only after every cleanup that may + * still use it has succeeded. When a dependent fails, the resource is retained + * and reported with that failure instead of being released underneath it. + */ +export function releaseAfter(dependents, release, retained) { + return async () => { + const failures = await collectCleanupFailures(dependents); + if (failures.length > 0) + throw new AggregateError( + [ + ...failures, + new Error(`Retained ${retained} because a dependent cleanup failed.`), + ], + "Verifier cleanup failed", + ); + await release(); + }; +} + /** Attempt every independent cleanup and preserve each failure. */ export async function closeVerifierResources(tasks) { const failures = await collectCleanupFailures(tasks); diff --git a/scripts/verify-native-ui.mjs b/scripts/verify-native-ui.mjs index ebc28c883c..252beaea05 100644 --- a/scripts/verify-native-ui.mjs +++ b/scripts/verify-native-ui.mjs @@ -21,6 +21,10 @@ let childExit; let fixturePid; let fixtureIdentity; let report; +// `open -W` exits only after the app exits, so its own exit is the proof that +// the fixture no longer runs from the root. Without it the root is retained. +let retainedRoot; +const FIXTURE_EXIT_GRACE_MS = 10_000; try { const childRetrievalTest = join(root, "child-retrieval-test"); await promisify(execFile)("/usr/bin/xcrun", [ @@ -288,10 +292,41 @@ try { } } } - if (child !== undefined && child.exitCode === null) { - child.kill("SIGTERM"); - await childExit; + const fixtureExited = + child === undefined || (await openExitedWithFixture(child, childExit)); + if (fixtureExited) await rm(root, { recursive: true, force: true }); + else { + // The app may have been reparented away from `open`; stopping `open` + // does not prove the fixture exited, so its root stays in place. + if (child.exitCode === null && child.signalCode === null) { + child.kill("SIGTERM"); + await childExit; + } + retainedRoot = root; + process.stderr.write( + `Source-owned UI fixture exit was not observed; retained ${root}\n`, + ); } - await rm(root, { recursive: true, force: true }); } +if (retainedRoot !== undefined) + throw new Error( + `Source-owned UI fixture exit was not observed; retained ${retainedRoot}`, + ); process.stdout.write(`${JSON.stringify(report)}\n`); + +// `open -W` exits on its own only after the app exits (or never launched it). +// A signaled `open`, or none within the grace period, leaves the app unproven. +async function openExitedWithFixture(launcher, exit) { + if (launcher.exitCode === null && launcher.signalCode === null) { + let timer; + const exited = await Promise.race([ + exit.then(() => true), + new Promise((resolve) => { + timer = setTimeout(() => resolve(false), FIXTURE_EXIT_GRACE_MS); + }), + ]); + clearTimeout(timer); + if (!exited) return false; + } + return launcher.exitCode !== null && launcher.signalCode === null; +} diff --git a/scripts/verify-recorded-crash.mjs b/scripts/verify-recorded-crash.mjs index 6a5be6c135..d4ce4aa629 100644 --- a/scripts/verify-recorded-crash.mjs +++ b/scripts/verify-recorded-crash.mjs @@ -4,7 +4,12 @@ import { writeFile, readFile, copyFile } from "node:fs/promises"; import { isAbsolute, join } from "node:path"; import { fileURLToPath } from "node:url"; import { PrivateRuntimeRoot } from "../dist/process/PrivateRuntimeRoot.js"; -import { createVerifierRun, completeVerifierRun } from "./lib/verifier-run.mjs"; +import { releaseAfter } from "./lib/verifier-cleanup.mjs"; +import { + assertVerifierCleanupComplete, + createVerifierRun, + completeVerifierRun, +} from "./lib/verifier-run.mjs"; import { verifyCompleteToolCatalog } from "./lib/verify-package-core.mjs"; import { connectRecordedCrash } from "./verify/recorded-crash/public.mjs"; import { @@ -41,6 +46,7 @@ const entrypoint = const failures = []; const sessions = []; let sentinel; +let verifier; let cases = 0; let trace; let catalogTools; @@ -340,25 +346,25 @@ try { } catch (cause) { failures.push(cause); } finally { - for (const close of [ - ...sessions.map((session) => () => session.close()), - ...(sentinel ? [() => sentinel.close()] : []), - () => root.close(), - ]) { - try { - await close(); - } catch (cause) { - failures.push(cause); - } + // Sessions, the sentinel and any surviving descendant may still use files + // under the root, so it is released only after all of them are proven gone. + try { + await releaseAfter( + [ + ...sessions.map((session) => () => session.close()), + ...(sentinel ? [() => sentinel.close()] : []), + async () => { + verifier = await completeVerifierRun(run); + assertVerifierCleanupComplete(verifier.process_lineage); + }, + ], + () => root.close(), + root.path, + )(); + } catch (cause) { + failures.push(cause); } } -const verifier = await completeVerifierRun(run); -try { - assert.equal(verifier.process_lineage.status, "verified"); - assert.deepEqual(verifier.process_lineage.descendants, []); -} catch (cause) { - failures.push(cause); -} const result = { status: failures.length === 0 ? "passed" : "failed", public_cases: cases, diff --git a/scripts/verify-signal-recording.mjs b/scripts/verify-signal-recording.mjs index 61d3afd831..2abeeb961e 100644 --- a/scripts/verify-signal-recording.mjs +++ b/scripts/verify-signal-recording.mjs @@ -17,7 +17,7 @@ import { StdioClientTransport } from "@modelcontextprotocol/client/stdio"; import { Ajv2020 } from "ajv/dist/2020.js"; import { PrivateRuntimeRoot } from "../dist/process/PrivateRuntimeRoot.js"; import { parseEvidence } from "../dist/domain/evidence.js"; -import { withVerifierCleanup } from "./lib/verifier-cleanup.mjs"; +import { releaseAfter, withVerifierCleanup } from "./lib/verifier-cleanup.mjs"; import { createVerifierRun, completeVerifierRun } from "./lib/verifier-run.mjs"; if (process.platform !== "linux") @@ -30,6 +30,19 @@ if (!isAbsolute(process.env.REA_SIGROK_COMMAND ?? "")) ); const entrypoint = process.argv[2] ?? fileURLToPath(new URL("./rea.mjs", import.meta.url)); +// Read host limits before acquiring the root so a failure cannot leak it. +const inheritedAddressSpace = (await readFile("/proc/self/limits", "utf8")) + .split("\n") + .find((line) => line.startsWith("Max address space ")) + ?.trim() + .match(/^Max address space\s+(\S+)\s+(\S+)\s+bytes$/u); +assert.ok(inheritedAddressSpace); +const expectedAddressSpace = Math.min( + 1073741824, + ...inheritedAddressSpace + .slice(1) + .map((value) => (value === "unlimited" ? Infinity : Number(value))), +); const run = createVerifierRun(); const root = await PrivateRuntimeRoot.create({ prefix: "rea-signal-verifier-", @@ -93,18 +106,6 @@ lines.push(`#${tick + 1000}`, "1!"); const source = lines.join("\n") + "\n"; let cases = 0; const discoveryMeasurements = []; -const inheritedAddressSpace = (await readFile("/proc/self/limits", "utf8")) - .split("\n") - .find((line) => line.startsWith("Max address space ")) - ?.trim() - .match(/^Max address space\s+(\S+)\s+(\S+)\s+bytes$/u); -assert.ok(inheritedAddressSpace); -const expectedAddressSpace = Math.min( - 1073741824, - ...inheritedAddressSpace - .slice(1) - .map((value) => (value === "unlimited" ? Infinity : Number(value))), -); await withVerifierCleanup(async () => { await writeFile(path, source); @@ -437,7 +438,14 @@ await withVerifierCleanup(async () => { "paged.vcd", "recording with spaces.vcd", ]); -}, [() => client.close(), () => transport.close(), () => root.close()]); +}, [ + // The MCP server child reads files under the root until it has exited. + releaseAfter( + [() => client.close(), () => transport.close()], + () => root.close(), + root.path, + ), +]); console.log( JSON.stringify( { diff --git a/scripts/verify-wasm-artifact.mjs b/scripts/verify-wasm-artifact.mjs index 53d0a4e879..9018c67f73 100644 --- a/scripts/verify-wasm-artifact.mjs +++ b/scripts/verify-wasm-artifact.mjs @@ -25,6 +25,10 @@ import { mcpTextValue, requireMcpToolError, } from "./lib/mcp-verifier-results.mjs"; +import { + closeVerifierResources, + releaseAfter, +} from "./lib/verifier-cleanup.mjs"; import { createVerifierRun, completeVerifierRun } from "./lib/verifier-run.mjs"; import { WABT_OPERATION_TIMEOUT_MS } from "./lib/wabt-verifier-timeouts.mjs"; @@ -623,35 +627,26 @@ try { } catch (cause) { failures.push(cause); } +// The MCP server child uses both roots until it exits, so they are released +// only after the client and transport close. After that the residue check and +// the two directories are independent owners; attempt each and keep failures. try { - await client.close(); -} catch (cause) { - failures.push(cause); -} -try { - await transport.close(); -} catch (cause) { - failures.push(cause); -} -try { - assert.ok( - !(await readdir(childTempRoot)).some((name) => - name.startsWith("rea-wabt-"), - ), - "Provider workspace leaked", - ); -} catch (cause) { - failures.push(cause); -} -// Each directory is an independent owner. Attempt every cleanup even if a -// residue assertion or another owner's close fails, retaining every error. -try { - await root.close(); -} catch (cause) { - failures.push(cause); -} -try { - await rm(childTempRoot, { recursive: true, force: true }); + await releaseAfter( + [() => client.close(), () => transport.close()], + () => + closeVerifierResources([ + async () => + assert.ok( + !(await readdir(childTempRoot)).some((name) => + name.startsWith("rea-wabt-"), + ), + "Provider workspace leaked", + ), + () => root.close(), + () => rm(childTempRoot, { recursive: true, force: true }), + ]), + `${root.path} and ${childTempRoot}`, + )(); } catch (cause) { failures.push(cause); } diff --git a/scripts/verify/recorded-crash/public.mjs b/scripts/verify/recorded-crash/public.mjs index ecdfd14cd2..6ebe7172ac 100644 --- a/scripts/verify/recorded-crash/public.mjs +++ b/scripts/verify/recorded-crash/public.mjs @@ -23,7 +23,16 @@ export async function connectRecordedCrash({ entrypoint, environment }) { try { await client.connect(transport); } catch (cause) { - await transport.close(); + // Keep the connect failure when closing the transport also fails. + try { + await transport.close(); + } catch (closeCause) { + throw new AggregateError( + [cause, closeCause], + "Recorded-crash MCP connection and transport cleanup failed", + { cause }, + ); + } throw cause; } return { diff --git a/tests/boundary/filesystem/verifierCleanup.test.ts b/tests/boundary/filesystem/verifierCleanup.test.ts new file mode 100644 index 0000000000..dc4f284a72 --- /dev/null +++ b/tests/boundary/filesystem/verifierCleanup.test.ts @@ -0,0 +1,57 @@ +import { expect, it } from "vitest"; +import { + closeVerifierResources, + releaseAfter, +} from "../../../scripts/lib/verifier-cleanup.mjs"; + +it("releases a root only after every dependent cleanup succeeds", async () => { + const order: string[] = []; + await closeVerifierResources([ + releaseAfter( + [ + async () => { + order.push("client"); + }, + async () => { + order.push("transport"); + }, + ], + async () => { + order.push("root"); + }, + "/fixture/root", + ), + ]); + expect(order).toEqual(["client", "transport", "root"]); +}); + +it("retains the root and reports every dependent failure", async () => { + const order: string[] = []; + const clientFailure = new Error("client close failed"); + const failure = await closeVerifierResources([ + releaseAfter( + [ + async () => { + order.push("client"); + throw clientFailure; + }, + async () => { + order.push("transport"); + }, + ], + async () => { + order.push("root"); + }, + "/fixture/root", + ), + ]).catch((cause: unknown) => cause); + // The later dependent is still attempted, but the root is not released. + expect(order).toEqual(["client", "transport"]); + expect(failure).toBeInstanceOf(AggregateError); + const [retention] = (failure as AggregateError).errors; + expect(retention).toBeInstanceOf(AggregateError); + expect((retention as AggregateError).errors).toEqual([ + clientFailure, + new Error("Retained /fixture/root because a dependent cleanup failed."), + ]); +}); diff --git a/tests/boundary/process/nativeUiVerifierActions.test.ts b/tests/boundary/process/nativeUiVerifierActions.test.ts index 7f6ff9bfd7..efeb3c5903 100644 --- a/tests/boundary/process/nativeUiVerifierActions.test.ts +++ b/tests/boundary/process/nativeUiVerifierActions.test.ts @@ -26,7 +26,7 @@ export const execFile=Object.assign(()=>{}, {[promisify.custom]:async (file,args if(args?.includes('capture-native-ui-scenario')){if(process.env.MODE!=='drop CLI')count++;return {stdout:JSON.stringify({normalized_result:captured()})};} return {stdout:'{"v":"slash/producer"}\\n{"v":1.0}\\n'}; }}); -export const spawn=(_command,args)=>{const child=new EventEmitter();child.exitCode=null;child.kill=()=>{child.exitCode=0;child.emit('exit',0);return true;};void writeFile(args[args.indexOf('--stdout')+1],JSON.stringify({pid:424242,window_id:7})+'\\n');return child;}; +export const spawn=(_command,args)=>{const child=new EventEmitter();child.exitCode=null;child.signalCode=null;globalThis.reaOpenChild=child;child.kill=()=>{child.exitCode=0;child.emit('exit',0);return true;};void writeFile(args[args.indexOf('--stdout')+1],JSON.stringify({pid:424242,window_id:7})+'\\n');return child;}; export const parseBinaryTarget=async path=>({ok:true,value:{path,sha256:'a'.repeat(64)}}); export const observeProcessStartIdentity=async()=>({state:'readable',identity:'authored-process-identity'}); export const observeNativeUi=async(target,operation)=>{ @@ -42,7 +42,7 @@ export const artifactMcpResult=async(_client,name)=>{if(name==='observe_native_u join(directory, "hook.mjs"), ` import {registerHooks} from 'node:module';import {pathToFileURL} from 'node:url'; -Object.defineProperty(process,'platform',{value:'darwin'});process.kill=()=>true; +Object.defineProperty(process,'platform',{value:'darwin'});process.kill=()=>{const open=globalThis.reaOpenChild;if(open?.exitCode===null){open.exitCode=0;queueMicrotask(()=>open.emit('exit',0,null));}return true;}; registerHooks({resolve(specifier,context,next){if(context.parentURL?.endsWith('/scripts/verify-native-ui.mjs')&&['node:child_process','../dist/application/BinaryTargetResolver.js','../dist/native/NativeUiObservation.js','./lib/artifact-e2e.mjs','../dist/process/ProcessOwnershipObservation.js'].includes(specifier))return {shortCircuit:true,url:pathToFileURL(process.env.RECORDING).href};return next(specifier,context);}}); `, );