Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/learnings/sessions.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,20 @@ Scope: `src/vs/sessions/**`
- **Evidence:** Historical issues included actions targeting the window-global session, stale repository context on recycled rows, and late async results publishing into a replacement view.
- **Disposition:** Candidate for focused UI specifications and lifecycle tests.

## Attribute a console error to the provider that backs the surface

- **Scope:** `src/vs/sessions/contrib/providers/**`
- **Learning:** The sessions list resolves every registered provider, so an error in the console may come from a provider unrelated to the surface the user was operating. Identify which provider backs that surface before editing; a stack trace shows where an error was thrown, not which feature produced it. Several providers share a product name across the workbench and the Copilot extension without sharing an implementation.
- **Evidence:** A cloud sandbox report was traced into the Copilot extension's similarly named cloud provider, which shares neither code nor identifier with the Agents Window sandbox provider.
- **Disposition:** Candidate for the Sessions skill if provider-attribution mistakes recur.

## Rule out local product configuration before suspecting product code

- **Scope:** `src/vs/sessions/**`, `src/vs/platform/agentHost/**`
- **Learning:** Features reading `product.defaultChatAgent` can misbehave in dev builds because `product.overrides.json` is gitignored and drifts from the shipped configuration without appearing in any diff. Compare the specific keys a feature reads against the distro mixin before treating the behavior as a code defect, and note that ordered configuration arrays carry meaning by index.
- **Evidence:** A rotated `providerScopes` array left index 0 — the permissive scope set that the cloud sandbox client reads directly — without repository access, presenting as an authentication bug in feature code.
- **Disposition:** Candidate for repository onboarding or debugging guidance if dev-only configuration drift recurs.

## Capture live editor state independently of persistence

- **Scope:** `src/vs/sessions/contrib/layout/**`
Expand Down
58 changes: 49 additions & 9 deletions src/vs/platform/agentHost/browser/agentHostProtocolClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { normalizeLegacyActionEnvelope } from '../common/state/legacyProtocolCom
import { SUPPORTED_PROTOCOL_VERSIONS } from '../common/state/protocol/version/registry.js';
import { isJsonRpcNotification, isJsonRpcRequest, isJsonRpcResponse, ProtocolError, ReconnectResultType, type ProtocolMessage, type IStateSnapshot } from '../common/state/sessionProtocol.js';
import { type IVscodeUpgradeResult } from '../common/state/protocolUpgrade.js';
import { isClientTransport, NonReconnectableTransportError, type IProtocolTransport } from '../common/state/sessionTransport.js';
import { isClientTransport, NonReconnectableTransportError, type AgentHostTransportFailureReason, type IProtocolTransport } from '../common/state/sessionTransport.js';
import { AhpErrorCodes, JsonRpcErrorCodes } from '../common/state/protocol/errors.js';
import { ChatSourceKind, ContentEncoding, ResourceRequestParams, type CompletionsParams, type CompletionsResult, type CreateTerminalParams, type ResolveSessionConfigResult, type SessionConfigCompletionsResult } from '../common/state/protocol/commands.js';
import type { InvokeChangesetOperationParams, InvokeChangesetOperationResult } from '../common/state/protocol/channels-changeset/commands.js';
Expand Down Expand Up @@ -156,6 +156,8 @@ interface IReconnectState {
attempt: number;
/** Timer for the next scheduled attempt, if any. */
timeoutHandle: ReturnType<typeof setTimeout> | undefined;
/** Deadline for the next scheduled attempt, if any. */
nextAttemptAt: number | undefined;
}

/**
Expand Down Expand Up @@ -248,14 +250,16 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
private readonly _onDidReceiveOtlpLogs = this._register(new Emitter<OtlpExportLogsParams>());
readonly onDidReceiveOtlpLogs = this._onDidReceiveOtlpLogs.event;

private readonly _onDidClose = this._register(new Emitter<void>());
private readonly _onDidClose = this._register(new Emitter<AgentHostTransportFailureReason | undefined>());
readonly onDidClose = this._onDidClose.event;

private readonly _onDidFatalClose = this._register(new Emitter<ProtocolError>());
readonly onDidFatalClose = this._onDidFatalClose.event;

private readonly _onDidChangeConnectionState = this._register(new Emitter<AgentHostClientState>());
readonly onDidChangeConnectionState = this._onDidChangeConnectionState.event;
private readonly _onDidScheduleReconnect = this._register(new Emitter<void>());
readonly onDidScheduleReconnect = this._onDidScheduleReconnect.event;

/**
* Discriminated state union. Read via narrowing (`_state.kind === ...`);
Expand Down Expand Up @@ -346,6 +350,13 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
return this._state.kind;
}

/** Deadline for the next scheduled reconnect attempt, if one is pending. */
get nextReconnectAt(): number | undefined {
return this._state.kind === AgentHostClientState.Reconnecting
? this._state.reconnect.nextAttemptAt
: undefined;
}

