Skip to content

fix(server): clean up undisclosed sessions when open fails or is cancelled - #126

Closed
xiaozhu1337 wants to merge 3 commits into
svnscha:developfrom
xiaozhu1337:fix/cleanup-failed-session-opens
Closed

xiaozhu1337 wants to merge 3 commits into
svnscha:developfrom
xiaozhu1337:fix/cleanup-failed-session-opens

Conversation

@xiaozhu1337

Copy link
Copy Markdown

What this changes

Closes #125.

Make all four open_* tools own their newly created debugger until initialization, mandatory/optional triage, output filtering, and the cancellation check succeed.

  • Use a standard-library ExitStack to register rollback alongside each session registration.
  • On failure or cancellation, use existing _close_session to remove and shut down only that call's undisclosed session.
  • Disarm rollback on success; subsequent commands and explicit close keep their existing behavior.
  • Keep filter hooks on the event-loop thread as before; run debugger creation, triage, and rollback on workers. Shield rollback from request cancellation.
  • Preserve the original open error even if ordinary shutdown raises, and retain existing CDB/KD close/resume policy.

The second commit mechanically extracts schemas, prompt/tool metadata, and presentation helpers to server_support.py, keeping the session-ownership code separate and both files under 800 lines. Existing model/helper names remain imported by server.py. A missing-prompt regression check covers the extracted error path. The ownership fix and extraction are separate commits for review.

Why

A successful debugger connection followed by an initial !peb timeout currently leaves an open session registered without ever returning its ID. Retries therefore accumulate debugger processes/connections the client cannot address. Initialization, optional sections, and output-filter errors have the same ownership gap.

This addresses a demonstrated session leak. It does not claim to prove or fully fix a reported Windows nonpaged-pool exhaustion: the exact kernel-side NPFS/CDB allocation mechanism remains unconfirmed. It does not change output-buffer limits, subprocess transport internals, or debugger startup behavior.

No overlap with #124's dump-command cancellation changes in debug_session.py; both fixes can coexist.

How it was verified

Windows, Python 3.12.10, fake debugger sessions through the real MCP tool handler; no live target or resource-exhaustion experiment.

  • The initial 21 regression cases all failed on unpatched develop due to missing shutdown.
  • Final ownership suite: 26 cases covering all four opens, initialization/first/second/optional command failures, repeated failed retries, unrelated-session preservation, successful retry/explicit close, shutdown errors, output-filter failures, filter thread preservation, and cancellation while the worker is initializing.
  • Added one error-path test for missing prompt files after extraction.
  • Full non-live suite: 170 passed, 15 deselected.
  • git diff --check passes; dependencies and lockfile are unchanged.
uv sync --group dev
uv run pytest src/mcp_windbg/tests -m "not live" -q

@svnscha

svnscha commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Merged as #136, which takes your two fix commits with your authorship on them. Thank you - this was the one item in the whole backlog whose defect and fix both held up unchanged under review.

I reproduced the leak first try against the real tool handler, and the ExitStack design is correct on all six paths I checked: constructor failure, init-command failure, mandatory and optional triage failure, an output-filter exception, and cancellation mid-initialization. It can never close a session whose id the caller already received, and the shield is real: with a 3 second shutdown under fail_after(1.0) the request still exited with the session gone.

Three changes on top, all explained in #136:

  • The rollback no longer resumes a live target. As submitted it sent a CTRL+B detach for a cdb remote and three g for kd. Since an open that failed never handed out an id, resuming on its behalf is a side effect the caller cannot opt out of, while close_kd_session does expose resume. CTRL+B detaches and resumes, so a live close with resume disabled now writes nothing.
  • It rolls back on failure only, rather than from a finally, which also ran on the success path and cost a second thread hop plus an uncancellable wait for a limiter token.
  • The tree kill is bounded, since a rolled-back open runs teardown on the event loop's behalf.

I left out the server_support.py extraction. I did check that dropping it was safe rather than assuming: cherry-picking only the two fix commits gives a byte-identical tree to reverting it, 169 of 170 non-live tests pass without it, nothing depends on the move, and validate-server-schema.py never imports the package. The only loss was one 19-line prompt-error test. The "keep both files under 800 lines" rationale did not hold up though - server.py on develop was already 884 lines and there is no linter enforcing a limit - so I would rather keep a bug fix reviewable on its own and take a split as its own change if it earns one.

One note on your ownership suite, which is otherwise good: test_cleanup_failure_does_not_hide_open_failure did not actually test that. str() on an MCPError embeds the chained traceback, so match="triage failed: !peb" passes even when the teardown error has replaced the open error as the primary message. I only found it because I introduced exactly that regression in #139 and the test stayed green. It now asserts on the primary message.

Your three follow-up findings landed too: markers in #137, bounded retention and deterministic teardown in #139.

@svnscha svnscha closed this Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed open tools leak undisclosed debugger sessions after triage or output-filter errors

2 participants