agentHost: Simplify SSH relay reconnect with dispose-and-recreate pattern#308106
Merged
roblourens merged 1 commit intomainfrom Apr 7, 2026
Merged
agentHost: Simplify SSH relay reconnect with dispose-and-recreate pattern#308106roblourens merged 1 commit intomainfrom
roblourens merged 1 commit intomainfrom
Conversation
…tern Refactor SSHConnection to use the same dispose-and-recreate pattern as TunnelConnection instead of the more complex isActiveRelay/replaceRelay approach. Key changes: - Remove isActiveRelay() and replaceRelay() from SSHConnection - Make _relay readonly (immutable connection objects) - Add detachSshClient() for ownership transfer during reconnect - Add removeListener to SSHClient interface for proper cleanup - Simplify relay close handlers to just call conn.dispose() - Add try/catch in reconnect to clean up SSH client on failure Add 18 new unit tests covering relay message routing, multi-host isolation, SSH close lifecycle, CLI install flow, reconnect failure cleanup, and listener cleanup across reconnects (33 total, up from 15).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors SSH relay reconnection in SSHRemoteAgentHostMainService to follow an immutable “dispose-and-recreate” connection model (matching the existing tunnel flow), avoiding in-place relay replacement and stale-close suppression logic.
Changes:
- Make
SSHConnectionimmutable w.r.t. its relay and adddetachSshClient()to transfer SSH client ownership during relay recreation. - Update relay close handling to dispose the active
SSHConnection(and suppress close effects from superseded connections via map-identity checks). - Expand the SSH remote agent host unit tests to cover reconnect behavior, relay message routing, listener cleanup, and failure cleanup.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/agentHost/node/sshRemoteAgentHostService.ts | Implements dispose-and-recreate relay reconnect, adds SSH client listener detachment, and simplifies relay close handling. |
| src/vs/platform/agentHost/test/node/sshRemoteAgentHostService.test.ts | Adds/updates unit tests for reconnect semantics, relay routing, event ordering, and SSH client listener cleanup/leak prevention. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
vijayupadya
approved these changes
Apr 6, 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.
Refactor
SSHConnectionto use the same immutable dispose-and-recreate pattern asTunnelConnectioninstead of the more complexisActiveRelay()/replaceRelay()approach.Problem
When a window reloads, we need to replace the WebSocket relay (for a fresh
initializehandshake) while keeping the SSH session alive. The previous implementation mutated_relayinside a livingSSHConnectionand required every relay close handler to checkisActiveRelay()to suppress stale close events. This was subtle and error-prone.Solution
Adopt the pattern from
TunnelConnection: connections are immutable, dispose-once objects. On reconnect, the oldSSHConnectionis disposed and a new one is created reusing the same SSH client.Key changes
isActiveRelay()andreplaceRelay()—SSHConnection._relayis nowreadonlydetachSshClient()— preventsdispose()from ending the SSH session during ownership transfer; also removes event listeners from the shared SSH clientremoveListenertoSSHClientinterface — enables proper listener cleanup on detachif (conn?.isActiveRelay(relay)) { ... }to() => { conn?.dispose(); }Test coverage
18 new unit tests (33 total, up from 15):
relaySenddelivery and unknown-connectionId no-op(Written by Copilot)