Skip to content

fix(transport): retain terminal cause for startup failures (#1441) - #1444

Open
ip2a wants to merge 1 commit into
modelcontextprotocol:mainfrom
ip2a:fix/1441-terminal-cause
Open

ip2a wants to merge 1 commit into
modelcontextprotocol:mainfrom
ip2a:fix/1441-terminal-cause

Conversation

@ip2a

@ip2a ip2a commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

What

Fixes #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 @jokemanfire's triage comment on #1441.

B — typed error in the terminal reason

  • Add WorkerQuitReason::StartupFailed { error, context } (additive; the enum is #[non_exhaustive]), carrying the real typed error instead of a synthetic placeholder.
  • Startup failures now log at 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-POST Err arm and expect_initialized now produce StartupFailed with the real error. When the in-flight send() 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

  • WorkerTransport now retains the worker's terminal reason in a watch slot, written exactly once when the worker task quits, so the reason survives the single fire-and-forget ack delivery window.
  • New provided Transport::quit_status() (default None — stdio/io/ws/custom transports are unaffected) returns the cause as an owned DynamicTransportError.
  • QuitReason gains an additive Transport(DynamicTransportError) variant; the service loop reads the retained status after transport.close() so waiting() / close() callers learn why the connection ended rather than an undifferentiated Closed.
  • Normal termination (cancel, clean transport close, handler termination, idle timeout) is not reported as Transport — only abnormal termination carries a cause.

Notes for reviewers

  • StartupFailed's error stays the typed E (per the triage discussion), not a generic parameter on QuitReason — the cause crosses the service API boundary as DynamicTransportError, which keeps QuitReason non-generic and non-breaking. The typed error itself is not Clone, 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.
  • Added a regression test (startup_failure_is_retained_in_quit_status): a worker whose startup fails retains its cause in quit_status even with no close() pending at the time of death.

Typed error access (updated after review)

A startup failure's typed error is shared via Arc between the in-flight send() 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 boxes T::Error directly; the retained quit reason wraps it in RetainedQuitReason<E> (unwrapped by RetainedQuitReason::startup_error()). It returns &T::Error on either path.
  • For streamable HTTP the live acknowledgement now yields StreamableHttpError::StartupShared (a Changed behavior noted in the CHANGELOG). Call StreamableHttpError::startup_error() on the borrowed error to recover the original HttpStatus / AuthRequired / ... variant — a no-op for every other variant, so it is safe to apply unconditionally:
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) => { /* a.www_authenticate_header */ }
        _ => {}
    }
}

