Skip to content

fix(web): disconnect a remote environment without removing it - #9687

Closed
Skilux wants to merge 3 commits into
pingdotgg:mainfrom
Skilux:fix/connections-disconnect-without-removing
Closed

fix(web): disconnect a remote environment without removing it#9687
Skilux wants to merge 3 commits into
pingdotgg:mainfrom
Skilux:fix/connections-disconnect-without-removing

Conversation

@Skilux

@Skilux Skilux commented Sep 4, 2026

Copy link
Copy Markdown

What Changed

packages/client-runtime/src/connection/registry.ts and src/state/connections.ts now expose connect and disconnect commands (the supervisor already implemented both, they were just never surfaced). apps/web/src/components/settings/ConnectionsSettings.tsx uses them: SavedBackendListRow now always renders its own "Remove" button plus a real Connect/Disconnect toggle driven by connection phase. That toggle is no longer disabled while connecting/reconnecting, and "Disconnect" now calls the disconnect command instead of the remove handler. The container wires the two new atom commands and tracks in-flight disconnect state separately from in-flight remove state. Added four tests in packages/client-runtime/src/connection/registry.test.ts covering disconnect-then-connect, no-op on an unregistered id, and SSH tunnel teardown on disconnect.

4 files changed, +248/-34.

Why

Fixes #9685. A saved remote environment that goes unreachable sits in "Failed to connect. Reconnecting..." forever, and the only enabled action is Remove, which unregisters the environment and clears its cached data. There was no way to pause the retry loop and keep the environment. Separately, a still-connected row's only other button was labelled "Disconnect" but called the remove handler, so it silently deleted the environment instead of disconnecting it.

The capability already existed in packages/client-runtime/src/connection/supervisor.ts (connect/disconnect, the latter setting desired: false so the run loop clears the lease, resets the retry ladder, and parks in available instead of retrying). It just was not exposed through the registry or the state layer. This PR wires the existing capability through rather than adding new machinery.

One behavior note: handleConnectSavedBackend now calls connect (sets desired: true and signals an immediate attempt) instead of retryNow. retryNow only resets the backoff ladder and does not set desired, so after a real disconnect a retry signal alone would leave the supervisor parked in available instead of reconnecting.

Surface check: apps/web is fixed directly; apps/desktop wraps apps/web and picks up the same fix. apps/mobile (SettingsEnvironmentsRouteScreen.tsx / ConnectionEnvironmentRow) has no connect/disconnect toggle on this row, only Remove, so it does not carry this bug. The shared connect/disconnect commands are now available in packages/client-runtime if mobile adds this control later, but adding a new mobile affordance is out of scope for this PR. No contract/schema or server changes.

UI Changes

Not motion or timing related, no video included.

Before, stuck reconnecting (Remove enabled, other button disabled reading "Connecting…"):

Before, stuck reconnecting

Before, connected row (single button labelled "Disconnect" that actually removes):

Before, connected row

After, stuck reconnecting (Remove plus an enabled "Disconnect"):

After, stuck reconnecting

After, disconnected and retained (row moves to "Available" with "Connect", still listed):

After, disconnected and retained

After, reconnected via Connect:

After, reconnected

Verification

  • vp test run packages/client-runtime/src/connection/registry.test.ts: 23 passed (19 pre-existing + 4 new). New tests: (a) disconnect on a connected environment moves it to phase available with desired: false, releases the session, keeps the registry entry and persisted target; (b) connect after a disconnect re-establishes a second session and returns to connected; (c) connect/disconnect on an unregistered environment id are no-ops; (d) disconnecting an SSH environment records the gateway teardown while the registry entry and stored profile survive (verified failing without the fix).
  • Targeted tsgo --noEmit: apps/web clean; packages/client-runtime shows only two pre-existing suggestions in files this PR does not touch (src/relay/discovery.ts, src/rpc/session.test.ts).
  • Targeted vp lint on the four touched files: one react(preserve-manual-memoization) warning at ConnectionsSettings.tsx:2103, confirmed pre-existing on main and unrelated to this change (desktopServerExposureState).
  • Manual end-to-end check in a real web client (node scripts/dev-runner.ts dev against an isolated T3 home, paired against a second node apps/server/src/bin.ts serve instance as the remote environment): killed the remote backend, confirmed pre-fix behavior (row stuck reconnecting, Remove enabled, other button disabled/"Connecting…"), then with the fix (same row, same session, Vite HMR swap) confirmed Remove + enabled Disconnect, clicking Disconnect moved the row to Available with the error cleared and retries stopped while the environment stayed listed, and Connect after restarting the backend returned it to Connected.
  • Environment for the manual check: macOS (Darwin 27.0.0, arm64), Node v26.8.1, pnpm 11.10.0, Chrome, upstream main at 7cf5b28.

