feat(mcplocal): every request answers, even when the pipeline wedges
The LLM budgets bound the failure we actually hit. This is the backstop for the ones they cannot see: a wedged plugin hook, a virtual-tool handler, an upstream path without its own timeout. transport.onmessage IS the whole request pipeline, and it writes to a socket that has already been hijacked from Fastify. The SDK does not await it, so a throw became an unhandled rejection and a hang became silence -- in both cases the client got no response at all and waited out its own timeout. onmessage is now structured so that reaching transport.send() is unconditional: the route call is raced against a deadline, and the notification flush and the send each carry their own catch, so a failure while flushing can no longer cost the client its response. transport.onerror was never assigned, so SDK-level transport errors were swallowed entirely. It is now. Shape of the answer is deliberate. A tools/call comes back as a SUCCESSFUL result with isError and readable text naming the trace code -- the same shape router.ts already uses for an expired _resultId, because a transport error tends to surface as a hard failure while a tool error is something the model reads and acts on. It also says the upstream may still be running, which is true and matters. Other methods get a JSON-RPC error (-32001). MCPLOCAL_TOOLCALL_DEADLINE_MS defaults to TOOLCALL_TIMEOUT_MS + 30s rather than 120s. The watchdog arms earlier in the request than mcpd's fetch does, so at equal values it would always fire first, masking mcpd's specific UpstreamTimeoutError with a generic message and cutting off pagination and the rest of the post-processing. There is a test asserting the two stay ordered. The pause queue is exempt, but not bypassed. It blocks until a human operator releases, edits or drops a response, so the deadline SUSPENDS -- clock stopped, then re-armed with whatever was left. A test asserts the re-arm, because an exemption that forgot to re-arm would silently make every paused request immortal. End-to-end: an upstream that never settles now yields a response in under a second carrying ⚠, the deadline, and `mcpctl trace <code>`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqMidYEGUJG5fxeoTELBu2
This commit is contained in:
74
src/mcplocal/tests/toolcall-deadline.test.ts
Normal file
74
src/mcplocal/tests/toolcall-deadline.test.ts
Normal file
@@ -0,0 +1,74 @@
|
||||
import { describe, it, expect, vi, afterEach } from 'vitest';
|
||||
import {
|
||||
createRequestDeadline,
|
||||
DeadlineExceededError,
|
||||
TOOLCALL_DEADLINE_MS,
|
||||
} from '../src/http/deadline.js';
|
||||
import { TOOLCALL_TIMEOUT_MS } from '../src/http/mcpd-client.js';
|
||||
|
||||
afterEach(() => { vi.restoreAllMocks(); });
|
||||
|
||||
describe('TOOLCALL_DEADLINE_MS', () => {
|
||||
it('exceeds mcpd\'s own tool-call timeout', () => {
|
||||
// The watchdog arms earlier in the request than mcpd's fetch does. At equal
|
||||
// values it would always fire first, masking mcpd's specific
|
||||
// UpstreamTimeoutError with a generic deadline message and cutting off
|
||||
// pagination and the rest of the post-processing.
|
||||
expect(TOOLCALL_DEADLINE_MS).toBeGreaterThan(TOOLCALL_TIMEOUT_MS);
|
||||
});
|
||||
|
||||
it('stays well below a typical client timeout so our error arrives first', () => {
|
||||
expect(TOOLCALL_DEADLINE_MS).toBeLessThan(600_000);
|
||||
});
|
||||
});
|
||||
|
||||
describe('createRequestDeadline', () => {
|
||||
it('rejects with DeadlineExceededError when the budget runs out', async () => {
|
||||
const d = createRequestDeadline('tools/call', 40);
|
||||
await expect(Promise.race([
|
||||
new Promise<never>(() => { /* the hang we are protecting against */ }),
|
||||
d.expiry,
|
||||
])).rejects.toBeInstanceOf(DeadlineExceededError);
|
||||
d.dispose();
|
||||
});
|
||||
|
||||
it('does not fire once disposed', async () => {
|
||||
const d = createRequestDeadline('tools/call', 30);
|
||||
d.dispose();
|
||||
const outcome = await Promise.race([
|
||||
d.expiry.then(() => 'fired', () => 'fired'),
|
||||
new Promise((r) => setTimeout(() => { r('quiet'); }, 120)),
|
||||
]);
|
||||
expect(outcome).toBe('quiet');
|
||||
});
|
||||
|
||||
it('stops the clock while suspended, so an operator pause cannot time out', async () => {
|
||||
const d = createRequestDeadline('tools/call', 100);
|
||||
|
||||
// A pause queue wait far longer than the whole deadline.
|
||||
const held = await d.suspended(async () => {
|
||||
await new Promise((r) => setTimeout(r, 250));
|
||||
return 'released';
|
||||
});
|
||||
expect(held).toBe('released');
|
||||
|
||||
// Still alive afterwards, with roughly the original budget left.
|
||||
const outcome = await Promise.race([
|
||||
d.expiry.then(() => 'fired', () => 'fired'),
|
||||
new Promise((r) => setTimeout(() => { r('still-running'); }, 40)),
|
||||
]);
|
||||
expect(outcome).toBe('still-running');
|
||||
d.dispose();
|
||||
});
|
||||
|
||||
it('re-arms after suspension rather than becoming immortal', async () => {
|
||||
const d = createRequestDeadline('tools/call', 60);
|
||||
await d.suspended(async () => { await new Promise((r) => setTimeout(r, 100)); });
|
||||
// The remaining budget must still expire — suspending is not a bypass.
|
||||
await expect(Promise.race([
|
||||
new Promise<never>(() => { /* hang */ }),
|
||||
d.expiry,
|
||||
])).rejects.toBeInstanceOf(DeadlineExceededError);
|
||||
d.dispose();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user