Repository navigation
fix(client): ignore late discovery replies after legacy fallback - #1436
charliermarsh wants to merge 1 commit into
Conversation
|
@jamadeo is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
There was a problem hiding this comment.
Reviewed at head c231e448 (code read, not just the description) and ran the touched test target locally.
Verified
cargo test -p rmcp --test test_client_lifecycle_modes --features "client server"→ 24 passed, 0 failed (including the newauto_fallback_uses_initialize_reply× 6 andauto_startup_rejects_unrelated_responses× 4 combinations).cargo fmt --checkclean on the head SHA.- The trace in #1400 (probe id 0 times out,
initializeid 1 sent, late-32601for id 0) is now handled:expect_responseskips the remembered probe id for both success and error replies, and theDiscoverOutcome::Legacypath correctly passesNonebecause that reply was already consumed. - Reading
error.id.as_ref().is_some_and(...)is right: a null-id error is still surfaced asUncorrelatedErrorResponserather than silently swallowed.
Asks
-
Unbounded wait in the timeout fallback (please confirm or bound). Skipping the late reply removes the fail-fast path, and
expect_responsehas no deadline of its own. I modelled "late probe reply, then the server never answersinitialize" with a duplex transport,start_paused = true, and a 300 s outer timeout:- upstream
main:still_waiting_after_300s=false— the handshake fails immediately with an uncorrelated-response error. - this head:
still_waiting_after_300s=true— still blocked.
So for a server that answers the probe but then stalls, this converts a fast failure into a possible indefinite hang. #1400 explicitly says an overall startup deadline can still be appropriate. Would you apply
auto_discover_timeout(or a separate fallback deadline) to the whole Auto startup, so the fallbackinitializewait is bounded too? If deliberately unbounded, a comment saying so would help. - upstream
-
Late successful
DiscoverResultis discarded (case_3_late_success). At that point the client has already sentinitializewith the legacy version, so continuing is defensible — but the test then asserts the legacy path succeeds. Can you state the intended post-state for a genuinely modern server whose discover success arrives just after the deadline (does it answerinitialize, and what should the client report if it does not)? Worth one line in the doc comment either way. -
Only the last probe id is remembered. With the version-negotiation retry loop, a server that answers twice (duplicate reply for the first attempt) can still deliver an id that is not
last_discover_idwhileinitializeis outstanding, and that still fails the handshake. Rare, but if you want the invariant to be "no discovery reply can fail the fallback", a small set of probe ids is a one-line change over a singleOption. -
Coordination with #1435. That PR keeps the HTTP transport reading the probe POST before falling back, which makes the late-reply window much smaller. They do not conflict as written, but the combination should be tested once both land — the HTTP path is not covered by these tests.
No approval from me: the questions in 1 and 2 are open.
Summary
A legacy stdio server that takes 12 seconds to start can connect if it ignores
server/discover, yet fail if it repliesMethod not found. After 10 seconds, the client has already sentinitializewith ID 1. When the discovery error arrives with ID 0, the client gives up before reading the valid reply for ID 1.We now remember the ID of the last discovery attempt. After a timeout, the client skips late successes and errors for that ID while it waits for
initialize.Closes #1400.