Skip to content

Commit e2ea9c0

Browse files
committed
fix(execution): stop the event buffer retaining a run-length backlog
The Redis byte-budget branch in doFlush requeued the rejected batch and rethrew, skipping the MAX_PENDING_EVENTS trim every other failure path applies. The backlog then grew for the rest of the run and each retry re-serialized it, so a wide parallel fan-out drove multi-GB heap growth and enough event-loop stall to push ALB p90 past 16s. Drop rejected chunks instead of requeueing, pace retries through the existing backoff, and split batches that exceed the single-write cap so an oversized batch can make progress instead of stalling forever. Terminal status is now writer-scoped, since a concurrent scheduled flush can be the loop that drains the final chunk. Record terminal stream meta when the terminal event cannot be buffered, so reconnecting readers stop polling an active stream until their deadline. Drop the unused reserve/release budget helpers.
1 parent 3de63c9 commit e2ea9c0

6 files changed

Lines changed: 392 additions & 179 deletions

File tree

apps/sim/app/api/workflows/[id]/execute/route.async.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ const {
4141
mockHandlePostExecutionPauseState,
4242
mockHasDurableExecutionOwner,
4343
mockInitializeExecutionStreamMeta,
44+
mockSetExecutionMeta,
4445
mockReleaseExecutionIdClaim,
4546
mockReleaseExecutionSlot,
4647
mockReleaseWorkflowToolExecutionClaim,
@@ -66,6 +67,7 @@ const {
6667
mockHandlePostExecutionPauseState: vi.fn(),
6768
mockHasDurableExecutionOwner: vi.fn(),
6869
mockInitializeExecutionStreamMeta: vi.fn(),
70+
mockSetExecutionMeta: vi.fn(),
6971
mockReleaseExecutionIdClaim: vi.fn(),
7072
mockReleaseExecutionSlot: vi.fn(),
7173
mockReleaseWorkflowToolExecutionClaim: vi.fn(),
@@ -128,6 +130,7 @@ vi.mock('@/lib/execution/event-buffer', () => ({
128130
createExecutionEventWriter: mockCreateExecutionEventWriter,
129131
flushExecutionStreamReplayBuffer: mockFlushExecutionStreamReplayBuffer,
130132
initializeExecutionStreamMeta: mockInitializeExecutionStreamMeta,
133+
setExecutionMeta: mockSetExecutionMeta,
131134
LIVE_ONLY_EXECUTION_EVENT_TYPES: new Set(),
132135
}))
133136

@@ -403,6 +406,7 @@ describe('workflow execute async route', () => {
403406
})
404407
mockHandlePostExecutionPauseState.mockResolvedValue(undefined)
405408
mockInitializeExecutionStreamMeta.mockReset().mockResolvedValue(true)
409+
mockSetExecutionMeta.mockReset().mockResolvedValue(true)
406410
mockFlushExecutionStreamReplayBuffer.mockReset().mockResolvedValue(true)
407411
mockCreateExecutionEventWriter.mockReset().mockReturnValue({
408412
write: vi.fn(async (event: unknown) => ({ event, eventId: '1' })),
@@ -456,6 +460,31 @@ describe('workflow execute async route', () => {
456460
expect(body).toContain('execution:completed')
457461
})
458462

463+
/**
464+
* A terminal event the replay buffer rejected leaves the stream meta on
465+
* `active`, so a reconnecting reader polls until its deadline and then errors.
466+
* Recording the status directly is the only signal it gets.
467+
*/
468+
it('records terminal stream meta when the replay buffer rejects the terminal event', async () => {
469+
mockCreateExecutionEventWriter.mockReturnValue({
470+
write: vi.fn(async (event: unknown) => ({ event, eventId: '1' })),
471+
writeTerminal: vi.fn(async () => {
472+
throw new Error('Execution memory limit exceeded. Reduce payload size and try again.')
473+
}),
474+
close: vi.fn().mockResolvedValue(undefined),
475+
})
476+
477+
const response = await POST(createBoundCopilotExecutionRequest(), {
478+
params: Promise.resolve({ id: 'workflow-1' }),
479+
})
480+
const body = await response.text()
481+
482+
expect(response.status).toBe(200)
483+
// The live client still receives the terminal event over SSE.
484+
expect(body).toContain('execution:completed')
485+
expect(mockSetExecutionMeta).toHaveBeenCalledWith('execution-123', { status: 'complete' })
486+
})
487+
459488
it('rejects a competing Copilot workflow execution before logging starts', async () => {
460489
mockClaimWorkflowToolExecution.mockResolvedValueOnce(null)
461490

apps/sim/app/api/workflows/[id]/execute/route.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ import {
5656
createExecutionEventWriter,
5757
flushExecutionStreamReplayBuffer,
5858
initializeExecutionStreamMeta,
59+
setExecutionMeta,
5960
type TerminalExecutionStreamStatus,
6061
} from '@/lib/execution/event-buffer'
6162
import { processInputFileFields } from '@/lib/execution/files'
@@ -1755,6 +1756,7 @@ async function handleExecutePost(
17551756
) => {
17561757
const isBuffered = !LIVE_ONLY_EXECUTION_EVENT_TYPES.has(event.type)
17571758
let eventToSend = event
1759+
let terminalBufferWriteFailed = false
17581760
if (isBuffered) {
17591761
try {
17601762
const entry = terminalStatus
@@ -1776,6 +1778,7 @@ async function handleExecutePost(
17761778
terminal: Boolean(terminalStatus),
17771779
error: toError(e).message,
17781780
})
1781+
terminalBufferWriteFailed = Boolean(terminalStatus)
17791782
terminalEventPublished ||= Boolean(terminalStatus)
17801783
}
17811784
}
@@ -1786,6 +1789,24 @@ async function handleExecutePost(
17861789
isStreamClosed = true
17871790
}
17881791
}
1792+
if (terminalBufferWriteFailed && terminalStatus) {
1793+
// The terminal event never reached the replay buffer, so a reconnecting
1794+
// reader would poll an `active` stream until its deadline. Record the
1795+
// terminal status on the stream meta directly — a plain HSET that bypasses
1796+
// the byte budget which rejected the event — so the reconnect route sees an
1797+
// ended run and closes cleanly. Runs after the live enqueue above: Redis is
1798+
// the most likely reason we are in this branch at all, and a slow best-effort
1799+
// durability write must never delay the primary delivery path.
1800+
const metaPersisted = await setExecutionMeta(executionId, {
1801+
status: terminalStatus,
1802+
})
1803+
if (!metaPersisted) {
1804+
reqLogger.error(
1805+
'Failed to record terminal execution meta after buffer write failure',
1806+
{ executionId, status: terminalStatus }
1807+
)
1808+
}
1809+
}
17891810
}
17901811

17911812
try {

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

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,214 @@ describe('execution event buffer', () => {
368368
expect(persistedEntries).toEqual([])
369369
})
370370

371+
/**
372+
* Requeueing a batch the budget rejected is what grew `pending` for a whole
373+
* run, each retry re-serializing an ever-larger array — the multi-GB heap
374+
* growth seen in production. Rejected bytes must be dropped, not retained.
375+
*/
376+
it('drops rejected batches instead of growing a backlog when the Redis budget is exhausted', async () => {
377+
mockRedis.incrby.mockResolvedValue(100000)
378+
let budgetExhausted = true
379+
mockRedis.eval.mockImplementation(async (script: string, ...args: unknown[]) => {
380+
if (isFlushScript(script)) {
381+
if (budgetExhausted) return [0, 'execution_redis_bytes', 64 * 1024 * 1024]
382+
const { zaddArgs } = parseFlushEvalArgs(args)
383+
for (let i = 0; i < zaddArgs.length; i += 2) {
384+
persistedEntries.push(JSON.parse(zaddArgs[i + 1] as string) as ExecutionEventEntry)
385+
}
386+
return [1, 1, 0]
387+
}
388+
return [1, 'ok', 0, 0]
389+
})
390+
391+
const writer = createExecutionEventWriter('exec-1')
392+
393+
for (let i = 0; i < 2500; i++) {
394+
await writer.write(makeEvent(`block-${i}`)).catch(() => {})
395+
}
396+
397+
// Once the budget frees the writer recovers, but only whatever accumulated
398+
// since the last rejection — never a run-length backlog.
399+
budgetExhausted = false
400+
await writer.flush()
401+
402+
expect(persistedEntries.length).toBeLessThanOrEqual(200)
403+
})
404+
405+
/**
406+
* Individual events are capped well below the single-write limit, but a burst
407+
* of large ones coalesces into a batch above it. Splitting is the only way the
408+
* buffer makes progress: no retry can shrink a batch it keeps whole.
409+
*/
410+
it('splits a batch that exceeds the single-write cap instead of stalling on it', async () => {
411+
mockRedis.incrby.mockResolvedValue(100)
412+
// Each event stays under the 8MiB per-event cap; two of them do not.
413+
const bigPayload = 'x'.repeat(2_500_000)
414+
415+
const writer = createExecutionEventWriter('exec-1')
416+
await writer.write(makeEvent(bigPayload))
417+
await writer.write(makeEvent(bigPayload))
418+
await writer.flush()
419+
420+
expect(persistedEntries).toHaveLength(2)
421+
expect(
422+
mockRedis.eval.mock.calls.filter(([script]) => isFlushScript(script as string))
423+
).toHaveLength(2)
424+
})
425+
426+
it('drops the terminal entry rather than leaving it queued when the budget is exhausted', async () => {
427+
mockRedis.incrby.mockResolvedValue(100)
428+
let budgetExhausted = true
429+
mockRedis.eval.mockImplementation(async (script: string, ...args: unknown[]) => {
430+
if (isFlushScript(script)) {
431+
if (budgetExhausted) return [0, 'execution_redis_bytes', 64 * 1024 * 1024]
432+
const { zaddArgs } = parseFlushEvalArgs(args)
433+
for (let i = 0; i < zaddArgs.length; i += 2) {
434+
persistedEntries.push(JSON.parse(zaddArgs[i + 1] as string) as ExecutionEventEntry)
435+
}
436+
return [1, 1, 0]
437+
}
438+
return [1, 'ok', 0, 0]
439+
})
440+
441+
const writer = createExecutionEventWriter('exec-1')
442+
443+
await expect(writer.writeTerminal(makeEvent('terminal'), 'complete')).rejects.toThrow(
444+
'Execution memory limit exceeded'
445+
)
446+
447+
// The failed terminal write stays surfaced through flush(), but its entry must
448+
// not linger in the backlog and reappear once the budget frees up.
449+
budgetExhausted = false
450+
await writer.flush().catch(() => {})
451+
452+
expect(persistedEntries).toEqual([])
453+
})
454+
455+
/**
456+
* A timer-driven flush carries no terminal status of its own. If it is the
457+
* loop that drains the final chunk, the terminal event lands without a status
458+
* and readers poll an `active` stream forever — while `writeTerminal` reports
459+
* success, so nothing degrades.
460+
*/
461+
it('applies terminal status even when a concurrent scheduled flush drains the final chunk', async () => {
462+
mockRedis.incrby.mockResolvedValue(100)
463+
const observedTerminalStatuses: string[] = []
464+
let releaseFirstFlush: (() => void) | undefined
465+
const firstFlushStarted = new Promise<void>((resolveStarted) => {
466+
let started = false
467+
mockRedis.eval.mockImplementation(async (script: string, ...args: unknown[]) => {
468+
if (!isFlushScript(script)) return [1, 'ok', 0, 0]
469+
const { terminalStatus, zaddArgs } = parseFlushEvalArgs(args)
470+
observedTerminalStatuses.push(terminalStatus)
471+
if (!started) {
472+
started = true
473+
resolveStarted()
474+
await new Promise<void>((resolve) => {
475+
releaseFirstFlush = resolve
476+
})
477+
}
478+
for (let i = 0; i < zaddArgs.length; i += 2) {
479+
persistedEntries.push(JSON.parse(zaddArgs[i + 1] as string) as ExecutionEventEntry)
480+
}
481+
return [1, 1, 0]
482+
})
483+
})
484+
485+
const writer = createExecutionEventWriter('exec-1')
486+
await writer.write(makeEvent('first'))
487+
await firstFlushStarted
488+
489+
const terminalWrite = writer.writeTerminal(makeEvent('terminal'), 'complete')
490+
// Let writeTerminal's queued body actually enqueue its entry before the
491+
// in-flight flush resolves — otherwise the scheduled loop finds nothing left
492+
// to drain and the race under test never forms.
493+
await new Promise((resolve) => setTimeout(resolve, 5))
494+
releaseFirstFlush?.()
495+
await terminalWrite
496+
497+
expect(observedTerminalStatuses).toContain('complete')
498+
})
499+
500+
/**
501+
* A terminal publish that threw must not be resurrected. Leaving the status
502+
* armed would let the next flush stamp the stream terminal for an event that
503+
* was discarded — telling readers the run ended cleanly while the caller was
504+
* told it failed.
505+
*/
506+
it('does not stamp terminal status on a later flush after the terminal publish failed', async () => {
507+
mockRedis.incrby.mockResolvedValue(100)
508+
const observedTerminalStatuses: string[] = []
509+
let failNextFlush = false
510+
mockRedis.eval.mockImplementation(async (script: string, ...args: unknown[]) => {
511+
if (!isFlushScript(script)) return [1, 'ok', 0, 0]
512+
if (failNextFlush) throw new Error('redis unavailable')
513+
const { terminalStatus, zaddArgs } = parseFlushEvalArgs(args)
514+
observedTerminalStatuses.push(terminalStatus)
515+
for (let i = 0; i < zaddArgs.length; i += 2) {
516+
persistedEntries.push(JSON.parse(zaddArgs[i + 1] as string) as ExecutionEventEntry)
517+
}
518+
return [1, 1, 0]
519+
})
520+
521+
const writer = createExecutionEventWriter('exec-1')
522+
await writer.write(makeEvent('a'))
523+
524+
failNextFlush = true
525+
await expect(writer.writeTerminal(makeEvent('terminal'), 'complete')).rejects.toThrow()
526+
527+
// flush() still surfaces the earlier terminal failure; what matters is that
528+
// the events it drains are not stamped terminal.
529+
failNextFlush = false
530+
await writer.flush().catch(() => {})
531+
532+
expect(observedTerminalStatuses).toEqual([''])
533+
expect(
534+
persistedEntries.map((entry) => (entry.event.data as { blockId: string }).blockId)
535+
).toEqual(['a'])
536+
})
537+
538+
/**
539+
* A budget rejection must not colour a later, unrelated failure: reporting a
540+
* Redis outage as "reduce payload size" sends the user after the wrong thing.
541+
*/
542+
it('reports the generic failure, not a stale budget rejection, on the terminal path', async () => {
543+
mockRedis.incrby.mockResolvedValue(100)
544+
let mode: 'budget' | 'outage' = 'budget'
545+
mockRedis.eval.mockImplementation(async (script: string) => {
546+
if (!isFlushScript(script)) return [1, 'ok', 0, 0]
547+
if (mode === 'budget') return [0, 'execution_redis_bytes', 64 * 1024 * 1024]
548+
throw new Error('redis unavailable')
549+
})
550+
551+
const writer = createExecutionEventWriter('exec-1')
552+
for (let i = 0; i < 200; i++) {
553+
await writer.write(makeEvent(`block-${i}`)).catch(() => {})
554+
}
555+
556+
mode = 'outage'
557+
await expect(writer.writeTerminal(makeEvent('terminal'), 'complete')).rejects.toThrow(
558+
'Failed to flush terminal execution event'
559+
)
560+
})
561+
562+
it('settles a scheduled flush that hits the budget instead of rejecting later callers', async () => {
563+
mockRedis.incrby.mockResolvedValue(100)
564+
mockRedis.eval.mockImplementation(async (script: string) => {
565+
if (isFlushScript(script)) {
566+
return [0, 'execution_redis_bytes', 64 * 1024 * 1024]
567+
}
568+
return [1, 'ok', 0, 0]
569+
})
570+
571+
const writer = createExecutionEventWriter('exec-1')
572+
await writer.write(makeEvent('a'))
573+
574+
await new Promise((resolve) => setTimeout(resolve, 60))
575+
576+
await expect(writer.flush()).resolves.toBeUndefined()
577+
})
578+
371579
it('preserves requested UserFile base64 when buffering terminal events', async () => {
372580
mockRedis.incrby.mockResolvedValue(100)
373581
const base64 = Buffer.from('hello').toString('base64')

0 commit comments

Comments
 (0)