fix: make transfer_task concurrency-safe under parallel tool calls by bernard-code-lab · Pull Request #4180 · docker/docker-agent

bernard-code-lab · 2026-09-06T20:52:11Z

Problem

When a model issues several transfer_task calls in one API response, Dispatcher.Process runs them in parallel goroutines via concurrent.MapSlice. Each goroutine re-resolved its caller through resolveSessionAgent, which reads the shared agentRouter.current that a sibling's swapCurrentAgent had already mutated:

T1: resolveSessionAgent(sess) -> "orchestrator"  -> validates OK
    runForwarding -> swapCurrentAgent -> current = "drafter"
T2: resolveSessionAgent(sess) -> "drafter"       <- reads the mutated value
    validateAgentInList("drafter", "drafter", ...) -> FAIL

The reported symptom is the loud one:

Agent drafter cannot transfer task to drafter: target agent not in sub-agents list.
No agents are configured in this list.

There is a quieter consequence too. runForwarding only pins the child session when the parent is already pinned, so a foreground delegation's child stays unpinned and loop.go re-resolves its agent from current on every iteration. With the deferred restores interleaving, a child could execute turns with another sibling's identity, toolset and model — no error, wrong output. validateDelegation inherits the same wrong caller, which can also produce a spurious delegation cycle detected.

atomic.Pointer keeps current free of data races, but it cannot make the read → validate → mutate → read sequence serializable.

Fix

Two orthogonal changes.

1. Resolve the caller from the batch snapshot. Dispatcher.Process already resolves the caller exactly once, before the fan-out, and stores it on the call that backs every handler's tools.Runtime. It just was not reachable. callRuntime now exposes it, and LocalRuntime.callerAgent prefers it over session resolution, falling back for hosts that provide no snapshot. No handler signature changes — rt tools.Runtime was already a parameter of ToolHandlerFunc.

2. Let one delegation own the shared current agent. runForwarding claims it with a CAS; a sibling that loses the claim pins its child instead of stomping the swap. This is the same isolation mode background delegations already use.

run_skill shares runForwarding and had the same latent defect when it landed in a batch alongside transfer_task; the same change fixes it.

Behavior change

With a single transfer_task in the batch — the common case — behavior is unchanged: the swap happens, the child stays unpinned, and the existing foreground tests pass untouched.

With N parallel transfers, the 2nd..Nth child is pinned. Pinned children do not take the handoff / force_handoff path (loop.go guards on sess.AgentName == ""). That is a pre-existing property of pinning, now reachable in a new scenario, and it is documented on SwitchCurrentAgent. If you would rather have no variance between the single and parallel cases, the alternative is to pin every child whenever the batch holds more than one delegation — same trade, but it gives up the untouched single-transfer path.

Out of scope

run_background_agent resolves its caller the same way, but agenttool.Handler.HandleRun does not receive a tools.Runtime, so fixing it means changing a signature outside pkg/runtime. Left for a follow-up; its children are already pinned, which bounds the impact.

Testing

Two regression tests, both verified to fail with the source changes reverted:

TestTransferTask_UsesBatchCallerSnapshotNotSharedCurrentAgent — deterministic, reproduces the exact error string from the issue.
TestTransferTask_ConcurrentForegroundTransfersStayIsolated — end-to-end through RunStream → Dispatcher.Process → concurrent.MapSlice, with three transfer_task calls in one assistant response. Without the fix two of the three children return no output and the race detector fires on a shared test stream, which is the cross-wiring made visible: two children had resolved to the same agent. With the fix it is stable over 20 runs under -race.

go build ./...                    Success
go test -race ./pkg/runtime/...   1141 passed
go test ./...                     6003 passed, 90 packages
golangci-lint run                 0 issues
go run ./lint .                   1929 files, no offenses
go mod tidy --diff                clean

task lint / task test were run as their underlying commands, since task is not installed locally.

aheritier · 2026-09-06T21:01:16Z

👋 Some commits in this PR are not signed and verified by GitHub. Please sign your commits with a GPG or SSH key registered in your GitHub account, then force-push.

Commits that are not verified: 7b3dc36

See GitHub's guide on signing commits for setup instructions. I've added status/needs-signed-commits; it will be removed automatically once every commit in this PR carries a valid GitHub-verified signature.

bernard-code-lab · 2026-09-10T23:56:32Z

Friendly follow-up 🙂

The signing issue is sorted out: b6fdcaf is now GitHub-verified, and status/needs-signed-commits has been removed. The branch is still MERGEABLE against main with no conflicts.

