Agent Host: surface remote connection state in sessions - #334111
Conversation
6d213ab to
af5ecf2
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Protocol-version inconsistency and several connection-state, throttling, and accessibility defects remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 8
New issues introduced by this change (9)
| Severity | Finding |
|---|---|
 |
src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts — Retaining disconnected entries changes connections from a live-connection list into a status… |
|
src/vs/platform/agentHost/common/remoteAgentHostBootstrapProgress.ts — If the event loop is delayed past _intervalMs, a new line can take this immediate branch while… |
|
src/vs/platform/agentHost/common/state/protocol/version/registry.ts — 1.0.0 is now advertised as the preferred protocol while PROTOCOL_VERSION above is still… |
|
src/vs/sessions/browser/parts/remoteHostUnavailableEmptyState.ts — The visible connection progress is not a live region, so screen-reader users will not hear… |
|
src/vs/sessions/browser/parts/sessionRemoteConnection.ts — This latch is local to one SessionRemoteConnection, but every chat group creates its own instance… |
|
src/vs/sessions/browser/parts/sessionRemoteConnection.ts — These observable writes notify autoruns synchronously. Clearing _attempt and _autoConnected… |
|
src/vs/sessions/browser/parts/sessionRemoteConnection.ts — An incompatible host is suppressed from the banner. For a chat with a rendered transcript,… |
|
src/vs/sessions/browser/parts/sessionRemoteConnection.ts — SessionReadOnlyBanner has role="status", so changing this message every second causes a screen… |
 |
