fix(mcplocal): stop the 30s proxy timeout killing agent turns
`mcpctl chat <agent>` failed with
HTTP 503 {"error":"service_unavailable","message":"Cannot reach mcpd daemon. Is it running?"}
while mcpd was answering /healthz in 32ms. The message was wrong in a way that
cost real debugging time: mcplocal was reaching mcpd fine and giving up after
30s. journalctl shows the signature plainly — statusCode 503 with
responseTime 30003.87 on POST /api/v1/agents/reviewer/chat.
This blocks the agentic-teams epic outright. An agent turn is a multi-turn
tool-use loop that runs for minutes by design, so a 30s ceiling on the chat path
is not a safety net, it is a guaranteed failure for every non-trivial turn.
Three defects, all in the same path:
1. One blanket budget for every forwarded route. DEFAULT_TIMEOUT_MS = 30_000 is
right for CRUD and wrong for chat. Chat, project chat, llm infer and
inference-task streams now get LONG_RUNNING_TIMEOUT_MS (600_000, override
with MCPLOCAL_LONG_TIMEOUT_MS) — matching STREAM_TIMEOUT_MS, which the CLI
already allowed. mcplocal in the middle was the binding constraint.
2. Timeouts were reported as connection failures. Split UpstreamTimeoutError
out of ConnectionError and map it to 504 with an accurate message that says
the daemon IS reachable. ConnectionError still means unreachable and still
returns 503. Verified nothing else branches on ConnectionError.
3. SSE was buffered. `forward()` reads the whole body through res.text(), so
even turns that finished in time arrived as one blob and the CLI's live
token output never appeared. Streaming routes now use forwardStream() and
pipe the body straight through, preserving content-type and
x-accel-buffering (dropping the latter lets intermediaries re-buffer and
reintroduces the stall).
Also closes the escape that produced the sibling `500 code:23` failure: the body
read in forward() was outside the try, so when mcpd had already written SSE
headers the raw DOMException reached Fastify unhandled.
Tests: 9 new proxy tests. The two that matter — "does not abort an agent chat
that outlives the CRUD budget" and "streams SSE through instead of buffering" —
were confirmed to FAIL against the pre-fix behaviour and pass after. Three
existing mcpd-client tests asserted the old taxonomy and were updated to assert
the new one deliberately.
Local: build clean, workspace 2375 passed, lint unchanged at 869.
NOT YET LIVE: mcplocal runs from the installed RPM, so this needs a package
rebuild + `systemctl --user restart mcplocal` to take effect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4wNHWf7xSwnZCWpJcyv9p
This commit is contained in:
196
src/mcplocal/tests/proxy-long-running.test.ts
Normal file
196
src/mcplocal/tests/proxy-long-running.test.ts
Normal file
@@ -0,0 +1,196 @@
|
||||
import Fastify, { type FastifyInstance } from 'fastify';
|
||||
import { describe, it, expect, afterEach } from 'vitest';
|
||||
|
||||
import {
|
||||
McpdClient,
|
||||
UpstreamTimeoutError,
|
||||
ConnectionError,
|
||||
LONG_RUNNING_TIMEOUT_MS,
|
||||
DEFAULT_TIMEOUT_MS,
|
||||
} from '../src/http/mcpd-client.js';
|
||||
import { registerProxyRoutes } from '../src/http/routes/proxy.js';
|
||||
|
||||
/**
|
||||
* Regression cover for the 30s proxy timeout that made `mcpctl chat` fail with
|
||||
* a misleading "Cannot reach mcpd daemon" 503 while mcpd was answering
|
||||
* /healthz in 32ms.
|
||||
*
|
||||
* Three separate defects are pinned here:
|
||||
* 1. chat routes inherited the 30s CRUD budget, so any turn longer than 30s
|
||||
* failed — and an agent turn is a tool-use loop that routinely exceeds it;
|
||||
* 2. a timeout was reported as a connection failure, sending diagnosis after
|
||||
* a network fault that did not exist;
|
||||
* 3. SSE was buffered through res.text(), so streaming never reached the
|
||||
* client even when the turn finished in time.
|
||||
*/
|
||||
let app: FastifyInstance | null = null;
|
||||
let upstream: FastifyInstance | null = null;
|
||||
|
||||
afterEach(async () => {
|
||||
if (app) { await app.close(); app = null; }
|
||||
if (upstream) { await upstream.close(); upstream = null; }
|
||||
});
|
||||
|
||||
/** A stand-in mcpd. Returns its base URL. */
|
||||
async function startUpstream(register: (a: FastifyInstance) => void): Promise<string> {
|
||||
upstream = Fastify();
|
||||
register(upstream);
|
||||
await upstream.listen({ port: 0, host: '127.0.0.1' });
|
||||
const addr = upstream.server.address();
|
||||
if (addr === null || typeof addr === 'string') throw new Error('no address');
|
||||
return `http://127.0.0.1:${String(addr.port)}`;
|
||||
}
|
||||
|
||||
async function startProxy(baseUrl: string, timeoutMs?: number): Promise<FastifyInstance> {
|
||||
app = Fastify();
|
||||
registerProxyRoutes(app, new McpdClient(baseUrl, 'test-token', {}, timeoutMs));
|
||||
await app.ready();
|
||||
return app;
|
||||
}
|
||||
|
||||
describe('proxy — long-running route budget', () => {
|
||||
it('gives chat routes the long budget, not the 30s CRUD default', () => {
|
||||
// The constants themselves are the contract: a 30s cap on an agent turn is
|
||||
// a guaranteed failure, not a safety net.
|
||||
expect(DEFAULT_TIMEOUT_MS).toBe(30_000);
|
||||
expect(LONG_RUNNING_TIMEOUT_MS).toBeGreaterThanOrEqual(600_000);
|
||||
});
|
||||
|
||||
it('does not abort an agent chat that outlives the CRUD budget', async () => {
|
||||
const base = await startUpstream((a) => {
|
||||
a.post('/api/v1/agents/:name/chat', async () => {
|
||||
// Longer than the (deliberately tiny) CRUD budget below. Before the
|
||||
// fix this inherited that budget and 503'd.
|
||||
await new Promise((r) => setTimeout(r, 250));
|
||||
return { answer: 'pong' };
|
||||
});
|
||||
});
|
||||
// CRUD budget of 50ms — a chat route must NOT inherit it.
|
||||
const proxy = await startProxy(base, 50);
|
||||
|
||||
const res = await proxy.inject({
|
||||
method: 'POST',
|
||||
url: '/api/v1/agents/reviewer/chat',
|
||||
payload: { message: 'hi' },
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
expect(res.json()).toEqual({ answer: 'pong' });
|
||||
});
|
||||
|
||||
it('still applies the short budget to ordinary CRUD routes', async () => {
|
||||
const base = await startUpstream((a) => {
|
||||
a.get('/api/v1/servers', async () => {
|
||||
await new Promise((r) => setTimeout(r, 300));
|
||||
return [];
|
||||
});
|
||||
});
|
||||
const proxy = await startProxy(base, 50);
|
||||
|
||||
const res = await proxy.inject({ method: 'GET', url: '/api/v1/servers' });
|
||||
// Times out — and is now reported honestly as a timeout, not a connection fault.
|
||||
expect(res.statusCode).toBe(504);
|
||||
expect(res.json().error).toBe('upstream_timeout');
|
||||
});
|
||||
|
||||
it('reports a timeout as 504, never as "cannot reach mcpd"', async () => {
|
||||
const base = await startUpstream((a) => {
|
||||
a.get('/api/v1/servers', async () => {
|
||||
await new Promise((r) => setTimeout(r, 300));
|
||||
return [];
|
||||
});
|
||||
});
|
||||
const proxy = await startProxy(base, 50);
|
||||
|
||||
const res = await proxy.inject({ method: 'GET', url: '/api/v1/servers' });
|
||||
const body = res.json();
|
||||
expect(body.message).toMatch(/did not respond within/);
|
||||
expect(body.message).not.toMatch(/Cannot reach mcpd/);
|
||||
expect(body.message).toMatch(/reachable/);
|
||||
});
|
||||
|
||||
it('streams SSE through instead of buffering it', async () => {
|
||||
const base = await startUpstream((a) => {
|
||||
a.post('/api/v1/agents/:name/chat', async (_req, reply) => {
|
||||
reply.raw.writeHead(200, {
|
||||
'Content-Type': 'text/event-stream',
|
||||
'Cache-Control': 'no-cache',
|
||||
'X-Accel-Buffering': 'no',
|
||||
});
|
||||
reply.raw.write('data: {"type":"text","delta":"po"}\n\n');
|
||||
reply.raw.write('data: {"type":"text","delta":"ng"}\n\n');
|
||||
reply.raw.write('data: [DONE]\n\n');
|
||||
reply.raw.end();
|
||||
return reply;
|
||||
});
|
||||
});
|
||||
const proxy = await startProxy(base, 50);
|
||||
|
||||
const res = await proxy.inject({
|
||||
method: 'POST',
|
||||
url: '/api/v1/agents/reviewer/chat',
|
||||
payload: { message: 'hi', stream: true },
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
// Content-type must survive — a client that gets application/json will not
|
||||
// parse the event stream.
|
||||
expect(res.headers['content-type']).toMatch(/text\/event-stream/);
|
||||
// x-accel-buffering=no must survive too, or intermediaries re-buffer the
|
||||
// stream and reintroduce the stall.
|
||||
expect(res.headers['x-accel-buffering']).toBe('no');
|
||||
expect(res.body).toContain('"delta":"po"');
|
||||
expect(res.body).toContain('"delta":"ng"');
|
||||
expect(res.body).toContain('[DONE]');
|
||||
});
|
||||
|
||||
it('relays a non-200 status from a streaming route', async () => {
|
||||
const base = await startUpstream((a) => {
|
||||
a.post('/api/v1/agents/:name/chat', async (_req, reply) => {
|
||||
return reply.code(404).send({ error: 'Agent not found' });
|
||||
});
|
||||
});
|
||||
const proxy = await startProxy(base, 50);
|
||||
|
||||
const res = await proxy.inject({
|
||||
method: 'POST',
|
||||
url: '/api/v1/agents/ghost/chat',
|
||||
payload: { message: 'hi' },
|
||||
});
|
||||
expect(res.statusCode).toBe(404);
|
||||
expect(res.body).toContain('Agent not found');
|
||||
});
|
||||
|
||||
it('still reports a genuinely unreachable daemon as 503', async () => {
|
||||
// Port 1 is reserved and refuses instantly.
|
||||
const proxy = await startProxy('http://127.0.0.1:1', 500);
|
||||
const res = await proxy.inject({ method: 'GET', url: '/api/v1/servers' });
|
||||
|
||||
expect(res.statusCode).toBe(503);
|
||||
expect(res.json().error).toBe('service_unavailable');
|
||||
});
|
||||
|
||||
it('propagates 401 from a streaming route so login guidance still fires', async () => {
|
||||
const base = await startUpstream((a) => {
|
||||
a.post('/api/v1/agents/:name/chat', async (_req, reply) => reply.code(401).send({}));
|
||||
});
|
||||
const proxy = await startProxy(base, 50);
|
||||
|
||||
const res = await proxy.inject({
|
||||
method: 'POST',
|
||||
url: '/api/v1/agents/reviewer/chat',
|
||||
payload: { message: 'hi' },
|
||||
});
|
||||
expect(res.statusCode).toBe(401);
|
||||
expect(res.json().message).toMatch(/mcpctl login/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('error taxonomy', () => {
|
||||
it('keeps timeout and unreachable as distinct types', () => {
|
||||
const timeout = new UpstreamTimeoutError('http://mcpd', 30_000);
|
||||
expect(timeout).not.toBeInstanceOf(ConnectionError);
|
||||
expect(timeout.timeoutMs).toBe(30_000);
|
||||
expect(timeout.message).toMatch(/did not respond within 30000ms/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user