fix(mcp): fan_out answers with a fan_out_id, and a batch can be polled (#2670) - #2679
Conversation
…ch failed` (#2661) The #914 gateway-timeout receipt covered the sequential /chat route only. `chat_with_agent(parallel=true, async=false)` dispatches through `client.ts::task()`, which held the backend fetch for `timeout_seconds + 60` (up to 7260s). The MCP client's own gateway timeout kills the JSON-RPC call long before that, so the caller saw a bare `fetch failed` while the target kept running — no receipt, no execution_id, nothing to poll. Fleet incident 2026-09-08: every duplicate dispatch in a three-hop cascade was a re-send after "could not confirm delivery" on this path. The #914 matcher could NOT simply be mirrored. /chat is queue-serialised, so "newest non-terminal MCP row wins" is near-unambiguous there. /task exists to run N tasks concurrently, and every filter that rule applies — triggered_by, source_mcp_key_id, the recency window — is IDENTICAL across one caller's concurrent tasks. Mirroring it would have handed caller A the execution_id of caller B's task; A then polls and acts on a well-formed FOREIGN result. Silent wrong data is worse than the loud error it replaces. Attribution is now provable or absent: - read the execution_id out of an idempotency 409 instead of collapsing it into an opaque `API error (409)` — an exact key->execution mapping the backend already sends (RELIABILITY-006) - match the call's own `message`, not just key + trigger - return NOTHING when more than one candidate survives; ambiguity yields no receipt, which is exactly the pre-#914 behaviour, so refusing to guess is never worse than before - recover on AbortError only — a TypeError may mean the request never landed, and on this route a concurrent peer row is the normal state Deliberately NOT done: re-POSTing the dispatch with the same key as a "probe". `idempotency_service.begin()` fails open, so on that path the probe would dispatch a second execution — precisely the bug being fixed. Three latent defects in the already-shipped /chat route, fixed alongside: - The recency window was a fixed 30s while the abort fires at MCP_CHAT_TIMEOUT_MS. Raising that documented operator knob to >=30s put every candidate row outside the window, silently degrading every receipt on both routes to the no-match throw — the knob disabled the feature it was meant to tune. The window is now derived (timeout + 10s). - The recovery lookup ran through `_fetch`, which has no AbortController and re-authenticates once on 401. We abort at 25s precisely BECAUSE the gateway ceiling is close; an unbounded lookup spends what is left of it and, when the backend is the slow party (the usual reason we aborted), reproduces the very `fetch failed` this feature exists to prevent. Now bounded by MCP_RECOVERY_TIMEOUT_MS with no retry, and reads 50 rows rather than 10. - The trigger allowlist had to become per-call-site. Widening the shared constant with `self_task` would have let a /chat abort attribute a concurrently-running parallel self-task row — /task is unqueued, so such a row can be RUNNING while a /chat sits queued. Backend counterpart: a failed/cancelled/timed-out sync /task left its idempotency claim in_flight for the full 24h TTL, because `_map_task_failure` raises between `begin()` and `complete()` and nothing covered that path. A legitimate retry then answered 409 for a day against a task dead for minutes, and REWORDING was the only way through — which derives a different key and dispatches a genuine duplicate. The wedge did not just block retries, it selected for the duplicate-dispatch behaviour #2661 exists to stop. `idem` is keyword-only and required so a third sync branch cannot silently reintroduce it. Also ships `.github/workflows/mcp-server-test.yml`. Nothing ran this package's suite: helper-mcp-test.yml covers src/helper-mcp only, and container-security filters on the Dockerfile, not src/**. So 341 tests across 28 files gated nothing, and `npm run build` — this package's only typecheck — first ran when deploy-dev built the image ON dev, breaking the deploy instead of the PR. Verified live against a running stack, not just in unit tests: - abort at 400ms on /task returned a real receipt; polling its execution_id resolved, with triggered_by=mcp, the exact message, and the real source_mcp_key_id — so the key filter genuinely engaged. The harness previously passed a fabricated keyId that could never match a row, making a green run meaningless; fixed here. - wedge before/after: without the fix a second send after a failure returned 409 request_in_progress against an already-dead task; with it, a fresh dispatch. - both new backend guards mutation-verified (remove the release -> 6 fail; drop idem= from a call site -> the AST guard fails). fan_out is the third route of this class and stays open (#2670) — it needs a fan_out_id receipt and no polling surface resolves one. The read-only idempotency lookup that would retire this heuristic on every route is #2671. Fixes #2661 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PmVdJbnrh1kZJjuYHQYNCE
- a selection heuristic ported to a route with different concurrency turns "no answer" into a confident wrong one; ambiguity must return nothing, and the identity the system already records beats elimination - a documented operator knob whose dependent constant is hardcoded is a silent kill-switch: raising MCP_CHAT_TIMEOUT_MS as documented disabled every receipt - a failure path between begin() and complete() wedges the idempotency claim, and the reword that gets a caller past the 409 is what creates a real duplicate — fix the choke point, guard the call sites with AST Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PmVdJbnrh1kZJjuYHQYNCE
#2670) The third and last route of the #914 class, and the one that hits it most reliably: a fan-out dispatches N tasks and by construction runs longer than any single one of them, so it exceeds the gateway ceiling more often than the two routes already fixed. `client.ts::fanOut()` carried the identical unbounded `(timeout_seconds ?? 7200) + 60`, so the MCP client's own 30-60s timeout killed the JSON-RPC call and the caller saw a bare `fetch failed` while every dispatched task kept running — with nothing to poll. **The receipt names a `fan_out_id`, not an `execution_id`.** A batch is N rows sharing one id, so a single execution id could only ever name an arbitrary member of it. `{status: "fan_out_timeout", agent, fan_out_id, execution_ids, task_count, message}`; `execution_ids` is evidence, not a manifest, since a slot-starved subtask has no row yet. **Ambiguity is redefined, not reused.** #2661 refuses when more than one ROW survives its filters, because on `/task` it cannot tell which row is the caller's. A fan-out stamps one `fan_out_id` on all N of its rows, so finding ANY row finds the batch and N survivors is the expected shape. The unit that must be unambiguous is the BATCH: `pickRecentFanOut` returns nothing when more than one distinct `fan_out_id` survives. Same rule ("a wrong id is worse than none"), measured on the right thing. **Status is deliberately not filtered.** `/chat` and `/task` require a non-terminal row, because a terminal one is evidence the receipt is unnecessary. By abort time a fan-out is normally a mix — some subtasks finished, others running — so requiring non-terminal rows would drop exactly the batches furthest along. The derived recency window (#2661's rule, `timeout + 10s`, never a fixed constant) is what bounds staleness. **The polling surface reads rows, not the idempotency snapshot.** `GET /api/agents/{name}/fan-out/{fan_out_id}` folds `schedule_executions` — where `fan_out_id` has been stamped on every subtask since FANOUT-001 — into `{status, total, completed, failed, running, results[]}`. `routers/fan_out.py` does store the whole aggregate under the call's key, which looks like a free receipt, but `complete()` runs only once the batch has FINISHED, so the snapshot cannot answer what a timed-out caller is actually asking: what is happening right now. That is also why this does not collapse into #2671. Batch status is `running` while any subtask can still change, then `completed` / `partial` / `failed`. `partial` exists because best-effort is the fan-out's default policy. `deadline_exceeded` is absent by construction — it is the dispatcher's verdict on its own outer deadline, not a property of any row. Per-task status is the EXECUTION status verbatim, so a subtask waiting for a slot is distinguishable from one that ran; the dispatch response's two-value `completed`/`failed` pair would report a healthy queued subtask as a failure. Enumeration-safe: malformed, unknown and belonging-to-another-agent are one 404. **Two backend contracts the client depends on.** The in-flight 409 now returns the same `{error, message, execution_id}` shape `/chat` and `/task` do — it was a bare string, so `fan_out` could not benefit from the #2661 client that reads that field — and the batch id is attached to the idempotency claim WHEN IT IS MINTED (`FanOutService.execute(on_started=…)`) rather than at `complete()`. Attaching at the end records it exactly when nobody needs it any more: the window in which a duplicate arrives, and in which this call's gateway gives up, is the whole run. The hook is best-effort — bookkeeping must not fail a dispatch. **MCP surface** (Invariant #13): new `get_fan_out_result` in `tools/executions.ts`, gated to `{self} ∪ permitted` like `get_execution_result` beside it; the `fan_out` description states the receipt and the retry asymmetry (an identical re-send dedupes server-side, a REWORDED one dispatches all N tasks again). `verify_914.ts` grows a `fanout` mode with three tasks, so the harness exercises batch identification rather than a single row. Tests: 13 TS cases on the matcher (N-rows-is-normal, two-batches-refuses, part-finished-still-found, per-call-site trigger set, derived window) and 26 backend cases on the fold, the read surface's enumeration safety, and the two contracts. `354` MCP tests and `15072` backend tests pass; the 15 failures in the full run are DNS-dependent SSRF tests that fail identically on the base branch with this change stashed. Fixes #2670 Follow-up of #2661 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN
|
Self re-review. Nothing further from me. Two things I went back and checked because they are where a silent truncation or a wrong count would hide.
The finding I would keep from building it is the one in the PR body: the issue suggested this might collapse into #2671's idempotency probe, and it does not — Merge after #2675 (#2661) — this reuses its attribution rules, its bounded recovery read and its 409 reader, and drops the "still unfixed" note it added. |
|
Deeper self re-review. No defect found; four things checked rather than assumed, and one inherited assumption worth naming.
The inherited assumption. The receipt says "the batch is still running". That is the same claim #914 and #2661 make on their routes, and it rests on the backend handler surviving the client disconnect — the MCP server aborts its Everything else from the first pass re-read clean. |
Resolve docs/memory/learnings.md: append-only log, both sides kept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht
|
merge-train 2026-09-10 (evening run): not on this train. This branch is stacked on #2675 ( |
…te the client change (#2661 review) The 2026-09-10 merge-train hold on #2675, item by item. Backend — `_dispatch_sync_backlog` had two exits between `begin()` and `complete()` that `_map_task_failure` never saw: - long-poll timeout: no longer raises from the `except`. The row is read first; a terminal the wait missed is reconstructed and settled the normal way (success → complete, failed → release). A row still queued/running completes the claim with a `queued_timeout` RECEIPT (`_queued_payload` shape, the MCP client's own vocabulary) — not released, because a retry would dispatch a second execution beside the live one; not left in_flight, because nothing downstream completes it and a retry after the row terminates would still 409 for a day. - vanished row (503): releases. Nothing is running under this key that anyone can find, so a retry must be allowed to dispatch. Four executed cases plus an AST guard that every `raise ChatDispatchError` in that function is preceded by a `fail()`/`complete()` in its block. MCP client — `task-receipt.test.ts` drives `TrinityClient.task()` and `chat()` against a stubbed `fetch`: the 409 → receipt branch, the abort → executions lookup → receipt path, the ambiguity throw, async_mode and TypeError NOT recovering. Mutation-checked: disabling either branch reds the file. - `chat()` now honours the same 409 rule the tool description promises for every sync route, through a shared `inFlightReplayReceipt` so the two routes cannot drift. - `verify_914.ts`: per-route prompt — in `both` mode the still-running chat row matched the task lookup's exact-message filter, two survivors, refused on ambiguity, exit 1. And `? 0 : 0` is now `? 0 : 1`. - `MCP_RECOVERY_TIMEOUT_MS` wired into all three composes + `.env.example`; `MCP_CHAT_TIMEOUT_MS` added to prod + hosted (it was dev-only). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht
…ed-timeout-receipt # Conflicts: # docs/memory/learnings.md
…2661) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht
…-receipt' into fix/2670-fanout-receipt # Conflicts: # docs/memory/learnings.md
|
Re-merged from #2675 at 🤖 Generated with Claude Code |
…dule (#2679) — mechanical, per the merge-train note on the PR The router is now also covered by executions.ts (get_fan_out_result) — the Invariant #13 header is how /validate-architecture tells "unexposed on purpose" from "forgotten", so it names both. The flow doc's harness line still advertised [chat|task|both]; verify_914.ts takes [chat|task|fanout|all]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018h3Ku8HFzwPfNR75kf1Gxe
|
merge-train 2026-09-11: on this train (after #2675, which it is stacked on), with one mechanical push to your branch — What I pushed and why. Validation of the #2670 delta (not the inherited #2661 material): no criticals, CSO clean, no schema change ( One genuine design gap, non-blocking, worth its own issue rather than a fix on this PR: Cheap follow-ups: nothing executes |
…ed-timeout-receipt # Conflicts: # docs/memory/learnings.md
# Conflicts: # docs/memory/architecture/mcp-server.md # docs/memory/feature-flows/mcp-orchestration.md # docs/memory/requirements/scheduling.md # src/mcp-server/scripts/verify_914.ts # src/mcp-server/src/client.test.ts # src/mcp-server/src/client.ts
|
merge-train: one more push to your branch — the stacked-base re-merge after #2675 squashed onto |
Resolves conflicts with #2679 (#2670) by adopting its shipped GET /api/agents/{name}/fan-out/{fan_out_id} and dropping this branch's duplicate route, get_status and batch_belongs_to. The GET gains an additive task_id from fan_out_task_id. Review fixes (#2524, merge-train 2026-09-09/10): - Create each subtask row at slot grant inside the max_concurrency semaphore instead of up front. Pre-created RUNNING rows waiting behind the semaphore were bulk-FAILed by the #106 no-session sweep; QUEUED rows would be claimed by claim_next_queued while _dispatch_all also dispatched them. The sync caller now waits for the shielded dispatch, then for queued rows to reach a terminal. - Default wait budget covers ceil(N / min(max_concurrency, max_parallel_tasks)) waves instead of one subtask's bound. - Snapshot subscription_id on fan-out rows (SUB-004). - Carry execute_task's error_code into the sync aggregate. - _fail_subtask gates side effects on the CAS and emits through spawn_task_terminal_event. - A failed fan-out DB poll read no longer escapes the wait. - MCP fan_out: async_mode param, corrected deadline wording. - Docs: architecture/execution.md, api-endpoints.md, requirements/scheduling.md 37.4, fan-out flow. - Real-schema test for the fan-out SQL. Renumber the Alembic revision to 0062_execution_fan_out_task_id off 0061_execution_open_canvas (single head). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the third and last route of the #914 gateway-timeout class.
The defect
client.ts::fanOut()carried the identical unbounded ceiling #2661 removed from sync/task:The MCP client's own 30–60s gateway timeout kills the JSON-RPC call long before that, so
fan_outsurfaced a barefetch failedwhile every dispatched task kept running. It is the route that hits this most reliably: a fan-out dispatches N tasks and by construction runs longer than any single one of them.Why the #914/#2661 receipt did not transfer
The id. Those receipts name one
execution_id. A fan-out creates N rows sharing onefan_out_id, so a single execution id could only ever name an arbitrary member of the batch. The receipt is{status: "fan_out_timeout", agent, fan_out_id, execution_ids, task_count, message}— andexecution_idsis evidence, not a manifest, since a slot-starved subtask has no row yet.The ambiguity rule. #2661 refuses when more than one row survives, because on
/taskit cannot tell which row is the caller's. Here N survivors is the expected shape — all of them are the caller's — so the unit that must be unambiguous is the batch:pickRecentFanOutreturns nothing when more than one distinctfan_out_idsurvives. Same rule ("a wrong id is worse than none"), measured on the right thing.The status filter.
/chatand/taskrequire a non-terminal row, because a terminal one is evidence the receipt is unnecessary. By abort time a fan-out is normally a mix — some subtasks finished, others running — so requiring non-terminal rows would drop exactly the batches furthest along. The derived window (timeout + 10s, never a constant — #2661's rule) is what bounds staleness.The polling surface
GET /api/agents/{name}/fan-out/{fan_out_id}foldsschedule_executions, wherefan_out_idhas been stamped on every subtask since FANOUT-001.runningcompletedpartialfailedPer-task status is the execution status verbatim (
queued/running/success/…), not the dispatch response's two-value pair — a live batch has to distinguish "waiting for a slot" from "running", and two values force a healthy queued subtask to be reported as a failure.deadline_exceededis absent by construction: it is the dispatcher's verdict on its own outer deadline, not a property of any row.Enumeration-safe (Invariant #8): malformed, unknown, and belonging-to-another-agent are one uniform 404.
It does not read the idempotency snapshot — and this is why #2670 does not collapse into #2671
The issue notes that
routers/fan_out.py:140already stores the entire aggregate under the call's key, so a read-only(scope, key)probe would givefan_outa receipt "for free". I checked that first, as suggested, and it does not hold:complete()runs only once the batch has finished, so the snapshot cannot answer the question a timed-out caller is actually asking — what is happening right now — which is the state every batch that issues a receipt is in. #2671 remains worth doing and would retire the executions scan; it would not have replaced this read surface.Backend contracts the client depends on
/chatand/task—{error, message, execution_id}instead of a bare string, so the bug(mcp): the sync /task route has no queued_timeout receipt — the MCP gateway kills the held fetch and the caller sees 'fetch failed' while the target keeps running (#914, second route) #2661 client that reads that field can turnAPI error (409)into a pollable receipt.FanOutService.execute(on_started=…), not atcomplete(). Attaching at the end records the id exactly when nobody needs it any more: the window in which a concurrent duplicate arrives, and in which this call's own gateway gives up, is the whole run. Best-effort — bookkeeping must not be able to fail a dispatch that is otherwise fine.Acceptance criteria
fanOut()bounded byMCP_CHAT_TIMEOUT_MSfan_out_id(+ theexecution_ids known at abort){error, execution_id}shape as/chatand/taskfan_outdescription states the contract;mcp-orchestration.mddrops the "still unfixed" note and gains route threeTests
13 TS cases on the matcher: N rows is the normal shape and not ambiguity, two distinct batches refuses, a part-finished batch is still found, rows from other routes and rows with no
fan_out_idare ignored, key scoping, the messages discriminator, the derived window, and the per-call-site trigger set (fan_outis in neither/chat's nor/task's set).26 backend cases: the fold (
runningoutranks every verdict;pending_retryandqueuedare live, not failed;cancelled/skippedare failures, not successes;deadline_exceededunreachable; the fold is pure), the read surface (7 malformed-id shapes are the same 404 as an unknown one and never reach the DB; an unknown batch is 404, not an empty "completed" aggregate; the read is scoped by agent as well as by id), and the two contracts above.354MCP tests,15072backend tests pass. The full-suite run shows 15 failures in DNS-dependent SSRF tests (test_ent399_ipv6_origin,test_mcp_validator::TestCgnatSsrfGuard, …) — these fail identically on the base branch with this change stashed, i.e. sandbox DNS, not this PR.Live harness:
npx tsx src/mcp-server/scripts/verify_914.ts <agent> fanout(three tasks, so it exercises batch identification rather than a single row).Fixes #2670
🤖 Generated with Claude Code
https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN