Skip to content

Commit 19bfde5

Browse files
ozzafarCopilot
andcommitted
Add regression tests for tool-call timeout handling
Extract the shared "race work against a timeout" logic used by DebuggingExecutor.dapRequest and DebugMCPServer.runTool into a tested withTimeout() utility (behavior preserved), and add regression coverage for the "server stuck when a tool call hangs" fix: - withTimeout.test.ts: resolves fast work, propagates work rejections, rejects with the caller's error when work hangs, and clears the timer so a late-but-eventual result still wins. - routing.test.ts: a control server that accepts but never responds must cause handleStartDebugging to reject with the actionable "did not respond within ... unresponsive" message. RoutingDebuggingHandler now accepts an optional forwardTimeoutMs override so the test runs fast. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ea24b92-40ad-4539-aa13-61aa6ff8751f
1 parent a6583de commit 19bfde5

6 files changed

Lines changed: 157 additions & 36 deletions

File tree

src/debugMCPServer.ts

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
IDebuggingHandler
1212
} from '.';
1313
import { logger } from './utils/logger';
14+
import { withTimeout } from './utils/withTimeout';
1415
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
1516
import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js';
1617
import { isInitializeRequest } from '@modelcontextprotocol/sdk/types.js';
@@ -182,23 +183,15 @@ export class DebugMCPServer {
182183
toolName: string,
183184
run: () => Promise<string>
184185
): Promise<{ content: { type: 'text'; text: string }[] }> {
185-
let timer: ReturnType<typeof setTimeout> | undefined;
186-
const backstop = new Promise<never>((_, reject) => {
187-
timer = setTimeout(() => {
188-
reject(new Error(
189-
`Tool "${toolName}" did not complete within ${Math.round(this.toolBackstopMs / 1000)}s and was aborted. ` +
190-
'The debug adapter or target VS Code window may be unresponsive. Try stop_debugging and retry, or reload the window.'
191-
));
192-
}, this.toolBackstopMs);
193-
});
194-
try {
195-
const result = await Promise.race([run(), backstop]);
196-
return { content: [{ type: 'text' as const, text: result }] };
197-
} finally {
198-
if (timer) {
199-
clearTimeout(timer);
200-
}
201-
}
186+
const result = await withTimeout(
187+
run(),
188+
this.toolBackstopMs,
189+
() => new Error(
190+
`Tool "${toolName}" did not complete within ${Math.round(this.toolBackstopMs / 1000)}s and was aborted. ` +
191+
'The debug adapter or target VS Code window may be unresponsive. Try stop_debugging and retry, or reload the window.'
192+
)
193+
);
194+
return { content: [{ type: 'text' as const, text: result }] };
202195
}
203196

