feat(chat): implement input state caching and management for chat sessions#309348
feat(chat): implement input state caching and management for chat sessions#309348DonJayamanne wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds extension-host-side caching for ChatSessionInputState keyed by session resource URIs, aiming to keep chat session input state stable across invocations and reduce repeated controller calls.
Changes:
- Added a per-session
ResourceMapcache for resolvedChatSessionInputStateobjects inExtHostChatSessions. - Added cache seeding and invalidation/cleanup paths (session dispose, controller unregister, provider option-group changes).
- Cleared cached input state when a chat session is released from
ExtHostChatAgents2.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/api/common/extHostChatSessions.ts | Introduces _inputStateCache, cache seeding, and cache invalidation APIs for chat session input state resolution. |
| src/vs/workbench/api/common/extHostChatAgents2.ts | Clears input-state cache on session release to avoid retaining stale state. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/vs/workbench/api/common/extHostChatSessions.ts:909
- The input-state cache can return stale option selections after session option updates.
$provideHandleOptionsChangeupdates existingChatSessionInputStateImplinstances via_setGroups(...)+_fireDidChange(), which bypasses thegroupssetter and therefore bypasses the new cache invalidation logic. With caching enabled, subsequentgetInputStateForSession(...)calls can keep returning an outdated cached state instead of recomputing/refreshing based on the updated options. Consider invalidating the cache for the affectedsessionResource(or at least the scheme) when handling option changes, or ensure option-change propagation updates the cached state as well.
if (sessionResource) {
const cached = this._inputStateCache.get(sessionResource);
if (cached) {
return cached;
}
}
- Files reviewed: 2/2 changed files
- Comments generated: 3
| // Seed the cache so the first $invokeAgent call after session open gets a cache hit | ||
| this._inputStateCache.set(sessionResource, inputState); | ||
|
|
There was a problem hiding this comment.
$provideChatSessionContent seeds _inputStateCache before awaiting provider.provider.provideChatSessionContent(...). If that provider call throws or the token is cancelled before the session is fully created, the cache entry for sessionResource will remain and can retain references unnecessarily. Consider seeding the cache only after provideChatSessionContent succeeds, or deleting the cache entry in an error/cancellation path.
| // Invalidate only the cache entries that hold this specific inputState object | ||
| for (const [uri, cached] of this._inputStateCache) { | ||
| if (cached === inputState) { | ||
| this._inputStateCache.delete(uri); | ||
| } | ||
| } |
There was a problem hiding this comment.
When inputState.groups is set, this callback updates entry.optionGroups (controller-wide) but only invalidates cache entries whose cached value is the same inputState object. If multiple session resources for the same scheme have cached input states, they can become inconsistent with the new optionGroups/defaults. Consider clearing cached input states for the whole scheme (or otherwise ensuring all affected cached states are refreshed) when controller-level option groups change.
This issue also appears on line 904 of the same file.
| // Invalidate only the cache entries that hold this specific inputState object | |
| for (const [uri, cached] of this._inputStateCache) { | |
| if (cached === inputState) { | |
| this._inputStateCache.delete(uri); | |
| } | |
| } | |
| // Option groups are controller-wide, so clear cached input states to force refresh. | |
| this._inputStateCache.clear(); |
| @@ -869,10 +917,17 @@ export class ExtHostChatSessions extends Disposable implements ExtHostChatSessio | |||
| token, | |||
| ); | |||
| if (result) { | |||
| if (sessionResource) { | |||
| this._inputStateCache.set(sessionResource, result); | |||
| } | |||
| return result; | |||
| } | |||
| } | |||
| return this._createInputStateFromOptions(controllerData?.optionGroups ?? [], initialSessionOptions); | |||
| const fallback = this._createInputStateFromOptions(controllerData?.optionGroups ?? [], initialSessionOptions); | |||
| if (sessionResource) { | |||
| this._inputStateCache.set(sessionResource, fallback); | |||
| } | |||
| return fallback; | |||
There was a problem hiding this comment.
New caching behavior in getInputStateForSession (cache hits, scheme-wide invalidation, and seeding on session open) is not covered by existing tests. There is already an ExtHostChatSessions test suite (src/vs/workbench/api/test/browser/mainThreadChatSessions.test.ts), so it would be good to add regression tests that (1) verify cached values are reused to avoid repeated controller calls and (2) verify cache invalidation when session options/input state change.
For #288457 (comment)
Root Cause Analysis
Bug:
getChatSessionInputStateCalled on Every Prompt SendSummary
The extension-side
getChatSessionInputStatehandler is invoked on every prompt send to a contributed chat session, even when the session is already open and no options have changed. This is redundant and potentially expensive — the extension handler may rebuild option groups, query remote state, etc.Symptoms
2+3) →getChatSessionInputStateis called ✓ (expected on first use)4+5) →getChatSessionInputStateis called again ✗ (unexpected)Root Cause
Call Chain
Why It Happens
In
extHostChatAgents2.ts, the$invokeAgentmethod builds achatSessionContextobject (lines 977-995) to pass to the agent'sinvoke()handler as part ofChatContext. To populatechatSessionContext.inputState, it callsgetInputStateForSession()— which unconditionally calls the extension'sgetChatSessionInputStatehandler.There is no caching of the resolved input state. Every prompt re-fetches it from the extension, even though:
Key Files
src/vs/workbench/api/common/extHostChatAgents2.tsgetInputStateForSessionon every$invokeAgentsrc/vs/workbench/api/common/extHostChatSessions.tsgetInputStateForSession()— unconditionally calls extension handlersrc/vs/workbench/api/common/extHostChatSessions.ts$provideChatSessionContent()— also calls handler on session openOther Call Sites of
getChatSessionInputStateThe handler is also called in these contexts (which are appropriate):
$provideChatSessionContent, line 596) — needed to set initial state$newChatSessionItem, line 1043) — needed for new sessions$provideChatSessionInputState, line 1091) — explicit refresh requestThe problematic call is specifically the one in
getInputStateForSession()triggered from$invokeAgent.Solution: Per-Session Input State Cache
Approach
Add a
ResourceMap<ChatSessionInputState>cache inExtHostChatSessionskeyed by session URI. Return cached results on subsequent prompt sends, and invalidate when the underlying state actually changes.Cache Lifecycle
Invalidation Points (6 total)
$disposeChatSessionContent$releaseSessioninextHostChatAgents2.ts$provideHandleOptionsChangeinputState.groupsonChangedDelegateincreateChatSessionInputState$provideChatSessionProviderOptionsWhy URI-Only Cache Key Is Safe
initialSessionOptions(passed alongside the session URI) is derived fromchatSessionsService.getSessionOptions(sessionResource)— it's the current option state for that specific session. When a user changes an option,$provideHandleOptionsChangefires, which invalidates the cache. So the options are implicitly part of the cache validity — they don't need to be part of the key.Changes Made
src/vs/workbench/api/common/extHostChatSessions.ts:_inputStateCache: ResourceMap<ChatSessionInputState>clearInputStateCache(resource)public method_clearInputStateCacheForScheme(scheme)private helpergetInputStateForSession()to check/populate cache$provideChatSessionContent()src/vs/workbench/api/common/extHostChatAgents2.ts:clearInputStateCachecall in$releaseSession()Leaks - objects created by createChatSessionInputState never disposed
Fix: ChatSessionInputState disposal leak
Problem Statement
ChatSessionInputStateImplobjects are created but never disposed. Each instance holds anEmitter<void>(#onDidChangeEmitter) which is never cleaned up. Additionally, these objects accumulate in theinputStates: Set<ChatSessionInputStateImpl>on the controller data — they are added viainputStates.add(inputState)but never removed.Root Cause Analysis
Where ChatSessionInputStateImpl is created
There are two categories of creation:
"Managed" instances — created via
controller.createChatSessionInputState()(line 516-548 in extHostChatSessions.ts). These are:inputStatesset (line 546)onChangedDelegatecallback that fires$updateChatSessionInputStateover the proxygetChatSessionInputStatehandler"Unmanaged" instances — created via
new ChatSessionInputStateImpl(groups)directly (lines 425, 857, 871, 1101). These are:inputStates_createInputStateFromOptionsLifecycle of managed instances
Multiple leak vectors
inputStatesset grows unbounded: Every call tocreateChatSessionInputStateadds to the set; nothing removes from it. The set is per-controller and lives forever. When$provideHandleOptionsChangefires, it iterates ALL inputStates (line 709) and updates them — stale ones too.#onDidChangeEmitternever disposed:ChatSessionInputStateImplhas anew Emitter<void>()but nodispose()method. The emitter leaks.Extension-side
onDidChangelistener leaks: The Copilot extension subscribesstate.onDidChange(...)(line 282) but the returnedIDisposableis never stored or disposed._inputStateCacheretains references: The cache is cleaned up in some paths but since the objects themselves are never disposed, any remaining reference keeps the emitter alive.When are new input states created?
$provideChatSessionContent)$newChatSessionItem)$provideChatSessionInputStateis called from the main threadgetInputStateForSession(every prompt send, if not cached)So over the lifetime of a VS Code window, if a user opens multiple chat sessions, each creates a new
ChatSessionInputStateImplthat is never cleaned up.Proposed Solution
1. Make
ChatSessionInputStateImpldisposableAdd a
dispose()method that:#onDidChangeEmitter2. Remove from
inputStatesset when no longer neededWhen a session's input state is replaced (e.g., new
getChatSessionInputStatecall for the same session) or the session is disposed, remove the old input state frominputStatesand dispose it.3. Specific changes
A.
ChatSessionInputStateImpl— add disposeB.
_inputStateCache— dispose evicted entriesWhen an entry is removed from
_inputStateCache, if the evicted value is aChatSessionInputStateImplthat is "unmanaged" (not in theinputStatesset), dispose it. For managed ones, the lifecycle is tied to theinputStatesset.Actually, looking more carefully: the cache stores whatever comes back from
getChatSessionInputState, which could be a managed instance. The managed instances are also ininputStates. So:_inputStateCacheshould NOT own disposal — it's just a lookup cache.inputStatesset is the owner of managed instances._createInputStateFromOptions) are not tracked anywhere and should either be tracked or made lightweight (no emitter).C.
inputStatesset — remove and dispose when session completesWhen
$disposeChatSessionContentis called or when the controller is disposed, clear and dispose allinputStates:inputStates, calldispose()on each, thenclear().D. Track association between session URI and its input state
Currently there's no mapping from session URI → its inputState in the
inputStatesset. The_inputStateCacheserves this purpose partially. We need to ensure that when a session's input state is replaced or the session is disposed, the corresponding entry ininputStatesis removed and disposed.Approach: Add a
ResourceMap<ChatSessionInputStateImpl>to track the active input state per session. When a new one is created for the same session, dispose the old one. On session dispose, look up and dispose.Alternatively, since the
_inputStateCachealready maps URI → inputState, we can dispose the old value when setting a new one, and dispose on delete. But we need to be careful: the_inputStateCachestores the interface type, not the impl.Simplest correct approach
ChatSessionInputStateImpldisposable (dispose the emitter)inputStatesset, dispose them:createChatSessionInputState: no change (we don't know which session it's for yet)_inputStateCache: dispose the removed value if it's a ChatSessionInputStateImpl$disposeChatSessionContent: already clears cache → will now disposeclearInputStateCache: already clears cache → will now disposeThe key insight is that the
_inputStateCacheis the right place to manage the lifecycle of per-session input states. When a cache entry is evicted (replaced or deleted), the old value should be disposed AND removed frominputStates.Files to Change
src/vs/workbench/api/common/extHostChatSessions.tsdispose()toChatSessionInputStateImpl_inputStateCache, also dispose the old value and remove frominputStatesinputStates_inputStateCache, dispose the old valuesrc/vs/workbench/api/test/browser/mainThreadChatSessions.test.ts(or equivalent)Todos