fix(client): abort legacy SSE reconnect chain when the originating request times out - #2616
fix(client): abort legacy SSE reconnect chain when the originating request times out#2616claude[bot] wants to merge 17 commits into
Code review found 3 potential issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 1 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | packages/client/src/client/streamableHttp.ts:1129-1142 |
Two surviving onerror-discipline sites in _send/_scheduleReconnection after the catch-shape migration |
| 🟡 Nit | packages/client/src/client/streamableHttp.ts:957-982 |
resumptionToken send: genuine resume-GET open failure fires neither onRequestStreamEnd nor a send() rejection |
| 🟣 Pre-existing | packages/core-internal/src/shared/protocol.ts:1596-1613 |
Legacy-era maxTotalTimeout settlement tears down the receive path but never POSTs notifications/cancelled |
Annotations
Check warning on line 1142 in packages/client/src/client/streamableHttp.ts
claude / Claude Code Review
Two surviving onerror-discipline sites in _send/_scheduleReconnection after the catch-shape migration
Two sibling sites of the onerror-discipline migration this PR performs survive in this file, both pre-dating the PR: (1) the main POST path's outer catch (~line 1194) guards onerror with only `options?.requestSignal?.aborted !== true` while its comment claims parity with `_handleSseStream`'s `isIntentionalAbort` — the `this._abortController?.signal.aborted` half is missing, so `transport.close()` landing mid-POST (deterministically for notification sends, which carry no requestSignal) surfaces a
Check warning on line 982 in packages/client/src/client/streamableHttp.ts
claude / Claude Code Review
resumptionToken send: genuine resume-GET open failure fires neither onRequestStreamEnd nor a send() rejection
The new comment on the resumptionToken short-circuit claims the resumed request "still fires the stream-end callback on a terminal non-resumable outcome", but that only holds for a 405 and a null body: a genuine failure opening the resume GET (fetch rejection, non-405/non-auth HTTP status, auth failure) lands in `_startOrAuthSse`'s catch, which rethrows into the new swallowing `.catch(() => {})` — `onRequestStreamEnd` never fires, and since `send()` on this path already resolved (fire-and-forget
Check notice on line 1613 in packages/core-internal/src/shared/protocol.ts
claude / Claude Code Review
Legacy-era maxTotalTimeout settlement tears down the receive path but never POSTs notifications/cancelled
Pre-existing issue (not introduced by this PR): a maxTotalTimeout settlement never routes through cancel() — _onprogress settles via responseHandler(error) directly — so on a legacy (2025-11-25) per-request-stream session no notifications/cancelled is ever POSTed for this settlement, even though plain-timeout and caller-abort settlements on the same session do POST, and this PR gave the modern era its wire cancel on this exact path (the .finally() stream-close abort). Since the PR already modifi