From 24797055570e351b52d3a0029e0a9d130bd0673f Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 19 Aug 2026 12:21:59 -0700 Subject: [PATCH 1/3] fix(tools): give internal routes transport headroom past their execution budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `timeout` param bounds the work an internal route was asked to do — the code a sandbox runs, the upstream call a proxy route makes. The fetch around it also pays authentication, body parsing, workspace authorization, worker acquisition, and response serialization, none of which that budget was sized for. Arming the client with the bare number made the caller give up at the same instant the route's own deadline fired, so the route could never win the race and report which part actually ran long — the caller saw an unattributable `Request timed out` instead of `Function execution timed out after 5000ms`. Add 30s of headroom, sized above the isolated-vm worker's own 10s startup budget so a cold worker spawn stays inside the transport deadline rather than aborting it. An execution abort signal, when present, still bounds the call. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/tools/index.test.ts | 47 ++++++++++++++++++++++++++++++++++++ apps/sim/tools/index.ts | 26 +++++++++++++++++--- 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/apps/sim/tools/index.test.ts b/apps/sim/tools/index.test.ts index bd8a5f3d9ea..c5e9b541799 100644 --- a/apps/sim/tools/index.test.ts +++ b/apps/sim/tools/index.test.ts @@ -2323,6 +2323,53 @@ describe('executeTool Function', () => { tools.function_execute = originalFunctionTool }) + it('gives an internal route transport headroom past its requested execution budget', async () => { + const originalFunctionTool = { ...tools.function_execute } + tools.function_execute = { + ...tools.function_execute, + transformResponse: vi.fn().mockResolvedValue({ success: true, output: {} }), + } + + let observedSignal: AbortSignal | undefined + global.fetch = Object.assign( + vi.fn().mockImplementation( + async (_url: string, init: RequestInit) => + new Promise((_resolve, reject) => { + observedSignal = init.signal as AbortSignal + observedSignal.addEventListener('abort', () => { + const err = new Error('aborted') + err.name = 'AbortError' + reject(err) + }) + }) + ), + { preconnect: vi.fn() } + ) as typeof fetch + + vi.useFakeTimers() + try { + const resultPromise = executeTool( + 'function_execute', + { code: 'return 1', timeout: 5000 }, + { skipPostProcess: true } + ) + + // The route owns the 5s execution budget and needs to outlive it to report + // its own timeout, so the transport must still be waiting at that instant. + await vi.advanceTimersByTimeAsync(5000) + expect(observedSignal?.aborted).toBe(false) + + await vi.advanceTimersByTimeAsync(30_000) + const result = await resultPromise + + expect(result.success).toBe(false) + expect(result.error).toMatch(/timed out after 35000ms/) + } finally { + vi.useRealTimers() + tools.function_execute = originalFunctionTool + } + }) + it('should add timing information to results', async () => { const result = await executeTool( 'http_request', diff --git a/apps/sim/tools/index.ts b/apps/sim/tools/index.ts index 7bcae647256..b2c6ce262d4 100644 --- a/apps/sim/tools/index.ts +++ b/apps/sim/tools/index.ts @@ -953,6 +953,24 @@ import { normalizeToolId } from '@/tools/normalize' const MAX_REQUEST_BODY_SIZE_BYTES = 10 * 1024 * 1024 // 10MB const MAX_TOOL_RESPONSE_BODY_BYTES = 10 * 1024 * 1024 // 10MB +/** + * Headroom added to an internal route's requested timeout before it becomes the + * transport deadline. + * + * A `timeout` param bounds the work the route was asked to do — the code a + * sandbox runs, the upstream call a proxy route makes. The fetch around it also + * pays authentication, body parsing, workspace authorization, worker + * acquisition, and response serialization, none of which that budget was sized + * for. Arming the client with the bare number makes the caller give up at the + * same instant the route's own deadline fires, so the route can never win the + * race and report which part actually ran long; the caller sees an + * unattributable `Request timed out` instead. + * + * Sized above the isolated-vm worker's own 10s startup budget so a cold worker + * spawn stays inside the transport deadline rather than aborting it. + */ +const INTERNAL_ROUTE_TRANSPORT_OVERHEAD_MS = 30_000 + /** * User-friendly error message for body size limit exceeded */ @@ -2540,9 +2558,11 @@ async function executeToolRequest( let didTimeout = false // With a caller/execution abort signal present, the plan-based timeout bounds the call and // this only acts as a ceiling; without one, keep the tighter default as the hang safety net. - const timeout = - requestParams.timeout || - (signal ? getMaxExecutionTimeout() : DEFAULT_EXECUTION_TIMEOUT_MS) + const timeout = requestParams.timeout + ? requestParams.timeout + INTERNAL_ROUTE_TRANSPORT_OVERHEAD_MS + : signal + ? getMaxExecutionTimeout() + : DEFAULT_EXECUTION_TIMEOUT_MS const timeoutId = setTimeout(() => { didTimeout = true controller.abort(new DOMException('timeout', 'AbortError')) From 448382504f882973aed6ca1829541396c1beb590 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 19 Aug 2026 12:22:10 -0700 Subject: [PATCH 2/3] improvement(executor): evaluate a condition list in one sandbox call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A condition block spent one `function_execute` round trip per branch, so a four-branch block that fell through to `else` paid four sandbox executions before routing. Build one script that tests each expression in order and returns the index of the first truthy one. Ordering and short-circuiting are unchanged: an expression is only reached once every earlier one returned falsy, so a later expression that throws is still never reached and the run takes the same branch it took before. The script's `catch` reports the index it was on as data rather than rethrowing, which is what lets the handler still name the failing branch in its error. A batch that produces no verdict falls back to one call per branch — the path this handler used before. That is load-bearing rather than redundant: a syntax error anywhere in the list fails the whole script at parse time, while evaluating one at a time only reaches, and so only fails on, the branches the run actually takes. A timed-out or cancelled batch skips the fallback, which would otherwise re-run every branch against the same stall. An unrecognized reply is treated as no verdict rather than as "nothing matched", so a garbled response cannot silently route the run down the else path. Co-Authored-By: Claude Opus 5 (1M context) --- .../condition/condition-handler.test.ts | 220 +++++++++-- .../handlers/condition/condition-handler.ts | 353 ++++++++++++++---- 2 files changed, 458 insertions(+), 115 deletions(-) diff --git a/apps/sim/executor/handlers/condition/condition-handler.test.ts b/apps/sim/executor/handlers/condition/condition-handler.test.ts index fa24cec1bfa..85bb1958c83 100644 --- a/apps/sim/executor/handlers/condition/condition-handler.test.ts +++ b/apps/sim/executor/handlers/condition/condition-handler.test.ts @@ -24,6 +24,17 @@ import { executeTool } from '@/tools' const mockExecuteTool = executeTool as ReturnType const mockCollectBlockData = collectBlockData as ReturnType + +/** The handler evaluates every testable branch in one call, so a whole condition list resolves to a single verdict. */ +const matchedAt = (index: number) => ({ + success: true, + output: { result: { matchedIndex: index } }, +}) +const noMatch = () => ({ success: true, output: { result: { matchedIndex: -1 } } }) +const threwAt = (index: number, message: string) => ({ + success: true, + output: { result: { matchedIndex: -1, threwAtIndex: index, message } }, +}) const mockConditionLogger = vi.mocked(loggerMock.createLogger).mock.results[ vi .mocked(loggerMock.createLogger) @@ -123,8 +134,8 @@ describe('ConditionBlockHandler', () => { vi.clearAllMocks() - // Default: condition evaluates to false (else path). Individual tests override with mockResolvedValueOnce. - mockExecuteTool.mockResolvedValue({ success: true, output: { result: false } }) + // Default: no branch matches (else path). Individual tests override with mockResolvedValueOnce. + mockExecuteTool.mockResolvedValue(noMatch()) }) it('should handle condition blocks', () => { @@ -135,7 +146,7 @@ describe('ConditionBlockHandler', () => { it('should execute condition block correctly and select first path', async () => { // Mock executeTool to return true for the condition - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [ { id: 'cond1', title: 'if', value: 'context.value > 5' }, @@ -162,7 +173,7 @@ describe('ConditionBlockHandler', () => { }) it('should pass correct parameters to function_execute tool', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [ { id: 'cond1', title: 'if', value: 'context.value > 5' }, @@ -195,7 +206,7 @@ describe('ConditionBlockHandler', () => { blockData: { 'huge-block': { payload: 'x'.repeat(1024) } }, blockNameMapping: { hugeblock: 'huge-block' }, }) - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [ { id: 'cond1', title: 'if', value: 'true' }, @@ -209,7 +220,7 @@ describe('ConditionBlockHandler', () => { }) it('should select the else path if other conditions fail', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'context.value < 0' }, @@ -247,7 +258,7 @@ describe('ConditionBlockHandler', () => { }) it('finds whitespace and mixed-case else branches during fallback', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'false' }, @@ -282,7 +293,7 @@ describe('ConditionBlockHandler', () => { it('should handle evaluation errors gracefully', async () => { const secret = 'condition-runtime-secret-value' - mockExecuteTool.mockResolvedValueOnce({ + mockExecuteTool.mockResolvedValue({ success: false, error: `Cannot read ${secret} through __var_API_KEY`, }) @@ -300,8 +311,25 @@ describe('ConditionBlockHandler', () => { expect(JSON.stringify(mockConditionLogger.error.mock.calls)).not.toContain('__var_API_KEY') }) + it('names the branch an expression threw on without leaking the failure into logs', async () => { + const secret = 'condition-throw-secret-value' + mockExecuteTool.mockResolvedValueOnce(threwAt(1, `Cannot read ${secret}`)) + + const conditions = [ + { id: 'cond1', title: 'if', value: 'context.value > 5' }, + { id: 'cond2', title: 'else if', value: 'context.missing.deep()' }, + { id: 'else1', title: 'else', value: '' }, + ] + + await expect( + handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(conditions) }) + ).rejects.toThrow(/Evaluation error in condition "else if": Cannot read/) + expect(mockExecuteTool).toHaveBeenCalledOnce() + expect(JSON.stringify(mockConditionLogger.error.mock.calls)).not.toContain(secret) + }) + it('should handle missing source block output gracefully', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [{ id: 'cond1', title: 'if', value: 'true' }] const inputs = { conditions: JSON.stringify(conditions) } @@ -318,7 +346,7 @@ describe('ConditionBlockHandler', () => { }) it('should throw error if target block is missing', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [{ id: 'cond1', title: 'if', value: 'true' }] const inputs = { conditions: JSON.stringify(conditions) } @@ -331,8 +359,7 @@ describe('ConditionBlockHandler', () => { }) it('should return no-match result if no condition matches and no else exists', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'false' }, @@ -358,7 +385,7 @@ describe('ConditionBlockHandler', () => { }) it('falls back to else path when loop context data is unavailable', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'context.item === "apple"' }, @@ -373,7 +400,7 @@ describe('ConditionBlockHandler', () => { }) it('should use collectBlockData to gather block state', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [ { id: 'cond1', title: 'if', value: 'true' }, @@ -403,9 +430,127 @@ describe('ConditionBlockHandler', () => { ) }) + describe('Batched evaluation', () => { + const manyConditions = [ + { id: 'cond1', title: 'if', value: 'context.value === 1' }, + { id: 'cond2', title: 'else if', value: 'context.value === 2' }, + { id: 'cond3', title: 'else if', value: 'context.value === 3' }, + { id: 'cond4', title: 'else if', value: 'context.value === 4' }, + { id: 'else1', title: 'else', value: '' }, + ] + + it('evaluates every testable branch in a single call', async () => { + mockExecuteTool.mockResolvedValueOnce(noMatch()) + + await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify(manyConditions), + }) + + expect(mockExecuteTool).toHaveBeenCalledOnce() + }) + + it('tests the expressions in declaration order and stops at the first truthy one', async () => { + mockExecuteTool.mockResolvedValueOnce(noMatch()) + + await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify(manyConditions), + }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + const code = toolParams.code as string + const positions = manyConditions.slice(0, 4).map((condition) => code.indexOf(condition.value)) + + expect(positions.every((position) => position >= 0)).toBe(true) + expect(positions).toEqual([...positions].sort((a, b) => a - b)) + // The else branch carries no expression and must never reach the sandbox. + expect(code).toContain('return { matchedIndex: -1 }') + }) + + it('never sends the else branch for evaluation', async () => { + mockExecuteTool.mockResolvedValueOnce(noMatch()) + + await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify([ + { id: 'cond1', title: 'if', value: 'context.value === 1' }, + { id: 'else1', title: 'else', value: 'SHOULD_NEVER_BE_EVALUATED' }, + { id: 'cond2', title: 'else if', value: 'context.value === 2' }, + ]), + }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + expect(toolParams.code).not.toContain('SHOULD_NEVER_BE_EVALUATED') + expect(toolParams.code).not.toContain('context.value === 2') + }) + + it('re-evaluates one branch at a time when the batch returns no verdict', async () => { + // A syntax error anywhere fails the whole script at parse time, so the + // fallback must still take the branch an earlier condition matches. + mockExecuteTool.mockResolvedValueOnce({ + success: false, + error: 'SyntaxError: Unexpected identifier', + }) + mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + + const result = await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify(manyConditions), + }) + + expect((result as any).selectedOption).toBe('cond1') + expect(mockExecuteTool).toHaveBeenCalledTimes(2) + }) + + it('does not fan a timed-out batch out into one call per branch', async () => { + mockExecuteTool.mockResolvedValue({ + success: false, + error: 'Request timed out after 5000ms', + }) + + await expect( + handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(manyConditions) }) + ).rejects.toThrow(/Evaluation error in condition "if".*Request timed out/) + expect(mockExecuteTool).toHaveBeenCalledOnce() + }) + + it('falls back when the batch reports a branch index outside the list', async () => { + mockExecuteTool.mockResolvedValueOnce(matchedAt(99)) + mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + + const result = await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify(manyConditions), + }) + + expect((result as any).selectedOption).toBe('cond1') + expect(mockExecuteTool).toHaveBeenCalledTimes(2) + }) + + it('does not take the else path on a reply that carries no verdict', async () => { + // Reading a garbled reply as "nothing matched" would silently reroute the + // run, so an unrecognized shape has to fall back rather than fall through. + mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: { ok: true } } }) + mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + + const result = await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify(manyConditions), + }) + + expect((result as any).selectedOption).toBe('cond1') + expect(mockExecuteTool).toHaveBeenCalledTimes(2) + }) + + it('does not retry per branch once the run has been cancelled', async () => { + mockContext.abortSignal = AbortSignal.abort() + mockExecuteTool.mockResolvedValue({ success: false, error: 'Execution cancelled' }) + + await expect( + handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(manyConditions) }) + ).rejects.toThrow(/Evaluation error in condition "if".*Execution cancelled/) + expect(mockExecuteTool).toHaveBeenCalledOnce() + }) + }) + describe('Multiple branches to same target', () => { it('should handle if and else pointing to same target', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [ { id: 'cond1', title: 'if', value: 'context.value > 5' }, @@ -431,7 +576,7 @@ describe('ConditionBlockHandler', () => { }) it('should select else branch to same target when if fails', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'context.value < 0' }, @@ -457,10 +602,8 @@ describe('ConditionBlockHandler', () => { }) it('should handle if→A, elseif→B, else→A pattern', async () => { - // First condition (cond1): false - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) - // Second condition (cond2): false - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + // Neither cond1 nor cond2 matches, so the else branch wins. + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'context.value === 1' }, @@ -486,7 +629,7 @@ describe('ConditionBlockHandler', () => { describe('Condition evaluation with different data types', () => { it('should evaluate string comparison conditions', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { name: 'test', status: 'active' }, @@ -506,7 +649,7 @@ describe('ConditionBlockHandler', () => { }) it('should evaluate boolean conditions', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { isEnabled: true, count: 5 }, @@ -526,7 +669,7 @@ describe('ConditionBlockHandler', () => { }) it('should evaluate array length conditions', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { items: [1, 2, 3, 4, 5] }, @@ -546,7 +689,7 @@ describe('ConditionBlockHandler', () => { }) it('should evaluate null/undefined check conditions', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { data: null }, @@ -568,8 +711,7 @@ describe('ConditionBlockHandler', () => { describe('Multiple else-if conditions', () => { it('should evaluate multiple else-if conditions in order', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(1)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { score: 75 }, @@ -612,9 +754,7 @@ describe('ConditionBlockHandler', () => { }) it('should skip to else when all else-if fail', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { score: 30 }, @@ -638,7 +778,7 @@ describe('ConditionBlockHandler', () => { describe('Condition with no outgoing edge', () => { it('should set selectedOption when condition matches but has no edge', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [ { id: 'cond1', title: 'if', value: 'true' }, @@ -660,7 +800,7 @@ describe('ConditionBlockHandler', () => { }) it('should set selectedOption when else is selected but has no edge', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'false' }, @@ -682,7 +822,7 @@ describe('ConditionBlockHandler', () => { }) it('should deactivate if-path when else is selected with no edge', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const conditions = [ { id: 'cond1', title: 'if', value: 'context.value > 100' }, @@ -715,7 +855,7 @@ describe('ConditionBlockHandler', () => { }) it('should handle conditions passed as array directly', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const conditions = [ { id: 'cond1', title: 'if', value: 'true' }, @@ -731,7 +871,7 @@ describe('ConditionBlockHandler', () => { describe('Source output filtering', () => { it('should not propagate error field from source block output', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { value: 10, text: 'hello', error: 'upstream block failed' }, @@ -753,7 +893,7 @@ describe('ConditionBlockHandler', () => { }) it('should not propagate _pauseMetadata from source block output', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { value: 10, _pauseMetadata: { contextId: 'abc' } }, @@ -774,7 +914,7 @@ describe('ConditionBlockHandler', () => { }) it('should still pass through non-control fields from source output', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) ;(mockContext.blockStates as any).set(mockSourceBlock.id, { output: { value: 10, text: 'hello', customData: { nested: true } }, @@ -798,7 +938,7 @@ describe('ConditionBlockHandler', () => { describe('Virtual block ID handling', () => { it('should use currentVirtualBlockId for decision key when available', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) mockContext.currentVirtualBlockId = 'virtual-block-123' @@ -817,7 +957,7 @@ describe('ConditionBlockHandler', () => { describe('Parallel branch handling', () => { it('should resolve connections and block data correctly when inside a parallel branch', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const parallelConditionBlock: SerializedBlock = { id: 'cond-block-1₍0₎', @@ -897,7 +1037,7 @@ describe('ConditionBlockHandler', () => { }) it('should find correct source block output in parallel branch context', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) const parallelConditionBlock: SerializedBlock = { id: 'cond-block-1₍1₎', @@ -969,7 +1109,7 @@ describe('ConditionBlockHandler', () => { }) it('should fall back to else when condition is false in parallel branch', async () => { - mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: false } }) + mockExecuteTool.mockResolvedValueOnce(noMatch()) const parallelConditionBlock: SerializedBlock = { id: 'cond-block-1₍2₎', diff --git a/apps/sim/executor/handlers/condition/condition-handler.ts b/apps/sim/executor/handlers/condition/condition-handler.ts index d21702ae81f..418cae900fb 100644 --- a/apps/sim/executor/handlers/condition/condition-handler.ts +++ b/apps/sim/executor/handlers/condition/condition-handler.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { getErrorMessage, toError } from '@sim/utils/errors' import { normalizeStringRecord, normalizeWorkflowVariables } from '@/lib/core/utils/records' import { isElseConditionTitle } from '@/lib/workflows/conditions' import type { BlockOutput } from '@/blocks/types' @@ -13,72 +14,214 @@ import { } from '@/executor/utils/subflow-utils' import type { SerializedBlock } from '@/serializer/types' import { executeTool } from '@/tools' +import type { ToolResponse } from '@/tools/types' const logger = createLogger('ConditionBlockHandler') const CONDITION_TIMEOUT_MS = 5000 +interface ConditionEntry { + id: string + title: string + value: string +} + +/** Verdict for a whole condition list evaluated in one function execution. */ +type ConditionEvaluation = + | { status: 'matched'; index: number } + | { status: 'no-match' } + | { status: 'expression-threw'; index: number; message: string } + | { status: 'no-verdict'; message: string; timedOut: boolean } + /** - * Evaluates a single condition expression. - * The resolver preserves legacy Condition expression substitution before this function executes the - * resulting JavaScript through the shared function execution boundary. + * Builds one script that tests each expression in order and reports the index of + * the first truthy one. * - * `blockData` is deliberately empty: the resolver already inlines every `` reference - * into the expression before this runs, so shipping the run's accumulated block outputs would only - * inflate the request body. Sending them blew the 10MB body cap on wide subflows, where a single - * flat `blockStates` map holds every branch's outputs. + * Ordering and short-circuiting match evaluating the expressions one call at a + * time: an expression is only reached once every earlier one returned falsy, so + * a later expression that throws is still never reached and the run takes the + * same branch it takes today. The `catch` reports the index it was on as data + * rather than rethrowing, which is what lets the caller name the failing branch. + * + * Each expression sits on its own line inside `Boolean(...)` so a trailing line + * comment ends before the closing parenthesis instead of swallowing it. + */ +function buildConditionScript(expressions: string[], evalContext: Record): string { + const tests = expressions + .map( + (expression, index) => + ` __simConditionIndex = ${index}\n` + + ` if (Boolean(\n${expression}\n )) return { matchedIndex: ${index} }` + ) + .join('\n') + + return [ + `const context = ${JSON.stringify(evalContext)};`, + 'let __simConditionIndex = -1', + 'try {', + tests, + ' return { matchedIndex: -1 }', + '} catch (__simConditionError) {', + ' return {', + ' matchedIndex: -1,', + ' threwAtIndex: __simConditionIndex,', + ' message:', + ' __simConditionError && __simConditionError.message', + ' ? String(__simConditionError.message)', + ' : String(__simConditionError),', + ' }', + '}', + ].join('\n') +} + +/** + * Runs condition code through the shared function execution boundary. * - * Returns true if condition is met, false otherwise. + * `blockData` is deliberately empty: the resolver already inlines every + * `` reference into the expression before this runs, so shipping + * the run's accumulated block outputs would only inflate the request body. + * Sending them blew the 10MB body cap on wide subflows, where a single flat + * `blockStates` map holds every branch's outputs. */ -async function evaluateConditionExpression( +async function runConditionCode( ctx: ExecutionContext, - conditionExpression: string, - providedEvalContext?: Record, + code: string, currentNodeId?: string -): Promise { - const evalContext = providedEvalContext || {} - - try { - const contextSetup = `const context = ${JSON.stringify(evalContext)};` - const code = `${contextSetup}\nreturn Boolean(${conditionExpression})` - - const { blockNameMapping, blockOutputSchemas } = collectBlockData(ctx, currentNodeId) - - const result = await executeTool( - 'function_execute', - { - code, - timeout: CONDITION_TIMEOUT_MS, - envVars: normalizeStringRecord(ctx.environmentVariables), - workflowVariables: normalizeWorkflowVariables(ctx.workflowVariables), - blockData: {}, - blockNameMapping, - blockOutputSchemas, - _context: { - workflowId: ctx.workflowId, - workspaceId: ctx.workspaceId, - userId: ctx.userId, - isDeployedContext: ctx.isDeployedContext, - enforceCredentialAccess: ctx.enforceCredentialAccess, - }, +): Promise { + const { blockNameMapping, blockOutputSchemas } = collectBlockData(ctx, currentNodeId) + + return executeTool( + 'function_execute', + { + code, + timeout: CONDITION_TIMEOUT_MS, + envVars: normalizeStringRecord(ctx.environmentVariables), + workflowVariables: normalizeWorkflowVariables(ctx.workflowVariables), + blockData: {}, + blockNameMapping, + blockOutputSchemas, + _context: { + workflowId: ctx.workflowId, + workspaceId: ctx.workspaceId, + userId: ctx.userId, + isDeployedContext: ctx.isDeployedContext, + enforceCredentialAccess: ctx.enforceCredentialAccess, }, - { executionContext: ctx } - ) + }, + { executionContext: ctx } + ) +} - if (!result.success) { - logger.error('Failed to evaluate condition', { - hasRuntimeError: Boolean(result.error), - }) - throw new Error(`Evaluation error in condition: ${result.error}`) +/** + * A failed batch normally falls back to one call per condition, which is exactly + * what this handler did before batching. A timeout is the one failure where that + * is the wrong move: the fallback would re-run every remaining condition against + * the same stalled transport, turning one slow call into as many slow calls as + * there are branches. Classified by message because the transport reports giving + * up as text, the same way `isRetryableFailure` does in `@/tools`. + */ +function isTimeoutFailure(error: string | undefined): boolean { + if (!error) return false + const message = error.toLowerCase() + return message.includes('timed out') || message.includes('timeout') +} + +/** Evaluates the whole condition list in a single function execution. */ +async function evaluateConditionList( + ctx: ExecutionContext, + expressions: string[], + evalContext: Record, + currentNodeId?: string +): Promise { + const result = await runConditionCode( + ctx, + buildConditionScript(expressions, evalContext), + currentNodeId + ) + + if (!result.success) { + const message = result.error ?? 'Condition evaluation failed' + return { status: 'no-verdict', message, timedOut: isTimeoutFailure(result.error) } + } + + const output = result.output?.result + if (!output || typeof output !== 'object') { + return { + status: 'no-verdict', + message: 'Condition evaluation returned no verdict', + timedOut: false, + } + } + + const { matchedIndex, threwAtIndex, message } = output as { + matchedIndex?: unknown + threwAtIndex?: unknown + message?: unknown + } + + if (typeof threwAtIndex === 'number' && threwAtIndex >= 0) { + if (threwAtIndex >= expressions.length) { + return { + status: 'no-verdict', + message: 'Condition evaluation reported an unknown branch', + timedOut: false, + } + } + return { + status: 'expression-threw', + index: threwAtIndex, + message: typeof message === 'string' && message ? message : 'Unknown evaluation error', + } + } + + // The script always reports a `matchedIndex`, so anything else is a response + // this handler did not produce. Treating that as "no branch matched" would + // silently route the run down the else path on a garbled reply. + if (typeof matchedIndex !== 'number') { + return { + status: 'no-verdict', + message: 'Condition evaluation returned an unrecognized verdict', + timedOut: false, } + } + if (matchedIndex < 0) { + return { status: 'no-match' } + } + if (matchedIndex >= expressions.length) { + return { + status: 'no-verdict', + message: 'Condition evaluation matched an unknown branch', + timedOut: false, + } + } - return Boolean(result.output?.result) - } catch (evalError: any) { - logger.error('Failed to evaluate condition', { - errorName: evalError?.name, - }) - throw new Error(`Evaluation error in condition: ${evalError.message}`) + return { status: 'matched', index: matchedIndex } +} + +/** + * Evaluates one expression in its own call. Kept as the fallback for a batch + * that produced no verdict: a syntax error anywhere in the list fails the whole + * script at parse time, while evaluating one at a time only reaches — and so + * only fails on — the branches the run actually takes. + */ +async function evaluateSingleCondition( + ctx: ExecutionContext, + expression: string, + evalContext: Record, + currentNodeId?: string +): Promise { + const code = `const context = ${JSON.stringify(evalContext)};\nreturn Boolean(${expression})` + const result = await runConditionCode(ctx, code, currentNodeId) + + if (!result.success) { + throw new Error(result.error ?? 'Condition evaluation failed') } + + return Boolean(result.output?.result) +} + +function conditionError(condition: ConditionEntry, message: string): Error { + return new Error(`Evaluation error in condition "${condition.title}": ${message}`) } /** @@ -175,7 +318,7 @@ export class ConditionBlockHandler implements BlockHandler { return rest } - private parseConditions(input: any): Array<{ id: string; title: string; value: string }> { + private parseConditions(input: any): ConditionEntry[] { try { const conditions = Array.isArray(input) ? input : JSON.parse(input || '[]') return conditions @@ -208,48 +351,108 @@ export class ConditionBlockHandler implements BlockHandler { return evalContext } + /** + * An else branch wins as soon as it is reached, so only the branches ahead of + * it are ever testable — matching the original loop, which returned on the + * first else it walked past rather than assuming else comes last. + */ private async evaluateConditions( - conditions: Array<{ id: string; title: string; value: string }>, + conditions: ConditionEntry[], outgoingConnections: Array<{ source: string; target: string; sourceHandle?: string }>, evalContext: Record, ctx: ExecutionContext, currentNodeId?: string ): Promise<{ selectedConnection: { target: string; sourceHandle?: string } | null - selectedCondition: { id: string; title: string; value: string } | null + selectedCondition: ConditionEntry | null }> { - for (const condition of conditions) { - if (isElseConditionTitle(condition.title)) { - const connection = this.findConnectionForCondition(outgoingConnections, condition.id) - return { selectedConnection: connection ?? null, selectedCondition: condition } + const elseIndex = conditions.findIndex((condition) => isElseConditionTitle(condition.title)) + const testable = elseIndex === -1 ? conditions : conditions.slice(0, elseIndex) + const elseCondition = elseIndex === -1 ? null : conditions[elseIndex] + + const matched = await this.findMatchingCondition(testable, evalContext, ctx, currentNodeId) + const selectedCondition = matched ?? elseCondition + + if (!selectedCondition) { + return { selectedConnection: null, selectedCondition: null } + } + + return { + selectedConnection: + this.findConnectionForCondition(outgoingConnections, selectedCondition.id) ?? null, + selectedCondition, + } + } + + private async findMatchingCondition( + conditions: ConditionEntry[], + evalContext: Record, + ctx: ExecutionContext, + currentNodeId?: string + ): Promise { + if (conditions.length === 0) return null + + const expressions = conditions.map((condition) => String(condition.value || '')) + + let evaluation: ConditionEvaluation + try { + evaluation = await evaluateConditionList(ctx, expressions, evalContext, currentNodeId) + } catch (error) { + evaluation = { + status: 'no-verdict', + message: getErrorMessage(error, 'Condition evaluation failed'), + timedOut: false, } + } + + switch (evaluation.status) { + case 'matched': + return conditions[evaluation.index] + case 'no-match': + return null + case 'expression-threw': + logger.error('Failed to evaluate condition', { conditionCount: conditions.length }) + throw conditionError(conditions[evaluation.index], evaluation.message) + case 'no-verdict': + // Retrying one branch at a time is what recovers a batch the sandbox + // could not parse. It is the wrong move for a stalled transport, which + // would re-run every branch against the same stall, or for a cancelled + // run, where every retry aborts on arrival — both surface the batch + // failure as it stands. The whole list was one call, so no single + // branch owns that failure; name the first, where evaluation started. + if (evaluation.timedOut || ctx.abortSignal?.aborted) { + logger.error('Failed to evaluate conditions', { conditionCount: conditions.length }) + throw conditionError(conditions[0], evaluation.message) + } + logger.warn('Batched condition evaluation produced no verdict, retrying one at a time', { + conditionCount: conditions.length, + }) + return this.findMatchingConditionIndividually(conditions, evalContext, ctx, currentNodeId) + } + } - const conditionValueString = String(condition.value || '') + private async findMatchingConditionIndividually( + conditions: ConditionEntry[], + evalContext: Record, + ctx: ExecutionContext, + currentNodeId?: string + ): Promise { + for (const condition of conditions) { try { - const conditionMet = await evaluateConditionExpression( + const conditionMet = await evaluateSingleCondition( ctx, - conditionValueString, + String(condition.value || ''), evalContext, currentNodeId ) - - if (conditionMet) { - const connection = this.findConnectionForCondition(outgoingConnections, condition.id) - if (connection) { - return { selectedConnection: connection, selectedCondition: condition } - } - return { selectedConnection: null, selectedCondition: condition } - } - } catch (error: any) { - logger.error('Failed to evaluate condition', { - errorName: error?.name, - hasTitle: typeof condition.title === 'string' && condition.title.length > 0, - }) - throw new Error(`Evaluation error in condition "${condition.title}": ${error.message}`) + if (conditionMet) return condition + } catch (error) { + logger.error('Failed to evaluate condition', { errorName: toError(error).name }) + throw conditionError(condition, getErrorMessage(error, 'Condition evaluation failed')) } } - return { selectedConnection: null, selectedCondition: null } + return null } private findConnectionForCondition( From 3c3ad93c9f1eaefd32c5e2c621aea39d0d5a2e8a Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 19 Aug 2026 12:38:32 -0700 Subject: [PATCH 3/3] fix(executor): wrap condition expressions the same way in both paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batched script put each expression on its own line inside `Boolean(...)` so a trailing line comment ended before the closing parenthesis; the per-branch fallback still inlined `Boolean(${expression})` on one line. That made the recovery path stricter than the path it recovers — a batch that failed to parse because of a later branch would fall back and then reject an earlier comment-bearing branch it should have matched. Both paths now wrap through `buildBooleanTest`, so they cannot drift again. Also narrows the evaluation-context boundary from `Record` to `Record`; the context is only ever serialized, never indexed. Co-Authored-By: Claude Opus 5 (1M context) --- .../condition/condition-handler.test.ts | 39 +++++++++++++++++++ .../handlers/condition/condition-handler.ts | 36 ++++++++++------- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/apps/sim/executor/handlers/condition/condition-handler.test.ts b/apps/sim/executor/handlers/condition/condition-handler.test.ts index 85bb1958c83..42e1890159f 100644 --- a/apps/sim/executor/handlers/condition/condition-handler.test.ts +++ b/apps/sim/executor/handlers/condition/condition-handler.test.ts @@ -499,6 +499,45 @@ describe('ConditionBlockHandler', () => { expect(mockExecuteTool).toHaveBeenCalledTimes(2) }) + it('emits a batch script a trailing line comment cannot break', async () => { + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) + + await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify([ + { id: 'cond1', title: 'if', value: 'context.value > 5 // gate' }, + { id: 'else1', title: 'else', value: '' }, + ]), + }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + // Compiling is the assertion: a comment that swallowed the closing + // parenthesis would fail the whole script at parse time. + expect(() => new Function(toolParams.code as string)).not.toThrow() + }) + + it('wraps a comment-bearing expression the same way when falling back', async () => { + // The fallback recovers a batch the sandbox could not parse, so it has to + // accept every expression the batch accepts — otherwise the recovery path + // rejects a branch the primary path would have matched. + mockExecuteTool.mockResolvedValueOnce({ + success: false, + error: 'Invalid JavaScript syntax: Unexpected token', + }) + mockExecuteTool.mockResolvedValueOnce({ success: true, output: { result: true } }) + + const result = await handler.execute(mockContext, mockBlock, { + conditions: JSON.stringify([ + { id: 'cond1', title: 'if', value: 'context.value > 5 // gate' }, + { id: 'cond2', title: 'else if', value: 'context.value ===' }, + { id: 'else1', title: 'else', value: '' }, + ]), + }) + + expect((result as any).selectedOption).toBe('cond1') + const [, fallbackParams] = mockExecuteTool.mock.calls[1] + expect(() => new Function(fallbackParams.code as string)).not.toThrow() + }) + it('does not fan a timed-out batch out into one call per branch', async () => { mockExecuteTool.mockResolvedValue({ success: false, diff --git a/apps/sim/executor/handlers/condition/condition-handler.ts b/apps/sim/executor/handlers/condition/condition-handler.ts index 418cae900fb..671fbc73eca 100644 --- a/apps/sim/executor/handlers/condition/condition-handler.ts +++ b/apps/sim/executor/handlers/condition/condition-handler.ts @@ -33,6 +33,19 @@ type ConditionEvaluation = | { status: 'expression-threw'; index: number; message: string } | { status: 'no-verdict'; message: string; timedOut: boolean } +/** + * Wraps one expression as a boolean test, on its own line so a trailing line + * comment ends before the closing parenthesis instead of swallowing it. + * + * The batched script and the per-branch fallback both wrap through here. Wrapping + * them separately let the two drift: an expression carrying a trailing comment + * parsed in the batch and failed in the fallback, so the recovery path rejected + * a branch the primary path accepted. + */ +function buildBooleanTest(expression: string): string { + return `Boolean(\n${expression}\n)` +} + /** * Builds one script that tests each expression in order and reports the index of * the first truthy one. @@ -42,16 +55,13 @@ type ConditionEvaluation = * a later expression that throws is still never reached and the run takes the * same branch it takes today. The `catch` reports the index it was on as data * rather than rethrowing, which is what lets the caller name the failing branch. - * - * Each expression sits on its own line inside `Boolean(...)` so a trailing line - * comment ends before the closing parenthesis instead of swallowing it. */ -function buildConditionScript(expressions: string[], evalContext: Record): string { +function buildConditionScript(expressions: string[], evalContext: Record): string { const tests = expressions .map( (expression, index) => ` __simConditionIndex = ${index}\n` + - ` if (Boolean(\n${expression}\n )) return { matchedIndex: ${index} }` + ` if (${buildBooleanTest(expression)}) return { matchedIndex: ${index} }` ) .join('\n') @@ -130,7 +140,7 @@ function isTimeoutFailure(error: string | undefined): boolean { async function evaluateConditionList( ctx: ExecutionContext, expressions: string[], - evalContext: Record, + evalContext: Record, currentNodeId?: string ): Promise { const result = await runConditionCode( @@ -207,10 +217,10 @@ async function evaluateConditionList( async function evaluateSingleCondition( ctx: ExecutionContext, expression: string, - evalContext: Record, + evalContext: Record, currentNodeId?: string ): Promise { - const code = `const context = ${JSON.stringify(evalContext)};\nreturn Boolean(${expression})` + const code = `const context = ${JSON.stringify(evalContext)};\nreturn ${buildBooleanTest(expression)}` const result = await runConditionCode(ctx, code, currentNodeId) if (!result.success) { @@ -335,8 +345,8 @@ export class ConditionBlockHandler implements BlockHandler { private buildEvaluationContext( ctx: ExecutionContext, sourceBlockId?: string - ): Record { - let evalContext: Record = {} + ): Record { + let evalContext: Record = {} if (sourceBlockId) { const sourceOutput = ctx.blockStates.get(sourceBlockId)?.output @@ -359,7 +369,7 @@ export class ConditionBlockHandler implements BlockHandler { private async evaluateConditions( conditions: ConditionEntry[], outgoingConnections: Array<{ source: string; target: string; sourceHandle?: string }>, - evalContext: Record, + evalContext: Record, ctx: ExecutionContext, currentNodeId?: string ): Promise<{ @@ -386,7 +396,7 @@ export class ConditionBlockHandler implements BlockHandler { private async findMatchingCondition( conditions: ConditionEntry[], - evalContext: Record, + evalContext: Record, ctx: ExecutionContext, currentNodeId?: string ): Promise { @@ -433,7 +443,7 @@ export class ConditionBlockHandler implements BlockHandler { private async findMatchingConditionIndividually( conditions: ConditionEntry[], - evalContext: Record, + evalContext: Record, ctx: ExecutionContext, currentNodeId?: string ): Promise {