Skip to content

Commit 156357c

Browse files
alexdimaCopilot
andauthored
agentHost: observe capabilities lazily in session adapters (#330853)
Every cached AgentHostSessionAdapter eagerly subscribed to the shared agent-capabilities observable, so a window restoring hundreds of sessions installed hundreds of observers and tripped the listener leak detector. Most of those observers had nothing to do: the autorun only re-applies a chat catalog, and an adapter that never received one has no catalog to reconcile. Install the observer on the first applyChatCatalog call instead, so only adapters with catalog state to reapply subscribe. Late-hydrating capabilities still re-expand a collapsed peer catalog. Found while self-hosting Insiders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3c7151d commit 156357c

3 files changed

Lines changed: 70 additions & 18 deletions

File tree

src/vs/sessions/contrib/providers/agentHost/AGENT_HOST_SESSIONS_PROVIDER.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ A single agent host session uses several distinct identifiers:
114114

115115
## Architecture
116116

117-
- **`AgentHostSessionAdapter`** (`baseAgentHostSessionsProvider.ts`) is the `ISession` implementation. It wraps an `IAgentSessionMetadata` from the backend and exposes the observable session surface (`status`, `title`, `workspace`, `mainChat`, `mode`, …). The base provider keeps a `_sessionCache` of adapters keyed by `rawId`. Adapter capabilities derive from a shared provider-to-capabilities lookup, so one root-state event listener and one catalog scan serve the entire cache; root-state errors and disconnects clear the lookup so stale capabilities are not retained.
117+
- **`AgentHostSessionAdapter`** (`baseAgentHostSessionsProvider.ts`) is the `ISession` implementation. It wraps an `IAgentSessionMetadata` from the backend and exposes the observable session surface (`status`, `title`, `workspace`, `mainChat`, `mode`, …). The base provider keeps a `_sessionCache` of adapters keyed by `rawId`. Adapter capabilities derive from a shared provider-to-capabilities lookup, so one root-state event listener and one catalog scan serve the entire cache; an adapter observes that lookup only after receiving a chat catalog that may need reapplication. Root-state errors and disconnects clear the lookup so stale capabilities are not retained.
118118
- **`NewSession`** is a disposable draft (pre-creation) session. Several can be in flight simultaneously; the management layer tears down superseded drafts via `deleteNewSession`. A draft eagerly creates its backend session once authentication settles, then **graduates** into a committed `AgentHostSessionAdapter` on first send.
119119
- The base provider is abstract; concrete providers supply: `connection`, `authenticationPending`, `resourceSchemeForProvider`, `_formatSessionTypeLabel`, `_adapterOptions` (workspace builder), `resolveWorkspace`, and optionally `_diffUriMapper`.
120120

src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -655,9 +655,10 @@ export class AgentHostSessionAdapter extends Disposable implements ISession {
655655
/**
656656
* The last {@link SessionState} applied to the chat catalog, retained so the
657657
* catalog can be re-reconciled when {@link capabilities} change after the
658-
* fact (see the capability autorun in the constructor).
658+
* fact.
659659
*/
660660
private _lastCatalogState: SessionState | undefined;
661+
private readonly _chatCatalogCapabilitiesObserver = this._register(new MutableDisposable());
661662
private readonly _rawId: string;
662663
private readonly _resourceScheme: string;
663664

@@ -915,19 +916,6 @@ export class AgentHostSessionAdapter extends Disposable implements ISession {
915916
supportsDelete: true,
916917
};
917918
});
918-
919-
// Re-apply the chat catalog when advertised capabilities change (e.g. the
920-
// agent host's root state arrives after the session's first state update).
921-
// Without this, a multi-chat session whose state was processed while
922-
// `supportsMultipleChats` was still `false` would stay collapsed to
923-
// `[defaultChat]` until the next session-state update.
924-
this._register(autorun(reader => {
925-
this.capabilities.read(reader);
926-
const state = this._lastCatalogState;
927-
if (state) {
928-
this._applyChatCatalog(state);
929-
}
930-
}));
931919
}
932920

933921
/**
@@ -948,7 +936,17 @@ export class AgentHostSessionAdapter extends Disposable implements ISession {
948936
*/
949937
applyChatCatalog(state: SessionState): void {
950938
this._lastCatalogState = state;
951-
this._applyChatCatalog(state);
939+
if (this._chatCatalogCapabilitiesObserver.value) {
940+
this._applyChatCatalog(state);
941+
} else {
942+
this._chatCatalogCapabilitiesObserver.value = autorun(reader => {
943+
this.capabilities.read(reader);
944+
const currentState = this._lastCatalogState;
945+
if (currentState) {
946+
this._applyChatCatalog(currentState);
947+
}
948+
});
949+
}
952950
}
953951

