feat: AgentMemoryService session cache + AgentMemoryCachePrimer#313124
feat: AgentMemoryService session cache + AgentMemoryCachePrimer#313124sneefyyy wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds conversation/session-scoped caching infrastructure for Copilot agent memory prompts, plus a small service to proactively warm that cache, as the first step toward user-scoped agent memories.
Changes:
- Add per-session memory prompt caching with in-flight request deduplication in
AgentMemoryService, and clear cache entries when a chat session disposes. - Introduce
AgentMemoryCachePrimerservice (new file) and register it in the extension’s service container. - Expand unit tests for the new caching/clearing behavior and for storing repo/user memories via the new memory API wrapper.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/tools/node/agentMemoryCachePrimer.ts | New primer service that fetches the memory prompt early to warm the cache. |
| extensions/copilot/src/extension/tools/common/agentMemoryService.ts | Implements session-keyed cache + in-flight dedupe and switches to @github/copilot-agentic-tools/memory for API calls. |
| extensions/copilot/src/extension/tools/common/test/agentMemoryService.spec.ts | Reworked tests to cover caching, cache clearing, and store operations against mocked memory API functions. |
| extensions/copilot/src/extension/extension/vscode-node/services.ts | Registers the new IAgentMemoryCachePrimer service. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 4
| } | ||
| // Call with sessionId but let service auto-determine repoNwo to match original working behavior | ||
| const response = await this.agentMemoryService.getMemoryPrompt(undefined, sessionId); | ||
| this.logService.info(`[AgentMemoryToolRegistrar] primed memory prompt cache for session ${sessionId || 'default'}, definitionVersion=${response?.storeToolDefinition?.definitionVersion ?? 'none'}`); |
There was a problem hiding this comment.
The log message still uses the old "AgentMemoryToolRegistrar" prefix and reports that the cache was primed even when sessionId is undefined (no caching happens in getMemoryPrompt) or when the fetch returns undefined. Update the prefix and only log a "primed" message when a response was successfully cached for a concrete session (otherwise log a more accurate message or lower the level).
| this.logService.info(`[AgentMemoryToolRegistrar] primed memory prompt cache for session ${sessionId || 'default'}, definitionVersion=${response?.storeToolDefinition?.definitionVersion ?? 'none'}`); | |
| if (!sessionId) { | |
| this.logService.debug('[AgentMemoryCachePrimer] fetched memory prompt without caching because no sessionId was provided'); | |
| return; | |
| } | |
| if (!response) { | |
| this.logService.debug(`[AgentMemoryCachePrimer] did not prime memory prompt cache for session ${sessionId} because no prompt response was returned`); | |
| return; | |
| } | |
| this.logService.info(`[AgentMemoryCachePrimer] primed memory prompt cache for session ${sessionId}, definitionVersion=${response.storeToolDefinition?.definitionVersion ?? 'none'}`); |
| if (response) { | ||
| this.logService.info(`[AgentMemoryService] Fetched memory prompt (${response.memoriesContext.memoriesCount} memories)${sessionId ? ` for conversation: ${sessionId}` : ''}`); | ||
| if (sessionId) { | ||
| this._conversationMemoryCache.set(sessionId, response); | ||
| } |
There was a problem hiding this comment.
clearCache(sessionId) is invoked on session disposal, but an in-flight _fetchMemoryPrompt() for that same sessionId can still resolve later and repopulate _conversationMemoryCache, defeating eviction and potentially leaking entries past session lifetime. Consider (1) clearing _inflightPrompts for the session and (2) adding a guard (e.g., per-session generation token / disposed-session set) so _fetchMemoryPrompt only caches if the session is still active / generation matches.
| @IChatSessionService private readonly chatSessionService: IChatSessionService, | ||
| ) { | ||
| super(); | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace on the blank line after super();. This commonly fails lint/format checks; please remove the whitespace so the blank line is empty.
| describe('getMemoryPrompt — cache lifecycle', () => { | ||
| it('fetches and caches response when sessionId is provided', async () => { | ||
| const svc = makeService(); | ||
| const result = await svc.getMemoryPrompt(undefined, 'session-1'); | ||
| expect(result).toBe(mockResponse); |
There was a problem hiding this comment.
The PR description mentions a unit test covering the stampede guard (two concurrent getMemoryPrompt calls should share one fetch), but this spec file doesn’t currently include a concurrency test for _inflightPrompts. Adding that test would ensure the deduplication behavior stays correct.
Summary
This PR is the first in a stack of three that implement user-scoped agent memories. It focuses on the caching layer and service infrastructure with no new user-facing behavior is introduced here.
Changes:
AgentMemoryService— session-keyed cache + stampede guardgetMemoryPrompt()now checks a per-session cache (Map<sessionId, MemoryPromptResponse>) before making a network request._inflightPrompts: Map<string, Promise<MemoryPromptResponse | undefined>>map deduplicates concurrent callers that arrive before the first response lands. Without this, multiple tools resolving in parallel on turn 0 would each fire a separate fetch.onDidDisposeChatSession).AgentMemoryCachePrimer— renamed fromAgentMemoryToolRegistraragentMemoryToolRegistrar.ts→agentMemoryCachePrimer.tsIAgentMemoryToolRegistrar→IAgentMemoryCachePrimerAgentMemoryToolRegistrar→AgentMemoryCachePrimerregisterMemoryTools()→primeCache()services.tsalongsideAgentMemoryService.Test coverage (
agentMemoryService.spec.ts)What's NOT in this PR
store_memorytool and turn-0 fix are in PR2.MemoryContextPromptmigration (CAPI coexistence) is in PR3.Test plan
AgentMemoryServiceunit tests pass (npm run test:unit)AgentMemoryCachePrimerimport resolves correctly inagentIntent.ts(part of PR2/PR3 stack)