fix(desktop): close SSH forwards before installing an update#14276

🟠 High updates/DesktopUpdates.ts:650

quitAndInstall can leave an SSH ssh -L forward orphaned, causing the relaunched app to fail when it reuses that local port. closeLocalForwards only closes the forwards present when it snapshots the map, so an IPC-driven ensureEnvironment that runs concurrently can register a new forward after the snapshot; because updater shutdown skips the manager finalizer, that entry is never killed. Block new SSH environment ensures once installation starts or make closeLocalForwards reject concurrent ensures.

Fixed in 9025a799e1. Cleanup closes admission before collecting registered targets and active target-lock keys, then drains those same per-target locks. An admitted creator settles before cleanup closes its forward; queued and late ensures reject inside the lock. The updater awaits the drain before installer handoff. The controlled in-flight readiness regression passes, as do all 87 focused/associated cases. The headless service/OpenSSH proof also verifies the old local listener is closed at handoff while remote HTTP remains alive. Native packaged update/relaunch approval is still outstanding.

Verified: admission closes before target collection, in-flight and later ensures are rejected under the target lock, and the updater awaits the drain before handoff. This resolves the race.

🟠 High updates/DesktopUpdates.ts:650

When quitAndInstall fails or emits an error, recovery kills every active SSH forward, so existing SSH-backed renderer sessions retain loopback URLs whose listeners no longer exist. recoverFailedInstall only restarts backend instances and never reruns ensureEnvironment; preserve or restore the forwards during recovery, or defer closeLocalForwards until the updater has committed to exit.

No longer relevant as of 9025a799e1c723e762d328da1ca5c1fc00497cd3

Addressed in 9025a799e1. Failure recovery reopens SSH admission. The existing renderer supervisor reconnect path calls driver.connect → resolver.prepare → ensureSshEnvironment and adopts its returned HTTP/WebSocket URLs, so recovery does not pin the old loopback port. The headless service/OpenSSH proof exercises the actual updater/desktop SSH services and real OpenSSH transport: typed/thrown/native errors and interruption reconnect successfully, including when the former port is occupied. Manager-owned remote finalizers also survive cleanup, so a later normal quit stops the remote once even without reconnect. Native installer adapters are stubbed in this proof; packaged platform verification and human lifecycle approval remain required.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Serialize cleanup with tunnel creation.

If ensureEnvironment is launching a tunnel while closeLocalForwards runs, tunnels.clear() can execute before createTunnelEntry calls tunnels.set(...). The new forward then escapes this cleanup and can remain open when the updater quits. Serialize cleanup with in-flight creation, and prevent new creation until installation quits or recovery completes.

✅ Confirmed as addressed by @juliusmarminge

Fixed in 9025a799e1: admission closes before the target/active-lock snapshot, cleanup drains every admitted per-target operation, and later ensures reject under the same lock. Failure recovery reopens admission for the existing renderer reconnect path; successful handoff leaves it closed. The in-flight creation regression and all 87 scoped cases pass; headless service/OpenSSH proof confirms actual forward closure before handoff without remote shutdown. No native installer approval is claimed.

@juliusmarminge The supplied code addresses this race. closeLocalForwards closes admission before the snapshot, then waits on each target lock before closing its tunnel. Later ensures reject under the same lock.

Thanks for the fix and validation details. This conclusion does not establish native installer behavior.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Do not let update cleanup wait, without interruption, on an in-flight ensure.

closeLocalForwards adds each key from targetLocks.activeKeys and then calls targetLocks.withLock(key, ...) for each key. The whole effect is wrapped in Effect.uninterruptible, so the wait to acquire each lock cannot be interrupted. The ensure that holds the lock can run for a long time:

• launchOrReuseRemoteServer in archive mode uses REMOTE_ARCHIVE_LAUNCH_TIMEOUT_MS (900 s).
• An ensure can also wait on an interactive password prompt.

DesktopUpdates.installDownloadedUpdate calls closeLocalForwards after it sets quitting to true and stops every backend, and it holds installMutex during the call. If the user starts an install while an SSH connect is in progress, the install stalls for the remaining duration of that ensure. During that time all backends are stopped and the app is in the quitting state. restore cannot cancel the wait, so the Effect.onInterrupt(() => resetInstallAction) path does not run either. When the ensure finishes, it creates a forward that cleanup closes right away, so the wait has no benefit.

Recommended fix: track the fibers of in-flight ensures in the manager. In closeLocalForwards, set forwardsClosed = true, interrupt those fibers, and then close the remaining entries. Another option is to make only the lock acquisition interruptible and keep the close of each entry uninterruptible, so a caller can bound the wait. If you choose that option, add a timeout at the call site in DesktopUpdates.

✅ Confirmed as addressed by @juliusmarminge

Fixed in 6d7f4095ea. Target-lock acquisition is now interruptible while each acquired local-forward close remains masked. The updater gives draining a 10-second budget and aborts before backend shutdown or installer handoff if an admitted launch is still blocked. Recovery reopens admission and clears install state; the independently admitted ensure can finish later, remains reusable, and is stopped once on normal shutdown.

The two fresh regressions first failed (87 pass / 2 fail), then all 89 focused cases passed. A follow-up ordering regression first failed (88 pass / 1 fail) and now proves no local backend stop or restart on timeout. Both whole-PR reviewers are clean. The real OpenSSH HTTP/WebSocket proof observed abort after 10,009 ms before releasing the launch gate, then verified late connection reuse and normal shutdown ownership. Native installer lifecycle approval remains outstanding.

@juliusmarminge Verified the fix in the current code.

• Target-lock acquisition is interruptible. Each acquired local-forward close remains uninterruptible.
• The updater applies a 10-second drain timeout before backend shutdown or installer handoff.
• Timeout recovery reopens SSH admission and clears install state.
• Regression assertions cover no backend stop or restart on timeout, late connection reuse, and exactly one remote stop on normal shutdown.

This addresses this finding. I inspected the code and regression assertions; I did not run the tests. Native installer lifecycle validation remains separate.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reopen admission before the second ensureEnvironment call.

closeLocalForwards sets forwardsClosed = true. After that, withTargetLock rejects every ensure with SshReadinessError until reopenLocalForwards runs. The test calls manager.ensureEnvironment(target) at Line 460 without reopening, so the call fails. CI confirms this failure: SshReadinessError: SSH forwards are closed for an update.

The scope assertions also depend on reopening. The manager finalizer runs the remote finalizers only when forwardsClosed is false. The test expects stopCommandCount === 1 after the scope closes, and that result requires reopened admission.

The new test at Lines 160-218 already covers rejection while admission is closed. This test should cover the reopen-and-reconnect path.

Source: Pipeline failures

✅ Addressed in commit c1fef03

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Import Duration. The Build and Typecheck job fails without it.

The new timeout test calls TestClock.adjust(Duration.zero) at Line 488 and TestClock.adjust(Duration.minutes(1)) at Line 501. The file does not import Duration. CI reports TS2304: Cannot find name 'Duration' at both lines, so the desktop typecheck fails.

Also applies to: 501-501

Source: Pipeline failures

✅ Addressed in commit c1fef03