feat: stdin handoff + true cancel abort on the modern era (#270 Phase 2 PR D) - #362
Merged
Merged
Conversation
…Phase 2 PR D) First step of PR D (§3.9 commit 1), inert until wired: `_post_and_stream` grows an optional `abort` event and every terminal exit consults it before anything else. On the modern era a `notifications/cancelled` has no wire representation at all — "Closing the SSE response stream is the cancellation signal. The server MUST treat a client disconnect as cancellation of that request. No notifications/cancelled message is required or expected." So the abort arrives as a cross-thread `resp.close()`, which surfaces here as an `httpx.HTTPError` with `emitted=False` — and today that sends the attempt loop back around to re-POST a cancelled `tools/call` up to MAX_RETRIES times (required change 2, the recorded trap). - `_StreamResult.aborted` marks the outcome; `_aborted_result()` builds it with `status_code` 0 so every status comparison a consumer might reach before the flag falls on the silent side. - The `httpx.HTTPError` arm consults the event first (stop-wins, the `_consume_restart` template) and returns the flagged result without writing: not the generic "stream interrupted" error, not the MRTR `input_required_abort` funnel (required changes 4 and 10). - A new `RuntimeError` arm catches `httpx.StreamClosed` — a `StreamError`, which subclasses RuntimeError and is a SIBLING of HTTPError — raised when the close lands mid-read. It would otherwise escape to run()'s never-crash net and write "internal relay error" under the cancelled id. Abort not set → re-raise loudly, as the listen loop's arm does. - The NORMAL 200 return is gated too: a close landing between chunks after stream EOF ends `iter_text()` without raising, which would otherwise leak a false-normal 200 into the recovery ladder and `_mrtr_run_retry`. - The event is consulted before each attempt and before the inter-attempt sleep, so a deliberate abort never re-issues the request (required changes 6 and 8). No caller passes `abort` yet, so behaviour is unchanged on both eras. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 2 PR D) Second step of PR D (§3.9 commit 2), still inert: nothing sets the abort event until the reader thread lands. `_InFlightPost` is the one cell a stdin cancel can act on. It follows PR B's `_ResourceSubscriptions` base change 5 exactly — the RESPONSE is closed, never the client, which run()'s teardown owns and whose close would make every later request raise. `_post_and_stream` takes the same `publish_response` parameter `_listen_stream_loop` already takes, hands over each attempt's live handle the moment the stream opens, and retracts it from a `finally`: a finished attempt's handle must never stay publishable, or a late cancel for request N would end request N+1's stream. Publication is owned by the CONSUMER (the stdin loop) and keyed on the id the local client is waiting on — the only id a cancel can name: - an ordinary modern request publishes AFTER every id-intake guard, so a cancel can never match an id the loop is about to reject, and stays published across the whole dispatch window (initial POST, the 401/403/404 recovery ladder, the trailing `_mrtr_run_retry`); - an MRTR retry chain publishes under the CLIENT's id `N`, not the minted id of the line that triggered it, and for the whole `_mrtr_run_retry` loop rather than per POST — a `requestState`-only chain re-POSTs with no client involvement, so a per-POST cell would be deaf in the gaps while the queued cancel line sits behind that frame. Consumers short-circuit on the flagged terminal result before anything else: run()'s loop at all four dispatch sites (top level plus the three recovery branches), and `_mrtr_run_retry` after the 401 arm, where it returns WITHOUT purging or answering — both belong to the queued cancel line's `_mrtr_handle_cancel`, and `_mrtr_abort` would write an error under N, exactly wrong for a cancel the client itself sent. `in_flight` is None on the legacy era, so every `abort`/`publish_response` argument stays None there and `_post_and_stream` is byte-identical (acceptance criterion #3). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Phase 2 PR D) Third step of PR D (§3.9 commit 3): the handoff itself. A daemon reader thread runs `for line in sys.stdin:` and feeds an unbounded FIFO queue that run()'s loop drains; the loop body, every id-intake guard, the MRTR machinery and the recovery ladder are untouched and still run one line at a time on the consumer thread. Why a thread at all: on the modern era a cancel is only actionable while a dispatch BLOCKS, and a blocking read on a daemon thread is the only portable way to be reading then. `select()` does not work on pipes on Windows and a blocked `ReadFile` cannot be cancelled from Python, so any second thread that touches stdin must own it forever — which is why the handoff is permanent rather than scoped to a dispatch. It iterates `sys.stdin` itself (the object `_enforce_lf_stdio` reconfigures on win32) and never mixes in `os.read(0)`/`sys.stdin.buffer`, which would strand Python-level buffered data. The reader mutates NO relay state (required change 7): per line it enqueues, unchanged and in order. Because exactly one thread ever reads stdin, nothing is reordered, no double-dispatch is possible and a partial-line split cannot occur. The queue is unbounded by construction — `SimpleQueue` has no `maxsize` at all (§3.7) — because a cap that blocked the reader would reinstate the deafness this PR removes. EOF enqueues a sentinel that the drain stops on; FIFO ordering makes the "drain fully before honoring EOF" rule automatic (required change 9), since every queued request is already ahead of the sentinel. AC3 (§3.2): the reader is spawned only when `era == "modern"`, and `stdin_source` IS `sys.stdin` on legacy, so the loop stays the literal synchronous one — same object, same iteration protocol, zero threads and zero new per-line calls. This is not cosmetic: an eager legacy-side `tracker.add` would make a cancel that today lands after a completed response start dropping that response mid-stream, a real observable change. `test_legacy_spawns_no_reader` pins both halves by thread NAME (sampled from inside a dispatch, the only window the reader is alive) rather than by a count the refresh timer and listen threads destabilise. All 1616 existing tests stay green with every modern-era test now running through the queue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…R D) Fourth step of PR D (§3.9 commit 4): the reader thread now acts on a `notifications/cancelled` it reads. Cancellation works on the modern era. Per line the reader runs the cheap pure-function `_extract_cancel_id` check and, on a match against the published in-flight request, sets the abort event and THEN closes the published response handle — that ordering is what lets the consumer, waking inside `_post_and_stream`'s exception arms, tell a deliberate cancellation from a transport fault. The line is still enqueued afterwards, unchanged and in order, so `tracker.add` and `_mrtr_handle_cancel` run on the consumer exactly as before. Closing the stream IS the cancellation here: "Closing the SSE response stream MUST be treated by the server as cancellation of that request. … The server SHOULD stop work on the cancelled request as soon as practical and MUST NOT send any further messages for it." Nothing is POSTed upstream — a `notifications/cancelled` is neither required nor expected on this transport and a v2 server may legally reject it — including mid-MRTR-retry, where the retry POST is disconnected instead. Twelve of the design's named tests, nine of them here: - test_modern_cancel_aborts_inflight_post — ONE POST, not MAX_RETRIES - test_aborted_id_writes_nothing — no error, no empty-response synthesis - test_next_request_dispatches_after_abort — the handoff itself - test_cancel_races_completion — a delivered response is never errored - test_stale_handle_never_closed — cell-level and end to end - test_swallowed_then_abort_no_input_required_abort — the funnel gate - test_cancel_during_mrtr_retry — retry disconnected, exactly one set of downstream minted cancels, nothing under N, nothing POSTed upstream - test_abort_checked_before_retry_attempt_and_sleep — both guards - test_stream_closed_not_internal_error — and it still re-raises unset Disabling `_abort_requested` fails eight of the ten in this class, so the revert-checks bite. `pytest-timeout` joins the dev extras: run() installs signal handlers, so these must drive it from the main thread and a regression that reinstates the deafness would otherwise hang CI silently. test_rapid_subscribes_coalesce_into_two_opens now waits for the last subscribe's synthesized reply rather than for the generator to have yielded it. On the modern era that generator runs on the reader thread, where a yield means ENQUEUED, not processed — the relay's coalescing guarantee is unchanged, the test's proxy for "all five landed" was not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…PR D) Fifth step of PR D (§3.9 commit 5). `test_eof_sentinel_drains_queue` pins required change 9 in both halves: the reader/drain pair in isolation, and end to end with the consumer provably BEHIND — parked mid-stream on request 2 while 3 and 4 sit queued and stdin has already ended. FIFO order is what makes the drain complete; honoring EOF the moment it is seen would silently lose work a client legitimately pipelined ahead of a slow call. `test_teardown_with_parked_reader` pins §3.8. A read parked in the C-level `readline()` is not portably interruptible — the same constraint that forced a blocking reader — so there is no dedicated client to close out from under it the way the listen threads get. The contract is `daemon=True` plus a tiny bounded join, and an unbounded one would hang every shutdown that happens while a client is idle. The test drives the real path: SystemExit is exactly what run()'s SIGTERM/SIGINT handler raises, and being a BaseException it walks past the loop's never-crash net straight into the `finally`. Replacing the bound with a plain join() fails it on elapsed time. EOF deliberately does NOT abort the in-flight POST, contrary to the design's proposed default for owner question 1, and the reader's docstring records why: "the client crashed after sending A" and `echo A | mcp-stdio` are byte-identical at this layer, both leave the consumer mid-dispatch at EOF, and cutting the second off means a piped request never gets its answer. It is unnecessary besides — a dead client makes `_write_line` raise BrokenPipeError, which run()'s never-crash net already absorbs before the consumer drains to the sentinel and exits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sixth and last step of PR D (§3.9 commit 6). Four comments literally predicted this PR and are now wrong; a fifth needed a word changed. - `_ModernState` no longer says aborting an in-flight POST "would require a second thread reading stdin … deliberately deferred". It now says what is actually true and why the handoff needed no re-audit: what moved to a second thread is READING stdin, not dispatching from it, so `protocol_version`, the recovery ladder, the MRTR dicts and that object each still have exactly one mutator. - run()'s docstring no longer lists cancelling an in-flight MRTR retry as "not yet bridged" — it is, and the note now points at where. - `_mrtr_handle_cancel` no longer claims "there is nothing in flight there to cancel"; there can be, and the reader has already closed it before this function sees the line, which is exactly what leaves the purge and the downstream cancels to run here once, in order. - The `mrtr_txns` and `headers_lock` notes now say "dispatches one line at a time" / "request handling is single-threaded" rather than "single-threaded", and name the reader as an enqueue-only thread. - `_handle_modern_special_method`'s reply-then-degrade rationale keeps its argument with the same correction. cli.py carries no "single-threaded stdin" claim, and README/docs describe only the cancel FILTER, which is unchanged on both eras. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Phase 2 PR D) Self-review of PR D's own test list found two arms with no coverage. `test_stream_closed_not_internal_error` set the abort event BEFORE calling `_post_and_stream`, so the loop-top pre-attempt guard returned first and no POST was ever issued — required change 3's abort branch was never entered and only its `raise` half was exercised. The event is now set from inside the stream, right before the `StreamClosed`, mirroring what the reader thread does (set, then close the handle), and the test asserts a POST actually happened. `test_abort_on_a_stream_that_ends_without_raising` is new and covers the false-normal terminal: a close landing between chunks after the underlying stream reached EOF ends `iter_text()` without raising, so NEITHER exception arm sees it. Without the gate on the normal 200 return that writes "empty response from server" under the cancelled id and hands the caller a healthy-looking 200 the recovery ladder and `_mrtr_run_retry` act on. Both revert-checks were confirmed to fail. Also: the pagination gap (owner question 2) is now recorded where a reviewer meets it — at the `_paginate_and_stream` branch in `_dispatch`, which visibly takes no `abort`/`publish_response` while its sibling call below does — and the teardown test's wall-clock bound goes from 5 s to 10 s for the Windows runner, still well under the 20 s an unbounded join parks for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
AI review (gpt-5.6-sol)Verdict: no blocking findings — 1 advisory. Advisory findings do not need to be resolved before merge; open an issue for the ones worth keeping. Scope: new commits since R6F1 — .github/workflows/ai-review.yml:686 - Malformed severity values can incorrectly request changes Findings ledger (all rounds)
Advisory per-push review (round 6) generated by ai-review.yml — verify findings before acting. |
… PR D) Round-1 AI review raised three cases where a cancel does not abort. All three are real, none is a regression — before this PR the modern era honored no cancel at all — and all three are properties of the design as specified rather than implementation errors. `_InFlightPost` now says so where a reader meets the cell: 1. BEFORE PUBLICATION (R1F1). The reader can enqueue a request and read its cancel before the consumer picks the request up, so the cancel matches nothing and is not re-tried on dequeue. Re-arming from a "seen before publication" set is not obviously correct: JSON-RPC lets a client reuse an id once the prior call is done — what the tracker's discard-on-reuse encodes — so a naive set would abort an unrelated later request under the same id. 2. BEFORE RESPONSE HEADERS (R1F2). `client.stream()` does not return until the server sends headers, so a server that does its long work first leaves `_response` None throughout: the event is set, nothing is closed. Closing that window costs a dedicated `httpx.Client` per abortable request — the listen threads' pattern — and is deliberately not paid here. 3. PAGINATION (R1F3) — already recorded at the `_paginate_and_stream` branch as owner question 2's known gap; cross-referenced now. In every case the cancel is still enqueued and still reaches the tracker and the MRTR bridge on the consumer; it simply cannot cut the request short. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Round-1 AI review triage — all three findings confirmed real, all three now documented in R1F1 — cancel read before publication. Confirmed. The reader can enqueue a request and read its cancel before the consumer picks the request up; the cancel matches nothing and is not re-tried on dequeue. Not fixed here, because the obvious fix is not obviously correct: JSON-RPC lets a client reuse an id once the prior call is done — exactly what the cancel tracker's R1F2 — POST waiting for response headers. Confirmed, and the sharpest of the three: R1F3 — paginated modern requests. Confirmed, and already the recorded resolution of owner question 2: |
… Phase 2 PR D) Design Amendment D2 (approved on #270) is now pinned by an end-to-end half, not only by a unit-level simulation, per the amendment's own requirement. `_AbortableStream` grows `close_ends_quietly`: instead of standing in for the socket teardown with an `httpx.ReadError`, the parked iterator simply RETURNS when the relay closes the handle — the case D2 exists for, where the close lands between chunks with the underlying stream already at EOF and `iter_text()` ends with no exception anywhere. `test_abort_on_a_stream_that_ends_without_raising` now runs that first: the reader THREAD closes the published handle while the consumer is provably parked mid-stream, the stream ends quietly, and the assertions are the wire-level ones — nothing under the cancelled id, no "empty response from server", exactly one POST. The unit half follows for a sharper message when the gate itself regresses. Revert-check confirmed on the cross-thread half: removing the pre-return check makes it fail with the exact harm D2 prevents — `{"error": {"code": -32000, "message": "empty response from server"}, "id": 2}` written under the id the client just cancelled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two halves were reordered so the cross-thread end-to-end case runs FIRST (it is the load-bearing evidence, and it fails with the exact harm the gate prevents), but the docstring still described the unit half first. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Code reviewFound 2 issues:
mcp-stdio/src/mcp_stdio/relay.py Lines 2974 to 2997 in 4b36c10
mcp-stdio/src/mcp_stdio/relay.py Lines 2586 to 2603 in 4b36c10 Sub-threshold advisories (verified real, below the reporting bar; recorded for the #270 completion record): the rate-limit sleep lacks the pre-sleep abort check its sibling retry sleep has (responsiveness only, self-heals at loop top); 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…n reader a per-line net (#362 code review) Finding 1: the two non-200 terminal returns of _post_and_stream never consulted the abort event, although the docstring promises "every terminal exit consults the event FIRST" (Design Amendment D2). Deterministic with the documented pre-headers window: a cancel sets the event while client.stream() awaits headers, the server then answers non-200, and the result came back aborted=False — run() answered (or, via the recovery ladder, re-POSTed) the id the client had withdrawn, and _mrtr_run_retry's aborted-guard was skipped, purging the transaction and stranding its outstanding minted dialogs. Both returns now classify the abort after draining the body; the rate-limit branch checks once for both of its exits, which also gives its Retry-After sleep the same "the client stops waiting NOW" rule as the RETRY_DELAY sleep (sub-threshold advisory folded in, loop-top comment updated). Finding 2: _stdin_reader_loop wrapped the whole stdin loop in one try/except while claiming to mirror run()'s per-line never-crash net. One raising iteration — e.g. RecursionError through _extract_cancel_id's narrow catch on a deeply-nested cancel params — ended the reader permanently, and the finally's EOF sentinel made a live session read as a clean client disconnect (executed repro: two legitimate requests silently vanished). The cancel-detect pair now recovers per line (line still enqueued, loud log once per reader lifetime, input_required_logged precedent); the outer net remains for stdin transport failure only. Six tests, each revert-checked: the two pre-headers classification units, the no-sleep-on-cancel unit, the end-to-end cancel-then-500-writes-nothing, and the three reader resilience tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YVg3toYKK6J2J9ggdrzYVf
Owner
Author
|
Code-review findings addressed in 994018c:
All six tests revert-checked (fix removed → named test fails → restored). Suite: 1629 → 1635 passed, 3 skipped. Remaining advisories ( 🤖 Generated with Claude Code |
This was referenced Jul 31, 2026
Closed
shigechika
added a commit
that referenced
this pull request
Jul 31, 2026
… reference peer (#367) (#368) * fix: clear the #270 Phase 2 recorded debt ahead of the v2 harness (#367) Four housekeeping edits carried over from the Phase 2 completion records, landed first so a harness failure can never be confounded with a known advisory. (a) CLAUDE.md truth-up. `run()`'s bullet gains the modern-era stdin reader thread (PR D), and the signal-handler bullet now covers both transports: `run()` runs daemon threads too, so pytest must drive it from the main thread just like `run_sse`. (b) `_mrtr_handle_cancel` docstring: the close-before-enqueue claim was overstated — it is false in the pre-headers window, where there is no published response to close yet. The OUTCOME still holds via the abort event and the D2 abort checks before every terminal return, so the wording now names the mechanism instead of one of its paths. (c) `_listen_stream_loop` reopen arm: `_consume_restart(...) or reopen`, not `reopen or _consume_restart(...)`. `or` keeps both operands, so the reopen decision is unchanged; what changes is that a pending restart is now consumed whenever an attempt ends with reopen=True. Left set, it would let a later unrelated `RuntimeError` (stop unset) read as a subscription change and defer #352 R5F1's deliberate re-raise by an iteration. Coalescing and stop-wins are both intact. (d) Cap-refusal once-latch. The `_LISTEN_MAX_SUBSCRIPTIONS` comment has always promised "logged once"; the log had no latch. One boolean on the subscriptions holder, global rather than per-URI so distinct over-cap URIs cannot spam stderr. (c) and (d) are behavior changes and each gets a revert-checked test. Both live in modern-era-only code (listen threads; the interception behind the modern dispatch), so the legacy wire stays byte-identical. Refs #367, #270 * test: scaffold the tests_integration harness (extra, conftest, stdio client) (#367) The `integration` optional-dependency group brings in the reference peer (python-sdk v2) and uvicorn. `[project].dependencies` is untouched — the runtime dependency set stays httpx-only and nothing under src/ imports either package. `tests_integration/` is a SIBLING of tests/, not a subdirectory: the unit gate runs `pytest tests/` by explicit path, so a sibling needs no pytest ini and no marker-based exclusion. A `find_spec("mcp")` guard skips collection when the extra is not installed, so a `[dev]`-only checkout can still run a bare `pytest`. conftest carries the determinism rules verbatim in its module docstring and the machinery behind them: `wait_until` (the one permitted sleep), pre-bound ephemeral ports, an in-process uvicorn thread with a bounded readiness poll plus TCP fallback, a bounded stderr drain, and yield fixtures ordered so teardown is relay-first / server-second. Timeouts are injected per item and FILTERED to this directory so a bare root `pytest` cannot stamp them onto the unit suite. The stdio client speaks 2025-06-18 deliberately — it is the legacy era the relay translates FROM, so reusing an SDK client would test the SDK against itself. It declares `elicitation: {}` (the 2025-era shape), which the reference peer was verified at implementation time to accept as form-mode support. Refs #367 * test: add the python-sdk v2 reference-peer app to the harness (#367) `_reference_server.build_app()` is a FACTORY (scenarios 5 and 6 need their own server) returning the ASGI app plus the side-channel handles that make in-process hosting worth it: the subscription bus, the listen handler, the cancellation event and the captured event loop. Registered surface, one entry per scenario: a plain `add` tool, a `Resolve`/`Elicit` resolver tool for MRTR, an async event-gated tool for cancellation, and one subscribable resource. The MRTR tool uses the resolver DAG rather than `ctx.elicit()`: on a modern (2026-07-28) connection the SDK forbids server-to-client requests outright, and the resolver is what the framework compiles into an `InputRequiredResult` for that era. The resolver is defined at MODULE level because the SDK evaluates tool annotations against the function's globals — a `Resolve(<closure local>)` raises `InvalidSignature`. Two things verified against the installed 2.0.0 and worth stating: - `ListenHandler.close()` IS a documented graceful-closure hook (each stream flushes and sends its `SubscriptionsListenResult` as the final frame), so scenario 5 can assert the strong form. Reaching it needs one private attribute — MCPServer builds its own handler and does not hand it back — so the harness registers its own through the documented `add_request_handler`. - The parked tool must EMIT before parking. The v2 streamable-HTTP path defers `http.response.start` until the handler's first notification or a 15 s ping, so a silently-parked tool leaves the relay with no response object to close for 15 seconds — PR D's pre-headers window, which scenario 6 is explicitly not testing. Refs #367 * test: six e2e scenarios against the v2 reference peer (#367) Each pins one #270 Phase 2 subsystem that until now was verified only against this project's own mocks: 1. `test_era_probe_classifies_the_v2_server_as_modern` — Phase 1 era detection. Asserts BOTH the synthesized 2025-06-18 InitializeResult and the relay's `protocol era: modern (auto-detected)` marker, because a v2 server answers a legacy `initialize` too and the handshake alone does not discriminate. 2. `test_initialize_tools_list_and_call` — AC3 header layer (`Mcp-Method`/`Mcp-Name`), `_meta` injection, `resultType` handling. 3. `test_mrtr_full_loop_under_the_original_id` — PR C (#356). The designated watchpoint, and it holds: the peer answers with `resultType: "input_required"` + an `inputRequests` map keyed `module:qualname` + an opaque `requestState`, the relay mints a plain 2025-era `elicitation/create` (with `mode: "form"` stripped), and the final result arrives under the client's ORIGINAL id. 4. `test_subscribe_publish_and_forward` — PR B (#358) / Design A9, the highest-risk point. The ack echo really does nest `resourceSubscriptions` under `params.notifications`, so the relay's honored-subset read matches the reference on the side A9 never exercised. 5. `test_listen_graceful_end_does_not_reconnect` — PR A (#352), strong form via the SDK's `ListenHandler.close()`, plus a bounded quiescence check for the absence of a reconnect. 6. `test_cancel_aborts_the_in_flight_post` — PR D (#362). The cancel is gated on the forwarded progress notification, the only downstream-observable proof the POST is past the pre-headers window; the three documented not-covered windows stay out of scope. Every wait is bounded, every negative assertion is a bounded drain with a cited justification, and no test asserts on elapsed time. Refs #367, #270 * ci: advisory integration workflow against the v2 reference peer (#367) ubuntu-only, `pull_request` + `workflow_dispatch`, `contents: read` (private repository — checkout fails without it), `timeout-minutes: 10` as the outermost of the three safety valves. Advisory BY CONSTRUCTION: main's required checks are dual-defined (branch protection + the `ci-gate-main` ruleset) and list only the six unit-matrix contexts, so a new workflow is advisory simply by not being added to either. That is deliberate until the suite proves stable, and promoting it later means editing both definitions. One Python version rather than a matrix — the subject is interop with the reference peer, and 3.10-3.14 are already covered by the unit job. The version smoke reads distribution metadata: `mcp.__version__` does not exist in v2.0.0. Refs #367 * docs: document how to run the integration harness (#367) CLAUDE.md's Build & Test block listed only `pytest tests/`, so a new session meeting `tests_integration/` would have no way to know it needs the `integration` extra or that the unit gate deliberately never walks it. Refs #367 * test: make the scenario 4/5 assertions sound (#367) Two defects in the scenarios as first written, both found by re-reading `ListenHandler` rather than by a failing run. Scenario 5's quiescence check was unsound. Two legitimate sources of an in-flight `resources/updated` survive the establishment probe: the probe publishes once per poll, so the iteration that succeeded can leave an earlier duplicate still travelling; and `ListenHandler.close()` explicitly "drains its buffered events" before sending the final frame, so a buffered publish is forwarded BY the graceful end itself. Either one would fail `assert late == []` while saying nothing about a reconnect, which is the only thing that check is about. Draining once after the graceful end is observed fixes it. Scenario 4 asserted over a later drain, which can legitimately come back empty — making the uri and subscription-id-leak checks silently vacuous. It now asserts on the notification the wait actually matched. Refs #367 * docs: cite the re-raise arm by name, not by line number (#367) Follow-up to this PR's housekeeping (c). The new comment at the reopen arm pointed at "the 5188 arm", which no repository comment does — line numbers here go stale on the next edit, and that one was already wrong after the comment itself was inserted. Names the `except RuntimeError` arm instead. Refs #367 * test: drop unused harness handles from the reference peer (#367) `Harness.url` and `Harness.release_waiter()` had no callers — the fixtures pass `port` and the parked tool is torn down by server shutdown, not by being released. Dead scaffolding in a brand-new harness is exactly the kind of thing that gets copied forward. `release` itself stays and is now documented as deliberately never set: it is what makes the park an awaitable checkpoint a cancel scope can interrupt, rather than a busy loop. Refs #367 * test: drop the last unused harness members (#367) Same standard as the previous commit, applied to what it missed: the `poll_tick` fixture had no consumers (`wait_until` owns the tick), and `Harness.app` / `Harness.server` were never read — the fixtures already hold the uvicorn server and thread they need for teardown, so the dataclass only ever needed the port and the side-channels. `StdioClient.expect_error` deliberately STAYS despite having no caller today: it is part of the client API the #367 design specified (§3.6), and a JSON-RPC client that can await a result but not an error is a trap for the next scenario. Refs #367 * test: pin the modern-envelope pass-through instead of claiming a strip (#367) Scenario 2's docstring asserted the relay "strips `resultType` and the server's `_meta` down to something a 2025-era client understands". Observed against the reference peer, that is false: a `complete` tools/call result reaches the stdio client verbatim — `resultType`, `structuredContent` and `_meta` included. The relay reads `resultType` only to spot `input_required` and route the MRTR branch. That behaviour is sound (MCP results are open, so an unknown field is ignored by a 2025-era client) but it was undocumented and untested. The docstring now says what happens, and the test pins it — a future "normalize the envelope" change would otherwise alter what reaches every downstream client with nothing going red. Refs #367 * test: dump relay stderr when a server-side wait times out (#367) The first CI run of this suite failed scenario 6 on ubuntu-latest (the other five passed) with nothing but "timed out after 10.0s waiting for the reference peer's tool to observe anyio cancellation" — which does not say whether the relay aborted the POST at all. That is the whole question: the abort line present means the relay did its job and the disconnect did not become a cancellation server-side; absent means the relay never acted. `StdioClient.expect_*` already dumps the stderr tail on failure; waits on a SERVER-side condition had no equivalent. `wait_until` now takes an optional `diagnose` callable, invoked only on timeout, and scenarios 5 and 6 pass the relay's stderr tail (scenario 6 additionally reports whether the abort line is present at all). Diagnostics only — no assertion, timing or fixture behaviour changes. The suite stays green locally (6 passed, ~17 s). Refs #367 * test: outwait the peer's SSE keep-alive in the cancel scenario (#367) First CI run diagnosis, not a tuned number. Scenario 6 failed on ubuntu-latest with `relay aborted the POST: True` in the log: the relay closed the in-flight upstream POST exactly as #270 PR D specifies, and the reference peer simply had not turned that into a cancellation within 10 s. Mechanism: the server has paused reading while it owns an open SSE response, so the client's FIN is not observed until uvicorn next tries to WRITE. This tool emits nothing after its progress report, so the next write is the SSE keep-alive at `_SSE_PING_INTERVAL` — 15 s. Any budget under that interval is unpassable on Linux no matter how healthy the relay is. macOS surfaces the same disconnect in 0.01 s (measured), which is why this was green locally 64 runs in a row. So the wait becomes 40 s (two ping intervals) and the test now MEASURES and prints the observed latency. Rule 4 forbids asserting on elapsed time, not recording it — and a printed number turns a future regression into a visible drift rather than a sudden red. CI gains `-s` because captured output is shown only for failing tests, so the healthy value would otherwise never reach the log. The per-test pytest-timeout is raised to 90 s for this test alone: a 40 s wait inside the injected 30 s default would race. Rule 3 in the conftest docstring is amended to allow a raised budget when the number is justified by a measured mechanism. This is a property of the reference peer, not of the relay, and it is the kind of thing only an end-to-end harness finds. Refs #367, #270 * fix: raise RelayDied when drain hits EOF (#368 review R1F1) `StdioClient.drain` silently returned whatever it had collected on relay stdout EOF, treating a crashed relay the same as successful quiescence. `_take` already raises `RelayDied` on EOF for exactly this reason, but `drain` did not, so a relay that crashed mid-drain would make every "nothing arrived" assertion built on it (e.g. scenario 6's "no late response for the cancelled id") pass VACUOUSLY — the exact failure this harness exists to catch, silently masked as a pass. `drain` now raises the same `RelayDied` on EOF, re-enqueueing the sentinel first exactly like `_take` does, so every later call fails just as fast. Checked all three existing call sites in test_e2e.py (scenarios 5 and 6) — none legitimately drains after an EXPECTED relay exit, so no `allow_eof` escape hatch was added; the default stays strict. Added tests_integration/test_stdio_client.py — a harness-correctness test (not a spec scenario) that kills the relay process and asserts `drain` raises `RelayDied`. Revert-checked: with the raise reverted to a plain `return collected`, the new test failed with "DID NOT RAISE RelayDied"; restored and confirmed green. * docs: correct CLAUDE.md's "zero threads" claim for the legacy era The run() bullet claimed the legacy era runs with "zero threads", which is false whenever OAuth is configured: run() calls _start_proactive_refresh() unconditionally, before the era branch, spawning a daemon thread on either era when a refresher/expiry getter is set (proactive refresh defaults on). The --oauth-eager cold-start daemon is the same shape — also started unconditionally before the era branch, gated only on --oauth-eager, not on era. Rescoped the sentence to the era-gated threads only (stdin reader, listen/resource streams) and added the era-independent OAuth machinery as its own clause. Extended the adjacent signal-handler bullet's thread list to match, so it does not repeat the same overclaim by omission. Wording-only, no behavior change — no test. * fix: bound the post-SIGKILL wait in StdioClient.close() close()'s docstring promised "EOF, then terminate, then kill — all bounded", and conftest's determinism rules require every teardown wait to be bounded (rule 6), but the final proc.wait() after kill() had no timeout. A process still alive after SIGKILL cannot be escalated further from user space (there is no stronger signal), so a stuck process there (kernel-side uninterruptible I/O) would hang this wait, and with it the whole test suite's teardown — exactly what rule 6 exists to prevent. Bounded it at 5.0, matching the first wait's default. Left the TimeoutExpired to propagate on expiry rather than swallowing it: this file's established pattern for an unbounded-wait risk is to surface it loudly (RelayDied from _take/drain), not hide it. Added a unit-style test driving a stub Popen through close()'s full EOF -> terminate -> kill escalation, asserting every wait() call (not just the first two) received a bounded timeout. Revert-checked: removing the new timeout made the assertion fail on the recorded None; restored and confirmed green.
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
1 participant
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.
Final Phase 2 PR for #270. Implements the PR D Final Design — Synthesis Verdict (verdict GO-WITH-CHANGES; all 11 required changes, implementation spec §3.1–3.9, test list §3.10), as amended by Design Amendments D1–D3.
The defect. On the modern era, cancellation does not exist at all today. A
notifications/cancelledsits unread in the pipe while the dispatch blocks insideresp.iter_text(), and spec rev 2026-07-28 offers no other signal:So honoring a cancel requires reading stdin while a dispatch blocks.
What was implemented
Era-gated permanent stdin handoff (§3.1/§3.2). A daemon reader thread runs
for line in sys.stdin:— the same text-mode object_enforce_lf_stdioreconfigures on win32, neveros.read(0)/sys.stdin.buffer— and feeds an unbounded FIFO queue that run()'s loop drains. Spawned once at era resolution,era == "modern"only. A blocking read on a daemon thread is the only portable mechanism (§3.6):select()does not work on pipes on Windows and a blockedReadFilecannot be cancelled from Python, so any second thread that touches stdin must own it forever — hence permanent, not scoped.The reader mutates no relay state (required change 7). Per line it enqueues, unchanged and in order, and acts out-of-band on exactly one thing: on a cancel matching the published in-flight request it sets the abort event and then closes the published response handle.
tracker.add,_mrtr_handle_cancel,protocol_version,session_id, the 401/403/404 ladder and every MRTR dict stay on the consumer, so each single-writer invariant survived without a re-audit. Because exactly one thread ever reads stdin, nothing is reordered, no double-dispatch is possible, and a partial-line split cannot occur.Abort is a distinct terminal outcome, never an exception (required changes 2/3/4/6/8). The cross-thread close surfaces as
httpx.HTTPErrorwithemitted=False— and untreated, the attempt loop re-POSTs a cancelledtools/callup to MAX_RETRIES times, the recorded trap._StreamResult.abortedplus_aborted_result()(status 0, so every status comparison a consumer might reach first falls on the silent side) close it:httpx.HTTPErrorarm consults the event first (stop-wins, the_consume_restarttemplate);RuntimeErrorarm catcheshttpx.StreamClosed— aStreamError, sibling ofHTTPError— which otherwise escapes to run()'s never-crash net and answers the cancelled id with "internal relay error"; abort unset → it re-raises;iter_text()without raising at all, which would leak a false-normal 200 into the recovery ladder and_mrtr_run_retry;input_required_abortfunnel, the "stream interrupted" error. The cancel tracker cannot substitute:tracker.addruns only when the consumer dequeues the cancel line, strictly after those writes;In-flight cell hygiene (required change 8).
_InFlightPostfollows PR B's_ResourceSubscriptionsbase change 5 exactly — the response is closed, never the client._post_and_streamtakes the samepublish_responseparameter_listen_stream_loopalready takes and retracts the handle from afinally, so a stale handle can never be closed on behalf of the next request. Publication is owned by the consumer and keyed on the id the client is waiting on.MRTR (§3.5, required changes 5/10). Nothing is ever POSTed upstream. A mid-retry cancel disconnects the retry POST;
_mrtr_run_retryreturns without purging or answering, leaving both to the queued cancel line's_mrtr_handle_cancel— purging there would strand the outstanding minted requests as dialogs nobody retires, and_mrtr_abortwould write an error under an id the client withdrew. A swallowed-then-aborted transaction does not route throughinput_required_abortfor the same reason.EOF and teardown (§3.8, required change 9). The reader enqueues a sentinel last, so FIFO order makes the drain complete. The reader is
daemon=Truewith a tiny bounded join: a read parked in the C-levelreadline()is not portably interruptible, and an unbounded join would hang every shutdown that happens while a client is idle.Doc truth-up (required change 11). Four comments literally predicted this PR (
_ModernState's "would require a second thread reading stdin … deliberately deferred",_mrtr_handle_cancel's "there is nothing in flight there to cancel", run()'s "not yet bridged", themrtr_txnsnote) and are corrected. cli.py carries no "single-threaded stdin" claim; README/docs describe only the cancel filter, unchanged on both eras.Owner questions (§4)
_paginate_and_streamcontinuation?_post_parsed, which buffers instead of streaming and publishes no handle, so it does not share the published-handle path and wiring it would mean new machinery. Consequence is benign and bounded: the abort event is set, no handle is found, the merged list still goes out (exactly pre-PR-D behaviour), the per-linefinallyclears the cell. Recorded in a comment at the branch itself, where a reviewer meets itqueue.SimpleQueue, which has nomaxsizeparameter at all. A cap that blocked the reader would reinstate the exact deafness this PR removes; unlike_MRTR_MAX_TXNSthere is no two-responses-for-one-id hazard to bound against. Rationale in a comment per §3.7Design Amendment D1 — approved (#270)
The design was amended; this implementation is conformant to the amended design. The evidence that drove it:
The proposed default ("yes — it falls out of the reader for free") is not implementable without a regression, and it does not fall out for free — it needs an explicit
abort_all()at EOF.run()test feedspatch("sys.stdin", StringIO(...)). The reader drains it and hits EOF while the consumer is dispatching line 1, so an EOF-abort fails a large share of the existing suite — colliding with "all existing tests stay green".echo '{...}' | mcp-stdio relay …is a supported invocation, and EOF-abort kills the only request the user asked for._write_lineraisesBrokenPipeError, run()'s never-crash net absorbs it, and the consumer drains to the sentinel and exits. EOF-abort buys latency, not correctness.Implemented as sentinel + full drain, no abort. Rationale recorded in
_stdin_reader_loop's docstring.D2 (the terminal-return abort check) and D3 (pagination known-gap +
SimpleQueue) were approved in the same comment. D2 is pinned bytest_abort_on_a_stream_that_ends_without_raising, whose primary half drives the non-raising path with a real cross-thread close from the reader thread; removing the pre-return check makes it fail with the exact harm it prevents —{"error": {"code": -32000, "message": "empty response from server"}, "id": 2}written under the id the client just cancelled.Design Amendment D2 — approved (#270)
Classifying abort only in the exception arms leaks a false-normal terminal: a cross-thread close landing between chunks, after the underlying stream already reached EOF, ends
iter_text()without raising at all, so neither exception arm sees it and_post_and_streamwould return a healthy-looking 200 withaborted=False. The abort event is now consulted before every terminal return.Test coverage
All 12 named tests from §3.10, plus one for the gap above. Every schedule point is an event or a queue — no sleeps, no real network.
test_modern_cancel_aborts_inflight_posttest_aborted_id_writes_nothingtest_next_request_dispatches_after_aborttest_cancel_races_completiontest_cancel_during_mrtr_retrytest_swallowed_then_abort_no_input_required_abortinput_required_abortfunnel gatetest_abort_checked_before_retry_attempt_and_sleeptest_stale_handle_never_closedfinallyun-publish (cell level) and the id match (end to end)test_eof_sentinel_drains_queuetest_legacy_spawns_no_readertest_teardown_with_parked_readertest_stream_closed_not_internal_errortest_abort_on_a_stream_that_ends_without_raisingRevert-checks verified, not just intended. Disabling
_abort_requestedfails 8 of the 10 end-to-end tests; disabling theRuntimeErrorarm, the false-normal gate, theHTTPErrorabort arm, and the bounded join each fail their named test.1629 passed, 3 skipped(from 1616 + 3 at8a0947b).ruff@0.16.0 checkandformat --checkclean.test_rapid_subscribes_coalesce_into_two_opensnow waits for the last subscribe's synthesized reply rather than for the generator to have yielded it: on the modern era that generator runs on the reader thread, where a yield means ENQUEUED, not processed. The relay's coalescing guarantee is unchanged; the test's proxy for "all five landed" was.AC3 confirmation
The legacy era is untouched by construction, not by assertion:
era == "modern"gate);stdin_sourceissys.stdinthere, so the loop is the literal synchronousfor line in sys.stdin:— same object, same iteration protocol, zero threads, zero new per-line calls;in_flightisNone, so everyabort/publish_responseargument staysNoneand_post_and_streamis byte-identical;notifications/cancelledkeeps being forwarded upstream as an ordinary POST while the cancel tracker drops the late response — spec-compliant with the 2025-06-18 SHOULDs.This is not cosmetic: an eager legacy-side
tracker.addwould make a cancel that today lands after a completed response start dropping that response mid-stream, a real timing-observable change.Notes for reviewers
__version__andCHANGELOG.mdare untouched (release-please). The diff ispyproject.toml,uv.lock,relay.py,test_relay.py.pytest-timeoutjoins[dev](henceuv.lock): run() installs signal handlers, so these tests must drive it from the main thread, and a regression that reinstates the deafness would hang CI silently instead of failing.stdin_sourceone-liner (the AC3 hinge); theabort.set()-before-resp.close()ordering inabort_if_matches(it is what lets the consumer tell a cancel from a transport fault); and publication keyed onclient_idfor the whole_mrtr_run_retryloop rather than per POST — a deliberate choice, because arequestState-only chain re-POSTs with no client involvement and a per-POST cell would be deaf in the gaps while the queued cancel sits behind that frame.🤖 Generated with Claude Code