fix(workflow): stop charging approval resumes to the crash recovery budget

[kojiwakayama]

Closes veryfront/veryfront-issue-inbox#696.

The defect

A composite runs its children against a synthetic WorkflowRun whose status is always "running". DAGExecutor reads run status to tell two situations apart:

• a run parked on a human decision — a composite recorded running is re-entered so its child resumes;
• a run whose worker died mid-node — a node recorded running is re-run under a bounded recovery budget.

At the top level the real run is waiting and this works. One level down the child graph always looks like the second case, so an ordinary approval resume re-enters the enclosing composite off the crash budget.

Why it stayed invisible, and when it stops being invisible

Re-entering the node overwrites the bumped attempt with a fresh attempt: 1, so in the common case the mis-accounting leaves no trace. It survives only when a node pushed to ready by the recovery path is queued behind maxConcurrency and an earlier node in the batch parks on a wait — that node never executes, so nothing overwrites its bump.

Reproduced end to end in src/workflow/executor/dag/index.test.ts with parallel > [parallel > wait, parallel > wait] at maxConcurrency: 1. On main, two ordinary approvals produce:

Node "outer" failed: Node "inner-a" was interrupted after 2 of 1 attempt(s); retry budget exhausted

Nobody interrupted inner-a.

Second defect, found in review: a run could report success with a step never executed

Review flagged that propagating the root status into child graphs would let a genuinely
interrupted nested step be skipped. Probing it found something worse: the same hole is on
main today, at the top level, and this branch did not create it.

Two nodes at maxConcurrency: 1 — a wait and a step — where a worker died with the step in
flight. Recover, then approve. On origin/main:

PASS1 waiting=true   executed=[]  sideEffect=running  attempt=2
PASS2 completed=true executed=[]  sideEffect=running

completed: true, the step never executed, still recorded running. No child graph is
involved: resumingWait is already true at the top level, and the same lines already skip a
non-composite there.

The rule was wrong, not the propagation. "A waiting run has nothing to recover" assumes parked
and interrupted are exclusive, and they are not — a worker can die with a step in flight while
a sibling wait is parked, and the run then reaches waiting with that step still running.
Only the composite case is a resume; everything else recorded running now falls through to
the recovery path, where the existing wait guard and retry budget already apply. When the
budget is gone the run fails loudly instead of reporting a success it did not earn.

If you are bisecting a report of a workflow that finished with a step never executed, this is
it, and it predates this branch. Fixed here rather than split out because this branch touches
the same eight lines and would otherwise widen its reach.

The fix

Thread the reason execution stopped rather than inferring it from a record that cannot carry it. A new ExecutionScope collects the values a child graph must agree with the root run about — rootRunId, executionRunId, ownership, and now resumingWait — and replaces the four positional arguments that were already being threaded verbatim through every level (executeUnwrapped, executeNode, dispatchNode, and each composite strategy). resumingWait is read once, in execute(), off the only run record that knows.

Re-entry behaviour is unchanged; only the accounting moves. Which composite types are re-entered on a wait resume (RESUMABLE_COMPOSITE_TYPES) is now the same rule at every depth, where before nested graphs used the recovery rule.

Tests

• New: wait resume inside a nested composite — the repro above (fails on main), plus a companion asserting a genuinely dead worker (status: "running") still recovers a nested node.
• Updated: workflow-tracing.test.ts calls the private executeUnwrapped directly and was updated to the scope argument. Its assertions — child span workflow.run_id vs the step hook's run id — are unchanged.

Verification

deno test src/workflow/executor/          12 passed (182 steps), 0 failed
deno test src/workflow/executor/dag/      99 steps, 0 failed
deno lint src/workflow/executor/          23 files, clean
deno check (3 touched files)              clean

src/workflow/blob/veryfront-cloud-storage.test.ts fails in this sandbox, but it fails identically on a clean origin/main worktree — it is an ambient globalThis.fetch stub reaching the real network, which is the hazard tracked in veryfront/veryfront-issue-inbox#692. Unrelated to this change.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 50 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: cb4dcf11-46e3-4cfe-91f6-92a0b8c5a6f1

📥 Commits

Reviewing files that changed from the base of the PR and between 3837403 and 78a68a7.

📒 Files selected for processing (4)

• src/workflow/executor/dag/index.test.ts
• src/workflow/executor/dag/index.ts
• src/workflow/executor/dag/types.ts
• src/workflow/executor/workflow-tracing.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.}

[github-actions]

📦 Client bundle boundary

Entrypoint	Modules	Source size	Server leaks
src/index.client.ts	326	1949 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: 89dd30c9b4

