Add scoped SSH preview port forwarding#4039
Conversation
Co-authored-by: codex <codex@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const retryPolicy = Schedule.spaced(Duration.millis(50)).pipe( | ||
| Schedule.take(Math.ceil(SSH_READY_TIMEOUT_MS / 50)), | ||
| ); | ||
| yield* net.hasListenerOnHost(localPort, "127.0.0.1").pipe( |
There was a problem hiding this comment.
🟡 Medium src/tunnel.ts:949
waitForTcpForwardReady treats any listener on 127.0.0.1:${localPort} as proof that the SSH forward is ready, but the local port is released before ssh is spawned. Another process can claim that port, and this probe will succeed against the unrelated listener while SSH is still connecting. This causes startSshTunnel to return a lease whose local port belongs to a different process rather than the requested remote forward. Consider probing the actual SSH forward instead of relying on the local port having any listener, or verify the listener is owned by the SSH process.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/ssh/src/tunnel.ts around line 949:
`waitForTcpForwardReady` treats any listener on `127.0.0.1:${localPort}` as proof that the SSH forward is ready, but the local port is released before `ssh` is spawned. Another process can claim that port, and this probe will succeed against the unrelated listener while SSH is still connecting. This causes `startSshTunnel` to return a lease whose local port belongs to a different process rather than the requested remote forward. Consider probing the actual SSH forward instead of relying on the local port having any listener, or verify the listener is owned by the SSH process.
| yield* Deferred.fail(pending.deferred, makeSshTunnelCancelledError(target)).pipe(Effect.ignore); | ||
| }); | ||
|
|
||
| yield* Scope.addFinalizer( |
There was a problem hiding this comment.
🟡 Medium src/tunnel.ts:1282
The manager-scope finalizer closes existing portForwards entries but never cancels pendingPortForwards. If the manager scope closes while createPortForwardEntry is still running, that pending creation can pass its pending-entry check, insert itself into portForwards, and keep its SSH child process alive in an independent entryScope after the manager is gone — leaking the forward. The finalizer should cancel pending port-forward creations (failing their Deferreds) so their onExit handlers close the entry scope.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/ssh/src/tunnel.ts around line 1282:
The manager-scope finalizer closes existing `portForwards` entries but never cancels `pendingPortForwards`. If the manager scope closes while `createPortForwardEntry` is still running, that pending creation can pass its pending-entry check, insert itself into `portForwards`, and keep its SSH child process alive in an independent `entryScope` after the manager is gone — leaking the forward. The finalizer should cancel pending port-forward creations (failing their `Deferred`s) so their `onExit` handlers close the entry scope.
| const localPort = yield* reserveLocalTunnelPort(); | ||
| const httpBaseUrl = `http://127.0.0.1:${localPort}/`; | ||
| const wsBaseUrl = `ws://127.0.0.1:${localPort}/`; | ||
| const entryScope = yield* Scope.make("sequential"); |
There was a problem hiding this comment.
🟡 Medium src/tunnel.ts:1624
When a fiber is interrupted after startSshTunnel succeeds but before Scope.addFinalizer registers the cleanup, the independently created entryScope is never closed. The Effect.onExit handler only wraps runWithSshAuth, so a successful tunnel start leaves the scope open and the SSH process running with no entry, lease, or finalizer to tear it down. Wrap the entire sequence from scope creation through finalizer registration so interruption at any point closes entryScope.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/ssh/src/tunnel.ts around line 1624:
When a fiber is interrupted after `startSshTunnel` succeeds but before `Scope.addFinalizer` registers the cleanup, the independently created `entryScope` is never closed. The `Effect.onExit` handler only wraps `runWithSshAuth`, so a successful tunnel start leaves the scope open and the SSH process running with no entry, lease, or finalizer to tear it down. Wrap the entire sequence from scope creation through finalizer registration so interruption at any point closes `entryScope`.
| input.timeoutMs ?? request.timeoutMs, | ||
| ); | ||
| return await currentStatus(threadRef, ready.tabId); | ||
| return await withBrowserNavigationRoute(route, async () => { |
There was a problem hiding this comment.
🟡 Medium preview/PreviewAutomationHosts.tsx:420
Concurrent navigate requests for the same tab can release the wrong SSH-forward scope. The request consumer invokes each handle independently, so when two navigations overlap and the older one finishes after the newer one, the older route.commit(ready.tabId) overwrites activeRoutes[tabId] and closes the newer route's still-active tunnel. The tab is then left displaying the newer forwarded URL through a tunnel that has already been released, breaking the remote preview. Consider serializing commits per tab or guarding commit with a navigation generation check so a stale request cannot evict a newer route.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/preview/PreviewAutomationHosts.tsx around line 420:
Concurrent `navigate` requests for the same tab can release the wrong SSH-forward scope. The request consumer invokes each `handle` independently, so when two navigations overlap and the older one finishes after the newer one, the older `route.commit(ready.tabId)` overwrites `activeRoutes[tabId]` and closes the newer route's still-active tunnel. The tab is then left displaying the newer forwarded URL through a tunnel that has already been released, breaking the remote preview. Consider serializing commits per tab or guarding `commit` with a navigation generation check so a stale request cannot evict a newer route.
| SshEnvironmentEffectContext | ||
| > { | ||
| const existing = portForwards.get(input.key); | ||
| if (existing !== undefined) { |
There was a problem hiding this comment.
🟡 Medium src/tunnel.ts:1688
ensurePortForwardEntry returns an existing port-forward entry after isRunning reports true, but a concurrent releasePortForward that drops the last lease can run closePortForwardEntry between the isRunning check and the return. The caller then receives a local port whose SSH process has already been killed, so the forward does not work. The lease added afterward is also registered on a closed entry and stays in portForwardLeases until explicitly released. Re-check that portForwards.get(input.key) === existing (and that the entry still has at least one lease) after isRunning succeeds, or hold a lock around the check-and-lease-acquire so the entry cannot be closed concurrently.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/ssh/src/tunnel.ts around line 1688:
`ensurePortForwardEntry` returns an existing port-forward entry after `isRunning` reports `true`, but a concurrent `releasePortForward` that drops the last lease can run `closePortForwardEntry` between the `isRunning` check and the return. The caller then receives a local port whose SSH process has already been killed, so the forward does not work. The lease added afterward is also registered on a closed entry and stays in `portForwardLeases` until explicitly released. Re-check that `portForwards.get(input.key) === existing` (and that the entry still has at least one lease) after `isRunning` succeeds, or hold a lock around the check-and-lease-acquire so the entry cannot be closed concurrently.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
Bugbot Autofix is ON, but a cloud agent failed to start.
Reviewed by Cursor Bugbot for commit e5f5c40. Configure here.
| } | ||
| if (previous !== undefined && previous !== acquired.scope) { | ||
| await closeRouteScope(previous); | ||
| } |
There was a problem hiding this comment.
Overlapping tab commits kill forward
Medium Severity
When a second preview navigation for the same tabId calls commit while an earlier ssh-forward route is still in use, commit closes the previous scope in activeRoutes without waiting for the first navigation to finish. The in-flight load can keep using a local port whose SSH forward was already torn down, so previews intermittently fail under overlapping automation or rapid URL submits.
Reviewed by Cursor Bugbot for commit e5f5c40. Configure here.
| if (scope === undefined) return; | ||
| activeRoutes.delete(tabId); | ||
| await closeRouteScope(scope); | ||
| } |
There was a problem hiding this comment.
SSH disconnect leaves stale routes
Medium Severity
Committed ssh-forward scopes live in activeRoutes until releaseBrowserNavigationRoute runs on successful tab close. SSH environment disconnect tears down port forwards in the desktop bridge, but nothing clears matching activeRoutes entries. An open preview tab can keep a committed route while the underlying forward is already gone, so refresh and navigation that do not acquire a new route can hit dead local ports.
Reviewed by Cursor Bugbot for commit e5f5c40. Configure here.
| if (scope === undefined) return; | ||
| activeRoutes.delete(tabId); | ||
| await closeRouteScope(scope); | ||
| } |
There was a problem hiding this comment.
SSH forwards leak on tab removal
High Severity
Committed ssh-forward routes are stored in activeRoutes until releaseBrowserNavigationRoute closes the scope and tears down the desktop lease. That release runs only after a successful closePreviewSession. Preview tabs removed via server closed events, reconcilePreviewServerSessions, or clearing snapshots never call it, so SSH port forwards and Effect scopes can outlive the tab.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e5f5c40. Configure here.
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. New feature adding SSH preview port forwarding with complex lifecycle management. Multiple unresolved review comments identify potential resource leaks, race conditions, and concurrency bugs that require human attention before merging. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Summary
Verification
vp checkvp run typecheckNote
Medium Risk
Cross-cutting changes to SSH tunnel lifecycle and preview navigation; lease or scope bugs could leak forwards or break remote previews, though auth and relay paths are largely unchanged.
Overview
Adds scoped SSH preview port forwarding so localhost/dev-server URLs on SSH environments resolve through ref-counted loopback tunnels instead of only rewriting hosts for direct private-network access.
The stack wires desktop IPC (
acquireSshPortForward/releaseSshPortForward) throughSshEnvironmentManager(shared forwards per remote port, TCP readiness, cleanup on disconnect), exposesforwardPortonSshEnvironmentGateway(desktop bridge with acquire/release; mobile blocked), and introducesEnvironmentPortRouterin client-runtime for direct vs private-network vsssh-forwardresolution (relay/T3 Connect ports still rejected).Web preview replaces synchronous
browserTargetResolverURL mapping with async route acquisition (commit/releaseper tab, SSH scopes held until tab close). Preview automation, manual navigation, discovered ports, and session close all use this lifecycle. Contracts addssh-forwardtoPreviewUrlResolution;NetService.hasListenerOnHostsupports forward readiness probes.Reviewed by Cursor Bugbot for commit e5f5c40. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add scoped SSH preview port forwarding with reference-counted lease management
acquirePortForwardandreleasePortForwardtoSshEnvironmentManager, tracking concurrent leases per target/remotePort and tearing down the SSH tunnel only when the last lease is released.EnvironmentPortRouterinpackages/client-runtime/src/preview/router.tsthat resolves preview navigation targets todirect,direct-private-network, orssh-forwardresolutions, acquiring scoped SSH port forwards for SSH connections.acquireBrowserNavigationTarget,releaseBrowserNavigationRoute,withBrowserNavigationRoute) so SSH-forward leases remain alive per tab until the tab is closed.acquireSshPortForwardandreleaseSshPortForwardchannels, wired throughDesktopSshEnvironmentand exposed onwindow.desktopBridge.hasListenerOnHosttoNetServiceand uses it to poll for TCP readiness when establishing port-forward tunnels, with a timeout bounded bySSH_READY_TIMEOUT_MS.📊 Macroscope summarized e5f5c40. 21 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.