fix(client): await reconnect POST before flushing pending sends

[dimakis]

Summary

• Reconnect POST was fire-and-forget — flushPendingSends() ran immediately after, so queued client messages could reach the server before it set up watches or reattached sessions
• Now flushPendingSends() is deferred until the reconnect POST resolves. On first connection (no reconnect needed), flush remains synchronous
• doPost catches internally, so flush always proceeds even if the POST fails

Found by Centaur review on #384.

Test plan

• New test: pending sends only flush after reconnect POST completes
• New test: pending sends flush even if reconnect POST fails (network error)
• All 22 existing SSE connection tests pass

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 2 issue(s).

packages/client/src/sse-connection.ts

Clean fix for a real race condition — reconnect POST correctly gates the pending-send flush via .then(), and both success/failure paths are tested. Two minor suggestions: document the intentional _open timing divergence from the WS transport, and consider a test for the else branch (reconnect with no tracked sessions).

• 🔵 regressions (L233): Subtle ordering change: _open now fires BEFORE pending sends are flushed in the reconnect path (previously it fired after). If any _open consumer immediately sends a new message via send(), that message bypasses the queue (_connected is true) and could arrive at the server before the deferred pending sends. In practice this is safe — the protocol parser only sets status: 'connected' on _open and doesn't trigger sends — but it diverges from the WS transport (connection.ts:232-233) which preserves flush-before-open ordering. Worth a comment if intentional. [fixable]

packages/client/src/__tests__/sse-connection.test.ts

Clean fix for a real race condition — reconnect POST correctly gates the pending-send flush via .then(), and both success/failure paths are tested. Two minor suggestions: document the intentional _open timing divergence from the WS transport, and consider a test for the else branch (reconnect with no tracked sessions).

• 🔵 missing_tests: No test for the else branch on reconnect: _isReconnect === true but seqBySession.size === 0 (all sessions cleared before reconnect). This path skips the reconnect POST and flushes synchronously. It's a minor gap since the logic is straightforward, but the two new tests only cover the if branch. [fixable]

[dimakis]

🔵 regressions: Subtle ordering change: _open now fires BEFORE pending sends are flushed in the reconnect path (previously it fired after). If any _open consumer immediately sends a new message via send(), that message bypasses the queue (_connected is true) and could arrive at the server before the deferred pending sends. In practice this is safe — the protocol parser only sets status: 'connected' on _open and doesn't trigger sends — but it diverges from the WS transport (connection.ts:232-233) which preserves flush-before-open ordering. Worth a comment if intentional. [fixable]

[dimakis]

Centaur Review

Found 2 issue(s) (1 warning).

packages/client/src/sse-connection.ts

Clean fix for a real race condition — pending sends correctly serialize behind the reconnect POST. Two minor points: consider .finally() over .then() for resilience, and note the _open-before-flush ordering diverges from the WS transports (safe today, worth documenting).

• 🟡 regressions (L233): In the reconnect path, _open now fires before flushPendingSends completes (since flush is deferred behind .then()). The WS transport (ws-connection.ts:254-279) explicitly flushes before broadcasting _open with a long comment explaining why: listeners reacting to _open may trigger new sends that interleave before queued ones. Currently the SSE _open listener only updates status (no send side-effects), so this is safe today — but diverges from the convention in the other two transport implementations.
• 🔵 unsafe_assumptions (L227): .then(() => this.flushPendingSends()) relies on doPost never rejecting (its internal catch swallows errors). If doPost is ever refactored to re-throw, pending sends would silently never flush. Using .finally(() => this.flushPendingSends()) would be more resilient and communicate intent more clearly — the flush should happen regardless of reconnect POST outcome. The failure-path test would catch a regression, but .finally() eliminates the risk entirely. [fixable]

[dimakis]

🟡 regressions: In the reconnect path, _open now fires before flushPendingSends completes (since flush is deferred behind .then()). The WS transport (ws-connection.ts:254-279) explicitly flushes before broadcasting _open with a long comment explaining why: listeners reacting to _open may trigger new sends that interleave before queued ones. Currently the SSE _open listener only updates status (no send side-effects), so this is safe today — but diverges from the convention in the other two transport implementations.

[dimakis]

🔵 unsafe_assumptions: .then(() => this.flushPendingSends()) relies on doPost never rejecting (its internal catch swallows errors). If doPost is ever refactored to re-throw, pending sends would silently never flush. Using .finally(() => this.flushPendingSends()) would be more resilient and communicate intent more clearly — the flush should happen regardless of reconnect POST outcome. The failure-path test would catch a regression, but .finally() eliminates the risk entirely. [fixable]