feat: run tool dispatcher calls in parallel#3467
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The parallelization design is solid. Each tool call gets its own *call struct (no shared per-call mutable state), session.AddMessage is internally goroutine-safe, outcomes[i] writes are to distinct indices, and the two-mutex approach (confirmationMu for serializing interactive confirmations, approvalMu for session permissions) is consistent with no lock-ordering inversions.
Specific correctness points verified:
stopOncecorrectly prevents double-cancel; even iferrBatchCanceledByUserfires beforeerrBatchStoppedByHook, theoutcomesloop afterwg.Wait()still findsStopRun: trueand returns(true, stopMessage)— the StopRun return value is never lost.cancellationOutcomereturningCallOutcome{}for hook-stopped siblings is handled correctly; the post-loop only checksStopRun, so empty outcomes are correctly skipped.- Lock ordering is consistent: the only nesting is
confirmationMu → approvalMu(insideautoApprovalAfterConfirmationWait), and there is no path that acquiresapprovalMufirst thenconfirmationMu. - Loop variable capture is safe under Go 1.22+ per-iteration semantics (confirmed by
wg.GoAPI usage).
One minor latency note left inline.
| } | ||
|
|
||
| slog.DebugContext(ctx, "Tools not approved, waiting for resume", "tool", c.tc.Function.Name, "session_id", c.sess.ID) | ||
| c.em.EmitToolCallConfirmation(c.tc, c.tool, c.a.Name(), c.confirmationMetadata(hookMeta)) |
There was a problem hiding this comment.
[LOW] confirmationMu held across EmitToolCallConfirmation and NotifyUserInput
confirmationMu is acquired at line 657 and not released until one of the select case arms fires (lines ~683–687). This means it is held across EmitToolCallConfirmation (line 675) and NotifyUserInput (line 678), which can dispatch hooks and emit events — potentially slow operations.
Under concurrent batches, any tool call that needs user confirmation will block at confirmationMu.Lock() until the current confirmation completes (including the hook dispatch latency). This is a minor latency concern rather than a correctness bug — the serialization is intentional and no deadlock is possible since hook dispatchers don't acquire confirmationMu. Consider whether the hook calls could be moved before the lock acquisition if they don't need to be serialized.
There was a problem hiding this comment.
Good point. The lock has to cover the confirmation event and wait because ResumeRequest is not keyed by tool-call ID; allowing a second confirmation to become visible before the first resolves would make delivery on the shared resume channel ambiguous. I added a short comment at the lock site to document that.
There was a problem hiding this comment.
Thanks for the clarification — that makes complete sense. Since ResumeRequest isn't keyed by tool-call ID, the serialization is architecturally required to prevent routing ambiguity on the shared channel. The comment you added documents that constraint nicely.
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
6c58412 to
f47e336
Compare
| if c.preYoloResult != nil && c.preYoloResult.Decision == hooks.DecisionAsk { | ||
| return PermissionDecision{}, false | ||
| } | ||
| decision := c.permissionDecision(c.tool.Annotations.ReadOnlyHint) |
There was a problem hiding this comment.
Passing readOnlyHint=true here re-applies the read-only fast-path inside askUser and silently skips a confirmation a pre_tool_use hook requested on a read-only tool. Pass false to match the deliberate Decide call in approveAndRun. A regression test where a read-only tool plus a hook returning Ask still prompts would guard this.
Summary
Validation
Note: a plain task test initially hit a live Moonshot alias API 404 because MOONSHOT_API_KEY was set locally; rerunning with live-provider env vars unset passed.