From e068885901d8e642238e063c289a2c9761288df6 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 24 Aug 2026 21:14:50 +1000 Subject: [PATCH 1/2] agent host: self-heal Copilot client cold-start config-changed abort (#332256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * agent host: self-heal Copilot client cold-start config-changed abort When a startup-config value changes while the Copilot SDK client is starting, `_ensureClient` aborts the now-stale start and throws `CopilotClientStartupConfigChangedError`. Because the next start uses the current config, that abort is transient — but it was surfaced to session restore, producing sticky failures such as "could not describe … yet" / "Couldn't open session". `_ensureClient` now transparently re-acquires the client once with the current config. The acquire-and-retry sequence is shared across all concurrent callers via `_ensureClientHealing`, so the retry budget is global (bounded by `MAX_STARTUP_CONFIG_RETRIES`) rather than per caller, and per-attempt coalescing in `_ensureClientOnce` is unchanged. No caller observes the transient abort. Related to https://github.com/microsoft/vscode-internalbacklog/issues/8895 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * agent host: address review on the _ensureClient self-heal Two findings from the automated review: - Clear `_ensureClientHealing` from inside the async sequence's `finally` rather than a trailing `.finally()` on a separate chain. The external chain ran one microtask after awaiting callers resumed, so a caller that resumed on success and immediately re-entered `_ensureClient` (e.g. after `_stopClient()`) could be handed the fulfilled handle for an already- stopped client, bypassing the `_clientStopping`/`_client` guards. Clearing in-sequence guarantees the handle is gone before the promise settles for any awaiter. Confirmed the ordering fix in isolation. - Strengthen the global-budget test so it actually discriminates. The previous version had both callers coalesce before attempt 1, so a per-caller budget would pass it too. The late joiner now arrives while attempt 2 is in flight (mid-retry) and is shown unable to drive a third start. Verified by mutation: removing the shared coalescing makes the test fail with startCallCount 3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../agentHost/node/copilot/copilotAgent.ts | 66 +++++- .../agentHost/test/node/copilotAgent.test.ts | 208 +++++++++++++++++- 2 files changed, 269 insertions(+), 5 deletions(-) diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts index 1866b6058b4553..83066cb3b0a69a 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts @@ -588,6 +588,14 @@ const COPILOT_DISCOVERY_BATCH_SIZE = 250; */ const CHAT_DISCOVERY_RETRY_DELAYS_MS = [250, 1_000, 5_000]; +/** + * How many times `_ensureClient` re-acquires the SDK client after a cold-start + * abort caused by a startup-config change. One extra attempt covers the common + * one-time startup settle observed in the field; the bound prevents livelock if + * the config keeps changing on every start. + */ +const MAX_STARTUP_CONFIG_RETRIES = 1; + /** `origin` value written by the VS Code extension-host Copilot CLI feature. */ const EXTENSION_HOST_CLI_MARKER_ORIGIN = 'vscode'; @@ -772,6 +780,12 @@ export class CopilotAgent extends Disposable implements IAgent { private _client: CopilotClient | undefined; private _clientStarting: Promise | undefined; + /** + * Coalesces the whole acquire-and-self-heal sequence in `_ensureClient` so + * that all concurrent callers share a single, global retry budget for + * startup-config-changed aborts (rather than each caller getting its own). + */ + private _ensureClientHealing: Promise | undefined; private _clientStopping: Promise | undefined; private _clientStartupAttemptCount = 0; private _resolvedProxy: string | undefined; @@ -1895,7 +1909,57 @@ export class CopilotAgent extends Disposable implements IAgent { throw terminalError; } - private async _ensureClient(): Promise { + /** + * Acquires the SDK client, transparently self-healing a single cold-start + * abort caused by a startup-config change (`CopilotClientStartupConfigChangedError`). + * That abort is transient: the superseded client was built with now-stale + * config and the next start uses the current config, so re-acquiring once + * returns a healthy client and no caller ever sees the abort. + * + * The re-acquire is bounded by {@link MAX_STARTUP_CONFIG_RETRIES}. All + * concurrent callers share one acquire-and-retry sequence via + * `_ensureClientHealing`, so the retry budget is global rather than per + * caller (a late caller cannot reset the budget and drive unbounded starts). + * The per-attempt coalescing in `_ensureClientOnce` (via `_clientStarting`) is + * unchanged. + */ + private _ensureClient(): Promise { + if (this._ensureClientHealing) { + return this._ensureClientHealing; + } + const healing = (async () => { + try { + for (let retries = 0; ; retries++) { + try { + return await this._ensureClientOnce(); + } catch (error) { + if (retries < MAX_STARTUP_CONFIG_RETRIES + && !this._shutdownPromise + && error instanceof CopilotClientStartupConfigChangedError) { + this._logService.info('[Copilot] Startup config changed while the client was starting; re-acquiring the client with the current config'); + continue; + } + throw error; + } + } + } finally { + // Clear the shared handle from inside the sequence so it is gone + // before this promise settles for any awaiting caller. Clearing it + // from a trailing `.finally()` on a separate chain would run one + // microtask too late: a caller resuming on success could re-enter + // `_ensureClient` (e.g. after `_stopClient()`) and be handed this + // fulfilled handle for an already-stopped client. Only one healing + // sequence is ever in flight — `_ensureClient` starts one only when + // the field is empty, and this is the only site that clears it — so + // this always owns the field here. + this._ensureClientHealing = undefined; + } + })(); + this._ensureClientHealing = healing; + return healing; + } + + private async _ensureClientOnce(): Promise { if (this._shutdownPromise) { throw new CancellationError(); } diff --git a/src/vs/platform/agentHost/test/node/copilotAgent.test.ts b/src/vs/platform/agentHost/test/node/copilotAgent.test.ts index aa93c3b3a43fc3..fddf06a0d5b84d 100644 --- a/src/vs/platform/agentHost/test/node/copilotAgent.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotAgent.test.ts @@ -3126,6 +3126,15 @@ suite('CopilotAgent', () => { } } + /** A client whose start can flip a startup-config value via `onAfterStart`. */ + class ConfigChangeOnStartClient extends TestCopilotClient { + onAfterStart: (() => void) | undefined; + override async start(): Promise { + await super.start(); + this.onAfterStart?.(); + } + } + class MutableLogService extends NullLogService { private _level = LogLevel.Info; @@ -3138,7 +3147,7 @@ suite('CopilotAgent', () => { } } - test('preserves configuration-changed outcome when stopping the started client fails', async () => { + test('self-heals a configuration-changed cold-start abort when stopping the started client fails', async () => { const client = new StopCountingClient([]); const startGate = new DeferredPromise(); client.startGate = startGate.p; @@ -3151,17 +3160,27 @@ suite('CopilotAgent', () => { configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: false }); startGate.complete(); - assert.strictEqual(await startup, undefined); + const catalog = await startup; const startupEvents = telemetryService.events.map(event => { const data = event.data as Record; - return { eventName: event.eventName, ...data, durationMs: typeof data.durationMs }; + return { + eventName: event.eventName, + outcome: data.outcome, + durationMs: typeof data.durationMs, + attemptNumber: data.attemptNumber, + startupFailureCause: data.startupFailureCause, + startupFailureResource: data.startupFailureResource, + startupExitCode: data.startupExitCode, + }; }); assert.deepStrictEqual({ + catalog, startCallCount: client.startCallCount, stopCount: client.stopCount, startupEvents, }, { - startCallCount: 1, + catalog: [], + startCallCount: 2, stopCount: 1, startupEvents: [{ eventName: 'agentHost.copilotClientStartup', @@ -3171,6 +3190,14 @@ suite('CopilotAgent', () => { startupFailureCause: 'configurationChanged', startupFailureResource: 'other', startupExitCode: undefined, + }, { + eventName: 'agentHost.copilotClientStartup', + outcome: 'success', + durationMs: 'number', + attemptNumber: 2, + startupFailureCause: undefined, + startupFailureResource: undefined, + startupExitCode: undefined, }], }); } finally { @@ -3181,6 +3208,179 @@ suite('CopilotAgent', () => { } }); + test('self-heals a clean configuration-changed cold-start abort and returns the catalog', async () => { + const sessionDataService = disposables.add(new TestSessionDataService()); + const ownedSession = AgentSession.uri('copilotcli', 'owned-selfheal'); + const ownedDb = sessionDataService.openDatabase(ownedSession); + await ownedDb.object.setMetadata('copilot.workingDirectory', URI.file('/workspace').toString()); + ownedDb.dispose(); + const client = new ConfigChangeOnStartClient([sdkSession('owned-selfheal')]); + const telemetryService = new RecordingTelemetryService(); + const { agent, configurationService } = createTestAgentContext(disposables, { sessionDataService, copilotClient: client, telemetryService }); + // Change a startup-config value only while the first client starts, so + // that start aborts as config-changed and the second start (with the + // now-current config) succeeds. + client.onAfterStart = () => { + if (client.startCallCount === 1) { + configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: false }); + } + }; + try { + const catalog = await agent.listChatsToMigrate(); + const startupOutcomes = telemetryService.events + .filter(event => event.eventName === 'agentHost.copilotClientStartup') + .map(event => (event.data as Record).outcome); + assert.deepStrictEqual({ + sessions: catalog?.map(session => sessionIdOfChat(session.chat)), + startCallCount: client.startCallCount, + stopCallCount: client.stopCallCount, + startupOutcomes, + }, { + sessions: ['owned-selfheal'], + startCallCount: 2, + stopCallCount: 1, + startupOutcomes: ['failure', 'success'], + }); + } finally { + await disposeAgent(agent); + } + }); + + test('self-heals a clean configuration-changed cold-start abort on the restore describe path', async () => { + const sessionDataService = disposables.add(new TestSessionDataService()); + const session = AgentSession.uri('copilotcli', 'restore-target'); + const db = sessionDataService.openDatabase(session); + await db.object.setMetadata('copilot.workingDirectory', URI.file('/workspace').toString()); + db.dispose(); + const client = new ConfigChangeOnStartClient([sdkSession('restore-target')]); + const { agent, configurationService } = createTestAgentContext(disposables, { sessionDataService, copilotClient: client }); + client.onAfterStart = () => { + if (client.startCallCount === 1) { + configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: false }); + } + }; + try { + const chat = defaultChatUri(session); + const metadata = await agent.getChatMetadata(chat, exactChatContext(session, chat, session)); + assert.deepStrictEqual({ + metadata: metadata && withoutUndefinedProperties(metadata), + startCallCount: client.startCallCount, + stopCallCount: client.stopCallCount, + getSessionMetadataCalls: client.getSessionMetadataCalls, + }, { + metadata: { + chat, + startTime: 1000, + modifiedTime: 2000, + summary: 'SDK restore-target', + workingDirectories: [URI.file('/workspace')], + }, + startCallCount: 2, + stopCallCount: 1, + getSessionMetadataCalls: ['restore-target'], + }); + } finally { + await disposeAgent(agent); + } + }); + + test('gives up after the bounded number of re-acquires when the startup config keeps changing', async () => { + const client = new ConfigChangeOnStartClient([]); + const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); + // Flip the value on every start so each attempt aborts as config-changed. + client.onAfterStart = () => { + configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: client.startCallCount % 2 === 0 }); + }; + try { + const result = await agent.listChatsToMigrate(); + assert.deepStrictEqual({ + result, + startCallCount: client.startCallCount, + }, { + result: undefined, + startCallCount: 2, + }); + } finally { + await disposeAgent(agent); + } + }); + + test('shares one global re-acquire budget: a late joiner cannot drive an extra start', async () => { + const releaseAttempt2 = new DeferredPromise(); + // Parks its own second start so the healing sequence is observably in + // flight (mid-retry, attempt 2 running) when the late joiner arrives. + class LateJoinerBudgetClient extends ConfigChangeOnStartClient { + override async start(): Promise { + if (this.startCallCount + 1 === 2) { + this.startGate = releaseAttempt2.p; + } + await super.start(); + } + } + const client = new LateJoinerBudgetClient([]); + const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); + // Alternate the value so every attempt's post-start config differs from + // its pre-start snapshot and aborts as config-changed. + client.onAfterStart = () => { + configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: client.startCallCount % 2 === 0 }); + }; + const ensureClient = () => (agent as unknown as { _ensureClient(): Promise })._ensureClient(); + try { + const first = ensureClient(); + // Wait until attempt 1 has aborted (config-changed) and attempt 2 has + // begun and parked on its gate: the sequence is now mid-retry. + for (let i = 0; i < 50 && client.startCallCount < 2; i++) { + await timeout(0); + } + // The late joiner arrives while attempt 2 is in flight. + const second = ensureClient(); + releaseAttempt2.complete(); + const outcomes = await Promise.allSettled([first, second]); + // Global budget: the late joiner shares the in-flight sequence, so it + // cannot force a third start. A per-caller budget would instead let + // `second` run its own retry after attempt 2 aborts, driving a third. + assert.deepStrictEqual({ + firstRejected: outcomes[0].status === 'rejected', + secondRejected: outcomes[1].status === 'rejected', + startCallCount: client.startCallCount, + }, { + firstRejected: true, + secondRejected: true, + startCallCount: 2, + }); + } finally { + releaseAttempt2.complete(); + await disposeAgent(agent); + } + }); + + test('coalesces concurrent client acquisitions across a single re-acquire', async () => { + const client = new ConfigChangeOnStartClient([]); + const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); + client.onAfterStart = () => { + if (client.startCallCount === 1) { + configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: false }); + } + }; + const ensureClient = () => (agent as unknown as { _ensureClient(): Promise })._ensureClient(); + try { + const first = ensureClient(); + const second = ensureClient(); + const [firstClient, secondClient] = await Promise.all([first, second]); + assert.deepStrictEqual({ + sameHealthyClient: firstClient === client && secondClient === client, + startCallCount: client.startCallCount, + stopCallCount: client.stopCallCount, + }, { + sameHealthyClient: true, + startCallCount: 2, + stopCallCount: 1, + }); + } finally { + await disposeAgent(agent); + } + }); + test('resolves the system proxy by default and bypasses it when disabled', async () => { const proxyResolver = new TestProxyResolver(); proxyResolver.resolvedProxy = 'http://system-proxy.example:8080'; From 588c06969866815b6bc8dfe8dd1616304e68881c Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 25 Aug 2026 00:52:26 +1000 Subject: [PATCH 2/2] agent host: normalize empty Kerberos proxy SPN to avoid spurious restart (#332306) * agent host: normalize empty Kerberos proxy SPN to avoid spurious restart An unset `http.proxyKerberosServicePrincipal` reaches the agent host two ways: absent (`undefined`) when the Copilot client first spawns, then as an empty string once the workbench mirrors its config (the transform coerces an unset value to `''` so that clearing a previously-set SPN still propagates under the host's merge reducer). `_refreshProxy` compared the two raw, so `'' !== undefined` was mistaken for a real proxy change and restarted the Copilot client. When that restart lands during a session restore it cancels the in-flight resume with a CancellationError that is not recovered, leaving an empty Chat Panel after restarting VS Code. Read the SPN through a single helper that normalizes `''` to `undefined` at both the applied-baseline (`_applyProxyEnv`) and effective-value (`_refreshProxy`) sites, so an absent-vs-empty transition is no longer seen as a change. Clearing a genuinely-set SPN still restarts. Also name the actual trigger in the restart log instead of the misleading `(none) -> (none)`. Fixes #332305 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../agentHost/node/copilot/copilotAgent.ts | 21 ++++- .../agentHost/test/node/copilotAgent.test.ts | 80 +++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts index 83066cb3b0a69a..ac16cc77919a39 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts @@ -4630,6 +4630,14 @@ export class CopilotAgent extends Disposable implements IAgent { // ---- helpers ------------------------------------------------------------ + /** + * Returns the effective Kerberos proxy SPN, treating an empty setting as absent. + */ + private _readKerberosSpn(env: Record): string | undefined { + const spn = env['COPILOT_PROXY_KERBEROS_SPN'] || this._configurationService.getRootValue(agentHostProxyConfigSchema, AgentHostProxyConfigKey.ProxyKerberosServicePrincipal); + return spn || undefined; + } + private _applyProxyEnv(env: Record): void { const proxy = this._isSystemProxyEnabled() ? this._resolvedProxy : undefined; this._appliedProxy = proxy; @@ -4639,7 +4647,7 @@ export class CopilotAgent extends Disposable implements IAgent { } this._logService.info('[Copilot] Resolved CAPI proxy and forwarded HTTP_PROXY/HTTPS_PROXY to Copilot SDK'); } - const kerberosSpn = env['COPILOT_PROXY_KERBEROS_SPN'] || this._configurationService.getRootValue(agentHostProxyConfigSchema, AgentHostProxyConfigKey.ProxyKerberosServicePrincipal); + const kerberosSpn = this._readKerberosSpn(env); this._appliedProxyKerberosSpn = kerberosSpn; if (kerberosSpn && !env['COPILOT_PROXY_KERBEROS_SPN']) { env['COPILOT_PROXY_KERBEROS_SPN'] = kerberosSpn; @@ -4683,7 +4691,7 @@ export class CopilotAgent extends Disposable implements IAgent { } this._resolvedProxy = proxy; const effectiveProxy = this._isSystemProxyEnabled() ? proxy : undefined; - const effectiveKerberosSpn = process.env['COPILOT_PROXY_KERBEROS_SPN'] || this._configurationService.getRootValue(agentHostProxyConfigSchema, AgentHostProxyConfigKey.ProxyKerberosServicePrincipal); + const effectiveKerberosSpn = this._readKerberosSpn(process.env); if (effectiveProxy === this._appliedProxy && effectiveKerberosSpn === this._appliedProxyKerberosSpn) { return; } @@ -4700,7 +4708,14 @@ export class CopilotAgent extends Disposable implements IAgent { return; } } - await this._requestClientRestart(`CAPI proxy configuration changed (${this._appliedProxy ?? '(none)'} -> ${effectiveProxy ?? '(none)'})`); + const changes: string[] = []; + if (effectiveProxy !== this._appliedProxy) { + changes.push(`proxy ${this._appliedProxy ?? '(none)'} -> ${effectiveProxy ?? '(none)'}`); + } + if (effectiveKerberosSpn !== this._appliedProxyKerberosSpn) { + changes.push('Kerberos SPN changed'); + } + await this._requestClientRestart(`CAPI proxy configuration changed (${changes.join(', ')})`); }).catch(error => this._logService.error('[Copilot] Failed to refresh CAPI proxy', error)); this._proxyRefresh = refresh; void refresh.finally(() => { diff --git a/src/vs/platform/agentHost/test/node/copilotAgent.test.ts b/src/vs/platform/agentHost/test/node/copilotAgent.test.ts index fddf06a0d5b84d..b7bfce679575fb 100644 --- a/src/vs/platform/agentHost/test/node/copilotAgent.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotAgent.test.ts @@ -3742,6 +3742,86 @@ suite('CopilotAgent', () => { } }); + test('does not restart the Copilot runtime when an unset Kerberos proxy SPN is mirrored as empty', async () => { + const client = new TestCopilotClient([]); + const proxyResolver = new TestProxyResolver(); + // The workbench mirrors an unset SPN as an empty string, which must not trigger a restart. + const previousSpnEnv = process.env['COPILOT_PROXY_KERBEROS_SPN']; + delete process.env['COPILOT_PROXY_KERBEROS_SPN']; + const { agent, configurationService } = createTestAgentContext(disposables, { + copilotClient: client, + proxyResolver, + }); + try { + await agent.listChatsToMigrate(); + const resolveProxyCallsBefore = proxyResolver.resolveProxyCalls; + configurationService.updateRootConfig({ [AgentHostProxyConfigKey.ProxyKerberosServicePrincipal]: '' }); + proxyResolver.fireConfigurationChange(); + for (let i = 0; i < 20; i++) { + await timeout(0); + } + + assert.deepStrictEqual({ + startCallCount: client.startCallCount, + stopCallCount: client.stopCallCount, + proxyRefreshRan: proxyResolver.resolveProxyCalls > resolveProxyCallsBefore, + }, { + startCallCount: 1, + stopCallCount: 0, + proxyRefreshRan: true, + }); + } finally { + if (previousSpnEnv === undefined) { + delete process.env['COPILOT_PROXY_KERBEROS_SPN']; + } else { + process.env['COPILOT_PROXY_KERBEROS_SPN'] = previousSpnEnv; + } + await disposeAgent(agent); + } + }); + + test('restarts the Copilot runtime without a Kerberos proxy SPN when a configured SPN is cleared', async () => { + const client = new TestCopilotClient([]); + const proxyResolver = new TestProxyResolver(); + const initialSpn = 'HTTP/initial.proxy'; + // Clearing a previously-set SPN also mirrors as an empty string, but here + // it is a real change: the client baked in the old SPN and must restart + // so the replacement runs without one. + const previousSpnEnv = process.env['COPILOT_PROXY_KERBEROS_SPN']; + delete process.env['COPILOT_PROXY_KERBEROS_SPN']; + const { agent, configurationService } = createTestAgentContext(disposables, { + copilotClient: client, + proxyResolver, + rootConfig: { [AgentHostProxyConfigKey.ProxyKerberosServicePrincipal]: initialSpn }, + }); + try { + await agent.listChatsToMigrate(); + configurationService.updateRootConfig({ [AgentHostProxyConfigKey.ProxyKerberosServicePrincipal]: '' }); + proxyResolver.fireConfigurationChange(); + for (let i = 0; i < 20 && client.stopCallCount < 1; i++) { + await timeout(0); + } + await agent.listChatsToMigrate(); + + assert.deepStrictEqual({ + startCallCount: client.startCallCount, + stopCallCount: client.stopCallCount, + kerberosSpn: getCreatedClientOptions(agent).at(-1)?.env?.['COPILOT_PROXY_KERBEROS_SPN'], + }, { + startCallCount: 2, + stopCallCount: 1, + kerberosSpn: undefined, + }); + } finally { + if (previousSpnEnv === undefined) { + delete process.env['COPILOT_PROXY_KERBEROS_SPN']; + } else { + process.env['COPILOT_PROXY_KERBEROS_SPN'] = previousSpnEnv; + } + await disposeAgent(agent); + } + }); + test('resolves the proxy on first client start without a bridge', async () => { const client = new TestCopilotClient([]); const proxyResolver = new TestProxyResolver();