Repository navigation
fix(desktop): guard terminal resume against in-flight connect - #5137
Conversation
handleResume only skipped when transport.socket was OPEN/CONNECTING, but for /hosts/ URLs connect() runs primeRelayAffinity async, leaving transport.socket null while connectionState is "connecting". A resume event (online/focus/visibilitychange) firing in that window saw the null socket, fell through, and called reconnectNow a second time — both pre-flights resolved into openSocket() and the first WebSocket was overwritten without being closed, orphaning it on client and server. Skip when connectionState is "connecting" too. reconnectNow stays unguarded so the watchdog can still force-recycle a half-open "open" socket (or a hung connect). Follow-up to #5135 (flagged in review).
|
Ready to review this PR? Stage has broken it down into 1 individual chapter for you:
Chapters generated by Stage for commit e2d2cb7 on Jun 5, 2026 3:27am UTC. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesResume Reconnect Suppression
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR tightens the guard in
Confidence Score: 4/5Safe to merge; the one-line guard change is mechanically correct and the rest of the transport logic is untouched. The fix correctly closes the pre-flight race window by checking connectionState instead of socket.readyState, and the dropped WebSocket.CONNECTING branch is fully covered. The only gap is that no regression test pins the specific invariant being fixed — a future refactor that reorders setConnectionState and openSocket could silently re-open the race without a failing test to catch it. apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts — verify the pre-flight race scenario is captured by a new test
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Adds connectionState === 'connecting' guard to handleResume, closing the race window where the /hosts/ async pre-flight leaves transport.socket null while a wake/resume event could call reconnectNow a second time and open a duplicate WebSocket. The logic is correct and the removed WebSocket.CONNECTING readyState check is safely subsumed by the connectionState guard. |
Sequence Diagram
sequenceDiagram
participant WD as Watchdog / reconnectNow
participant C as connect()
participant PF as primeRelayAffinity
participant OS as openSocket()
participant HR as handleResume
WD->>C: reconnectNow → connect()
C->>C: setConnectionState("connecting")
C->>PF: await primeRelayAffinity(url) [async]
Note over C,PF: transport.socket = null, connectionState = "connecting"
Note over HR: online/focus event fires
HR->>HR: "check connectionState === "connecting" → bail ✓"
Note over HR: old code saw socket=null, fell through, called reconnectNow again ✗
PF-->>OS: resolves → openSocket()
OS->>OS: new WebSocket(url)
OS->>OS: "transport.socket = socket"
Note over OS: Only one socket created
Comments Outside Diff (1)
-
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts, line 204-219 (link)Missing regression test for the guarded race
The existing test suite doesn't cover the specific scenario this PR fixes: a resume event firing while
connectionState === "connecting"andtransport.socket === null(i.e., during the/hosts/pre-flight window). Without a test, a future refactor that movessetConnectionState(transport, "connecting")afteropenSocket()would silently re-introduce the orphaned-socket race. A test that mocksprimeRelayAffinityto return a deferred promise, fires a windowonlineevent mid-flight, then assertsMockWebSocket.instances.length === 1after both promises resolve would pin this invariant.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts Line: 204-219 Comment: **Missing regression test for the guarded race** The existing test suite doesn't cover the specific scenario this PR fixes: a resume event firing while `connectionState === "connecting"` and `transport.socket === null` (i.e., during the `/hosts/` pre-flight window). Without a test, a future refactor that moves `setConnectionState(transport, "connecting")` after `openSocket()` would silently re-introduce the orphaned-socket race. A test that mocks `primeRelayAffinity` to return a deferred promise, fires a window `online` event mid-flight, then asserts `MockWebSocket.instances.length === 1` after both promises resolve would pin this invariant. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:204-219
**Missing regression test for the guarded race**
The existing test suite doesn't cover the specific scenario this PR fixes: a resume event firing while `connectionState === "connecting"` and `transport.socket === null` (i.e., during the `/hosts/` pre-flight window). Without a test, a future refactor that moves `setConnectionState(transport, "connecting")` after `openSocket()` would silently re-introduce the orphaned-socket race. A test that mocks `primeRelayAffinity` to return a deferred promise, fires a window `online` event mid-flight, then asserts `MockWebSocket.instances.length === 1` after both promises resolve would pin this invariant.
Reviews (1): Last reviewed commit: "fix(desktop): guard terminal resume agai..." | Re-trigger Greptile
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Follow-up to #5135, addressing the race flagged by Greptile (P1) and cubic (P2) in review.
Problem
handleResumeonly bailed whentransport.socketwasOPEN/CONNECTING. But for remote/hosts/URLs,connect()runsprimeRelayAffinityasynchronously before opening the socket, so there's a window wheretransport.socketisnullwhileconnectionStateis"connecting".Wake-time sequence that orphans a socket:
reconnectNow→connect()starts the async pre-flight →transport.socketisnull, state"connecting"online/focus/visibilitychangeevent fires →handleResumesees thenullsocket, skips the early-return, callsreconnectNowagainopenSocket()(both pass thecurrentUrl/connectionStateguards), opening two WebSockets — the first is overwritten intransport.socketwithout being closed, leaving it orphaned on client and server.Fix
Bail out of
handleResumewhenconnectionState === "connecting"too, covering the pre-flight window.reconnectNowis deliberately left unguarded so the watchdog can still force-recycle a genuinely half-open socket (which reports"open") or a hung connect.Testing
bun test(3 pass),bun run lint(clean),bun run typecheck(pass).Summary by cubic
Prevent double WebSocket openings during terminal resume by bailing out when a connect is already in flight. This avoids orphaned sockets on remote
/hosts/connections.handleResume, return early whenconnectionState === "connecting"or the socket isOPEN, covering the pre-flight window beforetransport.socketis set.Written for commit e2d2cb7. Summary will update on new commits.
Summary by CodeRabbit