fix(execution): compact loop state before serializing a pause snapshot - #6256
fix(execution): compact loop state before serializing a pause snapshot#6256waleedlatif1 wants to merge 3 commits into
Conversation
A loop compacts its accumulated iteration outputs when it exits, but a pause is by definition mid-flight and never reaches that point. The running total therefore arrived at the serializer uncompacted and tripped its size assertion, which throws rather than degrades — turning the pause into a failed run, so no paused_executions row was ever written. The approval notification goes out during block execution, well before the engine builds the paused result, so the approver was left holding a working looking resume link pointing at a row that never existed, and the run reported a generic failure with no hint that a byte budget caused it. Run the same compaction the loop performs on exit, and register the keys it mints: reads are gated on the context's key list, so a resumed run could not materialize the offloaded values otherwise. The assertion stays — it is a valid post-condition, and an unstorable snapshot means an unresumable pause.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Serialization now also asserts parallel execution state is compact, matching loops. Reviewed by Cursor Bugbot for commit 1f833ee. Configure here. |
Greptile SummaryThe PR compacts accumulated loop and parallel execution state before serializing pause snapshots, preserving authorization for newly materialized values.
Confidence Score: 5/5The PR appears safe to merge because no eligible or outstanding blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/execution/engine.ts | Makes paused-result construction asynchronous and compacts execution scopes before snapshot serialization. |
| apps/sim/executor/execution/snapshot-serializer.ts | Adds bounded loop and parallel state compaction, access-key registration, and parallel snapshot-size enforcement. |
| apps/sim/executor/execution/engine.test.ts | Adds engine-level coverage proving an oversized loop can produce a paused result. |
| apps/sim/executor/execution/snapshot-serializer.test.ts | Adds focused coverage for loop and parallel compaction, access authorization, and the small-state fast path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Execution reaches HITL pause] --> B[Mark execution paused]
B --> C[Measure loop and parallel state]
C -->|Within limit| E[Serialize pause snapshot]
C -->|Oversized| D[Compact payloads and record access keys]
D --> E
E --> F[Build paused execution result]
Reviews (2): Last reviewed commit: "fix(execution): cover every subflow fiel..." | Re-trigger Greptile
The first pass only compacted a loop's completed iteration outputs, which left the same pause failure reachable by four other routes: a forEach collection, an in-flight iteration output, two loops each individually under the limit but oversized together, and a parallel's accumulated branch outputs. Parallel state was not even asserted, so it shipped an oversized snapshot silently rather than failing. Compact every field that accumulates, assert parallel state alongside loop state, and offload at a threshold far below the snapshot's own — the assertion measures the combined record, so compacting at its ceiling is a no-op in exactly the case that needs it. Skip the pass entirely when the state already fits, so a pause per iteration inside a modest loop pays one bounded measurement rather than a structural rebuild each time, and count the compaction against the recorded duration instead of stopping the clock before it runs. Add an engine-level test: the serializer tests all passed with the call removed, leaving the wiring itself undefended.
|
Reworked in 1f833ee after an independent audit. The bots were clean on the first round — Greptile 5/5, Bugbot pass, CI green, zero threads — and the fix was still materially incomplete. It only covered one of five routes to the same failure. The audit proved the rest with tests:
The third is the one that shows the shape of the bug: the assertion measures the combined record while compaction ran per scope, so compacting at the snapshot's own ceiling did nothing in exactly the case that needed it. Offloading now uses a threshold well below that ceiling, and only once the state is already oversized. The fourth is worse than the bug being fixed — a parallel traded a loud failure for a quiet one. It is now asserted alongside loop state. The wiring was undefended. Deleting Also addressed: the pass is skipped when the state already fits (it was an unconditional structural rebuild on every pause, which for approval-inside-a-loop is O(N²) across a run), the duration no longer stops the clock before compaction runs, and the fixtures use the real 101 files / 1853 tests green; the one failure in |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1f833ee. Configure here.
| } | ||
| if (scope.items?.length) { | ||
| scope.items = await compactList(scope.items) | ||
| } |
There was a problem hiding this comment.
Stale loop item after compaction
High Severity
The compactPauseSnapshotScopes function compacts a loop's scope.items array but misses refreshing scope.item, which holds the current element. If scope.item contains a large payload, serializeLoopExecutions will include this uncompacted value in the snapshot. This causes pause serialization to fail due to exceeding size limits, preventing the execution from being recorded as paused.
Reviewed by Cursor Bugbot for commit 1f833ee. Configure here.
Compacting a loop's `items` was a regression: the orchestrator indexes that collection to derive the current `item`, the resume path rebuilds the scope verbatim without materializing anything, and the loop resolver asserts no refs reach it — so an oversized forEach would have traded a failed pause for a broken resume. `currentIterationOutputs` is excluded for the same reason: the block executor has already compacted its entries, and they resolve through the reference path rather than being read raw. Offloading is now limited to exactly the accumulators the orchestrators themselves compact when a subflow exits. An oversized `items` collection therefore still fails the pause; that is the honest outcome until it can be handled without breaking iteration.
|
Converting to draft. A full-lifecycle audit found this is not a strict improvement, and I don't think it should merge in this shape. The failure was moved, not removed. The snapshot can end up larger than the one that was rejected. The key registration I added is a no-op for two of the three fields. 64 KiB is the mechanism, not a policy. It exists to force the aggregate branch of Resume gets slower, not lighter. Refs are never hydrated on rebuild — only warmed, serially, one storage GET at a time — and the result is discarded into an LRU. This is a DB-row-size change, not a memory change. Also: The right approach is a chunked manifest — The one piece worth keeping regardless is the |
1 participant
Summary
orchestrators/loop.ts), but a pause is by definition mid-flight and never reaches that point. The running total arrived atserializePauseSnapshotuncompacted and trippedassertSnapshotValueIsCompact, which throws rather than degrades.handlePostExecutionPauseStatenever writes thepaused_executionsrow.Why this is worse than a failed run
The HITL approval notification — email/Slack, containing the resume links — is sent during block execution (
human-in-the-loop-handler.ts), well before the engine builds the paused result. So the approver receives a working-looking approval link pointing at a row that is never created, and the run surfaces a generic "Execution failed" with no indication that a byte budget caused it.Reachability
Not theoretical. Nothing caps the accumulation:
allIterationOutputsis compacted per iteration (loop.ts) but never in aggregate until loop exit.executor.tsrebuilds it from the snapshot — so an "approve each item" loop grows across every approval.The canonical failing workflow is the most idiomatic HITL pattern there is:
forEachover records → work → approval per record. It works for the first few hundred approvals and then abruptly stops pausing.Why not degrade instead
pause-persistence.tsalready handles a missing seed by failing the run, so catching the throw would produce the same failed run with a vaguer error. The snapshot is the point of a pause — an unstorable snapshot means an unresumable pause. The assertion stays as a valid post-condition; this satisfies it rather than tripping it.Note the sibling call in
getSerializableExecutionStateis guarded, but it is not an equivalent case: it only runs on non-paused exits where the snapshot feeds a display payload, so degrading there costs a UI detail rather than the resume artifact.Registering the minted keys
Compaction creates refs at pause time, and reads are gated on the context's key list (
materialization.server.ts), so the resumed run could not materialize them unless their keys reach the snapshot'strustedLargeValueAccess.recordMaterializedAccessKeysis called for exactly that. This was caught by the test below, not by inspection.Type of Change
Testing
Three cases in
snapshot-serializer.test.ts, each mutation-verified:executor+lib/executionsuites green (100 files, 1848 tests). One unrelated failure inexecutor/handlers/pi/cloud-review-tools.test.tsis pre-existing on clean staging — verified by stashing.Not exercised against a live paused workflow.
Checklist