fix(transport): deliver first prompts reliably across reconnect - #455
Conversation
Centaur ReviewFound 10 issue(s) (2 critical) (5 warning).
|
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (2 warning).
server/chat-rest-handler.ts
Well-designed durable prompt delivery system with solid test coverage. No critical bugs found; the main concerns are around edge-case test gaps in the outbox and minor UX polish for transient delivery status.
- 🟡 unsafe_assumptions (L105): When no SSE stream exists (no X-Connection-ID header), the delegate SseTransport has isOpen()=false. For a brand-new session that has no watchers yet, events emitted synchronously during handleSendV2 (before the outbox-triggered replay adds a watcher) are sent to the delegate, which silently drops them. Startup errors from the async startChat() .catch() handler would be lost until the client's SSE stream connects and replays. The failSendCommand path in transport.send covers error events, but non-error early events (like session_state_changed) may be missed.
packages/client/src/send-outbox.ts
Well-designed durable prompt delivery system with solid test coverage. No critical bugs found; the main concerns are around edge-case test gaps in the outbox and minor UX polish for transient delivery status.
- 🟡 bugs (L108): When stop() is called during an in-flight fetch and the fetch completes successfully before the abort takes effect,
if (!this.active) returndiscards the successful response without shifting the entry or notifying _send_accepted. The entry remains in the queue and will be re-POSTed on next start(). The server deduplicates correctly, but the user sees no acceptance notification until the retry succeeds. The test 'acknowledges a stopped in-flight prompt on restart' covers the lifecycle but does not assert that the first successful response is captured.[fixable]
packages/client/src/__tests__/send-outbox.test.ts
Well-designed durable prompt delivery system with solid test coverage. No critical bugs found; the main concerns are around edge-case test gaps in the outbox and minor UX polish for transient delivery status.
- 🔵 missing_tests: Several outbox paths lack test coverage: (1) 429/5xx server responses triggering retry, (2) the 100-entry queue limit returning false from enqueue, (3) receipt validation failure (mismatched clientMsgId or non-boolean accepted) entering the retry path, (4) _send_pending notification details (retrying flag). These are edge cases but they exercise distinct code paths in pump().
[fixable]
packages/client/src/sse-connection.ts
Well-designed durable prompt delivery system with solid test coverage. No critical bugs found; the main concerns are around edge-case test gaps in the outbox and minor UX polish for transient delivery status.
- 🔵 style (L473): checkAndReconnect(true) on every desktop visibilitychange to 'visible' tears down and rebuilds a potentially healthy EventSource. The WS transport uses force=false for browser events, reserving force=true for Capacitor hooks. The design doc explicitly defends this choice (EventSource readyState doesn't prove the connection survived suspension), so this is intentional, but it adds unnecessary latency on desktop tab switches where the connection is usually fine. Consider checking es.readyState before forcing, or using a short heartbeat to detect stale connections.
[fixable]
packages/client/src/store.ts
Well-designed durable prompt delivery system with solid test coverage. No critical bugs found; the main concerns are around edge-case test gaps in the outbox and minor UX polish for transient delivery status.
- 🔵 style (L680): The sendError field is set to 'Sending…' for the initial _send_pending notification, which is a transient status indicator, not an error. Using the same field for both status and errors means the UI cannot distinguish them visually. Consider a separate sendStatus field or a structured {status, message} object if you want to style these differently in the future.
[fixable]
| handleSendV2(connectionId, transport, msg, ctx); | ||
| res.status(202).json({ ok: true }); | ||
| const receipt = acceptSendCommand(ctx.eventStore, msg, (command, sessionId) => { | ||
| const delegate = new SseTransport(connectionId, sseRegistry); |
There was a problem hiding this comment.
🟡 unsafe_assumptions: When no SSE stream exists (no X-Connection-ID header), the delegate SseTransport has isOpen()=false. For a brand-new session that has no watchers yet, events emitted synchronously during handleSendV2 (before the outbox-triggered replay adds a watcher) are sent to the delegate, which silently drops them. Startup errors from the async startChat() .catch() handler would be lost until the client's SSE stream connects and replays. The failSendCommand path in transport.send covers error events, but non-error early events (like session_state_changed) may be missed.
| timeout = setTimeout(() => abort.abort(), this.config.timeoutMs ?? 15000); | ||
| }), | ||
| ]); | ||
| if (!this.active) return; |
There was a problem hiding this comment.
🟡 bugs: When stop() is called during an in-flight fetch and the fetch completes successfully before the abort takes effect, if (!this.active) return discards the successful response without shifting the entry or notifying _send_accepted. The entry remains in the queue and will be re-POSTed on next start(). The server deduplicates correctly, but the user sees no acceptance notification until the retry succeeds. The test 'acknowledges a stopped in-flight prompt on restart' covers the lifecycle but does not assert that the first successful response is captured. [fixable]
| this.boundOnVisibility = () => { | ||
| if (document.visibilityState === 'visible') { | ||
| this.checkAndReconnect(); | ||
| this.checkAndReconnect(true); |
There was a problem hiding this comment.
🔵 style: checkAndReconnect(true) on every desktop visibilitychange to 'visible' tears down and rebuilds a potentially healthy EventSource. The WS transport uses force=false for browser events, reserving force=true for Capacitor hooks. The design doc explicitly defends this choice (EventSource readyState doesn't prove the connection survived suspension), so this is intentional, but it adds unnecessary latency on desktop tab switches where the connection is usually fine. Consider checking es.readyState before forcing, or using a short heartbeat to detect stale connections. [fixable]
| .messages.messages.some((m) => m.messageId === msg.clientMsgId); | ||
| if (visible) { | ||
| store.setState({ | ||
| sendError: |
There was a problem hiding this comment.
🔵 style: The sendError field is set to 'Sending…' for the initial _send_pending notification, which is a transient status indicator, not an error. Using the same field for both status and errors means the UI cannot distinguish them visually. Consider a separate sendStatus field or a structured {status, message} object if you want to style these differently in the future. [fixable]
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
LGTM — no issues found.
Returning to Mitzo could strand the first prompt: the client queued it based on stale running state, ignored POST failures, and reconnect could rekey the session while the query loop retained its original key.
This change keeps runtime identity stable and makes prompt delivery independent of SSE readiness. A durable command receipt allocates the session ID before dispatch, so a lost acknowledgement can be retried without starting another session. The client retains unacknowledged prompts in a per-tab outbox, retries with the same ID, bounds hung requests, and shows delivery status. Reconnect restores tracked sessions even when a prompt was accepted before the first stream welcome.
Includes the always-send removal from #445; retains cursor replay and periodic sync rather than adopting #440 wholesale. See
docs/design/prompt-delivery.mdfor the contract and boundaries. Server restart recovery surfaces ambiguous execution as interrupted rather than repeating possible tool side effects.Validation: