perf(ssh): cut warm SSH connect from ~97s to ~19s on high-latency links#9015
perf(ssh): cut warm SSH connect from ~97s to ~19s on high-latency links#9015nasagong wants to merge 4 commits into
Conversation
Adds a main-side timeStartupStep helper mirroring the renderer's -done/-failed and durationMs conventions, and wraps the relay establish phases plus the managed-hook install probes. Gated behind ORCA_STARTUP_DIAGNOSTICS=1; these timings located the SSH cold-connect bottlenecks addressed in the follow-up commits.
The 16 per-agent hook installers ran serially, each paying its own SFTP round trips: ~93.5s per connect at 150ms RTT, dominating SSH connect time. Concurrent execution is safe because installers touch disjoint per-agent paths and shared script-dir writes are atomic (tmp+rename). Measured 93.5s -> 11.0s on the same host. Adds a regression test for result order and per-agent failure isolation under concurrency.
Renderer startup only reaches its ssh:connect step after the full hydration chain, serializing SSH handshake and relay deploy behind it. Main now starts the same reconnects at did-finish-load; the renderer's own reconnect joins them via connectInFlight, so state transitions are unchanged. The eager pass waits for the renderer's explicit credential-listener-ready signal and is skipped entirely without one, so an unexpected credential prompt can never fire before its listener exists. Targets that last required a prompt keep deferring to tab focus.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemote managed hook installers now run concurrently while returning ordered per-agent statuses. SSH startup reconnects are gated on renderer credential-listener readiness, exposed through preload and renderer IPC wiring and triggered after the main window loads. Relay establishment and remote hook installation now record startup timing and failure diagnostics. Tests cover installer ordering, reconnect control flow, startup wiring, and updated IPC mocks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
🧹 Nitpick comments (1)
src/main/window/attach-main-window-services.test.ts (1)
234-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the load callback invokes eager reconnect.
This only proves a
did-finish-loadcallback exists and does not throw; an empty callback would pass. Mock or spy oneagerReconnectSshTargetsFromShutdown, invokeloadHandler, and assert exactly one call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3ceb040-30e8-4495-b4c0-947fe7396f72
📒 Files selected for processing (13)
src/main/agent-hooks/remote-hook-service-installers.test.tssrc/main/agent-hooks/remote-managed-hook-installers.tssrc/main/ipc/ssh.test.tssrc/main/ipc/ssh.tssrc/main/ssh/ssh-relay-session.tssrc/main/startup/startup-diagnostics.tssrc/main/window/attach-main-window-services.test.tssrc/main/window/attach-main-window-services.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/hooks/useIpcEvents.test.tssrc/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/web/web-preload-api.ts
…mers Applies CodeRabbit review on stablyai#9015: - whenSshCredentialListenerReady left its waiter in the array after the timeout resolved, and resetSshHandlerStateForTests dropped waiters without clearing their timers, retaining dead callbacks and 10s timer handles past test cleanup. Waiters now carry their timer, remove themselves as the timeout fires, and are cancelled on reset. - The did-finish-load wiring test only asserted the callback did not throw, so an empty callback would have passed. It now mocks the ssh module and asserts eagerReconnectSshTargetsFromShutdown is called exactly once, and not before the event.
|
thank you for the PR! will get it in ASAP |
3 participants
Summary
Fixes #9014
SSH connect time on a ~150ms RTT link drops from ~97s to ~19s (measured, warm relay). Three focused commits:
timeStartupStep(mirrors the renderer helper's-done/-failed/durationMsconventions) wrapping the relay establish phases and hook-install probes. Gated behindORCA_STARTUP_DIAGNOSTICS=1, zero effect when disabled. These timings located the bottleneck below.mkdirpRemotealready tolerates concurrent mkdir. Measured 93.5s → 11.0s.ssh:connectafter its full hydration chain, serializing the SSH handshake behind startup. Main now starts the same reconnects atdid-finish-load; the renderer's own reconnect joins via the existingconnectInFlightdedup, so state transitions are unchanged. The eager pass waits for the renderer's explicit credential-listener-ready signal and is skipped entirely without it (fallback = exactly the old behavior). Passphrase-flagged targets keep deferring to tab focus.Screenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildNew tests: concurrent-installer result order and per-agent failure isolation; eager-reconnect gating (no ready signal → eager skipped entirely, late signal ignored), passphrase-target skip, and renderer join dedup on the in-flight connect. Also manually verified against a real Linux remote over ~150ms RTT across repeated cold starts, including two targets connecting concurrently and session restore with live terminals.
AI Review Report
Reviewed with Claude Code and a Codex adversarial review (final verdict: approve). Main risks checked:
did-finish-loadhook in main.MaxSessions-limited remotes are unaffected.Codex initially flagged that an eager connect could race the renderer's credential-request listener registration and lose a prompt (invisible 120s wait). Resolved deterministically: eager waits for an explicit listener-ready IPC signal and is skipped without it; Codex re-review approved.
Security Audit
ssh:credentialListenerReadyinvoke (signal only, idempotent, no data in or out).Notes
remote-managed-hook-installers.ts) — happy to rebase whichever lands second.npm installof node-pty) is out of scope, tracked in SSH relay: pre-bundle node-pty per platform (VS Code-style) to eliminate npm install on remote #1693.