agentHost: Enforce fresh remote transport lifecycle - #336330
Draft
roblourens wants to merge 1 commit into
Draft
roblourens wants to merge 1 commit into
roblourens wants to merge 1 commit into
Conversation
Separate ordinary ensure/open operations from explicit reconnects so initialized relays retain their protocol clients while replacement clients always receive fresh transports. Make SSH and WSL relay replacement explicit, preserve tunnel metadata and location-change behavior, and cover connection races across transports.\n\nFixes #335919.\n\n(Written by Copilot)\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Concurrent SSH calls and WSL recovery races can skip replacement or leave invalid or unowned relays.
Get a fresh assessment by requesting another Copilot review.
Review tier: Balanced
Findings: 1
Open (3)
What changed in this PR
Separates ordinary remote-host connection reuse from explicit replacement while enforcing fresh transport ownership across renderer/shared-process boundaries.
Changes:
- Adds distinct ensure and reconnect lifecycle paths.
- Introduces explicit SSH/WSL relay replacement modes and ownership tracking.
- Expands lifecycle, race, metadata, and regression coverage.
| File | Description |
|---|---|
src/vs/sessions/contrib/providers/remoteAgentHost/test/electron-browser/tunnelAgentHostServiceImpl.test.ts |
Tests desktop tunnel ensure/reconnect behavior. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/tunnelAgentHost.contribution.test.ts |
Tests serialized tunnel reconnects. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostSessionsProvider.test.ts |
Tests explicit reconnect hook selection. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/browserTunnelAgentHostService.test.ts |
Tests browser/web tunnel lifecycle and metadata. |
src/vs/sessions/contrib/providers/remoteAgentHost/REMOTE_AGENT_HOST_SESSIONS_PROVIDER.md |
Documents transport ownership invariants. |
src/vs/sessions/contrib/providers/remoteAgentHost/electron-browser/tunnelAgentHostServiceImpl.ts |
Splits desktop tunnel ensure and replacement. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/wslAgentHost.contribution.ts |
Separates WSL ensure and recovery hooks. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/webTunnelAgentHostService.ts |
Splits web tunnel ensure and replacement. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/webTunnelAgentHostService.contribution.ts |
Delegates tunnel reconnection. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/tunnelAgentHost.contribution.ts |
Serializes and tracks tunnel operations. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/sshAgentHost.contribution.ts |
Separates SSH ensure and recovery paths. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/remoteAgentHostSessionsProvider.ts |
Adds explicit reconnect configuration. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/entryDrivenProviderContribution.ts |
Forwards reconnect hooks to providers. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/browserTunnelAgentHostService.ts |
Splits browser tunnel ensure and replacement. |
src/vs/platform/agentHost/test/node/wslRemoteAgentHostService.test.ts |
Tests WSL relay replacement. |
src/vs/platform/agentHost/test/node/sshRemoteAgentHostService.test.ts |
Tests SSH replacement modes and identifiers. |
src/vs/platform/agentHost/test/electron-browser/wslRemoteAgentHostService.test.ts |
Tests renderer-side WSL ensure behavior. |
src/vs/platform/agentHost/test/electron-browser/sshRemoteAgentHostService.test.ts |
Tests SSH reuse, replacement, and ownership. |
src/vs/platform/agentHost/test/electron-browser/remoteAgentHostService.test.ts |
Tests shared lifecycle and race handling. |
src/vs/platform/agentHost/node/wslRemoteAgentHostService.ts |
Implements WSL reuse/replacement modes. |
src/vs/platform/agentHost/node/sshRemoteAgentHostService.ts |
Implements SSH replacement modes and relay IDs. |
src/vs/platform/agentHost/electron-browser/wslRemoteAgentHostServiceImpl.ts |
Tracks mutable WSL relay ownership. |
src/vs/platform/agentHost/electron-browser/sshRemoteAgentHostServiceImpl.ts |
Tracks mutable SSH relay ownership. |
src/vs/platform/agentHost/common/wslRemoteAgentHost.ts |
Defines WSL connection modes. |
src/vs/platform/agentHost/common/tunnelAgentHost.ts |
Adds the tunnel reconnect contract. |
src/vs/platform/agentHost/common/sshRemoteAgentHost.ts |
Defines SSH connection modes. |
src/vs/platform/agentHost/common/remoteAgentHostService.ts |
Adds ensure semantics and fresh-transport contract. |
src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts |
Implements retention, queued replacement, and error tracking. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| async connect(config: ISSHAgentHostConfig, replaceRelay?: boolean): Promise<ISSHConnectResult> { | ||
| async connect(config: ISSHAgentHostConfig, mode: SSHConnectionMode = SSHConnectionMode.Reuse): Promise<ISSHConnectResult> { |
Comment on lines
+306
to
+312
| connection: this._relayClientFactory.createClient(this._mainService, result.connectionId, result.address, result, remoteAgentHostCommand, connectionId => { | ||
| if (this._connections.get(handle.connectionId) === handle) { | ||
| this._connections.delete(handle.connectionId); | ||
| handle.replaceConnectionId(connectionId); | ||
| this._connections.set(handle.connectionId, handle); | ||
| this._onDidChangeConnections(); | ||
| } |
Comment on lines
+199
to
+201
| return mode === WSLConnectionMode.Reuse | ||
| ? existingPendingConnect | ||
| : existingPendingConnect.then(() => this.connect(config, WSLConnectionMode.Replace)); |
Contributor
Screenshot ChangesBase: Changed (1)
|
| Before | After |
|---|---|
 |
 |
5 insignificant change(s) omitted (≤20 px, Δ≤2). See CI logs for details.
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.
Summary
Architectural invariant
A newly constructed AgentHostProtocolClient must never be bound to a transport already initialized by another client. An already-live transport instead retains its protocol client. Ordinary ensure/open operations preserve connected, connecting, reconnecting, and incompatibly-live clients; explicit recovery and preferred-location changes serialize behind in-flight work and replace the connection. Protocol soft recovery retains the client and replaces only its relay.
This fixes the repeated SSH picker flow where a new renderer client sent a second initialize over the shared process's retained relay and received Method not found: initialize.
Validation
A real repeated-picker run against a dedicated SSH host or WSL was not available in this environment. The renderer/shared-process lifecycle is covered with focused transport tests. Config-less SSH recovery continues to use existing agent-auth fallback because credentials are intentionally not persisted.
Fixes #335919.
(Written by Copilot)