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

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5593 fix(rivetkit-core): always signal actor stop handle on teard …
• #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
• main

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

_{change vqowwtqx}

[claude]

Code Review

Reviewed the fix for marking completed inner-loop iterations visited on resume, plus the new loops.test.ts / rollback.test.ts coverage.

Bug: rollback replay of a completed loop skips the visited-marking gap when historySize > 1

In context.ts, the rollbackMode && metadata.status === "completed" branch (around lines 1252-1306) computes firstRetained via firstRetainedLoopIteration, but when firstRetained !== 0 it still only replays a single iteration:

} else {
    rollbackIteration = loopData.iteration;
    rollbackState = loopData.state as S;
}
for (; rollbackIteration <= loopData.iteration; rollbackIteration++) { ... }

This loop always starts at loopData.iteration (the last one) rather than firstRetained, so it only ever re-executes the final retained iteration, then returns before reaching markCompletedLoopIterationsVisited (which is only called from the non-rollback branches further down).

collectLoopPruning retains a range of iterations, [currentIteration - historySize, loopData.iteration], whenever historySize > 1. Any retained iteration older than the last one (i.e. firstRetained < loopData.iteration) is therefore never marked visited on this rollback path. If that completed loop is nested inside a try/race/join/another loop, the ancestor's validateComplete() walks all history entries under its prefix and throws HistoryDivergedError for the unvisited mid-range entries.

The new "should roll back retained iterations when loop history is pruned" test in rollback.test.ts uses historySize: 1, which forces firstRetained === loopData.iteration (no gap), the degenerate case that happens not to exercise this path. None of the added tests nest a historySize > 1 completed loop inside a try/race/join, so this gap isn't caught by the new suite. I verified this against the code directly: the rollback loop body only ever runs once when firstRetained !== 0, and no other call site marks the skipped intermediate iterations visited.

Suggested fix: when firstRetained !== 0, iterate from firstRetained and mark the intermediate retained iterations visited for the ancestor's validateComplete(), even though only the final iteration needs to actually re-run for rollback-handler registration.

Other observations (non-blocking)

• markCompletedLoopIterationsVisited and firstRetainedLoopIteration both do a full scan over this.storage.history.entries per call. These run on every resume of a completed/existing loop (including nested loops), so a workflow with many loops and a large retained history could scale poorly on the hot resume path. Worth considering a location-indexed lookup instead of a full scan, especially since collectLoopPruning already computes a bounded iteration range for the same data.
• The "Defensive: redundant under the prefix invariant..." comments are a nice touch documenting why an apparently-dead check is retained, good adherence to the repo's comment-style guidance (complete sentences, explains non-obvious rationale, no em dashes).
• Test coverage is otherwise solid: the loop/inner-loop resume tests correctly cover both the "still suspended" and "completed-early-return" cases for sibling inner loops, and the rollback tests cover completed loops, Loop.break(undefined), and pruned history, just missing the nested-ancestor-branch + historySize > 1 combination described above.

Style / conventions

Changes otherwise follow repo conventions (structured comments as complete sentences, no em dashes, formatting consistent with surrounding code).