Skip to content

Commit 412ab18

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 could drive unbounded heap growth and stall the event loop. 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, and a terminal event whose batch was dropped is retried on its own rather than lost with it. 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 412ab18

6 files changed

Lines changed: 450 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: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
* @vitest-environment node
33
*/
44
import { redisConfigMockFns, resetRedisConfigMock } from '@sim/testing'
5+
import { sleep } from '@sim/utils/helpers'
56
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
67
import type { ExecutionEventEntry } from '@/lib/execution/event-buffer'
78
import type { ExecutionEvent } from '@/lib/workflows/executor/execution-events'
@@ -368,6 +369,259 @@ describe('execution event buffer', () => {
368369
expect(persistedEntries).toEqual([])
369370
})
370371

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

0 commit comments

Comments
 (0)