fix(execution): offload buffered event values under budget pressure - #6231
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Compaction tracks Terminal publish ordering is fixed when the budget forces chunking: queued events ahead of the terminal entry are drained before stamping terminal status or publishing the terminal alone, so reconnect readers are not told the run ended while lower event IDs are still pending. Transient drain failures retain the backlog and withhold terminal status. On resume, non-terminal buffer write failures now degrade to live-only delivery (like the execute route) instead of failing executor callbacks after work completed; failed writes no longer assign Reviewed by Cursor Bugbot for commit e2e4138. Configure here. |
|
@cursor review |
c268b9e to
f169aff
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
f169aff to
3219b47
Compare
Greptile SummaryThis PR makes large-value offloading pressure-sensitive, preserves inline delivery for short executions, and prevents resume-path replay-buffer failures from failing completed workflow work.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior resume-path propagation and transient terminal-drain data-loss issues are addressed, while the remaining replay-gap behavior above the shared large-value cap is the previously documented degradation tradeoff.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/execution/event-buffer.ts | Adds pressure-based compaction, inline fallback, produced-byte accounting, and ordered terminal retry behavior; the previously reported transient-drain loss is addressed. |
| apps/sim/lib/execution/event-buffer.test.ts | Adds focused regression tests for both pressure-gate directions, burst behavior, terminal ordering, transient failures, and fallback persistence. |
| apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts | Converts non-terminal resume buffer-write failures into live-only delivery while leaving the reconnect cursor unset. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
E[Execution event] --> P{Produced bytes exceed pressure mark?}
P -- No --> D[Compact with shared large-value threshold]
P -- Yes --> T[Compact with tighter pressure threshold]
T --> O{Offload succeeds?}
O -- Yes --> R[Queue compacted event for Redis replay]
O -- No --> D
D --> R
R --> F{Terminal event with predecessor backlog?}
F -- No --> W[Flush normally]
F -- Yes --> B[Drain predecessors with terminal status disarmed]
B --> C{Backlog empty?}
C -- Yes --> S[Publish terminal event and status]
C -- No --> X[Reject terminal write and degrade safely]
Reviews (7): Last reviewed commit: "fix(execution): keep an event in the buf..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3219b47. Configure here.
|
@cursor review |
|
Converting to draft. Verification before merge turned up problems that go past the two open findings, and I don't think this should land as written. The compacted event goes to the live client, not just replay. Both open findings are correct, and the second is a mistake in my own fix.
Other blockers before this is worth reopening:
The incident fix is #6229, which stands alone and does not depend on this. The one piece here worth keeping independently is the |
An execution buffers EVENT_LIMIT events inside a per-execution byte budget, so a full ring only fits if events average under budget/EVENT_LIMIT. Values were only offloaded to object storage at the shared 8 MiB cap, far above that, so a run emitting large block outputs exhausted its budget within a few dozen events and stayed pinned at its ceiling for the rest of its life. Applying that ceiling to every run would be worse than the problem: the SSE stream carries the compacted event and the terminal renders a ref only as a preview, so ordinary block outputs would stop being readable live, and every value would cost an object-storage write on the hot path. Engage the tight ceiling only once a run has actually buffered past half its budget. A short run keeps full-fidelity output and pays nothing; a runaway one stops accumulating. Both bounds derive from the existing budget rather than being asserted, and preserved UserFile base64 is exempt — it is an explicit request for inline delivery, already bounded by its own cap and the strip-and-recompact fallback. Also stop a failed resume-path buffer write from failing the run: it was awaited bare, so the rejection propagated into the executor callback and failed work that had already completed. The buffer only backs reconnect replay, so degrade to live-only delivery the way the execute route does.
684ce98 to
2e6bcd5
Compare
waleedlatif1
changed the title
|
Rebuilt on top of #6229 (now in staging) and reworked in response to the pre-merge review. The flat threshold is gone. The blocking finding was that lowering it degrades the live stream, not just replay: Replaced with pressure-driven offload. The tight ceiling engages only once a run has buffered past half its budget. Short runs — the overwhelming majority — behave exactly as they do on staging: full-fidelity output, no extra writes. Only a run actually accumulating toward exhaustion starts offloading. Both bounds derive from the existing budget constants ( The two findings from the previous round are addressed at the root rather than patched:
The resume-path fix is kept and is independently valuable: the non-terminal buffer write was awaited bare, so a failure propagated into the executor callback and failed work that had already run. Both new tests are mutation-checked in both directions — gate-never-engages and gate-always-engages each turn one red. |
|
@cursor review |
…us last Pressure was read from bytes counted once a flush succeeded, but a burst is compacted long before the scheduled flush runs — so the very batch that exhausts the budget went through at the loose ceiling and was dropped instead of offloaded. Count bytes as each event is compacted. Separately, the terminal-alone retry stamped terminal status while entries queued ahead of it were still unwritten. Terminal status is the reader's end-of-run signal: a reconnecting client drains what is in Redis and closes, so those entries were stranded behind a stream it had already finished with. Drain the backlog first, then publish the terminal event.
|
@cursor review |
Draining the backlog ahead of the terminal event left the terminal status armed, so whichever chunk emptied the queue stamped the run complete before its terminal event was written — the inverse of the ordering the drain was added to guarantee. Disarm the status for the drain and restore it afterwards. The drain's result was also discarded: a transient Redis failure requeues its batch, and the unconditional reassignment that followed dropped those events even though the budget never rejected them. Keep whatever could not be persisted, and publish the terminal event alone only once nothing earlier is still queued — failing otherwise lets the caller degrade, which records the status without claiming the missing events arrived. Leave eventId unset on a failed resume-path write. Assigning 0 was persisted by clients as a reconnect cursor and rewound them to the start of the run.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 84d9164. Configure here.
…ails Durable compaction runs before an event is queued, so a storage or metadata failure dropped it from replay entirely — a reconnecting client would never see it, even though the live path carried on. Offloading under pressure is only an optimization that keeps a heavy run from exhausting its budget, so when the value cannot be persisted, fall back to buffering it inline: exactly what the run would have done before pressure engaged.
|
Addressed the remaining 4/5 concern in e2e4138 — the one about durable compaction running before an event is queued, so a storage failure omits it from replay. That was right, and worth fixing rather than documenting. Offloading under pressure is only an optimization — it keeps a heavy run from exhausting its budget. Losing the event from replay is a worse outcome than carrying it inline, so a failed pressure offload now falls back to the shared cap: exactly what the run would have done before pressure engaged. if (valueThresholdBytes === undefined) return compactEventForBuffer(event, options)
try {
return await compactEventForBuffer(event, { ...options, valueThresholdBytes })
} catch (error) {
logger.warn('Pressure offload failed; buffering the event inline instead', { … })
return compactEventForBuffer(event, options)
}The fallback only applies when pressure was active, so a run below the mark behaves exactly as it does on staging. Covered by |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e2e4138. Configure here.
1 participant
Summary
An execution buffers
EVENT_LIMITevents inside a per-execution byte budget, so a full ring only fits if events average underbudget / EVENT_LIMIT. Values were only offloaded to object storage at the shared 8 MiB cap — far above that — so a run emitting large block outputs exhausted its budget within a few dozen events and stayed pinned at its ceiling for the rest of its life.Rather than applying that ceiling to every run, engage it only once a run has buffered past half its budget.
Why not a flat lower threshold
A flat cutoff was the first attempt and was wrong on two counts:
execute/route.tssendsentry.event— the compacted event — to the active SSE client, andstructured-output.tsxrenders aLargeValueRefasref.previewonly (a truncated string, or{ length: N }/{ keys: [...] }for arrays and objects). There is no client-side rehydration. So a flat cutoff makes ordinary block outputs unreadable in the terminal, live and on replay.Pressure-driven avoids both: a short run keeps full-fidelity output and pays nothing extra, while a runaway one stops accumulating. Both bounds derive from the existing budget constants rather than being asserted.
Also in this PR
Stop a failed resume-path buffer write from failing the run.
human-in-the-loop-manager.tsawaited the non-terminal write bare — while the terminal write directly above it already had a.catch— so a write failure propagated into the executor callback and failed work that had already completed. The buffer only backs reconnect replay, so it now degrades to live-only delivery the way the execute route does.Exemption
Preserved UserFile base64 is exempt from pressure offloading: it is an explicit request for inline delivery, and is already bounded by its own cap and the strip-and-recompact fallback.
Type of Change
Testing
Two tests pinning both directions of the pressure gate — values stay inline below the mark, and offload above it — each verified to fail against the opposite mutant (gate never engages / gate always engages). Full
lib/execution,lib/workflowsandapp/api/workflowssuites green (98 files, 1645 tests).Not exercised against a real Redis or a live run; the Lua is mocked throughout.
Checklist