Skip to content

fix(debugger): cancel timed-out commands on dump sessions - #124

Closed
robster7674 wants to merge 2 commits into
svnscha:developfrom
robster7674:fix/cancel-timed-out-dump-commands
Closed

robster7674 wants to merge 2 commits into
svnscha:developfrom
robster7674:fix/cancel-timed-out-dump-commands

Conversation

@robster7674

Copy link
Copy Markdown

What this changes

A command that outruns its timeout is now cancelled with CTRL+BREAK on dump sessions too, not only live targets. Related hardening, all in debug_session.py:

  • Every session starts in its own process group (previously live only), so the break can be aimed at it alone.
  • After a successful resync, a cheap .echo <marker> absorbs a break that may still be pending (the slow command can finish between the timeout and the signal), so it cannot abort the next real command early with truncated output that reads as success.
  • Output a cancelled/abandoned command prints late is dropped instead of leading the next command's reply, on the pipe path (dump sessions) and on the Unicode-log path (multibyte code pages).

Why

With open_kd_dump (1.4.0) people will open large kernel dumps. !process 0 7 over one runs for minutes. Today, on a dump session nothing breaks in, so kd keeps running it and every later command (the retry, !irql, even .echo) queues behind it and times out too: the session is wedged until closed. We hit this on real complete dumps; verified on kd that !process 0 7 stops within about a second after CTRL+BREAK and the next command runs normally.

How it was verified

Four new hermetic tests in test_debug_session.py (cancel-and-continue on a dump, the no-op ahead of the next command, late output of an abandoned command, and the Unicode-log case), plus the existing test_debug_session.py / test_unicode_log.py suites. Run on Linux with a stub winreg; the one test_kd_session.py failure there is the existing hard-coded C:\fake\dumps assertion, which is Windows-only. The live kd check above was run by hand on a Windows worker.

Happy to split this (cancel-on-dump vs. the stray-break no-op vs. late-output handling) or adjust style if you would rather review it in pieces. We carry this as a patch in our vendored copy and would like to converge on upstream.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@svnscha
svnscha changed the base branch from main to develop October 1, 2026 16:55

@svnscha svnscha left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. I retargeted this to develop, which is the working branch; main tracks the last release.

One regression needs correcting before merging: in src/mcp_windbg/debug_session.py:374, the new dump-only abandoned-marker branch replaces buffer without updating self._reader_buffer. After a late marker is discarded, subsequent output is appended to the new list, but _snapshot_partial_output() still reads the old list. A later timeout can therefore return empty or stale partial output instead of the current command's diagnostics.

Please add self._reader_buffer = buffer immediately after buffer = [], matching the expected-marker branch above, and cover partial-output capture after an abandoned marker. I reproduced this with the existing fake debugger by delivering a stale marker, then making !clrstack print output while swallowing its completion marker: partial_output is [] instead of ["OUT:!clrstack"]. The same test passes on develop and with that one-line correction applied in memory.

Validation otherwise looks good: all four new regressions fail against develop and pass with this PR; the full Windows suite passes with 160 passed, 2 skipped and 93% coverage. The skips are the two live-kernel scenarios because no kernel target is configured. I also checked follow-up commands after timeouts on both committed dump fixtures with real CDB and KD; the PR keeps abandoned output out of those replies.

…arker

The dump-only abandoned-marker branch replaced the reader's buffer without
updating self._reader_buffer, so _snapshot_partial_output() kept reading the
old list. A command that timed out after a late marker was discarded reported
empty or stale partial output instead of its own.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@robster7674

robster7674 commented Oct 2, 2026 •

Copy link
Copy Markdown
Author

Thanks for the careful review and the repro. Fixed in 14c281d:

  • debug_session.py: self._reader_buffer = buffer right after buffer = [] in the dump-only abandoned-marker branch, matching the expected-marker branch above it.
  • New test test_timeout_after_an_abandoned_marker_still_reports_partial_output, following your repro: a stale marker is delivered, then !clrstack prints output while its completion marker is swallowed.

Edit: replaced my earlier Linux-only numbers with a Windows run. I ran the steps of the test job from build-and-test.yml at 14c281d on a fresh Windows Server 2025 VM (build 26100), with real cdb.exe/kd.exe 10.0.28000.2705. One difference from CI: the debuggers came from the Windows SDK installer (Debugging Tools feature), not the Store WinDbg package.

