diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 099208716a..14f8f42c55 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -334,7 +334,7 @@ jobs: - name: Process filesystem snapshot descriptor regressions run: npm run test:focused -- src/process/capture/FilesystemSnapshot.test.ts - name: Native Darwin ownership and process cleanup regressions - run: npm run test:focused -- tests/boundary/process/darwinProcessRunTokenReader.test.ts tests/boundary/process/processCaptureLifecycle.test.ts tests/boundary/process/processCli.test.ts tests/boundary/mcp/sessionMcp.test.ts + run: npm run test:focused -- tests/boundary/process/darwinProcessRunTokenReader.test.ts tests/boundary/process/directAnalysisSignals.test.ts tests/boundary/process/processCaptureLifecycle.test.ts tests/boundary/process/processCli.test.ts tests/boundary/mcp/sessionMcp.test.ts reconstruction-readiness: name: Reconstruction readiness diff --git a/docs/mcp-contracts.md b/docs/mcp-contracts.md index 90ffbc97de..1eb03986e5 100644 --- a/docs/mcp-contracts.md +++ b/docs/mcp-contracts.md @@ -103,9 +103,11 @@ still run synchronously, so control messages can wait for those phases to release the event loop. A rejected client promise alone does not establish that the server has stopped its work. -CLI calls work without a progress token and translate SIGINT into the same -AbortSignal used by providers. Existing controlled-process cleanup and provider -shutdown rules still apply; REA never kills a process it cannot prove it owns. +Direct CLI analysis calls work without a progress token and translate SIGINT or +SIGTERM into the same AbortSignal used by providers. Signal guards remain active +through provider cleanup, including repeated delivery by package runners. +Existing controlled-process cleanup and provider shutdown rules still apply; +REA never kills a process it cannot prove it owns. ## Ghidra first-query deadlines and recovery diff --git a/src/application/DirectAnalysis.ts b/src/application/DirectAnalysis.ts index db54c06b5f..f9f15244ff 100644 --- a/src/application/DirectAnalysis.ts +++ b/src/application/DirectAnalysis.ts @@ -388,11 +388,15 @@ const withProcessCancellation = async ( if (suppliedSignal !== undefined) return operation(suppliedSignal); const controller = new AbortController(); const cancel = (): void => controller.abort(); - process.once("SIGINT", cancel); + // Package runners can forward a terminal interrupt after REA already received + // it directly. Keep both guards installed until provider cleanup has settled. + process.on("SIGINT", cancel); + process.on("SIGTERM", cancel); try { return await operation(controller.signal); } finally { process.off("SIGINT", cancel); + process.off("SIGTERM", cancel); } }; diff --git a/tests/boundary/process/directAnalysisSignals.test.ts b/tests/boundary/process/directAnalysisSignals.test.ts new file mode 100644 index 0000000000..e72ce596ea --- /dev/null +++ b/tests/boundary/process/directAnalysisSignals.test.ts @@ -0,0 +1,78 @@ +import { spawn } from "node:child_process"; +import { access, readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import { expect, it, onTestFinished } from "vitest"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +it.skipIf(process.platform === "win32").each([ + ["direct", "SIGINT"], + ["native", "SIGINT"], + ["managed", "SIGINT"], + ["direct", "SIGTERM"], + ["native", "SIGTERM"], + ["managed", "SIGTERM"], +] as const)( + "finishes %s cleanup when %s is delivered again", + async (mode, signal) => { + const root = await createTestTempDirectory("rea-direct-analysis-signals-"); + const child = spawn( + process.execPath, + ["tests/fixtures/directAnalysisInterrupt.mjs", root, mode], + { stdio: ["ignore", "pipe", "pipe"] }, + ); + let stdout = "", + stderr = ""; + child.stdout.on("data", (chunk: Buffer) => { + stdout += chunk.toString(); + }); + child.stderr.on("data", (chunk: Buffer) => { + stderr += chunk.toString(); + }); + const closed = new Promise<{ + code: number | null; + signal: NodeJS.Signals | null; + }>((resolve, reject) => { + child.once("error", reject); + child.once("close", (code, exitSignal) => + resolve({ code, signal: exitSignal }), + ); + }); + onTestFinished(async () => { + await writeFile(join(root, "release-cleanup"), "release"); + if (child.exitCode === null && child.signalCode === null) + child.kill("SIGKILL"); + await closed; + }); + const waitFor = async (name: string) => { + const deadline = Date.now() + 10_000; + while (Date.now() < deadline) { + if (child.exitCode !== null || child.signalCode !== null) + throw new Error(`Child exited before ${name}: ${stderr}`); + try { + await access(join(root, name)); + return; + } catch (cause: unknown) { + if ( + !(cause instanceof Error) || + !("code" in cause) || + cause.code !== "ENOENT" + ) + throw cause; + } + await delay(20); + } + throw new Error(`Child did not reach ${name}: ${stderr}`); + }; + await waitFor("executing"); + child.kill(signal); + await waitFor("cleanup-started"); + child.kill(signal); + await writeFile(join(root, "release-cleanup"), "release"); + expect(await closed, stderr).toEqual({ code: 0, signal: null }); + expect(await readFile(join(root, "cleanup-complete"), "utf8")).toBe( + "complete", + ); + expect(JSON.parse(stdout)).toMatchObject({ code: "cancelled" }); + }, +); diff --git a/tests/composition/directAnalysisFactories.test.ts b/tests/composition/directAnalysisFactories.test.ts index 2de45c68e0..5dfffb8229 100644 --- a/tests/composition/directAnalysisFactories.test.ts +++ b/tests/composition/directAnalysisFactories.test.ts @@ -68,12 +68,14 @@ describe("one-shot analysis factory boundary", () => { const [path] = await createBinarySessionTargets(); const failure = new Error("fixture execution failed"); const factories = recordingFactories(failure); - const listeners = process.listenerCount("SIGINT"); + const interruptListeners = process.listenerCount("SIGINT"); + const terminateListeners = process.listenerCount("SIGTERM"); await expect( runDirectAnalysis(factories.dependencies, path, "read_bytes", {}), ).rejects.toBe(failure); expect(factories.clientsClosed()).toBe(1); - expect(process.listenerCount("SIGINT")).toBe(listeners); + expect(process.listenerCount("SIGINT")).toBe(interruptListeners); + expect(process.listenerCount("SIGTERM")).toBe(terminateListeners); expect(factories.sessions[0]?.activeTarget()).toBeUndefined(); }); diff --git a/tests/fixtures/directAnalysisInterrupt.mjs b/tests/fixtures/directAnalysisInterrupt.mjs new file mode 100644 index 0000000000..76ecde8ea2 --- /dev/null +++ b/tests/fixtures/directAnalysisInterrupt.mjs @@ -0,0 +1,81 @@ +import { access, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import { + runDirectAnalysis, + runProviderAnalysis, +} from "../../dist/application/DirectAnalysis.js"; +import { BinarySession } from "../../dist/application/binary/BinarySession.js"; +import { SessionProviderRouter } from "../../dist/application/binary/SessionProviderRouter.js"; +import { createAnalysisExecution } from "../../dist/application/AnalysisProvider.js"; +import { AnalysisCancelledError } from "../../dist/domain/analysisErrorCore.js"; +import { err, ok } from "../../dist/domain/result.js"; + +const [root, mode] = process.argv.slice(2); +if (!root || !["direct", "native", "managed"].includes(mode)) + throw new Error( + "Expected a fixture root and a direct, native, or managed mode", + ); +const target = join(root, "target.hop"); +await writeFile(target, "fixture"); +const client = { + async execute(operation, _arguments, options) { + if (operation === "health") + return ok( + createAnalysisExecution(null, { + id: "fixture", + name: "Fixture", + version: "1", + }), + ); + await writeFile(join(root, "executing"), "ready"); + await new Promise((resolve) => { + if (options.signal.aborted) resolve(); + else options.signal.addEventListener("abort", resolve, { once: true }); + }); + return err(new AnalysisCancelledError(operation)); + }, + async close() { + await writeFile(join(root, "cleanup-started"), "ready"); + while (true) { + try { + await access(join(root, "release-cleanup")); + break; + } catch (cause) { + if (cause.code !== "ENOENT") throw cause; + } + await delay(5); + } + await writeFile(join(root, "cleanup-complete"), "complete"); + }, +}; +const createSession = () => + new BinarySession(SessionProviderRouter.single(() => client)); +const dependencies = { + createBinarySession: createSession, + createManagedBinarySession: createSession, +}; +const interruptListeners = process.listenerCount("SIGINT"); +const terminateListeners = process.listenerCount("SIGTERM"); +// The production provider owns sockets/process handles; keep this fixture alive +// while its replacement waits only on a signal. +const keepAlive = setInterval(() => {}, 1000); +const result = + mode === "direct" + ? await runDirectAnalysis(dependencies, target, "read_bytes", { + address: "0x0", + length: 1, + }) + : await runProviderAnalysis( + dependencies, + target, + mode === "managed" ? "inspect_managed_artifact" : "procedure_address", + mode === "managed" ? {} : { procedure: "fixture" }, + ); +clearInterval(keepAlive); +if ( + process.listenerCount("SIGINT") !== interruptListeners || + process.listenerCount("SIGTERM") !== terminateListeners +) + throw new Error("Analysis leaked process cancellation listeners"); +process.stdout.write(JSON.stringify(result));