From a341c8cb4dd8c15cb2ebb704b7898c4f5478984d Mon Sep 17 00:00:00 2001 From: Abigail-2459 Date: Sat, 17 Oct 2026 23:39:02 -0400 Subject: [PATCH 1/3] test(mcpl): the request-timeout tests decide when a late reply comes, instead of sleeping for it Three tests in test/mcpl-request-timeout.test.ts slept a fixed time for a stub server's late tools/call reply: 350 ms for a reply at 300 ms, and 600 ms for replies at 500 ms. On a loaded runner the reply lands after the sleep. CI for cabf3d0 failed that way on macOS with Node 24, in "a mandatory call deadline overrides requestTimeoutMs: 0 and surfaces its late response" (#478). On main at e670363, with the reply moved past the sleep, that test and "a late STATEFUL orphaned response is surfaced" see no orphaned-response, and "a timed-out request that later receives a response does not crash" passes even when the reply comes after it. The stub now holds the tools/call reply until the host's next tools/list. After the rejection, each of the three tests calls sendToolsList(). The stub writes the held reply ahead of the list's own on the same stdout, and the connection handles lines in order, so the late reply has been handled by the time the list resolves. The reply can't arrive before the timeout either. Each of the three has a 30 s timeout of its own, since a timeout that never fires now leaves the call pending. "requestTimeoutMs: 0 disables the per-request watchdog" waited 300 ms, but the default timeout is 60 s, so it passed just the same when 0 fell back to the default. It now ticks a mocked setTimeout past 60 s and checks that the call is still pending. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-request-timeout.test.ts | 63 ++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/test/mcpl-request-timeout.test.ts b/test/mcpl-request-timeout.test.ts index 2778469e3..f61a4f634 100644 --- a/test/mcpl-request-timeout.test.ts +++ b/test/mcpl-request-timeout.test.ts @@ -12,9 +12,12 @@ import { McplServerConnection } from '../src/mcpl/server-connection.js'; import type { McplHostCapabilities, McplServerConfig } from '../src/mcpl/types.js'; // Stdio server: answers initialize and tools/list, but swallows tools/call — -// the "zulip HTML-error / event-loop-wedge" profile (alive, deaf). +// the "zulip HTML-error / event-loop-wedge" profile (alive, deaf). A test can +// make it hold a reply in `held` instead, and the next tools/list then writes +// that reply ahead of its own, so the late reply comes when the test asks. const HUNG_SERVER = ` let buf = ''; +let held = null; process.stdin.on('data', (c) => { buf += c.toString('utf8'); let i; @@ -22,9 +25,12 @@ process.stdin.on('data', (c) => { const line = buf.slice(0, i); buf = buf.slice(i + 1); if (!line.trim()) continue; let m; try { m = JSON.parse(line); } catch { continue; } - const reply = (result) => process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id: m.id, result }) + '\\n'); + const reply = (result, id = m.id) => process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id, result }) + '\\n'); if (m.method === 'initialize') reply({ capabilities: {} }); - else if (m.method === 'tools/list') reply({ tools: [{ name: 'stuck', description: 's', inputSchema: { type: 'object' } }] }); + else if (m.method === 'tools/list') { + if (held) { reply(held.result, held.id); held = null; } + reply({ tools: [{ name: 'stuck', description: 's', inputSchema: { type: 'object' } }] }); + } // tools/call: deliberately NO response, connection stays open. } }); @@ -84,14 +90,19 @@ test('a request the server answers in time is unaffected by the timeout', async assert.equal(list.tools.length, 1); }); -test('requestTimeoutMs: 0 disables the per-request watchdog', async () => { +test('requestTimeoutMs: 0 disables the per-request watchdog', async (t) => { connection = await McplServerConnection.connect(config({ requestTimeoutMs: 0 }), HOST_CAPS); connection.ready(); + // On a mocked clock, past the 60 s default: a watchdog that 0 failed to + // disable would have fired by then. + t.mock.timers.enable({ apis: ['setTimeout'] }); let settled = false; const call = connection.sendToolsCall('stuck', {}).catch(() => {}).finally(() => { settled = true; }); - await new Promise((r) => setTimeout(r, 300)); + t.mock.timers.tick(60_001); + await new Promise((r) => setImmediate(r)); assert.equal(settled, false, 'with timeout disabled the request stays pending'); + t.mock.timers.reset(); // Cleanup: closing the connection rejects the pending request. await connection.close(); @@ -114,11 +125,14 @@ test('the timeout error warns the model the tool may have completed server-side' ); }); -test('a timed-out request that later receives a response does not crash (orphaned response ignored)', async () => { - // Server that answers tools/call after 500ms — beyond a 100ms timeout. +// The next three tests hold the server's reply, so a timeout that never fires +// leaves the call pending: the test's own timeout turns that into a failure. +test('a timed-out request that later receives a response does not crash (orphaned response ignored)', { timeout: 30_000 }, async () => { + // The server holds its tools/call reply until the next tools/list, so the + // reply comes after the 100ms timeout. const SLOW_SERVER = HUNG_SERVER.replace( '// tools/call: deliberately NO response, connection stays open.', - "else if (m.method === 'tools/call') setTimeout(() => reply({ content: [] }), 500);", + "else if (m.method === 'tools/call') held = { id: m.id, result: { content: [] } };", ); connection = await McplServerConnection.connect( { id: 'slow', command: process.execPath, args: ['-e', SLOW_SERVER], requestTimeoutMs: 100 }, @@ -127,19 +141,20 @@ test('a timed-out request that later receives a response does not crash (orphane connection.ready(); await assert.rejects(connection.sendToolsCall('stuck', {}), /did not respond/); - // Wait past the server's late response; it must be ignored, not crash. - await new Promise((r) => setTimeout(r, 600)); + // The late reply arrives ahead of the list's own, on the same stream, so by + // the time the list resolves it has been handled: ignored, not a crash. const list = await connection.sendToolsList(); assert.equal(list.tools.length, 1); }); -test('a late STATEFUL orphaned response is surfaced via mcpl:orphaned-response (not silently dropped)', async () => { - // Server answers tools/call after 500ms with a checkpoint-carrying result — - // beyond a 100ms timeout. The host advanced no checkpoint; the server did. - // That divergence must be greppable, so the connection emits an event. +test('a late STATEFUL orphaned response is surfaced via mcpl:orphaned-response (not silently dropped)', { timeout: 30_000 }, async () => { + // The server holds a checkpoint-carrying tools/call result until the next + // tools/list, after a 100ms timeout. The host advanced no checkpoint; the + // server did. That divergence must be greppable, so the connection emits an + // event. const STATEFUL_SLOW = HUNG_SERVER.replace( '// tools/call: deliberately NO response, connection stays open.', - "else if (m.method === 'tools/call') setTimeout(() => reply({ content: [], state: { checkpoint: 'cp-late', data: { n: 1 } } }), 500);", + "else if (m.method === 'tools/call') held = { id: m.id, result: { content: [], state: { checkpoint: 'cp-late', data: { n: 1 } } } };", ); connection = await McplServerConnection.connect( { id: 'slow-stateful', command: process.execPath, args: ['-e', STATEFUL_SLOW], requestTimeoutMs: 100 }, @@ -151,20 +166,20 @@ test('a late STATEFUL orphaned response is surfaced via mcpl:orphaned-response ( connection.on('orphaned-response', (info) => orphaned.push(info)); await assert.rejects(connection.sendToolsCall('stuck', {}), /did not respond/); - await new Promise((r) => setTimeout(r, 600)); + // The next request releases the late reply ahead of its own answer, so it + // has been handled by the time the list resolves, and the connection still + // works afterwards. + const list = await connection.sendToolsList(); + assert.equal(list.tools.length, 1); assert.equal(orphaned.length, 1, 'the dropped stateful response must be surfaced'); assert.equal(orphaned[0].hadState, true, 'the event records that state was carried'); - - // The connection still works afterwards. - const list = await connection.sendToolsList(); - assert.equal(list.tools.length, 1); }); -test('a mandatory call deadline overrides requestTimeoutMs: 0 and surfaces its late response', async () => { +test('a mandatory call deadline overrides requestTimeoutMs: 0 and surfaces its late response', { timeout: 30_000 }, async () => { const SLOW_SERVER = HUNG_SERVER.replace( '// tools/call: deliberately NO response, connection stays open.', - "else if (m.method === 'tools/call') setTimeout(() => reply({ content: [] }), 300);", + "else if (m.method === 'tools/call') held = { id: m.id, result: { content: [] } };", ); connection = await McplServerConnection.connect( { id: 'mandatory-deadline', command: process.execPath, args: ['-e', SLOW_SERVER], requestTimeoutMs: 0 }, @@ -178,7 +193,9 @@ test('a mandatory call deadline overrides requestTimeoutMs: 0 and surfaces its l connection.sendToolsCallWithDeadline('stuck', {}, 75), /outcome is unknown.*verify state before retrying/i, ); - await new Promise((r) => setTimeout(r, 350)); + // Only the deadline could settle the call, since the server sends nothing + // until the next request, which releases the late reply ahead of its own. + await connection.sendToolsList(); assert.deepEqual(orphaned, [{ method: 'tools/call', From 3dfee705a2f04e309e9a2dbc55555d4fd92c1a86 Mon Sep 17 00:00:00 2001 From: Abigail-2459 Date: Sat, 17 Oct 2026 23:55:59 -0400 Subject: [PATCH 2/3] test(mcpl): the releasing tools/list races no watchdog, and each test checks its late reply was sent From our internal review of the previous commit: - The plain and stateful tests' connection times out at 100 ms, and the tools/list that releases the held reply inherited that timeout, so a slow round trip could still fail them. After the rejection, both now lift the connection's request timeout, a private field sendRequest reads for each request. - The stub's tools/list answer says, in its tool's description, when it wrote a held reply first. Each of the three tests checks it, so a test whose stub held nothing, such as after the comment its replace() looks for changed, fails instead of passing without a late reply. - The plain test checks that its late reply emitted no orphaned-response. - The watchdog test ticks past 2^31 - 1 ms, the longest delay setTimeout takes, so any finite watchdog that 0 armed would fire, and restores the real timers in a finally before its assertion. - The comments say the next tools/list releases the held reply. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-request-timeout.test.ts | 49 +++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/test/mcpl-request-timeout.test.ts b/test/mcpl-request-timeout.test.ts index f61a4f634..a100f94c8 100644 --- a/test/mcpl-request-timeout.test.ts +++ b/test/mcpl-request-timeout.test.ts @@ -11,6 +11,9 @@ import assert from 'node:assert/strict'; import { McplServerConnection } from '../src/mcpl/server-connection.js'; import type { McplHostCapabilities, McplServerConfig } from '../src/mcpl/types.js'; +// What the stub's tools/list answer says when it wrote a held reply ahead of it. +const RELEASED = 'released a held reply first'; + // Stdio server: answers initialize and tools/list, but swallows tools/call — // the "zulip HTML-error / event-loop-wedge" profile (alive, deaf). A test can // make it hold a reply in `held` instead, and the next tools/list then writes @@ -28,8 +31,9 @@ process.stdin.on('data', (c) => { const reply = (result, id = m.id) => process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id, result }) + '\\n'); if (m.method === 'initialize') reply({ capabilities: {} }); else if (m.method === 'tools/list') { + const released = held !== null; if (held) { reply(held.result, held.id); held = null; } - reply({ tools: [{ name: 'stuck', description: 's', inputSchema: { type: 'object' } }] }); + reply({ tools: [{ name: 'stuck', description: released ? '${RELEASED}' : 's', inputSchema: { type: 'object' } }] }); } // tools/call: deliberately NO response, connection stays open. } @@ -53,6 +57,13 @@ function config(overrides?: Partial): McplServerConfig { }; } +/** Lift a connection's request timeout, so the tools/list that releases a + * held reply has no watchdog of its own to race. It sets a private field, + * which sendRequest reads for each request. */ +function liftRequestTimeout(c: McplServerConnection): void { + (c as unknown as { requestTimeoutMs: number }).requestTimeoutMs = 0; +} + let connection: McplServerConnection | null = null; afterEach(async () => { await connection?.close(); @@ -94,15 +105,19 @@ test('requestTimeoutMs: 0 disables the per-request watchdog', async (t) => { connection = await McplServerConnection.connect(config({ requestTimeoutMs: 0 }), HOST_CAPS); connection.ready(); - // On a mocked clock, past the 60 s default: a watchdog that 0 failed to - // disable would have fired by then. + // On a mocked clock, past the longest delay setTimeout takes (2^31 - 1 ms): + // a watchdog that 0 failed to disable, the 60 s default or any other, would + // have fired by then. t.mock.timers.enable({ apis: ['setTimeout'] }); let settled = false; const call = connection.sendToolsCall('stuck', {}).catch(() => {}).finally(() => { settled = true; }); - t.mock.timers.tick(60_001); - await new Promise((r) => setImmediate(r)); + try { + t.mock.timers.tick(2_147_483_647); + await new Promise((r) => setImmediate(r)); + } finally { + t.mock.timers.reset(); + } assert.equal(settled, false, 'with timeout disabled the request stays pending'); - t.mock.timers.reset(); // Cleanup: closing the connection rejects the pending request. await connection.close(); @@ -140,11 +155,17 @@ test('a timed-out request that later receives a response does not crash (orphane ); connection.ready(); + const orphaned: unknown[] = []; + connection.on('orphaned-response', (info) => orphaned.push(info)); + await assert.rejects(connection.sendToolsCall('stuck', {}), /did not respond/); - // The late reply arrives ahead of the list's own, on the same stream, so by - // the time the list resolves it has been handled: ignored, not a crash. + liftRequestTimeout(connection); + // The next tools/list makes the stub write the late reply ahead of the + // list's own, on the same stream, so by the time the list resolves the late + // reply has been handled: ignored, with no event and no crash. const list = await connection.sendToolsList(); - assert.equal(list.tools.length, 1); + assert.equal(list.tools[0]?.description, RELEASED, 'the stub wrote the late reply first'); + assert.deepEqual(orphaned, [], 'a plain late reply is ignored, not surfaced'); }); test('a late STATEFUL orphaned response is surfaced via mcpl:orphaned-response (not silently dropped)', { timeout: 30_000 }, async () => { @@ -166,11 +187,12 @@ test('a late STATEFUL orphaned response is surfaced via mcpl:orphaned-response ( connection.on('orphaned-response', (info) => orphaned.push(info)); await assert.rejects(connection.sendToolsCall('stuck', {}), /did not respond/); - // The next request releases the late reply ahead of its own answer, so it + liftRequestTimeout(connection); + // The next tools/list releases the late reply ahead of its own answer, so it // has been handled by the time the list resolves, and the connection still // works afterwards. const list = await connection.sendToolsList(); - assert.equal(list.tools.length, 1); + assert.equal(list.tools[0]?.description, RELEASED, 'the stub wrote the late reply first'); assert.equal(orphaned.length, 1, 'the dropped stateful response must be surfaced'); assert.equal(orphaned[0].hadState, true, 'the event records that state was carried'); @@ -194,8 +216,9 @@ test('a mandatory call deadline overrides requestTimeoutMs: 0 and surfaces its l /outcome is unknown.*verify state before retrying/i, ); // Only the deadline could settle the call, since the server sends nothing - // until the next request, which releases the late reply ahead of its own. - await connection.sendToolsList(); + // until the next tools/list, which releases the late reply ahead of its own. + const list = await connection.sendToolsList(); + assert.equal(list.tools[0]?.description, RELEASED, 'the stub wrote the late reply first'); assert.deepEqual(orphaned, [{ method: 'tools/call', From b6de2aa4c895cfc6bb8f2199d2b0f34c9f55728a Mon Sep 17 00:00:00 2001 From: Abigail-2459 Date: Sun, 18 Oct 2026 00:08:46 -0400 Subject: [PATCH 3/3] test(mcpl): the timeout lift checks that the field it sets is there From review: liftRequestTimeout sets the connection's private requestTimeoutMs through a cast. If the field were renamed, the cast would set a property nothing reads, and the plain and stateful tests would race a 100 ms watchdog again, which shows only as a flake. The helper now asserts the field is an own property of the connection first, so a rename fails both tests on every run. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-request-timeout.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/mcpl-request-timeout.test.ts b/test/mcpl-request-timeout.test.ts index a100f94c8..1491ff093 100644 --- a/test/mcpl-request-timeout.test.ts +++ b/test/mcpl-request-timeout.test.ts @@ -59,8 +59,10 @@ function config(overrides?: Partial): McplServerConfig { /** Lift a connection's request timeout, so the tools/list that releases a * held reply has no watchdog of its own to race. It sets a private field, - * which sendRequest reads for each request. */ + * which sendRequest reads for each request; the check fails the test if the + * field is renamed, where the lift would otherwise set nothing. */ function liftRequestTimeout(c: McplServerConnection): void { + assert.ok(Object.hasOwn(c, 'requestTimeoutMs'), 'the connection still has its requestTimeoutMs field'); (c as unknown as { requestTimeoutMs: number }).requestTimeoutMs = 0; }