Repository navigation
fix(debugger): let REA exit when a debugger could not be stopped - #8652
Conversation
`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 <noreply@anthropic.com>
kaoru0822-kitauji
added
bug
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The PR appears safe to merge, with the previous finding fixed and no new actionable issues found. SummaryThis PR lets REA exit after debugger cleanup fails while reporting the retained processes.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI final cleanup or MCP shutdown"] --> B["Restore process and pipe references"]
B --> C["Try to close each debugger"]
C --> D{"Cleanup confirmed?"}
D -->|Yes| E["Remove the stopped session"]
D -->|No| F["Keep ownership and release event-loop references"]
F --> G["Report retained PIDs"]
G --> H["REA can exit with a nonzero status"]
Reviews (4) · Last reviewed commit: "test(debugger): compare retained-debugge..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // 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`); |
There was a problem hiding this comment.
The new checks here and in shutdown.test.ts hard-code “Check process … and its target”. docs/testing.md requires guidance checks to use the owning formatter or constant, alongside independent checks of the facts. Compare with retainedDebuggerRemediation([pid]) while keeping the separate process-ID and exit checks. This avoids failures on harmless wording changes. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/boundary/cli/debuggerOneShotExit.test.ts
Line: 398
Comment:
**Wording edits break tests**
The new checks here and in `shutdown.test.ts` hard-code “Check process … and its target”. `docs/testing.md` requires guidance checks to use the owning formatter or constant, alongside independent checks of the facts. Compare with `retainedDebuggerRemediation([pid])` while keeping the separate process-ID and exit checks. This avoids failures on harmless wording changes. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/morluto/rea/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Confirmed and fixed in 245860975, a test-only change. docs/testing.md asks guidance checks to compare with the owning formatter, alongside independent checks of the facts.
Root cause: the CLI-level MCP test and shutdown.test.ts matched the sentence "Check process … and its target", so a harmless rewording would have failed them.
Fix:
- Those two tests and the one-shot
remediation.actioncheck now compare withretainedDebuggerRemediation([pid]), exported fromsrc/process/ownedDebuggerCleanupError.ts. Delivery adds no wrapping: stderr is that string plus a newline, and the CLI'sremediation.actionis that string exactly. - A formatter comparison alone would still pass if the formatter dropped the process, so each of those tests also checks that the guidance names the PID as a whole word. A new module test does the same for several PIDs.
- The separate exit status,
retained_debugger_pidsand fake-process checks are unchanged.
Validation:
- With the formatter changed to drop the PID, and the runtime rebuilt, 4 CLI and MCP tests, the
shutdowntest and the module test all fail. - With the wording changed and the PID kept, those same tests pass.
- With the real formatter:
debuggerOneShotExit11/11 in 3 runs,shutdown8/8,ownedDebuggerSession44/44, andcheck:fastandformat:checkpass.
Main had not moved, so there is no merge in this push. No runtime code changed, and the source file is back to its committed state. Leaving this thread open for review.
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 <noreply@anthropic.com>
What broke: When a one-shot debugger command could not confirm its debugger stopped,
rea debug-with-rizinandrea debug-with-gdbprintedcleanup_incompleteand then never exited. A stdio MCP server whose client disconnected hung the same way.The fix:
closeOwnedDebuggerSessions, which both managers finish through, now releases retained sessions from the event loop, as supervised providers already do, and names their PIDs. The process exits 1, or 130/143 on a signal for the one-shot commands, and REA does not stop the debugger. The one-shot result and the MCP shutdown report both name the process.The risk: A retained debugger now outlives REA with its pipes closed. Real Rizin and GDB were not run, so what they do on that EOF is unverified. Every exit path names the process to check.
Summary
No issue is linked because none exists for this general behavior. It was first seen while working on #8642, whose reporter saw
rea debug-with-rizin /usr/bin/cat dr --inferior-tty <pty> --format jsonprint its result and stay hung at 30 s. #8647 removes that one trigger. This PR covers any incomplete debugger cleanup, including the refusal #8647 adds.An agent or script waiting on the CLI blocked forever, with no sign that the result was already final. Now it gets the same result, a nonzero exit, and the process to check.
Problem and expected behavior
Smallest trigger: a debugger that does not confirm detach, for example one that answers
oljwith malformed JSON or stops answering. Observed onf95bdbce7throughscripts/rea.mjswith a fake launcher: the result was printed,process.exitCodewas 1, and the process was still running 8 s later.process.getActiveResourcesInfo()listedProcessWrapand the child's pipes.The violated invariant is the one
ProviderProcessSupervisor.unrefalready states: incomplete cleanup does not prevent host exit. Debugger sessions never reached it, because they stop throughstopOwnedDebuggerProcesswithownsProcessLifetime: false.Change and scope
src/process/ownedDebuggerSession.ts: whencloseOwnedDebuggerSessionsretains a session, it unreferences that session's process and pipes. A retry restores them first. It now throwsOwnedDebuggerCleanupError, anAggregateErrorthat also carries the retained PIDs. Stopped peers are not touched.src/process/ownedDebuggerCleanupError.ts, new: that error type and the one reader that finds its PIDs through composed failures, since runtime owners wrap it in further aggregates before anything reports it. It has no dependencies becausesrc/main/shutdown.tsloads on the deferred startup path.src/cli/reverseEngineeringCommands.ts:closeDebuggerCommandSessionaddsretained_debugger_pidsand a remediation naming the process. The session UUID inresourcesmeans nothing once the process has ended.src/main/shutdown.ts,src/server/createServer.ts: the MCP shutdown report writes the same remediation to stderr and addsretained_debugger_pidsto its error log, its debugfailure_cause, and the server's onclose log. Before this, those logs said only "REA provider cleanup failed", so an exiting server named nothing.src/gdb/GdbSessionManager.ts: a request issued during cleanup no longer holds the host until its 120 s command deadline. Without this, an unanswered detach kept GDB's process alive after the result, with only aTimeoutleft active. Cleanup already bounds that wait at 1000 ms. Rizin's cleanup requests are already bounded, so its manager is unchanged.Ownership. The
AGENTS.mdrule to retain ownership of failed cleanup still holds: the session stays in the map and retryable for as long as the owner lives, and an MCP session's ordinaryclosekeeps its references. A one-shot process's life ends with its result, so it reports what it could not release. It sends no signal and does not claim cleanup.Not included:
Overlap with open PRs, from reading their diffs; none is depended on:
closeOwnedDebuggerSessions, so they need reconciling.Scope: ordinary fix within the existing debugger boundary; no maintainer scope decision is required.
Contract and boundary impact
closeOwnedDebuggerSessionsowns the final release;closeDebuggerCommandSessionowns the one-shot report. Consumers are both debugger commands and MCP shutdown. No superseded path to remove.retained_debugger_pidsand a more specificremediation.action, and MCP shutdown gains the same process naming on stderr and in logs. Exit changes from never to 1, 130 or 143 for the CLI, and to 1 for an MCP server, including on a signal.partial_observation.Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.
Evidence and regression coverage
All of this was executed unless marked source-derived.
Tests added or updated:
tests/boundary/cli/debuggerOneShotExit.test.tsspawns the compiledscripts/rea.mjswith fake Rizin and GDB launchers and a real PTY. The fakes model each request REA sends and use real reply shapes. A request they do not model fails the test. For both debuggers it covers:tests/boundary/process/ownedDebuggerSession.test.tsgains mixed outcomes, retry ordering, and finding PIDs through composed and cyclic failures.tests/boundary/mcp/shutdown.test.tsgains the report at stderr, error and debug level for a nested failure.reverseEngineeringCommandsincomplete case now asserts the PID.Base reproduction:
f95bdbce7: 8 of the 10 CLI tests failed by exceeding their 20 s deadline, the 2 clean controls passed, and the 2 module tests and the extended CLI case failed.origin/main: all 4 selected retained-debugger cases fail on the same deadline (malformed and MCP disconnect, both debuggers), both clean controls pass, and the newshutdowntest fails.Mutants, each failing the tests that should catch it:
ref(): 1 module testshutdowntestfailure_cause: theshutdowntestdror-gdb-set auto-load off: both clean controls, with "unmodeled" in the failureNative Linux: on
1f317ff78, all 8Test shardjobs passed on Ubuntu. Shard 8/8 randebuggerOneShotExit.test.ts(10 tests, none skipped) and shard 5/8 ranownedDebuggerSession.test.ts(42 tests). That was before the changes above, so the current tests are confirmed on Linux only by the CI run on this head. The launchers are Node scripts with no shell, so they cannot hit a dash versus bash difference.Runs on this Mac: all local runs were on macOS. Persistent debuggers are Linux-gated, so the CLI tests open that gate with an
--importpreload. Each mode stops before any/procread, so the test does not depend on/proc. Windows skips the file because it needs a PTY.Scratch counterexamples, not committed, through the same entrypoint and all ending on their own:
capability_unavailable, debugger stopped by SIGTERMrea mcp --http, Rizin SIGTERM and GDB SIGINTUser-visible output, trimmed:
{"code":"cleanup_incomplete","retryable":false, "remediation":{"action":"REA exits without confirming that the debugger stopped, and no later REA command can retry this cleanup. Check process 14876 and its target, and stop them if they are still running."}, "details":{"cleanup":"incomplete","resources":["45d558a0-…"], "diagnostics":{"reason":"Owned Rizin debugger inferior detach could not be verified: Rizin returned malformed JSON for `olj`.","retained_debugger_pids":[14876]}, "partial_observation":{"operation":"rizin_debug_command", "…":"…"}}}When the command itself also failed or was interrupted, the field is at
details.diagnostics.cleanup_error.details.diagnostics.retained_debugger_pids.remediation.actionnames the process in each retained case I ran.MCP shutdown, stderr trimmed. This report already runs once per runtime owner, so the lines can repeat; I left that as it was.
Remaining proof gaps:
catcase was not rerun.reportMcpShutdownFailure, whichshutdown.test.tscovers directly.Applicable lifecycle regressions: cleanup failure after success, with the observation and remaining resources retained; cancellation during final cleanup; mixed child outcomes; cleanup retry at module level. CLI retry does not exist, since the process ends.
For evidence-bearing changes:
Validation performed
All on the merged head
236d2263f, which includesorigin/mainat2ac6d3efe:245860975:debuggerOneShotExit11/11 in 3 runs,shutdown8/8,ownedDebuggerSession44/44,check:fastandformat:checkpassed. The guidance checks now compare withretainedDebuggerRemediationand separately check that the PID is named: a formatter that drops the PID fails them, and a harmless rewording passes.The counts below are from the earlier run on that merged head:
vitest --project cli-boundary tests/boundary/cli/debuggerOneShotExit.test.ts, 5 runs: 11/11 each.gdbStartupCleanupCli,reverseEngineeringCommands,commandEvidenceDelivery,cliToolCoverage: 44 passed.ownedDebuggerSession,providerProcess,mcpHttpShutdown: 72 passed.tests/boundary/providers: 319 passed, 24 skipped.shutdown,optionalProviderShutdown,mainErrors,serverLifecycleMcpand the MCP debugger suites: 83 passed.npm run test:focused -- tests/boundary/mcp/deferredRuntime.test.ts: 28 passed. It needs a prepared catalog, so it ran separately; a plainvitestrun stops with ENOENT on.cache/mcp-tool-catalog.json.mainStartupCancellation: 10 passed.tests/composition/reverseEngineering*: 125 passed.ciChangeScope,reverseToolsMutationConfig,productionReachabilityReport: 367 passed.npm run check:fast,npm run format:check,npm run docs:check,npm run metadata:check: all passed.One earlier failure, now explained:
cancels and drains GDB starts before closeAll resolvesfailed once under load with "fake GDB did not start". This PR does not cause it, for two reasons:closeAll.startissues no request withcleanupStartedset, so neither event-loop change has run. The 10 s hook timeout that followed is the test's ownstopGate, never released once it has thrown.f95bdbce7, it failed that way in 2 of 8 runs. With the fix it failed in 0 of 8. Those counts do not show a difference between the two, only that base fails the same way.It is an existing load-sensitive wait, which I left alone.
No cost measurement: analysis work is unchanged, and the success path adds one
ref()per session.Compatibility, safety, and release
Author readiness
Human owner: @kaoru0822-kitauji
Declarations entered by Claude Code on the owner's standing written instruction.
Before merge
type(scope): outcome.🤖 Generated with Claude Code