refactor(transport): SSOT phase 3 — clean reconnect - #440
Conversation
handleReconnect no longer does reattach/rekey/zombie cleanup — that's all handled by handleSendV2 on the first user message. Reconnect now only does: watch + cursor replay + suspend resume + boot context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Periodic sync (5s timer retrying missed events) is redundant now that reconnect replays via EventStore cursor on welcome. Removes setEventStore, startPeriodicSync, stopPeriodicSync, EventStoreAdapter interface, and all associated tests and wiring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reconnect POST no longer defers _connected — the client marks connected immediately on welcome. handleSendV2 handles ownership on first message, and replayed events arrive via SSE regardless of POST outcome. Removes doReconnectPost, scheduleReconnect, reconnectTimer, and reconnectDelayMs. Replaces 12 deferred/stale/failure tests with 4 fire-and-forget tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reconnect is REST-only — keep the schema export for the REST handler but exclude it from IncomingWsMessageV2. The WS dispatcher already ignores it with a comment explaining why. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Stale session test now asserts remove is NOT called (deferred to handleSendV2). Suspend resume test clears reattachChat mock to avoid bleed from prior tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing setSessionState mock to routes and suspend-routes tests. Remove reconnect from WS union test since P3 moved it to REST-only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4e04c2f to
fe40257
Compare
P3 removed ReconnectMessage from the WS union (reconnect is now REST-only), but the switch case was left behind causing a type error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (1 critical) (3 warning).
packages/protocol/src/ws-schemas-v2.ts
Sound simplification overall, but removing ReconnectMessage from the WS union breaks the WS transport fallback (MitzoConnection still sends reconnect over WS). The fire-and-forget design also introduces a subtle cursor race between send and reconnect POST arrival order.
- 🔴 regressions (L129): ReconnectMessage removed from WS union breaks the WS transport fallback.
MitzoConnection(packages/client/src/connection.ts:228) still sends{ type: 'reconnect' }over WS on reconnect. Since it's no longer in the discriminated union, the server'sdispatchV2MessageZod parse rejects it and the message is silently dropped. This means WS clients get no event replay, no cursor reset, and no boot_context re-send on reconnect. Either updateMitzoConnectionto send reconnect via REST POST (likeSseConnectiondoes), or keepReconnectMessagein the WS union with a no-op handler.[fixable]
server/ws-handler-v2.ts
Sound simplification overall, but removing ReconnectMessage from the WS union breaks the WS transport fallback (MitzoConnection still sends reconnect over WS). The fire-and-forget design also introduces a subtle cursor race between send and reconnect POST arrival order.
- 🔵 style (L915): Dead code:
case 'reconnect':can never be reached becausereconnectwas removed from theIncomingWsMessageV2union. The Zod parse on line 900 rejects reconnect messages before this switch. Remove this case to avoid confusion.[fixable]
packages/client/src/sse-connection.ts
Sound simplification overall, but removing ReconnectMessage from the WS union breaks the WS transport fallback (MitzoConnection still sends reconnect over WS). The fire-and-forget design also introduces a subtle cursor race between send and reconnect POST arrival order.
- 🟡 unsafe_assumptions (L211): Fire-and-forget reconnect POST means sends can arrive at the server before handleReconnect runs. handleSendV2 calls
watch()(line 541 of ws-handler-v2.ts) but does NOT callresetCursor(), so the cursor starts at 0 for that connection+session. If a broadcast fires during the race window (between send and reconnect POST processing), the cursor advances. When handleReconnect then callsresetCursor(lastSeq), it could roll the cursor backward, potentially causing duplicate event delivery on the next reconnect. Consider adding aresetCursorcall in the handleSendV2 takeover/reattach path, or documenting why this is acceptable.[fixable]
packages/client/src/__tests__/sse-connection.test.ts
Sound simplification overall, but removing ReconnectMessage from the WS union breaks the WS transport fallback (MitzoConnection still sends reconnect over WS). The fire-and-forget design also introduces a subtle cursor race between send and reconnect POST arrival order.
- 🟡 missing_tests: The test for 'sends reconnect POST fire-and-forget on reconnect welcome' verifies the POST is sent, but there's no test for the case where a send is queued AND the reconnect POST races with it. Specifically: queue a send during reconnect, receive welcome, verify both reconnect and send are dispatched without ordering guarantees (the new 'flushes pending sends immediately' test covers this but doesn't test that the server can handle either arriving first).
packages/client/__tests__/connection.test.ts
Sound simplification overall, but removing ReconnectMessage from the WS union breaks the WS transport fallback (MitzoConnection still sends reconnect over WS). The fire-and-forget design also introduces a subtle cursor race between send and reconnect POST arrival order.
- 🟡 regressions: The
connection.test.tstests at line 197 ('reconnects after close and sends hello + reconnect with tracked sessions') still assert that reconnect is sent over WS and expect it to work. These tests pass because they only check the client-side behavior (message is sent), but they're now testing a path that's broken server-side (reconnect over WS is silently dropped). These tests give false confidence.[fixable]
Reviewed at fe40257
|
|
||
| // ReconnectMessage is handled via REST POST (not WS) — exported for | ||
| // the REST handler but excluded from the WS union. | ||
| export const IncomingWsMessageV2 = z.discriminatedUnion('type', [ |
There was a problem hiding this comment.
🔴 regressions: ReconnectMessage removed from WS union breaks the WS transport fallback. MitzoConnection (packages/client/src/connection.ts:228) still sends { type: 'reconnect' } over WS on reconnect. Since it's no longer in the discriminated union, the server's dispatchV2Message Zod parse rejects it and the message is silently dropped. This means WS clients get no event replay, no cursor reset, and no boot_context re-send on reconnect. Either update MitzoConnection to send reconnect via REST POST (like SseConnection does), or keep ReconnectMessage in the WS union with a no-op handler. [fixable]
| case 'hello': | ||
| // Already handled at routing layer, ignore duplicate | ||
| break; | ||
| case 'reconnect': |
There was a problem hiding this comment.
🔵 style: Dead code: case 'reconnect': can never be reached because reconnect was removed from the IncomingWsMessageV2 union. The Zod parse on line 900 rejects reconnect messages before this switch. Remove this case to avoid confusion. [fixable]
| this._connected = true; | ||
| this.flushPendingSends(); | ||
| this.listener?.({ type: '_open' }); | ||
| this.doPost('reconnect', { |
There was a problem hiding this comment.
🟡 unsafe_assumptions: Fire-and-forget reconnect POST means sends can arrive at the server before handleReconnect runs. handleSendV2 calls watch() (line 541 of ws-handler-v2.ts) but does NOT call resetCursor(), so the cursor starts at 0 for that connection+session. If a broadcast fires during the race window (between send and reconnect POST processing), the cursor advances. When handleReconnect then calls resetCursor(lastSeq), it could roll the cursor backward, potentially causing duplicate event delivery on the next reconnect. Consider adding a resetCursor call in the handleSendV2 takeover/reattach path, or documenting why this is acceptable. [fixable]
… crashes Without this try/catch, an unhandled error from the transport layer kills the server, losing all in-memory state and triggering replay storms on client reconnect. Cherry-picked from #396 (now closed as superseded by P3). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Rebased onto main, fixed CI (dead reconnect case in WS handler switch, missing setSessionState mocks in tests, removed reconnect from protocol union test). Cherry-picked interrupt guard from #396. Ready for review. |
Centaur ReviewFound 6 issue(s) (2 warning).
|
…nt cursor race - Restore ReconnectMessage in WS union and handler (WS clients still send reconnect over WS until P4 removes WS transport) - Document why cursor race between fire-and-forget reconnect POST and handleSendV2 is benign (single-threaded + client seq dedup) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All three findings addressed: restored WS reconnect in union + handler, documented cursor race as benign (single-threaded + client seq dedup), cherry-picked interrupt guard from #396. Please re-review. |
Centaur ReviewFound 6 issue(s) (3 warning).
|
- Add warn log for failed POSTs (was silent catch) - Retry reconnect POST on next EventSource reconnect if it fails - Flush pending sends AFTER reconnect POST (ordering guarantee) - Reattach detached sessions on reconnect (was deferred to send) - Fix comment to lead with client-side seq dedup, not event loop - Add test for SSE event delivery after reconnect POST failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 7 issue(s) (1 critical) (5 warning).
packages/client/src/sse-connection.ts
Critical bug: doPost doesn't check res.ok, so HTTP 500 from the reconnect endpoint silently clears the retry state — a regression from the old doReconnectPost which explicitly handled non-ok responses. Secondary issues: _pendingReconnectSessions is not cleared on disconnect() and never reconciled with seqBySession, and the new retry mechanism has zero test coverage.
- 🔴 bugs (L274):
doPostdoes not checkres.ok.fetch()only rejects on network errors — an HTTP 500 resolves the promise successfully. The reconnect POST.then()handler (line 224) clears_pendingReconnectSessionson resolve, so a server 500 is treated as success: cursor is never reset, replay never happens, and the retry mechanism is silently disarmed. The olddoReconnectPostexplicitly checkedres.okand calledscheduleReconnect()on non-ok responses. Fix: checkres.okindoPostand throw on non-ok, or check it at the reconnect call site.[fixable] - 🟡 bugs (L60):
disconnect()does not clear_pendingReconnectSessions. If a reconnect POST failed (setting_pendingReconnectSessions), then the user disconnects and later reconnects, the stale sessions array will be retried on the next welcome — even if those sessions no longer exist locally (cleared viaclearSession). Addthis._pendingReconnectSessions = null;indisconnect().[fixable] - 🟡 unsafe_assumptions (L212):
_pendingReconnectSessionsis used as a retry payload but is never reconciled withseqBySession. If a session is cleared (clearSession) while_pendingReconnectSessionsholds a reference to it, the next welcome retries a reconnect POST for a session the client no longer tracks. The stale entry will cause unnecessary server work and log noise. Consider filtering_pendingReconnectSessionsagainst currentseqBySessionkeys before retrying.[fixable] - 🔵 style (L86): Bare
.catch(() => {})on fourdoPostcalls silently swallows errors. Now thatdoPostlogs aconsole.warnbefore throwing, the catch is functionally correct, but the pattern is fragile — a future caller might assumedoPostdoesn't throw. Consider havingdoPostnot throw (log-only) for fire-and-forget calls, and a separatedoPostOrThrowfor the reconnect path that needs error discrimination.
packages/client/src/__tests__/sse-connection.test.ts
Critical bug: doPost doesn't check res.ok, so HTTP 500 from the reconnect endpoint silently clears the retry state — a regression from the old doReconnectPost which explicitly handled non-ok responses. Secondary issues: _pendingReconnectSessions is not cleared on disconnect() and never reconciled with seqBySession, and the new retry mechanism has zero test coverage.
- 🟡 missing_tests: No test covers the HTTP 500 (non-ok) response case for reconnect POST. The old suite had two tests for this (
stays disconnected when reconnect POST fails,recovers after failed reconnect when EventSource auto-reconnects). With the new fire-and-forget model, the behavior for non-ok responses is different from network errors — but no test verifies which path is taken. Given thedoPostbug above, a 500 silently clears_pendingReconnectSessions.[fixable] - 🟡 missing_tests: No test verifies the
_pendingReconnectSessionsretry-on-next-welcome mechanism — the key new behavior this PR introduces. A test should verify: (1) reconnect POST fails →_pendingReconnectSessionsretained, (2) EventSource auto-reconnects → next welcome retries with the saved sessions, (3) success clears_pendingReconnectSessions.[fixable]
server/ws-handler-v2.ts
Critical bug: doPost doesn't check res.ok, so HTTP 500 from the reconnect endpoint silently clears the retry state — a regression from the old doReconnectPost which explicitly handled non-ok responses. Secondary issues: _pendingReconnectSessions is not cleared on disconnect() and never reconciled with seqBySession, and the new retry mechanism has zero test coverage.
- 🟡 regressions (L259):
handleReconnectnow only reattaches sessions whereownerConnection === connectionId. When a user's device gets a new connectionId (e.g. app restart), the session's clientId still has the old connectionId prefix. In this scenario, reconnect won't reattach (owner mismatch), and the agent's transport stays stale until the user sends a message (triggeringhandleSendV2takeover). During this gap, the agent may try to write to a dead transport — permission prompts, for example, would be lost. The old code handled this by checkingownerGoneand reattaching eagerly.[fixable]
Reviewed at 13153d6
| }, this.config.reconnectDelayMs); | ||
| } | ||
|
|
||
| private async doPost(endpoint: string, body: Record<string, unknown>): Promise<void> { |
There was a problem hiding this comment.
🔴 bugs: doPost does not check res.ok. fetch() only rejects on network errors — an HTTP 500 resolves the promise successfully. The reconnect POST .then() handler (line 224) clears _pendingReconnectSessions on resolve, so a server 500 is treated as success: cursor is never reset, replay never happens, and the retry mechanism is silently disarmed. The old doReconnectPost explicitly checked res.ok and called scheduleReconnect() on non-ok responses. Fix: check res.ok in doPost and throw on non-ok, or check it at the reconnect call site. [fixable]
| @@ -60,10 +59,6 @@ export class SseConnection implements ChatConnection { | |||
|
|
|||
| disconnect(): void { | |||
There was a problem hiding this comment.
🟡 bugs: disconnect() does not clear _pendingReconnectSessions. If a reconnect POST failed (setting _pendingReconnectSessions), then the user disconnects and later reconnects, the stale sessions array will be retried on the next welcome — even if those sessions no longer exist locally (cleared via clearSession). Add this._pendingReconnectSessions = null; in disconnect(). [fixable]
| // Fire reconnect POST if reconnecting with sessions, or retry a | ||
| // previously failed reconnect. handleSendV2 handles ownership on first | ||
| // message, and replayed events arrive via SSE regardless. | ||
| const sessions = |
There was a problem hiding this comment.
🟡 unsafe_assumptions: _pendingReconnectSessions is used as a retry payload but is never reconciled with seqBySession. If a session is cleared (clearSession) while _pendingReconnectSessions holds a reference to it, the next welcome retries a reconnect POST for a session the client no longer tracks. The stale entry will cause unnecessary server work and log noise. Consider filtering _pendingReconnectSessions against current seqBySession keys before retrying. [fixable]
|
|
||
| if (this._connected && this._connectionId) { | ||
| this.doPost(endpoint, msg); | ||
| this.doPost(endpoint, msg).catch(() => {}); |
There was a problem hiding this comment.
🔵 style: Bare .catch(() => {}) on four doPost calls silently swallows errors. Now that doPost logs a console.warn before throwing, the catch is functionally correct, but the pattern is fragile — a future caller might assume doPost doesn't throw. Consider having doPost not throw (log-only) for fire-and-forget calls, and a separate doPostOrThrow for the reconnect path that needs error discrimination.
| newClientId, | ||
| }); | ||
| } | ||
| if (found && ctx.sessionRegistry.isActive(found.clientId)) { |
There was a problem hiding this comment.
🟡 regressions: handleReconnect now only reattaches sessions where ownerConnection === connectionId. When a user's device gets a new connectionId (e.g. app restart), the session's clientId still has the old connectionId prefix. In this scenario, reconnect won't reattach (owner mismatch), and the agent's transport stays stale until the user sends a message (triggering handleSendV2 takeover). During this gap, the agent may try to write to a dead transport — permission prompts, for example, would be lost. The old code handled this by checking ownerGone and reattaching eagerly. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (3 warning).
packages/client/src/sse-connection.ts
Sound simplification — fire-and-forget reconnect and periodic sync removal are architecturally justified. Main concern is _pendingReconnectSessions not being cleared in disconnect(), which can cause post-teardown side effects, plus missing test coverage for the new retry mechanism.
- 🟡 bugs (L60):
disconnect()does not clear_pendingReconnectSessions. Ifdisconnect()is called while a reconnect POST is in-flight, the.then()callback can still fire and callflushPendingSends()after the connection is torn down. Additionally, the stale sessions array persists in memory. Addthis._pendingReconnectSessions = null;todisconnect().[fixable] - 🟡 unsafe_assumptions (L223): The reconnect POST's
.then()callback clears_pendingReconnectSessionsand flushes sends without checking whether_connectionIdhas changed since the POST was initiated. If a new welcome arrives while the POST is in-flight (EventSource auto-reconnect), the new welcome captures_pendingReconnectSessions(line 213), starts a second POST, and then the stale.then()clears the flag. This is benign only if the server handles duplicate reconnect POSTs with different connection IDs idempotently — which it does (handleReconnect is stateless per call) — but the doubleflushPendingSends()could cause sends to fire twice. Consider capturing_connectionIdbefore the POST and comparing in the callback.[fixable]
server/ws-handler-v2.ts
Sound simplification — fire-and-forget reconnect and periodic sync removal are architecturally justified. Main concern is _pendingReconnectSessions not being cleared in disconnect(), which can cause post-teardown side effects, plus missing test coverage for the new retry mechanism.
- 🔵 style (L260): Uses
found.clientId.split(':')[0]while every other ownership check in this file (lines 519, 681, 819, 868) uses thegetOwnerConnection()utility defined at line 93. UsegetOwnerConnection(found.clientId)for consistency — they return the same result but the utility is the convention and would track any format changes.[fixable] - 🟡 regressions (L259): handleReconnect now only reattaches sessions owned by the reconnecting connection (
ownerConnection === connectionId). Previously, it also handled cross-connection reattach when the owner was gone. This is deferred to handleSendV2, but creates a window where a non-owner reconnecting client sees events via broadcast while the agent's transport remains stale. Passive observers (watch-only, never send) will never trigger the handleSendV2 ownership path. This is an intentional design choice (documented in comments), but worth noting as a behavioral change.
packages/client/src/__tests__/sse-connection.test.ts
Sound simplification — fire-and-forget reconnect and periodic sync removal are architecturally justified. Main concern is _pendingReconnectSessions not being cleared in disconnect(), which can cause post-teardown side effects, plus missing test coverage for the new retry mechanism.
- 🔵 missing_tests: No test verifies that
disconnect()during an in-flight reconnect POST doesn't cause side effects (the old testbails out if disconnect() called during in-flight reconnect POSTwas removed). The fire-and-forget model changes the expected behavior, but there should be a test confirming that the.then()callback doesn't flush sends or emit_openafterdisconnect().[fixable] - 🔵 missing_tests: No test verifies the retry path: reconnect POST fails (setting
_pendingReconnectSessions), then a new welcome arrives and retries with the stored sessions. The_pendingReconnectSessionsretry mechanism is the key new behavior replacing the removedscheduleReconnect, but it has no dedicated test.[fixable]
Reviewed at 1737898
| @@ -60,10 +59,6 @@ export class SseConnection implements ChatConnection { | |||
|
|
|||
| disconnect(): void { | |||
There was a problem hiding this comment.
🟡 bugs: disconnect() does not clear _pendingReconnectSessions. If disconnect() is called while a reconnect POST is in-flight, the .then() callback can still fire and call flushPendingSends() after the connection is torn down. Additionally, the stale sessions array persists in memory. Add this._pendingReconnectSessions = null; to disconnect(). [fixable]
| this._connected = true; | ||
| if (sessions) { | ||
| this._pendingReconnectSessions = sessions; | ||
| this.doPost('reconnect', { type: 'reconnect', sessions }).then( |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The reconnect POST's .then() callback clears _pendingReconnectSessions and flushes sends without checking whether _connectionId has changed since the POST was initiated. If a new welcome arrives while the POST is in-flight (EventSource auto-reconnect), the new welcome captures _pendingReconnectSessions (line 213), starts a second POST, and then the stale .then() clears the flag. This is benign only if the server handles duplicate reconnect POSTs with different connection IDs idempotently — which it does (handleReconnect is stateless per call) — but the double flushPendingSends() could cause sends to fire twice. Consider capturing _connectionId before the POST and comparing in the callback. [fixable]
| }); | ||
| } | ||
| if (found && ctx.sessionRegistry.isActive(found.clientId)) { | ||
| const ownerConnection = found.clientId.split(':')[0]; |
There was a problem hiding this comment.
🔵 style: Uses found.clientId.split(':')[0] while every other ownership check in this file (lines 519, 681, 819, 868) uses the getOwnerConnection() utility defined at line 93. Use getOwnerConnection(found.clientId) for consistency — they return the same result but the utility is the convention and would track any format changes. [fixable]
| newClientId, | ||
| }); | ||
| } | ||
| if (found && ctx.sessionRegistry.isActive(found.clientId)) { |
There was a problem hiding this comment.
🟡 regressions: handleReconnect now only reattaches sessions owned by the reconnecting connection (ownerConnection === connectionId). Previously, it also handled cross-connection reattach when the owner was gone. This is deferred to handleSendV2, but creates a window where a non-owner reconnecting client sees events via broadcast while the agent's transport remains stale. Passive observers (watch-only, never send) will never trigger the handleSendV2 ownership path. This is an intentional design choice (documented in comments), but worth noting as a behavioral change.
- Check res.ok in doPost (RED: HTTP 500 was treated as success) - Clear _pendingReconnectSessions on disconnect (YELLOW: stale retry) - Filter pending reconnect sessions against seqBySession (YELLOW: cleared sessions retried) - Reattach detached sessions when owner connection is gone (YELLOW: device restart gap) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 8 issue(s) (2 critical) (4 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (1 warning).
packages/client/src/sse-connection.ts
Clean simplification that removes periodic sync and defers ownership operations to handleSendV2. Architecturally sound — seq dedup guarantees correctness. One minor inefficiency where retry reconnect uses stale lastSeq, and one style inconsistency with getOwnerConnection.
- 🟡 bugs (L215): When
_pendingReconnectSessionsretries on the next welcome, the storedlastSeqvalues are from the original failed attempt. Between failure and retry,seqBySessionmay have advanced viatrackSeq()as SSE events arrived. The filter (line 215-216) only checksseqBySession.has()for existence but doesn't refreshlastSeqfrom the current map. Consider replacing the pending entries'lastSeqwith current values:pending.map(s => ({ ...s, lastSeq: this.seqBySession.get(s.sessionId)! })). Functionally safe due to seq dedup but causes unnecessary replay of events the client already has.[fixable] - 🔵 unsafe_assumptions (L226): Setting
_connected = truebefore the reconnect POST completes meanssend()(line 86-88) will fire user messages immediately viadoPost(), potentially arriving at the server before the reconnect POST. The comment at ws-handler-v2.ts:563-569 documents this is safe due to client-side seq dedup, but there is no server-side ordering guarantee. If a user message arrives beforehandleReconnectsets up the cursor,broadcast()events may be delivered at cursor=0 until the reconnect POST arrives. This is self-healing but could cause a burst of duplicate events.
server/ws-handler-v2.ts
Clean simplification that removes periodic sync and defers ownership operations to handleSendV2. Architecturally sound — seq dedup guarantees correctness. One minor inefficiency where retry reconnect uses stale lastSeq, and one style inconsistency with getOwnerConnection.
- 🔵 style (L260): Uses
found.clientId.split(':')[0]instead of the existinggetOwnerConnection(found.clientId)helper (defined at line 93 and used elsewhere in the same file, e.g. lines 527, 689, 827). Inconsistent — use the helper for uniformity and to benefit from itsindexOffast-path.[fixable] - 🔵 regressions (L259): Reconnect no longer calls
rekeyChat()— ownership transfer is deferred tohandleSendV2. Between reconnect and first send, the session'sclientIdstill references the old connectionId while the transport points to the new connection. This is fine for event delivery (transport is reattached), but if the agent emits logs or metrics usingclientId, they'll reference the stale connectionId until the first send triggers rekey. This is a deliberate design choice (documented in the test renames) — flagging for awareness, not as a bug.
packages/client/src/__tests__/sse-connection.test.ts
Clean simplification that removes periodic sync and defers ownership operations to handleSendV2. Architecturally sound — seq dedup guarantees correctness. One minor inefficiency where retry reconnect uses stale lastSeq, and one style inconsistency with getOwnerConnection.
- 🔵 missing_tests: No test verifies that
_pendingReconnectSessionsare filtered correctly when a session is cleared viaclearSession()between the failed reconnect POST and the retry welcome. The filter at line 215 depends on this, and a test like 'retried reconnect excludes sessions cleared between attempts' would validate the guard.[fixable]
| // message, and replayed events arrive via SSE regardless. | ||
| // Filter pending retries against current seqBySession — sessions may | ||
| // have been cleared (clearSession) since the retry was queued. | ||
| const pending = this._pendingReconnectSessions?.filter((s) => |
There was a problem hiding this comment.
🟡 bugs: When _pendingReconnectSessions retries on the next welcome, the stored lastSeq values are from the original failed attempt. Between failure and retry, seqBySession may have advanced via trackSeq() as SSE events arrived. The filter (line 215-216) only checks seqBySession.has() for existence but doesn't refresh lastSeq from the current map. Consider replacing the pending entries' lastSeq with current values: pending.map(s => ({ ...s, lastSeq: this.seqBySession.get(s.sessionId)! })). Functionally safe due to seq dedup but causes unnecessary replay of events the client already has. [fixable]
| lastSeq, | ||
| })) | ||
| : null); | ||
| this._connected = true; |
There was a problem hiding this comment.
🔵 unsafe_assumptions: Setting _connected = true before the reconnect POST completes means send() (line 86-88) will fire user messages immediately via doPost(), potentially arriving at the server before the reconnect POST. The comment at ws-handler-v2.ts:563-569 documents this is safe due to client-side seq dedup, but there is no server-side ordering guarantee. If a user message arrives before handleReconnect sets up the cursor, broadcast() events may be delivered at cursor=0 until the reconnect POST arrives. This is self-healing but could cause a burst of duplicate events.
| }); | ||
| } | ||
| if (found && ctx.sessionRegistry.isActive(found.clientId)) { | ||
| const ownerConnection = found.clientId.split(':')[0]; |
There was a problem hiding this comment.
🔵 style: Uses found.clientId.split(':')[0] instead of the existing getOwnerConnection(found.clientId) helper (defined at line 93 and used elsewhere in the same file, e.g. lines 527, 689, 827). Inconsistent — use the helper for uniformity and to benefit from its indexOf fast-path. [fixable]
| newClientId, | ||
| }); | ||
| } | ||
| if (found && ctx.sessionRegistry.isActive(found.clientId)) { |
There was a problem hiding this comment.
🔵 regressions: Reconnect no longer calls rekeyChat() — ownership transfer is deferred to handleSendV2. Between reconnect and first send, the session's clientId still references the old connectionId while the transport points to the new connection. This is fine for event delivery (transport is reattached), but if the agent emits logs or metrics using clientId, they'll reference the stale connectionId until the first send triggers rekey. This is a deliberate design choice (documented in the test renames) — flagging for awareness, not as a bug.
- Refresh lastSeq from current seqBySession on pending retry (YELLOW: stale seq causes unnecessary replay)
- Guard reconnect POST callback against stale connectionId (YELLOW: double-flush on rapid reconnect)
- Use getOwnerConnection() helper instead of .split(':')[0] (BLUE: consistency)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 8 issue(s) (5 warning).
|
- Defer _connected until reconnect POST succeeds (YELLOW: prevents sends from bypassing queue during in-flight window) - Don't flush pending sends on POST failure (YELLOW: server hasn't set up cursor/replay, sends stay queued for next attempt) - Add test for send queuing during reconnect POST in-flight (YELLOW) - Fix 4 stale "periodic sync" comments (BLUE: removed in P3) - Document cursor-at-0 bandwidth trade-off in handleSendV2 (YELLOW) - Document transport/clientId mismatch window on reattach (YELLOW) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 7 issue(s) (4 warning).
packages/client/src/sse-connection.ts
Well-structured simplification that replaces periodic sync with on-demand reconnect replay. The stale-callback guard and zombie-deferral design are sound, but the stale-callback race and zombie-reattach window deserve test coverage, and the doPost silent-resolve on null connectionId is a subtle contract mismatch.
- 🟡 bugs (L291): doPost silently resolves (doesn't throw) when _connectionId is null. When called from the reconnect .then() chain (line 236), if _connectionId were somehow cleared between the welcome event and the fetch, the success handler would fire without the POST ever being sent — marking _connected=true and flushing sends into the void. In practice this path is unreachable (connectionId is set at line 208 before the POST), but the contract is misleading: callers assume doPost either succeeds or throws, yet this early return is a silent success.
[fixable] - 🔵 style (L220): The session-selection logic (lines 217-227) is dense: a null-coalescing chain of filtered/mapped pending sessions falling back to a ternary for fresh sessions. Consider extracting to a private method like
getReconnectSessions()for readability.[fixable]
server/ws-handler-v2.ts
Well-structured simplification that replaces periodic sync with on-demand reconnect replay. The stale-callback guard and zombie-deferral design are sound, but the stale-callback race and zombie-reattach window deserve test coverage, and the doPost silent-resolve on null connectionId is a subtle contract mismatch.
- 🟡 unsafe_assumptions (L571): The comment explains that between watch() (line 570) and the reconnect POST arriving, the cursor starts at 0, so broadcasts may deliver duplicates. This relies entirely on client-side seq dedup. If a client implementation doesn't dedup (e.g. a future non-Zustand consumer), it will see duplicate events. The comment documents the trade-off well, but the gap is structural — consider adding a TODO for P4 to unify cursor reset.
- 🟡 regressions (L267): handleReconnect now reattaches detached sessions without checking EventStore state. Previously, it cross-referenced storeState (ENDED/CLOSING) to detect zombies and called registry.remove() before attempting reattach. Now it reattaches any active+detached session unconditionally. If a session's EventStore state is ENDED but the SessionRegistry hasn't been cleaned up yet (race window), handleReconnect will reattach a zombie. The zombie is only caught later if the user sends a message (handleSendV2 line 586). For observe-only reconnects (no send), the zombie stays reattached until the next tick or server restart.
[fixable]
packages/client/src/__tests__/sse-connection.test.ts
Well-structured simplification that replaces periodic sync with on-demand reconnect replay. The stale-callback guard and zombie-deferral design are sound, but the stale-callback race and zombie-reattach window deserve test coverage, and the doPost silent-resolve on null connectionId is a subtle contract mismatch.
- 🟡 missing_tests: The stale-callback guard for racing welcome events is no longer tested. The old test 'ignores stale reconnect POST when a newer welcome arrives' verified that when two welcome events arrive in quick succession, only the second (current) POST's callback marks connected. The new code still has this guard (line 238/245: connectionId !== postConnectionId), but no test exercises it. This was a real-world race condition worth covering.
[fixable] - 🔵 missing_tests: No test verifies that SSE events (es.onmessage) update seqBySession while the reconnect POST is in-flight. The old test 'dispatches SSE events to listener while reconnect POST is in-flight' was removed. The new code refreshes lastSeq from seqBySession on retry (line 219), so verifying that SSE events advance the seq map during the in-flight window is important for the retry-with-fresh-seq guarantee.
[fixable]
packages/protocol/src/ws-schemas-v2.ts
Well-structured simplification that replaces periodic sync with on-demand reconnect replay. The stale-callback guard and zombie-deferral design are sound, but the stale-callback race and zombie-reattach window deserve test coverage, and the doPost silent-resolve on null connectionId is a subtle contract mismatch.
- 🔵 style (L127): The added comment says 'WS clients still send it over WS until P4 removes the WS chat transport' — this is a phase-plan reference that will become stale. Consider phrasing it as current behavior ('WS clients may also send reconnect over WS') without the roadmap reference.
[fixable]
| } | ||
|
|
||
| private async doPost(endpoint: string, body: Record<string, unknown>): Promise<void> { | ||
| if (!this._connectionId) return; |
There was a problem hiding this comment.
🟡 bugs: doPost silently resolves (doesn't throw) when _connectionId is null. When called from the reconnect .then() chain (line 236), if _connectionId were somehow cleared between the welcome event and the fetch, the success handler would fire without the POST ever being sent — marking _connected=true and flushing sends into the void. In practice this path is unreachable (connectionId is set at line 208 before the POST), but the contract is misleading: callers assume doPost either succeeds or throws, yet this early return is a silent success. [fixable]
| const pending = this._pendingReconnectSessions | ||
| ?.filter((s) => this.seqBySession.has(s.sessionId)) | ||
| .map((s) => ({ ...s, lastSeq: this.seqBySession.get(s.sessionId)! })); | ||
| const sessions = |
There was a problem hiding this comment.
🔵 style: The session-selection logic (lines 217-227) is dense: a null-coalescing chain of filtered/mapped pending sessions falling back to a ternary for fresh sessions. Consider extracting to a private method like getReconnectSessions() for readability. [fixable]
| } | ||
| applySkillPolicy(activeClientId); | ||
| ctx.connRegistry.watch(connectionId, sessionId); | ||
| // No resetCursor here — handleReconnect (fire-and-forget POST) sets |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The comment explains that between watch() (line 570) and the reconnect POST arriving, the cursor starts at 0, so broadcasts may deliver duplicates. This relies entirely on client-side seq dedup. If a client implementation doesn't dedup (e.g. a future non-Zustand consumer), it will see duplicate events. The comment documents the trade-off well, but the gap is structural — consider adding a TODO for P4 to unify cursor reset.
| ctx.sessionRegistry.remove(found!.clientId); | ||
| } | ||
| if (found && running && !ctx.sessionRegistry.isAttached(found.clientId)) { | ||
| if (found && ctx.sessionRegistry.isActive(found.clientId)) { |
There was a problem hiding this comment.
🟡 regressions: handleReconnect now reattaches detached sessions without checking EventStore state. Previously, it cross-referenced storeState (ENDED/CLOSING) to detect zombies and called registry.remove() before attempting reattach. Now it reattaches any active+detached session unconditionally. If a session's EventStore state is ENDED but the SessionRegistry hasn't been cleaned up yet (race window), handleReconnect will reattach a zombie. The zombie is only caught later if the user sends a message (handleSendV2 line 586). For observe-only reconnects (no send), the zombie stays reattached until the next tick or server restart. [fixable]
|
|
||
| // ─── Union ────────────────────────────────────────────────────────────────── | ||
|
|
||
| // ReconnectMessage is primarily handled via REST POST (SSE transport), but WS |
There was a problem hiding this comment.
🔵 style: The added comment says 'WS clients still send it over WS until P4 removes the WS chat transport' — this is a phase-plan reference that will become stale. Consider phrasing it as current behavior ('WS clients may also send reconnect over WS') without the roadmap reference. [fixable]
- Throw on doPost when connectionId is null (YELLOW: silent success) - Add P4 TODO for cursor reset unification (YELLOW: dedup assumption) - Check EventStore state before reattaching on reconnect (YELLOW: zombie) - Extract getReconnectSessions() for readability (BLUE: dense logic) - Remove roadmap reference from WS reconnect comment (BLUE: stale) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
handleReconnect— reattach/rekey/zombie cleanup removed (~60 lines).handleSendV2already handles all of this on the first user message, making it redundant on reconnect.ConnectionRegistrythat retried missed events is eliminated (~130 lines). Cursor-based replay on welcome covers the gap._connecteduntil the reconnect POST succeeds (~100 lines). Connection is marked immediately on welcome; the POST is advisory.ReconnectMessagefrom WS union — reconnect is REST-only, not dispatched via WebSocket.Net: -1,043 lines across 9 files. All 177 affected tests pass.
Context
Phase 3 of the Transport SSOT series (P0 #431, P1 #433). The reconnect "ownership dance" duplicated logic already handled by
handleSendV2on the first user message. Periodic sync was a safety net made redundant by cursor replay on welcome.What's kept
/reconnectREST endpoint +handleReconnecthandler (watch + cursor reset + EventStore replay + boot context + suspend resume)doPost('reconnect', ...)on welcome (fire-and-forget)ConnectionRegistry.resetCursor()and cursor tracking inbroadcast()Test plan
npx vitest run— all 177 tests pass (ws-handler-v2, connection-registry, sse-connection)🤖 Generated with Claude Code