refactor: split workflow dispatchLocked and stop abandoning ready nodes on error #446

New issue

Open

opened 2026-07-25 23:31:18 +02:00 by dries · 0 comments

dries commented

2026-07-25 23:31:18 +02:00

Owner

Code-quality audit (P2). internal/workflows/service.go:1902-2085 — dispatchLocked is 184 lines at 4 levels of nesting.

The problem

An unbounded for {} wraps a node loop that inlines: claim, shard provisioning, prompt interpolation, item substitution, schema-instruction append, agent start, agent inspect, schema-correction retry, and completion.

This is a latent bug, not only a readability issue. Several returns sit inside the node loop, so a failure on one node silently abandons every remaining ready node in that pass:

result, inspectErr := s.agent.Inspect(ctx, AgentSession{...})
if inspectErr != nil {
    return inspectErr          // service.go:2021 — remaining ready nodes never dispatched
}

Same shape for the SetWorkflowAgentSessionState error returns below it. A transient inspect failure on node A stalls node B, C, ... until the next dispatch tick, and if the error is sticky the run wedges.

Suggested fix

Extract startAgentAttempt, inspectAgentAttempt, requestSchemaCorrection.
Flatten the driver loop; collect per-node errors instead of returning from inside the loop, so one node's failure settles that node and the pass continues.
Bound the outer for {} with an explicit progress/iteration guard.

Acceptance criteria

dispatchLocked is under ~60 lines and at most 2 nesting levels.
A per-node error settles only that node; remaining ready nodes in the same pass still dispatch.
Regression test: two ready nodes, Inspect fails for the first — assert the second still starts. Must fail on current main before the fix.

Effort: L.

Code-quality audit (P2). `internal/workflows/service.go:1902-2085` — `dispatchLocked` is 184 lines at 4 levels of nesting. ## The problem An unbounded `for {}` wraps a node loop that inlines: claim, shard provisioning, prompt interpolation, item substitution, schema-instruction append, agent start, agent inspect, schema-correction retry, and completion. **This is a latent bug, not only a readability issue.** Several `return`s sit inside the node loop, so a failure on one node silently abandons every remaining ready node in that pass: ```go result, inspectErr := s.agent.Inspect(ctx, AgentSession{...}) if inspectErr != nil { return inspectErr // service.go:2021 — remaining ready nodes never dispatched } ``` Same shape for the `SetWorkflowAgentSessionState` error returns below it. A transient inspect failure on node A stalls node B, C, ... until the next dispatch tick, and if the error is sticky the run wedges. ## Suggested fix - Extract `startAgentAttempt`, `inspectAgentAttempt`, `requestSchemaCorrection`. - Flatten the driver loop; collect per-node errors instead of returning from inside the loop, so one node's failure settles that node and the pass continues. - Bound the outer `for {}` with an explicit progress/iteration guard. ## Acceptance criteria - [ ] `dispatchLocked` is under ~60 lines and at most 2 nesting levels. - [ ] A per-node error settles only that node; remaining ready nodes in the same pass still dispatch. - [ ] Regression test: two ready nodes, `Inspect` fails for the first — assert the second still starts. Must fail on current `main` before the fix. Effort: L.