Repository navigation
fix(remote-runtime): retry a pane attach that fails while the runtime is unreachable - #18496
luisgonzaleznf wants to merge 2 commits into
Conversation
… is unreachable A client that starts while its paired runtime is unreachable attaches to already-open panes instead of creating them, so the failure lands in attach() rather than connect(). attach() never sets `connected`, so the recoverable branch's scheduleResubscribeAfterTransportClose() returned at its `!connected` guard: nothing armed, `connecting` latched, and neither emitRecoveryState() nor surfaceErrorMessage() reached. getRecoveryState() then reported 'connecting', which the banner filter hides — the pane rendered as a mounted xterm bound to nothing, with no text, no error and no retry, and stayed that way after the runtime came back. connect() handles the identical error class by arming scheduleConnectRetryAfterRecoverableFailure(); that asymmetry was the bug. Arm the same retry when a recoverable error arrives with no live stream, and stop the replay wiping its own recovery window: attach() cancelled recovery on entry and its catch cancelled unconditionally, so every replay restarted the 250ms..30s ladder and the auto-recovery deadline never latched — no 'disconnected' phase, so no Reconnect button and no parked retry. connect() cancels only on the gone and fatal paths, for the same reason. Also fire pane recovery when a runtime environment becomes reachable. The existing `online` and system-resume triggers both miss a tailnet or VPN returning: navigator.onLine never went false because the LAN never dropped, and no power transition occurs. A parked pane would otherwise wait for a manual Reconnect indefinitely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughRemote runtime attach recovery now distinguishes recovery replays from mount-initiated attaches. Recoverable attach failures retry through the transport entry point without resetting the recovery epoch. Non-recoverable errors retain direct error handling. The recovery trigger hook observes runtime status changes and retries parked recoveries when an environment becomes reachable. New tests cover retry scheduling, recovery revival, manual and global triggers, and non-recoverable failures. Merge Risk: ⚪ Minimal · up to This change adds recovery for remote pane attachment failures and retries recovery when runtime reachability returns. No concrete unresolved risk is identified for the current change. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the bug, root cause, implementation, linked issue, tests, and known limitations. However, it omits the required Visual Proof section and does not provide the required template checklist evidence for this behavior change. Resolution Add the required Visual Proof section with before-and-after screenshots or a video. If visual proof is not applicable, write N/A and explain why. Complete the relevant testing and checklist items, including manual-test status and validation commands. Full details: Linked Issues checkExplanation The changes address all coding objectives in issue Full details: Out of Scope Changes checkExplanation The modified transport logic, reachability trigger, and recovery tests directly support issue
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 |
…unctions Satisfies the diff-scoped docstring coverage gate; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one coverage gap worth closing before merge.
Reviewed changes
- Attach-failure recovery replay —
handleRemoteTerminalErrornow routes a recoverable error with no live stream toscheduleConnectRetryAfterRecoverableFailure(), instead of a silentscheduleResubscribeAfterTransportClose()no-op that leftconnectinglatched with nothing armed. - Replay no longer resets the recovery ladder — a
replayingRecoveryAttachflag skipsrecovery.cancel()inattach()(and its async catch) so a replay continues the same epoch rather than re-arming the deadline every attempt. - Environment-reachable trigger —
useRemoteRuntimeRecoveryTriggersnow subscribes toruntimeStatusByEnvironmentIdand firesretryAllRemoteRuntimePtyRecoveriesNow()when an environment transitions to reachable or bumpsconnectionGeneration. - Tests — new
remote-runtime-attach-failure-recovery.test.tsmirrors the connect-side suite and pins backoff arming, parked revival, and the non-recoverable control.
ℹ️ New environment-reachable trigger has no test coverage
The transport changes are well covered, but the hasEnvironmentBecomeReachable predicate and the store subscription it feeds are pure logic with zero unit coverage — the existing use-remote-runtime-recovery-triggers.test.ts was not extended. A regression here (e.g. a false-positive transition that fires retryAllRemoteRuntimePtyRecoveriesNow() on every status write, or a missed connectionGeneration bump) would be invisible to the suite.
Technical details
# Test the environment-reachable trigger
## Affected sites
- src/renderer/src/runtime/use-remote-runtime-recovery-triggers.ts:11-31 — `hasEnvironmentBecomeReachable` (module-private, not exported)
- src/renderer/src/runtime/use-remote-runtime-recovery-triggers.ts:47-56 — store subscription wiring
## Required outcome
- A unit test that asserts `retryAllRemoteRuntimePtyRecoveriesNow` fires on a genuine reachability transition and does NOT fire on unrelated store changes.
## Suggested approach
- Either export `hasEnvironmentBecomeReachable` and test it directly (null → non-null status, absent → present, `connectionGeneration` increase, and the no-op cases: unchanged status, diagnostics-only overlay), or extend the existing `renderHook` test to drive a real `setRuntimeEnvironmentStatus` transition against the store and assert the mock fires.
- Note the store is a module singleton; reset it with `useAppStore.setState(useAppStore.getInitialState(), true)` around the test to avoid cross-test contamination.ℹ️ Nitpicks
- The environment-reachable trigger is global: any environment becoming reachable unlatches or advances backoffs for all parked panes, including those bound to still-unreachable environments (where the retry just fails and re-parks). This matches the existing
online/systemResumedsemantics, so it is consistent, but it is slightly over-broad.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Documentation only — the new commit adds JSDoc blocks to
replayLastTransportEntryPoint,handleRemoteTerminalError,attach,hasEnvironmentBecomeReachable,useRemoteRuntimeRecoveryTriggers, and the test'sinstallUnreachableRuntime. Descriptions accurately match the underlying behavior; no logic changed.
The prior review's coverage-gap note for the environment-reachable trigger still stands and is not addressed by this commit, but this delta introduces no new concerns.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Closing: #18495 was fixed on |
1 participant
Fixes #18495.
The bug
A client that starts while its paired runtime is unreachable attaches to already-open panes rather than creating them, so the failure lands in
attach()rather thanconnect().attach()never setsconnected, so the recoverable branch inhandleRemoteTerminalErrorcallsscheduleResubscribeAfterTransportClose(), which returns at its ownif (destroyed || !connected || !handle)guard. Thereturnafter it skipsconnecting = false,emitRecoveryState()andsurfaceErrorMessage(), sogetRecoveryState()reports'connecting'— which the banner filter hides.Result: a mounted xterm bound to nothing. No text, no error, no retry, forever — including after the runtime returns. The pty keeps its host-side dimensions (~53 cols in the reported case) precisely because no client ever attached.
connect()handles the identical error class by armingscheduleConnectRetryAfterRecoverableFailure(). That asymmetry is the bug.Changes
remote-runtime-pty-transport.tsconnect()does instead of latching silently. A live stream still goes throughscheduleResubscribeAfterTransportClose().attach()cancelled recovery on entry, so each replay restarted the 250ms..30s ladder and the deadline never latched — no'disconnected'phase, so no Reconnect button and no parked retry. Now skipped when the attach is the replay.connect()cancels only on the gone and fatal paths.use-remote-runtime-recovery-triggers.tsonlineand system-resume triggers both miss a tailnet or VPN returning —navigator.onLinenever went false because the LAN never dropped, and there is no power transition. The store already suppresses no-op re-probe writes, so this only fires on a real transition.Net behaviour: the pane retries for the auto-recovery window, then latches to a visible
disconnectedwith a working Reconnect button, and auto-revives when the environment returns.Tests
New
remote-runtime-attach-failure-recovery.test.ts, mirroring the existingremote-runtime-connect-failure-recovery.test.ts(which covers theconnect()side of the same class, #12684):Verified red/green — on unpatched code the first two fail with
expected 'connecting' to be 'backoff'/'disconnected', which is the bug reproduced exactly.What is not covered
The unit tests prove the transport state machine. They do not prove the end-to-end render — the deliberate repro (runtime unreachable → launch client → restore reachability, against a build carrying this patch) has not been run. Worth doing before merge.
🤖 Generated with Claude Code