agentHost: keep SSH connection registered after incompatible handshake#319455
Draft
roblourens wants to merge 4 commits into
Draft
agentHost: keep SSH connection registered after incompatible handshake#319455roblourens wants to merge 4 commits into
roblourens wants to merge 4 commits into
Conversation
When the remote server rejects the client's protocol version, SSH _setupConnection used to throw before calling addManagedConnection. That left IRemoteAgentHostService with no entry for the host, so triggerServerUpgrade could not find the still-open relay and the 'Update Server' flow was unusable from an SSH agent host. Extend addManagedConnection with an optional status parameter, and update the SSH path to detect incompatible handshakes via RemoteAgentHostConnectionStatus.fromConnectError. On an incompatible result, register the managed connection with status 'incompatible' and keep the relay (handle + protocolClient) alive so triggerServerUpgrade can reuse it, while still rethrowing the original error so the caller surfaces the failure to the user. Adds unit tests at both layers. Fixes #319381 (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the “Update Server” flow for SSH-bootstrapped remote agent hosts when the initial protocol handshake fails due to an unsupported protocol version, by still registering the underlying transport as a managed connection (marked incompatible) so triggerServerUpgrade can reuse it.
Changes:
- Extend
IRemoteAgentHostService.addManagedConnectionto accept an optionalstatus(defaulting toconnected). - Update the SSH connection setup flow to register an
incompatiblemanaged connection onUnsupportedProtocolVersion, while still rejecting the original connect attempt. - Add/extend unit tests to ensure incompatible connections remain upgrade-addressable but are not treated as “connected”.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/agentHost/test/electron-browser/sshRemoteAgentHostService.test.ts | Adds a test asserting SSH keeps an incompatible transport registered for upgrade without tearing down the relay. |
| src/vs/platform/agentHost/test/electron-browser/remoteAgentHostService.test.ts | Adds a test ensuring triggerServerUpgrade works even when the managed connection is incompatible. |
| src/vs/platform/agentHost/electron-browser/sshRemoteAgentHostServiceImpl.ts | Registers managed SSH connections with incompatible status on handshake mismatch (without disconnecting the tunnel). |
| src/vs/platform/agentHost/common/remoteAgentHostService.ts | Extends the addManagedConnection API contract with an optional status parameter and documents its intent. |
| src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts | Implements the new status parameter, deriving connected from status and returning the provided status. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 2
After the upgrade RPC succeeds and the SSH provider calls
`SSHRemoteAgentHostService.reconnect()`, the main-side
`replaceRelay` path tears down the old WebSocket relay and creates a
fresh but it deliberately keeps the same `connectionId` soone
nothing else (like the WebSocket relay channel routing) breaks.
That meant `_setupConnection` saw the old renderer-side handle in its
`_connections` map and short-circuited:
const existing = this._connections.get(result.connectionId);
if (existing) { return existing; }
So the stale `RemoteAgentHostProtocolClient` (permanently stuck in
`incompatible` after the original handshake rejection) was reused,
even though the underlying server had just been upgraded and would
now happily accept our protocol version. The connection therefore
never recovered until the window was reloaded.
Only short-circuit on existing handles when the managed entry is
still in a usable state (`getConnection` returns the client). When
it isn't, drop the stale without running itshandle
`disconnectFn`, since the main service kept the SSH client alive
across `replaceRelay` and we'd otherwise kill the brand-new tunnel
and fall through to a fresh handshake. The subsequent
`addManagedConnection` call replaces the stale managed entry and
disposes the old protocol client.
Adds a regression test that pins a stable `connectionId` across
`connect`/`reconnect` to mimic `replaceRelay` and asserts that
the second handshake produces a fresh client, a fresh `connected`
registration, and no main-tunnel disconnect.
(Written by Copilot)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hake Mirrors the SSH fix for the tunnel paths (desktop and web). When the protocol handshake fails with `UnsupportedProtocolVersion`, the service now registers the still-open relay as an `incompatible` managed connection instead of disposing it, so the renderer's `triggerServerUpgrade` flow can locate the protocol client and send `_vscodeUpgrade` over the existing transport. (Written by Copilot) Note: tunnels generate a fresh `connectionId` on every connect (unlike SSH's `replaceRelay` path), so the corresponding "stale-handle on reconnect" fix from the SSH side is not needed reconnect alwayshere creates a brand-new protocol client. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Match the production shape (server advertises supportedVersions: ['^' + PROTOCOL_VERSION] in protocolServerHandler.ts) instead of date-like placeholders. No behavioural change. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
connor4312
approved these changes
Jun 2, 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.
Fixes #319381
Problem
When an SSH-bootstrapped remote agent host fails its handshake because the remote rejects the client's protocol version, the renderer surfaces the error correctly but the "Update Server" upgrade flow is unusable.
Root cause:
_setupConnectioninSshRemoteAgentHostServiceImplthrows onUnsupportedProtocolVersionbefore callingIRemoteAgentHostService.addManagedConnection. With no entry in the platform service,triggerServerUpgradecannot locate the still-open SSH relay and the upgrade RPC has no transport to ride on.This matched the WebSocket path's earlier behaviour for the same case, which was already fixed by registering an
incompatiblemanaged connection. SSH (and tunnels) needed the same treatment.Fix
IRemoteAgentHostService.addManagedConnectionwith an optionalstatus: RemoteAgentHostConnectionStatusparameter (defaults toconnected, so existing callers are unchanged)._setupConnection(and the parallel tunnel/web-tunnel paths), wrap the handshake in a try/catch. IfRemoteAgentHostConnectionStatus.fromConnectErrorclassifies the error asincompatible:status: incompatible, keeping thehandleandprotocolClientalive sotriggerServerUpgradecan reuse the transport.connectWithProgress) still surfaces the failure to the user.protocolClientand the underlying transport as before and rethrow.Applies to:
sshRemoteAgentHostServiceImpl.tstunnelAgentHostServiceImpl.tswebTunnelAgentHostService.tsTests
remoteAgentHostService.test.ts: new test asserts that anincompatiblemanaged connection is reachable fortriggerServerUpgradewhilegetConnectionstill reports it as not connected.sshRemoteAgentHostService.test.ts: new tests assert that (a) an incompatible handshake registers the managed connection withincompatiblestatus, does not tear down the relay, and still rejects the connect promise with the originalProtocolError; (b) reconnect after an incompatible handshake replaces the stale handle and re-handshakes against the upgraded server.Validation
compile-check-ts-native,valid-layers-check,precommithygiene all clean.remoteAgentHostServiceandsshRemoteAgentHostServiceall green.UnsupportedProtocolVersionhandshake against a current dev build, clicked "Update supervisor reported the upgrade RPC and restarted on the new commit, and the desktop dropped the stale incompatible handle, re-handshook against the upgraded server, and connected cleanly.Server"Notes for reviewers
IConnectionEntrystill carries bothconnected: booleanandstatus: RemoteAgentHostConnectionStatus. This PR keeps the former derived from the latter viaisConnected(status)but does not deduplicate the two worth a follow-up to converge call sites on one source of truth.fields(Written by Copilot)