Repository navigation
perf(startup): stop an unreachable SSH host from gating local terminal restore - #18164
Conversation
📝 WalkthroughWalkthroughThe startup flow adds a dedicated Git-environment barrier for shell-PATH and managed WSL readiness. Renderer hydration uses this barrier before worktree refresh and restores only active-workspace SSH targets synchronously. Other SSH targets reconnect in the background and update deferred state. Browser-session profile fetching no longer blocks terminal restoration. Benchmark tooling adds persisted Git and unreachable SSH fixtures, SSH timing metrics, fixture validation, and process-tree cleanup. Merge Risk: 🔵 Low · up to The PR makes local terminal restoration independent of unreachable inactive SSH hosts and narrows Git startup waiting, substantially reducing startup delays. It is mergeable with explicit owner awareness that overlapping startup attempts could leave stale deferred SSH restoration state, and that the benchmark flag should reject invalid counts. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed rationale, implementation changes, trade-offs, testing, benchmarks, and known failures. However, it omits required template content, including the Linked Issue, Visual Proof or an explicit N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections. Resolution Add the missing template sections. Include a real linked issue after "Fixes #"; add visual proof or write "N/A" with a reason; complete AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist; and retain the existing testing and verification details.
✨ 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 |
There was a problem hiding this comment.
Important
One race in the new deferred-target bookkeeping can re-mark an already-connected background target as deferred, reintroducing the exact "fresh pane instead of reattach" behavior this PR removes.
Reviewed changes
This run reviewed the SSH reconnect partitioning, the new app:awaitGitEnvironmentStartupBarrier git-environment fence, the shellPathReady publication, and the benchmark harness fixture extraction.
- SSH reconnect partitioning —
restoreSshConnectionsForStartupnow awaits only the active workspace'sblockingConnectionIdsand dials the rest in the background without a timeout, clearing the deferred flag on connect. - Active-workspace target derivation —
collectActiveWorkspaceSshTargetIdsnames the blocking set from the active repo'sconnectionIdplus SSH PTY ids embedded in restored tabs and split leaves. - Git-environment barrier — a new
app:awaitGitEnvironmentStartupBarrierIPC fences worktree hydration onshellPathReady+managedWslCliStartupBarrierReady, andstate.shellPathReadyis now published frominitializeMainProcessRuntimeLaunch. - Benchmark harness — fixture builders extracted to
startup-bench-state-fixture.mjs,repoRootfixed, and process-group teardown corrected.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
…l restore An asleep or unreachable SSH target held the terminal-restoration gate for the full 15s reconnect timeout, so no terminal restored — local ones included. Startup now awaits only the target that owns the active workspace's tabs and lets the rest connect in the background, folded into the existing deferred path that reattaches their PTYs on tab focus. Also splits the renderer's git-environment fence out of the first-window PTY services barrier: worktree hydration needs shell-PATH generation and the managed WSL CLI registration, not a daemon PTY spawn or a hook-server bind. Terminal restoration still fences on the first-window services via app:prepareTerminalStartupRestoration. Measured with tests/tools/benchmarks/startup-time-bench.mjs (382 restored tabs, 28k-file profile, medians of 3): unreachable SSH host: 17.27s -> 1.34s to renderer-startup-hydration-done all-local: 1.98s -> 1.33s
…d background SSH target undeferred app-startup-routing.test.ts pinned the old step names, so the two ordering cases went vacuous-then-red when the barrier split. Repoint them at the steps that now carry the same fences: 'git-environment-barrier-await' (shell PATH + managed WSL, the fence host Git needs) before hydration worktrees, and 'prepare-terminal-startup-restoration' (which awaits firstWindowStartupServicesReady in main) before terminal reconnect. Both still fail against main's hydration source. Also: the timed-out-eager rewrite of the deferred list re-added background targets that had already connected, undoing removeDeferredSshReconnectTarget and sending fresh panes on a reachable host down the cold-restore path.
09b88ba to
c8e5519
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the fix for the deferred-target bookkeeping race and the startup-ordering oracle updates.
- Deferred re-add fix —
restoreSshConnectionsForStartupnow accumulates connected background targets in aconnectedBackgroundTargetIdsset and filters them out of the timed-out deferred rewrite, so a reachable background host no longer gets pushed back into the deferred list. - Race regression test — a new unit test proves a connected background target is not resurrected into the deferred list when a blocking target times out, and it fails against the prior replace-all behavior.
- Ordering oracle updates —
app-startup-routing.test.tsnow pins the renamedgit-environment-barrier-awaitandprepare-terminal-startup-restorationstages against the hydration chain, matching the source.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: aba3c3e7-9d7f-4016-893e-f171f53ff730
📒 Files selected for processing (17)
src/main/startup/desktop-startup-ordering.test.tssrc/main/startup/main-process-ipc-bootstrap.tssrc/main/startup/main-process-runtime-launch.tssrc/preload/api/app-api.tssrc/preload/api/app-bridge.tssrc/renderer/src/app-shell/startup-actions-selector.test.tssrc/renderer/src/app-shell/startup-actions-selector.tssrc/renderer/src/app-shell/use-app-startup-hydration.tssrc/renderer/src/app-startup-routing.test.tssrc/renderer/src/startup/active-workspace-ssh-targets.test.tssrc/renderer/src/startup/active-workspace-ssh-targets.tssrc/renderer/src/startup/ssh-startup-reconnect.tssrc/renderer/src/startup/startup-ssh-connection-restore.test.tssrc/renderer/src/startup/startup-ssh-connection-restore.tssrc/renderer/src/web/preload-api/web-app-api.tstests/tools/benchmarks/startup-bench-state-fixture.mjstests/tools/benchmarks/startup-time-bench.mjs
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| case '--ssh-unreachable-targets': | ||
| args.sshUnreachableTargets = Number(next()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate --ssh-unreachable-targets as a bounded integer.
Number() accepts Infinity, fractions, and negative values. Infinity makes buildUnreachableSshTargets() loop until the process fails. Fractional values make the recorded count differ from the target count. Values above 254 exceed the fixture address range.
Reject values that are not safe integers in the range 0..254, or generate valid addresses across additional ranges.
Proposed fix
case '--ssh-unreachable-targets':
- args.sshUnreachableTargets = Number(next())
+ args.sshUnreachableTargets = Number(next())
+ if (
+ !Number.isSafeInteger(args.sshUnreachableTargets) ||
+ args.sshUnreachableTargets < 0 ||
+ args.sshUnreachableTargets > 254
+ ) {
+ throw new Error('--ssh-unreachable-targets must be an integer from 0 to 254')
+ }
break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case '--ssh-unreachable-targets': | |
| args.sshUnreachableTargets = Number(next()) | |
| case '--ssh-unreachable-targets': | |
| args.sshUnreachableTargets = Number(next()) | |
| if ( | |
| !Number.isSafeInteger(args.sshUnreachableTargets) || | |
| args.sshUnreachableTargets < 0 || | |
| args.sshUnreachableTargets > 254 | |
| ) { | |
| throw new Error('--ssh-unreachable-targets must be an integer from 0 to 254') | |
| } | |
| break |
|
Readiness review (batch G). CI was red on this branch before the rebase — Both cases pinned What I changed (2 commits squashed into one on top of yours, then rebased onto main):
Independently verified from your claims:
Local after rebase: 526 tests pass across One residual, not blocking: a background target on an older/wrapped provider whose |
…l restore (stablyai#18164) * perf(startup): stop an unreachable SSH host from gating local terminal restore An asleep or unreachable SSH target held the terminal-restoration gate for the full 15s reconnect timeout, so no terminal restored — local ones included. Startup now awaits only the target that owns the active workspace's tabs and lets the rest connect in the background, folded into the existing deferred path that reattaches their PTYs on tab focus. Also splits the renderer's git-environment fence out of the first-window PTY services barrier: worktree hydration needs shell-PATH generation and the managed WSL CLI registration, not a daemon PTY spawn or a hook-server bind. Terminal restoration still fences on the first-window services via app:prepareTerminalStartupRestoration. Measured with tests/tools/benchmarks/startup-time-bench.mjs (382 restored tabs, 28k-file profile, medians of 3): unreachable SSH host: 17.27s -> 1.34s to renderer-startup-hydration-done all-local: 1.98s -> 1.33s * fix(startup): restore the startup-ordering oracle and keep a connected background SSH target undeferred app-startup-routing.test.ts pinned the old step names, so the two ordering cases went vacuous-then-red when the barrier split. Repoint them at the steps that now carry the same fences: 'git-environment-barrier-await' (shell PATH + managed WSL, the fence host Git needs) before hydration worktrees, and 'prepare-terminal-startup-restoration' (which awaits firstWindowStartupServicesReady in main) before terminal reconnect. Both still fail against main's hydration source. Also: the timed-out-eager rewrite of the deferred list re-added background targets that had already connected, undoing removeDeferredSshReconnectTarget and sending fresh panes on a reachable host down the cold-restore path.

ELI5
If one of your remote machines was asleep, Orca waited up to 15 seconds for it before bringing back any of your terminals — including the local ones that had nothing to do with that machine. Now it only waits for the remote machine whose terminals you're actually looking at. The rest connect quietly in the background and reattach to the exact same running sessions when you click their tabs.
What was wrong
use-app-startup-hydration.tsgates terminal restoration behindsetTerminalStartupRestorationReady(true). Three things sat inside that gate that did not need to.1. SSH reconnect awaited every target, not just the one in view.
restoreSshConnectionsForStartupawaitedssh.connectfor every non-passphrase target that was live at shutdown (SSH_RECONNECT_TIMEOUT_MS = 15_000), then serially polledssh.getStateper target. Only then did recover-legacy-worker-terminals, snapshot-capabilities, and reconnect-terminals run, and only then did the gate open. One asleep host cost 15 s before any local terminal restored.2. Git worktree hydration was fenced on the PTY daemon spawn.
awaitFirstWindowStartupServices()beforefetch-hydration-worktreesresolvesPromise.all([firstWindowStartupServicesReady, managedWslCliStartupBarrierReady]), andfirstWindowStartupServicesReadyis the daemon-PTY-provider start and the agent-hook-server bind. The git scan needs neither.3.
fetch-browser-session-profileswas awaited on the gate. On a remote runtime that action iscallRuntimeRpc('browser.profileList', …, { timeoutMs: 15_000 })— a second 15-second timeout on the terminal-restoration path.What changed
restoreSshConnectionsForStartuptakesblockingConnectionIds. Only those are awaited; every other eager target is registered as deferred up front and itsssh.connectis fired without a timer.collectActiveWorkspaceSshTargetIds(new, unit-tested) derives the blocking set from the active workspace's repoconnectionIdplus the SSH target embedded in its restored PTY ids — the PTY id is used because SSH worktrees are absent fromworktreesByRepoat cold start.deferredSshReconnectTargets, because a still-deferred connected target sends fresh panes downstartFreshColdRestoreAgentResumeinstead of the normalrunDeferredSessionReattachChoice. Without this, a reachable host would behave differently than it does today.app:awaitGitEnvironmentStartupBarrierIPC =Promise.all([shellPathReady, managedWslCliStartupBarrierReady]).state.shellPathReadyis now published frominitializeMainProcessRuntimeLaunch(the field existed but was never assigned). Worktree hydration uses this;app:prepareTerminalStartupRestorationstill awaits the first-window services before anything is restored.fetch-browser-session-profilesstarts at the same point in the chain but is no longer awaited.Corrections to the reported findings
The shell-PATH claim needed checking on Windows, and the conclusion is stronger than reported. On non-Windows,
main-process-runtime-launch.ts:302awaitsshellPathReadybefore the window opens, so it is already resolved — as reported. But on packaged Windows the window opens beforeshellPathReadyresolves (startWindowsDesktopBeforeShellPathReady), andfirstWindowStartupServicesReadyisshellPathReady.then(startServices).then(v => v.firstWindowReady)— so there the old await genuinely was the shell-PATH fence, and removing it outright would have been the "broken git environment" trade the task warned about. Rather than keep a platform-conditional await, this PR keeps an explicitshellPathReadyfence on the git path.Separately,
configureWindowsHostGitEnvironmentReadinessalready makes every host-Git spawn await the same fence internally (prepareWindowsHostGitEnvironment, called fromgit-exec-file.ts,git-spawn.ts,git-stream-stdout.ts), so this is belt-and-braces. It is kept because relying on a fence three modules away is not something a future reader should have to rediscover.The duplicate
recoverLegacyWorkerTerminalsis load-bearing — left in place. The pre/post pair was added together in #11271. UndermaterializeRenderer: true, the pre-reconnect run callsports.reveal(candidate), which materializes a legacy worker's surface in the renderer store.reconnectPersistedTerminalsthen readstabsByWorktree— so the pre run can change what reconnect sees. Removing it is not a no-op and it is not removed here. (startup-degraded-recovery.ts:89-95keeps the same pre/post pair, confirming the intent.)The
onboardingPromiseawait is not a round trip.window.api.onboarding.get()is started at the top of the chain (line 98) and is overlapped by ~13 awaited stages before it is joined. Measured cost at the join point is ~0 ms, and moving the onboarding callback past the gate would delay the onboarding modal on a first run for no gain. Left alone.Measurements
tests/tools/benchmarks/startup-time-bench.mjs, 382 restored tabs, 28k-file synthetic profile,--wait-for-event renderer-startup-hydration-done, medians of 3 iterations, macOS arm64. Baselines were re-run back-to-back with the "after" runs because this machine's load varies; both baseline sets are shown.Unreachable SSH host (
--ssh-unreachable-targets 1, target on 203.0.113.1 — RFC 5737 TEST-NET-3, so the TCP handshake never completes and never resets, the wire behaviour of a sleeping host):renderer-ssh-reconnectAll local, no SSH:
The barrier line is finding 2 in isolation: 168–213 ms → 2–3 ms, because the daemon spawn now overlaps the worktree scan instead of preceding it. On this fixture the git scan is one small repo; at 10 repos / 423 worktree entries the overlap is larger, not smaller.
Raw results:
tests/tools/benchmarks/results/startup-{base2-local,base2-unreachable-ssh,after-local,after-unreachable-ssh}-*.json.Benchmark harness fixes needed to take these numbers
repoRootwasresolve(scriptDir, '..', '..'), correct when the script lived attools/benchmarks/but stale since chore: declutter top-level repo layout #11890 moved it totests/tools/benchmarks/. It resolved to<root>/tests, so the harness always failed itsout/main/index.jsprecondition.daemon-coldstart-bench.mjsandmain-thread-jank-bench.mjscarry the identical stale constant and are presumably also unrunnable; not touched here.killProcessTreeSIGKILLed only the direct child on POSIX. Electron's helper processes inherit the stderr pipe, so the harness never saw EOF and hung forever after its last iteration — the results JSON was written but nothing printed. Now spawnsdetachedand kills the process group.--ssh-unreachable-targets Nto seed the unreachable-host case, andrendererSshReconnectMs/rendererStartupBarrierMsto the phase table.startup-bench-state-fixture.mjsto keep the harness undermax-lineswithout a disable.Negative user-facing trade-offs
Non-active SSH targets now connect on tab focus rather than during boot. This is a real behaviour change, stated plainly:
reconnect-terminals(it is fired earlier than the old awaited group, and two IPC stages run in between), sosshConnectionStatesreadsconnectedand restoration is byte-identical to today. The target is also removed fromdeferredSshReconnectTargetson connect, so panes with no restored session take the same non-deferred code path they take today.waitForSshConnectionjoins the same in-flightssh.connect(sshConnectPromisesdedupes;ssh-connect-flow.ts:59serialises concurrent connects for one target in main) and reattachespendingSessionId— the same PTY id, the same relay, the same replay buffer. The user sees the parked snapshot for the extra few hundred ms instead of a blank pane. This is not a new path: it is exactly what a timed-out eager target has always done, and what every passphrase target has always done.What cannot regress:
reconnectPersistedTerminalswrites the session id intodeferredSshSessionIdsByTabIdand leavestab.ptyIdandptyIdsByTabIdset. Nothing clears a PTY id. A deferred id reads as liveness to the orphan sweep, so deferring is the safe direction (see the [Bug] Remote - Terminal window auto-closing. Unable to recover #9911 comment inworkspace-terminal-reconnect.ts).refreshTerminalProviderSnapshotCapabilitiesreturnsauthoritative: null(unknown) for a PTY on an unconnected target and keeps the pane mounted and eviction-exempt; main's legacy-worker recovery classifies an unreachable adoption asunverifiableand defers it. Loss of contact staysunverifiable, neverexited.use-terminal-watcher-effects.ts:142,worktree-agent-activation-gate.ts:53), so an early click is only an active-workspace change. After the gate opens, the pane runs the deferred flow and joins the in-flight connect. The gate opens ~15 s sooner, so this window is now shorter, not longer.golden-worktree-create-switch(which creates a worktree via host Git) passes.One deliberate asymmetry: the
ssh.getStatecompatibility poll ("older/wrapped providers may return no state from connect") still runs only for awaited targets. A background target on such a provider gets its state from the ordinaryssh:stateChangedpush, and the pane'swaitForSshConnectionresolves onssh.connectreturning without throwing regardless of the state value, so it cannot strand.Reliability contract
terminal-restore.pty-identity— every terminal that would have been restored is restored with the same PTY id, the same replay/scrollback, and the same live/parked state, regardless of which SSH targets were reachable at boot.ssh.verdict-vocabulary— a deferred reconnect never yieldsexited.golden-quit-relaunch-session.spec.ts(the exact file and the live extra terminal survive quit → relaunch) passes on this branch. Deterministic layer:startup-ssh-connection-restore.test.tsproves an unreachable non-active target does not extend the awaited window while an active one still does; 3 of its 5 cases fail onmainand the 2 that pass are the await-everything controls.config/reliability-gates.jsoncentry covers renderer startup SSH partitioning.node config/scripts/check-reliability-gates.mjspasses (100 gates). Recorded as an accepted gap rather than a new gate.managedWslCliStartupBarrierReadystays on the git path. Mobile/relay — unaffected;web-app-api.tsresolves the new barrier immediately, as it does the existing one. macOS — measured. Linux/Windows — reasoned, not run; the Windows-specific risk is the shell-PATH fence, addressed above and additionally covered by a structural test.timeoutMsis now optional and omitted for connects nobody awaits). One IPC round trip added on the git path, one removed from the gate.renderer-ssh-reconnect-donenow reportsbackgroundTargetsalongsideeagerTargets/deferredTargets;renderer-git-environment-barrier-await-donereplacesrenderer-first-window-services-await-done; the bench surfaces both.Verify
pnpm tc— clean.npx oxlint <changed files>— clean;oxfmt --checkclean.npx vitest run --config config/vitest.config.ts src/renderer/src/app-shell src/renderer/src/startup src/main/startup— 435 passed, 1 skipped file.ssh-pane-connect-gate,pty-connection-deferred-ssh-passphrase,pty-connection-parked-ssh-snapshot,ssh-reattach-model-restore,pty-connection-direct-ssh-reattach-retry,ssh-reconnect-model-paint-gate,store-session-terminal-reconnect,src/renderer/src/store/terminals— 88 passed.golden-quit-relaunch-sessionandgolden-worktree-create-switche2e pass.golden-terminal-file-linkfails 2 tests ("Monaco opened a different file identity") — verified pre-existing: the identical two failures reproduce on a baseline build ofmainwith no changes applied.