Repository navigation
Harden the dual-era stream loop's era-lock and rejection semantics - #3040
Merged
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Post-merge review follow-up to #3038. An adversarial review pass over the dual-era driver found a family of cases where a request that failed could still lock the connection's era — and on stdio a connection is a whole subprocess, so a locked-then-stranded connection has no recovery short of a restart.
The era-lock fixes
The invariant the driver documents is: released auto-negotiating clients fall back on any error code except -32022, so a failed probe must leave the legacy handshake available. Three openers violated it:
clientInfo: 42): classification checks key presence only, so the era locked modern before pydantic rejected the content. The request failed with a fallback-eligible -32602, but the fallback then hit -32022 forever. The lock now commits only after the request succeeds, mirroring the legacy side's existing "lock only on success" rule.initializestamped with the envelope triple: routed modern (the triple won), failed METHOD_NOT_FOUND (initializedoesn't exist at 2026-07-28), and locked the era anyway.initializeis legacy-distinctive by definition, so it now always takes the handshake path regardless of decoration.subscriptions/listenas the opener: the not-served-on-this-transport rejection used to land after the lock; now, like every failed request, it locks nothing.No conforming client — released SDK or otherwise — produces any of these frames; the fixes are robustness against buggy third-party modern clients.
Compatibility restoration
A bare
server/discoveron a legacy-locked connection now falls through to the loop runner's per-version validation, restoring the byte-identical METHOD_NOT_FOUND (-32601, same message and data) a handshake-only server produced — #3038 had it rejecting with -32600. Only envelope-bearing frames get the cross-era INVALID_REQUEST; a conforming legacy client can never send the reserved triple, so legacy traffic is byte-identical again in full.Note for anyone bisecting client behavior: v2.0.0b1 ships the -32600 variant for this one frame shape (it carries #3038 but not this fix), so a legacy-locked bare
server/discoveranswers -32600 on b1 and -32601 before #3038 and from this PR onward.Smaller fixes
classify_inbound_requestrejects a present-but-non-string protocol version with a descriptive INVALID_PARAMS instead of raisingValidationErrorout of its own -32022 payload construction (requestedis astrfield). The dispatcher's exception ladder happened to mask that as a generic -32602 — same code, now deliberate, with a message that names the defect. Unreachable over HTTP (the header rung fires first), so the wire delta is stdio-only.transport.can_send_requestso per-message transport metadata agrees with its denial, matching the modern HTTP entry's context — closing a latent inconsistency before anything consumes the field.NotifyOnlyOutboundinherits the no-channel refusal instead of duplicating it; thehas_standalone_channel/Raisescontracts now document that a modern connection refuses requests on a real channel.subscriptions/listen; a staleserve_loopdocstring claim is corrected. Two pyright suppressions in the stdio test doubles are replaced with typeshed-compatibleBuffersignatures.Validation
Every fix has a pinning test (rejection codes, lock neutrality of each failing opener, the byte-identical legacy fall-through, the classifier guard, the transport mask). Beyond the suite (100% coverage), each formerly-stranding opener was driven against a live stdio subprocess and shown to leave the legacy handshake working on the same pipe, with the full original probe matrix (auto/legacy/pinned negotiation, pipelined probe ordering, -32022 round-trip) passing unchanged.
Round 2 — review follow-ups
Review found that the round-1 lock-on-success rule still approximated "client-visible success" with "the handler returned". Two windows violated the invariant, plus two cross-transport divergences on the modern path:
initializeoverwrote the committed legacy lock — bricking the freshly-handshaked client — and the mirror direction (a legacy commit clobbering a modern lock) was possible too. The era cell is now monotone: oneera_settlessettlement predicate with site-local atomic commits, first success wins, and a straggler from the other era can never move a committed lock. Its response still stands — it was already earned."Request cancelled", but the era had already locked. The settlement predicate now skips the lock when the request's cancel event is set, tying "success" to what the client actually observes. (A cancel racing a genuinely blocked response write can still slip through; that residue belongs to the dispatcher's existing respond-after-cancel divergence and dissolves with it.)str(e)on the wire — while the identical request over modern HTTP produced the generic-32603 Internal server error. The code-0 pin exists for v1 compat, which the modern era doesn't have. Both modern entries now share onemodern_error_databoundary; debugraise_exceptionsstill re-raises.clientInfo/clientCapabilitiesto not-supplied while the stream path rejected them atConnection.from_envelopeconstruction. For spec methods both transports already rejected identically (the kernel's per-version params surface types the reserved_metakeys strictly); custom methods diverged.from_envelopenow takes the classifier's raw values and owns the coercion, so every modern entry — HTTP, stream loop, andmodern_on_request— builds connection identity identically.None == Noneequality and make the absent-header outcome depend on the body value. An absent or disagreeing version header is uniformlyHEADER_MISMATCH, the HTTP wire is untouched wherever it was well-defined, and the string guard is reachable only on header-less transports, making the round-1 "stdio-only delta" claim above exactly true. (Through the shipped HTTP manager the era routing is header-only, so these cells harden the modern entry's own contract.) The publicServer.rundocstring also picked up the first-success wording.Each behavioral fix carries a regression test that failed on the round-1 code (the overwrite and cancelled-away tests reproduce the races deterministically), and the scenarios were re-driven against a live stdio subprocess: the pipelined-race brick, the sanitized internal error, the failed-opener fallback, and the cancel-never-locks path all behave as documented, with auto/legacy negotiation unchanged.
Round 3 — the timeout brick and the third modern entry
Further review surfaced the last member of the brick family, plus a pre-existing gap in the parity story:
mode='auto'(reproduced live): aserver/discoverprobe that outlives the client's 10s timeout can still succeed on a slow-starting server — the response was written, the server cannot know the client stopped waiting, and discover is inline so even a courtesy cancel could not land before the commit. The connection locks modern, the pipelined fallbackinitializegets -32022, andnegotiate_autotreated that as terminal: the defaultmode='auto'could not connect to any stdio server that takes >10s to its first response. No server-side change can close this; the client-side fix is that -32022 from the fallback handshake is itself positive modern evidence (it names the server's versions), so the policy now re-probes once at a mutual version — bounded by the existing two-attempt loop, so a pathologically slow server still terminates with a genuine error. Covered by policy unit tests and a deterministic end-to-end test (a relay holds the probe frame until the post-timeoutinitializeis sent — no sleeps).Client(server)in modern mode delivered the protocol-forbiddenelicitation/createframe to the client, which answered a misleadingMethod not found. The entry's dispatch context is now wrapped in the same server-requests denial, so the call refuses server-side with the no-back-channel contract — which is also the exact wording the client-callbacks docs page already used to describe this path.AI Disclaimer