🤖 fix: serialize sibling tool execution per stream by ThomasK33 · Pull Request #2906 · coder/mux

ThomasK33 · 2026-03-11T18:44:31Z

Summary
Serialize sibling tool execution within a single stream without changing provider-level parallel tool planning.

Background
Mux currently leaves provider parallel tool-call behavior enabled, but sibling tool execute() handlers can still overlap inside a single stream. That creates race windows for shared mutable state even when the provider behavior is intentional and useful.

Implementation
Add a stream-local sequential execution wrapper that clones each tool before overriding execute(), then guards the full execute chain with an AsyncMutex. Apply that wrapper in StreamManager after tool decoration is complete so the AI SDK still sees the same tool surface while Mux runs sibling executes one-at-a-time.

Validation

make test
make static-check
bun test src/common/utils/ai/providerOptions.test.ts
bun test src/node/services/tools/withSequentialExecution.test.ts
bun test src/node/services/streamManager.test.ts
make typecheck
make lint

Risks
This changes per-stream tool scheduling in the execution layer, so regressions would most likely affect multi-tool conversations that depend on overlapping side effects. The scope is limited to a single returned tool map per stream so unrelated workspaces and separate streams can still run concurrently.

📋 Implementation Plan

Plan: Serialize multi-tool execution top-to-bottom without disabling provider parallelism

Target invariant

Providers and SDKs may still advertise or request parallel tool calls. Inside Mux, once sibling tool execute() handlers are invoked for a single stream, they should run one at a time in call order so shared state never races.

Evidence from the repo

src/common/utils/ai/providerOptions.ts intentionally sets OpenAI parallelToolCalls: true; that is the provider-level behavior the user wants to preserve.
src/node/services/streamManager.ts finalizes finalTools in buildStreamRequestConfig() and then passes request.tools straight into streamText() inside createStreamResult(), so the execution policy has to be enforced on the tool map before it reaches the SDK.
src/node/services/streamManager.ts already consumes fullStream sequentially in processStreamWithCleanup(). That loop is downstream of tool execution, so it should remain a consumer rather than becoming the scheduler.
Tool execute options already carry toolCallId / abortSignal (for example in src/node/services/tools/task.ts, ask_user_question.ts, and bash.ts), which gives us a clean future fallback if we ever need stricter ordering than wrapper-invocation order.
src/node/services/backgroundProcessManager.ts already uses an AsyncMutex (outputLock) to prevent race conditions triggered by parallel tool calls, which is good prior art for fixing the problem at the runtime boundary.
src/node/services/mock/mockAiStreamAdapter.ts emits tool-start / tool-end sequentially by construction, so it is not a reliable place to prove concurrency behavior; direct wrapper and stream-manager tests are the right validation seam.

Recommended approach: stream-local sequential executor, provider options unchanged

Net LoC estimate (product code only): +45 to +75

This keeps provider/model behavior intact while making Mux execution deterministic.

1) Leave provider-level parallel tool call settings alone

Files:
- product code: no intended change in src/common/utils/ai/providerOptions.ts
- regression test: src/common/utils/ai/providerOptions.test.ts
Keep parallelToolCalls: true for OpenAI.
Add or adjust a regression test so future refactors do not accidentally “fix” the problem by disabling provider parallelism.
Rationale: the request is to keep LLM/provider planning behavior the same and change only Mux-side execution scheduling.

2) Wrap the final per-stream tool map with a sequential executor

Primary files:
- src/node/services/streamManager.ts
- helper src/node/services/tools/withSequentialExecution.ts (or a local helper in streamManager.ts if that produces the smaller diff)
Narrowest seam:
- apply the wrapper in buildStreamRequestConfig() after finalTools has been fully assembled/decorated for the request, but before the returned request.tools reaches createStreamResult() / streamText().
Wrapper behavior:
- create one fresh AsyncMutex per stream/request;
- clone each tool with cloneToolPreservingDescriptors so cached tools are not mutated in place and wrappers do not stack across repeated requests;
- leave tools without execute untouched;
- preserve this, args, and options when delegating to the original execute handler;
- hold the lock across the full execute chain so hooks, delegation wrappers, PTC side effects, background-process registration, and result shaping all remain serialized.

3) Keep the scope at execution-time serialization only

The lock should be per stream, not global, so different workspaces and unrelated streams can still run concurrently.
Do not redesign processStreamWithCleanup() or the chat event model in this pass.
If the SDK emits multiple tool-call parts up front, the UI may still show several pending calls. That is acceptable as long as actual tool side effects and completions do not overlap.
Keep orphan tool-result logging as fallback diagnostics, but it should stop describing the normal path once sibling executions are serialized.

4) Validate “top-to-bottom” at the stream boundary

