From 45e56a2531a9aa9ba9ee89d06719d0c3c67b94c6 Mon Sep 17 00:00:00 2001 From: "vs-code-engineering[bot]" Date: Tue, 4 Aug 2026 00:12:38 +0000 Subject: [PATCH] [cherry-pick] Revert https://github.com/microsoft/vscode/pull/327408 --- .../platform/agentHost/common/agentService.ts | 13 - .../platform/agentHost/node/agentHostMain.ts | 4 +- .../agentHost/node/agentHostServerMain.ts | 4 +- .../platform/agentHost/node/agentService.ts | 5 +- .../agentHost/node/claude/claudeAgent.ts | 58 +--- .../agentHost/node/codex/codexAgent.ts | 26 +- .../agentHost/node/copilot/copilotAgent.ts | 183 ++---------- .../agentHost/test/node/claudeAgent.test.ts | 55 ---- .../node/codex/codexPrewarmEviction.test.ts | 6 +- .../agentHost/test/node/copilotAgent.test.ts | 261 +----------------- 10 files changed, 55 insertions(+), 560 deletions(-) diff --git a/src/vs/platform/agentHost/common/agentService.ts b/src/vs/platform/agentHost/common/agentService.ts index 33dc9de05a9a8e..0abe188886b0a0 100644 --- a/src/vs/platform/agentHost/common/agentService.ts +++ b/src/vs/platform/agentHost/common/agentService.ts @@ -1711,19 +1711,6 @@ export interface IAgent { /** Available models from this provider. */ readonly models: IObservable; - /** - * Re-enumerate this provider's model list and publish the result to - * {@link models}. Called both on provider-owned triggers (authentication, - * transport changes) and periodically by the host's model-refresh - * scheduler, so implementations MUST coalesce concurrent calls into a - * single backend request and MUST NOT reject: a failed refresh is logged - * and leaves the last known-good list in place. - * - * Optional so providers without a dynamic model catalog (mocks, test - * agents) need not implement it. - */ - refreshModels?(): Promise; - /** List persisted sessions from this provider. */ listSessions(): Promise; diff --git a/src/vs/platform/agentHost/node/agentHostMain.ts b/src/vs/platform/agentHost/node/agentHostMain.ts index 09bab2109f3367..d7ac1750928028 100644 --- a/src/vs/platform/agentHost/node/agentHostMain.ts +++ b/src/vs/platform/agentHost/node/agentHostMain.ts @@ -17,7 +17,6 @@ import * as os from 'os'; import * as inspector from 'inspector'; import { AgentHostByokModelsEnabledEnvVar, AgentHostClaudeAgentEnabledEnvVar, AgentHostCodexAgentEnabledEnvVar, AgentHostIpcChannels, IAgentHostInspectInfo, IAgentHostSocketInfo, IAgentService, IConnectionTrackerService, isAgentEnabled } from '../common/agentService.js'; import { AgentHostCodexEnabledConfigKey, platformRootSchema } from '../common/agentHostSchema.js'; -import { AgentModelRefreshScheduler, MODEL_REFRESH_INTERVAL_MS } from './agentModelRefreshScheduler.js'; import { AgentService } from './agentService.js'; import { IAgentHostStateManager } from './agentHostStateManager.js'; import { IAgentConfigurationService } from './agentConfigurationService.js'; @@ -281,6 +280,7 @@ async function startAgentHost(): Promise { throw err; } +<<<<<<< HEAD // Keep every provider's model catalog fresh. Provider-owned refresh // triggers (authentication, transport flips, client restarts) are all // edge-based, so this periodic tick is the only thing that notices a model @@ -290,6 +290,8 @@ async function startAgentHost(): Promise { // can ever drain. disposables.add(instantiationService.createInstance(AgentModelRefreshScheduler, agentService.agents, agentService.onDidStartTurn, MODEL_REFRESH_INTERVAL_MS)); +======= +>>>>>>> e5b4addd5c7 (Revert https://github.com/microsoft/vscode/pull/327408 (#328871)) // Surface agent-SDK download progress to clients as generic `progress` // notifications. The downloader fires process-global frames keyed by package // id; the agent service fans each out to the `createSession` progress tokens diff --git a/src/vs/platform/agentHost/node/agentHostServerMain.ts b/src/vs/platform/agentHost/node/agentHostServerMain.ts index cd0e3456963b13..54862e7ffa3635 100644 --- a/src/vs/platform/agentHost/node/agentHostServerMain.ts +++ b/src/vs/platform/agentHost/node/agentHostServerMain.ts @@ -50,7 +50,6 @@ import { CodexProxyService, ICodexProxyService } from './codex/codexProxyService import { AgentSdkDownloader, IAgentSdkDownloader, type IAgentSdkDownloadProgress } from './agentSdkDownloader.js'; import { IAgentHostOTelService } from '../common/otel/agentHostOTelService.js'; import { AgentHostOTelService } from './otel/agentHostOTelService.js'; -import { AgentModelRefreshScheduler, MODEL_REFRESH_INTERVAL_MS } from './agentModelRefreshScheduler.js'; import { AgentService } from './agentService.js'; import { IAgentHostStateManager } from './agentHostStateManager.js'; import { AgentHostClaudeAgentEnabledEnvVar, AgentHostClaudeSdkRootEnvVar, AgentHostCodexAgentEnabledEnvVar, IAgentService, AgentHostCodexAgentSdkRootEnvVar, isAgentEnabled } from '../common/agentService.js'; @@ -369,6 +368,7 @@ async function main(): Promise { }); } +<<<<<<< HEAD // Keep every provider's model catalog fresh. Provider-owned refresh // triggers (authentication, transport flips, client restarts) are all // edge-based, so this periodic tick is the only thing that notices a model @@ -378,6 +378,8 @@ async function main(): Promise { // can ever drain. disposables.add(instantiationService.createInstance(AgentModelRefreshScheduler, agentService.agents, agentService.onDidStartTurn, MODEL_REFRESH_INTERVAL_MS)); +======= +>>>>>>> e5b4addd5c7 (Revert https://github.com/microsoft/vscode/pull/327408 (#328871)) // WebSocket server const wsServer = disposables.add(await WebSocketProtocolServer.create({ port: options.port, diff --git a/src/vs/platform/agentHost/node/agentService.ts b/src/vs/platform/agentHost/node/agentService.ts index ad665c9e6457b1..8710a78c70b65b 100644 --- a/src/vs/platform/agentHost/node/agentService.ts +++ b/src/vs/platform/agentHost/node/agentService.ts @@ -12,7 +12,7 @@ import { Disposable, DisposableMap, DisposableResourceMap, DisposableStore, IDis import { LRUCache, ResourceMap } from '../../../base/common/map.js'; import { getExtensionForMimeType, getMediaMime } from '../../../base/common/mime.js'; import { Schemas } from '../../../base/common/network.js'; -import { IObservable, observableValue } from '../../../base/common/observable.js'; +import { observableValue } from '../../../base/common/observable.js'; import { dirname as resourcesDirname, extname as resourcesExtname, extUriBiasedIgnorePathCase, isEqual, isEqualOrParent, joinPath } from '../../../base/common/resources.js'; import { URI } from '../../../base/common/uri.js'; import { generateUuid } from '../../../base/common/uuid.js'; @@ -548,6 +548,7 @@ export class AgentService extends Disposable implements IAgentService { this._serverToolHost = new AgentServerToolHost(this._stateManager, buildServerToolGroups(this._createSessionServerToolAccessor())); } +<<<<<<< HEAD /** * The registered providers. Exposed so process-lifetime background jobs * (notably {@link AgentModelRefreshScheduler}) can observe registrations @@ -566,6 +567,8 @@ export class AgentService extends Disposable implements IAgentService { return this._sideEffects.onDidStartTurn; } +======= +>>>>>>> e5b4addd5c7 (Revert https://github.com/microsoft/vscode/pull/327408 (#328871)) // ---- provider registration ---------------------------------------------- /** diff --git a/src/vs/platform/agentHost/node/claude/claudeAgent.ts b/src/vs/platform/agentHost/node/claude/claudeAgent.ts index 44418a363f171e..211dbc17b5131b 100644 --- a/src/vs/platform/agentHost/node/claude/claudeAgent.ts +++ b/src/vs/platform/agentHost/node/claude/claudeAgent.ts @@ -228,13 +228,6 @@ export class ClaudeAgent extends Disposable implements IAgent { private readonly _models = observableValue(this, []); readonly models: IObservable = this._models; - /** - * In-flight {@link refreshModels} call, so overlapping triggers (an auth - * token change, a transport flip, or a periodic tick from the host's - * model-refresh scheduler) collapse into a single enumeration instead of - * racing each other's writes to {@link _models}. - */ - private _modelRefreshInFlight: Promise | undefined; private _githubToken: string | undefined; private _proxyHandle: IClaudeProxyHandle | undefined; @@ -484,11 +477,7 @@ export class ClaudeAgent extends Disposable implements IAgent { const next = this._resolveTransportMode(); if (next !== this._transportMode) { this._transportMode = next; - // Proxy and native enumerate different catalogs. Do not retain - // models from the previous transport if the replacement cannot - // enumerate its own list. - this._models.set([], undefined); - void this._startModelRefresh(); + void this._refreshModels(); // Flipping into proxy makes GitHub Copilot auth newly required. // If no proxy handle was ever established, proactively ask the // client to authenticate rather than waiting for the next command @@ -512,7 +501,7 @@ export class ClaudeAgent extends Disposable implements IAgent { // kick off the initial enumeration ourselves. (Transport *flips* // after construction are covered by the `onDidRootConfigChange` // subscription above.) `queueMicrotask` runs it off the ctor stack. - queueMicrotask(() => { void this._startModelRefresh(); }); + queueMicrotask(() => { void this._refreshModels(); }); } } @@ -610,13 +599,7 @@ export class ClaudeAgent extends Disposable implements IAgent { this._githubToken = token; this._logService.info('[Claude] Auth token updated'); oldHandle?.dispose(); - if (tokenChanged) { - // A different account can have different model entitlements. Do - // not retain the previous token's catalog if enumeration for the - // replacement token fails. - this._models.set([], undefined); - } - void this._startModelRefresh(); + void this._refreshModels(); return true; } @@ -629,34 +612,6 @@ export class ClaudeAgent extends Disposable implements IAgent { return this._transportMode === 'proxy'; } - /** - * {@link IAgent.refreshModels}. Coalesces onto an in-flight refresh and - * never rejects — {@link _refreshModels} already logs and handles failure. - * - * Only safe for callers with no new input to apply (the host's periodic - * scheduler). Triggers that invalidate the in-flight request — a rotated - * token, a transport flip — must call {@link _startModelRefresh} so they - * are not answered by a refresh bound to the superseded input. - */ - refreshModels(): Promise { - return this._modelRefreshInFlight ?? this._startModelRefresh(); - } - - /** - * Unconditionally begins a refresh, superseding any in-flight one as the - * coalescing target. The superseded request stays harmless: its own - * stale-write guard drops the result if the token or transport moved on. - */ - private _startModelRefresh(): Promise { - const refresh = this._refreshModels().finally(() => { - if (this._modelRefreshInFlight === refresh) { - this._modelRefreshInFlight = undefined; - } - }); - this._modelRefreshInFlight = refresh; - return refresh; - } - private async _refreshModels(): Promise { const proxyAtStart = this._isProxyEnabled(); const tokenAtStart = this._githubToken; @@ -678,10 +633,9 @@ export class ClaudeAgent extends Disposable implements IAgent { this._models.set(filtered, undefined); } catch (err) { this._logService.error(err, '[Claude] Failed to refresh models'); - // Keep the last known-good catalog. A periodic refresh is advisory; - // a transient service failure must not make every model disappear. - // Input changes that invalidate the catalog clear it at the point - // where that input changes. + if (this._isProxyEnabled() === proxyAtStart && (!proxyAtStart || this._githubToken === tokenAtStart)) { + this._models.set([], undefined); + } } } diff --git a/src/vs/platform/agentHost/node/codex/codexAgent.ts b/src/vs/platform/agentHost/node/codex/codexAgent.ts index 5dd64bdfacd838..47ff9615dd68e0 100644 --- a/src/vs/platform/agentHost/node/codex/codexAgent.ts +++ b/src/vs/platform/agentHost/node/codex/codexAgent.ts @@ -1080,24 +1080,14 @@ export class CodexAgent extends Disposable implements IAgent { } } - /** - * {@link IAgent.refreshModels}. Coalesces onto an in-flight refresh — from - * an account/usage-source change or an earlier tick — rather than issuing a - * second enumeration, and never rejects: {@link _refreshModels} logs and - * applies its own stale-write guards on failure. - */ - refreshModels(): Promise { - return this._modelsRefreshPromise ?? this._queueModelRefresh(); - } - - private _queueModelRefresh(): Promise { + private _queueModelRefresh(): void { const refreshPromise = this._refreshModels().finally(() => { if (this._modelsRefreshPromise === refreshPromise) { this._modelsRefreshPromise = undefined; } }); this._modelsRefreshPromise = refreshPromise; - return refreshPromise; + void this._modelsRefreshPromise; } private _ensureAuthenticated(): string | undefined { @@ -1338,9 +1328,9 @@ export class CodexAgent extends Disposable implements IAgent { this._models.set(models, undefined); } catch (err) { this._logService.warn(`[Codex] Failed to refresh models: ${err instanceof Error ? err.message : String(err)}`); - // Keep the last known-good catalog. Usage-source changes clear the - // list in `_applyUsageSourceChange`; a transient periodic failure - // must not make every model disappear. + if (this._usageSource === usageSource && this._githubToken === token) { + this._models.set([], undefined); + } } } @@ -1372,9 +1362,9 @@ export class CodexAgent extends Disposable implements IAgent { } } catch (err) { this._logService.warn(`[Codex] Failed to refresh OpenAI models: ${err instanceof Error ? err.message : String(err)}`); - // Keep the last known-good catalog. Usage-source changes clear the - // list in `_applyUsageSourceChange`; a transient periodic failure - // must not make every model disappear. + if (this._usageSource === 'openai') { + this._models.set([], undefined); + } } } diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts index f30f1efdfa904e..c95b9ee8227f28 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts @@ -7,7 +7,7 @@ import { CopilotClient, RuntimeConnection, type CopilotClientOptions, type GitHu import * as fs from 'fs/promises'; import * as os from 'os'; import { pathToFileURL } from 'url'; -import { CancelablePromise, createCancelablePromise, DeferredPromise, Delayer, disposableTimeout, Limiter, SequencerByKey } from '../../../../base/common/async.js'; +import { CancelablePromise, createCancelablePromise, Delayer, disposableTimeout, Limiter, SequencerByKey } from '../../../../base/common/async.js'; import { type CancellationToken } from '../../../../base/common/cancellation.js'; import { CancellationError } from '../../../../base/common/errors.js'; import { Emitter, Event } from '../../../../base/common/event.js'; @@ -336,42 +336,18 @@ export class CopilotAgent extends Disposable implements IAgent { * Bounded exponential-backoff retry for {@link _refreshModels}. The SDK's * `models.list` RPC can fail transiently (e.g. a `429 "too many requests"` * right after startup). Without a retry the model picker would stay empty - * until the next external refresh trigger (a GitHub token change, a CLI - * client restart, or the host's periodic scheduler), so we retry a few - * times before giving up. Overridable in tests to avoid real delays. + * until the GitHub token next changes — the only other trigger for a + * refresh — so we retry a few times before giving up. Overridable in tests + * to avoid real delays. */ protected readonly _modelRefreshMaxAttempts: number = 5; protected readonly _modelRefreshBaseDelayMs: number = 1_000; protected readonly _modelRefreshMaxDelayMs: number = 30_000; /** Pending model-refresh retry timer; cleared on a fresh refresh, shutdown, or dispose. */ private readonly _modelRefreshRetry = this._register(new MutableDisposable()); - /** - * Invalidates model requests bound to a superseded token/client/catalog - * source. Token identity alone is insufficient: restarting the client for - * a `COPILOT_GH_HOST` change keeps the same token while changing the CAPI - * endpoint whose catalog is authoritative. - */ - private _modelCatalogGeneration = 0; - /** - * Forced refreshes are deferred to the next task so related lifecycle - * changes (for example an auth update arriving with a startup-config - * change) collapse into one enumeration of the final token/client source. - */ - private _scheduledModelRefresh: { readonly deferred: DeferredPromise; generation: number } | undefined; - private readonly _modelRefreshSchedule = this._register(new MutableDisposable()); - /** - * In-flight {@link refreshModels} call, so overlapping triggers (an auth - * token change landing on top of a periodic tick) collapse into a single - * `models.list` request. Only covers the request itself: {@link _refreshModels} - * returns as soon as it *schedules* a backoff retry, so a pending retry - * never suppresses a later tick — which is what lets the scheduler act as - * the long-term retry path once the bounded attempts are exhausted. - */ - private _modelRefreshInFlight: Promise | undefined; private _client: CopilotClient | undefined; private _clientStarting: Promise | undefined; - private _clientStopping: Promise | undefined; /** * Proxy URL injected into the running client's subprocess env (`undefined` * when none was injected). Used to detect when a token change alters the @@ -654,15 +630,6 @@ export class CopilotAgent extends Disposable implements IAgent { this._sessions.clearAndDisposeAll(); this._mcpNotificationSubs.clearAndDisposeAll(); await this._stopClient(); - // The model list came from the subprocess we just tore down, and the - // replacement may be pointed at a different CAPI endpoint entirely - // (`COPILOT_GH_HOST` routes through this same helper). Re-enumerate - // rather than serving the old client's catalog until the next token - // change. Not hooked in `_ensureClient`, since `_listModels` calls - // it and would recurse. - this._capiModels = []; - this._publishModels(); - void this._scheduleModelRefresh(); } /** @@ -871,7 +838,7 @@ export class CopilotAgent extends Disposable implements IAgent { this._logService.info(`[Copilot] Auth token ${tokenChanged ? 'updated' : 'unchanged'}`); if (tokenChanged) { await this._restartClientIfProxyChanged(); - void this._scheduleModelRefresh(); + void this._refreshModels(); } return true; } @@ -974,71 +941,7 @@ export class CopilotAgent extends Disposable implements IAgent { : AgentHostClientType.Unknown; } - /** - * {@link IAgent.refreshModels}. Coalesces onto an in-flight refresh and - * never rejects — {@link _refreshModels} already logs and retains the last - * known-good list on failure. - * - * Only safe for callers with no new input to apply (the host's periodic - * scheduler). Triggers that invalidate the in-flight request — a rotated - * token, a restarted client — must call {@link _scheduleModelRefresh} so they - * are not answered by a refresh bound to the superseded input. - */ - refreshModels(): Promise { - return this._scheduledModelRefresh?.deferred.p ?? this._modelRefreshInFlight ?? this._startModelRefresh(++this._modelCatalogGeneration); - } - - /** - * Invalidates an in-flight refresh immediately, then starts one refresh on - * the next task. Repeated lifecycle triggers before that task - * share the same deferred and enumerate only the final token/client source. - */ - private _scheduleModelRefresh(): Promise { - const generation = ++this._modelCatalogGeneration; - if (this._scheduledModelRefresh) { - this._scheduledModelRefresh.generation = generation; - return this._scheduledModelRefresh.deferred.p; - } - - const scheduled = { deferred: new DeferredPromise(), generation }; - this._scheduledModelRefresh = scheduled; - this._modelRefreshSchedule.value = disposableTimeout(() => { - void (async () => { - try { - // A config-triggered restart clears `_client` before its - // asynchronous `stop()` completes. Wait for that stop so this - // refresh cannot resurrect the client midway through teardown. - await this._clientStopping; - if (this._scheduledModelRefresh !== scheduled) { - return; - } - this._scheduledModelRefresh = undefined; - this._modelRefreshSchedule.clear(); - await this._startModelRefresh(scheduled.generation); - } catch (err) { - this._logService.error(err, '[Copilot] Failed to schedule model refresh'); - } finally { - if (this._scheduledModelRefresh === scheduled) { - this._scheduledModelRefresh = undefined; - this._modelRefreshSchedule.clear(); - } - scheduled.deferred.complete(); - } - })(); - }, 0); - return scheduled.deferred.p; - } - - private _startModelRefresh(generation: number): Promise { - const refresh = this._refreshModels(0, generation).finally(() => { - if (this._modelRefreshInFlight === refresh) { - this._modelRefreshInFlight = undefined; - } - }); - this._modelRefreshInFlight = refresh; - return refresh; - } - private async _refreshModels(attempt = 0, generation = this._modelCatalogGeneration): Promise { + private async _refreshModels(attempt = 0): Promise { // A fresh refresh (e.g. a token change) supersedes any scheduled retry. this._modelRefreshRetry.clear(); @@ -1057,7 +960,7 @@ export class CopilotAgent extends Disposable implements IAgent { } try { const models = await this._listModels(tokenAtRefreshStart); - if (this._githubToken === tokenAtRefreshStart && this._modelCatalogGeneration === generation) { + if (this._githubToken === tokenAtRefreshStart) { this._capiModels = models; this._publishModels(); } @@ -1065,14 +968,14 @@ export class CopilotAgent extends Disposable implements IAgent { // Token rotated mid-flight — a newer refresh owns the result — or // teardown began while the request was in flight, in which case a // retry would just resurrect the client we are tearing down. - if (this._githubToken !== tokenAtRefreshStart || this._modelCatalogGeneration !== generation || this._shutdownPromise) { + if (this._githubToken !== tokenAtRefreshStart || this._shutdownPromise) { return; } if (attempt + 1 < this._modelRefreshMaxAttempts) { const delay = this._modelRefreshBackoff(attempt); this._logService.warn(`[Copilot] Failed to refresh models (attempt ${attempt + 1}), retrying in ${delay}ms`, err); this._modelRefreshRetry.value = disposableTimeout(() => { - void this._refreshModels(attempt + 1, generation); + void this._refreshModels(attempt + 1); }, delay); return; } @@ -1138,40 +1041,18 @@ export class CopilotAgent extends Disposable implements IAgent { return Math.round(exp / 2 + Math.random() * (exp / 2)); } - private _stopClient(): Promise { + private async _stopClient(): Promise { // Any parked restart is satisfied by this stop: the next `_ensureClient` - // starts from the current config, so nothing is left to re-apply. Cleared - // synchronously so a concurrent `_applyPendingClientRestart` bails rather - // than stopping a client this call is already tearing down. + // starts from the current config, so nothing is left to re-apply. this._pendingClientRestartReasons.clear(); - if (this._clientStopping) { - return this._clientStopping; - } - const stopping = (async () => { - const clientStarting = this._clientStarting; - if (clientStarting) { - try { - await clientStarting; - } catch { - // A failed/stale start owns its own cleanup. Continue so - // any client it managed to publish is still stopped below. - } - } - const client = this._client; - this._client = undefined; - this._clientStarting = undefined; - await client?.stop(); - // The runtime subprocess is now dead, so it is safe to release the BYOK - // proxy handle: the next session launch mints a fresh nonce. See the - // ownership invariant on `CopilotSessionLauncher.disposeByokProxyHandle`. - await this._sessionLauncher.disposeByokProxyHandle(); - })().finally(() => { - if (this._clientStopping === stopping) { - this._clientStopping = undefined; - } - }); - this._clientStopping = stopping; - return stopping; + const client = this._client; + this._client = undefined; + this._clientStarting = undefined; + await client?.stop(); + // The runtime subprocess is now dead, so it is safe to release the BYOK + // proxy handle: the next session launch mints a fresh nonce. See the + // ownership invariant on `CopilotSessionLauncher.disposeByokProxyHandle`. + await this._sessionLauncher.disposeByokProxyHandle(); } /** @@ -1215,15 +1096,6 @@ export class CopilotAgent extends Disposable implements IAgent { // ---- client lifecycle --------------------------------------------------- private async _ensureClient(): Promise { - if (this._shutdownPromise) { - throw new CancellationError(); - } - while (this._clientStopping) { - await this._clientStopping; - if (this._shutdownPromise) { - throw new CancellationError(); - } - } if (this._client) { return this._client; } @@ -1369,10 +1241,6 @@ export class CopilotAgent extends Disposable implements IAgent { }; const client = this._createCopilotClient(clientOptions); await client.start(); - if (this._shutdownPromise) { - await client.stop(); - throw new CancellationError(); - } if (this._isSessionSyncEnabled() !== sessionSyncAtStartup || this._isRubberDuckEnabled() !== rubberDuckAtStartup || this._getCopilotSdkLogLevelSetting() !== copilotSdkLogLevelSettingAtStartup || this._getEnterpriseHost() !== enterpriseHostAtStartup || this._isSystemProxyEnabled() !== systemProxyEnabledAtStartup) { await client.stop(); throw new Error('Copilot startup config changed while the client was starting'); @@ -3039,13 +2907,6 @@ export class CopilotAgent extends Disposable implements IAgent { async shutdown(): Promise { this._shutdownPromise ??= (async () => { - // Invalidate any request that started before teardown. Token - // identity alone does not change during shutdown, so without this - // guard a late success could republish after the host stopped. - this._modelCatalogGeneration++; - this._modelRefreshSchedule.clear(); - this._scheduledModelRefresh?.deferred.complete(); - this._scheduledModelRefresh = undefined; // Cancel any pending model-refresh retry so its timer cannot fire // after teardown and resurrect the client. this._modelRefreshRetry.clear(); @@ -3054,7 +2915,11 @@ export class CopilotAgent extends Disposable implements IAgent { for (const sessionId of sessionIds) { await this._sessionSequencer.queue(sessionId, () => this._destroyAndDisposeSession(sessionId)); } - await this._stopClient(); + await this._client?.stop(); + this._client = undefined; + // Release the BYOK proxy handle only after the runtime subprocess is + // gone, mirroring `_stopClient` and the proxy ownership invariant. + await this._sessionLauncher.disposeByokProxyHandle(); })(); return this._shutdownPromise; } diff --git a/src/vs/platform/agentHost/test/node/claudeAgent.test.ts b/src/vs/platform/agentHost/test/node/claudeAgent.test.ts index 8adafa709f7b32..778dd0891964ee 100644 --- a/src/vs/platform/agentHost/test/node/claudeAgent.test.ts +++ b/src/vs/platform/agentHost/test/node/claudeAgent.test.ts @@ -1025,61 +1025,6 @@ suite('ClaudeAgent', () => { ); }); - test('coalesces concurrent refreshModels calls onto one CAPI models request', async () => { - const { agent, api } = createTestContext(disposables); - // Block the first request in flight so the second caller has something - // to coalesce onto: a periodic scheduler tick landing on top of an - // auth-triggered refresh must not double-hit the service. - const gate = new DeferredPromise(); - let modelsCalls = 0; - api.models = async () => { modelsCalls++; await gate.p; return [...ALL_MODELS]; }; - await agent.authenticate('https://api.github.com', 'tok'); - await tick(); - - const first = agent.refreshModels(); - const second = agent.refreshModels(); - gate.complete(); - await Promise.all([first, second]); - - assert.deepStrictEqual({ - modelsCalls, - hasModels: agent.models.get().length > 0, - }, { - modelsCalls: 1, - hasModels: true, - }); - }); - - test('keeps the last known-good models when a periodic refresh fails', async () => { - const { agent, api } = createTestContext(disposables); - api.models = async () => [...ALL_MODELS]; - await agent.authenticate('https://api.github.com', 'tok'); - await agent.refreshModels(); - const modelIds = agent.models.get().map(model => model.id); - - api.models = async () => { throw new Error('transient failure'); }; - await agent.refreshModels(); - - assert.deepStrictEqual(agent.models.get().map(model => model.id), modelIds); - }); - - test('clears models when enumeration for a replacement token fails', async () => { - const { agent, api } = createTestContext(disposables); - api.models = async token => { - if (token === 'tokB') { - throw new Error('token B failure'); - } - return [...ALL_MODELS]; - }; - await agent.authenticate('https://api.github.com', 'tokA'); - await agent.refreshModels(); - - await agent.authenticate('https://api.github.com', 'tokB'); - await agent.refreshModels(); - - assert.deepStrictEqual(agent.models.get(), []); - }); - test('native transport: models populate from supportedModels() with no proxy start and no CAPI models() call', async () => { const { agent, proxy, api, sdk } = createTestContext(disposables, { rootConfig: { claudeUseCopilotProxy: false } }); let capiModelsCalls = 0; diff --git a/src/vs/platform/agentHost/test/node/codex/codexPrewarmEviction.test.ts b/src/vs/platform/agentHost/test/node/codex/codexPrewarmEviction.test.ts index 9f051d8a0f5137..99f4d91e2d89a3 100644 --- a/src/vs/platform/agentHost/test/node/codex/codexPrewarmEviction.test.ts +++ b/src/vs/platform/agentHost/test/node/codex/codexPrewarmEviction.test.ts @@ -8,7 +8,11 @@ import assert from 'assert'; import { PassThrough } from 'stream'; import { Emitter } from '../../../../../base/common/event.js'; import type { DisposableStore } from '../../../../../base/common/lifecycle.js'; +<<<<<<< HEAD import { Schemas } from '../../../../../base/common/network.js'; +======= +import { waitForState } from '../../../../../base/common/observable.js'; +>>>>>>> e5b4addd5c7 (Revert https://github.com/microsoft/vscode/pull/327408 (#328871)) import { URI } from '../../../../../base/common/uri.js'; import { sep } from '../../../../../base/common/path.js'; import { isWindows } from '../../../../../base/common/platform.js'; @@ -188,7 +192,7 @@ async function createAgent(disposables: Pick, options: I instantiationService.stub(ILogService, logService); const agent = disposables.add(instantiationService.createInstance(CodexAgent)); await agent.authenticate(agent.getProtectedResources()[0].resource, 'test-token'); - await agent.refreshModels(); + await waitForState(agent.models, models => models.length > 0); return agent; } diff --git a/src/vs/platform/agentHost/test/node/copilotAgent.test.ts b/src/vs/platform/agentHost/test/node/copilotAgent.test.ts index a48660e288140b..930c794d9497ca 100644 --- a/src/vs/platform/agentHost/test/node/copilotAgent.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotAgent.test.ts @@ -11,7 +11,7 @@ import * as fs from 'fs/promises'; import * as os from 'os'; import { VSBuffer } from '../../../../base/common/buffer.js'; import { DeferredPromise, timeout } from '../../../../base/common/async.js'; -import { CancellationError, isCancellationError } from '../../../../base/common/errors.js'; +import { isCancellationError } from '../../../../base/common/errors.js'; import { Disposable, type DisposableStore, type IDisposable, type IReference } from '../../../../base/common/lifecycle.js'; import { Emitter, Event } from '../../../../base/common/event.js'; import { Schemas } from '../../../../base/common/network.js'; @@ -348,28 +348,19 @@ class TestCopilotClient implements ITestCopilotClient { models: { list: async params => { this.modelListRequests.push(params); - const gate = this.modelListGates.shift() ?? this.modelListGate; - const models = this.modelListResponses.shift() ?? this._models; const error = this.modelListErrors.shift(); - await gate; if (error) { throw error; } - return { models: models.map(toSdkModelInfo) }; + return { models: this._models.map(toSdkModelInfo) }; } }, }; startCallCount = 0; stopCallCount = 0; - startGate: Promise | undefined; listSessionCallCount = 0; readonly modelListRequests: Parameters[0][] = []; readonly modelListErrors: Error[] = []; - /** When set, `models.list` records its request then blocks on this until resolved. */ - modelListGate: Promise | undefined; - /** Per-request gates and results, captured when each request starts. */ - readonly modelListGates: Promise[] = []; - readonly modelListResponses: ITestCopilotModelInfo[][] = []; readonly getSessionMetadataCalls: string[] = []; readonly deletedSessionIds: string[] = []; @@ -380,7 +371,6 @@ class TestCopilotClient implements ITestCopilotClient { async start(): Promise { this.startCallCount++; - await this.startGate; } async stop(): ReturnType { this.stopCallCount++; @@ -1332,37 +1322,6 @@ suite('CopilotAgent', () => { } }); - test('coalesces concurrent refreshModels calls onto one models.list request', async () => { - const client = new TestCopilotClient([], [{ - id: 'gpt-4o', - name: 'GPT-4o', - }]); - const agent = createTestAgent(disposables, { copilotClient: client }); - try { - // Block the first request in flight so the second caller has - // something to coalesce onto: an auth-triggered refresh landing on - // top of a periodic scheduler tick must not double-hit the service. - const gate = new DeferredPromise(); - client.modelListGate = gate.p; - await agent.authenticate('https://api.github.com', 'token'); - - const first = agent.refreshModels(); - const second = agent.refreshModels(); - gate.complete(); - await Promise.all([first, second]); - - assert.deepStrictEqual({ - requests: client.modelListRequests, - modelNames: agent.models.get().map(m => m.name), - }, { - requests: [{ gitHubToken: 'token' }], - modelNames: ['GPT-4o'], - }); - } finally { - await disposeAgent(agent); - } - }); - test('does not refresh models or restart the client after shutdown', async () => { const client = new TestCopilotClient([], [{ id: 'gpt-4o', @@ -1394,63 +1353,6 @@ suite('CopilotAgent', () => { } }); - test('does not publish an in-flight model refresh after shutdown', async () => { - const client = new TestCopilotClient([], [{ - id: 'initial', - name: 'Initial', - }]); - const agent = createTestAgent(disposables, { copilotClient: client }); - try { - await agent.authenticate('https://api.github.com', 'token'); - await waitForState(agent.models, models => models.some(model => model.id === 'initial')); - await Promise.resolve(); - - const gate = new DeferredPromise(); - client.modelListGates.push(gate.p); - client.modelListResponses.push([{ id: 'late', name: 'Late' }]); - const requestsBefore = client.modelListRequests.length; - const refresh = agent.refreshModels(); - for (let i = 0; i < 500 && client.modelListRequests.length <= requestsBefore; i++) { - await timeout(1); - } - assert.strictEqual(client.modelListRequests.length, requestsBefore + 1, 'expected the gated model request to start'); - - await agent.shutdown(); - gate.complete(); - await refresh; - - assert.deepStrictEqual(agent.models.get().map(model => model.id), ['initial']); - } finally { - await disposeAgent(agent); - } - }); - - test('stops a client that finishes starting after shutdown begins', async () => { - const client = new TestCopilotClient([]); - const startGate = new DeferredPromise(); - client.startGate = startGate.p; - const agent = createTestAgent(disposables, { copilotClient: client }); - try { - const listPromise = agent.listSessions(); - await Promise.resolve(); - const shutdownPromise = agent.shutdown(); - startGate.complete(); - - await assert.rejects(listPromise, CancellationError); - await shutdownPromise; - - assert.deepStrictEqual({ - starts: client.startCallCount, - stops: client.stopCallCount, - }, { - starts: 1, - stops: 1, - }); - } finally { - await disposeAgent(agent); - } - }); - test('createSession infers workspace-less from an omitted workingDirectory and uses a stable scratch dir', async () => { const userHome = URI.file(await fs.mkdtemp(`${os.tmpdir()}/qc-home-`)); const agent = createTestAgent(disposables, { userHome }); @@ -1588,14 +1490,8 @@ suite('CopilotAgent', () => { class StopCountingClient extends TestCopilotClient { stopCount = 0; - stopGate: Promise | undefined; - stopError: Error | undefined; override async stop(): ReturnType { this.stopCount++; - await this.stopGate; - if (this.stopError) { - throw this.stopError; - } return super.stop(); } } @@ -1700,159 +1596,6 @@ suite('CopilotAgent', () => { } }); - test('re-enumerates models after a startup-config restart', async () => { - const client = new StopCountingClient([], [{ id: 'gpt-4o', name: 'GPT-4o' }]); - const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); - try { - await agent.authenticate('https://api.github.com', 'token'); - await agent.listSessions(); - await waitForState(agent.models, m => m.length > 0); - const requestsBefore = client.modelListRequests.length; - - // The catalog belonged to the subprocess being torn down, and the - // replacement may point at a different CAPI endpoint entirely. - configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: true }); - for (let i = 0; i < 500 && client.modelListRequests.length <= requestsBefore; i++) { - await timeout(1); - } - - assert.deepStrictEqual({ - stopCount: client.stopCount, - refreshesAfterRestart: client.modelListRequests.length - requestsBefore, - }, { - stopCount: 1, - refreshesAfterRestart: 1, - }); - } finally { - await disposeAgent(agent); - } - }); - - test('coalesces concurrent token and startup-config refresh triggers', async () => { - const client = new StopCountingClient([], [{ id: 'gpt-4o', name: 'GPT-4o' }]); - const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); - const stopGate = new DeferredPromise(); - try { - await agent.authenticate('https://api.github.com', 'token-a'); - await agent.listSessions(); - await waitForState(agent.models, models => models.length > 0); - await Promise.resolve(); - const requestsBefore = client.modelListRequests.length; - client.stopGate = stopGate.p; - - configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: true }); - await agent.authenticate('https://api.github.com', 'token-b'); - await timeout(10); - assert.strictEqual(client.modelListRequests.length, requestsBefore, 'model refresh must wait for the old client to stop'); - stopGate.complete(); - for (let i = 0; i < 500 && client.modelListRequests.length <= requestsBefore; i++) { - await timeout(1); - } - await Promise.resolve(); - - assert.deepStrictEqual({ - stopCount: client.stopCount, - refreshes: client.modelListRequests.length - requestsBefore, - lastToken: client.modelListRequests.at(-1)?.gitHubToken, - }, { - stopCount: 1, - refreshes: 1, - lastToken: 'token-b', - }); - } finally { - stopGate.complete(); - await disposeAgent(agent); - } - }); - - test('does not start a replacement client while the previous client is stopping', async () => { - const client = new StopCountingClient([]); - const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); - const stopGate = new DeferredPromise(); - try { - await agent.authenticate('https://api.github.com', 'token'); - await agent.listSessions(); - client.stopGate = stopGate.p; - - configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: true }); - const listPromise = agent.listSessions(); - await timeout(10); - assert.strictEqual(client.startCallCount, 1, 'replacement client must wait for the old client to stop'); - - stopGate.complete(); - await listPromise; - assert.deepStrictEqual({ - starts: client.startCallCount, - stops: client.stopCount, - }, { - starts: 2, - stops: 1, - }); - } finally { - stopGate.complete(); - await disposeAgent(agent); - } - }); - - test('a failed client stop does not poison later model refreshes', async () => { - const client = new StopCountingClient([], [{ id: 'gpt-4o', name: 'GPT-4o' }]); - const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); - try { - await agent.authenticate('https://api.github.com', 'token'); - await waitForState(agent.models, models => models.length > 0); - await agent.listSessions(); - const requestsBefore = client.modelListRequests.length; - client.stopError = new Error('stop failed'); - - configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: true }); - await timeout(10); - client.stopError = undefined; - await agent.refreshModels(); - - assert.strictEqual(client.modelListRequests.length, requestsBefore + 1); - } finally { - await disposeAgent(agent); - } - }); - - test('drops an in-flight catalog from the previous client generation', async () => { - const client = new StopCountingClient([], [{ id: 'initial', name: 'Initial' }]); - const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client }); - try { - await agent.authenticate('https://api.github.com', 'token'); - await waitForState(agent.models, models => models.some(model => model.id === 'initial')); - await Promise.resolve(); - - const staleGate = new DeferredPromise(); - const replacementGate = new DeferredPromise(); - client.modelListGates.push(staleGate.p, replacementGate.p); - client.modelListResponses.push( - [{ id: 'stale', name: 'Stale' }], - [{ id: 'replacement', name: 'Replacement' }], - ); - const requestsBefore = client.modelListRequests.length; - const staleRefresh = agent.refreshModels(); - for (let i = 0; i < 500 && client.modelListRequests.length < requestsBefore + 1; i++) { - await timeout(1); - } - - configurationService.updateRootConfig({ [CopilotCliConfigKey.RubberDuck]: true }); - for (let i = 0; i < 500 && client.modelListRequests.length < requestsBefore + 2; i++) { - await timeout(1); - } - assert.deepStrictEqual(agent.models.get(), []); - - replacementGate.complete(); - await waitForState(agent.models, models => models.some(model => model.id === 'replacement')); - staleGate.complete(); - await staleRefresh; - - assert.deepStrictEqual(agent.models.get().map(model => model.id), ['replacement']); - } finally { - await disposeAgent(agent); - } - }); - test('restarts the idle client when the rubber duck config changes', async () => { const client = new StopCountingClient([]); const { agent, configurationService } = createTestAgentContext(disposables, { copilotClient: client });