Python pytest coverage (--fail-under=88)
3.10.22 161 passed, 2 skipped 92%
3.11.17 161 passed, 2 skipped 92%
3.12.15 161 passed, 2 skipped 92%
3.13.16 161 passed, 2 skipped 92%
3.14.8 161 passed, 2 skipped 92%
  • The 2 skips are Resume and wait against a real kernel target and Kernel session against a real target; no kernel target was configured.
  • uv lock --check, python -m mcp_windbg --help and scripts/validate-server-schema.py all exited 0.
  • The new test, on the same VM (Python 3.14.8): with debug_session.py from 336de15 it fails with assert [] == ['OUT:!clrstack']; with the file from 14c281d it passes.

Correction to what I wrote here before: I had said two tests failed on Linux because of running off-Windows, without having checked. Both pass on Windows in all five runs above, and the Linux causes are:

  • test_dump_launches_kd_with_dash_z_and_dump_dir_symbols: os.path.dirname(os.path.abspath(r"C:\fake\dumps\...")) resolves to the working directory under posixpath, so the -y argument is not C:\fake\dumps.
  • Open dump without a path: server.py imports winreg, which does not exist on Linux. I had stubbed it with an empty module to get the package to import, so get_local_dumps_path hit a missing winreg.OpenKey (and WindowsError is undefined there).

CI on this PR is still waiting for workflow approval.

@svnscha

svnscha commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Landed as #138. Thank you - the diagnosis was right, the mechanism works, and you were right to push on it.

I verified the premise rather than taking it on trust, because "CTRL+BREAK cancels a command the engine is running on a static dump" is the kind of claim a fake cannot settle. On a real cdb against a committed dump:

[  8.00s] sending CTRL_BREAK_EVENT to the process group
[  8.00s] "0:000>   ^ User interrupted operation error in '.for (r $t0 = 0; ...)'"
[  8.00s] '0:000> COMMAND_COMPLETED_MARKER_1'
[  8.50s] '0:000> COMMAND_COMPLETED_MARKER_2'   <- next command works

And the wedge you describe, before and after, with a 3 second timeout:

before: the command times out, then .lastevent ALSO times out after 20s
after:  the command is cancelled in 3.00s, .lastevent returns in 0.00s

CREATE_NEW_PROCESS_GROUP on all four session types checked out too: no orphan cdb.exe after a hard kill, and the tree kill is unaffected. The one visible change is that Ctrl+C in the server's own terminal no longer reaches dump children.

Two changes from your version.

The stray-break absorb is not included. Its premise is that a break reaching an idle debugger stays pending and aborts the next real command. Measured, such a break is dropped. The only command at risk is one starting within about 10ms of the signal (at a 0ms gap 7 of 15 were truncated; at 10ms or more, 0 of 15), and with the absorb in place 3 of 12 were still truncated, so it did not close the window it aimed at. In practice the next command arrives after a model round trip. That is about 25 source and 16 test lines left out, and it is the only part of your patch I would ask you to drop if you carry this downstream.

Lost sync is handled per session kind. Rather than your "may need a manual break-in" hint, which a dump user cannot act on because send_ctrl_break rejects a dump session, a live target now keeps its session and that hint, while a dump is closed. A consolidated branch proposed closing on every dump timeout; that is too blunt, since it would cost a reopen and every resolved symbol even when the cancel would have worked in under a second. Cancel first, close only if the cancel failed to resync.

Also corrected: the comments referenced issues #449 and #468, which do not exist in this repository.

One thing I added, because your own note said the hermetic tests mostly encode the fake: test_dump_cancel_live.py, a real-engine check that a busy loop on a dump session is cancelled and the next command returns in under 5 seconds. It carries the live marker and passed in CI, so this behaviour is now gated by the project's own matrix rather than by hand. Related: CI now runs on contributor pull requests without manual approval, which is why none of this backlog had ever been tested by it.

Still outstanding and noted in my own notes: !process 0 7 against a genuinely large kernel dump. The committed minidump is too small to make a command run for minutes, so if that offer of large bitmap dumps still stands I would like to take you up on it.

@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.

3 participants