Since this PR comes from a fork, no workflows have run yet — they need a maintainer to approve the CI run. @aheritier, since you triaged this one, would you mind kicking off CI and/or assigning a reviewer when you get a chance?

Happy to rebase, split the change, or adjust the approach if anything looks off. Thanks!

aheritier · 2026-09-11T11:57:39Z

Review: request changes

The caller-snapshot portion of this fix is sound and the new regression tests are meaningful. However, the runtime-global agentSwitchInFlight claim introduces a functional regression in sequential nested foreground delegation.

Blocking

pkg/runtime/agent_delegation.go:372: the claim remains held for the full outer delegation lifetime. In a sequential root → lead → worker chain, the nested worker is therefore pinned despite having no concurrent sibling. That suppresses its configured force_handoff (loop.go:1035 only runs it when sess.AgentName == ""). This was verified against the merge base: the base reaches finisher; this PR stops at worker.

Please scope the claim to the contending sibling set (for example, by parent session), or decide pinning from the batch size at dispatch time. This should also make the intended behavior deterministic.

Also needed

The CAS winner is nondeterministic, so the child retaining switch/handoff behavior varies across runs.
Add coverage for nested foreground transfer_task (root → lead → worker).
Required CI has not run on this fork PR: there are no check runs and ci / gate is pending approval.

Local validation was green: go build ./..., go test ./..., go test -race ./pkg/runtime/..., golangci-lint run ./pkg/runtime/..., and go run ./lint ..

Do not submit a formal GitHub review; post only this PR conversation comment.

aheritier · 2026-09-11T13:09:10Z

requires a review of @rumpl or @dgageot who are more experts than me on this topic

When a model issues several transfer_task calls in one response, the dispatcher runs them in parallel goroutines. Each one re-resolved its caller from the shared current agent that a sibling's swapCurrentAgent had already mutated, so the later calls validated the target against the wrong caller and failed with "target agent not in sub-agents list", and a child that did start could run its turns as another sibling's agent. Resolve the caller from the snapshot Dispatcher.Process already takes before the fan-out, exposed on the per-call tools.Runtime handle. For the shared current agent itself, decide from the shape of the batch rather than by claiming it at runtime. Dispatcher.Process tallies the calls per tool before the fan-out, so a delegation knows whether any sibling delegation runs beside it: a solo one swaps the current agent as before, while every member of a multi-call batch pins its child session instead — the isolation mode background delegations already use. Deciding up front avoids two defects an in-flight claim has. A claim held for the outer delegation's lifetime also pins a sequential nested child, which silently suppresses that child's force_handoff, since the runtime only honours it for unpinned sessions. And letting the first arrival win makes the child that keeps switch events vary between runs. The tally is per tool, not per batch: only delegations contend for the current agent, so a transfer_task sharing its batch with an unrelated tool stays solo. run_skill shares runForwarding and had the same latent defect when it landed in a batch alongside transfer_task; it is fixed by the same change. run_background_agent resolves its caller the same way, but agenttool's HandleRun does not receive a tools.Runtime, so that path is left for a follow-up. Its children are already pinned, which bounds the impact. Fixes docker#4156 Signed-off-by: bernard-code-lab <rafaelc869@gmail.com>

handoff read the shared current agent to decide who was handing off. A transfer_task running beside it in the same parallel batch may already have swapped that field to its own target, so the handoff was validated against the transfer target's handoffs list instead of the caller's and was rejected with "No agents are configured in this list" — the same failure docker#4156 reports for transfer_task, on the sibling tool. Resolve the caller from the dispatcher's pre-fan-out snapshot, the handle transfer_task already uses. Hosts without a batch fall back to session resolution, which is what the old lookup did for them anyway. This covers caller resolution only. handleHandoff still calls setCurrentAgent outside the pin/switch machinery, so a handoff sharing a batch with a delegation can still clobber the current agent; unlike transfer_task's call-scoped swap, handoff rewires the session for every later turn, so giving it the same treatment is a routing change rather than a bug fix. Tracked separately. Signed-off-by: bernard-code-lab <rafaelc869@gmail.com>

bernard-code-lab · 2026-09-11T14:16:29Z

Thanks @aheritier — the nested-delegation regression was real, and reproducing it changed the shape of the fix. Force-pushed; the branch is now two commits.

The claim is gone, not rescoped

You offered two options: scope the claim to the contending sibling set, or decide pinning from the batch size at dispatch. Working through the second showed it makes the first unnecessary — so agentSwitchInFlight is deleted rather than re-scoped.