src/vs/platform/agentHost/common/remoteAgentHostService.ts — This JSDoc describes a disconnect-reason accessor that no longer exists and is immediately followed… |
What changed in this PR
Surfaces remote Agent Host connection state throughout Sessions, adding recovery UI, WSL auto-start, reconnect controls, and safer terminal/task behavior.
Changes:
- Adds connection status projection, recovery banners, progress, retry, and auto-start UI.
- Preserves disconnected host state and improves reconnect/bootstrap handling.
- Adds broad unit and screenshot coverage.
| File | Description |
|---|---|
src/vs/sessions/test/browser/sessionReadOnlyBanner.fixture.ts |
Adds connection banner fixtures. |
src/vs/sessions/test/browser/remoteHostUnavailableEmptyState.fixture.ts |
Adds recovery-state fixtures. |
src/vs/sessions/test/browser/chatGroupsView.test.ts |
Tests connection recovery surfaces. |
src/vs/sessions/SESSIONS.md |
Documents remote connection state. |
src/vs/sessions/services/sessions/test/browser/visibleSessions.test.ts |
Tests status forwarding. |
src/vs/sessions/services/sessions/common/session.ts |
Defines session-facing connection status. |
src/vs/sessions/services/sessions/browser/visibleSessions.ts |
Forwards status through wrappers. |
src/vs/sessions/contrib/terminal/test/browser/sessionsTerminalContribution.test.ts |
Tests terminal connection gating. |
src/vs/sessions/contrib/terminal/test/browser/agentHostSessionTaskRunner.test.ts |
Tests task connection gating. |
src/vs/sessions/contrib/terminal/browser/sessionsTerminalContribution.ts |
Defers terminals while unavailable. |
src/vs/sessions/contrib/terminal/browser/agentHostSessionTaskRunner.ts |
Blocks unavailable-host tasks. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/wslAgentHost.contribution.test.ts |
Tests WSL status wiring. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostSessionsProvider.test.ts |
Tests projected provider status. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/wslAgentHost.contribution.ts |
Adds WSL auto-start policy. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/webSocketAgentHost.contribution.ts |
Removes removal override. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/sshAgentHost.contribution.ts |
Removes removal override. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/remoteAgentHostSessionsProvider.ts |
Exposes status and reconnect controls. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/remoteAgentHost.contribution.ts |
Registers WSL auto-start setting. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/entryDrivenProviderContribution.ts |
Unifies connection cleanup. |
src/vs/sessions/contrib/providers/agentHost/test/browser/localAgentHostSessionsProvider.test.ts |
Verifies local status remains absent. |
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts |
Projects host status into sessions. |
src/vs/sessions/contrib/chat/test/browser/chatView.test.ts |
Tests delayed provider loading. |
src/vs/sessions/contrib/chat/browser/chatView.ts |
Retries unresolved chat loads. |
src/vs/sessions/common/agentHostSessionsProvider.ts |
Extends provider recovery API. |
src/vs/sessions/browser/parts/sessionRemoteConnection.ts |
Derives connection presentation and recovery. |
src/vs/sessions/browser/parts/sessionReadOnlyBanner.ts |
Supports connection icons and actions. |
src/vs/sessions/browser/parts/remoteHostUnavailableEmptyState.ts |
Implements centered recovery UI. |
src/vs/sessions/browser/parts/media/remoteHostUnavailableEmptyState.css |
Styles the recovery UI. |
src/vs/sessions/browser/parts/chatView.ts |
Exposes transcript visibility. |
src/vs/sessions/browser/parts/chatGroupView.ts |
Integrates recovery surfaces. |
src/vs/platform/agentHost/test/node/wslRemoteAgentHostService.test.ts |
Tests WSL timing and progress. |
src/vs/platform/agentHost/test/electron-browser/remoteAgentHostService.test.ts |
Tests retained state and reconnects. |
src/vs/platform/agentHost/test/electron-browser/agentHostProtocolClient.test.ts |
Tests reasons and retry deadlines. |
src/vs/platform/agentHost/test/common/remoteAgentHostBootstrapProgress.test.ts |
Tests bootstrap progress parsing. |
src/vs/platform/agentHost/node/wslRemoteAgentHostService.ts |
Splits startup and idle budgets. |
src/vs/platform/agentHost/node/sshRemoteAgentHostHelpers.ts |
Re-exports shared redaction. |
src/vs/platform/agentHost/electron-browser/wslRemoteAgentHostServiceImpl.ts |
Classifies stopped WSL hosts. |
src/vs/platform/agentHost/common/wslRemoteAgentHost.ts |
Defines auto-start setting ID. |
src/vs/platform/agentHost/common/state/sessionTransport.ts |
Adds typed failure reasons. |
src/vs/platform/agentHost/common/state/protocol/version/registry.ts |
Adds protocol 1.0 support entry. |
src/vs/platform/agentHost/common/remoteAgentHostService.ts |
Expands connection status API. |
src/vs/platform/agentHost/common/remoteAgentHostBootstrapProgress.ts |
Adds shared progress reporter. |
src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts |
Retains failures and accelerates retries. |
src/vs/platform/agentHost/browser/agentHostProtocolClient.ts |
Exposes retry deadlines and reasons. |
.github/learnings/sessions.md |
Records Sessions debugging guidance. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Screenshot ChangesBase: Added (6)
|
A session backed by an unreachable remote host previously spun forever with
no explanation and no way to recover. Surface that state and make it
actionable:
- Derive a session-scoped `remoteConnectionStatus` from the provider so the
chat surface can react to connection state, not just host-scoped UI.
- Add machine-readable transport failure reasons so a stopped host is
distinguishable from an unreachable one.
- Show a centered recovery state with a Start action when a session has no
visible transcript, and a quiet inline banner when a rendered transcript
drops mid-use.
- Report live bootstrap progress ("Downloading server (24%)") while a
connect is in flight, via a shared progress parser.
- Split WSL startup, idle, and ceiling timeouts so a cold VM boot is not
mistaken for a hung connection.
- Gate terminal launches on host availability and re-resolve chat content
when a provider registers late.
Collect the connection concerns in ChatGroupView behind a single
SessionRemoteConnection, expressing state as observables with one derived
resolving which surface is visible. Read-only remains a peer of connection
state rather than part of it, since a read-only chat can also be
reconnecting. The quiet-reconnect delay is now a deadline, so re-arming is
idempotent instead of relying on a guard field.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds an opt-in, kind-scoped policy that starts a stopped remote agent host instead of waiting for the user to press Start. Providers expose it through `IAgentHostAutoConnect` (label, observable value, setter) and choose how it is backed; WSL backs it with `chat.agentHost.wsl.autoStart`. The recovery screen and the inline banner both render the checkbox and live connect progress. The Start action is never rendered while an automatic start is pending: the content derivation itself returns the connecting presentation, so this holds structurally rather than depending on autorun ordering. Two ordering bugs surfaced while building this. A connect that resolved without reaching the host cleared the in-flight attempt and re-opened the automatic gate, spinning forever behind a permanent "Waiting for agent host connection...". The gate is now latched per outage and released once the host is reachable, so a mid-session drop still gets its own attempt while an ineffective connect does not retrigger. The service fired its connection-change notification from inside a failing dial, before clearing the in-flight marker. A consumer dialing from that notification joined the dial that had just failed, so nothing reconnected and its `waitForConnection` never settled. The marker is now cleared before notifying; `_connectTo` clears by identity, so a dial started from the notification survives. Both are covered by regression tests that reproduce the original hangs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
While a protocol client waits out its exponential backoff the banner now reads "Reconnecting to <host> in 5s" and counts down, with a Try Now action that skips the remaining delay. Try Now accelerates the client's in-place retry rather than redialling, so the outbox and session state survive. It falls back to a fresh dial only when there is no client to accelerate, which happens now that a rejected factory retains a client-less entry. The backoff deadline travels on the `reconnecting` status. The client stays in that state across rounds, so the deadline is refreshed through a dedicated `onDidScheduleReconnect` event rather than by re-firing the connection-state event: consumers of that event do real work per transition, and repeating it each round would have unclear blast radius. Also offers a Retry action on the generic "Cannot reach <host>" state, on both the banner and the centered recovery surface. A tunnel that dies is usually transient. This stays manual: unlike a stopped WSL distro there is nothing local to start, so retrying automatically would only hammer an unreachable endpoint. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Retaining a disconnected entry turned `connections` from a liveness list into a status catalog, but three consumers still read presence as "connected": cloud sandbox negative reconciliation would never tear down a failed environment, and the cloud sandbox and Dev Container connect-failure paths would skip their cleanup and leak a staged connection. Each now tests the status. Documented the broadened semantics on the interface and removed an orphaned JSDoc block for an accessor that no longer exists. Reverted a stray `1.0.0` entry in the supported-protocol list. It broke the registry's documented first-entry invariant against `PROTOCOL_VERSION` and the handshake test, and had nothing to do with this work. `setSession` now writes in one transaction. These observable writes notify autoruns synchronously, so clearing the gates while the previous session was still selected could start the host being switched away from. The banner explains an incompatible host instead of staying silent. Once a transcript is rendered the centered recovery state is skipped, leaving the banner as the only surface, so suppressing it meant no explanation at all. Accessibility: the banner's live region now announces dedicated text rather than its visible text, so a per-second countdown no longer queues an utterance per tick, and connect progress is announced as it advances. Bootstrap progress discards a queued report before publishing an immediate one. If the event loop stalled past the throttle interval, the stale pending value could land after the newer one and make displayed progress run backwards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9e1c8e3 to
b0c6eef
Compare
Its field initializer read _instantiationService, a parameter property of the same class, which class-field semantics initialize after field initializers run. Caught by define-class-fields-check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Benjamin Christopher Simmonds (@benibenj)Matched files:
Sandeep Somavarapu (@sandy081)Matched files:
Ladislau Szomoru (@lszomoru)Matched files:
|
| // rejected by its precondition check, never reaching the handshake below. | ||
| // Retain the entry so its status remains visible to consumers. | ||
| const disconnectReason = err instanceof NonReconnectableTransportError ? err.reason : AgentHostTransportFailureReason.Unknown; | ||
| this._entries.set(address, { |
There was a problem hiding this comment.
AI Review: factory.createConnection can reject after its address was removed or remote hosts were disabled or disposed. Unlike the guarded success path below, this catch always recreates a disconnected entry; no later reconciliation is guaranteed, and re-adding or re-enabling the address can then skip automatic dialing because _entries.has(address) is true. Please mirror the success-path validity checks before retaining the failure, including service liveness, enablement, current configuration, and absence of a replacement entry.
| */ | ||
| reconnectNow(address: string): void { | ||
| const normalized = normalizeRemoteAgentHostAddress(address); | ||
| if (this._entries.get(normalized)?.client?.reconnectNow()) { |
There was a problem hiding this comment.
AI Review: AgentHostProtocolClient.reconnectNow() returns false once its backoff timer fires and the reconnect attempt is in flight. The service status is not refreshed immediately, so Try Now can remain clickable until the separate countdown tick; clicking then falls through to reconnect(), disposes the state-preserving client, and starts a fresh one. When an entry has a client, invoke its method and return regardless of the boolean result; use the fresh-dial fallback only when client is absent.
| } | ||
|
|
||
| this._actionDisposables.clear(); | ||
| dom.clearNode(this._actionContainer); |
There was a problem hiding this comment.
AI Review: The reconnect countdown updates every second, and each update calls setContent, which clears and recreates the Try Now link here. If a keyboard user has focused that link, the next tick detaches it and drops focus back to the document. Please update the existing action in place when it is unchanged, or explicitly preserve and restore focus across replacement; a focused fake-timer test would cover this.
|
|
||
| private _setRemoteHostUnavailableEmptyState(content: IRemoteHostUnavailableEmptyStateContent | undefined): void { | ||
| this._remoteHostUnavailableEmptyState.setContent(content); | ||
| this._contentContainer.classList.toggle('remote-host-unavailable', !!content); |
There was a problem hiding this comment.
AI Review: When recovery appears, this hides a potentially focused chat composer; when recovery disappears, the recovery subtree and its focused control are hidden. Chromium leaves focus in the invisible composer or moves it to body, and the connection-state autorun never calls ChatGroupView.focus(). Please detect whether focus belongs to the outgoing surface before toggling it and, only then, focus the incoming recovery state or chat view.
| title: derived(this, reader => this._defaultChatTitleOverride.read(reader) ?? this.title.read(reader)), | ||
| updatedAt: this.updatedAt, | ||
| status: derived(this, reader => this._defaultChatStatusOverride.read(reader) ?? this.status.read(reader)), | ||
| status: toPresentedSessionStatus(this, defaultChatStatus, connectionStatus), |
There was a problem hiding this comment.
AI Review: This makes mainChat.status connection-aware, but the visible single-chat header still reads raw session.status. If that status is InProgress or NeedsInput, its spinner remains beside the new unavailable-host recovery UI. Please have the header use mainChat.status for single-chat presentation, or derive an equivalent presentation status from remoteConnectionStatus, while preserving the raw lifecycle state needed elsewhere.
| } catch (error) { | ||
| this._logService.info(`[SessionRemoteConnection] connect rejected: ${error}`); | ||
| this._failAttempt(session, statusBefore); | ||
| onUnexpectedError(error); |
There was a problem hiding this comment.
AI Review: provider.connect() can reject for routine remote-host failures. This catch already logs the rejection and transitions back to the recovery UI, but onUnexpectedError also sends it through BaseErrorTelemetry as an UnhandledError; the new tests have to replace the global handler for these expected cases. Please remove this escalation and keep the explicit logging and recovery state.
| this._wiredAddresses.add(address); | ||
| } | ||
| } else if (this._clearConnectionOnRemoval && !connectionInfo && this._wiredAddresses.delete(address)) { | ||
| } else if (this._wiredAddresses.delete(address)) { |
There was a problem hiding this comment.
AI Review: During an in-place reconnect, the service sets entry.connected = false while retaining the same protocol client. getConnection() then returns undefined, so this branch calls clearConnection() on every soft reconnect, clearing subscriptions, automation/root state, and unsent new-session drafts despite the client preserving its state and outbox. Please keep the provider bound while status is reconnecting and clear only for terminal disconnect, incompatibility, or removal.
|
|
||
| const surface = derived<IChatGroupSurface>(reader => { | ||
| const readOnly = readOnlyContent.read(reader); | ||
| if (readOnly) { |
There was a problem hiding this comment.
AI Review: readOnlyContent wins before transcript and recovery checks. A failed cloud-sandbox open can produce a non-archived read-only fallback with empty history and a disconnected provider that supports Retry; this branch then shows only "This chat is read-only" over an empty transcript and suppresses recovery. Please keep the archived/Unarchive case at highest priority, but let remote recovery win over the generic read-only banner or merge its action into that banner.
| } | ||
| this._activeKey = undefined; | ||
| this._activeSessionId = undefined; | ||
| if (!preserveActiveTerminalState) { |
There was a problem hiding this comment.
AI Review: If reconnecting starts while initial terminal creation is pending, this path invalidates that operation but preserves _activeKey and _activeSessionId. The stale terminal is discarded, while the connected update sees matching markers and returns without creating a replacement, leaving no automatic retry. Please preserve these markers only when a terminal is already established, or clear them when invalidating pending creation.
Remote agent host connection state was modelled well enough for the workspace picker, but nothing in the session surface consumed it. A session whose host had gone away sat in a permanent loading state with no explanation and no way out — several people resorted to bouncing through the workspace picker to force a reconnect.
This makes connection state a first-class part of the session surface.
What a user sees
Downloading server (24%),Waiting for agent host connection…Reconnecting…Reconnecting to Ubuntu in 5s, counting down, with Try NowCannot reach X, dead endOpt-in "Automatically Start WSL When Opening Chats" (
chat.agentHost.wsl.autoStart, default off) starts a stopped host instead of waiting for a click. Scoped per host kind, and per outage rather than per session — a mid-session drop gets its own attempt. Exposed throughIAgentHostAutoConnectso any provider can back it however it likes; WSL is the only one wired up today.Try Now accelerates the protocol client's in-place retry rather than redialling, so the outbox and session state survive.
How it works
ISession.remoteConnectionStatusprojects provider connection status into the session model (undefinedfor local sessions).SessionRemoteConnectionowns presentation and recovery and derives exactly one surface — read-only banner, centered recovery, or connection banner — so the three can't fight over the same space. Disconnects carry a machine-readable reason (HostNotRunningvsUnknown) so the UI can tell "switched off, startable" from "unreachable".The Start action is never rendered while an automatic start is pending: the content derivation itself returns the connecting presentation, so this holds structurally rather than depending on autorun ordering.
Bugs found along the way
Three were pre-existing and reachable without any of this; the new UI just made them visible.
waitForConnectionnever settled — an indefinite hang.Each has a regression test that reproduces the original symptom — verified by reverting the fix and confirming the test hangs or times out rather than merely failing.
Cleanup
Removed the
_clearConnectionOnRemovalopt-out and its SSH/WebSocket overrides so clear-on-removal is uniform (a removed SSH host used to come back). Extracted the duplicated bootstrap-progress log parsing into a shared reporter. Replaced string disconnect reasons with aconst enumincluding a requiredUnknown, so a new reason can't be silently forgotten. Dropped a redundant disconnect-reason map now that the status already carries it. Net ~190 lines of duplication and dead paths removed.Notes for reviewers
The duplication between
SessionRemoteConnectionStatusand the platform status type is deliberate — it keepssession.tsfree of platform imports. The exhaustive projection switch is what catches drift, and it did during this work.Unifying this behind one status model is what makes the richer flows above possible at all; the previous per-transport implementations had no shared vocabulary to build on.
Tested by hand against WSL (stopped distro, mid-session shutdown, cold boot, reconnect) and tunnels.