fix(workflow): preserve queued recovery reservations

[kojiwakayama]

Summary

• distinguish a durably reserved recovery from one that actually entered an execution batch
• reuse a queued reservation across wait and approval-resume passes without spending another attempt
• keep the batch-start node-state write authoritative before any recovered side effect runs
• preserve retry exhaustion after a started recovery crashes again

Red and green

The new maxConcurrency: 1 regression starts with a parked wait ahead of an interrupted step. Before the fix, recovery persisted attempt=2 with the old startedAt, and the approval-resume pass failed with retry budget exhausted before the step ran.

The reservation now persists attempt=2 with no startedAt. The approval-resume pass reuses that reservation, batch start durably restores startedAt, and the side effect executes exactly once. Existing out-of-budget and no-silent-completion tests remain green.

Verification

• deno task test:file src/workflow/executor (12 groups, 201 steps)
• focused DAG regression (113 steps)
• Node targeted executor tests (115 pass)
• Bun targeted executor tests (115 pass)
• deno task fmt:check
• deno task lint
• deno task typecheck
• deno task docs:api-reference:check

Refs veryfront/veryfront-issue-inbox#719

[github-actions]

📦 Client bundle boundary

Entrypoint	Modules	Source size	Server leaks
src/index.client.ts	327	1961 KiB	✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b9fcc6cea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Warning

Review limit reached

@kojiwakayama, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 23 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 32f7b815-3034-4156-8d08-7e162c77eb45

📥 Commits

Reviewing files that changed from the base of the PR and between f8dbecb and 3b9fcc6.

📒 Files selected for processing (3)

• src/workflow/executor/dag/index.test.ts
• src/workflow/executor/dag/index.ts
• src/workflow/executor/workflow-executor.test.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kwakayama]

Closing in favour of #3981, which fixes the same defect. Both branches were opened against veryfront/veryfront-issue-inbox#719 within two hours of each other and change the same block of DAGExecutor, so they cannot both land.

They are the same fix twice

I wrote outcome-only tests that assert nothing about either mechanism, and ran them on the merge base (1a3cda1de3), on this head (3b9fcc6cea) and on #3981's head:

Scenario	base	#3992	#3981
A parked wait ahead of an interrupted node at maxConcurrency: 1 (the #719 report)	RED	green	green
The bound still holds after a queued node is resumed and starts	RED	green	green
Two interrupted nodes competing for one slot each keep their own recovery	RED	green	green
A started recovery is refused a second run	green	green	green

On the defect itself the two branches are indistinguishable. Each branch's own tests fail on the other's head, but only on assertions about its own mechanism: when onRecoveryScheduled fires, and what its payload carries. Those are not outcome differences.

The one behavioural difference decided it

Codex's P1 on this branch is correct, and I reproduced it. Take a node persisted as running with attempt: 1 and no startedAt, under the default one-attempt policy. Resume it, let it start, kill the worker, resume again from the durable write that was in force while it executed:

base       side effect ran 1x, second resume refused as out of budget
#3992      side effect ran 2x
#3981      side effect ran 1x, second resume refused as out of budget

recoveryReserved reads true for that state, the ceiling widens to maxAttempts + 1, the attempt is not charged, and the node collects a second recovery that main refuses. That is a duplicate side effect, and it fails open.

It is reachable. startedAt is optional on NodeState and in the valibot schema, run.nodeStates is a public input to DAGExecutor.execute(), and WorkflowBackend is a supported third-party extension point. Any of those can produce a running node with no timestamp.

The approach here is sound apart from that, and the marker Codex suggests would fix it, but it would mean adding a persisted field to a public type. #3981 gets the same outcome with no new durable state: it holds recovered nodes in an in-process queue and charges the attempt when a node is admitted to a batch, so nothing is ever inferred from startedAt.

Nothing from this branch is lost

Carried onto #3981 in b3a96e4cc7:

• your assertExists on the persisted startedAt in workflow-executor.test.ts, verbatim in intent. It is red there under a mutation that drops startedAt from the durable recovery write.
• a regression guard built from the Codex finding, still spends only one recovery when the interrupted state has no startedAt, which is red on this branch's head and red on fix(workflow): charge crash recovery when a node starts, not when it queues #3981 under a mutation that charges only when startedAt is present. The sentinel cannot come back unnoticed.

I also mutation-tested both branches' bounds while comparing them. Removing the exhaustion bound for child graphs was caught by nothing on either branch, so #3981 now carries a test for it too.

Thanks for the work here. The reservation framing is what made the startedAt question visible at all, and the Codex thread on this PR is what settled which branch ships.

[kwakayama]

One correction to the reachability argument above, so the record is precise. I wrote that run.nodeStates is "a public input to DAGExecutor.execute()". DAGExecutor is not exported, so that is the wrong mechanism.

The accurate path is the backend. WorkflowBackend is exported from src/workflow/index.ts and createWorkflowClient({ backend }) accepts an implementation of it, which src/server/handlers/request/project-run-execute.handler.ts already does. A custom backend persists and returns WorkflowRun.nodeStates, and the executor consumes whatever it returns. src/workflow/index.ts documents that surface directly: "Node ids match the keys the executor writes into run.nodeStates ... so a project can join metadata to run state to build its own view."

So a backend that does not round-trip the optional startedAt produces exactly the state in question. The conclusion is unchanged, and the measured two-executions result stands.