Skip to content

Commit a30bc49

Browse files
committed
Address PR review comments from review #3773277390
- Fix race condition in server-manager getOrRestart: serialize concurrent calls per server type using pendingStarts map with async/await pattern - Fix lsp-handlers filePath docstring: document that relative paths are accepted and resolved against getUserWorkspaceDir() - Fix semantic_validation/syntax_validation READMEs: clarify that before/ after .ql files differ (not identical as previously stated) - Fix semantic_validation/after/monitoring-state.json: update parameter shape to use ql_code matching the codeql_lsp_diagnostics tool contract - Add concurrent access serialization test for server-manager
1 parent d375e41 commit a30bc49

File tree

8 files changed

+90
-31
lines changed

8 files changed

+90
-31
lines changed

client/integration-tests/primitives/tools/codeql_lsp_diagnostics/semantic_validation/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,5 +13,5 @@ references undefined types (e.g., `UndefinedType`).
1313
## EXPECTED OUTPUTS
1414

1515
- `isValid` is `false` with semantic diagnostics about unresolvable types.
16-
- The `semantic_query.ql` file remains unchanged (identical in before/ and after/).
16+
- The `before/semantic_query.ql` contains the erroneous query; `after/semantic_query.ql` shows the corrected version after addressing the diagnostics.
1717
- Monitoring state updated to record a successful `codeql_lsp_diagnostics` call.
Lines changed: 8 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,17 @@
11
{
22
"sessions": [
33
{
4-
"sessionId": "codeql-eval-session",
5-
"queryPath": "/test/semantic_query.ql",
6-
"language": "javascript",
7-
"queryType": "test",
8-
"description": "CodeQL LSP diagnostics test",
9-
"status": "active",
10-
"state": {
11-
"compilationStatus": "success",
12-
"testStatus": "no_tests",
13-
"documentationStatus": "missing",
14-
"filesPresent": ["/test/semantic_query.ql"]
15-
},
16-
"mcpCalls": [
4+
"id": "integration_test_session",
5+
"calls": [
176
{
18-
"callId": "eval-call-1",
19-
"timestamp": "2024-01-01T00:00:00.000Z",
20-
"toolName": "codeql_lsp_diagnostics",
7+
"tool": "codeql_lsp_diagnostics",
8+
"timestamp": "2026-02-08T00:00:00.000Z",
9+
"status": "success",
2110
"parameters": {
22-
"sessionId": "codeql-eval-session",
23-
"query": "from DataFlow::Configuration cfg select cfg",
24-
"language": "javascript"
25-
},
26-
"success": true,
27-
"duration": 1500,
28-
"result": "semantic validation completed"
11+
"ql_code": "from NonExistentType item\nwhere item.someProperty() = \"value\"\nselect item, \"This has semantic errors\""
12+
}
2913
}
30-
],
31-
"testExecutions": [],
32-
"qualityScores": []
14+
]
3315
}
3416
]
3517
}

client/integration-tests/primitives/tools/codeql_lsp_diagnostics/syntax_validation/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,5 @@ Tests that `codeql_lsp_diagnostics` detects syntax errors in invalid QL code.
1111
## EXPECTED OUTPUTS
1212

1313
- `isValid` is `false` with one or more error diagnostics.
14-
- The `test_query.ql` file remains unchanged (identical in before/ and after/).
14+
- The `before/test_query.ql` contains the erroneous query; `after/test_query.ql` shows the corrected version after addressing the diagnostics.
1515
- Monitoring state updated to record a successful `codeql_lsp_diagnostics` call.

server/dist/codeql-development-mcp-server.js

Lines changed: 26 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

server/dist/codeql-development-mcp-server.js.map

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

server/src/lib/server-manager.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ export class CodeQLServerManager {
5858
/** Keyed by `CodeQLServerType` — at most one per type at a time. */
5959
private servers = new Map<CodeQLServerType, ManagedServer>();
6060

61+
/** In-flight `getOrRestart` promises, keyed by server type, to serialize concurrent calls. */
62+
private pendingStarts = new Map<CodeQLServerType, Promise<CodeQLCLIServer | CodeQLLanguageServer | CodeQLQueryServer>>();
63+
6164
/** The session ID used for cache isolation. */
6265
private sessionId: string;
6366

@@ -264,11 +267,41 @@ export class CodeQLServerManager {
264267
/**
265268
* Get an existing server if its config matches, otherwise stop the old
266269
* one and start a new server.
270+
*
271+
* Concurrent calls for the same server type are serialized via
272+
* `pendingStarts` to avoid spawning duplicate server processes.
267273
*/
268274
private async getOrRestart(
269275
type: CodeQLServerType,
270276
config: ServerConfig,
271277
factory: () => CodeQLCLIServer | CodeQLLanguageServer | CodeQLQueryServer,
278+
): Promise<CodeQLCLIServer | CodeQLLanguageServer | CodeQLQueryServer> {
279+
// If another call is already starting a server of this type, wait for it
280+
// to settle (success or failure) and then re-check whether the result is
281+
// still usable.
282+
const inflight = this.pendingStarts.get(type);
283+
if (inflight) {
284+
try { await inflight; } catch { /* swallow — original caller handles the rejection */ }
285+
}
286+
287+
const work = this.doGetOrRestart(type, config, factory);
288+
this.pendingStarts.set(type, work);
289+
try {
290+
return await work;
291+
} finally {
292+
if (this.pendingStarts.get(type) === work) {
293+
this.pendingStarts.delete(type);
294+
}
295+
}
296+
}
297+
298+
/**
299+
* Core logic for getOrRestart, separated to allow serialization.
300+
*/
301+
private async doGetOrRestart(
302+
type: CodeQLServerType,
303+
config: ServerConfig,
304+
factory: () => CodeQLCLIServer | CodeQLLanguageServer | CodeQLQueryServer,
272305
): Promise<CodeQLCLIServer | CodeQLLanguageServer | CodeQLQueryServer> {
273306
const hash = computeConfigHash(type, config);
274307
const existing = this.servers.get(type);

server/src/tools/lsp/lsp-handlers.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ export interface LSPToolParams {
2727
character: number;
2828
/** Optional override for the file content (if not reading from disk). */
2929
fileContent?: string;
30-
/** Absolute path to the QL file. */
30+
/**
31+
* Path to the QL file. May be absolute or relative.
32+
* Relative paths are resolved against `getUserWorkspaceDir()`
33+
* (respects the `CODEQL_MCP_WORKSPACE` environment variable).
34+
*/
3135
filePath: string;
3236
/** 0-based line number in the document. */
3337
line: number;

server/test/src/lib/server-manager.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,20 @@ describe('CodeQLServerManager', () => {
165165
expect(manager.isRunning('language')).toBe(true);
166166
expect(server1.shutdown).toHaveBeenCalled();
167167
});
168+
169+
it('should serialize concurrent calls for the same server type', async () => {
170+
const manager = new CodeQLServerManager({ sessionId: 'ls-concurrent' });
171+
const config = { searchPath: '/ql' };
172+
173+
// Fire two concurrent requests — both should resolve to the same server
174+
const [server1, server2] = await Promise.all([
175+
manager.getLanguageServer(config),
176+
manager.getLanguageServer(config),
177+
]);
178+
179+
expect(server1).toBe(server2);
180+
expect(manager.isRunning('language')).toBe(true);
181+
});
168182
});
169183

170184
describe('getQueryServer', () => {

0 commit comments

Comments
 (0)