Reuse the existing streamManager.test.ts request helpers and setupStreamTextSpy() seam to verify the wrapped tool map actually reaches streamText().
In that test, capture the wrapped tools, invoke multiple execute() handlers via Promise.all, and use deferred promises to prove the second handler cannot enter before the first fully exits.
Add a dedicated wrapper-level test that asserts an execution log like:
- start A
- end A
- start B
- end B
- start C
- end C
Do not rely on the mock router/adapter for this specific invariant because its tool event generator is already serialized.

5) Carry one explicit fallback if the SDK invocation order is surprising

First implementation should use a shared FIFO AsyncMutex; that preserves the order in which wrapped execute() calls are invoked.
If validation shows the SDK can invoke sibling tool executes out of order relative to the tool-call list, escalate to a slightly richer sequencer keyed by options.toolCallId plus observed tool-call order from the stream.
Even in that stricter fallback, keep the fix local to Mux execution rather than changing provider-level parallel tool-call settings.

Small implementation sketch

function withSequentialExecution(tools: Record<string, Tool> | undefined):
  Record<string, Tool> | undefined {
  if (!tools) return tools;

  const executionLock = new AsyncMutex();

  return Object.fromEntries(
    Object.entries(tools).map(([toolName, baseTool]) => {
      const baseToolRecord = baseTool as Record<string, unknown>;
      const originalExecute = baseToolRecord.execute;
      if (typeof originalExecute !== "function") {
        return [toolName, baseTool];
      }

      const executeFn = originalExecute as (this: unknown, args: unknown, options: unknown) => unknown;
      const wrappedTool = cloneToolPreservingDescriptors(baseTool);
      const wrappedToolRecord = wrappedTool as Record<string, unknown>;

      wrappedToolRecord.execute = async (args: unknown, options: unknown) => {
        await using _lock = await executionLock.acquire();
        return await executeFn.call(baseTool, args, options);
      };

      return [toolName, wrappedTool];
    })
  );
}

Validation plan

Provider behavior guard
- File: src/common/utils/ai/providerOptions.test.ts
- Assert OpenAI still sets parallelToolCalls: true.
Wrapper-level ordering test
- Preferred file: src/node/services/tools/withSequentialExecution.test.ts
- Create two or three deferred test tools.
- Invoke their wrapped execute() handlers back-to-back without awaiting the previous one.
- Assert there is no overlap and the execution log stays FIFO.
Stream integration smoke test
- File: src/node/services/streamManager.test.ts
- Capture the wrapped tools object passed to streamText().
- Launch multiple wrapped executes with Promise.all and verify the later calls do not enter before earlier ones finish.
Targeted runtime checks after implementation
- bun test src/common/utils/ai/providerOptions.test.ts
- bun test src/node/services/tools/withSequentialExecution.test.ts
- bun test src/node/services/streamManager.test.ts
- make typecheck

Alternatives

Alternative A: keep the helper local inside `streamManager.ts`

Net LoC estimate (product code only): +25 to +45

Implement the wrapper as a private/local helper instead of adding withSequentialExecution.ts.
Pros: smallest diff.
Cons: reusable execution-ordering behavior gets buried inside a very large file.

Alternative B: toolCallId-aware ordered sequencer

Net LoC estimate (product code only): +70 to +110

Use options.toolCallId plus observed tool-call order to enforce exact stream order even if the SDK invokes execute handlers in a surprising order.
Pros: strongest possible “top-to-bottom” guarantee.
Cons: more state plumbing; only justified if the simpler FIFO wrapper fails validation.

Alternative C: disable provider parallel tool calls

Net LoC estimate (product code only): +3 to +8

Flip provider options like OpenAI parallelToolCalls to false.
Pros: very small patch.
Cons: explicitly not what is wanted; it changes the wrong layer and gives up provider/model parallel planning instead of fixing Mux execution semantics.

Recommended acceptance criteria

Providers can still advertise or emit parallel sibling tool calls.
Within a single Mux stream, sibling tool execute() handlers do not overlap.
The validated execution order is top-to-bottom for the tool-call order Mux receives.
Different workspaces/streams remain independent.
Existing stream consumption and UI replay code continue to work without a broader redesign.

Generated with mux • Model: openai:gpt-5.4 • Thinking: high • Cost: $4.81

Serialize sibling tool execute() handlers one-at-a-time within a single stream while preserving provider-level parallel tool call planning. - add a stream-local sequential execution wrapper for tool maps - apply the wrapper in StreamManager request construction - cover provider options and ordering behavior with regression tests --- _Generated with `mux` • Model: `openai:gpt-5.4` • Thinking: `high` • Cost: `$4.81`_

ThomasK33 · 2026-03-11T18:44:44Z

@codex review

