Tear down SSH tunnel when removing a remote#311992
Merged
roblourens merged 2 commits intomainfrom Apr 22, 2026
Merged
Conversation
Removing an SSH-backed agent host previously only disposed the renderer-side protocol client and SSHRelayTransport. SSHRelayTransport.dispose() removed the IPC listeners but did not tell the shared-process side to close the SSH tunnel, so the tunnel leaked and the next connect silently reused it. Fix this by giving IRemoteAgentHostService a generic ownership hook for transport-level teardown: - Rename addSSHConnection -> addManagedConnection and add an optional transportDisposable parameter. The platform service registers it on the same per-entry DisposableStore as the protocol client, so it runs on removeRemoteAgentHost, on config-driven reconciliation, and on service disposal. - SSHRemoteAgentHostService passes a transport disposable that synchronously drops the renderer-side handle from its connections map, fires the change event, marks the handle closed, disposes it, then best-effort tells the shared-process service to disconnect. The early 'return existing' branches in connect()/reconnect() are now safe across remove -> reconnect because the map is cleared synchronously. - Web/desktop tunnel renderers just pick up the rename. Their existing transports (TunnelConnectionTransport.dispose, TunnelRelayTransport.dispose) already close the underlying connection, so no behavior change there. Adds renderer-side lifecycle tests covering connect/remove/reconnect and main-side close, plus platform-side tests proving the transport disposable runs on entry removal, replacement, and service dispose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ensures SSH-backed remote agent host tunnels are properly torn down when a remote is removed by introducing a transport-level teardown hook owned by IRemoteAgentHostService, and wiring SSH connections to use it.
Changes:
- Renames
IRemoteAgentHostService.addSSHConnectiontoaddManagedConnectionand adds an optionaltransportDisposablethat is disposed with the entry lifecycle (remove/reconcile/service dispose). - Updates SSH and tunnel/web-tunnel connection registration to use
addManagedConnection; SSH now supplies a transport disposable that drops renderer handles and best-effort disconnects the shared-process tunnel. - Adds/extends unit tests to validate transport teardown on removal/replacement/service disposal and SSH lifecycle behavior (remove → reconnect, main close cleanup).
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/remoteAgentHost/electron-browser/tunnelAgentHostServiceImpl.ts | Updates tunnel-backed connection registration to call addManagedConnection. |
| src/vs/sessions/contrib/remoteAgentHost/browser/webTunnelAgentHostService.ts | Updates web tunnel-backed connection registration to call addManagedConnection. |
| src/vs/sessions/contrib/remoteAgentHost/browser/remoteAgentHostActions.ts | Updates inline comment to reflect the renamed API (addManagedConnection). |
| src/vs/platform/agentHost/test/electron-browser/sshRemoteAgentHostService.test.ts | Adds renderer-side SSH service tests covering teardown and reconnect behavior. |
| src/vs/platform/agentHost/test/electron-browser/remoteAgentHostService.test.ts | Extends tests to verify transportDisposable disposal on remove/replace/service dispose. |
| src/vs/platform/agentHost/electron-browser/sshRemoteAgentHostServiceImpl.ts | Implements SSH transport teardown via transportDisposable and switches to addManagedConnection. |
| src/vs/platform/agentHost/common/remoteAgentHostService.ts | Updates service contract: rename to addManagedConnection and document transportDisposable semantics. |
| src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts | Implements addManagedConnection and ensures transport disposables are owned by per-entry stores. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Share a _setupConnection helper between connect() and reconnect() that wraps the full setup (handshake, handle creation, registration with IRemoteAgentHostService) in a single try/catch. On any failure after the shared-process tunnel is established, drop the renderer-side handle, fire the change event, dispose the protocol client, and best-effort tell main to so we don't leak the SSH tunnel if registration throws.disconnect Previously reconnect() had no error handling at all. - Replace the polling-based awaitClientThenResolve helper in the renderer test with a deterministic DeferredPromise that resolves the moment the Nth protocol client is created, removing potential CI flakiness on slow machines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
pwang347
approved these changes
Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removing an SSH-backed agent host previously only disposed the renderer-side protocol client and
SSHRelayTransport.SSHRelayTransport.dispose()removed the IPC listeners but did not tell the shared-process side to close the SSH tunnel, so the tunnel leaked and the next connect silently reused it.Fix
Give
IRemoteAgentHostServicea generic ownership hook for transport-level teardown:addSSHConnection→addManagedConnectionand add an optionaltransportDisposableparameter. The platform service registers it on the same per-entryDisposableStoreas the protocol client, so it runs onremoveRemoteAgentHost, on config-driven reconciliation, and on service disposal.SSHRemoteAgentHostServicepasses a transport disposable that synchronously drops the renderer-side handle from its_connectionsmap, fires the change event, marks the handle closed, disposes it, then best-effort tells the shared-process service todisconnect(). The earlyreturn existingbranches inconnect()/reconnect()are now safe across remove → reconnect because the map is cleared synchronously.TunnelConnectionTransport.dispose,TunnelRelayTransport.dispose) already close the underlying connection, so no behavior change there.Tests
sshRemoteAgentHostService.test.tscovers the renderer lifecycle: connect registers a transport disposable, removal tears down both the renderer handle and the shared-process tunnel, reconnect after removal does not reuse the old handle, and main-process close cleans up without double-disconnecting.remoteAgentHostService.test.tsproves thetransportDisposableruns on entry removal, replacement, and service dispose.**/agentHost/**/*.test.jssuite passes (714 / 1 pending / 0 failing).(Written by Copilot)