Fix SSE keep-alive timer lifecycle in Streamable HTTP server transport (v1.x) - #2547
Merged
Merged
Conversation
Keep-alive timers were tracked in a transport-level map keyed by stream id. The standalone GET stream and its resumed successors share one id, so a stale connection's cancel callback could stop the resumed stream's keep-alive and delete its stream mapping, and the interval's error handler could clear the wrong timer. Timers are now owned by the stream they were armed for: startKeepAlive returns the handle, the stream's own cancel/cleanup clears it, and the interval clears itself on write failure. Cancel callbacks only delete the stream mapping when it still points at their own stream. A resume closes the superseded stream cleanly, and a resume that completes after the transport closed (or after its stream was cancelled) returns 404 instead of registering a stream nothing can clean up. The POST path now arms keep-alive after its fallible awaits and releases the stream and request correlations if they fail. Non-finite keepAliveMs values disable keep-alive, and values above 2^31-1 are clamped instead of firing every millisecond.
🦋 Changeset detectedLatest commit: 0f983d0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Hmm yeh this is better :) thanks!! |
mattzcarey
approved these changes
Jul 24, 2026
A response completing while its SSE stream was being resumed could be written to the evicted stream (lost) or delivered twice when the event store made it replay-visible before the write resolved. A stream that disconnected before its response completed also left the response unstored, so a polling client could never retrieve it, and the request correlation maps leaked. send() now stores response events even when the request stream is disconnected (matching the standalone stream path), re-reads the stream registration after the store write, skips events the resumed stream's replay already delivered, and releases request correlations once the response is safely replayable. Without an event store, or in JSON response mode where a replay can never settle the pending response, completing against a missing stream still surfaces an error. Also guards session initialization and stream registration against a transport that closed while the request body or the session initialization callback was pending.
felixweinberger
marked this pull request as draft
July 24, 2026 16:57
Extract keep-alive validation into a shared sseKeepAlive helper matching main: non-finite and sub-millisecond intervals disable keep-alive, and oversized delays are clamped instead of firing every millisecond. Also ported from the main-branch implementation: handleRequest returns 404 once the transport is closed instead of registering streams nothing can clean up, SSE responses send X-Accel-Buffering: no (and the POST stream gains no-transform) so proxy buffering does not swallow frames, and a DELETE request closes the transport even when the onsessionclosed callback throws. send() now also releases request correlations before reporting an undeliverable response, so the entries don't outlive it.
send() released a completed response's correlations to Last-Event-ID replay whenever an event store was configured, but a client can only resume a stream if it actually holds a cursor: clients that never received an id-bearing event (for example pre-2025-11-25 clients whose stream carried no notifications) can never issue the resume, so their lost responses were released silently where they previously surfaced through onerror. Request streams are now marked resumable when an id-bearing event is delivered on them - the priming event, a stored notification, or a replay - and the silent hand-off applies only to those streams. A response completing while the transport closes is a no-op instead of a spurious error, failed POST registrations release their tracking entry, and the changeset now also describes the send() delivery-model changes.
This was referenced Jul 27, 2026
There was a problem hiding this comment.
Beyond the inline findings, two candidates were examined and ruled out this pass: the sseKeepAlive.ts camelCase filename (matches the existing convention of its siblings, e.g. webStandardStreamableHttp.ts, despite CLAUDE.md's hyphens rule), and the resume supersede path applying to a half-open predecessor — when the event store implements getStreamIdForEventId, the 409 conflict check runs first, so the supersede-without-conflict-check shape only exists for stores without that optional method.
Extended reasoning...
This run's inline comments (two nits, one pre-existing InMemoryEventStore issue) are posted separately; this note only records what else was examined and refuted so a later pass doesn't re-explore it from scratch. No prior run left a ruled-out note on this PR, so this is the first and only such record. It is informational, not a correctness guarantee.
The closed-transport no-op in send() ran after the response was recorded in _requestResponseMap, so a send parked on the event-store write when close() swept the maps re-populated them and stranded the response payload and its correlations on the closed transport. Release them before returning.
Repeated close() re-ran the stream sweep and re-fired onclose. The main branch guards close() re-entry; this restores the same guard, which the DELETE handler's unconditional close in its finally block now relies on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2538. Keep-alive timers are now owned by the stream they were armed for instead of being tracked in a transport-level map keyed by stream id:
startKeepAlivereturns the handle, the stream's own cancel/cleanup clears it, and the interval clears itself on write failure. The shared timer map,stopKeepAlive, and theclose()timer sweep are removed. Cancel callbacks only delete the stream mapping when it still points at their own stream (matching main), a resume closes the superseded stream instead of leaving it hanging, the POST path arms after its fallible awaits, and non-finitekeepAliveMsvalues disable keep-alive (values above 2^31-1 are clamped).Motivation and Context
The standalone GET stream and its resumed successors share one stream id, so with the shared timer map a few ordinary disconnect/reconnect orderings tear down the wrong timer:
await writePrimingEvent(...). IfeventStore.storeEventrejects, the handler returns 400 and the Response is discarded, so nothing can ever cancel the stream; the timer keeps firing untilclose().close()during areplayEventsAfterawait doesn't stop the continuation from arming a timer afterwards, which nothing can clear.keepAliveMs: NaN/Infinity/ values above 2^31-1 pass the<= 0guard, andsetIntervalclamps such delays to ~1ms — flooding every SSE stream with comment frames.Number(process.env.SOME_UNSET_VAR)is an easy way to hit this.How Has This Been Tested?
Seven new regression tests (fake timers), each failing on current v1.x before the fix. Full test suite passes. Also verified against a real
http.Serverover raw sockets: after the stale connection drops, the resumed stream keeps receiving keep-alive frames and server notifications, andkeepAliveMs: NaNwrites nothing.Breaking Changes
None.
Types of changes
Checklist
Update (second commit): the review comments on the first commit pointed at the same stale-capture class inside
send(). Rather than spot-guards, the second commit givessend()a consistent model: the event store is the source of truth, live streams are best-effort delivery.Last-Event-ID.onsessioninitializedcallback was pending.Seven more regression tests, each pinned red-green.