954952
private _applyChatCatalog(state: SessionState): void {

src/vs/sessions/contrib/providers/agentHost/test/browser/localAgentHostSessionsProvider.test.ts

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { DeferredPromise, raceTimeout, timeout } from '../../../../../../base/co
99
import { Codicon } from '../../../../../../base/common/codicons.js';
1010
import { Emitter, Event } from '../../../../../../base/common/event.js';
1111
import { DisposableMap, DisposableStore, ImmortalReference, toDisposable, type IReference } from '../../../../../../base/common/lifecycle.js';
12-
import { autorun, constObservable, ISettableObservable, observableValue, type IObservable } from '../../../../../../base/common/observable.js';
12+
import { autorun, constObservable, ISettableObservable, observableFromEvent, observableValue, type IObservable } from '../../../../../../base/common/observable.js';
1313
import { URI } from '../../../../../../base/common/uri.js';
1414
import { isEqual } from '../../../../../../base/common/resources.js';
1515
import { mock } from '../../../../../../base/test/common/mock.js';
@@ -45,7 +45,7 @@ import { IActiveSession } from '../../../../../services/sessions/common/sessions
4545
import { ISessionsService } from '../../../../../services/sessions/browser/sessionsService.js';
4646
import { IAgentCustomizationScope, IAgentHostActiveClientService } from '../../../../../../workbench/contrib/chat/browser/agentSessions/agentHost/agentHostActiveClientService.js';
4747
import { LocalAgentHostSessionsProvider } from '../../browser/localAgentHostSessionsProvider.js';
48-
import { AgentHostSessionAdapter } from '../../browser/baseAgentHostSessionsProvider.js';
48+
import { AgentHostSessionAdapter, type IAgentHostAdapterOptions } from '../../browser/baseAgentHostSessionsProvider.js';
4949
import { IAutomationStorageService } from '../../../../automations/common/automationStorageService.js';
5050
import { TestAutomationStorageService } from '../../../../automations/test/browser/automationTestUtils.js';
5151
import { ILabelService } from '../../../../../../platform/label/common/label.js';
@@ -4216,6 +4216,60 @@ suite('LocalAgentHostSessionsProvider', () => {
42164216
});
42174217
});
42184218

4219+
test('session adapters observe capabilities only after receiving a chat catalog', () => {
4220+
let listenerCount = 0;
4221+
let agentCapabilities = new Map<string, AgentInfo['capabilities']>([['copilotcli', {}]]);
4222+
const capabilitiesChanged = disposables.add(new Emitter<void>({
4223+
onDidAddListener: () => listenerCount++,
4224+
onWillRemoveListener: () => listenerCount--,
4225+
}));
4226+
const capabilitiesObs = observableFromEvent(disposables, capabilitiesChanged.event, () => agentCapabilities);
4227+
const instantiationService = disposables.add(new TestInstantiationService());
4228+
instantiationService.stub(IGitHubService, new class extends mock<IGitHubService>() { });
4229+
instantiationService.stub(ISessionsService, new class extends mock<ISessionsService>() {
4230+
override readonly activeSession = constObservable<IActiveSession | undefined>(undefined);
4231+
});
4232+
instantiationService.stub(IPullRequestIconCache, new class extends mock<IPullRequestIconCache>() { });
4233+
const options: IAgentHostAdapterOptions = {
4234+
icon: Codicon.copilot,
4235+
loading: constObservable(false),
4236+
buildWorkspace: () => undefined,
4237+
instantiationService,
4238+
getConnection: () => undefined,
4239+
agentCapabilities: capabilitiesObs,
4240+
};
4241+
const adapters = Array.from({ length: 200 }, (_, index) => disposables.add(instantiationService.createInstance(
4242+
AgentHostSessionAdapter,
4243+
createSession(`lazy-capabilities-${index}`),
4244+
'local-agent-host',
4245+
'agent-host-copilotcli',
4246+
'copilotcli',
4247+
options,
4248+
)));
4249+
const sessionUri = AgentSession.uri('copilotcli', 'lazy-capabilities-0').toString();
4250+
const defaultChat = buildDefaultChatUri(sessionUri);
4251+
const peerChat = buildChatUri(sessionUri, 'peer-1');
4252+
4253+
const listenerCountBeforeCatalog = listenerCount;
4254+
adapters[0].applyChatCatalog(makeState([
4255+
makeChatSummary(defaultChat, ''),
4256+
makeChatSummary(peerChat, 'Peer'),
4257+
], { defaultChat }));
4258+
const listenerCountAfterCatalog = listenerCount;
4259+
agentCapabilities = new Map([['copilotcli', { multipleChats: { fork: true } }]]);
4260+
capabilitiesChanged.fire();
4261+
4262+
assert.deepStrictEqual({
4263+
listenerCountBeforeCatalog,
4264+
listenerCountAfterCatalog,
4265+
chatFragmentsAfterHydration: adapters[0].chats.get().map(chat => chat.resource.fragment),
4266+
}, {
4267+
listenerCountBeforeCatalog: 0,
4268+
listenerCountAfterCatalog: 1,
4269+
chatFragmentsAfterHydration: ['', 'peer-1'],
4270+
});
4271+
});
4272+
42194273
test('forkChat forwards the source chat and turn to the host and surfaces a new peer chat', () => runWithFakedTimers<void>({ useFakeTimers: true }, async () => {
42204274
const provider = createProvider(disposables, agentHost);
42214275
const session = setupMultiChatSession(provider, 'multi-fork');

0 commit comments

Comments
 (0)