Fix SSE keep-alive timer lifecycle in Streamable HTTP server transport (v1.x) - #2547
Code review found 7 potential issues
Found 3 candidates, confirmed 7. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 6 |
| 🟣 Pre-existing | 1 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/server/webStandardStreamableHttp.ts:425-431 |
Priming/resumability gate uses unvalidated lexicographic version comparison; port drops main supportsEmptySSEData member |
| 🟡 Nit | src/server/webStandardStreamableHttp.ts:981-986 |
Concurrent DELETEs double-fire onsessionclosed: no in-flight guard around the awaited callback |
| 🟣 Pre-existing | src/server/webStandardStreamableHttp.ts:1130-1138 |
Pre-existing: fixed _GET_stream id collides across sessions in a shared EventStore, replaying other sessions standalone |
Annotations
Check warning on line 431 in src/server/webStandardStreamableHttp.ts
claude / Claude Code Review
Priming/resumability gate uses unvalidated lexicographic version comparison; port drops main supportsEmptySSEData membership guard
The priming/resumability gate uses a bare lexicographic comparison (protocolVersion >= '2025-11-25') on a version string that, for initialize requests, comes verbatim from the request body and is never validated against SUPPORTED_PROTOCOL_VERSIONS — so any unsupported string sorting above the threshold ('draft', 'latest', '2026-01-01') enables priming events, closeSSEStream callbacks, and the new _resumableStreams silent-release path for a client the server will actually negotiate down. Main gua
Check warning on line 986 in src/server/webStandardStreamableHttp.ts
claude / Claude Code Review
Concurrent DELETEs double-fire onsessionclosed: no in-flight guard around the awaited callback
Concurrent DELETE requests can invoke `onsessionclosed` twice for the same session: while the first DELETE is parked on `await Promise.resolve(this._onsessionclosed?.(...))` (line 982), `_closed` is still false (it's only set inside `close()`, which runs in the `finally` after the callback settles), so a second DELETE passes both the `handleRequest` `_closed` guard and `validateSession` and fires the callback again. Note this window also exists pre-PR and on main, so nothing regresses at merge —
Check notice on line 1138 in src/server/webStandardStreamableHttp.ts
claude / Claude Code Review
Pre-existing: fixed _GET_stream id collides across sessions in a shared EventStore, replaying other sessions standalone messages on resume
Pre-existing issue (not introduced by this PR): every transport instance stores standalone server-initiated messages under the same fixed stream id `'_GET_stream'`, and the `EventStore` contract has no session scoping — so when one store instance is shared across sessions (as the SDK's own `ssePollingExample` does with its module-level `InMemoryEventStore`, and as any multi-node Redis/DB store would), a session-A standalone resume via `replayEventsAfter` replays every subsequent `'_GET_stream'`