agentHost: keep SSH connection registered after incompatible handshake (#319455)#319661
Merged
Conversation
#319455) * agentHost: keep SSH connection registered after incompatible handshake 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> * agentHost: re-handshake on SSH reconnect after incompatible 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> * agentHost: keep tunnel connection registered after incompatible handshake 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> * agentHost: use SemVer protocol version strings in upgrade tests 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the remote Agent Host upgrade and recovery flow by ensuring that connections whose protocol handshake fails due to an incompatible protocol version remain registered (and therefore “addressable”) for the subsequent Update Server / _vscodeUpgrade path. It also fixes an SSH reconnect edge case where a stale, permanently-incompatible protocol client could be reused after the server was upgraded.
Changes:
- Extend
IRemoteAgentHostService.addManagedConnectionto accept an optionalstatusso managed connections can be registered asincompatible(transport alive, handshake rejected). - Update SSH + tunnel connection setup paths to detect
UnsupportedProtocolVersionhandshake failures and register the still-open relay asincompatiblesotriggerServerUpgradecan reuse it. - Add unit/regression tests covering “incompatible-but-addressable” upgrade and SSH reconnect re-handshake behavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/providers/remoteAgentHost/electron-browser/tunnelAgentHostServiceImpl.ts | Keep tunnel relay registered on incompatible handshake; plumb status into managed-connection registration. |
| src/vs/sessions/contrib/providers/remoteAgentHost/browser/webTunnelAgentHostService.ts | Mirror incompatible-handshake registration logic for web tunnel transports. |
| src/vs/platform/agentHost/electron-browser/sshRemoteAgentHostServiceImpl.ts | Register incompatible SSH connections; avoid reusing stale incompatible handles on reconnect by checking getConnection(...). |
| src/vs/platform/agentHost/common/remoteAgentHostService.ts | Extend addManagedConnection API contract and document the status semantics. |
| src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts | Implement status plumbing for managed connections (connected vs incompatible) and ensure triggerServerUpgrade can target the stored client regardless of handshake state. |
| src/vs/platform/agentHost/test/electron-browser/sshRemoteAgentHostService.test.ts | Add tests for “incompatible keeps tunnel registered” and “reconnect after upgrade re-handshakes with fresh client”. |
| src/vs/platform/agentHost/test/electron-browser/remoteAgentHostService.test.ts | Add test ensuring incompatible managed connections remain upgrade-addressable via triggerServerUpgrade. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 1
dileepyavan
approved these changes
Jun 3, 2026
bhavyaus
approved these changes
Jun 3, 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.
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)
After the upgrade RPC succeeds and the SSH provider calls
SSHRemoteAgentHostService.reconnect(), the main-sidereplaceRelaypath tears down the old WebSocket relay and creates a fresh but it deliberately keeps the sameconnectionIdsoone nothing else (like the WebSocket relay channel routing) breaks.That meant
_setupConnectionsaw the old renderer-side handle in its_connectionsmap and short-circuited:const existing = this._connections.get(result.connectionId); if (existing) { return existing; }
So the stale
RemoteAgentHostProtocolClient(permanently stuck inincompatibleafter 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 (
getConnectionreturns the client). When it isn't, drop the stale without running itshandledisconnectFn, since the main service kept the SSH client alive acrossreplaceRelayand we'd otherwise kill the brand-new tunnel and fall through to a fresh handshake. The subsequentaddManagedConnectioncall replaces the stale managed entry and disposes the old protocol client.Adds a regression test that pins a stable
connectionIdacrossconnect/reconnectto mimicreplaceRelayand asserts that the second handshake produces a fresh client, a freshconnectedregistration, and no main-tunnel disconnect.(Written by Copilot)
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 anincompatiblemanaged connection instead of disposing it, so the renderer'striggerServerUpgradeflow can locate the protocol client and send_vscodeUpgradeover the existing transport. (Written by Copilot)Note: tunnels generate a fresh
connectionIdon every connect (unlike SSH'sreplaceRelaypath), so the corresponding "stale-handle on reconnect" fix from the SSH side is not needed reconnect alwayshere creates a brand-new protocol client.Match the production shape (server advertises supportedVersions: ['^' + PROTOCOL_VERSION] in protocolServerHandler.ts) instead of date-like placeholders. No behavioural change.
(Written by Copilot)