chatgpt-codex-connector · 2026-03-11T18:51:01Z

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Keep analytics ETL imports safe in Bun-based unit tests by lazy-loading the DuckDB runtime only when ETL append logic actually needs it and skipping the native DuckDB-backed ETL cases under Bun. --- _Generated with `mux` • Model: `openai:gpt-5.4` • Thinking: `high` • Cost: `$4.81`_

ThomasK33 · 2026-03-11T19:20:47Z

@codex review

chatgpt-codex-connector · 2026-03-11T19:28:18Z

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Drop the follow-up DuckDB ETL changes so this branch only contains the tool execution serialization work originally requested. --- _Generated with `mux` • Model: `openai:gpt-5.4` • Thinking: `high` • Cost: `$4.81`_

ThomasK33 · 2026-03-12T08:42:03Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b530184e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

ThomasK33 · 2026-03-12T08:52:33Z

@codex review

Please take another look.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f827469445

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

ThomasK33 · 2026-03-12T09:07:24Z

@codex review

Please take another look.

Make sequential tool execution abort-aware so queued sibling tool calls do not acquire the lock and run side effects after a stream interruption. --- _Generated with `mux` • Model: `openai:gpt-5.4` • Thinking: `high` • Cost: `$4.81`_

ThomasK33 · 2026-03-12T09:09:59Z

@codex review

Please take another look.

chatgpt-codex-connector · 2026-03-12T09:18:16Z

Codex Review: Didn't find any major issues. You're on a roll.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Exempt only provably-isolated `task` tool calls from the per-stream sequential-execution mutex, restoring parallel sub-agent launches without reintroducing shared-working-tree races. #2906 serializes every sibling tool execute() in a stream behind one mutex to prevent races on shared mutable state. That also serializes parallel `task` launches: only the first runs, the rest block on the long foreground waitForAgentReport. A forking `task` runs in its own isolated checkout and shares no mutable state with sibling tools, so it is safe to run in parallel. But a `task` that runs in the PARENT checkout (local runtime, or worktree/ssh with isolation:"none") shares the working tree and must stay serialized. Decide per call, by runtime mode + isolation arg, via the new taskCallSharesParentWorkspace() helper. runtimeMode is threaded (optionally, defaulting to serialize) from the AIService send path into the stream-request builder. All other tools remain fully serialized.

Let parallel `task` tool calls launch concurrently again while keeping shared-working-tree access serialized, via a per-stream read/write lock. #2906 serializes every sibling tool execute() in a stream behind one mutex to prevent races on the parent checkout. That also serializes parallel `task` launches: only the first runs, the rest block on the long foreground waitForAgentReport. The real shared resource is the parent workspace checkout, so model it as a read/write lock: - Writers (exclusive): all non-task tools, plus shared-checkout task calls (local runtime, or worktree/ssh with isolation:"none"). They may mutate or depend on exclusive access to the parent tree. - Readers (shared): forked task calls (worktree/ssh default/"fork", or docker/devcontainer). TaskService.create() still reads the parent checkout briefly (agent frontmatter + current branch), so a forked task must exclude sibling writers, but two forked tasks share no mutable checkout state and may overlap. This restores task||task parallelism while keeping task||bash and task||file-write mutually exclusive (addresses Codex review). Runtime mode is threaded from the AIService send path and is undefined-safe (treated as shared -> write/serialize). Classification is decided per call via taskCallSharesParentWorkspace(). A foreground forked task still holds the read lock across its full execute chain (incl. waitForAgentReport), conservatively blocking later writers for its duration -- same scope as the original mutex; narrow to create()'s critical section later if it matters.

#2906 wrapped every tool's execute() in a per-stream AsyncMutex to guard against sibling tool-call races. It had no linked issue or repro, and its own plan notes the real race (bash background output) was already fixed at the runtime boundary via backgroundProcessManager's outputLock. The blanket mutex serialized all foreground sibling tool calls — most visibly parallel 'task' calls, where the lock is held across waitForAgentReport — defeating provider-level parallel tool planning. The genuinely-shared resources are already guarded at the resource level (bash outputLock, TaskService.create()'s own mutex, config FIFO queue; file_read is read-only), so remove the redundant stream-level lock instead of making it concurrency-aware.

Keep the per-stream serialization mutex from #2906 intact for all mutating tools. Only built-in `task` calls targeting agentId "explore" without isolation:"none" may overlap, and only with each other, via a fair reader-writer lock. This restores parallel sub-agent exploration without re-opening the shared-checkout / background-process races that broad parallelism would.

