From 1f317ff78f7759b64e32e838486af2df53772f20 Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Mon, 2 Nov 2026 07:37:44 +0000 Subject: [PATCH 1/3] fix(debugger): let REA exit when a debugger could not be stopped `rea debug-with-gdb` and `rea debug-with-rizin` printed cleanup_incomplete and set exit status 1, but the retained child and its pipes kept the event loop alive, so the process never exited. A stdio MCP server whose client disconnected hung the same way. closeOwnedDebuggerSessions is the owner both managers finish through. It now unreferences retained sessions, as supervised providers already do after incomplete cleanup, restores the references on retry, and names the retained process IDs. The one-shot commands report those IDs, since no later command can retry. Debugger ownership, signals and detach behavior are unchanged. GDB cleanup requests also no longer hold the host until their 120 s command deadline, because cleanup already bounds its own wait. Co-Authored-By: Claude Opus 5.5 --- docs/cli.md | 10 + scripts/ci/scopes.mjs | 1 + src/cli/reverseEngineeringCommands.ts | 19 +- src/gdb/GdbSessionManager.ts | 2 + src/process/ownedDebuggerSession.ts | 65 +++- .../boundary/cli/debuggerOneShotExit.test.ts | 361 ++++++++++++++++++ .../cli/reverseEngineeringCommands.test.ts | 12 +- .../boundary/filesystem/ciChangeScope.test.ts | 1 + .../process/ownedDebuggerSession.test.ts | 103 ++++- 9 files changed, 543 insertions(+), 31 deletions(-) create mode 100644 tests/boundary/cli/debuggerOneShotExit.test.ts diff --git a/docs/cli.md b/docs/cli.md index e1575faa0e9..26a31463324 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -569,6 +569,16 @@ 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. A stdio MCP server whose client disconnects likewise exits with status 1 +instead of waiting on a debugger it could not stop, and logs the failure. + 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..1014232e412 100644 --- a/scripts/ci/scopes.mjs +++ b/scripts/ci/scopes.mjs @@ -103,6 +103,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..e3b28040670 100644 --- a/src/cli/reverseEngineeringCommands.ts +++ b/src/cli/reverseEngineeringCommands.ts @@ -28,6 +28,7 @@ import { createRizinDebugSessionManager, } from "../composition/debuggerSessions.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { OwnedDebuggerCleanupError } from "../process/ownedDebuggerSession.js"; import { analysisEvidenceDeliveryOptions, cliEvidenceDeliveryInputError, @@ -144,6 +145,13 @@ 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 = + cause instanceof OwnedDebuggerCleanupError + ? cause.retainedProcessIds + : []; + const identified = retainedPids.length > 0; throw new ProviderCleanupError( providerId, sessionId === undefined ? [] : [sessionId], @@ -158,8 +166,17 @@ const closeDebuggerCommandSession = async ( : cause instanceof Error ? cause.message : String(cause), + ...(identified ? { retained_debugger_pids: [...retainedPids] } : {}), + }, + { + cause, + operation, + ...(identified + ? { + remediationAction: `REA exits without confirming that the debugger stopped, and no later REA command can retry this cleanup. Check process ${retainedPids.join(", ")} and its target, and stop them if they are still running.`, + } + : {}), }, - { 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/process/ownedDebuggerSession.ts b/src/process/ownedDebuggerSession.ts index 009bb42e829..6a1899f35ed 100644 --- a/src/process/ownedDebuggerSession.ts +++ b/src/process/ownedDebuggerSession.ts @@ -364,42 +364,73 @@ 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 }; +} + +/** 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; + } +} + /** * 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/tests/boundary/cli/debuggerOneShotExit.test.ts b/tests/boundary/cli/debuggerOneShotExit.test.ts new file mode 100644 index 00000000000..4dc71259879 --- /dev/null +++ b/tests/boundary/cli/debuggerOneShotExit.test.ts @@ -0,0 +1,361 @@ +import { chmod, readFile, writeFile } from "node:fs/promises"; +import { join, resolve } from "node:path"; + +import { describe, expect, onTestFinished } from "vitest"; + +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { + cliProcessEnvironment, + cliTest, +} from "../../support/cli/cliFixture.js"; +import type { ProcessResult } 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. + */ +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 (silent) return; + if (line === "olj") { + note("inventory-request"); + if (mode === "unanswered") silent = true; + else process.stdout.write(mode === "clean" ? "[]\\0" : "not-json\\0"); + } else if (line.startsWith("!echo ")) process.stdout.write(line.slice(6) + "\\0"); + else if (line === "e dbg.exitkills") process.stdout.write("false\\0"); + else process.stdout.write("rax = 0x0\\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) ?? []; + if (token === undefined) return; + const reply = (tail) => process.stdout.write(token + tail + "\\n"); + 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 === "-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") { + reply("^exit"); + process.exit(0); + } else reply("^done"); +}); +`; + +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"); + }); + 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, + ), + }; +}; + +const expectRetainedOutcome = ( + name: DebuggerName, + result: ProcessResult, + pid: number, +): void => { + expect(JSON.parse(result.stdout)).toMatchObject({ + code: "cleanup_incomplete", + retryable: false, + remediation: { action: expect.stringContaining(String(pid)) }, + details: { + operation: debuggers[name].operation, + cleanup: "incomplete", + partial_observation: { + operation: debuggers[name].operation, + normalized_result: { completion_status: "complete" }, + }, + }, + }); + expect(result.stdout).toContain(`"retained_debugger_pids":[${String(pid)}]`); +}; + +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", "gdb"] as const)( + "ends the stdio MCP server after its client leaves a retained %s session", + { timeout: TEST_DEADLINE_MS }, + async (name, { processes, cliCompileCache }) => { + const tty = await rizinTestInferiorTty(); + const launch = await prepareLaunch(name, "malformed", cliCompileCache); + + const child = processes.spawn( + process.execPath, + [...launch.command, "mcp"], + { env: launch.env }, + ); + let stdout = ""; + child.stdout.on("data", (chunk: Buffer) => (stdout += 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"); + + child.stdin.end(); + + await expect + .poll(() => child.exitCode, { timeout: EXIT_DEADLINE_MS }) + .toBe(1); + const pid = await launch.debuggerPid(); + 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/process/ownedDebuggerSession.test.ts b/tests/boundary/process/ownedDebuggerSession.test.ts index 155f7457e16..5f9d1a4b13b 100644 --- a/tests/boundary/process/ownedDebuggerSession.test.ts +++ b/tests/boundary/process/ownedDebuggerSession.test.ts @@ -11,6 +11,8 @@ import { expect, it, onTestFinished, vi } from "vitest"; import { closeOwnedDebuggerSessions, + OwnedDebuggerCleanupError, + type OwnedDebuggerStopResult, stopOwnedDebuggerProcess, verifyOwnedInferiorsEnded, } from "../../../src/process/ownedDebuggerSession.js"; @@ -29,6 +31,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 +544,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 +556,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 +593,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 +778,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 +791,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 +1380,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 +1400,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 +1422,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 +1434,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 +1470,67 @@ 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); +}); From 091733a86a6b27f046c8e5025a829811f842bdd7 Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Mon, 2 Nov 2026 08:34:14 +0000 Subject: [PATCH 2/3] fix(debugger): name retained debuggers when an MCP server shuts down An MCP server that exits with a retained debugger logged only "REA provider cleanup failed" and "could not close cleanly", so nothing named the process left for the caller to check. The PIDs were on OwnedDebuggerCleanupError, but each shutdown reporter read only the outer aggregate's message, and the debug failure_cause description dropped the field too. The failure type now lives in its own dependency-free module with one reader that walks composed failures. The shutdown report, the server's onclose log, the debug description and the one-shot CLI all use it, and the shutdown report writes the same remediation the CLI returns. The fake debuggers now model each request exactly, with real reply shapes, and a request they do not model fails the test. Co-Authored-By: Claude Opus 5.5 --- docs/cli.md | 5 +- scripts/ci/scopes.mjs | 1 + src/cli/reverseEngineeringCommands.ts | 22 ++- src/main/shutdown.ts | 15 +- src/process/ownedDebuggerCleanupError.ts | 45 ++++++ src/process/ownedDebuggerSession.ts | 16 +- src/server/createServer.ts | 6 +- .../boundary/cli/debuggerOneShotExit.test.ts | 148 ++++++++++++------ tests/boundary/mcp/shutdown.test.ts | 72 +++++++++ .../process/ownedDebuggerSession.test.ts | 20 ++- 10 files changed, 271 insertions(+), 79 deletions(-) create mode 100644 src/process/ownedDebuggerCleanupError.ts diff --git a/docs/cli.md b/docs/cli.md index 26a31463324..ad67594eb4d 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -576,8 +576,9 @@ 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. A stdio MCP server whose client disconnects likewise exits with status 1 -instead of waiting on a debugger it could not stop, and logs the failure. +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 diff --git a/scripts/ci/scopes.mjs b/scripts/ci/scopes.mjs index 1014232e412..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", diff --git a/src/cli/reverseEngineeringCommands.ts b/src/cli/reverseEngineeringCommands.ts index e3b28040670..9998c2217bc 100644 --- a/src/cli/reverseEngineeringCommands.ts +++ b/src/cli/reverseEngineeringCommands.ts @@ -28,7 +28,11 @@ import { createRizinDebugSessionManager, } from "../composition/debuggerSessions.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; -import { OwnedDebuggerCleanupError } from "../process/ownedDebuggerSession.js"; +import { + retainedDebuggerDiagnostics, + retainedDebuggerProcessIds, + retainedDebuggerRemediation, +} from "../process/ownedDebuggerCleanupError.js"; import { analysisEvidenceDeliveryOptions, cliEvidenceDeliveryInputError, @@ -147,11 +151,7 @@ const closeDebuggerCommandSession = async ( } 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 = - cause instanceof OwnedDebuggerCleanupError - ? cause.retainedProcessIds - : []; - const identified = retainedPids.length > 0; + const retainedPids = retainedDebuggerProcessIds(cause); throw new ProviderCleanupError( providerId, sessionId === undefined ? [] : [sessionId], @@ -166,16 +166,14 @@ const closeDebuggerCommandSession = async ( : cause instanceof Error ? cause.message : String(cause), - ...(identified ? { retained_debugger_pids: [...retainedPids] } : {}), + ...retainedDebuggerDiagnostics(cause), }, { cause, operation, - ...(identified - ? { - remediationAction: `REA exits without confirming that the debugger stopped, and no later REA command can retry this cleanup. Check process ${retainedPids.join(", ")} and its target, and stop them if they are still running.`, - } - : {}), + ...(retainedPids.length === 0 + ? {} + : { remediationAction: retainedDebuggerRemediation(retainedPids) }), }, ); } 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 6a1899f35ed..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 = @@ -373,21 +374,6 @@ interface RetainableDebuggerSession { readonly supervisor: { ref(): void; unref(): void }; } -/** 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; - } -} - /** * Stop every owned session and retain failed sessions for possible recovery. * Callers must block replacement admission and drain in-flight starts until 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 index 4dc71259879..180bb6af7d2 100644 --- a/tests/boundary/cli/debuggerOneShotExit.test.ts +++ b/tests/boundary/cli/debuggerOneShotExit.test.ts @@ -3,12 +3,16 @@ import { join, resolve } from "node:path"; import { describe, expect, onTestFinished } from "vitest"; +import { MCP_SHUTDOWN_FAILED } from "../../../src/main/messages.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { cliProcessEnvironment, cliTest, } from "../../support/cli/cliFixture.js"; -import type { ProcessResult } from "../../support/process/processFixture.js"; +import type { + ProcessResult, + TestProcesses, +} from "../../support/process/processFixture.js"; import { rizinTestInferiorTty } from "../providers/RizinTestInferiorTty.js"; /** @@ -16,6 +20,7 @@ import { rizinTestInferiorTty } from "../providers/RizinTestInferiorTty.js"; * 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"); @@ -42,14 +47,18 @@ const fakeRizin = `${fakeDebuggerPrelude} process.stdout.write("\\0"); let silent = false; onLines((line) => { - if (silent) return; - if (line === "olj") { + 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 (line.startsWith("!echo ")) process.stdout.write(line.slice(6) + "\\0"); - else if (line === "e dbg.exitkills") process.stdout.write("false\\0"); - else process.stdout.write("rax = 0x0\\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"); + } }); `; @@ -61,14 +70,17 @@ 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) ?? []; - if (token === undefined) return; - const reply = (tail) => process.stdout.write(token + tail + "\\n"); - if (request.startsWith("-inferior-tty-set ")) { + 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=[]"); @@ -76,9 +88,12 @@ onLines((line) => { else if (inventoryRequests <= 2) reply(liveGroup); else note("inventory-request"); } else if (request === "-gdb-exit") { - reply("^exit"); + process.stdout.write(token + "^exit\\n"); process.exit(0); - } else reply("^done"); + } else { + note("unmodeled " + line); + if (token !== undefined) reply('^error,msg="unmodeled request"'); + } }); `; @@ -167,6 +182,9 @@ const prepareLaunch = async ( 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, @@ -208,6 +226,49 @@ const expectRetainedOutcome = ( 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", () => { @@ -306,53 +367,46 @@ describe.skipIf(process.platform === "win32")( }, ); - cliTest.for(["rizin", "gdb"] as const)( - "ends the stdio MCP server after its client leaves a retained %s session", + 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, { processes, cliCompileCache }) => { + async ([name, trigger], { processes, cliCompileCache }) => { const tty = await rizinTestInferiorTty(); const launch = await prepareLaunch(name, "malformed", cliCompileCache); - const child = processes.spawn( - process.execPath, - [...launch.command, "mcp"], - { env: launch.env }, - ); - let stdout = ""; - child.stdout.on("data", (chunk: Buffer) => (stdout += 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 { child, stderr } = await startMcpSession( + processes, + name, + launch, + tty, ); - const { startTool } = debuggers[name]; - await request(2, "tools/call", { - name: startTool.name, - arguments: startTool.arguments(tty), - }); - expect(stdout).toContain("session_id"); - child.stdin.end(); + 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(`Check process ${String(pid)} and its target`); + 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/mcp/shutdown.test.ts b/tests/boundary/mcp/shutdown.test.ts index 21222d7bd5a..00ba16d8f2e 100644 --- a/tests/boundary/mcp/shutdown.test.ts +++ b/tests/boundary/mcp/shutdown.test.ts @@ -6,6 +6,7 @@ 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 } 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 +76,77 @@ 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`, + expect.stringContaining("Check process 4242 and its target"), + ]); + 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 5f9d1a4b13b..8b2b37a95f5 100644 --- a/tests/boundary/process/ownedDebuggerSession.test.ts +++ b/tests/boundary/process/ownedDebuggerSession.test.ts @@ -10,8 +10,11 @@ import { spawn as spawnPty } from "@lydell/node-pty"; import { expect, it, onTestFinished, vi } from "vitest"; import { - closeOwnedDebuggerSessions, OwnedDebuggerCleanupError, + retainedDebuggerProcessIds, +} from "../../../src/process/ownedDebuggerCleanupError.js"; +import { + closeOwnedDebuggerSessions, type OwnedDebuggerStopResult, stopOwnedDebuggerProcess, verifyOwnedInferiorsEnded, @@ -1534,3 +1537,18 @@ it("holds the host again while retrying a retained session and leaves it held on 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([]); +}); From 2458609756e17b706ff32e5cb5207041c3937436 Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Mon, 2 Nov 2026 10:15:12 +0000 Subject: [PATCH 3/3] test(debugger): compare retained-debugger guidance with its formatter The new checks froze the wording "Check process ... and its target". docs/testing.md asks guidance checks to compare with the owning formatter, alongside independent checks of the facts. They now do, and each still proves the process ID is named, because a formatter comparison alone would pass if the formatter dropped it. Co-Authored-By: Claude Opus 5.5 --- tests/boundary/cli/debuggerOneShotExit.test.ts | 12 ++++++++++-- tests/boundary/mcp/shutdown.test.ts | 9 +++++++-- tests/boundary/process/ownedDebuggerSession.test.ts | 7 +++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/tests/boundary/cli/debuggerOneShotExit.test.ts b/tests/boundary/cli/debuggerOneShotExit.test.ts index 180bb6af7d2..b5d8b949f43 100644 --- a/tests/boundary/cli/debuggerOneShotExit.test.ts +++ b/tests/boundary/cli/debuggerOneShotExit.test.ts @@ -4,6 +4,7 @@ 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, @@ -205,6 +206,11 @@ const prepareLaunch = async ( }; }; +/** 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, @@ -213,7 +219,7 @@ const expectRetainedOutcome = ( expect(JSON.parse(result.stdout)).toMatchObject({ code: "cleanup_incomplete", retryable: false, - remediation: { action: expect.stringContaining(String(pid)) }, + remediation: { action: retainedDebuggerRemediation([pid]) }, details: { operation: debuggers[name].operation, cleanup: "incomplete", @@ -223,6 +229,7 @@ const expectRetainedOutcome = ( }, }, }); + expectNamesProcess(retainedDebuggerRemediation([pid]), pid); expect(result.stdout).toContain(`"retained_debugger_pids":[${String(pid)}]`); }; @@ -395,7 +402,8 @@ describe.skipIf(process.platform === "win32")( // Nothing else reports these once the server has exited. await expect .poll(stderr, { timeout: EXIT_DEADLINE_MS }) - .toContain(`Check process ${String(pid)} and its target`); + .toContain(`${retainedDebuggerRemediation([pid])}\n`); + expectNamesProcess(retainedDebuggerRemediation([pid]), pid); const logs = stderr() .split("\n") .filter((line) => line.startsWith("{")) diff --git a/tests/boundary/mcp/shutdown.test.ts b/tests/boundary/mcp/shutdown.test.ts index 00ba16d8f2e..bf01cebbd25 100644 --- a/tests/boundary/mcp/shutdown.test.ts +++ b/tests/boundary/mcp/shutdown.test.ts @@ -6,7 +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 } from "../../../src/process/ownedDebuggerCleanupError.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"; @@ -121,8 +124,10 @@ it("names retained debuggers in stderr and every log level when shutdown ends", expect(output).toEqual([ `${MCP_SHUTDOWN_FAILED}\n`, - expect.stringContaining("Check process 4242 and its target"), + `${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({ diff --git a/tests/boundary/process/ownedDebuggerSession.test.ts b/tests/boundary/process/ownedDebuggerSession.test.ts index 8b2b37a95f5..c46a66d7ab1 100644 --- a/tests/boundary/process/ownedDebuggerSession.test.ts +++ b/tests/boundary/process/ownedDebuggerSession.test.ts @@ -12,6 +12,7 @@ import { expect, it, onTestFinished, vi } from "vitest"; import { OwnedDebuggerCleanupError, retainedDebuggerProcessIds, + retainedDebuggerRemediation, } from "../../../src/process/ownedDebuggerCleanupError.js"; import { closeOwnedDebuggerSessions, @@ -1552,3 +1553,9 @@ it("finds retained debugger IDs through composed failures without looping on cyc 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); +});