fix(desktop): bound reconnect awaits so a stuck IPC round-trip can't latch the UI frozen - #93476
Closed
chelsealong wants to merge 2 commits into
Closed
chelsealong wants to merge 2 commits into
chelsealong wants to merge 2 commits into
Conversation
…latch the UI frozen After a liveness-probe-triggered reconnect on a remote gateway, attemptReconnect() awaits desktop.getConnection() and resolveGatewayWsUrl() with no timeout. If either stalls (e.g. main process wedged mid-revalidation even though the backend itself is reachable), the `reconnecting` guard never clears, so every later scheduleReconnect()/attemptReconnect() early-returns forever and the UI stays stuck in "reconnecting" until the app is restarted. Bound both awaits with a 20s timeout so a stall rejects instead of hanging; the existing catch/finally already clears the guard and resumes backoff on rejection. gateway.connect() keeps its own separate connect timeout. Fixes NousResearch#93454
…h#93454) attemptReconnect() awaited desktop.revalidateConnection?.() unbounded, immediately before the two IPC calls the previous commit wrapped in withTimeout(). A wedged revalidation after a liveness-probe trip - the exact trigger NousResearch#93454 and this file's own comment describe - hung that await forever, so the reconnecting guard never cleared and the prior fix never got reached. Wrap it in the same 20s withTimeout() (still swallowing the result via .catch, matching its existing best-effort semantics) and extend the regression test to hang revalidateConnection() specifically, proving getConnection() and the socket still proceed once the stall times out.
Collaborator
|
Merged via #93662 with your commits cherry-picked (authorship preserved); we bounded the sibling boot/gateway-switch awaits on top. Prior art credit to @victorftrdba's #40008. Thanks @chelsealong! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Bounds the three IPC-round-trip awaits in the primary reconnect loop
(
attemptReconnect()inuse-gateway-boot.ts) with a 20s timeout, so astuck main-process call can no longer permanently freeze Desktop's
reconnect backoff.
Problem
Fixes #93454.
Desktop connected to a remote gateway over an SSH tunnel periodically
freezes on reconnect after a transient liveness-probe failure. The UI is
left stuck ("gateway needs setup" / stuck reconnecting) with no error
surfaced, even though the backend itself is confirmed healthy the whole
time (
curl .../api/status→ 200 OK). Only killing and relaunching theapp recovers it.
attemptReconnect()sets areconnectingguard, then does:revalidateConnection(),getConnection(), andresolveGatewayWsUrl()areall IPC calls into the Electron main process with no timeout of their own. If
any stalls (a wedged revalidation after the liveness probe trips, even
though the remote backend answers fine), the await never settles. While it's
pending,
reconnectingnever clears, so every laterscheduleReconnect()/attemptReconnect()early-returns forever — the backoff loop is latchedand the UI never self-heals.
An earlier version of this PR bounded only
getConnection()andresolveGatewayWsUrl()— review correctly flagged that the PR's owndiagnosis names a "wedged revalidation" as the trigger, and that call
(
revalidateConnection()) sat one line above the first bounded await,untouched, with only a
.catch(() => undefined)that swallows a rejectionbut does nothing for a promise that never settles. This revision bounds
that call too.
This is the same defect described (and fixed, but never merged) in closed
PR #40008 for a different trigger (no-network suspend); the code has
since grown several more
getConnection()/resolveGatewayWsUrl()callsites, so that patch no longer applies, but the primary automatic
reconnect loop this issue reports against still has no timeout.
Fix
Add a small
withTimeout()helper and wrap all three awaits inattemptReconnect()with a 20s bound — includingrevalidateConnection(),which keeps its original best-effort
.catch(() => undefined)(a stall ora rejection there is still non-fatal to the reconnect attempt; only
getConnection()/resolveGatewayWsUrl()failing aborts it). On timeouteach await rejects, the existing
catch/finallyclears thereconnectingguard, andscheduleReconnect()resumes the backoff — so theUI recovers on its own once the gateway is reachable again, matching the
reporter's expected behavior.
gateway.connect()already has its ownseparate 15s connect timeout and is unchanged.
How to Test
revalidateConnection()/getConnection()/resolveGatewayWsUrl()IPC call to hang (e.g. a wedged main-processrevalidation) while the remote backend itself keeps answering
/api/statuswith 200 OK.loop resumes, and the app reconnects on its own.
Automated regression tests in
use-gateway-boot.test.tsx:desktop.getConnection()forever (new Promise(() => undefined)) on every reconnect attempt after a drop, and asserts afurther reconnect attempt still fires after the internal timeout elapses.
desktop.revalidateConnection()specifically (the callnamed in the bug report and this file's own comment as the trigger,
getConnection()left fast/unmocked) and assertsgetConnection()isstill reached and the socket reopens once the timeout elapses — this is
the case review found missing from the first version of this PR.
Confirmed the new revalidateConnection() test fails without the fix
(reverted only the source file via
git stash push -- <file>, kept thetest):
Checklist
AGENTS.md.without the fix.
AI assistance disclosure
This change was drafted with AI assistance (Claude) and reviewed before
submission.