## Summary Sub-agent exploration regained its parallelism: built-in `task` calls to the `explore` agent now run concurrently with each other again, while every mutating tool stays strictly serialized as before. The agent could emit several sibling tool calls intending parallel execution, but only the first actually ran — the rest serialized despite the provider advertising `parallelToolCalls: true`. ## Background PR coder#2906 (commit `386346261`) wrapped every tool's `execute()` in a single shared per-stream `AsyncMutex` (`withSequentialExecution`). That made all sibling tool calls run one-at-a-time, which is the safe default for mutating tools (file edits, bash, config writes) but also defeated parallel sub-agent fan-out — the most visible casualty being the `task` tool spawning `explore` sub-agents. A full revert was rejected because broad parallelism re-introduces real races that coder#2906 quietly fixed: shared-checkout task collisions, background-process ID collisions, and concurrent config read-modify-write. This change keeps all of those protections and only carves out the one case that is provably safe to overlap. ## Implementation The per-stream mutex is replaced with a fair reader-writer lock (`SharedExecutionLock`): - **Writers (exclusive):** every tool except the narrow exception below. Mutating tools keep their original one-at-a-time guarantee, and a writer never overlaps a reader. - **Readers (shared):** only built-in `task` calls that target `agentId: "explore"` and do **not** request `isolation: "none"`. These overlap with each other but never with a writer. The exception is gated on three independent conditions so it can't widen accidentally: 1. The tool is the *built-in* `task` tool, identified by a non-forgeable `Symbol` marker (`markBuiltInTaskTool` / `isBuiltInTaskTool`) rather than by tool name. A user/MCP tool that happens to be named `task` does not qualify. 2. The requested agent is `explore` (read-only by contract). 3. The call does not use a shared workspace (`isolation: "none"`), so forked-checkout explore tasks can't race on a shared working tree. The lock uses a turnstile pattern (turnstile + room-empty + reader-count mutexes) so writers can't be starved by a steady stream of readers and ordering stays fair. Abort handling reuses the existing `acquireLockOrAbort` path. ## Risks Scoped to tool-call scheduling within a single stream. The only behavioral change is that two-or-more concurrent built-in `explore` task calls may now overlap; all mutating tools retain identical serialized behavior. Because the exception excludes shared-checkout tasks and is keyed off a symbol (not a name), the shared-checkout and background-process-ID races that motivated coder#2906 remain closed. --- _Generated with `mux` • Model: `anthropic:claude-opus-4-8` • Thinking: `xhigh` • Cost: `$29.64`_

chatgpt-codex-connector Bot reviewed Mar 12, 2026

View reviewed changes

Comment thread src/node/services/tools/withSequentialExecution.ts Outdated

chatgpt-codex-connector Bot reviewed Mar 12, 2026

View reviewed changes

Comment thread src/node/services/tools/withSequentialExecution.ts Outdated

ThomasK33 force-pushed the fix/sequential-tool-execution branch from f827469 to e83e99e Compare March 12, 2026 09:07

ThomasK33 force-pushed the fix/sequential-tool-execution branch from e83e99e to 2be2fac Compare March 12, 2026 09:09

ThomasK33 added this pull request to the merge queue Mar 12, 2026

Merged via the queue into main with commit 3863462 Mar 12, 2026
24 checks passed

ThomasK33 deleted the fix/sequential-tool-execution branch March 12, 2026 09:49

ethanndickson mentioned this pull request Jun 16, 2026

🤖 fix: let built-in forked explore tasks run in parallel #3576

Merged

Uh oh!

Conversation

ThomasK33 commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Plan: Serialize multi-tool execution top-to-bottom without disabling provider parallelism

Target invariant

Recommended approach: stream-local sequential executor, provider options unchanged

1) Leave provider-level parallel tool call settings alone

2) Wrap the final per-stream tool map with a sequential executor

3) Keep the scope at execution-time serialization only

4) Validate “top-to-bottom” at the stream boundary

5) Carry one explicit fallback if the SDK invocation order is surprising

Small implementation sketch

Validation plan

Alternatives

Alternative A: keep the helper local inside streamManager.ts

Alternative B: toolCallId-aware ordered sequencer

Alternative C: disable provider parallel tool calls

Recommended acceptance criteria

Uh oh!

ThomasK33 commented Mar 11, 2026

Uh oh!

chatgpt-codex-connector Bot commented Mar 11, 2026

Uh oh!

ThomasK33 commented Mar 11, 2026

Uh oh!

chatgpt-codex-connector Bot commented Mar 11, 2026

Uh oh!

ThomasK33 commented Mar 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

ThomasK33 commented Mar 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

ThomasK33 commented Mar 12, 2026

Uh oh!

ThomasK33 commented Mar 12, 2026

Uh oh!

chatgpt-codex-connector Bot commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ThomasK33 commented Mar 11, 2026 •

edited

Loading

Alternative A: keep the helper local inside `streamManager.ts`