ℹ️ 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".

[kwakayama]

Duplicate of this from my side: I opened #3943 against the same issue twelve minutes after you, same diagnosis. I am closing mine. Yours is the better shape: bundling rootRunId, executionRunId, ownership, and resumingWait into ExecutionScope removes four positional arguments where mine added a fifth, and it puts the invariant in a type where someone will read it.

I checked yours properly before closing mine, and it holds up:

• It threads the real root status, it does not special-case the symptom.
• It covers all five composite paths: executeParallelNode, executeBranchNode, executeSubWorkflowNode, and the map and loop strategies through the executeChildGraph closures.
• All three of my fail-first tests pass on 89dd30c9b4 unchanged, including a three-level nest (parallel > parallel > parallel > wait), so there is no depth hole.

One test of mine is worth taking, and I can show why rather than assert it. Your crash companion proves a dead worker still recovers a nested node. It does not prove that recovery is still bounded down there. Mutating your branch to drop the child-graph bound:

-        if (attempts > maxAttempts) {
+        if (isDurableRun && attempts > maxAttempts) {

leaves both of your new tests green:

spends no recovery budget when an approval resumes a nested composite ... ok
still recovers a nested node when the worker died mid-run ... ok

A regression that removed the bound entirely inside child graphs would ship green. This test catches it (it goes red under that mutation, and passes on your branch as-is):

    it("still bounds recovery for a node a dead worker left running inside a composite", async () => {
      const executed: string[] = [];
      const exec = new DAGExecutor({
        stepExecutor: new MockStepExecutor(new Map(), (node) => {
          executed.push(node.id);
          return { success: true, output: node.id, executionTime: 1 };
        }),
      });

      const nodes: WorkflowNode[] = [
        {
          id: "outer",
          dependsOn: [],
          config: {
            type: "parallel",
            nodes: [{ id: "leaf", dependsOn: [], config: { type: "step" } }],
          } as any,
        },
      ];

      // A running run, so a node recorded running is an interrupted attempt.
      // The leaf already spent its one recovery, so it must not run again.
      const run = createTestRun({
        status: "running",
        nodeStates: {
          outer: { nodeId: "outer", status: "running", attempt: 1, startedAt: new Date() },
          leaf: { nodeId: "leaf", status: "running", attempt: 2, startedAt: new Date() },
        },
      });

      const result = await exec.execute(nodes, run);

      assertEquals(executed, []);
      assertEquals(result.completed, false);
      assertEquals(result.nodeStates["leaf"]!.status, "failed");
    });

It needs NodeState in the type import, which your diff already adds. Take it or leave it, no need to reply.

[kojiwakayama]

Re: P1 "Preserve recovery for interrupted nested siblings" — confirmed, and it predates this branch

Probed it before accepting it. The finding is real, and the fix is pushed in 78a68a7.

It reproduces on main, at the top level. Two nodes at maxConcurrency: 1, a run recovered from a crash and then approved:

PASS1 waiting=true   executed=[]  sideEffect=running  attempt=2
PASS2 completed=true executed=[]  sideEffect=running

completed: true, the step never executed, still recorded running. No child graph involved — resumingWait is already true at the top level, and lines 160–162 already skip a non-composite there. Propagating the flag into child graphs widens the reach; it does not create the hole.

Root cause is the rule, not the propagation. "A waiting run has nothing to recover" assumes parked and interrupted are exclusive. They are not: a worker can die with a step in flight while a sibling wait is parked, and the run then reaches waiting with that step still running. Narrowing the wait-resume branch to only re-enter composites — and letting everything else fall through to the recovery path, where the existing wait guard and retry budget already apply — fixes both.

After the fix, same probe:

PASS1 waiting=true    executed=[]              sideEffect=running attempt=2
PASS2 completed=false executed=[]              sideEffect=failed    ← loud, not silent
RECOVER completed=true executed=["sideEffect"] sideEffect=completed ← budget intact

PASS2 still does not run the step, because its budget was already spent by the recovery scheduled in PASS1 that never got its turn behind the concurrency limit. That is the correct outcome from the recorded state, and it now fails with retry budget exhausted instead of reporting a success it did not earn. (The narrower question of whether a node queued-but-never-started should be charged a recovery at all is real but separate — the attempt is written before scheduling on purpose, so it survives a second crash. Not changing that here.)

Two regression tests added in src/workflow/executor/dag/index.test.ts:

• recovers an interrupted step on a wait resume instead of skipping it
• never reports completion while an interrupted step is out of budget

deno test src/workflow/ — 81 passed (837 steps), 0 failed. fmt:check and lint:anti-slop clean.