fix(ws): recover broken chat connections and subscriptions - #1301
materemias wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesThe WebSocket server now handles WebSocket protocol
Sequence Diagram(s)sequenceDiagram
participant WebSocketProvider
participant useChatSessionState
participant ChatServer
participant SessionMessagesAPI
WebSocketProvider-->>useChatSessionState: websocket_reconnected
useChatSessionState->>ChatServer: chat.subscribe
ChatServer-->>useChatSessionState: chat_subscribed
useChatSessionState->>SessionMessagesAPI: request latest messages
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The WebSocket recovery changes have no remaining verified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 11 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit reads each line, Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The new chat.ping handler can emit pong frames with a null nonce on invalid input, which breaks the documented nonce-matching contract and should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the chat /ws transport against half-open connections by adding an application-level ping/pong probe + bounded reconnect handshakes, and updates chat session logic to only refresh persisted history after a replacement socket has explicitly re-acknowledged its subscription.
Changes:
- Add periodic
chat.pingprobes with pong deadlines + connect deadlines to retire stuck sockets and trigger reconnects. - Ensure chat subscription recovery waits for a matching
chat_subscribedacknowledgement (with a 10s bound) before refreshing persisted history. - Add targeted frontend and backend tests plus protocol documentation updates for the new heartbeat and resubscribe behavior.
File summaries
| File | Description |
|---|---|
| src/shared/types.ts | Update shared transport documentation to include pong as a gateway frame kind. |
| src/shared/context/WebSocketContext.tsx | Implement ping/pong probing, connect timeout, and safer reconnect/retirement logic for the shared chat socket. |
| src/shared/tests/webSocketContext.test.tsx | Add coverage for half-open recovery, pong matching, stale callbacks, token rotation, and cleanup behaviors. |
| src/modules/chat/hooks/useChatSessionState.ts | Add reconnect acknowledgement gating + deadline to prevent history refresh before a replacement subscription is confirmed. |
| src/modules/chat/hooks/useChatRealtimeHandlers.ts | Remove reconnect callback path and ensure reconnect notifications never become transcript rows. |
| src/modules/chat/ChatInterface.tsx | Wire subscribe into session state and remove the prior reconnect handler wiring. |
| src/modules/chat/tests/transcriptScrollOwnership.test.tsx | Update test harness to satisfy the new subscribe requirement. |
| src/modules/chat/tests/reconnectSubscription.test.tsx | Add coverage for resubscribe ack gating, hidden behavior, timeouts, and stale listener protection. |
| server/shared/types.ts | Add pong to gateway event kind union. |
| server/modules/websocket/services/chat-websocket.service.ts | Add chat.ping handling that replies with pong. |
| server/modules/websocket/tests/chat-ping.test.ts | Add a backend test ensuring pings only reply to the origin socket and do not touch providers/broadcast. |
| server/modules/websocket/README.md | Document the broken-channel recovery mechanism and the resubscribe-ack gating. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case 'chat.ping': | ||
| sendJson(ws, { | ||
| kind: 'pong', | ||
| nonce: typeof data.nonce === 'string' ? data.nonce : null, | ||
| }); | ||
| return; |
|
|
||
| import { api } from '@/shared/api'; | ||
| import type { MarkSessionIdle, SessionActivityMap,Project,ProjectSession,LLMProvider,NormalizedMessage,ChatMessage,DiffCalculator } from '@/shared/types'; | ||
| import type { MarkSessionIdle, SessionActivityMap,Project,ProjectSession,LLMProvider,NormalizedMessage,ChatMessage,DiffCalculator,ServerEvent } from '@/shared/types'; |
6678985 to
0f8be0a
Compare
0f8be0a to
5085474
Compare
|
@materemias please break up the PR into small pieces with a reproduction step of the error for each. |
Purpose
Supersedes #1283 with one commit based on upstream
mainat5e73a49b. This carries the reviewed reconnect fix and adds recovery when a replacement socket never acknowledges its chat subscription.Restore the shared
/wschat connection when it stops working, then restore the selected chat's subscription and persisted history. Provider startup, abort behavior, defaults, authentication modes,/shell, and plugin WebSockets are unchanged.Behavior
chat.ping30 seconds after connection or a matching pong. If no matching pong arrives within 3 seconds, retire the socket and retry after the existing 3-second delay. A handshake stuck for 30 seconds also retries. The gateway rejects missing or non-string nonces withprotocol_error/INVALID_NONCE, without emitting a pong or closing the socket.pageshowprobes as faster triggers, but do not depend on those events. Use one transport timer and reject stale socket callbacks; token changes, logout, and unmount clean up the old connection.chat_subscribedacknowledgement before refreshing history. Synthetic reconnect notifications never enter transcript storage.The server sends replay after the acknowledgement, so replay and the history request may overlap. Browser timer suspension/throttling can delay recovery. Failed application messages are not automatically resent. Retry backoff and provider-run interruption changes are outside this PR.
Verification
Native Chromium fixtures, using actual client hooks and gateway code:
5e73a49bstayed apparently connected with no replacement or new message after 51 seconds. The carried transport fix made exactly one replacement by the 40-second observation and received the message, without visibility/page-show events.These are isolated fixtures with stubbed authentication/provider execution, not a physical laptop-suspend test or a provider-backed conversation.
The real-socket gateway regression rejects a malformed nonce without a pong, then confirms valid pings still work on the same socket without provider calls or broadcasts. It fails before nonce validation and passes afterward.
Commands on this branch:
npm run test:client: 424 passed.npm test: 416 passed, one Codex fixture-dependent skip.npm run typecheck: passed.npm run build: passed, with CSS/bundle-size warnings.npm run lint: successful exit, 132 warnings and no errors.Summary by CodeRabbit
New Features
chat.pingrequests with correspondingpongresponses.Bug Fixes