Repository navigation
fix(browser): a failed SSH route keeps its card while the host redials - #23465
Conversation
A browser route that already failed swapped its "SSH connection unavailable" card for "Connecting" on every dial of its host, and re-ran prepare once per dial cycle, so the card flickered and its buttons detached mid-click. Only a route that is still preparing now waits on a dialing host; a failed route keeps its card until the host actually connects, which re-derives it. Retry and Try anyway land on preparing together with the new attempt, so a press while the host dials waits for the connect instead of starting a prepare the effect immediately cancels. The escape-hatch e2e now makes the host truly unreachable before the disconnect; it passed before only because the card stayed latched over a host the terminal had already reconnected.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe browser route now keeps an existing error visible during host reconnects. Retry and “Try anyway” set the route to preparing, and preparation waits until the host connects. Unit tests cover error persistence and recovery, including a route that becomes SSH-routed during a dial. End-to-end coverage configures the SSH relay to refuse connections before testing the local-browsing escape hatch. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Switching SSH targets during a connection attempt can show the wrong unavailable card and start preparation too early. The unavailable-host test also does not establish that the intended redial occurred. Correct these paths before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to After a workspace changes SSH hosts, a failure card from the old host can briefly appear for the new one while it connects. The card may offer a local-browsing choice for the wrong host. A browser page is not shown automatically, but the host-switch behavior warrants review. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 140750cb-2380-4491-af55-6aa2d01a6cf8
📒 Files selected for processing (4)
src/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.host-connection.test.tsxsrc/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.tstests/e2e/helpers/docker-ssh-relay-target.tstests/e2e/local-ssh-browser-routing.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| if (result.status !== 0 && !/permission denied/i.test(lastProbe)) { | ||
| return |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Require sustained refusal after the sshd reload.
The first SSH probe can fail while sshd closes and reopens its listening sockets on SIGHUP. This condition returns immediately, even if sshd then binds an externally reachable address. The E2E test can disconnect a relay that is about to accept the automatic redial. Confirm refusal after the reload has settled, rather than returning on one failed probe. (github.com)
🧰 Tools
🪛 ast-grep (0.45.3)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync, spawnSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
| await expect | ||
| .poll(async () => (await readSshState(orcaPage, remote.targetId)).status, { | ||
| timeout: 180_000, | ||
| message: 'the redial of an unreachable host must settle as a failure, not an auth prompt' | ||
| }) | ||
| .toMatch(/^(disconnected|error|reconnection-failed)$/) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '550,685p' tests/e2e/local-ssh-browser-routing.spec.ts
rg -n 'reconnection-failed|readSshState|reconnect' tests/e2e/local-ssh-browser-routing.spec.ts src/renderer/src/lib/worktree-host-connection-phase.tsRepository: stablyai/orca
Length of output: 8875
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test imports and helpers ---'
sed -n '1,135p' tests/e2e/local-ssh-browser-routing.spec.ts
printf '%s\n' '--- relay/refusal helper definitions and uses ---'
rg -n -C 8 'refuseDockerSshRelayTargetConnections|startDockerSshRelayTarget|connectDockerSshRelayTarget|startSshRemoteOnlyBrowserFixture|readSshState|ssh\.disconnect|reconnection-failed' tests/e2e src
printf '%s\n' '--- relevant SSH state and reconnect implementation ---'
rg -n -C 12 'disconnect\(|reconnect|reconnection-failed|reconnection|connection generation|status.*disconnected|status.*error' src/renderer src/main packages 2>/dev/null || true
printf '%s\n' '--- all assertions in the unavailable-host test ---'
sed -n '585,665p' tests/e2e/local-ssh-browser-routing.spec.tsRepository: stablyai/orca
Length of output: 45658
🏁 Script executed:
set -eu
rg -n -C 12 'refuseDockerSshRelayTargetConnections|startDockerSshRelayTarget|connectDockerSshRelayTarget|startSshRemoteOnlyBrowserFixture' tests/e2e
rg -n -C 12 'function readSshState|const readSshState|async function readSshState|ssh\.disconnect|reconnection-failed|reconnect' tests/e2e/local-ssh-browser-routing.spec.ts src
sed -n '70,125p' tests/e2e/local-ssh-browser-routing.spec.ts
sed -n '610,650p' tests/e2e/local-ssh-browser-routing.spec.tsRepository: stablyai/orca
Length of output: 45646
🏁 Script executed:
set -eu
printf '%s\n' '--- helper file ---'
helper=$(rg --files tests/e2e | rg '/docker-ssh-relay-target\.ts$' | head -n 1)
printf 'helper=%s\n' "$helper"
cat -n "$helper"
printf '%s\n' '--- exact SSH symbols ---'
rg -n -C 10 'reconnectAttempt|connectionGeneration|reconnection-failed|ssh:disconnect|disconnect.*targetId|function.*disconnect|const.*disconnect' src/main src/renderer src/shared shared tests/e2e --glob '*.{ts,tsx}' --glob '!**/*.test.tsx' --glob '!**/*.test.ts' 2>/dev/null | head -n 1200Repository: stablyai/orca
Length of output: 42706
🏁 Script executed:
set -eu
printf '%s\n' '--- SSH disconnect bindings ---'
rg -n -C 14 'name:\s*["'\'']ssh\.disconnect|ssh\.disconnect|disconnectSsh|disconnect.*targetId|intentionallyClosed' src/main src/renderer src/shared tests/e2e --glob '*.{ts,tsx}' --glob '!**/*.test.ts' --glob '!**/*.test.tsx' 2>/dev/null | head -n 1600
printf '%s\n' '--- reconnectAttempt transitions ---'
rg -n -C 14 'reconnectAttempt' src/main src/renderer src/shared --glob '*.{ts,tsx}' --glob '!**/*.test.ts' --glob '!**/*.test.tsx' 2>/dev/null | head -n 1600Repository: stablyai/orca
Length of output: 42069
🏁 Script executed:
set -eu
printf '%s\n' '--- disconnect implementation ---'
rg -n -C 30 'disconnectRegisteredSshTarget|manualDisconnect|intentionallyClosed|invalidateConnectAttempt' src/main --glob '*.{ts,tsx}' | head -n 1800
printf '%s\n' '--- SSH retry scheduler ---'
rg -n -C 30 'reconnectAttempt|schedule.*reconnect|reconnect.*schedule|retry.*connect|reconnection-failed' src/main --glob '*ssh*.ts' --glob '*.{ts,tsx}' | head -n 1800Repository: stablyai/orca
Length of output: 42274
Trigger the SSH loss path before asserting redial failure.
The test calls explicit ssh.disconnect. That teardown invalidates pending transport reconnects, so this sequence does not exercise automatic redial. The relay refusal affects only new connections, and the current status polls can pass on the initial disconnected state.
Trigger a real transport drop, then assert a non-zero reconnectAttempt before checking the terminal failure state.
Suggested test change
- await orcaPage.evaluate(
- async (targetId) => window.api.ssh.disconnect({ targetId }),
- remote.targetId
- )
+ expect(
+ killSshRelayTargetTransport(target),
+ 'the container had no established SSH session to drop'
+ ).toBeGreaterThan(0)
await expect
.poll(async () => (await readSshState(orcaPage, remote.targetId)).status, {
timeout: 60_000,
message: 'the SSH target never left the connected state'
})
.not.toBe('connected')
+ await expect
+ .poll(async () => (await readSshState(orcaPage, remote.targetId)).reconnectAttempt ?? 0, {
+ timeout: 180_000,
+ message: 'the SSH target never attempted an automatic redial'
+ })
+ .toBeGreaterThan(0)![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Narrowed the host-wait gate.
awaitingHostnow requiresstate.kind === 'preparing', so a dial holds only a route that has not settled; an already-failed route keeps itsssh-unavailablecard and still re-derives when the host connects on a new epoch. - Retry / Try anyway land on
preparingwith the new attempt. The newrederivehelper writespreparingin the same update as the attempt bump, so the prepare effect's first run sees a dialing host and waits rather than starting a prepare its own preparing write cancels. - Test expectation flipped and two cases added. A dial now keeps the error card (was
preparing); new tests pin card identity acrossconnecting/unavailablecycles with a single prepare, and Retry-while-dialing preparing exactly once on connect. Both fail onmain, and the Retry test fails if thepreparingwrite is removed. - E2E fixture correction.
refuseDockerSshRelayTargetConnectionsmakes the fixture sshd loopback-only (self-validated by a real ssh probe), and the SSH-unavailable spec now disconnects a genuinely unreachable host and asserts it is notconnectedbefore exercising the escape hatch.
I traced the one path where the new condition differs from the old !routeReady for reasons other than a settled error: a targetId appearing (state.kind === 'unrouted') or a stale ready for a previous target, while the host is dialing. In those cases the effect does issue a prepare for the still-dialing host, but it cannot surface: effectiveState forces preparing in-render so the gate mounts no webview, the write is keyed to the current targetId, and the outcome matches what the old code produced once the phase reached unavailable. No wrong card or wrong-egress mount.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Only a failed or ready route is exempt from the host wait; a route that just became routed (or still shows another target's page) has no answer for this target, so it must not start a prepare that its own preparing write cancels. The escape-hatch spec now reads the settled failure from the renderer store: main's ssh:getState drops the entry on disconnect and on a failed connect, so its status is null there and never matches the failure pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 985b4e57-1817-465e-b100-80b19baa4572
📒 Files selected for processing (3)
src/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.host-connection.test.tsxsrc/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.tstests/e2e/local-ssh-browser-routing.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| // Unrouted and another target's page are unsettled too: neither answers for this target. | ||
| const routeReady = state.kind === 'ready' && state.targetId === targetId | ||
| const awaitingHost = routeHost?.phase === 'connecting' && !routeReady | ||
| const awaitingHost = routeHost?.phase === 'connecting' && state.kind !== 'error' && !routeReady |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,220p' src/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.ts
sed -n '125,225p' src/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.host-connection.test.tsxRepository: stablyai/orca
Length of output: 12311
Associate route errors with their target.
When target A fails and the route changes to target B, state still contains A’s error. If B is connecting, state.kind === 'error' makes awaitingHost false, so the effect can call prepareSshWorkspacePartition before B connects. effectiveState can also expose A’s error while B is the active target.
Store the error’s targetId. Exclude the error from the host wait only when it belongs to the current target. Otherwise, return preparing and wait for B’s connection.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Broadened the host-wait gate.
awaitingHostis nowrouteHost?.phase === 'connecting' && state.kind !== 'error' && !routeReadyinstead of the prior revision'sstate.kind === 'preparing'. Only a settled failure keeps its card through a dial; an unrouted route and a stalereadyfor another target wait again rather than preparing against a dialing host. This is a strict subset of main's!routeReadywait plus the error carve-out, so it cannot add a wait main lacked — it only fixes the two edges the previous revision dropped. - Pinned the unrouted-mid-dial path with a test.
waits for a dialing host when the route becomes routed mid-dialdrivesexecutionHostIdfrom null to a target while the host isconnecting, assertingpreparingwith no prepare, then exactly one prepare andreadyon connect.mocks.executionHostIdwas made mutable (reset per test) to allow it. - Fixed the e2e settlement poll's source. The poll before the escape hatch now reads the renderer store via the new
readRendererSshStatusinstead of main'sssh:getState, which drops a target's entry on disconnect and on a failed connect — so the previous poll could never observe a settled failure.
ℹ️ PR description still names the superseded condition
The Mechanism paragraph describes the wait as phase === 'connecting' && state.kind === 'preparing', which was true only for the first commit. The head condition is state.kind !== 'error' && !routeReady. The rest of the description (the before/after and the Retry rationale) still holds; only that one expression is stale.
Technical details
# Update the PR description's mechanism sentence
## Affected sites
- PR body, "**Mechanism:**" paragraph — names `phase === 'connecting' && state.kind === 'preparing'`.
- `src/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.ts:75` — actual head condition is `routeHost?.phase === 'connecting' && state.kind !== 'error' && !routeReady`.
## Required outcome
- The description's mechanism sentence matches the code on the branch, so a reviewer does not infer a narrower wait than the code implements.
## Suggested approach (optional)
- Replace the condition in the `Mechanism` sentence with `phase === 'connecting' && state.kind !== 'error' && !routeReady` and keep the surrounding Retry rationale.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready to mergeHead: Review loops: One independent review loop came back clean. It traced every route state (failed, preparing, ready, unrouted, a target switch, and "unverifiable" or remote-runtime-owned hosts) and confirmed the ablations: main's condition fails 2 of the new tests, and removing the "preparing" write on Retry fails the Retry test. The loops never repeated or passed three, so no separate architecture pass was triggered. Architecture: Before this PR, one condition answered two questions: whether the route has an answer yet, and whether the host is dialing. So every redial reset a route that had already failed. The fix narrows that condition in place instead of adding another guard. A dial holds only a route that has no answer yet, and a new connection still re-derives a failed route automatically. Route state stays per tab, derived from the shared host-connection signal. Moving it to one owner per SSH target would let tabs share a single prepare, but that is a design choice that predates this PR and isn't needed for this fix. Readiness checklist: Run at the start and at the end, and both PASS with no P0, P1 or P2 findings. It covers SSH/network, crash and retry, security, performance, correctness, backward compatibility and mobile (no mobile-facing surface touched), cross-platform, and code organization. The PR merges cleanly onto current main, and main has not changed the touched files. Validation:
Gaps:
Existing issue found during QA, not caused by this PR:
|
ELI5
On a workspace hosted over SSH, a browser tab that could not reach the host shows an "SSH connection unavailable" card with Retry and "Browse from this device instead". Since #22810, that card flickered away every time another part of the app started redialing the host, so the buttons could vanish under your cursor. Now the card stays put while the host is being dialed, and the tab still recovers by itself once the host actually connects.
What Changed
Before: #22810 made SSH browser tabs wait while their host connects, so a tab opened during startup no longer latched "SSH connection unavailable". But the wait also applied to a tab that had already failed. When the host started dialing again, for example because a terminal in the same workspace redialed it, the error card was swapped for "Connecting through the SSH host" and the route was prepared again. The card and its buttons unmounted and remounted once per dial attempt.
After:
Mechanism: in
use-ssh-workspace-browser-route.ts, the "awaiting host" condition is nowphase === 'connecting' && state.kind !== 'error' && !routeReady, where main hadphase === 'connecting' && !routeReady. A route that already failed, or a page already ready for this target, does not wait on a dial. Anything unsettled does: a route mounted mid-dial, one just switched from unrouted to routed, or a page still showing another target. Retry and "Try anyway" set the route to preparing in the same update as the new attempt, so the prepare effect's first run already sees whether the host is dialing, and no prepare is started and then cancelled.E2E spec correction (
local-ssh-browser-routing.spec.ts): the "holds the mount and offers a working local escape hatch" test disconnects SSH and expects the unavailable card. In every CI run, including the passing ones before #22810, the workspace's terminal redialed the host about 0.4 s after the disconnect and it reconnected. Before #22810 the test passed only because the card stayed stuck on screen over a host that was connected again, which is the stuck state #22810 removed. Now that the route correctly recovers when the host comes back, the test makes the host genuinely unreachable before disconnecting (sshd stops accepting new connections, and the container stays up for the rest of the test). It also asserts the host is not connected right before using the escape hatch, so it can no longer pass on a reconnected host. This is a fix to the test, not a weakening. The redial after an explicit Disconnect is a separate product bug, fixed in its own PR.Why
The card flickered because one condition was answering two different questions: "has this route got an answer yet?" and "is the host being dialed?" A dial is a transient. It should hold a route that is still waiting for its first answer, but it should not take back an answer the user is already looking at. Keying the wait on the route's own unsettled state keeps #22810's startup fix and removes the flicker, without adding new state.
Alternatives considered:
Linked Issue
Follow-up to #22810. CI signal this closes:
e2e / ssh docker watcher isolation (1/4),local-ssh-browser-routing.spec.ts:552, which passed 8/8 before #22810 and fails on every run containing it (for example runs 36342176474 and 36346378299). It also reddens other PRs whose changes trigger that lane.Visual Proof
Live run against a real Linux SSH host, reached through a local forwarder that could be stopped to make the host unreachable. Both builds ran as background dev apps. "Before" is this branch's merge base (it has #22810 but not this fix).
Testing
New unit test: a failed route keeps its exact error card through connecting / unavailable / connecting / unavailable with a single prepare, then recovers to ready on connect. It fails on main at the first
connecting.New unit test: Retry while the host dials waits and prepares exactly once on connect.
New unit test: a route that becomes routed while the host dials waits for the connect instead of starting a prepare that would fail.
Updated expectation: during a dial, an ssh-unavailable route keeps its error card instead of switching to preparing.
Ablations: main's condition reddens 2 tests; the earlier
state.kind === 'preparing'condition reddens the routed-mid-dial test; removing the preparing write in Retry reddens the Retry test.The e2e spec now polls the renderer's SSH state (what the panes read). The main process drops a target's entry on disconnect or a failed connect, so polling it would read nothing in exactly the state being waited for.
pnpm tc:web,oxlint,check:code-quality:changed,check:react-doctor:changedare clean.Live: the Docker SSH e2e lane in this PR's CI runs the corrected spec, and all four shards pass.
Live SSH run, before and after builds, each starting from the same state:
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Author X handle: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)