204197
/**

src/debuggingExecutor.ts

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import * as vscode from 'vscode';
44
import { DebugState, StackFrame } from './debugState';
55
import { logger } from './utils/logger';
6+
import { withTimeout } from './utils/withTimeout';
67

78
/**
89
* Outcome of dispatching `testing.debugAtCursor`.
@@ -62,22 +63,14 @@ export class DebuggingExecutor implements IDebuggingExecutor {
6263
command: string,
6364
args: unknown
6465
): Promise<any> {
65-
let timer: ReturnType<typeof setTimeout> | undefined;
66-
const timeout = new Promise<never>((_, reject) => {
67-
timer = setTimeout(() => {
68-
reject(new Error(
69-
`Debug adapter did not respond to '${command}' within ` +
70-
`${Math.round(DebuggingExecutor.DAP_REQUEST_TIMEOUT_MS / 1000)}s (it may be unresponsive).`
71-
));
72-
}, DebuggingExecutor.DAP_REQUEST_TIMEOUT_MS);
73-
});
74-
try {
75-
return await Promise.race([session.customRequest(command, args), timeout]);
76-
} finally {
77-
if (timer) {
78-
clearTimeout(timer);
79-
}
80-
}
66+
return withTimeout(
67+
Promise.resolve(session.customRequest(command, args)),
68+
DebuggingExecutor.DAP_REQUEST_TIMEOUT_MS,
69+
() => new Error(
70+
`Debug adapter did not respond to '${command}' within ` +
71+
`${Math.round(DebuggingExecutor.DAP_REQUEST_TIMEOUT_MS / 1000)}s (it may be unresponsive).`
72+
)
73+
);
8174
}
8275

8376
/**

src/routingDebuggingHandler.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,16 @@ export class RoutingDebuggingHandler implements IDebuggingHandler {
1919

2020
// Bound the router->worker round-trip so a hung worker can't hang the
2121
// forward forever. Kept above the worker's own timeout to avoid preempting
22-
// slow-but-progressing operations.
22+
// slow-but-progressing operations. `forwardTimeoutMs` can be passed
23+
// explicitly (mainly for tests) to override the derived value.
2324
private readonly forwardTimeoutMs: number;
2425

2526
constructor(
2627
private readonly registry: WorkspaceRegistry,
27-
timeoutInSeconds: number = 180
28+
timeoutInSeconds: number = 180,
29+
forwardTimeoutMs?: number
2830
) {
29-
this.forwardTimeoutMs = timeoutInSeconds * 1000 + 15_000;
31+
this.forwardTimeoutMs = forwardTimeoutMs ?? timeoutInSeconds * 1000 + 15_000;
3032
}
3133

3234
/**

src/test/routing.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import * as assert from 'assert';
44
import * as fs from 'fs';
5+
import * as http from 'http';
56
import * as os from 'os';
67
import * as path from 'path';
78
import { ControlServer } from '../controlServer';
@@ -37,13 +38,15 @@ class RecordingHandler implements IDebuggingHandler {
3738
suite('Multi-window routing', () => {
3839
let dir: string;
3940
const servers: ControlServer[] = [];
41+
const deadServers: http.Server[] = [];
4042

4143
setup(() => {
4244
dir = fs.mkdtempSync(path.join(os.tmpdir(), 'debugmcp-routing-test-'));
4345
});
4446

4547
teardown(async () => {
4648
await Promise.all(servers.splice(0).map(s => s.stop()));
49+
await Promise.all(deadServers.splice(0).map(s => new Promise<void>(resolve => s.close(() => resolve()))));
4750
fs.rmSync(dir, { recursive: true, force: true });
4851
});
4952

@@ -143,6 +146,49 @@ suite('Multi-window routing', () => {
143146
await assert.rejects(() => routing.handleStepOver(), /no active debug target/i);
144147
});
145148

149+
/**
150+
* Register a window whose control server accepts connections but never
151+
* responds, to simulate a hung/unresponsive VS Code window.
152+
*/
153+
async function startDeadWindow(fileName: string, workspaceFolders: string[]): Promise<void> {
154+
const server = http.createServer(() => { /* accept, never respond */ });
155+
deadServers.push(server);
156+
const port: number = await new Promise(resolve => {
157+
server.listen(0, '127.0.0.1', () => resolve((server.address() as { port: number }).port));
158+
});
159+
fs.writeFileSync(
160+
path.join(dir, fileName),
161+
JSON.stringify({
162+
pid: process.pid,
163+
controlPort: port,
164+
controlToken: 'secret',
165+
workspaceFolders,
166+
name: fileName,
167+
updatedAt: Date.now()
168+
}),
169+
'utf8'
170+
);
171+
}
172+
173+
test('rejects with an actionable error when the target window is unresponsive', async () => {
174+
const repoA = path.join(dir, 'repoA');
175+
await startDeadWindow('a.json', [repoA]);
176+
177+
// 200ms forward timeout so the hung round-trip fails fast in the test.
178+
const routing = new RoutingDebuggingHandler(new WorkspaceRegistry(process.pid, dir), 180, 200);
179+
const start = Date.now();
180+
await assert.rejects(
181+
() => routing.handleStartDebugging({
182+
fileFullPath: path.join(repoA, 'm.py'),
183+
workingDirectory: repoA
184+
}),
185+
/did not respond within[\s\S]*unresponsive/i
186+
);
187+
const elapsed = Date.now() - start;
188+
assert.ok(elapsed >= 150, `should wait for the ~200ms forward timeout, only took ${elapsed}ms`);
189+
assert.ok(elapsed < 5000, `forward timeout should fire promptly, took ${elapsed}ms`);
190+
});
191+
146192
test('control server rejects requests with a wrong token', async () => {
147193
const repoA = path.join(dir, 'repoA');
148194
await startWindow('a.json', [repoA], new RecordingHandler('A'), 'right-token');
@@ -160,3 +206,4 @@ suite('Multi-window routing', () => {
160206
);
161207
});
162208
});
209+

