Skip to content

Commit d3a1d6f

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(executor): preserve non-retryable tool failures
1 parent 185e93e commit d3a1d6f

3 files changed

Lines changed: 80 additions & 22 deletions

File tree

apps/sim/executor/execution/block-executor.retry.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,17 @@ import { BlockType, EDGE } from '@/executor/constants'
1010
import type { DAGNode } from '@/executor/dag/builder'
1111
import { BlockExecutor } from '@/executor/execution/block-executor'
1212
import { ExecutionState } from '@/executor/execution/state'
13+
import { GenericBlockHandler } from '@/executor/handlers/generic/generic-handler'
1314
import type { BlockHandler, ExecutionContext } from '@/executor/types'
1415
import { attachTrustedExecutionCost } from '@/executor/utils/errors'
1516
import { VariableResolver } from '@/executor/variables/resolver'
1617
import type { SerializedBlock, SerializedWorkflow } from '@/serializer/types'
18+
import { executeTool } from '@/tools'
19+
import { getTool } from '@/tools/utils'
20+
21+
vi.mock('@/blocks/index', () => ({ getBlock: vi.fn() }))
22+
vi.mock('@/tools', () => ({ executeTool: vi.fn() }))
23+
vi.mock('@/tools/utils', () => ({ getTool: vi.fn() }))
1724

1825
vi.mock('@/ee/access-control/utils/permission-check', () => ({
1926
validateBlockType: vi.fn(),
@@ -139,6 +146,40 @@ describe('BlockExecutor retry', () => {
139146
expect(ctx.blockLogs[0]?.tries).toBe(2)
140147
})
141148

149+
it.each([
150+
{ retryable: false, attempts: 1 },
151+
{ retryable: true, attempts: 3 },
152+
{ retryable: undefined, attempts: 3 },
153+
])(
154+
'executes a generic tool $attempts times when retryable is $retryable',
155+
async ({ retryable, attempts }) => {
156+
const block = createBlock(enabled)
157+
block.config.tool = 'synthetic_write'
158+
vi.mocked(getTool).mockReturnValue({
159+
id: 'synthetic_write',
160+
name: 'Synthetic Write',
161+
description: 'A synthetic write for retry testing',
162+
version: '1.0',
163+
params: {},
164+
request: { url: 'https://example.com/write', method: 'POST' },
165+
})
166+
vi.mocked(executeTool).mockResolvedValue({
167+
success: false,
168+
error: 'Write outcome is unknown',
169+
output: { detail: 'Response was lost' },
170+
...(retryable !== undefined ? { retryable } : {}),
171+
})
172+
const state = new ExecutionState()
173+
const ctx = createContext(state)
174+
const executor = buildExecutor(block, new GenericBlockHandler(), state)
175+
176+
await expect(executor.execute(ctx, createNode(block), block)).rejects.toThrow(
177+
'Write outcome is unknown'
178+
)
179+
expect(executeTool).toHaveBeenCalledTimes(attempts)
180+
}
181+
)
182+
142183
it('adds the trusted cost of failed Function tries to the successful result', async () => {
143184
const block = createBlock(enabled)
144185
const firstFailure = new Error('first attempt failed')

apps/sim/executor/handlers/generic/generic-handler.test.ts

Lines changed: 34 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import '@sim/testing/mocks/executor'
22

33
import { beforeEach, describe, expect, it, type Mock, vi } from 'vitest'
4+
import { NonRetryableExecutionError } from '@/lib/execution/non-retryable-error'
45
import { HarmonicBlock } from '@/blocks/blocks/harmonic'
56
import { KnowledgeBlock } from '@/blocks/blocks/knowledge'
67
import { getBlock } from '@/blocks/index'
@@ -593,30 +594,42 @@ describe('GenericBlockHandler', () => {
593594
expect(mockExecuteTool).not.toHaveBeenCalled()
594595
})
595596

596-
it('should handle tool execution errors correctly', async () => {
597-
const inputs = { param1: 'value' }
598-
const errorResult = {
599-
success: false,
600-
error: 'Custom tool failed',
601-
output: { detail: 'error detail' },
602-
}
603-
mockExecuteTool.mockResolvedValue(errorResult)
597+
it.each([undefined, true, false])(
598+
'preserves failure details when retryable is %s',
599+
async (retryable) => {
600+
const inputs = { param1: 'value' }
601+
const errorResult = {
602+
success: false,
603+
error: 'Custom tool failed',
604+
output: { detail: 'error detail' },
605+
statusCode: 503,
606+
...(retryable !== undefined ? { retryable } : {}),
607+
}
608+
mockExecuteTool.mockResolvedValue(errorResult)
604609

605-
await expect(handler.execute(mockContext, mockBlock, inputs)).rejects.toThrow(
606-
'Custom tool failed'
607-
)
610+
let thrown: unknown
611+
try {
612+
await handler.execute(mockContext, mockBlock, inputs)
613+
} catch (error) {
614+
thrown = error
615+
}
608616

609-
// Re-execute to check error properties after catching
610-
try {
611-
await handler.execute(mockContext, mockBlock, inputs)
612-
} catch (e: any) {
613-
expect(e.toolId).toBe('some_custom_tool')
614-
expect(e.blockName).toBe('Test Generic Block')
615-
expect(e.output).toEqual({ detail: 'error detail' })
617+
expect(thrown).toBeInstanceOf(Error)
618+
expect(thrown instanceof NonRetryableExecutionError).toBe(retryable === false)
619+
expect(thrown).toMatchObject({
620+
message: 'Custom tool failed',
621+
toolId: 'some_custom_tool',
622+
toolName: 'Some Custom Tool',
623+
blockId: 'generic-block-1',
624+
blockName: 'Test Generic Block',
625+
output: { detail: 'error detail' },
626+
statusCode: 503,
627+
timestamp: expect.any(String),
628+
...(retryable === false ? { retryable: false } : {}),
629+
})
630+
expect(mockExecuteTool).toHaveBeenCalledTimes(1)
616631
}
617-
618-
expect(mockExecuteTool).toHaveBeenCalledTimes(2) // Called twice now
619-
})
632+
)
620633

621634
it.concurrent('should handle tool execution errors with no specific message', async () => {
622635
const inputs = { param1: 'value' }

apps/sim/executor/handlers/generic/generic-handler.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { isDeepStrictEqual } from 'node:util'
22
import { createLogger } from '@sim/logger'
33
import { toError } from '@sim/utils/errors'
44
import { isPlainRecord } from '@sim/utils/object'
5+
import { NonRetryableExecutionError } from '@/lib/execution/non-retryable-error'
56
import { getBlock } from '@/blocks/index'
67
import { isMcpTool } from '@/executor/constants'
78
import type { BlockHandler, BlockNodeMetadata, ExecutionContext } from '@/executor/types'
@@ -344,7 +345,10 @@ export class GenericBlockHandler implements BlockHandler {
344345
? errorDetails.join(' - ')
345346
: `Block execution of ${tool?.name || block.config.tool} failed with no error message`
346347

347-
const error = new Error(errorMessage)
348+
const error =
349+
result.retryable === false
350+
? new NonRetryableExecutionError(errorMessage)
351+
: new Error(errorMessage)
348352

349353
Object.assign(error, {
350354
toolId: block.config.tool,

0 commit comments

Comments
 (0)