Skip to content

fix(orchestration): collapse a duplicate create instead of spawning a second child - #1069

Merged
Juliusolsson05 merged 2 commits into
mainfrom
fix/orchestration-duplicate-create
Sep 20, 2026
Merged

Juliusolsson05 merged 2 commits into
mainfrom
fix/orchestration-duplicate-create

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

Fixes #952.

The incident

On 2026-09-12 one orchestration_create_agent call produced two children 0.9 s apart — same title, cwd, role and prompt, both bootstrapped, both working in the same worktree and writing the same report path. Only one was returned, so the parent could neither see nor close the other and it ran unobserved.

What was ruled out, and what was not

  • request / dispatchRendererRequest send once. There is no retry path.
  • The only timeout is 30 s. The observed gap was 0.9 s.
  • The renderer request is addressed to one window, never broadcast — a decision made in f7b4408a (Refs feat(window): support multiple application windows for multi-monitor workspaces #688), because answering an MCP mutation from the wrong window's workspace model is worse than not answering.
  • Not a double renderer handling either: a second resolveOrchestrationRequest for an already-resolved requestId is dropped, so the extra child would carry no prompt-delivery record and no delivered bootstrap prompt — and both children were bootstrapped.

That leaves a second complete tool invocation, which points above this process. Nobody observed it, so this PR does not claim a root cause. It makes the operation idempotent, because the damage is real whatever the cause.

What it does

orchestration_create_agent runs through bridge.createAgentCallOnce(key, …). An identical call arriving while one is still running awaits it and returns its result.

The unit is the whole tool call, not createAgent. One invocation is create → deliver the bootstrap prompt → mark delivered, and the delivery is the slow, failure-prone part. It lives on the bridge because BuiltInMcpHttpHost builds a fresh server with a stateless transport per POST — a map owned by the server would never see the duplicate — while the key is built by the tool handler, which owns the argument schema.

The key is every argument, prompt included. Two creates that differ by prompt are two different jobs. It is typed Record<keyof OrchestrationCreateAgentArgs, unknown> so the next field added to the tool is a compile error rather than a silent over-collapse, builtInMcpDomains is sorted, and [] normalizes with absent.

In flight only. A window over completed calls would swallow a deliberate "run that exact task again".

What the first version got wrong

The review on this PR caught a regression worse than the bug. Keyed on the child's shape with the prompt excluded, a concurrent fan-out of N workers differing only by prompt collapsed into one child — N-1 tasks silently never run, the losing callers handed prompt_delivery_failed with disposition: 'retry-same-session', an instruction to re-send the prompt into the child already running it, plus a false incident in the always-on journal. MAX_ACTIVE_RENDERER_REQUESTS = 1 makes such a burst overlap by construction, so a fan-out was the most exposed shape.

Five of the eight key fields were also unconstrained by the tests — dropping cwd from the key would have handed a caller a child working in the wrong repository, with the suite green.

Tests

orchestrationDuplicateCreate.system.test.ts drives the real MCP server, the real tool handler and the real bridge over an in-memory transport. Only the two true edges are faked: sendToWindow (no renderer here) and deliverPromptToAgent — and that fake reproduces SessionManager's real in-flight reservation, because a duplicate colliding with it is one of the failures being fixed.

mutation result
remove the dedupe (the bug) 4 failed
key drops prompt (the fan-out) 2 failed
key drops cwd / providerRuntime / runId 1 failed each
key drops role / title / kind 1 failed each
key drops builtInMcpDomains 1 failed
domains not sorted 1 failed
parentSessionId out of the key 1 failed
never clear the in-flight entry 2 failed
dedupe the create instead of the call 2 failed
restored 16 passed

npx tsc -b clean; src/main/orchestration/ + src/mcp/ 48 passed.

🤖 Generated with Claude Code

Juliusolsson05 and others added 2 commits September 20, 2026 02:59
… second child

On 2026-09-12 one `orchestration_create_agent` call produced TWO children
0.9 s apart — same title, cwd, role and prompt, both bootstrapped, both
working in the same worktree and writing the same report path. Only one was
returned, so the parent could neither see nor close the other and it ran
unobserved.

WHAT THIS IS NOT: our retry. `request` and `dispatchRendererRequest` send
once; the only timeout is 30 s, which does not match a 0.9 s gap; and the
renderer request is addressed to ONE window, broadcasting having been
considered and rejected there in #992 for exactly this failure mode. Both
children were bootstrapped, which means two COMPLETE tool invocations ran —
the duplicate arrived from the MCP client, above anything this codebase
controls, and it is not reproducible from here.

So this does not claim a root cause it cannot prove. It makes the operation
idempotent, because the damage is real whatever the cause: duplicate paid
agent work, two writers racing on the same files, and side effects like
commits or pushes performed twice.

WHY IN-FLIGHT ONLY, rather than a time window over completed creates: two
identical creates overlapping in flight is never something a caller intends —
nobody needs a second identical worker started before the first has finished
starting. A completed-create window would silently collapse a deliberate
"spawn another one just like that", which is a legitimate request. This
refuses to guess about anything the caller could plausibly mean.

The duplicate receives the first call's child rather than an error: the caller
asked for a child and there is one, so failing would turn a duplicate delivery
into a visible failure for work that succeeded.

One test in this file needed care: the bridge already serialises renderer
requests, so "only one request was sent" is true WITHOUT the dedupe and would
pass for the wrong reason. The assertion is made after the first create
settles, which is when an undeduped second would dispatch off the queue.

Mutation-checked: removing the dedupe fails 1, dropping the title from the key
fails 1, never clearing the in-flight entry fails 2.

Fixes #952

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings on #1069. The first version was worse than the bug it fixed.

FINDING 1 (critical): it collapsed a deliberate FAN-OUT.
The key was the child's SHAPE and deliberately left the prompt out, on the
reasoning that the prompt is delivered after the child exists. Every field but
`kind` is optional, so N concurrent workers differing only by prompt shared one
key: one child was created, N-1 tasks were silently never run, and the callers
that lost were told `prompt_delivery_failed`. That is the normal way this tool
is used — it is what "two reviewers per PR" does — and
`MAX_ACTIVE_RENDERER_REQUESTS = 1` makes such a burst overlap in flight by
construction, so a fan-out was the MOST exposed shape, not the least. The
rationale was also just wrong: `args.prompt` is in hand three lines above the
`createAgent` call and simply was not passed down.

Two creates that differ by prompt are two different jobs. The prompt is in the
key now.

FINDING 2 (high): the deduped duplicate got a visible FAILURE.
The comment claimed the duplicate receives the first call's child rather than
an error. For a create WITH a prompt — the incident's path — that was false:
both callers resumed with the same session id, the winner reserved the
delivery, and the duplicate hit SessionManager's in-flight guard and received
`prompt_delivery_failed` with `retrySafe: true,
disposition: 'retry-same-session'` — an instruction to re-send the prompt into
the child already running it — plus a false incident in the always-on journal.

FINDING 4 (medium): the window covered a fraction of the call.
One invocation is create -> deliver the bootstrap prompt -> mark delivered, and
the delivery is the slow, failure-prone part. Deduping only the create left a
duplicate arriving during delivery free to spawn a second child.

All three have one fix: dedupe the WHOLE TOOL CALL. `createAgentCallOnce` lives
on the bridge because `BuiltInMcpHttpHost` builds a fresh server with a
stateless transport for every POST — a map owned by the server would never see
the duplicate — but the key is built by the tool handler, which owns the
argument schema.

FINDING 3 (high, tests): five of the eight key fields were unconstrained.
`cwd`, `providerRuntime`, `runId`, `role` and `builtInMcpDomains` could all be
dropped from the key with the suite still green; dropping `cwd` would have
handed a caller a child working in the wrong repository. The new suite drives
the REAL MCP server, the REAL tool handler and the REAL bridge over an
in-memory transport, faking only the two true edges (`sendToWindow`, and a
`deliverPromptToAgent` that reproduces the real in-flight reservation). Every
field is now pinned by a mutation.

FINDING 5 (medium): a false WHY comment. The one-window decision was attributed
to #992, which is the unified-workspace-layout issue and says nothing about
orchestration. It comes from `f7b4408a` (Refs #688), and its reason is that
answering an MCP mutation from the wrong window's workspace model is worse than
not answering. The comment also asserted the duplicate "arrived from the MCP
client" as fact; it now says what was ruled out and stops there.

FINDING 6: the key is typed `Record<keyof OrchestrationCreateAgentArgs, unknown>`
against a named schema constant, so the next field added to the tool is a
compile error here instead of a silent over-collapse.

FINDING 7: `builtInMcpDomains` is sorted and `[]` normalizes with absent, so
the same request spelled two ways is one call.

FINDING 8: the headline test no longer passes by timing out; the fake renderer
answers every request, so an undeduped duplicate fails on a real assertion.

Mutation-checked, 16 tests:

  remove the dedupe (the bug)          4 failed
  key drops prompt (the fan-out)       2 failed
  key drops cwd / runtime / runId      1 failed each
  key drops role / title / kind        1 failed each
  key drops builtInMcpDomains          1 failed
  domains not sorted                   1 failed
  parentSessionId out of the key       1 failed
  never clear the in-flight entry      2 failed
  dedupe the create, not the call      2 failed
  restored                            16 passed

Fixes #952

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review round addressed. Findings 1, 2 and 4 turned out to be one fix, and finding 1 was right that the first version was worse than the bug.

The fan-out collapse was the real defect here. Keying on the child's shape with the prompt excluded meant N concurrent workers differing only by prompt became one child, and N-1 tasks were silently never run — which is exactly how I dispatch reviewers on this repo. Your rationale rebuttal was also correct: args.prompt is in hand three lines above the createAgent call and simply was not passed down. The prompt is in the key now.

2 and 4 fall out of moving the unit to the whole tool call. A duplicate now awaits the first invocation and receives its result, so there is no second deliverPromptToAgent, no prompt_delivery_failed carrying retry-same-session for a child that is already running the prompt, no false journal incident, and no window that ends before the slowest segment. It went on the bridge for the reason you identified — BuiltInMcpHttpHost builds a fresh server per POST, so a server-owned map would never see the duplicate.

3 — your mutation table was the most useful thing in the review. All five surviving fields are pinned now, through the real server and the real handler rather than the bridge in isolation:

mutation before after
key drops cwd 14 passed 1 failed
key drops providerRuntime 14 passed 1 failed
key drops runId 14 passed 1 failed
key drops role 14 passed 1 failed
key drops builtInMcpDomains 14 passed 1 failed

Plus prompt (2 failed), parentSessionId, unsorted domains, and "dedupe the create instead of the call" (2 failed).

5 — the #992 attribution was wrong and I have replaced it with f7b4408a (Refs #688) and its actual reason. The "arrived from the MCP client" line now states what was ruled out and stops, including the double-renderer-handling argument you verified.

6 — the key is Record<keyof OrchestrationCreateAgentArgs, unknown> over a named schema constant. 7 — domains sorted, [] normalized with absent. 8 — the fake renderer answers every request type, so an undeduped duplicate fails on an assertion instead of hanging for 5 s.

Fixes #952 kept: the window now covers the entire invocation rather than a fraction of it, which is the coverage the body previously implied without showing.

@Juliusolsson05
Juliusolsson05 merged commit de55efb into main Sep 20, 2026
2 checks passed
@Juliusolsson05
Juliusolsson05 deleted the fix/orchestration-duplicate-create branch September 20, 2026 10:50
Juliusolsson05 added a commit that referenced this pull request Sep 20, 2026
Four PRs merged (#1068, #1069, #1071, #1072), closing #1025, #952 and #930.
#1073 and #1074 are open with their review findings fixed. #993 stays open
deliberately: the mechanism does not reproduce the reported snapshot.

Records the two reviewer instructions that produced every blocking finding —
mutate the implementation and report the survivors, and count the real corpus
rather than reasoning about it — because three of the four blockers were
regressions this loop had just introduced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(orchestration): one orchestration_create_agent call can spawn two children

1 participant