/**
* The latest `initialize` response from the host, or `undefined` if
* the handshake has not completed yet. Exposed observably so callers can
Expand Down Expand Up @@ -473,6 +484,14 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
if (this._state.kind === next.kind) {
return;
}
if (this._state.kind === AgentHostClientState.Reconnecting) {
const reconnect = this._state.reconnect;
if (reconnect.timeoutHandle !== undefined) {
clearTimeout(reconnect.timeoutHandle);
reconnect.timeoutHandle = undefined;
}
reconnect.nextAttemptAt = undefined;
}
this._state = next;
this._onDidChangeConnectionState.fire(next.kind);
}
Expand All @@ -487,7 +506,7 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
}

private _newReconnectState(): IReconnectState {
return { gate: this._newReconnectGate(), outbox: [], attempt: 0, timeoutHandle: undefined };
return { gate: this._newReconnectGate(), outbox: [], attempt: 0, timeoutHandle: undefined, nextAttemptAt: undefined };
}

override dispose(): void {
Expand Down Expand Up @@ -555,7 +574,7 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
throw error;
}
if (error instanceof NonReconnectableTransportError) {
this._handleFatalClose(protocolError);
this._handleFatalClose(protocolError, error.reason);
throw error;
}
if (this._state.kind === AgentHostClientState.Reconnecting) {
Expand Down Expand Up @@ -680,6 +699,24 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
return true;
}

/**
* Skips the remaining backoff and retries immediately. Returns `false` when
* there is no pending retry to accelerate.
*/
reconnectNow(): boolean {
if (this._state.kind !== AgentHostClientState.Reconnecting || this._state.reconnect.timeoutHandle === undefined) {
return false;
}
const reconnect = this._state.reconnect;
clearTimeout(reconnect.timeoutHandle);
reconnect.timeoutHandle = undefined;
reconnect.nextAttemptAt = undefined;
reconnect.attempt = 0;
this._onDidScheduleReconnect.fire();
void this._attemptReconnect();
return true;
}

private _scheduleReconnect(userInitiated = false): void {
if (this._state.kind !== AgentHostClientState.Reconnecting || !this._transportFactory) {
return;
Expand All @@ -703,12 +740,15 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
const attempt = reconnect.attempt + 1;
const delay = computeReconnectDelay(this._reconnectPolicy, attempt);
this._logService.info(`[RemoteAgentHostProtocol] Reconnecting to ${this._address} in ${delay}ms (attempt ${attempt}).`);
reconnect.nextAttemptAt = Date.now() + delay;
reconnect.timeoutHandle = setTimeout(() => {
if (this._state.kind === AgentHostClientState.Reconnecting) {
this._state.reconnect.timeoutHandle = undefined;
this._state.reconnect.nextAttemptAt = undefined;
}
void this._attemptReconnect();
}, delay);
this._onDidScheduleReconnect.fire();
}

private async _attemptReconnect(): Promise<void> {
Expand Down Expand Up @@ -779,7 +819,7 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
}
if (err instanceof NonReconnectableTransportError) {
const protocolError = new ProtocolError(AHP_CLIENT_CONNECTION_CLOSED, err.message);
this._handleFatalClose(protocolError);
this._handleFatalClose(protocolError, err.reason);
return;
}
if (err instanceof InitialAuthenticationError) {
Expand Down Expand Up @@ -1684,12 +1724,12 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
}
}

private _handleFatalClose(error: ProtocolError): void {
private _handleFatalClose(error: ProtocolError, reason?: AgentHostTransportFailureReason): void {
this._onDidFatalClose.fire(error);
this._handleClose(error);
this._handleClose(error, reason);
}

private _handleClose(error: ProtocolError): void {
private _handleClose(error: ProtocolError, reason?: AgentHostTransportFailureReason): void {
if (this._state.kind === AgentHostClientState.Closed) {
return;
}
Expand All @@ -1715,7 +1755,7 @@ export class AgentHostProtocolClient extends Disposable implements IAgentConnect
this._implicitReadGrants.clear();
this._resourceService.connectionClosed(this._resourceIdentity);
this._transitionTo({ kind: AgentHostClientState.Closed, error });
this._onDidClose.fire();
this._onDidClose.fire(reason);
}

private async _raceClose<T>(promise: Promise<T>): Promise<T> {
Expand Down
75 changes: 59 additions & 16 deletions src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import {
RemoteAgentHostEntryType,
} from '../common/remoteAgentHostService.js';
import { computeReconnectDelay, hasExhaustedReconnectAttempts } from '../common/reconnectPolicy.js';
import { NonReconnectableTransportError } from '../common/state/sessionTransport.js';
import { AgentHostTransportFailureReason, NonReconnectableTransportError } from '../common/state/sessionTransport.js';
import { AgentHostProtocolClient, InitialAuthenticationError } from './agentHostProtocolClient.js';
import { WebSocketClientTransport } from './webSocketClientTransport.js';
import { AGENT_HOST_LABEL_FORMATTER, AGENT_HOST_SCHEME, agentHostAuthority, normalizeRemoteAgentHostAddress } from '../common/agentHostUri.js';
Expand All @@ -51,7 +51,7 @@ import { agentsWindowAgentHostClientInfo, editorWindowAgentHostClientInfo } from
/** Tracks a single remote connection through its lifecycle. */
interface IConnectionEntry {
readonly store: DisposableStore;
readonly client: IRemoteAgentHostProtocolClient;
client?: IRemoteAgentHostProtocolClient;
/**
* Optional teardown for the shared-process tunnel that this entry's
* transport is using (SSH or dev-tunnels). Tracked separately from
Expand Down Expand Up @@ -234,8 +234,8 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo
result.push({
address,
name: this._names.get(address) ?? address,
clientId: entry.client.clientId,
defaultDirectory: entry.client.defaultDirectory,
clientId: entry.client?.clientId,
defaultDirectory: entry.client?.defaultDirectory,
status: entry.status,
});
}
Expand Down Expand Up @@ -285,9 +285,9 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo

async triggerServerUpgrade(address: string, method: string): Promise<IVscodeUpgradeResult> {
const normalized = normalizeRemoteAgentHostAddress(address);
const entry = this._entries.get(normalized);
if (!entry) {
throw new Error(`No remote agent host entry found for ${address}.`);
const client = this._entries.get(normalized)?.client;
if (!client) {
throw new Error(`No usable remote agent host client found for ${address}.`);
}
// The protocol client may be in any state: it might have completed
// the handshake (Connected) or it might be sitting on an
Expand All @@ -296,7 +296,7 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo
// method name the host advertised in its `_meta` payload; the
// server handler allows it pre-`initialize`.
const result = await raceTimeout(
entry.client.triggerVscodeUpgrade(method),
client.triggerVscodeUpgrade(method),
RemoteAgentHostService.UpgradeRequestTimeout,
);
if (result === undefined) {
Expand Down Expand Up @@ -365,6 +365,17 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo
void this._connectTo(entryToReconnect, { userInitiated });
}

/**
* Skips a protocol client's pending backoff, or starts a fresh user-initiated dial.
*/
reconnectNow(address: string): void {
const normalized = normalizeRemoteAgentHostAddress(address);
if (this._entries.get(normalized)?.client?.reconnectNow()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
this.reconnect(normalized, true);
}

async waitForConnection(address: string): Promise<IRemoteAgentHostConnectionInfo> {
if (this._store.isDisposed) {
throw new Error('Remote agent host service is disposed.');
Expand Down Expand Up @@ -438,11 +449,11 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo
notifyConnectionClosed(address: string): void {
const normalized = normalizeRemoteAgentHostAddress(address);
const entry = this._entries.get(normalized);
if (entry) {
if (entry?.client) {
this._logService.info(`[RemoteAgentHost] notifyConnectionClosed: notifying protocol client for ${normalized}`);
entry.client.notifyTransportClosed();
} else {
this._logService.info(`[RemoteAgentHost] notifyConnectionClosed: no entry found for ${normalized} (already removed?)`);
this._logService.info(`[RemoteAgentHost] notifyConnectionClosed: no active client found for ${normalized}`);
}
}

Expand Down Expand Up @@ -576,7 +587,27 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo
createdConnection = await factory.createConnection(entryToCreate, options);
} catch (err) {
this._logService.error(`[RemoteAgentHost] Failed to create a connection to ${address}. Verify address and connectionToken`, err);
// A factory can fail before any client exists — a stopped WSL distro is
// 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, {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

store: new DisposableStore(),
connected: false,
status: RemoteAgentHostConnectionStatus.disconnectedBecause(disconnectReason),
reconnectTransfersTransportOwnership: false,
});
Comment thread
connor4312 marked this conversation as resolved.
this._rejectPendingConnectionWait(address, err);
// Clear the in-flight marker before notifying. A consumer may dial again
// from this notification — an automatic start does — and `reconnect`
// joins a pending dial rather than racing it. Leaving the marker set
// would join *this* dial, which has already failed, so the new request
// would never connect and its `waitForConnection` would never settle.
// `_connectTo` clears by identity, so a fresh dial started here survives.
this._pendingConnects.delete(address);
// Nothing else reports this failure: no entry was created, so consumers
// only learn the address became unavailable from this notification.
this._onDidChangeConnections.fire();
if (!isTerminalConnectError(err) && !this._store.isDisposed && this._remoteAgentHostsEnabled.get()) {
this._scheduleReconnect(address, entryToCreate.connectionToken);
}
Expand Down Expand Up @@ -611,13 +642,15 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo
// current entry for this address is still the one we created.
const isCurrentEntry = () => this._entries.get(address) === entry;

store.add(client.onDidClose(() => {
store.add(client.onDidClose(reason => {
if (!isCurrentEntry()) {
return;
}
this._logService.warn(`[RemoteAgentHost] Connection closed: ${address}`);
entry.connected = false;
entry.status = RemoteAgentHostConnectionStatus.disconnected;
entry.status = RemoteAgentHostConnectionStatus.disconnectedBecause(reason ?? AgentHostTransportFailureReason.Unknown);
entry.client = undefined;
disposeEntry(entry);
this._onDidChangeConnections.fire();
// Schedule reconnect if the address is still configured. This is
// the "fatal" path — the protocol client already gave up its own
Expand All @@ -628,14 +661,23 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo

// Surface self-healing transport drops separately so outer reconnect
// loops do not replace the protocol client while it restores itself.
store.add(client.onDidScheduleReconnect(() => {
// The client stays `reconnecting` across backoff rounds, so only this
// event reports that the deadline moved.
if (!isCurrentEntry() || entry.status.kind !== 'reconnecting') {
return;
}
entry.status = RemoteAgentHostConnectionStatus.reconnectingUntil(client.nextReconnectAt);
this._onDidChangeConnections.fire();
}));
store.add(client.onDidChangeConnectionState(state => {
if (!isCurrentEntry()) {
return;
}
switch (state) {
case 'reconnecting':
entry.connected = false;
entry.status = RemoteAgentHostConnectionStatus.reconnecting;
entry.status = RemoteAgentHostConnectionStatus.reconnectingUntil(client.nextReconnectAt);
this._onDidChangeConnections.fire();
break;
case 'connected':
Expand Down Expand Up @@ -715,9 +757,10 @@ export class RemoteAgentHostService extends Disposable implements IRemoteAgentHo
return;
}

entry.status = RemoteAgentHostConnectionStatus.disconnected;
// Clean up the failed entry
this._entries.delete(address);
const disconnectReason = err instanceof NonReconnectableTransportError ? err.reason : AgentHostTransportFailureReason.Unknown;
entry.status = RemoteAgentHostConnectionStatus.disconnectedBecause(disconnectReason);
entry.client = undefined;
// Clean up the failed client while retaining its entry and status.
disposeEntry(entry);
this._rejectPendingConnectionWait(address, err);
this._onDidChangeConnections.fire();
Expand Down
Loading
Loading