fix(workflow-engine): mark completed inner-loop iterations visited on resume

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5605 fix(workflow-engine): mark completed inner-loop iterations v … 👈
• #5603 fix(rivetkit): expose engine ray IDs on errors for debug
• #5601 fix(frontend): submit Vercel drain grace period
• #5604 fix(rivetkit-napi): remove stale HttpRequestBodyStream test …
• main

Get stack: forklift get 5605
Push local edits: forklift submit
Merge when ready: forklift merge 5605

_{change vqowwtqx}

[claude]

Review

Good fix for the core bug: a completed loop previously returned its saved output without ever marking its nested history entries visited, so an enclosing branch validateComplete() would wrongly throw HistoryDivergedError, and in rollback mode a completed loop nested rollback handlers were never registered at all. The new markCompletedLoopIterationsVisited and the rollback re-walk address both problems, and the added tests (loops.test.ts, rollback.test.ts) exercise the described scenarios well.

Likely bug: rollback re-walk only replays the last iteration once any pruning has happened, silently skipping other retained iterations

In context.ts, the new rollback branch (around line 1251-1306):

const firstRetained = this.firstRetainedLoopIteration(location, loopData.iteration);
if (firstRetained === 0) {
    rollbackIteration = 0;
    rollbackState = config.state as S;
} else {
    rollbackIteration = loopData.iteration;   // jumps straight to the last iteration
    rollbackState = loopData.state as S;
}

This treats "some pruning happened" as equivalent to "only the last iteration is retained," but historySize (default DEFAULT_LOOP_HISTORY_PRUNE_INTERVAL = 20) can retain many iterations, not just one.

Concrete trace with default settings (historySize/historyPruneInterval = 20): a loop that runs 45 iterations (indices 0-44) before breaking. Periodic pruning at iteration 40 deletes iterations 0-19 (keepFrom = 40-20 = 20). At completion, collectLoopPruning(location, 45, 20, 20) computes keepFrom = 45-20 = 25 and deletes iterations 20-24. Final retained set: iterations 25-44 (20 iterations), so firstRetainedLoopIteration returns 25, which is neither 0 nor loopData.iteration (44).

When rollback mode later hits this completed loop, the else branch sets rollbackIteration = 44 and replays only iteration 44. Iterations 25-43 still have step entries in storage.history.entries, but they are never passed through config.run(...) in rollback mode, so:

• Their step(...) rollback handlers are never registered in rollbackActions, so those side effects are silently never rolled back.
• Their history keys are never added to visitedKeys. If this loop is nested inside another loop/try/join/race that calls validateComplete() over a prefix containing it, this throws HistoryDivergedError, which would abort the rollback replay partway through.

This is a realistic scenario (any loop that runs more than historySize iterations, such as a polling or retry loop, followed by a later step failing and triggering rollback), which is exactly the class of case this PR targets. The added test ("should roll back retained iterations when loop history is pruned") only covers historySize: 1, where firstRetained happens to equal loopData.iteration, so it does not catch this gap for historySize > 1.

Since intermediate per-iteration state genuinely is not persisted (only the final loop state is), full replay of skipped iterations may not be feasible, but it would be worth at least calling something like markCompletedLoopIterationsVisited for the iterations that cannot be replayed (mirroring the non-rollback path), so this degrades to "cannot roll back that particular side effect" instead of crashing or aborting the whole rollback. A test with historySize > 1 and more iterations than the retained window would help pin down the intended behavior here.

Minor

• The metadata.status === "completed" check and the loopData.output !== undefined check right below it both now call markCompletedLoopIterationsVisited(location, loopData.iteration + 1) and return the same output. This duplication predates this PR, but since both branches now share identical follow-up logic, it might be worth collapsing them.
• markCompletedLoopIterationsVisited and firstRetainedLoopIteration both do a full scan of this.storage.history.entries (not just the loop own subtree) on every resume of a completed or resuming loop. This matches the existing scanning style elsewhere in the file (e.g. collectLoopPruning), so it is consistent, but for workflows with many loops and large retained history this is O(loops x total entries) per replay. Likely fine given historySize bounds it, but worth keeping in mind if replay latency ever becomes a concern.

Test coverage

The new tests for the main visited-marking fix (nested loop resume, sibling completed/suspending inner loops, rollback of a completed loop, Loop.break(undefined), and single-iteration-retained pruning) are solid and match the scenarios described in the comments well. As noted above, adding a rollback plus pruning test with historySize > 1 would help cover the partial-retention gap described above.