Repository navigation
fix(ssh): saved port forwards come back after a restart that reconnects through the host's Orca server (#32484) - #33388
Conversation
…tunnel restores saved forwards (#32484)
… not only after an SSH connect (#32484) A host whose managed server the app reaches at startup through its tunnel publishes its served status from the tunnel path, which never ran the forward restore. The served status itself now triggers the restore, from any publisher, and a host kept on the legacy relay bridge restores too.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 41 seconds. View limit details
Walkthrough
Priority: Severity of issue fixed: Medium Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 37.04% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 18 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: restoring saved SSH port forwards after an Orca restart and host reconnect. Linked Issues check Issue #32484requires saved SSH port-forward mappings to remain and work after Orca restarts and reconnects to the remote host. The whole-PR summary shows these changes for that requirement:
- src/mai…Out of Scope Changes check Every production change in the summary relates to restoring or preserving saved SSH port forwards after restart, which is the scope of #32484:
- Restore triggers: ssh-host-server-connect.ts (removed r…Description check The description follows the template well. It has an ELI5, a detailed What Changed list, a Why section, a linked issue (Fixes #32484), a Visual Proof section marked N/A with a reason, a Testing sectio…Full details: Linked Issues check
-
Explanation
Issue
#32484requires saved SSH port-forward mappings to remain and work after Orca restarts and reconnects to the remote host. The whole-PR summary shows these changes for that requirement:- src/main/ssh/ssh-host-server-status.ts adds a served-status listener. ssh.ts restores saved forwards when a host reports a served status (managed, or relay with bridged === true).
- runtime-environment-managed-tunnel.ts restores saved forwards after the managed tunnel comes up, with tunnelUp: true. This covers startup when no SSH connect runs and no status is published.
- ssh-managed-port-forward-restore.ts restores once per transport and again after a restored forward closes. Failed binds are retried with backoff (30 s start, 10 min cap). Unsettled statuses do not restore.
- ssh-port-forward-persistence.ts and ssh-port-forward-handlers.ts keep unrestored saved forwards on add, edit, and remove. Edits and removals run through one per-host queue.
Tests cover these paths. Unit tests cover served status, setting-up status, tunnel-only restore, transport and retry behavior, persistence on add/remove, local-port reuse, and edit/restore ordering. The Docker e2e cases in ssh-orcad-workspace-tools.spec.ts cover restart with an SSH connect and restart with every ssh:connect forbidden. The PR reports that the tunnel-only case fails on main and passes on the final head. The previous PASS remains supported by the current summary. The issue does not require a platform-specific fix. Verification was on Linux only. This is a residual risk, not a missing requirement.
Full details: Out of Scope Changes check
-
Explanation
Every production change in the summary relates to restoring or preserving saved SSH port forwards after restart, which is the scope of
#32484:- Restore triggers: ssh-host-server-connect.ts (removed restore call), ssh.ts, ssh-host-server-status.ts, runtime-environment-managed-tunnel.ts.
- Restore logic and retry state: ssh-managed-port-forward-restore.ts, ssh-port-forward-unrestored.ts, ssh-port-forward-persistence.ts.
- Edit and remove persistence and ordering: ssh-port-forward-handlers.ts, ssh-port-forward-persistence.ts.
- Teardown cleanup of restore state: ssh-session-teardown.ts.
The test, mock, harness, and e2e helper changes (ssh-ipc-mock-shapes.ts, ssh-ipc-module-mocks.ts, ssh-ipc-test-harness.ts, orcad-managed-workspace.ts, and the test files) support these behaviors. The relaunch helper change adds an optional launch-options parameter only for the restart e2e case. The PR says this is client-only with no wire change. No unrelated product behavior appears in the summary.
Full details: Description check
-
Explanation
The description follows the template well. It has an ELI5, a detailed What Changed list, a Why section, a linked issue (Fixes
#32484), a Visual Proof section marked N/A with a reason, a Testing section with specific tests and e2e runs, a Review section, a Notes section covering cross-platform, remote/SSH, and backwards compatibility, and a checked checklist. The AI Disclosure section is absent. The template says to fill it in for non-StablyAI contributors, and the description does not say whether AI was used. The description does not state whether the 'Testing' manual test checkbox was checked, but it lists automated tests and says local verification was done. Overall the description is mostly complete.
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
…once the tunnel is up Add, edit and remove now merge with saved forwards that aren't live, so a restart that restored nothing no longer lets one re-added forward erase the rest. A call that brings up the managed server's tunnel restores too, so a failed update check or redeploy can't skip it, and a restore runs once per transport instead of on every status note.
![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
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c1cb51a7-fa58-4b80-8078-6eb17a6cf27d
-
src/main/ipc/runtime-environments/runtime-environment-managed-tunnel.test.tssrc/main/ipc/runtime-environments/runtime-environment-managed-tunnel.tssrc/main/ipc/ssh/ssh-managed-port-forward-restore.tssrc/main/ipc/ssh/ssh-port-forward-handlers.test.tssrc/main/ipc/ssh/ssh-port-forward-handlers.tssrc/main/ipc/ssh/ssh-port-forward-persistence.tssrc/main/ipc/ssh/ssh-port-forward-restore.test.tssrc/main/ipc/ssh/ssh.test.tssrc/main/ipc/ssh/ssh.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
-
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
4ae3104e-7c3d-4d19-8442-f14a0c62cb02
-
src/main/ipc/ssh/ssh-managed-port-forward-restore.tssrc/main/ipc/ssh/ssh-port-forward-persistence.tssrc/main/ipc/ssh/ssh-port-forward-restore.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
-
| if (connectionManager!.getConnection(targetId) !== conn) { | ||
| await portForwardManager!.removeForwardAndWait(entry.id) | ||
| return | ||
| return false |
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
#!/bin/bash # Inspect generation changes and the forward manager's transport binding. ast-grep outline src/main/ssh/connection/ssh-connection.ts --match 'getConnectGeneration|attemptConnect|reconnect' --view expanded fd -i 'ssh-port-forward-manager' src/main/ssh src/main/ipc/ssh -e ts rg -n -C 7 '\b(addForward|closeStaleForwards)\s*\(|connectGeneration\s*(\+\+|=)' src/main/ssh src/main/ipc/ssh
Repository: stablyai/orca
Length of output: 41481
Script executed:
#!/bin/bash set -e printf '%s\n' '--- persistence ---' nl -ba src/main/ipc/ssh/ssh-port-forward-persistence.ts | sed -n '1,145p' printf '%s\n' '--- port-forward manager ---' nl -ba src/main/ssh/port-forwarding/ssh-port-forward.ts | sed -n '1,285p' printf '%s\n' '--- connection client/generation declarations and uses ---' rg -n -C 8 -F -e 'connectGeneration' -e 'getClient()' -e 'getConnect' -e 'setClient' -e 'sshClient' src/main/ssh/connection src/main/ssh | head -n 260 printf '%s\n' '--- reconnect replacement and connection manager identity ---' rg -n -C 8 -F -e 'new SshConnection' -e 'setConnection' -e 'connections.set' -e 'replace' -e 'attemptConnect' src/main/ssh/connection src/main/ssh/connection-manager.ts src/main/ssh 2>/dev/null | head -n 320
Repository: stablyai/orca
Length of output: 41868
Script executed:
#!/bin/bash set -e printf '%s\n' '--- SshConnection public lifecycle and reconnect ---' nl -ba src/main/ssh/connection/ssh-connection.ts | sed -n '280,340p;430,500p;720,825p;1000,1085p' printf '%s\n' '--- reconnect owner ---' nl -ba src/main/ssh/connection/ssh-connection-reconnect.ts | sed -n '1,190p' printf '%s\n' '--- connection manager lifecycle ---' rg -n -C 10 -F -e 'new SshConnection' -e 'reconnect' -e 'disconnectConnection' -e 'getConnection(targetId)' src/main/ssh/connection/ssh-connection-manager.ts src/main/ssh/connection printf '%s\n' '--- SSH2 provider ---' nl -ba src/main/ssh/port-forwarding/ssh2-port-forward-provider.ts | sed -n '1,260p' printf '%s\n' '--- system SSH provider ---' nl -ba src/main/ssh/port-forwarding/system-ssh-port-forward-provider.ts | sed -n '1,220p' printf '%s\n' '--- provider type and isLive implementations ---' nl -ba src/main/ssh/port-forwarding/ssh-port-forward-provider.ts | sed -n '1,180p' rg -n -C 8 -F 'isLive' src/main/ssh/port-forwarding
Repository: stablyai/orca
Length of output: 45663
Script executed:
#!/bin/bash set -e rg -n -C 5 -F -e 'getConnectGeneration' -e 'currentGeneration' -e 'closeTransportsForReconnect' src/main/ssh/connection/ssh-connection.ts
Repository: stablyai/orca
Length of output: 1522
Detect an in-place reconnect during restoration.
SshConnection can replace its SSH client while retaining the same connection object. If this occurs while addForward awaits, the identity checks pass, but the forward remains bound to the previous client and is persisted. Capture getConnectGeneration() before restoration and reject the restore when the generation changes after an awaited operation.
Suggested fix
-
const conn = connectionManager!.getConnection(targetId) if (!conn) { return false } + const connectGeneration = conn.getConnectGeneration() // Replaced transports can leave forwards holding local ports but reaching nothing. await portForwardManager!.closeStaleForwards(targetId, conn) + if ( + connectionManager!.getConnection(targetId) !== conn || + conn.getConnectGeneration() !== connectGeneration + ) { + return false + } const live = new Set(portForwardManager!.listForwards(targetId).map(forwardKey)) @@ - if (connectionManager!.getConnection(targetId) !== conn) { + if ( + connectionManager!.getConnection(targetId) !== conn || + conn.getConnectGeneration() !== connectGeneration + ) { return false } @@ - if (connectionManager!.getConnection(targetId) !== conn) { + if ( + connectionManager!.getConnection(targetId) !== conn || + conn.getConnectGeneration() !== connectGeneration + ) { await portForwardManager!.removeForwardAndWait(entry.id) return false }
A restore that ran mid-edit could reopen the mapping the edit was replacing and save both. Every add, edit, remove and restore for a host now runs in order through the same queue.
…ling Each runtime call triggers a restore over the managed tunnel, and with system SSH every attempt is a new login. Restores on the same transport now run at most once per 30 s, a forward that keeps failing waits longer each time, and teardown forgets both. A tunnel with no status yet counts only outside a connect; a remove re-checks its forward inside the queue; drop a circular listener test and the plain restart e2e case.
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c73e1bb7-037d-4ff6-a49b-ab50e56dc86e
-
src/main/ipc/ssh/ssh-managed-host-port-forward-lifecycle.test.tssrc/main/ipc/ssh/ssh-managed-port-forward-restore.tssrc/main/ipc/ssh/ssh-port-forward-handlers.test.tssrc/main/ipc/ssh/ssh-port-forward-handlers.tssrc/main/ipc/ssh/ssh-port-forward-persistence.tssrc/main/ipc/ssh/ssh-port-forward-restore.test.tssrc/main/ipc/ssh/ssh-port-forward-unrestored.tssrc/main/ipc/ssh/ssh-session-teardown.tstests/e2e/ssh-orcad-workspace-tools.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
-
ELI5
When Orca restarted, saved SSH port forwards sometimes didn't come back. The app only turned them back on during an explicit SSH connect. A host the app reconnected through its Orca server's tunnel at startup never ran that connect, so the forwards stayed off and the Ports panel listed none.
What Changed
Why
Hooking each publisher separately is how the gap opened. The relay path used to restore, then the managed connect path was added, and the tunnel path was missed. Triggering on the status itself, plus on the tunnel coming up, covers every way a host comes back. Merging on write means a gap like this can no longer erase saved settings. One queue per host for every forward change removes the restore/edit races, which avoids guarding each one separately.
Credit to @Fosol for the report and the detail that nothing was even listed after a restart.
Linked Issue
Fixes #32484
Visual Proof
N/A. This restores existing behaviour in the main process, and the Docker e2e below shows it working.
Testing
tests/e2e/ssh-orcad-workspace-tools.spec.ts: "across a restart without an SSH connect". Everyssh:connectis forbidden, so only the server's tunnel brings the host back.fetch failedon the forwarded port. With the fix, all cases pass on the final head (run 40169347641).ssh.test.ts,ssh-port-forward-restore.test.ts,ssh-port-forward-handlers.test.ts,runtime-environment-managed-tunnel.test.ts.Follow-up: a separate PR will list saved forwards that couldn't start, with the reason, and let you remove them.
Review
Codex review against pinned base 407e9eb: the final head is clean. Earlier rounds found P2s, all fixed here: a failed bind blocked retries, a closed forward wasn't restored again, a reused port kept its old mapping, a restore could race an edit, a cancelled start recorded a backoff, and a reconnect inherited failures. An independent Opus review found no P0/P1; its P2/P3 items are fixed here.
Agent skill upstream boundary
Notes
Remote/mixed versions: client-only, with no wire change. Folder workspaces: forwards are per SSH host, not per workspace, so they aren't affected. Cross-platform: no platform-specific code.
Recovery: forwards that were never touched after a failed restore are still saved, and they reopen with this fix. Forwards erased when the user re-added another one before this fix can't be recovered, so they must be added once more.
Checklist