Review follow-ups

Two AI review findings, both confirmed against the source and fixed:

  • Macroscope, registry.ts lease race (9a847ea): acquireSupervisor released the per-environment lease before returning, so connect/disconnect flipped the intent outside it and a concurrent register or platform reconcile could swap in a fresh, connected supervisor in the gap. Acquisition and the intent change now share one lease via acquireSupervisorLocked.
  • Cursor Bugbot, SSH tunnel left open (8cc9f68): a real behavior gap, since pre-PR the connected-row control removed the environment and remove tears the tunnel down. disconnect now calls ssh.disconnect for SSH targets, best effort. resolver.prepare runs ssh.prepare on every attempt, so the next connect re-establishes it.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes (not applicable, not motion/timing related)

Claude Opus 5 (1M context) running in T3 Code via the Claude Code harness did this work.

Note

Add connect and disconnect to EnvironmentRegistry and settings UI

  • Adds connect and disconnect operations to registry.ts and exposes them as catalog commands in connections.ts.
  • The disconnect operation releases the supervisor session and tears down managed SSH tunnels, ignoring cleanup errors.
  • The settings UI in ConnectionsSettings.tsx separates Remove and Disconnect actions, allowing connected environments to be disconnected without losing their registration.
  • Behavioral Change: The saved-backend Connect action now calls the explicit catalog connect command instead of the retry-now command.

Macroscope summarized 8cc9f68.

A saved remote environment that cannot be reached sits in "Failed to
connect. Reconnecting..." forever. The only enabled action on that row was
Remove, because the second button was disabled while the phase was
connecting or reconnecting. On a connected row the same button read
"Disconnect" but called the remove handler, so the one control that looked
like a pause actually deleted the environment and its cached data.

The connection supervisor already models this: `disconnect` clears the
lease, cancels the backoff timer and parks the state machine in
"available" until a connect request arrives. Nothing exposed it. This
threads `connect` and `disconnect` through EnvironmentRegistry and the
environment-catalog atoms, then makes the row use them, so Disconnect
pauses reconnect attempts and keeps the registration while Remove stays
the separate destructive action.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 4, 2026
Comment thread packages/client-runtime/src/connection/registry.ts Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 07a8ad7. Configure here.

Comment thread packages/client-runtime/src/connection/registry.ts
…sconnect

`acquireSupervisor` released the per-environment lease before returning, so
`connect` and `disconnect` flipped the intent outside it. A concurrent
`register` or platform reconcile could close and replace that supervisor in
the gap, leaving the command to update a discarded supervisor while a fresh,
connected one took over. Disconnect then reported success with the
environment still connected.

Split the locked acquisition body into `acquireSupervisorLocked` and let
`connect` and `disconnect` run it inside `withLeaseLock` together with the
intent change. `withLeaseLock` is not reentrant, hence the separate locked
variant rather than nesting. Holding the lease across a supervisor call is
already the established pattern here: `createServiceScope` calls
`supervisor.connect` under the same lease, and the supervisor's signal queue
is unbounded so the offer cannot block.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Sep 4, 2026
Parking the supervisor closes the RPC session but leaves a managed SSH
environment's tunnel running. Before this PR the only control on a connected
SSH row removed the environment, and `remove` does tear the tunnel down, so
turning that control into a real disconnect would have left the tunnel up
with no way to stop it short of removing the environment.

`disconnect` now calls `ssh.disconnect` for SSH targets after flipping the
intent, best effort so a failed teardown still leaves the environment
parked. This does not strand the environment: `resolver.prepare` runs
`ssh.prepare` on every connection attempt, so the next connect re-establishes
the tunnel.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@macroscopeapp

macroscopeapp Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 8cc9f68

Macroscope's review found this PR approvable — This is a focused connection-control bug fix that separates Remove from Disconnect, preserves saved environments, stops reconnect loops, and cleans up managed SSH tunnels. The runtime changes are narrowly scoped, user-triggered, and covered by targeted registry tests, with no schema, deployment, default, or static-analysis configuration changes.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge

Copy link
Copy Markdown
Member

Superseded by #11478, which already landed and closed #9685 (switch/disconnect saved environments without removing them). Closing this PR as leftover hygiene.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Remote environment stuck reconnecting cannot be disconnected without removing it

3 participants