Uh oh!
There was an error while loading. Please reload this page.
Add SSE response mode to the 2026 streamable-HTTP server entry - #3001
Conversation
Uh oh!
There was an error while loading. Please reload this page.
The modern (2026-07-28) single-exchange entry could only respond with application/json, so a handler's ctx.report_progress() / ctx.log() was a no-op on that path. This adds the text/event-stream response mode: the handler runs as a sibling task inside an EventSourceResponse via data_sender_callable=, request-scoped notifications stream as SSE events before the terminal JSON-RPC response, and client disconnect cancels the handler (stream-close = cancellation per the transport spec). The existing json_response: bool on the session manager governs the modern path too — JSON mode keeps today's drop-notifications behaviour. Accept validation lands at the same time (mode-aware, matching the legacy entry; check_accept_headers hoisted to module level so both entries share it). progress_token_from_params() extracted to shared/ with the bool-guard. Burns the tools-call-with-progress conformance waiver, the _MODERN_NOTIFY_DROP KnownFailure rows, and the streaming story's http-asgi:modern xfail.
7382d8e to
ea346dfCompareThere was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The previous commit committed text/event-stream headers immediately, so a METHOD_NOT_FOUND raised inside serve_one reached the wire as HTTP 200 + an SSE event — breaking the spec MUST that an unimplemented method respond 404. server-stateless conformance failed 7/25 on that. The handler now runs as a sibling task while the parent waits for the first event from the rendezvous stream. If the handler completes (or raises) without emitting, the result is written as application/json with the ERROR_CODE_HTTP_STATUS-mapped status — same as JSON mode. text/event-stream headers go out only once a notification has actually arrived; from there a disconnect-watcher in the same task group cancels serve_one on http.disconnect. EventSourceResponse is dropped for this path (it sends headers on __call__ and its task group can't adopt an already-running handler); SSE frames are written directly via ASGI send. Also: drop the _check_accept_headers wrapper method (review feedback).
Uh oh!
There was an error while loading. Please reload this page.
The 5820a22 raw-ASGI rewrite dropped the comment-line keepalive that EventSourceResponse used to provide, so a long-silent handler behind a proxy idle-read timeout would have its stream closed (which on this path cancels the handler). The receive loop now emits ': ping' on a 15s move_on_after between events, matching the legacy paths' interval.
Uh oh!
There was an error while loading. Please reload this page.
A handler that runs silent past _SSE_PING_INTERVAL now commits text/event-stream and starts pinging instead of leaving zero bytes on the wire — so a proxy idle-read timeout can't close the connection (which on this path cancels the handler) before the result arrives. Kernel-dispatch errors still get the table-mapped JSON status because they resolve well within the 15s window.
The two keepalive tests now run on trio's autojumping MockClock instead of a real 50ms anyio.sleep against a 10ms patched interval. The simulated 2.5s-silent handler takes no wall-clock time, the ping count is asserted exactly (== 2) instead of by presence, and there is no scheduler-timing window. Same pattern as test_jsonrpc_dispatcher.py and test_timeouts.py.
There was a problem hiding this comment.
I didn't find any new issues in the latest revision — the keepalive and Cache-Control concerns from my earlier inline comments look properly addressed (bounded first-event wait, ping loop, no-transform header) and are now covered by tests — but the hand-rolled ASGI SSE write path with deferred-start, disconnect-driven cancellation, and status-mapping semantics is complex enough that it warrants a human reviewer's sign-off.
Extended reasoning...
Overview
This PR adds an SSE response mode to the 2026-07-28 single-exchange streamable-HTTP server entry (_streamable_http_modern.py), so request-scoped progress and log notifications stream before the terminal response. It threads json_response through the manager, hoists check_accept_headers to module level in streamable_http.py, extracts progress_token_from_params into shared/jsonrpc_dispatcher.py, burns several conformance/interaction xfails, and adds a substantial unit-test suite for the new path (including trio MockClock keepalive timing tests and a disconnect-cancellation test).
Security risks
Low. The Accept-header validation is a byte-for-byte hoist of the existing parser (the q-value gap noted by another bot is pre-existing, not introduced here). No auth, crypto, or permission logic is touched; the new code only changes how a server's own response is framed on the wire. The notify path serializes JSON-RPC notifications it already constructs, so there is no new injection surface.
Level of scrutiny
High. This is production transport code on the default (json_response=False) path for the modern protocol revision, and the SSE branch is hand-rolled ASGI (deferred response start, a rendezvous memory stream, a sibling-task handler, disconnect-driven cancellation, and move_on_after-based keepalive). Earlier review iterations on this very PR surfaced two real robustness regressions (missing keepalive, unbounded pre-commit silence) that were only caught by careful reasoning about proxy idle timeouts — exactly the kind of subtle concurrency/deployment behavior that deserves a human maintainer's judgment on the final shape, including the deferred-start design tradeoff itself.
Other factors
All prior reviewer feedback (maxisbey's wrapper-removal comment and my inline findings on Cache-Control parity, the missing keepalive ping, and the unbounded first-event wait) has been addressed in follow-up commits with accompanying tests, and the current diff reflects those fixes. Test coverage of the new path is good: ordering, error-before/after-commit, 406 mode-aware Accept handling, wildcard Accept, late-notify drop, silent-handler commit, and disconnect cancellation are all exercised. I found no remaining bugs, but the size and criticality of the change put it outside what I'm willing to shadow-approve.
The modern (2026-07-28) single-exchange entry could only respond with
application/json, so a handler'sctx.report_progress()/ request-scopednotifications/messagewas a no-op on that path. This adds thetext/event-streamresponse mode so those notifications stream before the terminal response.Part of #2893; tracked under #2891.
Motivation and Context
The transport spec says a server MAY respond to a POST with either
application/jsonortext/event-stream; the SSE stream carriesnotifications/progress/notifications/messagerelated to the originating request, then the terminal response, then closes. Stream-close is the cancellation signal. The legacy stack already does both modes viajson_response: bool; this is the modern entry catching up.How Has This Been Tested?
tests/server/test_streamable_http_modern.py: SSE streams progress + log in order before the result; JSON mode drops; handler error is a 200 +JSONRPCErrorevent; no-token → progress no-op; mode-aware 406 on Accept;*/*wildcard; late notify after stream-close drops; client disconnect cancels the handler andconnection.exit_stackstill unwinds.[streamable-http-2026-07-28]legs of the interaction-suite progress/logging requirements now pass.streamingexample story'shttp-asgi:modernleg now passes.tools-call-with-progressserver scenario fromexpected-failures.2026-07-28.yml.Breaking Changes
None for users — the manager's existing
json_responseflag governs the modern path too (defaultFalse= SSE, same as legacy). The modern entry now validatesAccept(mode-aware: JSON mode requiresapplication/json; SSE mode requires both), so a client that omittedAcceptentirely now gets 406 — but the spec already requires clients to send both.Types of changes
Checklist
Additional context
Shape:
_SingleExchangeDispatchContextgainsprogress_token+sink: MemoryObjectSendStream | None.notify()writes an SSE-framed event to the sink (catchingClosedResourceError/BrokenResourceError→ debug-log and drop, mirroring_JSONRPCDispatchContext); JSON mode hassink=Noneso it returns.progress()reads the token and callsnotify().send_raw_requeststill raisesNoBackChannelError— SSE is notify-only; sampling/elicitation/list-roots stay on the MRTR path.SSE write path — deferred start: the handler runs as a sibling task while the parent waits for the first event from a rendezvous (buffer=0) memory stream. If the handler completes (or raises) without emitting, the result is written as
application/jsonwith theERROR_CODE_HTTP_STATUS-mapped status — so the spec's404/400MUSTs hold for kernel-dispatch errors (METHOD_NOT_FOUND, missing-capability) even withjson_response=False.text/event-streamheaders go out only once a notification has actually arrived; from there awatch_disconnecttask in the same group cancelsserve_oneonhttp.disconnect(stream-close = cancellation), and amove_on_after(15)around the receive loop emits: pingcomment-line keepalives between events, matching the legacy paths' interval. SSE frames are written directly via ASGIsendrather thanEventSourceResponsebecause ESR sends headers on__call__and its internal task group can't adopt an already-running handler.async with send_ch:guarantees the stream closes on every exit path.Shared helpers:
check_accept_headers()hoisted to module level instreamable_http.py(the legacy method body never readself);progress_token_from_params()extracted toshared/jsonrpc_dispatcher.pywith the existing bool-guard. Both entries / both dispatch contexts call them.Coverage notes: the
streamingstory's modern-HTTP cancellation leg now exercises two pre-existing client-sideexcept Exceptionblocks inclient/streamable_http.py(forwarding an SSE event to aread_stream_writerthe cancel already closed). Promoted fromno cover→lax no coverwith an in-file comment; thelogger.exception("Error parsing SSE message")there is misleading (it'sBrokenResourceError, not a parse failure) — worth a follow-up but out of scope here.AI Disclaimer