Skip to content

Commit e3d6098

Browse files
committed
fix(execution): tag Sim invariant failures and keep network errors out of system faults
- Add SystemError and isSystemError to @sim/utils/errors; throw SystemError where an internal tool operation has no registered handler so a broken registry still faults the job after executeTool flattens the error. - isProgrammingError excludes errors carrying a cause, so fetch's TypeError('fetch failed', { cause }) for an unreachable host is not treated as a defect in Sim code. - Track the schedule job fault in a box so a thrown undefined still faults. - Drop the partial executor-errors mock; the real hasExecutionResult suffices.
1 parent 5f56ea9 commit e3d6098

7 files changed

Lines changed: 108 additions & 33 deletions

File tree

‎apps/sim/background/async-preprocessing-correlation.test.ts‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,13 @@ const {
2626
mockExecuteWorkflowCore,
2727
mockExecutionSnapshot,
2828
mockWasExecutionFinalizedByCore,
29-
mockHasExecutionResult,
3029
mockIsWorkflowTimedOut,
3130
mockGetScheduleTimeValues,
3231
mockGetSubBlockValue,
3332
} = vi.hoisted(() => ({
3433
mockExecuteWorkflowCore: vi.fn(),
3534
mockExecutionSnapshot: vi.fn(),
3635
mockWasExecutionFinalizedByCore: vi.fn(),
37-
mockHasExecutionResult: vi.fn(),
3836
mockIsWorkflowTimedOut: vi.fn(() => false),
3937
mockGetScheduleTimeValues: vi.fn(),
4038
mockGetSubBlockValue: vi.fn(),
@@ -74,11 +72,6 @@ vi.mock('@/executor/execution/snapshot', () => ({
7472
ExecutionSnapshot: mockExecutionSnapshot,
7573
}))
7674

77-
vi.mock('@/executor/utils/errors', async (importOriginal) => ({
78-
...(await importOriginal<typeof import('@/executor/utils/errors')>()),
79-
hasExecutionResult: mockHasExecutionResult,
80-
}))
81-
8275
import { buildBlockExecutionError } from '@/executor/utils/errors'
8376
import type { SerializedBlock } from '@/serializer/types'
8477
import { executeScheduleJob } from './schedule-execution'
@@ -128,7 +121,6 @@ const principal = {
128121
describe('async preprocessing correlation threading', () => {
129122
beforeEach(() => {
130123
mockWasExecutionFinalizedByCore.mockReturnValue(false)
131-
mockHasExecutionResult.mockReturnValue(false)
132124
mockIsWorkflowTimedOut.mockReturnValue(false)
133125
resetDbChainMock()
134126
dbChainMockFns.limit.mockResolvedValue([
@@ -382,7 +374,6 @@ describe('async preprocessing correlation threading', () => {
382374
executionTimeout: {},
383375
})
384376
mockExecuteWorkflowCore.mockRejectedValueOnce(rawError)
385-
mockHasExecutionResult.mockImplementation((error) => error === rawError)
386377
mockWasExecutionFinalizedByCore.mockReturnValue(true)
387378

388379
await expect(

‎apps/sim/background/schedule-execution.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -861,7 +861,7 @@ export async function executeScheduleJob(
861861
* (failure count, next run, claim) has run, so faulting the job to alert
862862
* on it never leaves the schedule claimed or its cadence stalled.
863863
*/
864-
let jobFault: unknown
864+
let jobFault: { error: unknown } | undefined
865865

866866
try {
867867
const [scheduleRecord] = await db
@@ -1213,7 +1213,7 @@ export async function executeScheduleJob(
12131213
)
12141214

12151215
const failure = await classifyWorkflowJobFailure({ error, executionId, loggingSession })
1216-
if (failure === 'job_fault') jobFault = error
1216+
if (failure === 'job_fault') jobFault = { error }
12171217
}
12181218
} catch (error: unknown) {
12191219
try {
@@ -1222,7 +1222,7 @@ export async function executeScheduleJob(
12221222
return
12231223
}
12241224

1225-
jobFault = error
1225+
jobFault = { error }
12261226
logger.error(`[${requestId}] Error processing schedule ${payload.scheduleId}`, error, {
12271227
cause: describeError(error),
12281228
})
@@ -1243,7 +1243,7 @@ export async function executeScheduleJob(
12431243
}
12441244
}
12451245

1246-
if (jobFault !== undefined) throw jobFault
1246+
if (jobFault) throw jobFault.error
12471247
})
12481248
} finally {
12491249
timeoutController.cleanup()

‎apps/sim/lib/workflows/executor/job-failure.ts‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { findCause, getErrorMessage, isProgrammingError } from '@sim/utils/errors'
1+
import { findCause, getErrorMessage, isSystemError } from '@sim/utils/errors'
22
import type { LoggingSession } from '@/lib/logs/execution/logging-session'
33
import { wasExecutionFinalizedByCore } from '@/lib/workflows/executor/execution-core'
44
import {
@@ -60,20 +60,12 @@ const UNATTRIBUTED_WORKFLOW_FAILURE_CODES: ReadonlySet<WorkflowExecutionErrorCod
6060
])
6161

6262
/**
63-
* Whether Sim's own code, not the workflow, caused the failure: a programming
64-
* error anywhere in the `.cause` chain, or a link marked `isSystemError` where
65-
* `executeTool` flattened such an error into a result. User code never produces
66-
* either, since sandboxes return its errors as data.
63+
* Whether Sim's own code, not the workflow, caused the failure anywhere in the
64+
* `.cause` chain. User code never produces a system error, since sandboxes
65+
* return its errors as data.
6766
*/
6867
function isSystemFailure(error: unknown): boolean {
69-
return (
70-
findCause(
71-
error,
72-
(value): value is Error =>
73-
isProgrammingError(value) ||
74-
(value instanceof Error && 'isSystemError' in value && value.isSystemError === true)
75-
) !== undefined
76-
)
68+
return findCause(error, isSystemError) !== undefined
7769
}
7870

7971
/**

‎apps/sim/tools/index.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,6 +1191,41 @@ describe('executeTool Function', () => {
11911191
}
11921192
)
11931193

1194+
it('marks a declared operation with no registered handler as a system error', async () => {
1195+
const mockTool = {
1196+
id: 'test_unregistered_operation',
1197+
name: 'Test Unregistered Operation',
1198+
description: 'Declares an operation no handler implements',
1199+
version: '1.0.0',
1200+
params: {},
1201+
operation: { input: createInternalToolOperationInput },
1202+
} satisfies InternalToolConfig<Record<string, unknown>>
1203+
;(tools as Record<string, unknown>).test_unregistered_operation = mockTool
1204+
mockGetInternalToolOperationHandler.mockResolvedValueOnce(undefined)
1205+
1206+
try {
1207+
const result = await executeTool(
1208+
'test_unregistered_operation',
1209+
{},
1210+
{
1211+
executionContext: createToolExecutionContext({
1212+
userId: 'user-1',
1213+
workspaceId: 'workspace-1',
1214+
workflowId: 'workflow-1',
1215+
}),
1216+
}
1217+
)
1218+
1219+
expect(result).toMatchObject({
1220+
success: false,
1221+
error: 'No internal operation registered for test_unregistered_operation',
1222+
isSystemError: true,
1223+
})
1224+
} finally {
1225+
Reflect.deleteProperty(tools, 'test_unregistered_operation')
1226+
}
1227+
})
1228+
11941229
it('preserves actorless schedule authority for registered operations', async () => {
11951230
const mockTool = {
11961231
id: 'test_actorless_registered_operation',

‎apps/sim/tools/index.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ import {
44
describeError,
55
findCause,
66
getErrorMessage,
7-
isProgrammingError,
7+
isSystemError,
8+
SystemError,
89
toError,
910
} from '@sim/utils/errors'
1011
import { sleep } from '@sim/utils/helpers'
@@ -2430,7 +2431,7 @@ async function executeToolImplementation(
24302431
// thrown error into a result object; an upstream provider's status stays
24312432
// on `output` where it cannot be mistaken for ours.
24322433
...(error instanceof HttpError ? { statusCode: error.statusCode } : {}),
2433-
...(findCause(error, isProgrammingError) ? { isSystemError: true as const } : {}),
2434+
...(findCause(error, isSystemError) ? { isSystemError: true as const } : {}),
24342435
timing: {
24352436
startTime: startTimeISO,
24362437
endTime: endTimeISO,
@@ -2738,7 +2739,7 @@ async function executeDeclaredInternalOperation({
27382739
})
27392740
} else {
27402741
const handler = await getInternalToolOperationHandler(toolId)
2741-
if (!handler) throw new Error(`No internal operation registered for ${toolId}`)
2742+
if (!handler) throw new SystemError(`No internal operation registered for ${toolId}`)
27422743
const requestedTimeout = Number(params.timeout)
27432744
const operationTimeout =
27442745
Number.isFinite(requestedTimeout) && requestedTimeout > 0
@@ -3293,7 +3294,7 @@ async function executeMcpTool(
32933294
logger.info(`[${actualRequestId}] Executing MCP tool: ${toolId}`)
32943295
validateRequestBodySize(JSON.stringify(params), actualRequestId, `mcp:${toolId}`)
32953296
const handler = await getInternalToolOperationHandler(toolId)
3296-
if (!handler) throw new Error(`No internal operation registered for ${toolId}`)
3297+
if (!handler) throw new SystemError(`No internal operation registered for ${toolId}`)
32973298
const resultResponse = await handler({
32983299
toolId,
32993300
input: params,

‎packages/utils/src/errors.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
getPostgresCancellationReason,
55
getPostgresErrorCode,
66
getTransientDatabaseFailure,
7+
isProgrammingError,
78
} from '@sim/utils/errors'
89
import { describe, expect, it } from 'vitest'
910

@@ -236,3 +237,27 @@ describe('describeError', () => {
236237
expect(described?.causeChain?.length).toBeLessThanOrEqual(10)
237238
})
238239
})
240+
241+
describe('isProgrammingError', () => {
242+
it('recognizes a defect the running code raised itself', () => {
243+
const thrown = (() => {
244+
try {
245+
JSON.parse('null').flatMap()
246+
} catch (error) {
247+
return error
248+
}
249+
})()
250+
251+
expect(isProgrammingError(thrown)).toBe(true)
252+
})
253+
254+
it('does not treat a runtime API reporting an external failure as a defect', () => {
255+
const networkFailure = new TypeError('fetch failed', {
256+
cause: Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:443'), {
257+
code: 'ECONNREFUSED',
258+
}),
259+
})
260+
261+
expect(isProgrammingError(networkFailure)).toBe(false)
262+
})
263+
})

‎packages/utils/src/errors.ts‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,14 +319,45 @@ export function findCause<T>(
319319

320320
/**
321321
* Whether `value` is a JavaScript runtime programming error: a type, reference
322-
* or range fault raised by the running code itself, as opposed to an `Error` a
323-
* caller throws on purpose to reject its input.
322+
* or range fault raised by the running code itself. A runtime API reporting an
323+
* external failure wraps what it observed as `cause` (`fetch` rejects a network
324+
* failure as `TypeError('fetch failed', { cause })`), while a defect such as
325+
* calling a missing method never carries one.
324326
*/
325327
export function isProgrammingError(
326328
value: unknown
327329
): value is TypeError | ReferenceError | RangeError {
328330
return (
329-
value instanceof TypeError || value instanceof ReferenceError || value instanceof RangeError
331+
(value instanceof TypeError ||
332+
value instanceof ReferenceError ||
333+
value instanceof RangeError) &&
334+
value.cause === undefined
335+
)
336+
}
337+
338+
/**
339+
* A broken invariant in Sim's own code, never a rejection of the caller's
340+
* input. Throw it where an invariant is checked so the failure is recognised as
341+
* a platform fault even after it is flattened into a plain error.
342+
*/
343+
export class SystemError extends Error {
344+
readonly isSystemError = true
345+
346+
constructor(message: string, options?: ErrorOptions) {
347+
super(message, options)
348+
this.name = 'SystemError'
349+
}
350+
}
351+
352+
/**
353+
* Whether Sim's own code caused `value`: a {@link SystemError}, an error that
354+
* carries its `isSystemError` flag across a flattening boundary, or a
355+
* programming error (see {@link isProgrammingError}).
356+
*/
357+
export function isSystemError(value: unknown): value is Error {
358+
return (
359+
isProgrammingError(value) ||
360+
(value instanceof Error && 'isSystemError' in value && value.isSystemError === true)
330361
)
331362
}
332363

0 commit comments

Comments
 (0)