Skip to content

Commit 6df6bbd

Browse files
authored
perf: Fix CKS bug and don't store full instructions contents on the ChatModel (#308920)
* perf: Fix CKS bug and don't store full instructions contents on the ChatModel * Add test
1 parent 7ff7930 commit 6df6bbd

File tree

3 files changed

+37
-5
lines changed

3 files changed

+37
-5
lines changed

src/vs/platform/contextkey/browser/contextKeyService.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -529,9 +529,9 @@ class ScopedContextKeyService extends AbstractContextKeyService {
529529
}
530530

531531
public disposeContext(contextId: number): void {
532-
if (this._isDisposed) {
533-
return;
534-
}
532+
// Always forward to parent even after disposal — a child context may
533+
// be disposed after us and must still reach the root ContextKeyService
534+
// to delete its entry from _contexts.
535535
this._parent.disposeContext(contextId);
536536
}
537537

src/vs/platform/contextkey/test/browser/contextkey.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,4 +298,27 @@ suite('ContextKeyService', () => {
298298

299299
assert.strictEqual(eventFired, true, 'Should fire event when setting different number');
300300
});
301+
302+
test('disposeContext forwards through disposed scoped service', () => {
303+
const root = testDisposables.add(new ContextKeyService(new TestConfigurationService()));
304+
const scoped = root.createScoped(document.createElement('div'));
305+
const child = scoped.createScoped(document.createElement('div'));
306+
307+
// Set a value on the child so we can observe it
308+
child.createKey('testKey', 'value');
309+
assert.strictEqual(child.getContextKeyValue('testKey'), 'value');
310+
311+
// Dispose the intermediate scoped service first
312+
scoped.dispose();
313+
314+
// Now dispose the child — this should still forward to root
315+
// and clean up the child's context entry. Before the fix,
316+
// ScopedContextKeyService.disposeContext bailed out when
317+
// _isDisposed was true, leaking Context objects.
318+
child.dispose();
319+
320+
// The child's context should no longer be accessible through root
321+
assert.strictEqual(root.getContextKeyValue('testKey'), undefined,
322+
'Child context should be cleaned up even when parent scoped service was disposed first');
323+
});
301324
});

src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ import { ChatSessionStore, IChatSessionEntryMetadata } from '../model/chatSessio
4848
import { IChatSlashCommandService } from '../participants/chatSlashCommands.js';
4949
import { IChatTransferService } from '../model/chatTransferService.js';
5050
import { chatSessionResourceToId, getChatSessionType, isUntitledChatSession, LocalChatSessionUri } from '../model/chatUri.js';
51-
import { ChatRequestVariableSet, IChatRequestVariableEntry } from '../attachments/chatVariableEntries.js';
51+
import { ChatRequestVariableSet, IChatRequestVariableEntry, isPromptTextVariableEntry } from '../attachments/chatVariableEntries.js';
5252
import { ChatAgentLocation, ChatModeKind } from '../constants.js';
5353
import { ChatMessageRole, IChatMessage, ILanguageModelsService } from '../languageModels.js';
5454
import { ILanguageModelToolsService } from '../tools/languageModelToolsService.js';
@@ -1171,8 +1171,17 @@ export class ChatService extends Disposable implements IChatService {
11711171
if (instructionEntries.length > 0) {
11721172
allContext.push(...instructionEntries);
11731173
}
1174+
1175+
// Store only non-instruction variables on the model.
1176+
// Automatically-added promptText entries (~33 KB each) are
1177+
// ephemeral — re-collected every turn, never rendered in
1178+
// the UI, and not needed in serialized session history.
1179+
const storedVariables = allContext.filter(v => !(isPromptTextVariableEntry(v) && v.automaticallyAdded));
1180+
model.updateRequest(request, { variables: storedVariables });
1181+
1182+
// The full set (including instructions) is passed to the
1183+
// agent request only — not stored on the request model.
11741184
let variableData: IChatRequestVariableData = { variables: allContext };
1175-
model.updateRequest(request, variableData);
11761185

11771186
// Merge resolved variables (e.g. images from directories) for the
11781187
// agent request only - they are not stored on the request model.

0 commit comments

Comments
 (0)