Skip to content

fix(server): bound retained output and keep diagnostics off stdout - #139

Merged
svnscha merged 2 commits into
developfrom
fix/diagnostics-and-bounds
Oct 8, 2026
Merged

svnscha merged 2 commits into
developfrom
fix/diagnostics-and-bounds

Conversation

@svnscha

@svnscha svnscha commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Harvested from the review of #130 and #131: the parts that fix something somebody can hit, each kept to the smallest change that does it.

What is fixed

--verbose wrote to stdout. On the stdio transport that is the JSON-RPC stream. The MCP SDK diverts fd 1 while it is serving, so in practice this was trailing garbage at exit rather than a split frame, and I want to be accurate about that rather than overstate it. It still has no business there.

--verbose configured no logging handler at all. Every logger.info in server.py and in a --filter-script was being discarded, in this tree and in the proposed one. Four lines of basicConfig(stream=sys.stderr). This is the half of the flag that genuinely did nothing.

Retained output was unbounded. MAX_PARTIAL_OUTPUT_* only ever bounded the diagnostic snapshot, not the output actually stored and returned, so a runaway command grew without limit. Now bounded by line count and per line, with a notice saying how it was cut, and the transport keeps being drained past the bound so the marker still completes the command and the session stays usable.

.logopen /u <path> was unquoted. This one is worse than it reads. Measured on real cdb with a spaced path:

.logopen /u C:\...\probe dir with spaces\unquoted.log
  -> "Opened log file 'C:\...\probe'"
     "    ^ Extra character error in '.logopen /u ...unquoted.log'"
  -> intended log exists = False, and a 954-byte file named 'probe' is left behind

So cdb opens a log at the truncated prefix and the intended file never appears, silently disabling the whole Unicode-log channel that #102 is about. Reachable whenever the resolved temp directory contains a space, such as a user name with a space. Invisible on this host and in CI, because both resolve TEMP to the 8.3 short form.

A failed close reported success. The record is dropped either way, so the id cannot be retried, and a debugger still holding its target is the one thing the caller needs to know about.

Also: dump discovery moves off the event loop, which a glob over a large or remote share can block while break-in has to stay answerable. The order stays sorted() - the consolidated branch quietly replaced it with a bare iglob, which changes the result order for every caller.

Deliberately not taken

#131's error_log.py. It wrote full tracebacks, absolute dump paths and captured partial_output target data to a hard-coded <package dir>/logs/errors.log, i.e. inside site-packages or the uv cache, readable by Users on a machine-wide install. Running the suite produced a 28,939 byte file. Critically it bypasses --filter-script, which the README presents as redacting data before it leaves the machine. Logging to stderr is the correct minimal fix.

Also not taken: list_dumps offset/limit. It defaults to limit=50, so an existing caller asking for a directory of 60 dumps silently gets 50 (measured over real stdio), while the tool description says nothing about pages.

Tests

uv run pytest src/mcp_windbg/tests/ -q   ->  206 passed, 2 skipped

Baseline before this change is 201. The 5 added cover the spaced log path, the line and per-line retention bounds, the honest close, and stable list_dumps order.

The fake now models cdb's real unquoted-path behaviour, so the quoting cannot be dropped again unnoticed. I checked that the new test actually fails without the fix: reverting just the quotes gives FAILED test_the_unicode_log_opens_on_a_path_containing_spaces.

Verbose over stdio was also verified by hand against a real server: first stdout line parses as JSON, 0 non-JSON lines on stdout.

Five small defects, each one somebody can hit.

- `--verbose` printed the debugger traffic to stdout. On the stdio transport
  that is the JSON-RPC stream. The MCP SDK diverts fd 1 while it is serving, so
  in practice this was trailing garbage rather than a split frame, but it has
  no business there either way.
- `--verbose` configured no logging handler at all, so every `logger.info` in
  the server and in a `--filter-script` was discarded. It now logs to stderr,
  which is the half of the flag that actually did nothing.
- The reader kept every line a command produced. `MAX_PARTIAL_OUTPUT_*` only
  ever bounded the diagnostic snapshot, not the output stored and returned, so
  a runaway command could grow without limit. Retention is now bounded by
  lines and per line, with a notice saying so, and the transport keeps being
  drained past the bound so the marker still completes the command.
- `.logopen /u <path>` was unquoted. Given a path containing a space cdb does
  not merely fail: it opens a log at the truncated prefix, reports "Extra
  character error", and never creates the intended file, which silently
  disables the Unicode-log channel that issue #102 is about. Reachable
  whenever the resolved temp directory has a space in it. The test fake now
  models that failure, so the quoting cannot be dropped again unnoticed.
- A close that failed to shut its debugger down still reported success. The
  record is dropped either way, so the id cannot be retried, and a debugger
  left holding its target is the one thing the caller needs to know about.

Also moves dump discovery off the event loop, which a glob over a large or
remote share can block while break-in has to stay answerable. The order stays
sorted: it is part of what callers see.

Ported from the review of the consolidated branch, which also proposed a
rotating error-log file. That is not taken: it wrote tracebacks, absolute dump
paths and captured target output to a hard-coded path inside the installed
package, bypassing the --filter-script the README presents as redacting data
before it leaves the machine.
Making `_close_session` propagate its shutdown error, so an explicit close can
report it, had a second effect I missed: the rollback of a failed open goes
through the same function, so a teardown failure there replaced the error that
explains why the open failed.

  before: MCPError: Error executing tool open_cdb_remote: cleanup failure
  after:  MCPError: Error executing tool open_cdb_remote: triage failed: !peb

The session record is gone either way, so there is no id to retry with and
"cleanup failure" tells the caller nothing they can act on. The teardown
failure is now logged instead, which is possible because --verbose finally
configures a handler.

The existing regression test did not catch this, and neither did the first one
I wrote. `str()` on an MCPError embeds the chained traceback, so both
`pytest.raises(..., match="triage failed")` and a substring check over the
whole string find the original error even when it has been replaced. Both now
assert on the primary message, and the new test was confirmed to fail without
the fix.
@svnscha

svnscha commented Oct 8, 2026

Copy link
Copy Markdown
Owner Author

Pushed a follow-up commit for a regression I introduced in this PR and then found while reviewing it.

Making _close_session propagate its shutdown error, so an explicit close_* can stop reporting success, also affects the rollback of a failed open, which goes through the same function. A teardown failure there replaced the error that explains why the open failed:

before: MCPError: Error executing tool open_cdb_remote: cleanup failure
after:  MCPError: Error executing tool open_cdb_remote: triage failed: !peb

The record is gone either way, so there is no id to retry with and "cleanup failure" tells the caller nothing actionable. It is logged now instead, which only works because --verbose finally configures a handler in this same PR.

Worth recording how close this came to shipping: three tests failed to catch it. The pre-existing test_cleanup_failure_does_not_hide_open_failure uses pytest.raises(..., match="triage failed: !peb"), and the first replacement I wrote checked "triage failed" in str(e). Both pass with the bug present, because str() on an MCPError embeds the chained traceback, so the original error text is in the string even when it has been replaced as the primary message. Asserting on str(e).splitlines()[0] discriminates, and I verified it in both directions:

with the fix:    1 passed
without the fix: AssertionError: the teardown failure replaced the open error:
                 Error executing tool open_cdb_remote: cleanup failure

Both tests now assert on the primary message. 207 passed, 2 skipped.

@svnscha
svnscha merged commit 48875bc into develop Oct 8, 2026
6 checks passed
@svnscha
svnscha deleted the fix/diagnostics-and-bounds branch October 8, 2026 22:11
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.

1 participant