Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@jokemanfire 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce07d77f97
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Both review findings addressed in the latest push: Surface retained status during initialization — Preserve the typed error in quit_status — 370 lib tests pass, all-features check clean, clippy/fmt clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d944818fcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Second round addressed in the latest push: Publish quit status before the receive channel closes — the worker task now holds a clone of Return the retained startup failure as a transport error — Keep the typed error until the receiver consumes it — the startup error is now shared via 370 lib tests pass (640 with auth), all-features check clean, clippy/fmt clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e22abf26a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Third round addressed in the latest push: Preserve startup error variants for live senders — the shared ack now uses Recover quit status from failed fallback sends — Auto fallback errors now recover Validation: 794 all-feature lib tests pass, the 10 streamable HTTP status metadata tests pass, strict all-target Clippy and rustfmt are clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a2d9c6ec3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Fourth round addressed in the latest push: Preserve the underlying startup error in Display — Avoid closing every fallback transport to poll worker status — Validation: 795 all-feature lib tests pass, both fallback regression tests pass, the 10 HTTP status metadata tests pass, and strict all-target Clippy/rustfmt are clean. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cd491fc3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Fifth round addressed in the latest push: Recover retained errors after legacy notification sends — fixed at the transport source rather than broadening client-side recovery. The legacy Added an integration regression for plain Validation: 795 all-feature lib tests, all 5 discover/startup integration tests, and all 10 HTTP status metadata tests pass. Strict Clippy passes for both all-target/all-feature and the enabled startup integration configuration; rustfmt is clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a8a93c455
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Sixth round addressed in the latest push: Preserve live errors during fallback recovery — fallback recovery now replaces only the synthetic channel-closed error. The initialized-notification 429 regression now runs in both plain Validation: 795 all-feature lib tests, all 6 startup integration cases, and all 10 HTTP metadata tests pass. Strict Clippy passes for all targets/features and the enabled startup integration configuration; rustfmt is clean. |
jokemanfire
left a comment
There was a problem hiding this comment.
Verified at head 9133157 (base f2b0ddf). The core mechanism works and I could not break the ordering trick; the asks below are about the shape of the new public surface, not about the fix being wrong.
What I ran
cargo test -p rmcp --test test_discover_http_client_startup --features "client reqwest transport-streamable-http-client-reqwest transport-streamable-http-server"-> 6 passed (incl. bothinitialized_notification_failure_preserves_typed_http_statuscases:initializeandauto_fallback).cargo test -p rmcp --lib --features "client server transport-worker transport-streamable-http-server" -- fallback startup_failure-> 3 passed (auto_fallback_send_recovers_retained_startup_cause,fallback_error_does_not_close_transport_without_quit_status,startup_failure_is_retained_in_quit_status).cargo fmt --checkclean.
The keep_input_open clone does what the comment claims: to_handler_tx is the sender of from_transport_rx (worker.rs:284-285), so receive() cannot observe EOF before the watch write at worker.rs:352-355. EOF => status visible is sound, and recover_fallback_error's close() branch therefore cannot block on a worker that is still inside a hanging startup POST (the send queue can close at run() return, but the task is then already at the publish point).
Asks
-
The retained cause is reported as
WorkerTransport<...>, andDynamicTransportError::downcastcannot reach it.WorkerTransport::quit_statusbuilds the error withtype_name::<Self>()/TypeId::of::<Self>()(worker.rs:541-548) and boxesRetainedQuitReason<W::Error>. Probe with a failingWorkerTransportat this head:transport_name = rmcp::transport::worker::WorkerTransport<zz_probe_quit_identity::FailWorker> type_id is Worker = true downcast io::Error = false downcast Retained = true display = Transport [rmcp::transport::worker::WorkerTransport<...>] error: Startup failed peer unreachable, when startup request failedConsequences on the exact path this PR is about (ack lost /
receive()EOF, i.e. the #1441 scenario):ClientInitializeError::TransportError { error, .. }.error.is::<StreamableHttpClientTransport<C>, RoleClient>()isfalse, sodowncast::<T, R>()- the API the PR body cites for typed recovery - fails; andDisplayprints an internal generic type. On the live-ack path the identity is still correct because the error is built withClientInitializeError::transport::<T>. The fake transport inservice/client.rs:2716hides this because itsquit_statususesSelf::name().Options: (a) rewrap at the points where
Tis known (expect_next_message,serve_inner) withDynamicTransportError::from_parts(T::name(), TypeId::of::<T>(), cause.error)and add adowncast_retained::<E>()accessor next todowncast; or (b) havequit_statusreturn the rawArc<WorkerQuitReason<W::Error>>onWorkerTransportand let each concrete transport construct theDynamicTransportErrorwith its own name/id. Either way, a test should go through a realWorkerTransport, otherwise ask 1 regresses silently. -
StartupSharedchanges what existing callers see on the live startup path. Previously the ack carried the original variant (responder.send(Err(err))), somatches!(e, StreamableHttpError::HttpStatus(_))/AuthRequired(_)matched for a 401 or 429 during initialize. Now it is one level deeper, and this repo's own test had to be changed to keep passing:test_streamable_http_status_metadata.rs:113now callserror.startup_error().auth_challenge()andis_authorization_required()still work (source()walk), but external code that matches the enum directly does not. Given the "additive API only" rule, please either state this explicitly in the PR body/release notes or add one accessor that unwraps both wrappers (e.g.ClientInitializeError::transport_error_typed::<T>() -> Option<&T::Error>), so callers do not need to know aboutStartupSharedat all. -
No CHANGELOG entry. This adds
QuitReason::Transport,WorkerQuitReason::StartupFailed,StreamableHttpError::StartupShared,Transport::{is_closed_error, supports_quit_status, quit_status}andRetainedQuitReason.crates/rmcp/CHANGELOG.mdhas an[Unreleased]section and the PR does not touch it; a one-lineFixedentry (plus aChangednote for theStartupSharednesting if you keep it) is the minimum. -
Non-blocking:
supports_quit_statusduplicatesquit_status().is_some()for every implementation except the side-effectingclose()call inrecover_fallback_error. If you keep it, the trait doc should say the only reason it exists is to avoid callingclose()on transports with no worker; three new methods on a public trait is a lot of surface for one bug fix.
|
Thanks for the careful review — the downcast gap on the retained path was real, and the fake transport was indeed hiding it. All four asks are addressed in the latest push (head 1. Typed access on the retained path — added The transport identity check is unchanged — the type id already matches because The fake transport in 2. if let Some(e) = error.downcast_ref::<StreamableHttpClientTransport<reqwest::Client>, RoleClient>() {
match e.startup_error() {
StreamableHttpError::HttpStatus(m) => { /* m.status, m.retry_after */ }
StreamableHttpError::AuthRequired(a) => { /* challenge */ }
_ => {}
}
}This is also stated explicitly in the PR body and in a CHANGELOG 3. CHANGELOG — added 4. Verified: 795 all-feature lib tests, 6 discover-startup + 10 HTTP-status-metadata integration tests, strict all-target clippy and rustfmt all clean. |
2524611 to
4405844
Compare
|
Rebased onto the latest main (v3.10.0, The rebase keeps the fix intact on the new API:
Head is now a single commit Re-verified on the rebased tree: 842 all-feature lib tests, discover-startup 6 + status-metadata 10 + empty-2xx-notification 6 + response-limits 80 integration tests, strict all-target clippy and rustfmt all clean. |
jokemanfire
left a comment
There was a problem hiding this comment.
Re-verified at head 4405844 (base c644b75, v3.10.0). Answers 1 and 4 from my previous review hold up; the rebase left three gates red, and one of them is exactly the failure mode ask 2 predicted. Details below are from the head-SHA run 43585767134 plus a local run at 4405844.
Blocking
1. Run Tests (no local feature) — 4 failures, crates/rmcp/tests/test_streamable_http_limit_lifecycle.rs
---- oversized_discover_fails_without_fallback_or_retry::case_1_json stdout ----
unexpected transport error: DynamicTransportError { transport_name: "rmcp::transport::worker::WorkerTransport<...>",
error: StartupShared(SharedStartupError { error: ResponseBodyTooLarge { max_size: 64 }, source: ResponseBodyTooLarge { max_size: 64 } }) }
I reproduced it locally: at 4405844 the four cases case_{1..4} fail, while at c644b75 the same four pass (4 passed). The assertion at test_streamable_http_limit_lifecycle.rs:481-491 is
matches!(error.error.downcast_ref::<StreamableHttpError<reqwest::Error>>(),
Some(StreamableHttpError::ResponseBodyTooLarge { max_size: 64 }))so downcast_ref works, but the variant is StartupShared and no longer matches. That is ask 2 in practice, not in theory: every caller matching a startup variant (HttpStatus, ResponseBodyTooLarge, AuthRequired, UnexpectedContentType) silently stops matching. ResponseBodyTooLarge came in with #1246, so these cases are not this PR's tests to rewrite.
Two honest ways out:
- (a) keep the live acknowledgement typed. Deliver the original
StreamableHttpError<E>on the ack path and keep theArcsharing only for the retained quit reason. This leaves the narrow single-shot race the earlier Codex finding describes, but that race belongs to the retained path and is already documented there. - (b) declare it breaking. Keep
StartupShared, usefix!:plus aBREAKING CHANGE:footer, adapt the fouroversized_discover_*cases tostartup_error(), and call it out in the release notes.VERSIONING.mdcounts "change behavior in a way callers could not reasonably anticipate" as breaking, so option (b) cannot ship under a plainfix:either.
2. Public API Check — removed impls
SEMVER_RELEASE_TYPE=minor DENY: --deny changed --deny removed
Error: Removed items not allowed: [impl Error for StreamableHttpError<E>,
impl Display for StreamableHttpError<E>, impl Display for WorkerQuitReason<E>]
The derived impls now carry bounds inferred from the SharedStartupError<E> source field, so their emitted signatures differ from c644b75. Under fix: the job denies changed/removed impls; same fork as 1(a)/1(b) — either keep the generated impls byte-identical or mark the commit breaking.
3. Generate Documentation — two rustdoc errors
error: unresolved link to `Error::source` crates/rmcp/src/transport/worker.rs:208
error: redundant explicit link target
[Error::source] has no Error in scope at that point; [std::error::Error::source] resolves. The doc job runs with -D warnings, so this blocks.
Settled from the previous round
- Ask 1 —
DynamicTransportError::downcast_refexists, looks throughRetainedQuitReason, the fake transport now boxes the same wrapper, andstartup_failure_is_retained_in_quit_statusasserts through a realWorkerTransport. Good; the identity confusion I hit is gone. - Ask 3 — I asked for a
CHANGELOG.mdentry. Withdrawn: this repo has noCHANGELOG.mdand release-plz derives notes from commits. The commit footer is the right channel, which is what item 2 needs anyway. - Ask 4 —
supports_quit_status()is a capability question ("does this transport ever publish status"), notquit_status().is_some(), and the trait doc now says so. Dropped.
Verification I did not do
I did not rebuild the full --all-features suite locally (your 842 figure); the three red gates above are from the head-SHA CI run and the targeted local reruns named in item 1.
…contextprotocol#1441) A streamable-http startup handshake failure was reported as WorkerQuitReason::Fatal with the real typed error replaced by a synthetic TransportChannelClosed, and the terminal reason had no retained carrier, so an observer whose own timeout already fired lost the cause entirely. Implements options B and C from the triage on modelcontextprotocol#1441: - WorkerQuitReason::StartupFailed carries the real typed error instead of a synthetic placeholder; startup failures log at warn, not error. - WorkerTransport retains the worker terminal reason in a watch slot, exposed via Transport::quit_status / supports_quit_status, so a late observer still learns the cause after the single ack delivery window. - DynamicTransportError::downcast_ref borrows the typed error through both the live-ack wrapper and the retained quit-reason wrapper, returning the same &T::Error on either path. - Transport::is_closed_error / Worker::is_closed_error let initialization recovery replace only synthetic channel-closed errors, never a live typed one. Rebased onto v3.10.0 (post_message_with_response_limits / response_limits API). BREAKING CHANGE: the live streamable-http startup acknowledgement now yields StreamableHttpError::StartupShared instead of the bare failure variant, so the typed error can be shared (via Arc) with the retained quit reason. Call StreamableHttpError::startup_error() on it — or DynamicTransportError::downcast_ref followed by startup_error() — to recover the original HttpStatus / AuthRequired / ResponseBodyTooLarge / ... variant; startup_error() is a no-op for every other variant. Code that matched a startup variant directly (e.g. matches!(e, StreamableHttpError::HttpStatus(_))) must go through startup_error().
4405844 to
78f77c1
Compare
|
Addressed the three red gates at the latest head ( Why (b) over (a): option (a) keeps the ack variant bare, but since the typed error is not Gates:
CHANGELOG: reverted the hand edit — this repo's changelog is release-plz-generated from commits, so the Verified on the rebased tree: 842 all-feature lib tests, 136 integration tests across discover-startup / status-metadata / limit-lifecycle / empty-2xx / response-limits / initialize-error / concurrency, strict all-target clippy, |
What
Fixes #1441 — a streamable-http startup handshake failure was reported as
WorkerQuitReason::Fatalwith the real typed error replaced by a syntheticTransportChannelClosed, and the terminal reason had no retained carrier, so an observer whose own timeout already fired lost the cause entirely.Implements options B and C from @jokemanfire's triage comment on #1441.
B — typed error in the terminal reason
WorkerQuitReason::StartupFailed { error, context }(additive; the enum is#[non_exhaustive]), carrying the real typed error instead of a synthetic placeholder.warn(they are ordinary lifecycle events — peer unreachable, TLS failure, auth challenge — not internal faults), addressing the 500+ spurious ERROR lines.streamable_http_client.rs: both the startup-POSTErrarm andexpect_initializednow produceStartupFailedwith the real error. When the in-flightsend()receiver has already gone away (its own timeout fired), the typed error is retained in the quit reason; when the ack succeeds, the real error's text is still captured into the quit reason's context so the cause is never masked.C — retain the terminal reason for late observers
WorkerTransportnow retains the worker's terminal reason in awatchslot, written exactly once when the worker task quits, so the reason survives the single fire-and-forget ack delivery window.Transport::quit_status()(defaultNone— stdio/io/ws/custom transports are unaffected) returns the cause as an ownedDynamicTransportError.QuitReasongains an additiveTransport(DynamicTransportError)variant; the service loop reads the retained status aftertransport.close()sowaiting()/close()callers learn why the connection ended rather than an undifferentiatedClosed.Transport— only abnormal termination carries a cause.Notes for reviewers
StartupFailed'serrorstays the typedE(per the triage discussion), not a generic parameter onQuitReason— the cause crosses the service API boundary asDynamicTransportError, which keepsQuitReasonnon-generic and non-breaking. The typed error itself is notClone, so it lives in exactly one place: the in-flight ack, or the retained quit reason when the ack receiver is gone. This matches the report's "exactly one delivery attempt" concern by adding the retained carrier.startup_failure_is_retained_in_quit_status): a worker whose startup fails retains its cause inquit_statuseven with noclose()pending at the time of death.Typed error access (updated after review)
A startup failure's typed error is shared via
Arcbetween the in-flightsend()acknowledgement and the retained quit reason, so no single owned value can be handed back on both paths. Use the borrowed accessor to reach it uniformly:DynamicTransportError::downcast_ref::<T, R>()looks through both delivery wrappers — the live ack boxesT::Errordirectly; the retained quit reason wraps it inRetainedQuitReason<E>(unwrapped byRetainedQuitReason::startup_error()). It returns&T::Erroron either path.StreamableHttpError::StartupShared(aChangedbehavior noted in the CHANGELOG). CallStreamableHttpError::startup_error()on the borrowed error to recover the originalHttpStatus/AuthRequired/ ... variant — a no-op for every other variant, so it is safe to apply unconditionally:The owned
DynamicTransportError::downcast::<T, R>()still works on the live-ack path (the boxed error isT::Error); it returnsErr(self)on the retained path, where only the borrowed accessor reaches the typed error.Verification
cargo test -p rmcp --lib --all-features: 795 passed, 0 failedcargo clippy -- -D warnings: cleancargo fmt --check: clean