diff --git a/docs/cli.md b/docs/cli.md index f6b7d55b083..76a3e5ef5ca 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -569,6 +569,17 @@ prints a failure, so it omits the same-session reuse and `parameter`) for a new invocation and rerun. For example, a Ghidra decompile deadline names `REA_GHIDRA_DECOMPILE_TIMEOUT_SECONDS`. +`rea debug-with-gdb` and `rea debug-with-rizin` hold their debugger only for the +command's own process. If cleanup cannot confirm the debugger stopped, they print +`cleanup_incomplete` and exit nonzero without stopping it. `remediation.action` +names the process, as does `retained_debugger_pids` in `details.diagnostics`, or +in `details.diagnostics.cleanup_error.details.diagnostics` when the command also +failed or was interrupted. Exiting closes the debugger's command and output pipes. +REA does not establish what the debugger or its target does next, so check that +process. An MCP server that shuts down likewise exits with status 1 instead of +waiting on a debugger it could not stop. It names the process on stderr and as +`retained_debugger_pids` in its error log. + If one-shot analysis fails and closing its session also fails, the original error details remain inline and `details.cleanup_error` describes the later close failure. Each diagnostic retains its own reported resources; the later diff --git a/scripts/ci/scopes.mjs b/scripts/ci/scopes.mjs index c0dff3ed66b..051406c6254 100644 --- a/scripts/ci/scopes.mjs +++ b/scripts/ci/scopes.mjs @@ -76,6 +76,7 @@ const PATH_SCOPES = { "src/frida/frida-bindings.d.ts", "src/objdump/**", "src/cli/reverseEngineeringCommands.ts", + "src/process/ownedDebuggerCleanupError.ts", "src/process/ownedDebuggerSession.ts", "src/rizin/**", "src/contracts/*ToolContracts.ts", @@ -103,6 +104,7 @@ const PATH_SCOPES = { "tests/boundary/providers/RizinInferiorTty.test.ts", "tests/boundary/providers/RizinTestInferiorTty.ts", "tests/boundary/process/ownedDebuggerSession.test.ts", + "tests/boundary/cli/debuggerOneShotExit.test.ts", "tests/boundary/cli/gdbStartupCleanupCli.test.ts", "tests/boundary/cli/reverseEngineeringCommands.test.ts", "tests/boundary/mcp/cutter*.test.ts", diff --git a/src/cli/reverseEngineeringCommands.ts b/src/cli/reverseEngineeringCommands.ts index b07da0176b3..9998c2217bc 100644 --- a/src/cli/reverseEngineeringCommands.ts +++ b/src/cli/reverseEngineeringCommands.ts @@ -28,6 +28,11 @@ import { createRizinDebugSessionManager, } from "../composition/debuggerSessions.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { + retainedDebuggerDiagnostics, + retainedDebuggerProcessIds, + retainedDebuggerRemediation, +} from "../process/ownedDebuggerCleanupError.js"; import { analysisEvidenceDeliveryOptions, cliEvidenceDeliveryInputError, @@ -144,6 +149,9 @@ const closeDebuggerCommandSession = async ( try { await manager.closeAll(); } catch (cause) { + // This process is the only owner, so a caller gets the debugger to check + // rather than a retry that no later command could perform. + const retainedPids = retainedDebuggerProcessIds(cause); throw new ProviderCleanupError( providerId, sessionId === undefined ? [] : [sessionId], @@ -158,8 +166,15 @@ const closeDebuggerCommandSession = async ( : cause instanceof Error ? cause.message : String(cause), + ...retainedDebuggerDiagnostics(cause), + }, + { + cause, + operation, + ...(retainedPids.length === 0 + ? {} + : { remediationAction: retainedDebuggerRemediation(retainedPids) }), }, - { cause, operation }, ); } }; diff --git a/src/gdb/GdbSessionManager.ts b/src/gdb/GdbSessionManager.ts index cf9047bc6dc..8eb8c3d7fb1 100644 --- a/src/gdb/GdbSessionManager.ts +++ b/src/gdb/GdbSessionManager.ts @@ -2164,6 +2164,8 @@ export class GdbSessionManager { ); }); }, this.#commandTimeoutMs); + // Cleanup bounds its own wait, so a late reply must not hold the host open. + if (session.cleanupStarted) timer.unref(); pending = { token, command, diff --git a/src/main/shutdown.ts b/src/main/shutdown.ts index 5614bc13a5d..124df659e34 100644 --- a/src/main/shutdown.ts +++ b/src/main/shutdown.ts @@ -3,6 +3,12 @@ import type { Logger } from "pino"; import { AnalysisError } from "../domain/analysisErrorBase.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import type { JsonValue } from "../domain/jsonValue.js"; +import { + OwnedDebuggerCleanupError, + retainedDebuggerDiagnostics, + retainedDebuggerProcessIds, + retainedDebuggerRemediation, +} from "../process/ownedDebuggerCleanupError.js"; import type { McpServerHandle, RuntimeDependencies } from "./types.js"; import { MCP_SHUTDOWN_FAILED } from "./messages.js"; @@ -103,10 +109,14 @@ export const reportMcpShutdownFailure = ( "MCP shutdown rejected", ); dependencies.setExitCode(1); - serverLogger.error(MCP_SHUTDOWN_FAILED); + serverLogger.error(retainedDebuggerDiagnostics(cause), MCP_SHUTDOWN_FAILED); dependencies.writeStderr(`${MCP_SHUTDOWN_FAILED}\n`); if (cause instanceof AnalysisError) dependencies.writeStderr(`${projectAnalysisError(cause).message}\n`); + // This process is about to end, so it is the last chance to name them. + const retainedPids = retainedDebuggerProcessIds(cause); + if (retainedPids.length > 0) + dependencies.writeStderr(`${retainedDebuggerRemediation(retainedPids)}\n`); }; const describeShutdownFailure = ( @@ -134,6 +144,9 @@ const describeShutdownFailureNode = ( (typeof code === "number" && Number.isFinite(code)) ? { code } : {}), + ...(cause instanceof OwnedDebuggerCleanupError + ? { retained_debugger_pids: [...cause.retainedProcessIds] } + : {}), ...(cause instanceof AggregateError ? { errors: cause.errors.map((error: unknown) => diff --git a/src/process/ownedDebuggerCleanupError.ts b/src/process/ownedDebuggerCleanupError.ts new file mode 100644 index 00000000000..20460a5d76c --- /dev/null +++ b/src/process/ownedDebuggerCleanupError.ts @@ -0,0 +1,45 @@ +/** Closing all sessions could not confirm that these debuggers stopped. */ +export class OwnedDebuggerCleanupError extends AggregateError { + /** Process IDs when cleanup gave up. Observations, not lasting proof of identity. */ + readonly retainedProcessIds: readonly number[]; + + constructor( + failures: readonly Error[], + message: string, + retainedProcessIds: readonly number[], + ) { + super(failures, message); + this.retainedProcessIds = retainedProcessIds; + } +} + +/** Retained debugger process IDs anywhere in a composed cleanup failure. */ +export const retainedDebuggerProcessIds = ( + cause: unknown, +): readonly number[] => { + const found = new Set(); + const visited = new WeakSet(); + const visit = (node: unknown): void => { + if (typeof node !== "object" || node === null || visited.has(node)) return; + visited.add(node); + if (node instanceof OwnedDebuggerCleanupError) + for (const pid of node.retainedProcessIds) found.add(pid); + if (node instanceof AggregateError) + for (const error of node.errors) visit(error); + if (node instanceof Error) visit(node.cause); + }; + visit(cause); + return [...found]; +}; + +/** Log or diagnostic fields naming those processes; empty when none are known. */ +export const retainedDebuggerDiagnostics = ( + cause: unknown, +): { readonly retained_debugger_pids?: number[] } => { + const pids = retainedDebuggerProcessIds(cause); + return pids.length === 0 ? {} : { retained_debugger_pids: [...pids] }; +}; + +/** What a caller must do once REA ends with these debuggers unconfirmed. */ +export const retainedDebuggerRemediation = (pids: readonly number[]): string => + `REA exits without confirming that the debugger stopped, and no later REA command can retry this cleanup. Check process ${pids.join(", ")} and its target, and stop them if they are still running.`; diff --git a/src/process/ownedDebuggerSession.ts b/src/process/ownedDebuggerSession.ts index 009bb42e829..8ba4b4e2bc9 100644 --- a/src/process/ownedDebuggerSession.ts +++ b/src/process/ownedDebuggerSession.ts @@ -4,6 +4,7 @@ import type { DebuggerAdmissionLimit } from "../config/debuggerSessionLimits.js" import { AnalysisResourceConstraintError } from "../domain/analysisErrorCore.js"; import type { JsonValue } from "../domain/jsonValue.js"; import { parseLinuxProcessStat } from "./LinuxProcessStat.js"; +import { OwnedDebuggerCleanupError } from "./ownedDebuggerCleanupError.js"; /** Result of terminating one debugger process owned by this application. */ export type OwnedDebuggerStopResult = @@ -364,42 +365,58 @@ export const stopOwnedDebuggerProcess = async ( ); }; +/** What closing all sessions needs from a session it may have to retain. */ +interface RetainableDebuggerSession { + readonly id: string; + readonly launched: { + readonly process: { readonly pid?: number | undefined }; + }; + readonly supervisor: { ref(): void; unref(): void }; +} + /** * Stop every owned session and retain failed sessions for possible recovery. * Callers must block replacement admission and drain in-flight starts until * this settles; incomplete cleanup leaves the old owner in the map for retry. + * + * Closing all sessions means their owner is finishing, so a retained session + * stops holding the host's event loop, as a supervised provider does after + * incomplete cleanup. Ownership in this process is unchanged, and a retry + * restores the references. */ export const closeOwnedDebuggerSessions = async < - Session extends { id: string }, + Session extends RetainableDebuggerSession, >( sessions: Map, stop: (session: Session) => Promise, debuggerName: string, ): Promise => { + const entries = [...sessions.entries()]; + for (const [, session] of entries) session.supervisor.ref(); const results = await Promise.allSettled( - [...sessions.entries()].map(async ([key, session]) => ({ - key, - session, - result: await stop(session), - })), + entries.map(async ([, session]) => stop(session)), ); const failures: Error[] = []; - for (const settled of results) { + const retained: Session[] = []; + for (const [index, settled] of results.entries()) { + const [key, session] = entries[index]!; if (settled.status === "rejected") { failures.push( settled.reason instanceof Error ? settled.reason : new Error(String(settled.reason)), ); - continue; - } - const { key, session, result } = settled.value; - if (result.status === "incomplete") failures.push(new Error(result.reason)); - else if (sessions.get(key) === session) sessions.delete(key); + retained.push(session); + } else if (settled.value.status === "incomplete") { + failures.push(new Error(settled.value.reason)); + retained.push(session); + } else if (sessions.get(key) === session) sessions.delete(key); } - if (failures.length > 0) - throw new AggregateError( - failures, - `One or more owned ${debuggerName} processes did not stop cleanly`, - ); + if (failures.length === 0) return; + for (const session of retained) session.supervisor.unref(); + throw new OwnedDebuggerCleanupError( + failures, + `One or more owned ${debuggerName} processes did not stop cleanly`, + retained.flatMap(({ launched }) => launched.process.pid ?? []), + ); }; diff --git a/src/server/createServer.ts b/src/server/createServer.ts index 5808bd1ba02..030a94eede1 100644 --- a/src/server/createServer.ts +++ b/src/server/createServer.ts @@ -13,6 +13,7 @@ import { registerCaptureTools } from "./registerCaptureTools.js"; import { registerSignalTools } from "./registerSignalTools.js"; import { registerWasmTools } from "./registerWasmTools.js"; import { registerApkSignatureTools } from "./registerApkSignatureTools.js"; +import { retainedDebuggerDiagnostics } from "../process/ownedDebuggerCleanupError.js"; import { snapshotEnvironment } from "../process/snapshotEnvironment.js"; import { captureProviderEnvironment } from "./providerEnvironment.js"; import { createByteEntropyService } from "../composition/binaryDiagnostics.js"; @@ -305,7 +306,10 @@ export const createServer = ( previousOnclose?.(); void closeProviders().catch((cause: unknown) => { logger.error( - { error: cause instanceof Error ? cause.message : String(cause) }, + { + error: cause instanceof Error ? cause.message : String(cause), + ...retainedDebuggerDiagnostics(cause), + }, "Provider cleanup failed during server shutdown", ); }); diff --git a/tests/boundary/cli/debuggerOneShotExit.test.ts b/tests/boundary/cli/debuggerOneShotExit.test.ts new file mode 100644 index 00000000000..b5d8b949f43 --- /dev/null +++ b/tests/boundary/cli/debuggerOneShotExit.test.ts @@ -0,0 +1,423 @@ +import { chmod, readFile, writeFile } from "node:fs/promises"; +import { join, resolve } from "node:path"; + +import { describe, expect, onTestFinished } from "vitest"; + +import { MCP_SHUTDOWN_FAILED } from "../../../src/main/messages.js"; +import { retainedDebuggerRemediation } from "../../../src/process/ownedDebuggerCleanupError.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { + cliProcessEnvironment, + cliTest, +} from "../../support/cli/cliFixture.js"; +import type { + ProcessResult, + TestProcesses, +} from "../../support/process/processFixture.js"; +import { rizinTestInferiorTty } from "../providers/RizinTestInferiorTty.js"; + +/** + * Fake debuggers behind the compiled public entrypoint. Real Rizin and GDB are + * not exercised: what they do once REA's end of their pipes closes is unverified. + * Every retained mode stays alive after stdin ends, like a debugger that ignores + * EOF, so a passing test also shows that REA did not stop what it reports retained. + * A request neither fake models is noted, and that note fails the test. + */ +const fakeDebuggerPrelude = `#!${process.execPath} +const { appendFileSync } = require("node:fs"); +const note = (line) => appendFileSync(process.env.FAKE_DEBUGGER_LOG, line + "\\n"); +const mode = process.env.FAKE_DEBUGGER_MODE; +note("pid " + process.pid); +process.on("SIGTERM", () => { note("sigterm"); process.exit(0); }); +process.stdin.setEncoding("utf8"); +process.stdin.on("end", () => note("stdin-end")); +setInterval(() => {}, 1000); +const onLines = (receive) => { + let input = ""; + process.stdin.on("data", (chunk) => { + input += chunk; + for (let end = input.indexOf("\\n"); end >= 0; end = input.indexOf("\\n")) { + receive(input.slice(0, end)); + input = input.slice(end + 1); + } + }); +}; +`; + +const fakeRizin = `${fakeDebuggerPrelude} +process.stdout.write("\\0"); +let silent = false; +onLines((line) => { + if (line === "dr") process.stdout.write("rax=0x1234\\0"); + else if (line === "olj") { + if (silent) return; + note("inventory-request"); + if (mode === "unanswered") silent = true; + else process.stdout.write(mode === "clean" ? "[]\\0" : "not-json\\0"); + } else if (/^!echo __REA_(COMMAND|CLEANUP)_COMPLETE_[0-9a-f]+__$/u.test(line)) { + if (!silent) process.stdout.write(line.slice(6) + "\\n\\0"); + } else { + note("unmodeled " + line); + process.stdout.write("unmodeled\\0"); + } +}); +`; + +const fakeGdb = `${fakeDebuggerPrelude} +process.stdout.write("(gdb)\\n"); +let tty = ""; +let inventoryRequests = 0; +let ttyReads = 0; +const liveGroup = '^done,groups=[{id="i1",type="process",pid="4194305",threads=[{id="1",state="stopped"}]}]'; +onLines((line) => { + const [, token, request] = /^(\\d+)(.*)$/u.exec(line) ?? []; + const reply = (tail) => process.stdout.write(token + tail + "\\n(gdb)\\n"); + if (request === "-gdb-set auto-load off") reply("^done"); + else if (request?.startsWith("-inferior-tty-set ")) { + tty = JSON.parse(request.slice("-inferior-tty-set ".length)); + reply("^done"); + } else if (request === "-inferior-tty-show") { + ttyReads += 1; + reply('^done,inferior_tty_terminal="' + (ttyReads > 1 ? "/dev/null" : tty) + '"'); + } else if (request === '-interpreter-exec console "show version"') { + process.stdout.write('~"GNU gdb (fake)\\\\n"\\n'); + reply("^done"); + } else if (request === "-list-thread-groups --recurse 1") { + inventoryRequests += 1; + if (mode === "clean") reply("^done,groups=[]"); + else if (mode === "malformed") reply('^error,msg="unavailable"'); + else if (inventoryRequests <= 2) reply(liveGroup); + else note("inventory-request"); + } else if (request === "-gdb-exit") { + process.stdout.write(token + "^exit\\n"); + process.exit(0); + } else { + note("unmodeled " + line); + if (token !== undefined) reply('^error,msg="unmodeled request"'); + } +}); +`; + +const debuggers = { + rizin: { + environmentKey: "REA_RIZIN_COMMAND", + source: fakeRizin, + arguments: (tty: string) => [ + "debug-with-rizin", + process.execPath, + "dr", + "--inferior-tty", + tty, + "--format", + "json", + ], + operation: "rizin_debug_command", + startTool: { + name: "start_rizin_debug_session", + arguments: (tty: string) => ({ + path: process.execPath, + inferior_tty: tty, + }), + }, + }, + gdb: { + environmentKey: "REA_GDB_COMMAND", + source: fakeGdb, + arguments: (tty: string) => [ + "debug-with-gdb", + "show version", + "--inferior-tty", + tty, + "--format", + "json", + ], + operation: "gdb_console", + startTool: { + name: "start_gdb_session", + arguments: (tty: string) => ({ inferior_tty: tty }), + }, + }, +} as const; + +type DebuggerName = keyof typeof debuggers; +type FakeMode = "clean" | "malformed" | "unanswered"; + +const EXIT_DEADLINE_MS = 20_000; +const TEST_DEADLINE_MS = 2 * EXIT_DEADLINE_MS; + +const isAlive = (pid: number): boolean => { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +}; + +/** Launch the compiled CLI against one fake debugger, on any POSIX host. */ +const prepareLaunch = async ( + name: DebuggerName, + mode: FakeMode, + compileCache: string, +) => { + const root = await createTestTempDirectory("rea-debugger-one-shot-exit-"); + const log = join(root, "debugger.log"); + await writeFile(log, ""); + const provider = join(root, name); + await writeFile(provider, debuggers[name].source); + await chmod(provider, 0o755); + // Persistent debugger sessions are Linux-only behind this gate. These modes + // stop before any /proc read, so the same workflow also runs on macOS. + const linuxGate = join(root, "linux-gate.mjs"); + await writeFile( + linuxGate, + 'Object.defineProperty(process, "platform", { value: "linux" });\n', + ); + const notes = async (): Promise => + (await readFile(log, "utf8")).split("\n").filter(Boolean); + const debuggerPid = async (): Promise => { + const line = (await notes()).find((note) => note.startsWith("pid ")); + if (line === undefined) throw new Error("The fake debugger did not start"); + return Number(line.slice("pid ".length)); + }; + onTestFinished(async () => { + const pid = await debuggerPid().catch(() => undefined); + if (pid !== undefined && isAlive(pid)) process.kill(pid, "SIGKILL"); + expect( + (await notes()).filter((note) => note.startsWith("unmodeled ")), + ).toEqual([]); + }); + return { + notes, + debuggerPid, + command: [ + "--import", + linuxGate, + resolve("scripts/rea.mjs"), + ] as readonly string[], + env: cliProcessEnvironment( + { + [debuggers[name].environmentKey]: provider, + FAKE_DEBUGGER_LOG: log, + FAKE_DEBUGGER_MODE: mode, + }, + compileCache, + ), + }; +}; + +/** Comparing with the formatter alone would still pass if it dropped the process. */ +const expectNamesProcess = (guidance: string, pid: number): void => { + expect(guidance).toMatch(new RegExp(`\\b${String(pid)}\\b`, "u")); +}; + +const expectRetainedOutcome = ( + name: DebuggerName, + result: ProcessResult, + pid: number, +): void => { + expect(JSON.parse(result.stdout)).toMatchObject({ + code: "cleanup_incomplete", + retryable: false, + remediation: { action: retainedDebuggerRemediation([pid]) }, + details: { + operation: debuggers[name].operation, + cleanup: "incomplete", + partial_observation: { + operation: debuggers[name].operation, + normalized_result: { completion_status: "complete" }, + }, + }, + }); + expectNamesProcess(retainedDebuggerRemediation([pid]), pid); + expect(result.stdout).toContain(`"retained_debugger_pids":[${String(pid)}]`); +}; + +/** Serve one MCP stdio client that has started a debugger session. */ +const startMcpSession = async ( + processes: TestProcesses, + name: DebuggerName, + launch: Awaited>, + tty: string, +) => { + const child = processes.spawn(process.execPath, [...launch.command, "mcp"], { + env: launch.env, + }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk: Buffer) => (stdout += String(chunk))); + child.stderr.on("data", (chunk: Buffer) => (stderr += String(chunk))); + const request = async ( + id: number, + method: string, + params: object, + ): Promise => { + child.stdin.write( + `${JSON.stringify({ jsonrpc: "2.0", id, method, params })}\n`, + ); + await expect + .poll(() => stdout, { timeout: EXIT_DEADLINE_MS }) + .toContain(`"id":${String(id)}`); + }; + await request(1, "initialize", { + protocolVersion: "2025-06-18", + capabilities: {}, + clientInfo: { name: "debugger-one-shot-exit", version: "1" }, + }); + child.stdin.write( + `${JSON.stringify({ jsonrpc: "2.0", method: "notifications/initialized" })}\n`, + ); + const { startTool } = debuggers[name]; + await request(2, "tools/call", { + name: startTool.name, + arguments: startTool.arguments(tty), + }); + expect(stdout).toContain("session_id"); + return { child, stderr: () => stderr }; +}; + +describe.skipIf(process.platform === "win32")( + "debugger processes that outlive their cleanup", + () => { + cliTest.for([ + ["rizin", "malformed"], + ["rizin", "unanswered"], + ["gdb", "malformed"], + ["gdb", "unanswered"], + ] as const)( + "ends one-shot %s with a retained debugger whose cleanup is %s", + { timeout: TEST_DEADLINE_MS }, + async ([name, mode], { processes, cliCompileCache }) => { + const tty = await rizinTestInferiorTty(); + const launch = await prepareLaunch(name, mode, cliCompileCache); + + const result = await processes.run( + process.execPath, + [...launch.command, ...debuggers[name].arguments(tty)], + { env: launch.env, timeoutMs: EXIT_DEADLINE_MS }, + ); + + expect(result).toMatchObject({ exitCode: 1, signal: null }); + const pid = await launch.debuggerPid(); + expectRetainedOutcome(name, result, pid); + // REA reports the debugger as retained and did not stop it. Exiting + // closes its command stream; that is the only thing it sees from REA. + await expect + .poll(launch.notes, { timeout: EXIT_DEADLINE_MS }) + .toContain("stdin-end"); + expect(await launch.notes()).not.toContain("sigterm"); + expect(isAlive(pid)).toBe(true); + }, + ); + + cliTest.for(["rizin", "gdb"] as const)( + "keeps a one-shot %s success and its exit status when cleanup is confirmed", + { timeout: TEST_DEADLINE_MS }, + async (name, { processes, cliCompileCache }) => { + const tty = await rizinTestInferiorTty(); + const launch = await prepareLaunch(name, "clean", cliCompileCache); + + const result = await processes.run( + process.execPath, + [...launch.command, ...debuggers[name].arguments(tty)], + { env: launch.env, timeoutMs: EXIT_DEADLINE_MS }, + ); + + expect(result).toMatchObject({ exitCode: 0, signal: null }); + expect(JSON.parse(result.stdout)).toMatchObject({ + operation: debuggers[name].operation, + normalized_result: { completion_status: "complete" }, + }); + expect(result.stdout).not.toContain("retained_debugger_pids"); + const pid = await launch.debuggerPid(); + await expect + .poll(() => isAlive(pid), { timeout: EXIT_DEADLINE_MS }) + .toBe(false); + }, + ); + + cliTest.for(["rizin", "gdb"] as const)( + "ends one-shot %s with its signal status when interrupted during final cleanup", + { timeout: TEST_DEADLINE_MS }, + async (name, { processes, cliCompileCache }) => { + const tty = await rizinTestInferiorTty(); + const launch = await prepareLaunch(name, "unanswered", cliCompileCache); + + const child = processes.spawn( + process.execPath, + [...launch.command, ...debuggers[name].arguments(tty)], + { env: launch.env }, + ); + let stdout = ""; + child.stdout.on("data", (chunk: Buffer) => (stdout += String(chunk))); + child.stdin.end(); + // The unanswered request is issued by final cleanup, after the command. + await expect + .poll(launch.notes, { timeout: EXIT_DEADLINE_MS }) + .toContain("inventory-request"); + child.kill("SIGINT"); + await expect + .poll(() => child.exitCode, { timeout: EXIT_DEADLINE_MS }) + .toBe(130); + await expect + .poll(() => stdout, { timeout: EXIT_DEADLINE_MS }) + .toContain("retained_debugger_pids"); + + const pid = await launch.debuggerPid(); + expectRetainedOutcome( + name, + { exitCode: 130, signal: null, stdout, stderr: "" }, + pid, + ); + expect(await launch.notes()).not.toContain("sigterm"); + expect(isAlive(pid)).toBe(true); + }, + ); + + cliTest.for([ + ["rizin", "client disconnect"], + ["gdb", "client disconnect"], + ["rizin", "SIGTERM"], + ] as const)( + "ends the stdio MCP server holding a retained %s session on %s and names it", + { timeout: TEST_DEADLINE_MS }, + async ([name, trigger], { processes, cliCompileCache }) => { + const tty = await rizinTestInferiorTty(); + const launch = await prepareLaunch(name, "malformed", cliCompileCache); + + const { child, stderr } = await startMcpSession( + processes, + name, + launch, + tty, + ); + + if (trigger === "SIGTERM") child.kill("SIGTERM"); + else child.stdin.end(); + + await expect + .poll(() => child.exitCode, { timeout: EXIT_DEADLINE_MS }) + .toBe(1); + const pid = await launch.debuggerPid(); + // Nothing else reports these once the server has exited. + await expect + .poll(stderr, { timeout: EXIT_DEADLINE_MS }) + .toContain(`${retainedDebuggerRemediation([pid])}\n`); + expectNamesProcess(retainedDebuggerRemediation([pid]), pid); + const logs = stderr() + .split("\n") + .filter((line) => line.startsWith("{")) + .map((line): unknown => JSON.parse(line)); + for (const msg of [ + "Provider cleanup failed during server shutdown", + MCP_SHUTDOWN_FAILED, + ]) + expect(logs).toContainEqual( + expect.objectContaining({ msg, retained_debugger_pids: [pid] }), + ); + expect(await launch.notes()).not.toContain("sigterm"); + expect(isAlive(pid)).toBe(true); + }, + ); + }, +); diff --git a/tests/boundary/cli/reverseEngineeringCommands.test.ts b/tests/boundary/cli/reverseEngineeringCommands.test.ts index 77a7ceef67c..854daed96fe 100644 --- a/tests/boundary/cli/reverseEngineeringCommands.test.ts +++ b/tests/boundary/cli/reverseEngineeringCommands.test.ts @@ -552,7 +552,16 @@ it.each(["incomplete", "interrupt", "control"] as const)( rawResult: { output: "registers" }, }, ); - const sessions = new Map([["fixture-session", { id: "fixture-session" }]]); + const sessions = new Map([ + [ + "fixture-session", + { + id: "fixture-session", + launched: { process: { pid: 4242 } }, + supervisor: { ref: vi.fn(), unref: vi.fn() }, + }, + ], + ]); vi.spyOn(RizinDebugSessionManager.prototype, "start").mockResolvedValue( ok({ session_id: "fixture-session", backend: null }), ); @@ -642,6 +651,7 @@ it.each(["incomplete", "interrupt", "control"] as const)( details: { cleanup: "incomplete", resources: ["fixture-session"], + diagnostics: { retained_debugger_pids: [4242] }, partial_observation: { operation: "rizin_debug_command", parameters: { session_id: "fixture-session" }, diff --git a/tests/boundary/filesystem/ciChangeScope.test.ts b/tests/boundary/filesystem/ciChangeScope.test.ts index 03b936b5d06..1be899b47ff 100644 --- a/tests/boundary/filesystem/ciChangeScope.test.ts +++ b/tests/boundary/filesystem/ciChangeScope.test.ts @@ -346,6 +346,7 @@ it.each([ "tests/support/reverseEngineeringService.ts", "tests/support/ownedRizinCommandEvidenceTests.ts", "tests/support/reverseDebuggerMcpTestHelpers.ts", + "tests/boundary/cli/debuggerOneShotExit.test.ts", "tests/boundary/cli/gdbStartupCleanupCli.test.ts", "vitest.reverse-tools.config.ts", "vitest.reverse-tools-semantics.config.ts", diff --git a/tests/boundary/mcp/shutdown.test.ts b/tests/boundary/mcp/shutdown.test.ts index 21222d7bd5a..bf01cebbd25 100644 --- a/tests/boundary/mcp/shutdown.test.ts +++ b/tests/boundary/mcp/shutdown.test.ts @@ -6,6 +6,10 @@ import { expect, it } from "vitest"; import { createManagedBinarySession } from "../../../src/composition/binary.js"; import { MCP_SHUTDOWN_FAILED } from "../../../src/main/messages.js"; import { createShutdown } from "../../../src/main/shutdown.js"; +import { + OwnedDebuggerCleanupError, + retainedDebuggerRemediation, +} from "../../../src/process/ownedDebuggerCleanupError.js"; import { registerProcessShutdown } from "../../../src/main/processShutdown.js"; import type { RuntimeDependencies } from "../../../src/main/types.js"; import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; @@ -75,6 +79,79 @@ it("keeps shutdown output stable while logging the rejected cause at debug level ); }); +it("names retained debuggers in stderr and every log level when shutdown ends", async () => { + const logs: string[] = []; + const output: string[] = []; + const requests: Array<() => void> = []; + const retained = new OwnedDebuggerCleanupError( + [new Error("detach unverified")], + "One or more owned GDB processes did not stop cleanly", + [4242], + ); + const dependencies: RuntimeDependencies = { + env: {}, + serve: () => { + throw new Error("unused fixture transport"); + }, + writeStderr: (text) => output.push(text), + setExitCode: () => undefined, + registerShutdown: (request) => { + requests.push(request); + return () => undefined; + }, + }; + createShutdown({ + // Runtime owners wrap this failure before it reaches the shutdown report. + handle: { + close: () => + Promise.reject( + new AggregateError( + [new AggregateError([retained], "REA provider cleanup failed")], + "REA server cleanup failed", + ), + ), + }, + session: createManagedBinarySession(), + dependencies, + serverLogger: pino( + { level: "debug" }, + { write: (line) => logs.push(line) }, + ), + }); + + requests[0]?.(); + await new Promise((resolve) => setImmediate(resolve)); + + expect(output).toEqual([ + `${MCP_SHUTDOWN_FAILED}\n`, + `${retainedDebuggerRemediation([4242])}\n`, + ]); + // Comparing with the formatter alone would still pass if it dropped the process. + expect(output[1]).toMatch(/\b4242\b/u); + const parsed = logs.map((line) => JSON.parse(line)); + expect(parsed).toContainEqual( + expect.objectContaining({ + level: 50, + msg: MCP_SHUTDOWN_FAILED, + retained_debugger_pids: [4242], + }), + ); + expect(parsed).toContainEqual( + expect.objectContaining({ + level: 20, + failure_cause: expect.objectContaining({ + errors: [ + expect.objectContaining({ + errors: [ + expect.objectContaining({ retained_debugger_pids: [4242] }), + ], + }), + ], + }), + }), + ); +}); + it("reports a typed binary cleanup failure and retains the client for retry", async () => { const [target] = await createBinarySessionTargets(); const failure = new ProviderCleanupError("fixture", ["owned-runtime"], { diff --git a/tests/boundary/process/ownedDebuggerSession.test.ts b/tests/boundary/process/ownedDebuggerSession.test.ts index 155f7457e16..c46a66d7ab1 100644 --- a/tests/boundary/process/ownedDebuggerSession.test.ts +++ b/tests/boundary/process/ownedDebuggerSession.test.ts @@ -9,8 +9,14 @@ import { spawn as spawnPty } from "@lydell/node-pty"; import { expect, it, onTestFinished, vi } from "vitest"; +import { + OwnedDebuggerCleanupError, + retainedDebuggerProcessIds, + retainedDebuggerRemediation, +} from "../../../src/process/ownedDebuggerCleanupError.js"; import { closeOwnedDebuggerSessions, + type OwnedDebuggerStopResult, stopOwnedDebuggerProcess, verifyOwnedInferiorsEnded, } from "../../../src/process/ownedDebuggerSession.js"; @@ -29,6 +35,15 @@ const createFakeSupervisor = () => ({ limitAdditionalOutputBytes: vi.fn(), dispose: vi.fn(), unrefOutputStreams: vi.fn(), + ref: vi.fn(), + unref: vi.fn(), +}); + +/** A session reduced to what closing all sessions reads from it. */ +const closableSession = (id: string, pid?: number) => ({ + id, + launched: { process: { pid } }, + supervisor: { ref: vi.fn(), unref: vi.fn() }, }); const startStopProbe = (source: string, pipeInput = false) => { @@ -533,7 +548,7 @@ it("retains an exited debugger after provider detach failed", async () => { get exited() { return exited; }, - launched: { process: { kill: () => true } }, + launched: { process: { pid: 101, kill: () => true } }, detachInferior: async () => { throw new Error("MI detach failed"); }, @@ -545,6 +560,8 @@ it("retains an exited debugger after provider detach failed", async () => { disposals += 1; }, unrefOutputStreams: () => {}, + ref: () => {}, + unref: () => {}, }, }; const sessions = new Map([[session.id, session]]); @@ -580,7 +597,7 @@ it("recovers failed detach only after the identified inferior ends and output se get exited() { return exited; }, - launched: { process: { kill: vi.fn(() => true) } }, + launched: { process: { pid: 101, kill: vi.fn(() => true) } }, detachInferior: async () => { throw new Error("detach failed"); }, @@ -765,7 +782,7 @@ it("retains an exited session while a timed-out detach attempt is still pending" get exited() { return exited; }, - launched: { process: { kill: () => true } }, + launched: { process: { pid: 101, kill: () => true } }, detachInferior: () => { detachCalls += 1; return new Promise<{ readonly status: "no-inferior" }>((resolve) => { @@ -778,6 +795,8 @@ it("retains an exited session while a timed-out detach attempt is still pending" waitForOutputClose: async () => true, dispose: vi.fn(), unrefOutputStreams: vi.fn(), + ref: vi.fn(), + unref: vi.fn(), }, }; const sessions = new Map([[session.id, session]]); @@ -1365,8 +1384,8 @@ it.skipIf(process.platform === "win32")( it("removes stopped sessions and retains incomplete sessions when closing all", async () => { const sessions = new Map([ - ["stopped", { id: "stopped" }], - ["incomplete", { id: "incomplete" }], + ["stopped", closableSession("stopped")], + ["incomplete", closableSession("incomplete")], ]); await expect( @@ -1385,8 +1404,8 @@ it("removes stopped sessions and retains incomplete sessions when closing all", }); it("does not remove a replacement session installed under the same key while stopping", async () => { - const original = { id: "shared-key", generation: "old" }; - const replacement = { id: "shared-key", generation: "new" }; + const original = closableSession("shared-key"); + const replacement = closableSession("shared-key"); const sessions = new Map([["shared-key", original]]); let finishStop!: (result: { readonly status: "stopped" }) => void; const stopping = new Promise<{ readonly status: "stopped" }>((resolve) => { @@ -1407,7 +1426,7 @@ it("does not remove a replacement session installed under the same key while sto }); it("closes an empty owned-session collection", async () => { - const sessions = new Map(); + const sessions = new Map>(); await expect( closeOwnedDebuggerSessions( sessions, @@ -1419,10 +1438,10 @@ it("closes an empty owned-session collection", async () => { it("settles every stop and retains rejected sessions while removing successful ones", async () => { const sessions = new Map([ - ["stopped", { id: "stopped" }], - ["rejected", { id: "rejected" }], - ["string-rejected", { id: "string-rejected" }], - ["incomplete", { id: "incomplete" }], + ["stopped", closableSession("stopped")], + ["rejected", closableSession("rejected")], + ["string-rejected", closableSession("string-rejected")], + ["incomplete", closableSession("incomplete")], ]); const calls: string[] = []; @@ -1455,3 +1474,88 @@ it("settles every stop and retains rejected sessions while removing successful o "incomplete", ]); }); + +it("stops retained sessions holding the host and names them, leaving stopped peers alone", async () => { + const stopped = closableSession("stopped", 101); + const rejected = closableSession("rejected", 102); + const incomplete = closableSession("incomplete", 103); + const unidentified = closableSession("unidentified"); + const sessions = new Map( + [stopped, rejected, incomplete, unidentified].map((session) => [ + session.id, + session, + ]), + ); + + const failure: unknown = await closeOwnedDebuggerSessions( + sessions, + async (session) => { + if (session === stopped) return { status: "stopped" }; + if (session === rejected) throw new Error("signal failed"); + return { status: "incomplete", reason: "still alive" }; + }, + "Rizin", + ).catch((cause: unknown) => cause); + + expect(failure).toBeInstanceOf(OwnedDebuggerCleanupError); + expect(failure).toBeInstanceOf(AggregateError); + expect((failure as OwnedDebuggerCleanupError).retainedProcessIds).toEqual([ + 102, 103, + ]); + expect(stopped.supervisor.unref).not.toHaveBeenCalled(); + for (const retained of [rejected, incomplete, unidentified]) + expect(retained.supervisor.unref).toHaveBeenCalledOnce(); + expect([...sessions.keys()]).toEqual([ + "rejected", + "incomplete", + "unidentified", + ]); +}); + +it("holds the host again while retrying a retained session and leaves it held on success", async () => { + const session = closableSession("retained", 101); + const sessions = new Map([[session.id, session]]); + const order: string[] = []; + session.supervisor.ref.mockImplementation(() => order.push("ref")); + session.supervisor.unref.mockImplementation(() => order.push("unref")); + let stopResult: OwnedDebuggerStopResult = { + status: "incomplete", + reason: "still alive", + }; + const stop = async () => { + order.push("stop"); + return stopResult; + }; + + await expect( + closeOwnedDebuggerSessions(sessions, stop, "GDB"), + ).rejects.toBeInstanceOf(OwnedDebuggerCleanupError); + expect(order).toEqual(["ref", "stop", "unref"]); + + order.length = 0; + stopResult = { status: "stopped" }; + await closeOwnedDebuggerSessions(sessions, stop, "GDB"); + expect(order).toEqual(["ref", "stop"]); + expect(sessions.size).toBe(0); +}); + +it("finds retained debugger IDs through composed failures without looping on cycles", () => { + const first = new OwnedDebuggerCleanupError([], "first", [101, 102]); + const second = new OwnedDebuggerCleanupError([], "second", [102, 103]); + const cyclic = new Error("cyclic", { cause: second }); + Object.assign(second, { cause: cyclic }); + const composed = new AggregateError( + [first, new Error("wrapped", { cause: cyclic }), new Error("peer"), null], + "outer", + ); + + expect(retainedDebuggerProcessIds(composed)).toEqual([101, 102, 103]); + expect(retainedDebuggerProcessIds(new Error("peer"))).toEqual([]); + expect(retainedDebuggerProcessIds(undefined)).toEqual([]); +}); + +it("names every retained debugger in its remediation", () => { + const remediation = retainedDebuggerRemediation([4242, 4243]); + expect(remediation).toMatch(/\b4242\b/u); + expect(remediation).toMatch(/\b4243\b/u); +});