Repository navigation
fix(server): close undisclosed sessions when an open fails - #136
Merged
Merged
Conversation
Three corrections to the rollback, found while reviewing it: - Roll back on failure only. The rollback ran from a `finally`, so a successful open also went through the worker pool a second time, costing another thread hop and an uncancellable wait for a limiter token. - Never resume a live target. Closing a session normally lets the target run again, which is what `close_kd_session`'s `resume` parameter is for. An open that failed never handed out a session id, so resuming on its behalf is a side effect the caller cannot opt out of. CTRL+B detaches *and* resumes, so a live close with resume disabled now writes nothing: dropping the client leaves the server's target exactly as it is. - Bound the tree kill. A rolled-back open runs teardown on the event loop's behalf, so an unresponsive `taskkill` must not keep the server from exiting. Also makes `cleanup` keyword-only, since as a trailing positional it could be shifted silently by a change to the settings tuple.
This was referenced Oct 8, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #125.
All four
open_*tools register their session before running init commands and triage, so a failure afterwards returns an MCP error with nosession_idwhile the session stays in_sessionswith its debugger alive. The caller never learns the id, so it can never close it, and a retry adds another one.Reproduced against the real registered tool handler on
develop, with a debugger that constructs fine and fails its first triage command the way a!pebtimeout does:After this change the same script reports 0 sessions left and 2 shutdowns.
What it does
An open now owns its debugger until initialization, triage and output filtering have all succeeded. An
ExitStackregisters the rollback alongside the session,pop_all()disarms it on success, and the rollback is shielded so cancellation cannot skip it. It only ever closes the session created by that call, so unrelated busy sessions are untouched.Two deliberate choices worth calling out:
close_kd_session'sresumeparameter is for. An open that failed never handed out an id, so resuming on its behalf would be a side effect the caller cannot opt out of. CTRL+B detaches and resumes, so a live close with resume disabled now writes nothing: dropping the client leaves the server's target exactly as it is.The tree kill is now bounded, since a rolled-back open runs teardown on the event loop's behalf and an unresponsive
taskkillmust not keep the server from exiting.Credit
The diagnosis, the reproduction and the
ExitStackdesign are @xiaozhu1337's, from #125 and #126; the first two commits here are theirs. This takes those two fix commits and leaves out the 433-lineserver_support.pyextraction that #126 also carried, so the fix can be reviewed on its own. The third commit is the follow-up above.Tests
Baseline on
developis 156 passed, 2 skipped, so this adds 33 cases: 30 covering ownership across all four open tools (init, mandatory and optional triage, output-filter and cancellation failures, repeated retries, unrelated-session preservation, shutdown errors) and 3 covering the release-target policy. Live tests included, driving realcdbandkd.