mirror of
https://github.com/morluto/rea
synced 2026-10-25 22:08:11 -04:00
fix(cli): complete cleanup after relayed interrupts (#1202)
* fix: Handle relayed SIGINT gracefully in withProcessCancellation Changes `process.once` to `process.on` for SIGINT in `DirectAnalysis.ts`. When REA is launched via npx, a single process-group SIGINT triggers two SIGINT events (one direct, one relayed). Previously, the first event would consume the handler and the second would abruptly terminate the Node process, abandoning Ghidra resource cleanup. This change ensures subsequent SIGINTs idempotently request cancellation without crashing the process, matching the behaviour of `withCommandCancellation`. * fix(cli): keep termination guards through provider cleanup * test(cli): exercise repeated analysis signals on macOS --------- Co-authored-by: N0zoM1z0 <1705004398@qq.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -388,11 +388,15 @@ const withProcessCancellation = async <Value>(
|
||||
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);
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
@@ -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" });
|
||||
},
|
||||
);
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
|
||||
+81
@@ -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));
|
||||
Reference in New Issue
Block a user