Dispatcher.Process already has the whole batch before the fan-out, so it now tallies calls per tool and passes the count down. A delegation with no sibling delegation swaps the current agent as before; every member of a multi-call batch pins its child. The contention only ever existed inside one batch — handleTaskTransfer is the sole producer of SwitchCurrentAgent — so knowing the batch shape up front removes the need for any runtime claim.

That fixes both of your points at once:

Sequential nesting — nothing is held across the outer delegation's lifetime, so the nested child is no longer pinned and its force_handoff fires again.
Nondeterminism — no CAS, no winner. Pinning every member of a parallel batch is also the self-consistent reading of the existing doc comment ("pinning is for concurrent sub-sessions that must NOT share the runtime's mutable currentAgent").

The tally is per tool rather than per batch, so a transfer_task sharing a batch with an unrelated tool stays solo.

Verifying it against the merge base

I reproduced your finding before fixing it. The new test is table-driven over three delegation shapes and, on the previous commit, failed exactly where you said:

shape	want	before
solo delegation honours `force_handoff`	1	1
sequential nested honours `force_handoff`	1	0
parallel batch pins every child	0	1

The nested case passes on the merge base, confirming a regression rather than a gap.

A coverage hole in my own fix

Mutating batchByTool[tc.Function.Name] to len(calls) left all 1145 package tests green — the per-tool decision was argued in a comment but exercised by nothing, since both my cases used homogeneous batches. Added a fourth: a batch mixing transfer_task with an unrelated tool, which the delegation must survive as solo. That case kills the mutant.

Second commit: `handoff` has the same defect

While tracing which tools contend for the current agent, handleHandoff turned out to read it for caller resolution and to discard the tools.Runtime this PR introduces. A transfer_task beside it in one batch can swap that field first, and the handoff is then validated against the transfer target's handoffs list. The test fails on the old code with:

Agent drafter cannot hand off to specialist: target agent not in handoffs list.
No agents are configured in this list.

That is #4156's own error string, on the sibling tool. The second commit resolves its caller from the same snapshot.

Deliberately not fixed here: handleHandoff still calls setCurrentAgent outside the pin/switch machinery. Unlike transfer_task's call-scoped swap, handoff rewires the session for all later turns, so routing it through the same machinery is a design change rather than a bug fix. Happy to open a separate issue — say the word if you'd rather I fold it in.

Two notes for the record

docs/tools/transfer-task/index.md documents transfer_task as blocking and points parallel work at background_agents. Pinning every child of a parallel batch moves the implementation toward that documented model.
Behaviour change: previously one of N parallel transfers emitted AgentSwitching/AgentInfo; now none do. Intended, but worth stating.

Validation

go build ./..., go vet, go test ./... (6002 tests), go test -race ./pkg/runtime/... (1147), go run ./lint . (19 cops, 1929 files, 0 offenses). The batch test is stable over 30 consecutive runs. Required CI has since run on the fork and is green apart from windows-tests — so please disregard that bullet in your review, it was accurate when you wrote it.

bernard-code-lab · 2026-09-12T17:01:15Z

Correction to my CI note above: I force-pushed about 30 seconds before posting it, so the green run I cited belonged to the superseded b6fdcaf, not to the current head. 6037fe5 has action_required check suites and no runs yet.

Locally on the current head: go test ./... (6002 tests), go test -race ./pkg/runtime/... (1147), go vet, and go run ./lint . are all clean.

@aheritier — could you approve the workflow run when you get a chance? No rush on the review itself.

aheritier · 2026-09-14T15:47:38Z

