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

New issue

Closed

opened 2026-07-25 23:31:18 +02:00 by dries · 2 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.

dries added the

labels

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

dries referenced this issue

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

refactor: split internal/workflows/service.go (2673 lines) #449

dries commented

2026-08-20 09:22:57 +02:00

Author

Owner

Validated against main @ 2d661364: partially fixed — the named wedge is gone. Inspect failures no longer return from the node loop: recordInspectFailure + continue, marking the attempt unknown only after the failure proves persistent (service.go:2233-2251; the comment there documents the old stall). Launch failures get bounded retries too (:2190-2211). Still true: state.db store-error returns inside the loop (ClaimWorkflowAgentAttempt :2152, AttachWorkflowAgentSession :2219, SetWorkflowAgentSessionState :2259/:2287, CompleteWorkflowAgentNode :2298) abandon the rest of the pass — but the periodic tick retries, and a sticky state.db failure breaks the engine regardless. Rescope: refactor + per-node error collection, no longer a run-wedging bug.

Validated against main @ 2d661364: **partially fixed — the named wedge is gone**. Inspect failures no longer return from the node loop: recordInspectFailure + continue, marking the attempt unknown only after the failure proves persistent (service.go:2233-2251; the comment there documents the old stall). Launch failures get bounded retries too (:2190-2211). Still true: state.db store-error returns inside the loop (ClaimWorkflowAgentAttempt :2152, AttachWorkflowAgentSession :2219, SetWorkflowAgentSessionState :2259/:2287, CompleteWorkflowAgentNode :2298) abandon the rest of the pass — but the periodic tick retries, and a sticky state.db failure breaks the engine regardless. Rescope: refactor + per-node error collection, no longer a run-wedging bug.

dries commented

2026-09-11 20:58:18 +02:00

Author

Owner

Closing as obsolete: the Workflows feature has been retired. internal/workflows no longer exists — DAG-based workflows were replaced by project-bound Routines (internal/routines, see #576 and docs/features/routines.md). The code this refactor targeted is gone, so there is nothing left to split or fix here.

Closing as obsolete: the Workflows feature has been retired. `internal/workflows` no longer exists — DAG-based workflows were replaced by project-bound Routines (`internal/routines`, see #576 and docs/features/routines.md). The code this refactor targeted is gone, so there is nothing left to split or fix here.

dries closed this issue

2026-09-11 20:58:22 +02:00