Skip to content

fix: preserve SSE resume cache and correct event replay - #1165

Open
freedom-winds wants to merge 3 commits into
modelcontextprotocol:mainfrom
freedom-winds:pr/09-rust-sdk-session-resume-and-cache-20260811200316-01
Open

fix: preserve SSE resume cache and correct event replay#1165
freedom-winds wants to merge 3 commits into
modelcontextprotocol:mainfrom
freedom-winds:pr/09-rust-sdk-session-resume-and-cache-20260811200316-01

Conversation

@freedom-winds

Copy link
Copy Markdown

Summary

Fix two issues in the Streamable HTTP server's SSE resume behavior:

  • The event identified by Last-Event-ID was replayed again.
  • Request-scoped SSE caches were deleted immediately after the final response, preventing clients from recovering responses after a disconnect.

Changes

  • Adjust the SSE resume offset to replay only events after Last-Event-ID.
  • Reject event IDs older than the retained cache window.
  • Mark completed request channels as complete while retaining their cached events.
  • Allow completed_cache_ttl to evict completed request caches.
  • Add regression coverage for retaining completed request channel state.

@freedom-winds
freedom-winds requested a review from a team as a code owner August 11, 2026 12:03
@github-actions github-actions Bot added T-core Core library changes T-transport Transport layer changes labels Aug 11, 2026
Comment thread crates/rmcp/src/transport/streamable_http_server/session/local.rs
Comment thread crates/rmcp/src/transport/streamable_http_server/session/local.rs Outdated
@github-actions github-actions Bot added the T-test Testing related changes label Aug 12, 2026
let sync_index = index.saturating_sub(front_event_id.index);
// Last-Event-ID is the last event the client received. The resume is
// valid as long as the next event has not already fallen out of cache.
let next_index = index.saturating_add(1);

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.

A standalone GET emits a synthetic priming event with ID 0. But CachedTx::new_common also assigns 0 to the first cached common event. How should a client that only received the priming event resume without sync(0) skipping that unseen notification?

Comment on lines +1421 to +1422
assert!(worker.tx_router[&http_request_id].completed_at.is_some());
assert!(worker.tx_router[&http_request_id].resources.is_empty());

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.

Could this regression exercise resume and assert that the cached final response is delivered?

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-test Testing related changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants