Skip to content

fix(stdio): drain in-flight request handlers before closing write stream on read-EOF - #284

Merged
cdeust merged 10 commits into
mainfrom
fix-stdio-drain-on-eof
Jul 30, 2026
Merged

fix(stdio): drain in-flight request handlers before closing write stream on read-EOF#284
cdeust merged 10 commits into
mainfrom
fix-stdio-drain-on-eof

Conversation

@cdeust

@cdeust cdeust commented Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

  • Root-caused the intermittent "Docker Smoke" CI failure (reproduced on main CI run 30504042295, PR fix(core): break the synaptic-plasticity import cycle (#233) #254, PR fix(sqlite): register explicit datetime adapter, close deprecation warning #266) to an upstream mcp 1.29.0 race: BaseSession._receive_loop closes the write stream unconditionally on stdin-EOF even when a request from the same batch (tools/list after initialize) is still in flight — the response is silently lost (caught ClosedResourceError, logged via logger.debug() on a handler-less logger).
  • Fixed at Cortex's composition root: mcp_server/infrastructure/stdio_transport.py interposes a write-stream proxy that defers the real close until the low-level server's run() has returned (i.e. until every dispatched handler has had its own chance to respond, per anyio task-group join semantics). mcp_server/__main__.py::main() now drives stdio through this wrapper instead of mcp.run(transport="stdio").
  • Regression tests at the SDK boundary pin the drain-before-close contract directly (not just the shell script), including a permanent characterization of the upstream defect. A scoped mutation run found further gaps in the new module and closed them: 42/43 mutants killed, 1 documented equivalent.
  • scripts/docker_smoke.sh gets an independent, second hardening: its timeout/gtimeout wrapper was measured (2026-07-30) to NOT reliably stop a genuinely hung container (SIGTERM to the docker run client doesn't reliably reach the container) — a --cidfile-based watchdog now docker kills the actual container after the same 60s budget.
  • Boy-scout: MIN_TOOL_COUNT had drifted to 49 (citing a test name that no longer exists) against the true baseline of 52 — corrected, along with every doc's test-count claim (6550 → 6562).

Diagnosis → fix, precisely

See mcp_server/infrastructure/stdio_transport.py's module docstring for the full happens-before argument and upstream file:line citations (mcp/shared/session.py ~L351-355, mcp/server/lowlevel/server.py ~L805-815, fastmcp/server/low_level.py ~L234-268).

Test plan

  • tests_py/infrastructure/test_stdio_transport.py — characterizes the upstream race directly (bare mcp._mcp_server.run) and pins the fix (_run_low_level_drained); the fix test verified to FAIL when the guard is bypassed (reproduces the exact reported symptom).
  • tests_py/infrastructure/test_stdio_transport_wiring.py — stateless parameter's actual MCP-lifecycle effect (omitted-default vs explicit), outer run_stdio_drained wrapper wiring (banner, transport context var, log level, log message content via caplog), all mutation-driven.
  • Scoped mutation run (scripts/mutation_check.sh): 42/43 killed, 1 documented equivalent (typing.cast's type argument, never read at runtime).
  • Full suite: 6562 passed locally (pytest, ~175s).
  • ruff check / ruff format --check: clean on all touched files.
  • pyright mcp_server/: 0 errors, 0 warnings, 0 informations.
  • shellcheck scripts/docker_smoke.sh: clean.
  • Real Docker validation (built cortex-smoke:local from the repo Dockerfile):
    • Happy path: 15/15 passing runs on the timeout-available path, 10/10 on the no-timeout-binary fallback path.
    • Negative control (fast-broken container, exits immediately): gate correctly FAILs.
    • Negative control (deliberately hanging container, sleep infinity): gate correctly FAILs in exactly 60s (the watchdog bound), zero leaked containers — verified on both code paths.
  • uv lock --check / scripts/generate_pip_constraints.py --check / scripts/check_doc_claims.py / scripts/generate_repo_badges.py --check: all clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u

Completion Ledger (§13.2) — review-fix + rebase delta (commits 740951f, f1222e6, a788e17)

Path introduced/changed Asserting evidence
run_stdio_drained(show_banner: bool | None = None) resolves fastmcp.settings.show_server_banner when omitted tests_py/infrastructure/test_stdio_transport_wiring.py::test_omitted_show_banner_resolves_from_fastmcp_settings_false — fails against the pre-fix hardcoded-True default (verified locally by reverting the fix and re-running just this test), passes against the fix
Settings resolution doesn't clobber a settings-True default when the setting itself is True (non-regression direction) ...::test_omitted_show_banner_resolves_from_fastmcp_settings_true
An explicit show_banner= argument still overrides the resolved setting ...::test_explicit_show_banner_wins_over_the_setting
Stale test asserting the pre-fix (buggy) behavior as if it were the contract Removed: test_omitted_show_banner_defaults_to_showing_it (replaced by the three tests above)
No new mutation gaps introduced by the banner-resolution change Scoped mutation run, stdio_transport.py mutated against both test_stdio_transport.py + test_stdio_transport_wiring.py: 43/44 killed, 1 documented-equivalent (pre-existing typing.cast type-argument mutant)
Rebase onto origin/main (ef42deb/#285) — conflicts resolved without content loss All 6 conflicts (.bestpractices.json, CLAUDE.md, CONTRIBUTING.md, README.md, assets/badge-tests.svg, docs/ASSURANCE-CASE.md) were identical prose differing only in the advertised test-count number; both sides' prose kept, no section dropped (diffed to confirm)
Test count re-derived from a live collection on the fully-rebased tree (not mid-conflict) pytest --collect-only -q → 6623; local full run: 6618 passed, 5 skipped, 117 subtests = 6623; CI's own Test (Python 3.12) job's "Check advertised test count" step passed on the pushed head
Doc-claim / badge gates clean at the recomputed count scripts/check_doc_claims.py --test-count 6623 → doc claims OK; scripts/generate_repo_badges.py --test-count 6623 && --check → badges OK (5 checked)
No unresolved conflict markers; .bestpractices.json still valid JSON grep sweep for labelled conflict markers (clean); json.load round-trip (clean)
Branch's own touched tests re-run post-rebase, no regression tests_py/infrastructure/test_stdio_transport.py + test_stdio_transport_wiring.py + tests_py/test_main.py: 24 passed
CHANGELOG reflects the observable-behavior fix (§13.1 H4) Appended to the existing [Unreleased] bullet for this same not-yet-released change (commit a788e17)
Lint / format / types clean on all touched files ruff check / ruff format --check: clean; pyright: 0 errors, 0 warnings, 0 informations
CI green per-job on the exact pushed head gh pr checks 284 on a788e172e2a27c7233600c9716ebe900e9cf00c4: all pass (Fuzz (scheduled batch) correctly skipping — schedule-only trigger, not applicable to a PR run); mergeStateStatus=CLEAN

Original PR's own Test plan checklist (Summary above) covers the drain-before-close fix itself and stands unchanged — reviewed and verified line-by-line prior to this delta.

@cdeust
cdeust force-pushed the fix-stdio-drain-on-eof branch from 17c0a55 to 0f3f8e3 Compare July 30, 2026 07:07
@cdeust cdeust mentioned this pull request Jul 30, 2026
Open
3 tasks
@cdeust
cdeust force-pushed the fix-stdio-drain-on-eof branch 2 times, most recently from da6e876 to 92c198b Compare July 30, 2026 08:38
cdeust and others added 5 commits July 30, 2026 11:28
@cdeust @claude
fix(stdio): drain in-flight request handlers before closing write str…
fccfbbb 
…eam on read-EOF

Docker Smoke intermittently reported no tools/list response with no
exception and no JSON-RPC error frame -- reproduced on main (CI run
30504042295, attempt 1 failed / attempt 2 succeeded on the identical
commit), on PR #254, and on PR #266.

Root cause is upstream, in mcp 1.29.0: BaseSession._receive_loop
(mcp/shared/session.py) closes the write stream unconditionally the
instant stdin reaches EOF, even when a request dispatched from an
earlier line in the same batch (tools/list after initialize) is still
running in its own task and has not called respond() yet.
Server._handle_request catches the resulting ClosedResourceError and
logs it via logger.debug() on a logger with zero handlers by default,
so the drop is completely silent. All three JSON-RPC lines are always
fully read (measured parsed_count == 3 on every trial, pass and fail
alike) -- nothing is ever left unread; only the already-computed
response is lost. fastmcp 3.4.5's LowLevelServer.run override removes
the base SDK's own `finally: tg.cancel_scope.cancel()` mitigation with
nothing in its place, so Cortex's stdio entry point inherited the
hazard unmitigated.

Fixed at Cortex's composition root:
mcp_server/infrastructure/stdio_transport.py interposes a write-stream
proxy that no-ops the SDK's premature aclose() and closes the real
stream only after the low-level server's run() call has returned --
which, by anyio task-group join semantics, is only once every
dispatched handler has had its own chance to respond.
mcp_server/__main__.py::main() now drives stdio through this wrapper
instead of mcp.run(transport="stdio") directly.

Regression tests at the SDK boundary
(tests_py/infrastructure/test_stdio_transport.py): one test reproduces
the drop against the bare upstream call directly (a permanent
characterization of the upstream defect), a second drives the
identical race through the fix and asserts the response survives --
verified to fail against the pre-fix code. A scoped mutation run
(scripts/mutation_check.sh) found further gaps, hardened in
test_stdio_transport_wiring.py (the `stateless` parameter's actual
MCP-lifecycle effect, and the outer run_stdio_drained wrapper's
banner/transport-context-var/log-message wiring); final scoped
mutation score: 42/43 killed, 1 documented equivalent.

scripts/docker_smoke.sh gains a second, independent hardening: its
timeout/gtimeout wrapper was itself measured not to reliably stop a
genuinely hung container (SIGTERM to the docker run CLIENT process
does not reliably reach the CONTAINER). A --cidfile-based watchdog now
docker kills the actual container ID after the same 60s budget,
verified against a deliberately hanging test image (fails in exactly
60s, no leaked container) and a deliberately broken/exiting one (fails
immediately) -- both directions proven stable across repeated runs, on
both the timeout-available and no-timeout-binary code paths.

Boy-scout: MIN_TOOL_COUNT's default and source comment had drifted to
49 (citing a test name that no longer exists) against the true current
baseline of 52 -- corrected in the same change, along with the
resulting test-count doc claims (6550 -> 6562 across README,
CONTRIBUTING, CLAUDE.md, the assurance case, .bestpractices.json, and
the tests badge).

Co-Authored-By: Claude <noreply@anthropic.com>
@cdeust
chore(docs): reconcile test-count claims after rebase onto main (6600)
ef384ed 
Rebasing fix-stdio-drain-on-eof onto current main (#283 landed since this
branch was authored, itself bumping the count to 6588) put the doc-count
claims six files carry out of sync with the rebased tree. Recomputed from
a real collection on this tree (not a delta against main's advertised
figure, and not the #287 local/CI-gap arithmetic from an earlier,
abandoned resolution attempt on this same branch, which does not apply
here):

  .venv/bin/python -m pytest --collect-only -q  ->  6600 tests collected

main's 6588 + this branch's 12 new tests (test_stdio_transport.py,
test_stdio_transport_wiring.py) = 6600, confirming the arithmetic.
@cdeust @claude
fix(stdio): resolve show_banner from fastmcp.settings when omitted (§…
740951f 
…13.2 review finding)

run_stdio_drained's show_banner default hardcoded True, diverging from the
call site it replaces: mcp.run(transport="stdio") goes through
TransportMixin.run_async, which resolves show_banner=None to
fastmcp.settings.show_server_banner (fastmcp==3.4.5, transport.py L56-57)
*before* reaching run_stdio_async -- run_stdio_async's own literal `True`
default is never actually exercised on that path. Mirroring only
run_stdio_async's signature dropped that settings resolution, so a user
who disabled the banner via FASTMCP_SHOW_SERVER_BANNER=false got it
printed on every stdio launch regardless.

Fix: show_banner defaults to None and resolves fastmcp.settings
.show_server_banner exactly where run_async does, before the explicit
argument would be checked, so an explicit argument still overrides it.

Tests (tests_py/infrastructure/test_stdio_transport_wiring.py):
- test_omitted_show_banner_resolves_from_fastmcp_settings_true / _false:
  pins the resolution in both directions; the _false variant fails
  against the pre-fix hardcoded-True default (verified locally by
  reverting the fix and re-running just that test).
- test_explicit_show_banner_wins_over_the_setting: an explicit argument
  is not clobbered by the setting.

Replaces the now-stale test_omitted_show_banner_defaults_to_showing_it,
which asserted the pre-fix (buggy) behavior as if it were the contract.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u
@cdeust @claude
chore(docs): correct test-count claims to 6623 (real post-rebase coll…
f1222e6 
…ection)

The prior docs-reconciliation commit's count (6621) was computed while a
rebase conflict resolution was still in progress -- before this branch's
stdio_transport.py banner fix (and its net +2 tests) had been replayed on
top. Recomputed from a clean `pytest --collect-only -q` on the fully
rebased tree: 6623. Both drift gates verified clean at this count:

  python3 scripts/check_doc_claims.py --test-count 6623   -> doc claims OK
  python3 scripts/generate_repo_badges.py --test-count 6623
  python3 scripts/generate_repo_badges.py --check --test-count 6623
    -> badges OK (5 checked)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u
@cdeust @claude
docs: note the show_banner review-fix in the Unreleased CHANGELOG entry
a788e17 
H4 (coding-standards.md §13.1): the banner-default correction is an
observable-behavior fix landing in the same unreleased change as the
stdio drain-on-EOF fix it's part of; appended to that entry rather than
opening a new bullet since it's the same not-yet-released feature.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u
@cdeust
cdeust force-pushed the fix-stdio-drain-on-eof branch from 92c198b to a788e17 Compare July 30, 2026 09:49
cdeust and others added 5 commits July 30, 2026 12:48
@cdeust @claude
refactor: Extract Function _resolve_show_banner from run_stdio_drained
a1d39cd 
run_stdio_drained measured at 54 signature-to-close lines (was 37 before
740951f), breaching both coding-standards.md §4.2's 50-line hard cap and
this repo's own stricter 40-line/method convention (CLAUDE.md:76) -- the
show_banner-resolution fix in 740951f added ~20 lines of docstring/citation
alongside 2 lines of logic. High-stakes module (async coordination /
stream-close-ordering primitive): no 20% flexibility applies.

Extract Function (Fowler 2018 Ch. 6): show_banner resolution + its sourced
citation move into _resolve_show_banner(show_banner: bool | None) -> bool.
The citation is relocated, not deleted (coding-standards.md §8 requires a
source for every claim).

While relocating it: the citation's two upstream line-number references
(fastmcp/server/mixins/transport.py L56-57 / L184-186) were verified
against the actually-installed fastmcp==3.4.5 in .venv and found to be a
consistent -32-line offset from the real run_async (L88-89) / run_stdio_async
(L216-218) locations -- corrected in place rather than carried forward
unchecked.

Before: run_stdio_drained 54 lines.
After: run_stdio_drained 39 lines, _resolve_show_banner 27 lines.
Tests: 732 passed, 5 skipped before and after (tests_py/infrastructure/);
0 modified, 0 added. ruff check/format clean.

Co-Authored-By: Claude <noreply@anthropic.com>
@cdeust @claude
refactor: Extract Function _run_mcp_with_guarded_stream from _run_low…
adbbc66 
…_level_drained

Boy-scout finding (coding-standards.md §14): while measuring run_stdio_drained
for the previous commit, _run_low_level_drained (same file, introduced in this
PR's own fccfbbb, not pre-existing) measured at 57 signature-to-close lines --
over coding-standards.md §4.2's 50-line hard cap and this repo's own stricter
40-line/method convention (CLAUDE.md:76). High-stakes module: no flexibility
applies. Fixed in the same PR as a separate commit per §14.1's "separate
commit when it aids review."

Extract Function (Fowler 2018 Ch. 6): the mcp._mcp_server.run(...) call, its
cast() and the cast()-equivalence citation (already mutation-verified
equivalent, scripts/mutation_check.sh) move into
_run_mcp_with_guarded_stream(...). The citation is relocated, not deleted
(coding-standards.md §8).

Before: _run_low_level_drained 57 lines.
After: _run_low_level_drained 39 lines, _run_mcp_with_guarded_stream 37 lines.
Tests: 732 passed, 5 skipped before and after (tests_py/infrastructure/);
0 modified, 0 added. ruff check/format + pyright (scoped to the touched
file, 0 errors) clean.

Scoped mutation run (scripts/mutation_check.sh pattern, mutmut 3.x) against
the full touched file: 54 mutants, 53 killed, 1 survivor --
x__run_mcp_with_guarded_stream__mutmut_9 (cast(None, guarded) in place of
cast(MemoryObjectSendStream[SessionMessage], guarded)) -- the same
documented-equivalent mutant already named in the relocated citation and in
the Unreleased CHANGELOG entry (typing.cast's type argument is never read at
runtime); not a new gap introduced by this extraction.

Co-Authored-By: Claude <noreply@anthropic.com>
@cdeust @claude
docs: note the run_stdio_drained size-refactor + citation fix in CHAN…
bd356a0 
…GELOG

Documents the two Extract Function refactors (_resolve_show_banner,
_run_mcp_with_guarded_stream) and the fastmcp line-number citation
correction in the same Unreleased entry the original stdio-drain fix and
its show_banner follow-up already live in.

Co-Authored-By: Claude <noreply@anthropic.com>
@cdeust @claude
merge: reconcile with origin/main (test count 6623 -> 6687)
3981edc 
main advanced past this branch's base (768ab09, 40cca6d merged since this
PR opened), moving the six suite-size doc/badge files' claimed count to
6673 and adding two mutation-hardening test files
(test_generate_pip_constraints.py, test_launcher_deps.py) that auto-merged
cleanly.

Conflicts in the six suite-size files (.bestpractices.json, CLAUDE.md,
CONTRIBUTING.md, README.md, assets/badge-tests.svg,
docs/ASSURANCE-CASE.md) were all pure number diffs (this branch's stale
6623 vs main's 6673) -- no differing prose to reconcile. Resolved to
main's number, then recomputed the MEASURED ABSOLUTE post-merge count
(`pytest --collect-only -q`): 6687, not main's 6673 -- this branch's own
test files (test_stdio_transport.py, test_stdio_transport_wiring.py,
_stdio_transport_helpers.py) add tests main doesn't have yet. Updated all
six files to 6687 (not the delta) and regenerated assets/badge-tests.svg
via scripts/generate_repo_badges.py rather than hand-editing the SVG.

Verified: scripts/check_doc_claims.py --test-count 6687 and
scripts/generate_repo_badges.py --check --test-count 6687 both clean;
re-ran this branch's own touched tests post-resolution (216 passed, 117
subtests) plus the two auto-merged test files -- 0 failures, 0 modified
beyond the merge itself.

Co-Authored-By: Claude <noreply@anthropic.com>
@cdeust @claude
merge: reconcile with origin/main (issue #293 monotone-floor doc-clai…
9c86e46 
…ms refactor)

main advanced again since the previous merge, landing #294 ("eliminate the
six-file test-count conflict class with a monotone floor", issue #293):
check_doc_claims.py and generate_repo_badges.py were split into
doc_claim_sources.py/doc_claim_scan.py/doc_claim_structural.py/
repo_badge_catalog.py, and the six suite-size prose files (.bestpractices.json,
CLAUDE.md, CONTRIBUTING.md, README.md, docs/ASSURANCE-CASE.md) now point at
`assets/badge-tests.svg` / `pytest --collect-only -q` instead of a hardcoded
number -- exactly the conflict class this branch's own two prior merges hit.

Conflicts in those five files were the branch's now-obsolete "6687" literal
vs. main's new no-number prose; resolved to main's version in every case (no
content to preserve on our side -- the number is the thing being eliminated).
None of the refactored scripts conflicted (this branch never touched them).

Recomputed the MEASURED ABSOLUTE post-merge count (`pytest --collect-only
-q`): 6706. Regenerated assets/badge-tests.svg via
`scripts/generate_repo_badges.py --test-count 6706` (it hadn't auto-merged
to the new count). Verified: `scripts/check_doc_claims.py --test-count 6706`
and `scripts/generate_repo_badges.py --check --test-count 6706` both clean;
re-ran this branch's own touched tests plus the newly-merged doc-claim/badge
tests -- 235 passed, 116 subtests, 0 failures, 0 test files modified by this
merge.

Co-Authored-By: Claude <noreply@anthropic.com>
@cdeust
cdeust merged commit 2590922 into main Jul 30, 2026
19 checks passed
@cdeust
cdeust deleted the fix-stdio-drain-on-eof branch July 30, 2026 11:51
This was referenced Jul 30, 2026
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cdeust