Skip to content

fix(workflow-engine): clear pruned loop history via batched, bounded deletes - #5654

Open
abcxff wants to merge 1 commit into
mainfrom
stack/fix-workflow-engine-clear-pruned-loop-history-via-batched-bounded-deletes-nrzuuryv
Open

fix(workflow-engine): clear pruned loop history via batched, bounded deletes#5654
abcxff wants to merge 1 commit into
mainfrom
stack/fix-workflow-engine-clear-pruned-loop-history-via-batched-bounded-deletes-nrzuuryv

Conversation

@abcxff

@abcxff abcxff commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Port of the workflows-repo fix. Add batchDelete to the EngineDriver
interface and all implementations, and route loop-history pruning through
runDeletes(), which coalesces keys into transaction-sized (MAX_KV_BATCH_ENTRIES)
batchDelete chunks run in bounded rounds (MAX_CONCURRENT_DELETES=64).

Previously deleteEntriesWithPrefix / flush fanned out one unbounded
Promise.all of single-statement deletes per key; cutting a loop history
of >128 entries exceeded the actor SQLite transaction coordinator's
128-permit admission cap (non-blocking try_acquire -> transaction_queue_full).

@abcxff

abcxff commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/actors

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

change nrzuuryv

@railway-app

railway-app Bot commented Sep 3, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5654 environment in rivet-frontend

Service Status Web Updated
frontend-cloud 😴 Sleeping (View Logs) Web Sep 3, 2026 at 4:51 pm UTC
kitchen-sink 😴 Sleeping (View Logs) Web Sep 3, 2026 at 4:51 pm UTC
frontend-inspector 😴 Sleeping (View Logs) Web Sep 3, 2026 at 4:51 pm UTC
ladle ✅ Success (View Logs) Web Sep 3, 2026 at 4:42 pm UTC
website ❌ Build Failed (View Logs) Web Sep 3, 2026 at 4:39 pm UTC
mcp-hub ✅ Success (View Logs) Web Sep 3, 2026 at 4:38 pm UTC

…deletes

Port of the workflows-repo fix. Add batchDelete to the EngineDriver
interface and all implementations, and route loop-history pruning through
runDeletes(), which coalesces keys into transaction-sized (MAX_KV_BATCH_ENTRIES)
batchDelete chunks run in bounded rounds (MAX_CONCURRENT_DELETES=64).

Previously deleteEntriesWithPrefix / flush fanned out one unbounded
Promise.all of single-statement deletes per key; cutting a loop history
of >128 entries exceeded the actor SQLite transaction coordinator's
128-permit admission cap (non-blocking try_acquire -> transaction_queue_full).
@abcxff
abcxff force-pushed the stack/fix-workflow-engine-clear-pruned-loop-history-via-batched-bounded-deletes-nrzuuryv branch from 4786f7c to 3dd5055 Compare September 3, 2026 16:39
@abcxff
abcxff changed the base branch from stack/feat-rivetkit-isolate-includestate-transaction-reads-with-a-committed-snapshot-snxxvxko to main September 3, 2026 16:39
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review

Solid, well-targeted fix for the loop-pruning path. The change correctly identifies and fixes the root cause: Promise.all over one driver.delete() per key created an unbounded fan-out of single-statement SQLite transactions, blowing past the actor's 128-permit transaction admission cap when pruning large loop histories. Routing everything through batchDelete (coalesced into <=128-key chunks) with a bounded MAX_CONCURRENT_DELETES=64 window fixes this while keeping headroom under the 128-permit cap.

Bug: the same unbounded fan-out still exists in replayWorkflowFromStep

workflow-engine/src/index.ts:832-837:

await Promise.all(
    entriesToDelete.flatMap(({ entry }) => [
        driver.delete(buildHistoryKey(entry.location)),
        driver.delete(buildEntryMetadataKey(entry.id)),
    ]),
);

This is structurally identical to the bug this PR fixes elsewhere: one driver.delete() per key, fanned out unbounded via Promise.all. entriesToDelete is every history entry from the replay target (or the whole history, when entryId is omitted) through the end, each contributing 2 individual deletes. Any workflow with more than ~64 history entries after the replay point will exceed the 128-permit admission cap the same way the original loop-pruning bug did, hitting transaction_queue_full.

tests/replay.test.ts does not exercise a large history, so this gap is not caught by current tests. Given the PR is explicitly porting a fix for this exact failure mode, it would be worth routing this call through the new runDeletes-style batching too (or exposing a shared helper) rather than leaving one of the two call sites fixed and the other not.

Correctness (loop-pruning path)

  • runDeletes() preserves the exact historyUpdated semantics from the old code (ops.length > 0 mirrors the old deleteOps.length > 0 check) - good parity.
  • Both EngineDriver implementations (WorkflowStorage in rivetkit/src/workflow/driver.ts and InMemoryDriver in workflow-engine/src/testing.ts) were updated; confirmed via grep there are no other implementers, so the interface addition does not leave a gap.
  • One subtle behavior change worth calling out (not a bug, but worth confirming): the old deleteEntriesWithPrefix awaited deletePrefix before running the parallel key deletes; the new runDeletes puts the prefix/range op and the batched key-delete ops in the same concurrency window, so they can now run concurrently. This looks safe since prefix/range deletes target the history-entry keyspace and the batched keys target the entry-metadata keyspace (disjoint), but it is a real ordering change from before, so worth double-checking no driver implementation relies on prefix-delete-then-key-delete ordering.

Minor / nit

  • MAX_CONCURRENT_DELETES = 64 is calibrated against a cap defined in a different subsystem entirely (the actor SQLite transaction coordinator's 128-permit try_acquire, which lives in the Rust engine, not this package). The comment documents the reasoning but there is no compile-time or cross-linked reference tying the two together; if the engine's permit cap ever changes, this constant could silently drift out of sync. A pointer comment to the Rust-side constant (or a shared doc reference) would make that coupling more discoverable.
  • InMemoryDriver.batchDelete (testing.ts) deletes keys one at a time in a loop rather than atomically, which is fine given the interface's "should be atomic if possible" wording, but worth noting it will not catch atomicity regressions in tests.
  • Also in index.ts, replayWorkflowFromStep builds entries via Promise.all(... loadMetadata(...)), one get() per history entry. Lower confidence than the delete fan-out above (reads may not hit the same transaction-admission path), but worth a look given the same function has the confirmed delete-side issue.

Test coverage
Good for the fixed path: prune-fanout.test.ts verifies both important properties directly - (1) large prefix deletes are coalesced into batchDelete calls with zero per-key delete() fan-out, and (2) concurrent delete ops are bounded, with the peak-in-flight assertion (toBe(MAX_CONCURRENT_DELETES)) actually exercising the boundary rather than just asserting <=. Nice touch using a real driver subclass instead of mocks, consistent with the repo's no-mocking test policy. The gap is replayWorkflowFromStep, which has no large-history test and, per above, still has the bug.

Recommendation: fix or explicitly scope out the replayWorkflowFromStep fan-out before merging, since it is the same failure mode the PR title and description say this change addresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant