Repository navigation
fix(recovery): decide stuck-task recovery under the dispatch state lock (#977) - #980
Merged
Merged
Conversation
…h state lock Closes #977. Both recovery engines judged a workflow task stranded from a scope read BEFORE awaiting hooks (active steps + fanned-out task ids). A nested/map child spawned AND claimed between that walk and the synchronous check was invisible: the parent row is untouched by a child spawn (so the `version` compare cannot see it) and the child's run is keyed on the child id. The parent was recovered — or blocked — while a child was executing. Every ledger claim is already made under `withStateLock` (dispatch-cycle / -single / -team → claimDispatchRun), so: - The watchdog's per-task decision moves into `recoverStuckTask`, run under the lock: liveness is read again inside it (a workflow's topology re-walked — a scope that changed since the scan is left for the next tick), then the row is validated fresh (inProgress, same `version`) and everything from that read to the store write is synchronous. The scan's own read stays as the pre-filter and feeds alert-only mode. - Restart recovery decides each candidate in `settleCandidate` under the same lock, so a claim in flight (HTTP is live during startup recovery) lands before the re-assessment reads the ledger. Tests: a child spawned and claimed during the scan leaves the parent alone; both engines park behind a held lock and see the claim its holder made. All three fail against the previous engines. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tate — never re-takes the dispatch lock Codex review of #980 found a deadlock already on main that the recovery lock would have widened: `resolveTeamAssignmentForStep` runs inside `dispatchWorkflowTask`, which the cycle/single dispatch call under `withStateLock`; on a transient routing failure it awaited `withStateLock` again to record the ladder entry. The lock is a non-reentrant promise queue, so that cycle never finished — and with recovery queued on the same lock, neither would the watchdog or startup recovery. The resolver now takes the caller's `state` (`StepTeamResolutionArgs.state`) and reads/records the ladder there; the lock holder saves, as it already did (a reload-and-save would have been overwritten by that save anyway). It never takes the lock itself. Also from the review: two more watchdog tests — a topology that changed during the scan with nothing live is skipped that tick and recovered once it holds still (pins scopeKey independently of the live-run read), and a claim racing the decision queues behind recovery's lock. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #977 — the residual recorded in #975's round-3 review.
Both recovery engines judged a workflow task stranded from a scope read before awaiting hooks. A nested/map child spawned and claimed between that walk and the synchronous check was invisible (parent row untouched, child run keyed on the child id), so the parent could be recovered or blocked while a child was executing.
Every ledger claim is already made under
withStateLock, so the decision and the write now happen under that lock:recoverStuckTaskruns per task under the lock — re-reads liveness (re-walks the workflow topology; a scope that changed since the scan is left for the next tick), validates the row fresh (inProgress, sameversion), then supersede / recover / block with nothing awaiting between the read and the store write. The scan's read stays as the pre-filter and feeds alert-only mode.settleCandidateunder the lock, so a claim in flight (HTTP is live during startup recovery) lands before the re-assessment reads the ledger.Deadlock fixed on the way (Codex review of this PR)
resolveTeamAssignmentForStepruns insidedispatchWorkflowTask, which the cycle/single dispatch already call underwithStateLock; on a transient team-routing failure it awaitedwithStateLockagain to record the ladder entry. The lock is a non-reentrant promise queue, so that dispatch cycle never finished — onmaintoday — and with recovery queued on the same lock the watchdog and startup recovery would have hung too. The resolver now records into the caller'sstate(newStepTeamResolutionArgs.state) and never takes the lock; the lock holder saves as it already did.dispatch-state.tsimports only the logger —check:cyclesunchanged.Verification
tests/core/{watchdog,restart-recovery,dispatch,dispatch-handoff,dispatch-team-step,continuation}.test.ts,tests/plugins/workflows/team-step-token.test.ts, health checks route suite green; typecheck, lint (3 pre-existing warnings),check:cyclesgreen.Docs:
.claude/knowledge/dispatch.md§ Restart recovery,.claude/knowledge/agent-system.md§ Watchdog,.claude/knowledge/team-aware-assignment.md(step ladder lives in the caller's state), spec D9 residual marked closed.🤖 Generated with Claude Code