Skip to content

Commit 01f34e5

Browse files
committed
fix(execution): measure pressure at write time and keep terminal status 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.
1 parent 2e6bcd5 commit 01f34e5

2 files changed

Lines changed: 87 additions & 5 deletions

File tree

apps/sim/lib/execution/event-buffer.test.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -671,6 +671,77 @@ describe('execution event buffer', () => {
671671
expect(persisted).not.toContain(payload)
672672
})
673673

674+
/**
675+
* Terminal status is the reader's end-of-run signal: once it lands, a
676+
* reconnecting client drains what is in Redis and closes. Stamping it while
677+
* lower event ids are still queued strands those events behind a stream the
678+
* reader has already finished with.
679+
*
680+
* Needs a backlog past the single-write cap so chunking leaves a remainder
681+
* behind the terminal entry — the only shape where that ordering can invert.
682+
*/
683+
it('does not stamp terminal status while earlier events are still queued', async () => {
684+
mockRedis.incrby.mockResolvedValue(100000)
685+
const idsAtStamp: number[] = []
686+
mockRedis.eval.mockImplementation(async (script: string, ...args: unknown[]) => {
687+
if (!isFlushScript(script)) return [1, 'ok', 0, 0]
688+
const { terminalStatus, zaddArgs } = parseFlushEvalArgs(args)
689+
// Reject any multi-entry batch, forcing the terminal-alone retry path.
690+
if (zaddArgs.length > 2) return [0, 'execution_redis_bytes', 64 * 1024 * 1024]
691+
for (let i = 0; i < zaddArgs.length; i += 2) {
692+
persistedEntries.push(JSON.parse(zaddArgs[i + 1] as string) as ExecutionEventEntry)
693+
}
694+
if (terminalStatus && idsAtStamp.length === 0) {
695+
idsAtStamp.push(...persistedEntries.map((e) => e.eventId))
696+
}
697+
return [1, 1, 0]
698+
})
699+
700+
// ~3MB per event, so three of them exceed the 8MiB single-write cap and the
701+
// chunk boundary leaves a remainder queued behind the terminal entry.
702+
const payload = 'x'.repeat(1_500_000)
703+
const writer = createExecutionEventWriter('exec-1', {
704+
workspaceId: 'ws-1',
705+
workflowId: 'wf-1',
706+
})
707+
for (let i = 0; i < 3; i++) {
708+
await writer.write(makeEvent(payload)).catch(() => {})
709+
}
710+
await writer.writeTerminal(makeEvent('terminal'), 'complete').catch(() => {})
711+
await writer.close().catch(() => {})
712+
713+
const terminalId = Math.max(...persistedEntries.map((e) => e.eventId))
714+
const strandedAtStamp = persistedEntries
715+
.map((e) => e.eventId)
716+
.filter((id) => id < terminalId && !idsAtStamp.includes(id))
717+
expect(strandedAtStamp).toEqual([])
718+
})
719+
720+
/**
721+
* Pressure has to be measured as events are produced, not once a flush
722+
* succeeds. A burst is compacted long before the scheduled flush runs, so
723+
* flush-time accounting would let the very batch that exhausts the budget
724+
* through at the loose ceiling and drop it instead of offloading it.
725+
*/
726+
it('engages pressure within a burst that has not flushed yet', async () => {
727+
mockRedis.incrby.mockResolvedValue(100000)
728+
const payload = 'x'.repeat(2 * 1024 * 1024)
729+
730+
const writer = createExecutionEventWriter('exec-1', {
731+
workspaceId: 'ws-1',
732+
workflowId: 'wf-1',
733+
})
734+
// No flush between writes: everything stays pending while the burst builds.
735+
for (let i = 0; i < 20; i++) {
736+
await writer.write(makeEvent(payload)).catch(() => {})
737+
}
738+
await writer.flush().catch(() => {})
739+
740+
// The later events in the burst must have been offloaded, not left inline.
741+
const persisted = JSON.stringify(persistedEntries)
742+
expect(persisted).toContain(LARGE_VALUE_REF_MARKER)
743+
})
744+
674745
it('preserves requested UserFile base64 when buffering terminal events', async () => {
675746
mockRedis.incrby.mockResolvedValue(100)
676747
const base64 = Buffer.from('hello').toString('base64')

apps/sim/lib/execution/event-buffer.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -765,9 +765,12 @@ export function createExecutionEventWriter(
765765
let flushTimer: ReturnType<typeof setTimeout> | null = null
766766
let consecutiveFlushFailures = 0
767767
/**
768-
* Bytes this execution has successfully buffered. Counted gross rather than
769-
* net of ring-buffer pruning, so it reaches the pressure mark early — erring
770-
* toward offloading sooner is the safe direction.
768+
* Bytes this execution has produced, counted as each event is compacted
769+
* rather than once a flush succeeds. A burst can be compacted long before the
770+
* scheduled flush runs, so flush-time accounting would let the very batch that
771+
* exhausts the budget through at the loose ceiling. Counted gross of
772+
* ring-buffer pruning too, so the mark is reached early — erring toward
773+
* offloading sooner is the safe direction.
771774
*/
772775
let bufferedBytes = 0
773776

@@ -947,7 +950,6 @@ export function createExecutionEventWriter(
947950
}
948951
consecutiveFlushFailures = 0
949952
lastResourceLimitError = null
950-
bufferedBytes += batchBytes
951953
if (chunkTerminalStatus) pendingTerminalStatus = undefined
952954
return true
953955
} catch (error) {
@@ -1032,6 +1034,7 @@ export function createExecutionEventWriter(
10321034
valueThresholdBytes: getValueThresholdBytes(),
10331035
})
10341036
const entry: ExecutionEventEntry = { eventId, executionId, event: compactEvent }
1037+
bufferedBytes += getJsonSize(entry) ?? 0
10351038
pending.push(entry)
10361039
if (pending.length >= FLUSH_MAX_BATCH) {
10371040
await flushPending()
@@ -1079,6 +1082,7 @@ export function createExecutionEventWriter(
10791082
valueThresholdBytes: getValueThresholdBytes(),
10801083
})
10811084
const entry: ExecutionEventEntry = { eventId, executionId, event: compactEvent }
1085+
bufferedBytes += getJsonSize(entry) ?? 0
10821086
pending.push(entry)
10831087
let ok = false
10841088
try {
@@ -1091,9 +1095,16 @@ export function createExecutionEventWriter(
10911095
// budget rejection specifically: a transient Redis error leaves the batch
10921096
// queued for retry, and clearing it here would turn that into data loss.
10931097
const remaining = pending.filter((pendingEntry) => pendingEntry !== entry)
1098+
// Drain what is queued ahead of the terminal event first. Terminal
1099+
// status is the reader's end-of-run signal: stamping it while lower
1100+
// event ids are still queued strands them behind a stream the reader
1101+
// has already drained and closed.
1102+
if (remaining.length > 0) {
1103+
pending = remaining
1104+
await flushPending(false)
1105+
}
10941106
pending = [entry]
10951107
ok = await flushPending(false)
1096-
pending = pending.concat(remaining)
10971108
}
10981109
} catch (error) {
10991110
discardTerminalEntry(entry)

0 commit comments

Comments
 (0)