+				t.Helper()
+				finisher, probe := handoffProbe("finisher")
+				worker := agent.New("worker", "worker agent",
+					agent.WithModel(&mockProvider{id: "test/mock-model",


[blocking] lint is red on this line:

pkg/runtime/agent_delegation_test.go:1887:1: File is not properly formatted (gofumpt)

Reproduced locally with golangci-lint 2.13.2. task format fixes it — the same &mockProvider{...} literal needs splitting in all four table cases.

aheritier · 2026-09-14T15:47:39Z

+
+// handoffProbe returns an agent whose model invocations are counted, standing
+// in for a force_handoff target that must (or must not) be reached.
+func handoffProbe(name string) (*agent.Agent, *handoffRecordingProvider) {


[blocking] Second lint failure:

pkg/runtime/agent_delegation_test.go:1844:19: handoffProbe - name always receives "finisher" (unparam)

All four call sites pass "finisher". Simplest fix is to drop the parameter and hardcode the name, since the probe is only ever the force_handoff target.

aheritier · 2026-09-14T15:47:40Z

+			// others' turns. Pin every member of the batch rather than letting
+			// one win a race — the parallel children are isolated, and which
+			// child keeps switch events no longer varies per run (#4156).
+			req.PinAgent = true


[blocking] Pinning every member of the batch silently disables force_handoff for all of them — loop.go:1035 gates applyForceHandoff on sess.AgentName == "". Your own table encodes this at agent_delegation_test.go:1961 (wantHandoffCalls: 0); on the merge base that case yields 1.

That conflicts with shipped docs that promise the opposite:

docs/configuration/agents/index.md:119 — "unconditionally receives the conversation whenever this agent produces a final response … guaranteeing deterministic pipelines"

docs/concepts/multi-agent/index.md:147 — "force_handoff removes that uncertainty … the runtime itself routes the conversation"

So a force_handoff pipeline breaks with no error as soon as the model batches its transfers — the same quiet wrong-output class this PR is removing. Please either keep force_handoff reachable for pinned delegation children, or update both doc sites and the SwitchCurrentAgent comment in the same change to state the parallel-batch exception. (The PR body’s "the 2nd..Nth child is pinned" is also stale — all children are.)

aheritier · 2026-09-14T15:47:41Z

+	batchByTool := make(map[string]int, len(calls))
+	for _, tc := range calls {
+		batchByTool[tc.Function.Name]++
+	}


[should-fix] Keying the tally on tc.Function.Name protects transfer_task from its own siblings, but not from the other tool that reaches runForwarding. With a batch of [transfer_task, run_skill] the transfer sees concurrentSiblings == 1, stays solo, and swaps current; runSkillFork leaves its child unpinned for an unpinned parent (skill_runner.go:147), so the fork resolves from the swapped field.

A reproducer on this head expected SKILL-RAN-AS-ROOT but received SKILL-RAN-AS-WORKER; the child had AgentName == "". This is pre-existing rather than a regression, but the PR body’s claim that run_skill is fixed by the same change only holds for caller resolution, not child execution. Count the delegating tools together (or expose batch size so runSkillFork can pin when >1), and add a mixed transfer_task + run_skill batch test beside the think case.

aheritier · 2026-09-14T15:47:43Z

+	// same batch may already have swapped it, which would validate the handoff
+	// against the transfer target's handoffs list instead of the real
+	// caller's (#4156).
+	currentAgent := r.callerAgent(rt, sess)


[should-fix] This changes behavior outside the parallel-batch scenario. callerAgent falls back to resolveSessionAgent, which prefers sess.AgentName, so a handoff from a pinned background session now validates against the pinned agent’s handoffs instead of the foreground agent’s — then reaches setCurrentAgent(next.Name()) at line 815 with no pinned-session guard, mutating shared current state from a background task. That is what #3886 forbids, and this change appears to make the path newly reachable where it previously failed validation.

Intended? Please add a handoff-from-a-pinned-session test to make the intended behavior explicit. (#4242 covers the batch-clobber case, but not this one.)

aheritier · 2026-09-14T16:01:21Z

👋 This PR has merge conflicts with the base branch. Please rebase or merge the latest base branch and resolve them. I've moved it to draft and added status/needs-rebase; it'll be picked back up automatically once the conflicts are cleared.

A pinned session (a background agent's session, or a child pinned by a parallel delegation batch) owns its own agent: the runtime's shared current agent belongs to the concurrent foreground loop and must not be mutated from it (docker#3886). handleHandoff resolved its caller from the batch snapshot, which falls back to session resolution, so a handoff from a pinned session started passing validation and then mutated that shared field. applyForceHandoff had the same shape, which is why loop.go skipped force_handoff for pinned sessions altogether — and why pinning a parallel batch's children silently disabled the "unconditional", "deterministic" routing the docs promise. Both now go through switchSessionAgent, which moves the session's own pin when it has one. That makes the skip unnecessary: the next loop iteration resolves the target instead of the agent that just stopped, and config validation already rejects force_handoff cycles, so the chain terminates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SWthkrTCN7ZDCKVYQ9PXgN

The batch tally was keyed by tool name, so a lone transfer_task standing beside a handoff still counted itself solo. It claimed the runtime's shared current agent and restored the caller on the way out, silently undoing the handoff that ran next to it — whether the handoff survived depended on which goroutine finished last, the same quiet wrong-output class this series removes. The dispatcher now hands every call the whole per-tool tally, and ConcurrentCalls sums the tools a handler names: it knows which ones contend for what it is about to claim, the dispatcher does not. transfer_task names handoff alongside itself, so a mixed batch pins the delegation and the handoff's switch stands. handoffProbe now builds a fresh stream per invocation: parallel children all routing to the same force_handoff target reach the probe at once, and mockStream's cursor is single-use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SWthkrTCN7ZDCKVYQ9PXgN

A batch of [transfer_task, run_skill] was still unguarded. The delegation saw no sibling of its own name, counted itself solo and swapped the shared current agent to its target; the fork skill's child, left unpinned, then resolved from that same field and ran as the delegation's target instead of as its caller. runSkillFork already resolves the caller before dispatch, so the child is now always pinned to it and never consults the shared field, whatever its siblings do. That closes the shape by construction rather than by naming run_skill in transfer_task's tally — handoff still needs the tally, since it moves the session's own agent and has no child to pin. handoffProbe drops its name parameter: every caller passed "finisher". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SWthkrTCN7ZDCKVYQ9PXgN

Resolves status/needs-rebase. One conflict, in handleTaskTransfer's delegationRequest literal: main added directNativeTransfer, this branch added CallerAgent and ConcurrentBatch. Both kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SWthkrTCN7ZDCKVYQ9PXgN

aheritier · 2026-09-15T14:31:29Z

👋 Some commits in this PR are not signed and verified by GitHub. Please sign your commits with a GPG or SSH key registered in your GitHub account, then force-push.

Commits that are not verified: 4983673, 7c9244f, 5883d1e, d2f36bb

See GitHub's guide on signing commits for setup instructions. I've added status/needs-signed-commits; it will be removed automatically once every commit in this PR carries a valid GitHub-verified signature.

aheritier · 2026-09-19T09:02:03Z

👋 This PR has merge conflicts with the base branch. Please rebase or merge the latest base branch and resolve them. I've moved it to draft and added status/needs-rebase; it'll be picked back up automatically once the conflicts are cleared.

bernard-code-lab requested a review from a team as a code owner September 6, 2026 20:52

bernard-code-lab mentioned this pull request Sep 6, 2026

transfer_task is not concurrency-safe: parallel calls race on current-agent pointer, later calls fail with "No agents are configured in this list" #4156

Open

aheritier added area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection kind/fix PR fixes a bug (maps to fix:). Use on PRs only. status/needs-signed-commits Some commits in the PR are signed with a valid SSH/GPG key labels Sep 6, 2026

bernard-code-lab force-pushed the fix/4156-concurrent-transfer-task branch from 7b3dc36 to b6fdcaf Compare September 7, 2026 00:24

aheritier removed the status/needs-signed-commits Some commits in the PR are signed with a valid SSH/GPG key label Sep 7, 2026

aheritier requested review from dgageot and rumpl September 11, 2026 13:09

bernard-code-lab added 2 commits September 11, 2026 11:12

bernard-code-lab force-pushed the fix/4156-concurrent-transfer-task branch from b6fdcaf to 6037fe5 Compare September 11, 2026 14:15

bernard-code-lab mentioned this pull request Sep 11, 2026

handoff is silently discarded when it shares a tool-call batch with transfer_task #4242

Open

aheritier reviewed Sep 14, 2026

View reviewed changes

aheritier added the status/needs-rebase PR has merge conflicts or is out of date with main label Sep 14, 2026

aheritier marked this pull request as draft September 14, 2026 16:11

bernard-code-lab and others added 3 commits September 15, 2026 14:14

aheritier added status/needs-signed-commits Some commits in the PR are signed with a valid SSH/GPG key and removed status/needs-rebase PR has merge conflicts or is out of date with main labels Sep 15, 2026

aheritier added the status/needs-rebase PR has merge conflicts or is out of date with main label Sep 19, 2026

Conversation

bernard-code-lab commented Sep 6, 2026

Problem

Fix

Behavior change

Out of scope

Testing

Uh oh!

aheritier commented Sep 6, 2026

Uh oh!

bernard-code-lab commented Sep 10, 2026

Uh oh!

aheritier commented Sep 11, 2026

Review: request changes

Blocking

Also needed

Uh oh!

aheritier commented Sep 11, 2026

Uh oh!

bernard-code-lab commented Sep 11, 2026

The claim is gone, not rescoped

Verifying it against the merge base

A coverage hole in my own fix

Second commit: handoff has the same defect

Two notes for the record

Validation

Uh oh!

bernard-code-lab commented Sep 12, 2026

Uh oh!

aheritier Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

aheritier Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

aheritier Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

aheritier Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

aheritier Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

aheritier commented Sep 14, 2026

Uh oh!

aheritier commented Sep 15, 2026

Uh oh!

aheritier commented Sep 19, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Second commit: `handoff` has the same defect