The owned DynamicTransportError::downcast::<T, R>() still works on the live-ack path (the boxed error is T::Error); it returns Err(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 failed
  • discover-startup integration: 6 passed; HTTP-status-metadata integration: 10 passed
  • strict all-target cargo clippy -- -D warnings: clean
  • cargo fmt --check: clean

@ip2a
ip2a requested a review from a team as a code owner November 11, 2026 01:24
@github-actions github-actions Bot added T-core Core library changes T-transport Transport layer changes labels Nov 11, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-14T13:56:42.429250Z 78f77c1 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown
Contributor

@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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread crates/rmcp/src/service.rs Resolved
Comment thread crates/rmcp/src/transport/worker.rs Resolved
@github-actions github-actions Bot added the T-service Service layer changes label Nov 11, 2026
@ip2a

ip2a commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Both review findings addressed in the latest push:

Surface retained status during initialization — expect_next_message now reads transport.quit_status() when the stream ends mid-handshake, so a startup failure's retained cause reaches ClientInitializeError::ConnectionClosed instead of a bare "connection closed". This covers the path where serve_inner is never created (init did not succeed).

Preserve the typed error in quit_status — quit_status no longer stringifies. The retained Arc<WorkerQuitReason<E>> is boxed into DynamicTransportError via a RetainedQuitReason<E> wrapper whose source() exposes the inner typed error. Callers can downcast to RetainedQuitReason<E> to recover the typed WorkerQuitReason<E> (and thus E), and source-only data such as an auth challenge survives the source chain (is_authorization_required keeps working). Regression test now asserts the downcast and the inner error kind.

370 lib tests pass, all-features check clean, clippy/fmt clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread crates/rmcp/src/service/client.rs Resolved
Comment thread crates/rmcp/src/transport/worker.rs Resolved
@ip2a

ip2a commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Second round addressed in the latest push:

Publish quit status before the receive channel closes — the worker task now holds a clone of to_handler_tx until after the watch write, so a concurrently polled receive() cannot observe EOF and snapshot quit_status() as None. EOF is now ordered after the terminal status is visible.

Return the retained startup failure as a transport error — expect_next_message now returns ClientInitializeError::TransportError carrying the owned DynamicTransportError cause (not a stringified ConnectionClosed), so the caller can inspect its source and downcast.

Keep the typed error until the receiver consumes it — the startup error is now shared via Arc on both sides: StartupFailed holds Arc<E>, and the ack carries the same value as StreamableHttpError::StartupShared (a dyn Error Arc). A receiver dropped after a successful queue no longer destroys the only copy — the typed error survives in the quit reason. The shared ack's #[source] keeps the source chain intact, so auth-challenge detection (is_authorization_required) still reaches the inner AuthRequired error.

370 lib tests pass (640 with auth), all-features check clean, clippy/fmt clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread crates/rmcp/src/service/client.rs Resolved
@github-actions github-actions Bot added the T-test Testing related changes label Nov 11, 2026
@ip2a

ip2a commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Third round addressed in the latest push:

Preserve startup error variants for live senders — the shared ack now uses SharedStartupError<E>, which retains the concrete Arc<StreamableHttpError<E>> and an erased handle to the same allocation for Error::source(). StreamableHttpError::startup_error() provides the equivalent typed extraction path: it transparently returns the original HttpStatus, AuthRequired, or other variant for inspection. auth_challenge() uses the same unwrapping, and the existing 429 regression now verifies status and Retry-After through this accessor. The typed value remains retained if the ack receiver is dropped.

Recover quit status from failed fallback sends — Auto fallback errors now recover quit_status() when the original startup failure closes the fallback request queue. Recovery also awaits transport closure when the status is not visible yet, ordering the read after the worker publishes its terminal state instead of reintroducing the EOF/status race. A regression test drives discovery timeout → fallback send failure and asserts the retained typed cause replaces the synthetic queue-closed error.

Validation: 794 all-feature lib tests pass, the 10 streamable HTTP status metadata tests pass, strict all-target Clippy and rustfmt are clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread crates/rmcp/src/service/client.rs Resolved
@ip2a

ip2a commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Fourth round addressed in the latest push:

Preserve the underlying startup error in Display — StartupShared now renders startup handshake failed: <original error>, and SharedStartupError delegates its own Display to the original typed error. Logs and enclosing ClientInitializeError retain actionable details such as HTTP 429; the status metadata test now asserts the underlying status is present in the rendered error.

Avoid closing every fallback transport to poll worker status — Transport now has a provided supports_quit_status() capability query (default false), implemented by WorkerTransport. Fallback recovery only awaits close() for worker-backed transports whose retained status is not visible yet. Transports using the default quit_status() keep their original error and are not closed or delayed. Added a regression test asserting both behaviors.

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.

@ip2a

ip2a commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread crates/rmcp/src/service/client.rs Resolved
@ip2a

ip2a commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

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 notifications/initialized POST and response processing are now treated as part of startup. On failure, the worker sends the original typed error through the notification responder as StartupShared and returns StartupFailed holding the same Arc, so both a live ordinary serve() caller and a late observer retain HTTP/TLS/auth metadata. No synthetic TransportChannelClosed is substituted.

Added an integration regression for plain ClientLifecycleMode::Initialize: initialize succeeds, the initialized notification receives 429, and the caller downcasts the transport error, reads HttpStatus { status: 429, retry_after: Some("17") } through startup_error(), and sees HTTP 429 in Display.

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread crates/rmcp/src/service/client.rs Resolved
@ip2a

ip2a commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Sixth round addressed in the latest push:

Preserve live errors during fallback recovery — fallback recovery now replaces only the synthetic channel-closed error. Worker and Transport expose an additive is_closed_error() classifier (default false); WorkerTransport delegates to its worker, and both streamable HTTP worker implementations identify their synthetic closed variant. A real live StartupShared/HttpStatus error is returned unchanged and remains directly downcastable; only a channel-closed placeholder is recovered from quit_status(). The close-and-wait synchronization is likewise restricted to that placeholder path.

The initialized-notification 429 regression now runs in both plain Initialize and Auto legacy-fallback modes. The Auto case unwraps LegacyFallbackFailed, then successfully downcasts the fallback error to StreamableHttpClientTransport<reqwest::Client> and reads status 429 plus Retry-After: 17, proving retained-status recovery no longer launders a live typed error.

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 jokemanfire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. both initialized_notification_failure_preserves_typed_http_status cases: initialize and auto_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 --check clean.

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

  1. The retained cause is reported as WorkerTransport<...>, and DynamicTransportError::downcast cannot reach it. WorkerTransport::quit_status builds the error with type_name::<Self>() / TypeId::of::<Self>() (worker.rs:541-548) and boxes RetainedQuitReason<W::Error>. Probe with a failing WorkerTransport at 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 failed
    

    Consequences 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>() is false, so downcast::<T, R>() - the API the PR body cites for typed recovery - fails; and Display prints an internal generic type. On the live-ack path the identity is still correct because the error is built with ClientInitializeError::transport::<T>. The fake transport in service/client.rs:2716 hides this because its quit_status uses Self::name().

    Options: (a) rewrap at the points where T is known (expect_next_message, serve_inner) with DynamicTransportError::from_parts(T::name(), TypeId::of::<T>(), cause.error) and add a downcast_retained::<E>() accessor next to downcast; or (b) have quit_status return the raw Arc<WorkerQuitReason<W::Error>> on WorkerTransport and let each concrete transport construct the DynamicTransportError with its own name/id. Either way, a test should go through a real WorkerTransport, otherwise ask 1 regresses silently.

  2. StartupShared changes what existing callers see on the live startup path. Previously the ack carried the original variant (responder.send(Err(err))), so matches!(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:113 now calls error.startup_error(). auth_challenge() and is_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 about StartupShared at all.

  3. No CHANGELOG entry. This adds QuitReason::Transport, WorkerQuitReason::StartupFailed, StreamableHttpError::StartupShared, Transport::{is_closed_error, supports_quit_status, quit_status} and RetainedQuitReason. crates/rmcp/CHANGELOG.md has an [Unreleased] section and the PR does not touch it; a one-line Fixed entry (plus a Changed note for the StartupShared nesting if you keep it) is the minimum.

  4. Non-blocking: supports_quit_status duplicates quit_status().is_some() for every implementation except the side-effecting close() call in recover_fallback_error. If you keep it, the trait doc should say the only reason it exists is to avoid calling close() on transports with no worker; three new methods on a public trait is a lot of surface for one bug fix.

@github-actions github-actions Bot added the T-documentation Documentation improvements label Nov 13, 2026
@ip2a

ip2a commented Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

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 2524611).

1. Typed access on the retained path — added DynamicTransportError::downcast_ref::<T, R>(). downcast consumes self and returns an owned T::Error, which is impossible when the typed error is shared via Arc between the ack and the quit reason (no owned value exists to hand back). downcast_ref borrows instead, so it reaches the same &T::Error on both delivery paths: the live ack (boxed T::Error directly) and the retained quit reason (a RetainedQuitReason<E> wrapper, unwrapped via the new RetainedQuitReason::startup_error()).

The transport identity check is unchanged — the type id already matches because StreamableHttpClientTransport<C> is an alias of WorkerTransport<...>; the failure was purely the extra wrapper, which downcast_ref now looks through. quit_status also names the transport via Self::name() for consistency with DynamicTransportError::new.

The fake transport in service/client.rs was masking exactly this — it boxed the bare io::Error. It now boxes a shared RetainedQuitReason just like the real WorkerTransport, and asserts through downcast_ref. worker.rs's startup_failure_is_retained_in_quit_status now asserts downcast_ref through a real WorkerTransport, so ask 1 cannot regress silently.

2. StartupShared nesting — one accessor pair covers both wrappers. downcast_ref returns &StreamableHttpError, and the existing startup_error() unwraps StartupShared to the original variant (a no-op for every other variant, so callers apply it unconditionally on either path):

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 Changed entry. The discover-startup integration test now asserts this downcast_ref + startup_error() path on a real streamable transport.

3. CHANGELOG — added Added / Fixed / Changed entries under [Unreleased] covering the full new public surface (QuitReason::Transport, WorkerQuitReason::StartupFailed, StreamableHttpError::StartupShared, Transport::{quit_status, supports_quit_status, is_closed_error}, RetainedQuitReason, DynamicTransportError::downcast_ref) and the StartupShared nesting.

4. supports_quit_status doc — the trait doc now explains why it is separate from quit_status().is_some(): a None status is ambiguous (worker still running vs. no worker at all), and recovery only wants to await close() on worker-backed transports — closing a child-process or custom transport could block indefinitely with no status ever appearing.

Verified: 795 all-feature lib tests, 6 discover-startup + 10 HTTP-status-metadata integration tests, strict all-target clippy and rustfmt all clean.

@ip2a
ip2a force-pushed the fix/1441-terminal-cause branch from 2524611 to 4405844 Compare November 14, 2026 10:32
@ip2a

ip2a commented Nov 14, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest main (v3.10.0, c644b75a) — the branch had gone dirty after upstream landed #1246 / #1424 / #1390 and the 3.10.0 release.

The rebase keeps the fix intact on the new API:

  • Startup POST / initialize-response failures still produce StartupFailed with the shared typed error. Upstream main is still the old fatal + TransportChannelClosed code here, so the fix is carried over unchanged.
  • The notifications/initialized failure handling is reapplied on top of the renamed post_message_with_response_limits / response_limits() API, and composes with fix(transport): accept any 2xx reply for notification-only POSTs #1424's expect_accepted_or_json (any-2xx) handling — a failure still delivers StartupShared to the waiter and retains StartupFailed.
  • Upstream's max_sse_event_size → response_limits refactor, the new limit configuration, and the new tests are adopted wholesale.

Head is now a single commit 4405844d on top of c644b75a; mergeable is back to true (no more dirty).

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 jokemanfire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the Arc sharing 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, use fix!: plus a BREAKING CHANGE: footer, adapt the four oversized_discover_* cases to startup_error(), and call it out in the release notes. VERSIONING.md counts "change behavior in a way callers could not reasonably anticipate" as breaking, so option (b) cannot ship under a plain fix: 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_ref exists, looks through RetainedQuitReason, the fake transport now boxes the same wrapper, and startup_failure_is_retained_in_quit_status asserts through a real WorkerTransport. Good; the identity confusion I hit is gone.
  • Ask 3 — I asked for a CHANGELOG.md entry. Withdrawn: this repo has no CHANGELOG.md and 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"), not quit_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().
@ip2a
ip2a force-pushed the fix/1441-terminal-cause branch from 4405844 to 78f77c1 Compare November 14, 2026 13:49
@github-actions github-actions Bot removed the T-documentation Documentation improvements label Nov 14, 2026
@ip2a

ip2a commented Nov 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the three red gates at the latest head (78f77c1), going with option (b).

Why (b) over (a): option (a) keeps the ack variant bare, but since the typed error is not Clone, the retained quit reason could only hold it when the ack was never received. On the ack-delivered path — the common case — StartupFailed would have to store None, and the worker's warn log would fall back to startup failed with no typed cause again. That reintroduces exactly the logging gap this PR sets out to remove (the 500+ masked ERROR lines), plus the single-shot race. Keeping the Arc sharing on both paths (b) preserves the typed error in the ack, the retained reason, and the log alike.

Gates:

  1. Run Tests (no local feature) — test_streamable_http_limit_lifecycle.rs's oversized_discover_* assertion now goes through .map(StreamableHttpError::startup_error) before matching ResponseBodyTooLarge. The other two downcast_ref::<StreamableHttpError> assertions — concurrency's SessionRecoveryTimeout and initialize_error's ReinitializationFailed — are runtime, not startup, errors, so they are never wrapped in StartupShared and are left unchanged. All 10 limit-lifecycle cases pass locally, including the 4 that were red.
  2. Public API Check — the commit is now fix(transport)!: with a BREAKING CHANGE: footer documenting the StartupShared nesting and the startup_error() / downcast_ref recovery, so the changed derived impls are intentional under a major bump.
  3. Generate Documentation — fixed the two intra-doc links in worker.rs (std::error::Error::source, and dropped the redundant explicit target on Transport::quit_status). cargo doc is clean under -D warnings.

CHANGELOG: reverted the hand edit — this repo's changelog is release-plz-generated from commits, so the BREAKING CHANGE: footer is the right channel. Thanks for the correction.

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, cargo doc -D warnings, and rustfmt all clean.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-service Service layer changes T-test Testing related changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

streamable-http client: startup failures are reported as WorkerQuitReason::Fatal with the real error replaced by a synthetic "Transport channel closed"

2 participants