src/test/withTimeout.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// Copyright (c) Microsoft Corporation.
2+
3+
import * as assert from 'assert';
4+
import { withTimeout } from '../utils/withTimeout';
5+
6+
/**
7+
* Regression tests for the shared timeout primitive that bounds every DAP
8+
* request (DebuggingExecutor.dapRequest) and every MCP tool call
9+
* (DebugMCPServer.runTool). These guard the "server stuck when a tool hangs"
10+
* fix: a hung operation must reject with the caller's error instead of
11+
* pending forever, and a fast operation must pass through untouched.
12+
*/
13+
suite('withTimeout', () => {
14+
test('resolves with the work value when it settles in time', async () => {
15+
const value = await withTimeout(
16+
Promise.resolve('ok'),
17+
1000,
18+
() => new Error('should not fire')
19+
);
20+
assert.strictEqual(value, 'ok');
21+
});
22+
23+
test('propagates a rejection from the work promise (not the timeout error)', async () => {
24+
await assert.rejects(
25+
withTimeout(
26+
Promise.reject(new Error('work failed')),
27+
1000,
28+
() => new Error('timeout error')
29+
),
30+
/work failed/
31+
);
32+
});
33+
34+
test('rejects with the onTimeout error when work never settles', async () => {
35+
const start = Date.now();
36+
await assert.rejects(
37+
withTimeout(
38+
new Promise<string>(() => { /* never settles */ }),
39+
50,
40+
() => new Error('adapter unresponsive')
41+
),
42+
/adapter unresponsive/
43+
);
44+
const elapsed = Date.now() - start;
45+
assert.ok(elapsed >= 40, `should wait for the ~50ms timeout, only took ${elapsed}ms`);
46+
assert.ok(elapsed < 1000, `timeout should fire promptly, took ${elapsed}ms`);
47+
});
48+
49+
test('clears the timer so a late-but-eventual work result still wins', async () => {
50+
// Work resolves before the (long) timeout; the timer must be cleared so
51+
// it neither delays the result nor rejects afterwards.
52+
const work = new Promise<string>(resolve => setTimeout(() => resolve('late-ok'), 20));
53+
const value = await withTimeout(work, 5000, () => new Error('should not fire'));
54+
assert.strictEqual(value, 'late-ok');
55+
// Give any stray timer a chance to (wrongly) fire; nothing should throw.
56+
await new Promise(resolve => setTimeout(resolve, 30));
57+
});
58+
});

src/utils/withTimeout.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Copyright (c) Microsoft Corporation.
2+
3+
/**
4+
* Race a promise against a timeout so a hung operation can't block forever.
5+
*
6+
* Resolves/rejects with `work`'s outcome if it settles within `timeoutMs`;
7+
* otherwise rejects with the Error from `onTimeout()`. The timer is always
8+
* cleared so a slow-but-eventual `work` doesn't leak a pending timer or a
9+
* late rejection. Note: `work` itself is not cancelled — this only bounds how
10+
* long the caller waits.
11+
*/
12+
export async function withTimeout<T>(
13+
work: Promise<T>,
14+
timeoutMs: number,
15+
onTimeout: () => Error
16+
): Promise<T> {
17+
let timer: ReturnType<typeof setTimeout> | undefined;
18+
const timeout = new Promise<never>((_, reject) => {
19+
timer = setTimeout(() => reject(onTimeout()), timeoutMs);
20+
});
21+
try {
22+
return await Promise.race([work, timeout]);
23+
} finally {
24+
if (timer) {
25+
clearTimeout(timer);
26+
}
27+
}
28+
}

0 commit comments

Comments
 (0)