Skip to content

Commit cd2c6de

Browse files
committed
fix(vscode): stable MCP server definition in VS Code
File watcher events (database discovery, query result creation) were triggering envBuilder.invalidate() + mcpProvider.fireDidChange(), causing VS Code to re-query provideMcpServerDefinitions() on every workspace file change. The debounce only coalesced rapid events but did not prevent the unnecessary re-provision cycle. The MCP server definition only needs to change when: - The extension itself changes (update/reinstall) - Workspace folder registration changes (folders added/removed) - Configuration changes that affect the server The running server discovers databases and query results on its own through filesystem scanning at tool invocation time, so notifying VS Code of file content changes is unnecessary. Also includes: - Prompt completion caching with 5s TTL to avoid repeated filesystem scans during rapid completion requests - Debounce timer cancellation when McpProvider is disposed - Unit tests verifying watchers do not trigger fireDidChange - Integration tests verifying file creation does not fire onDidChangeMcpServerDefinitions
1 parent fd50a2a commit cd2c6de

13 files changed

Lines changed: 642 additions & 173 deletions

extensions/vscode/esbuild.config.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ const testSuiteConfig = {
3737
'test/suite/bridge.integration.test.ts',
3838
'test/suite/copydb-e2e.integration.test.ts',
3939
'test/suite/extension.integration.test.ts',
40+
'test/suite/file-watcher-stability.integration.test.ts',
4041
'test/suite/mcp-completion-e2e.integration.test.ts',
4142
'test/suite/mcp-prompt-e2e.integration.test.ts',
4243
'test/suite/mcp-resource-e2e.integration.test.ts',

extensions/vscode/src/extension.ts

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -48,16 +48,14 @@ export async function activate(
4848
const queryWatcher = new QueryResultsWatcher(storagePaths, logger);
4949
disposables.push(dbWatcher, queryWatcher);
5050

51-
// When databases or query results change, rebuild the environment and
52-
// notify the MCP provider that the server definition has changed.
53-
dbWatcher.onDidChange(() => {
54-
envBuilder.invalidate();
55-
mcpProvider.fireDidChange();
56-
});
57-
queryWatcher.onDidChange(() => {
58-
envBuilder.invalidate();
59-
mcpProvider.fireDidChange();
60-
});
51+
// File-content changes (new databases, query results) do NOT require
52+
// a new MCP server definition. The running server discovers files on
53+
// its own through filesystem scanning at tool invocation time. The
54+
// definition only needs to change when the server binary, workspace
55+
// folder registration, or configuration changes.
56+
//
57+
// The watchers are still useful: they log file events for debugging
58+
// and DatabaseWatcher tracks known databases internally.
6159
} catch (err) {
6260
logger.warn(
6361
`Failed to initialize file watchers: ${err instanceof Error ? err.message : String(err)}`,

extensions/vscode/src/server/mcp-provider.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ export class McpProvider
3333
*/
3434
private readonly _extensionVersion: string;
3535

36+
/**
37+
* Handle for the pending debounced `fireDidChange()` timer.
38+
* Used to coalesce rapid file-system events into a single notification.
39+
*/
40+
private _debounceTimer: ReturnType<typeof globalThis.setTimeout> | undefined;
41+
3642
constructor(
3743
private readonly serverManager: ServerManager,
3844
private readonly envBuilder: EnvironmentBuilder,
@@ -51,9 +57,18 @@ export class McpProvider
5157
* will NOT restart the server. Use for lightweight updates (file watcher
5258
* events, extension changes, background install completion) where the
5359
* running server can continue with its current environment.
60+
*
61+
* Debounced: rapid-fire calls (e.g. from file-system watchers during
62+
* a build) are coalesced into a single notification after a short delay.
5463
*/
5564
fireDidChange(): void {
56-
this._onDidChange.fire();
65+
if (this._debounceTimer !== undefined) {
66+
globalThis.clearTimeout(this._debounceTimer);
67+
}
68+
this._debounceTimer = globalThis.setTimeout(() => {
69+
this._debounceTimer = undefined;
70+
this._onDidChange.fire();
71+
}, 1_000);
5772
}
5873

5974
/**
@@ -68,6 +83,11 @@ export class McpProvider
6883
* Use for changes that require a server restart (configuration changes).
6984
*/
7085
requestRestart(): void {
86+
// Cancel any pending debounced fireDidChange — the restart supersedes it.
87+
if (this._debounceTimer !== undefined) {
88+
globalThis.clearTimeout(this._debounceTimer);
89+
this._debounceTimer = undefined;
90+
}
7191
this.envBuilder.invalidate();
7292
this._revision++;
7393
this.logger.info(

extensions/vscode/test/extension.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ vi.mock('../src/bridge/database-watcher', () => ({
7777
const listeners: Function[] = [];
7878
return {
7979
onDidChange: (listener: Function) => { listeners.push(listener); return { dispose: () => {} }; },
80+
_fire: () => { for (const fn of listeners) fn(); },
8081
getKnownDatabases: vi.fn().mockReturnValue(new Set()),
8182
dispose: vi.fn(),
8283
};
@@ -88,6 +89,7 @@ vi.mock('../src/bridge/query-results-watcher', () => ({
8889
const listeners: Function[] = [];
8990
return {
9091
onDidChange: (listener: Function) => { listeners.push(listener); return { dispose: () => {} }; },
92+
_fire: () => { for (const fn of listeners) fn(); },
9193
dispose: vi.fn(),
9294
};
9395
}),
@@ -107,6 +109,8 @@ import { activate, deactivate } from '../src/extension';
107109
import * as vscode from 'vscode';
108110
import { McpProvider } from '../src/server/mcp-provider';
109111
import { EnvironmentBuilder } from '../src/bridge/environment-builder';
112+
import { DatabaseWatcher } from '../src/bridge/database-watcher';
113+
import { QueryResultsWatcher } from '../src/bridge/query-results-watcher';
110114

111115
function createMockContext(): vscode.ExtensionContext {
112116
return {
@@ -208,4 +212,42 @@ describe('Extension', () => {
208212
spy.mockRestore();
209213
}
210214
});
215+
216+
it('should NOT call fireDidChange when database watcher fires', async () => {
217+
await activate(ctx);
218+
219+
const dbWatcherInstance = vi.mocked(DatabaseWatcher).mock.results[0]?.value;
220+
const mcpProviderInstance = vi.mocked(McpProvider).mock.results[0]?.value;
221+
const envBuilderInstance = vi.mocked(EnvironmentBuilder).mock.results[0]?.value;
222+
223+
mcpProviderInstance.fireDidChange.mockClear();
224+
mcpProviderInstance.requestRestart.mockClear();
225+
envBuilderInstance.invalidate.mockClear();
226+
227+
// Simulate a database watcher event (e.g. codeql-database.yml created)
228+
dbWatcherInstance._fire();
229+
230+
expect(mcpProviderInstance.fireDidChange).not.toHaveBeenCalled();
231+
expect(mcpProviderInstance.requestRestart).not.toHaveBeenCalled();
232+
expect(envBuilderInstance.invalidate).not.toHaveBeenCalled();
233+
});
234+
235+
it('should NOT call fireDidChange when query results watcher fires', async () => {
236+
await activate(ctx);
237+
238+
const queryWatcherInstance = vi.mocked(QueryResultsWatcher).mock.results[0]?.value;
239+
const mcpProviderInstance = vi.mocked(McpProvider).mock.results[0]?.value;
240+
const envBuilderInstance = vi.mocked(EnvironmentBuilder).mock.results[0]?.value;
241+
242+
mcpProviderInstance.fireDidChange.mockClear();
243+
mcpProviderInstance.requestRestart.mockClear();
244+
envBuilderInstance.invalidate.mockClear();
245+
246+
// Simulate a query results watcher event (e.g. .bqrs file created)
247+
queryWatcherInstance._fire();
248+
249+
expect(mcpProviderInstance.fireDidChange).not.toHaveBeenCalled();
250+
expect(mcpProviderInstance.requestRestart).not.toHaveBeenCalled();
251+
expect(envBuilderInstance.invalidate).not.toHaveBeenCalled();
252+
});
211253
});

extensions/vscode/test/server/mcp-provider.test.ts

Lines changed: 91 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, it, expect, vi, beforeEach } from 'vitest';
1+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
22

33

44
import { McpProvider } from '../../src/server/mcp-provider';
@@ -236,4 +236,94 @@ describe('McpProvider', () => {
236236
it('should be disposable', () => {
237237
expect(() => provider.dispose()).not.toThrow();
238238
});
239+
240+
// ─────────────────────────────────────────────────────────────────────────
241+
// fireDidChange — debouncing
242+
// ─────────────────────────────────────────────────────────────────────────
243+
244+
describe('fireDidChange debouncing', () => {
245+
beforeEach(() => {
246+
vi.useFakeTimers();
247+
});
248+
249+
afterEach(() => {
250+
vi.useRealTimers();
251+
});
252+
253+
it('should debounce rapid calls into a single event', () => {
254+
const listener = vi.fn();
255+
provider.onDidChangeMcpServerDefinitions(listener);
256+
257+
// Fire 10 rapid calls
258+
for (let i = 0; i < 10; i++) {
259+
provider.fireDidChange();
260+
}
261+
262+
// Before timer fires, no events should have been emitted
263+
expect(listener).not.toHaveBeenCalled();
264+
265+
// After debounce period, exactly one event should fire
266+
vi.runAllTimers();
267+
expect(listener).toHaveBeenCalledTimes(1);
268+
});
269+
270+
it('should coalesce events within the debounce window', () => {
271+
const listener = vi.fn();
272+
provider.onDidChangeMcpServerDefinitions(listener);
273+
274+
provider.fireDidChange();
275+
vi.advanceTimersByTime(100);
276+
provider.fireDidChange();
277+
vi.advanceTimersByTime(100);
278+
provider.fireDidChange();
279+
280+
// Still within debounce window — no events yet
281+
expect(listener).not.toHaveBeenCalled();
282+
283+
// Let debounce timer complete
284+
vi.runAllTimers();
285+
expect(listener).toHaveBeenCalledTimes(1);
286+
});
287+
288+
it('should fire separate events for calls outside debounce window', () => {
289+
const listener = vi.fn();
290+
provider.onDidChangeMcpServerDefinitions(listener);
291+
292+
provider.fireDidChange();
293+
vi.runAllTimers();
294+
expect(listener).toHaveBeenCalledTimes(1);
295+
296+
// Second call well after first debounce completed
297+
provider.fireDidChange();
298+
vi.runAllTimers();
299+
expect(listener).toHaveBeenCalledTimes(2);
300+
});
301+
302+
it('should not debounce requestRestart (fires immediately)', () => {
303+
const listener = vi.fn();
304+
provider.onDidChangeMcpServerDefinitions(listener);
305+
306+
provider.requestRestart();
307+
308+
// requestRestart should fire immediately without waiting for debounce
309+
expect(listener).toHaveBeenCalledTimes(1);
310+
});
311+
312+
it('should cancel pending debounced fire when requestRestart is called', () => {
313+
const listener = vi.fn();
314+
provider.onDidChangeMcpServerDefinitions(listener);
315+
316+
// Start a debounced fireDidChange
317+
provider.fireDidChange();
318+
expect(listener).not.toHaveBeenCalled();
319+
320+
// requestRestart should fire immediately and cancel the pending debounce
321+
provider.requestRestart();
322+
expect(listener).toHaveBeenCalledTimes(1);
323+
324+
// Running timers should NOT fire the debounced event again
325+
vi.runAllTimers();
326+
expect(listener).toHaveBeenCalledTimes(1);
327+
});
328+
});
239329
});
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/**
2+
* Integration tests for file watcher stability.
3+
*
4+
* Verifies that workspace file changes (database discovery, query result
5+
* creation) do NOT trigger redundant MCP server definition change events.
6+
*
7+
* The MCP server definition should only change when:
8+
* - The extension itself changes (update / reinstall)
9+
* - Workspace folder registration changes (folders added / removed)
10+
* - Configuration changes that affect the server
11+
*
12+
* File content changes are NOT a reason to re-provide the definition:
13+
* the running server discovers files on its own through filesystem
14+
* scanning at tool invocation time.
15+
*/
16+
17+
import * as assert from 'assert';
18+
import * as fs from 'fs';
19+
import * as path from 'path';
20+
import * as vscode from 'vscode';
21+
22+
const EXTENSION_ID = 'advanced-security.vscode-codeql-development-mcp-server';
23+
24+
/** Milliseconds to wait past the 1 s debounce to confirm no event fires. */
25+
const SETTLE_MS = 2_500;
26+
27+
suite('File Watcher Stability Tests', () => {
28+
let api: any;
29+
const cleanupPaths: string[] = [];
30+
31+
suiteSetup(async function () {
32+
this.timeout(10_000);
33+
const ext = vscode.extensions.getExtension(EXTENSION_ID);
34+
assert.ok(ext, `Extension ${EXTENSION_ID} not found`);
35+
api = ext.isActive ? ext.exports : await ext.activate();
36+
37+
if (!vscode.workspace.workspaceFolders?.length) {
38+
console.log(
39+
'[file-watcher-stability] Skipping — requires workspace with at least one folder',
40+
);
41+
this.skip();
42+
}
43+
44+
// Let any startup-related events settle before running tests.
45+
await new Promise((r) => globalThis.setTimeout(r, SETTLE_MS));
46+
});
47+
48+
suiteTeardown(() => {
49+
for (const p of cleanupPaths) {
50+
try {
51+
fs.rmSync(p, { recursive: true, force: true });
52+
} catch {
53+
/* best-effort */
54+
}
55+
}
56+
});
57+
58+
// -----------------------------------------------------------------
59+
// Test 1: BQRS file creation must NOT fire definition change event
60+
// -----------------------------------------------------------------
61+
test('Creating a BQRS file should NOT trigger onDidChangeMcpServerDefinitions', async function () {
62+
this.timeout(10_000);
63+
64+
const mcpProvider = api.mcpProvider;
65+
assert.ok(mcpProvider, 'API missing mcpProvider');
66+
assert.ok(
67+
typeof mcpProvider.onDidChangeMcpServerDefinitions === 'function',
68+
'mcpProvider missing onDidChangeMcpServerDefinitions',
69+
);
70+
71+
let changeCount = 0;
72+
const disposable = mcpProvider.onDidChangeMcpServerDefinitions(() => {
73+
changeCount++;
74+
});
75+
76+
try {
77+
const workspaceRoot =
78+
vscode.workspace.workspaceFolders![0].uri.fsPath;
79+
const bqrsPath = path.join(
80+
workspaceRoot,
81+
`stability-test-${Date.now()}.bqrs`,
82+
);
83+
cleanupPaths.push(bqrsPath);
84+
85+
fs.writeFileSync(bqrsPath, Buffer.from([0x00]));
86+
87+
// Wait past the debounce period (1 s) plus buffer
88+
await new Promise((r) => globalThis.setTimeout(r, SETTLE_MS));
89+
90+
assert.strictEqual(
91+
changeCount,
92+
0,
93+
`onDidChangeMcpServerDefinitions fired ${changeCount} time(s) after BQRS file creation; ` +
94+
'file content changes should NOT trigger MCP server definition updates',
95+
);
96+
} finally {
97+
disposable.dispose();
98+
}
99+
});
100+
101+
// -----------------------------------------------------------------
102+
// Test 2: codeql-database.yml creation must NOT fire change event
103+
// -----------------------------------------------------------------
104+
test('Creating a codeql-database.yml should NOT trigger onDidChangeMcpServerDefinitions', async function () {
105+
this.timeout(10_000);
106+
107+
const mcpProvider = api.mcpProvider;
108+
assert.ok(mcpProvider, 'API missing mcpProvider');
109+
110+
let changeCount = 0;
111+
const disposable = mcpProvider.onDidChangeMcpServerDefinitions(() => {
112+
changeCount++;
113+
});
114+
115+
try {
116+
const workspaceRoot =
117+
vscode.workspace.workspaceFolders![0].uri.fsPath;
118+
const dbDir = path.join(
119+
workspaceRoot,
120+
`stability-test-db-${Date.now()}`,
121+
);
122+
fs.mkdirSync(dbDir, { recursive: true });
123+
cleanupPaths.push(dbDir);
124+
125+
const ymlPath = path.join(dbDir, 'codeql-database.yml');
126+
fs.writeFileSync(ymlPath, 'primaryLanguage: javascript\n');
127+
128+
await new Promise((r) => globalThis.setTimeout(r, SETTLE_MS));
129+
130+
assert.strictEqual(
131+
changeCount,
132+
0,
133+
`onDidChangeMcpServerDefinitions fired ${changeCount} time(s) after codeql-database.yml creation; ` +
134+
'file content changes should NOT trigger MCP server definition updates',
135+
);
136+
} finally {
137+
disposable.dispose();
138+
}
139+
});
140+
});

0 commit comments

Comments
 (0)