Skip to content

Commit b127bf4

Browse files
authored
fix(executor): bound condition loops and reject unnamed variable assignments (#9463)
* fix(executor): stop condition loops at an iteration ceiling and reject unnamed variable assignments A While loop, or a Do While loop given a condition, had no iteration bound: a body that never changed what the condition checks repeated until the run timed out, and billing-disabled deployments run untimed, so it repeated until the process stopped. Such loops now fail with an error on the loop after 10,000 iterations if the condition is still true. For, For Each and count-driven Do While loops are unchanged. A Variables assignment with a value but no variable name was skipped without an error. It now fails the block; blank editor rows are still ignored, and a row that selects its variable by id alone now updates that variable. The block's published input description named the wrong field ("name"), which is corrected to variableName. * fix(executor): give untyped variable assignments their target variable's type * chore(executor): shrink the explicit-any baseline for the variables handler
1 parent dff74b6 commit b127bf4

9 files changed

Lines changed: 304 additions & 18 deletions

File tree

‎apps/docs/content/docs/workflows/blocks/loop.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ For uses the configured count; For Each uses the collection length. `1,000` is a
6161

6262
An empty array skips the body and returns empty results. Missing, null, or empty-string collection configuration is an error rather than an empty iteration set.
6363

64-
While and conditional Do While have no fixed iteration ceiling. Ensure the condition eventually becomes false. Iterations add sequential execution time; connect error paths inside the loop when an item failure should be handled.
64+
While and conditional Do While stop after `10,000` iterations if the condition is still true, and the run fails with an error on the loop. Ensure the condition eventually becomes false. Iterations add sequential execution time; connect error paths inside the loop when an item failure should be handled.
6565

6666
Loops and Parallel containers can nest in either direction. Use the reference picker for the appropriate container scope in a nested flow.
6767

‎apps/docs/content/docs/workflows/blocks/variables.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ Then select `retryCount` in a Variables block and set its value to `<increment.r
2727

2828
**Variable Assignments** contains the variables to update and their new values. Select an existing variable, then enter a literal value or an earlier output, such as `<api.data.email>`.
2929

30-
The block converts each value to the selected variable's type. It does not evaluate JavaScript arithmetic. Use a Function for calculations, then assign its result.
30+
The block converts each value to the selected variable's type. It does not evaluate JavaScript arithmetic. Use a Function for calculations, then assign its result. A row with a value but no selected variable fails the block; empty rows are ignored.
3131

3232
Assignments are written in order, but all references are resolved before the assignments run. A later row in the same block does not read a value just written by an earlier row.
3333

‎apps/sim/blocks/blocks/variables.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@ export const VariablesBlock: BlockConfig = {
4040
inputs: {
4141
variables: {
4242
type: 'json',
43-
description: 'Array of variable objects with name and value properties',
43+
description:
44+
"Array of assignments, each { variableName, value }: variableName is an existing workflow variable's name, value its new value",
4445
},
4546
},
4647
outputs: {

‎apps/sim/executor/constants.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,12 @@ export const DEFAULTS = {
196196
BLOCK_TITLE: 'Untitled Block',
197197
WORKFLOW_NAME: 'Workflow',
198198
DEFAULT_LOOP_ITERATIONS: 1000,
199+
/**
200+
* Most iterations a condition-driven loop (While, or Do While with a condition) may run.
201+
* Its body can leave the condition unchanged, and billing-disabled deployments run
202+
* untimed, so without this a stuck condition repeats until the process is stopped.
203+
*/
204+
MAX_CONDITION_LOOP_ITERATIONS: 10_000,
199205
MAX_PARALLEL_BRANCHES: 20,
200206
MAX_NESTING_DEPTH: 10,
201207
/** Maximum child workflow depth for propagating SSE callbacks (block:started, block:completed). */
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import type { SessionPrincipal } from '@sim/auth/principal'
2+
import {
3+
createSerializedBlock,
4+
createSerializedWorkflow,
5+
} from '@sim/testing/factories/serialized-block.factory'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
import { BlockType, DEFAULTS } from '@/executor/constants'
8+
import { DAGExecutor } from '@/executor/execution/executor'
9+
import type { ExecutionResult } from '@/executor/types'
10+
import { hasExecutionResult } from '@/executor/utils/errors'
11+
import type { SerializedLoop, SerializedWorkflow } from '@/serializer/types'
12+
13+
const { bodyRuns, conditionResult, runaway } = vi.hoisted(() => ({
14+
bodyRuns: { count: 0 },
15+
conditionResult: { stopAfter: Number.POSITIVE_INFINITY },
16+
/** Cancels a loop the ceiling failed to stop, so a regression fails instead of hanging. */
17+
runaway: { controller: new AbortController(), limit: 30_000 },
18+
}))
19+
20+
/** Count conditions resolve to `return Boolean(<index> < <count>)`; user conditions stay opaque. */
21+
vi.mock('@/lib/execution/isolated-vm', () => ({
22+
executeInIsolatedVM: vi.fn(async ({ code }: { code: string }) => {
23+
const indexCheck = /^return Boolean\((\d+) < (\d+)\)$/.exec(code)
24+
if (indexCheck) return { result: Number(indexCheck[1]) < Number(indexCheck[2]) }
25+
return { result: bodyRuns.count < conditionResult.stopAfter }
26+
}),
27+
}))
28+
29+
vi.mock('@/executor/handlers/registry', () => ({
30+
createBlockHandlers: () => [
31+
{
32+
canHandle: () => true,
33+
execute: async (_ctx: unknown, block: { id: string }) => {
34+
if (block.id === 'tick') bodyRuns.count++
35+
if (bodyRuns.count >= runaway.limit) runaway.controller.abort()
36+
return { ok: true }
37+
},
38+
},
39+
],
40+
}))
41+
42+
const PRINCIPAL: SessionPrincipal = { kind: 'session', userId: 'user-1', sessionId: 'session-1' }
43+
const CEILING = DEFAULTS.MAX_CONDITION_LOOP_ITERATIONS
44+
45+
function loopWorkflow(loop: Partial<Omit<SerializedLoop, 'id' | 'nodes'>>): SerializedWorkflow {
46+
const blocks = [
47+
createSerializedBlock({ id: 'start', name: 'start', type: BlockType.STARTER }),
48+
createSerializedBlock({ id: 'loop', name: 'loop', type: BlockType.LOOP }),
49+
createSerializedBlock({ id: 'tick', name: 'tick', type: BlockType.FUNCTION }),
50+
]
51+
return {
52+
...createSerializedWorkflow(blocks, [
53+
{ source: 'start', target: 'loop' },
54+
{ source: 'loop', target: 'tick', sourceHandle: 'loop-start-source' },
55+
]),
56+
loops: { loop: { id: 'loop', nodes: ['tick'], iterations: 0, ...loop } },
57+
}
58+
}
59+
60+
/** Runs the workflow and returns its result, including the one a failed run attaches to its throw. */
61+
async function run(workflow: SerializedWorkflow): Promise<ExecutionResult> {
62+
try {
63+
return await new DAGExecutor({
64+
workflow,
65+
contextExtensions: {
66+
workspaceId: 'ws',
67+
executionId: 'exec-1',
68+
principal: PRINCIPAL,
69+
abortSignal: runaway.controller.signal,
70+
},
71+
}).execute('wf')
72+
} catch (error) {
73+
if (hasExecutionResult(error)) return error.executionResult
74+
throw error
75+
}
76+
}
77+
78+
describe('condition-driven loop iteration ceiling', () => {
79+
beforeEach(() => {
80+
bodyRuns.count = 0
81+
conditionResult.stopAfter = Number.POSITIVE_INFINITY
82+
runaway.controller = new AbortController()
83+
})
84+
85+
it.each([
86+
['While', { loopType: 'while', whileCondition: '<variable.i> < 3' }],
87+
['Do While', { loopType: 'doWhile', doWhileCondition: '<variable.i> < 3', iterations: 5 }],
88+
] as const)(
89+
'fails a %s loop whose condition never turns false at the ceiling',
90+
async (label, loop) => {
91+
const result = await run(loopWorkflow(loop))
92+
93+
expect(bodyRuns.count).toBe(CEILING)
94+
expect(result.success).toBe(false)
95+
expect(result.error).toContain(`${label} loop stopped after 10,000 iterations`)
96+
expect(result.logs?.find((log) => log.blockId === 'loop')).toMatchObject({
97+
success: false,
98+
error: expect.stringContaining('condition still true'),
99+
})
100+
},
101+
60_000
102+
)
103+
104+
it('ends a While loop normally when its condition turns false on the last allowed iteration', async () => {
105+
conditionResult.stopAfter = CEILING
106+
107+
const result = await run(
108+
loopWorkflow({ loopType: 'while', whileCondition: '<variable.i> < 3' })
109+
)
110+
111+
expect(bodyRuns.count).toBe(CEILING)
112+
expect(result.success).toBe(true)
113+
}, 60_000)
114+
115+
it.each([
116+
['For', { loopType: 'for', iterations: CEILING + 5 }],
117+
['count-driven Do While', { loopType: 'doWhile', iterations: CEILING + 5 }],
118+
] as const)(
119+
'leaves a %s loop free to run its configured count past the ceiling',
120+
async (_label, loop) => {
121+
const result = await run(loopWorkflow(loop))
122+
123+
expect(result.success).toBe(true)
124+
expect(bodyRuns.count).toBe(CEILING + 5)
125+
},
126+
60_000
127+
)
128+
})

‎apps/sim/executor/handlers/variables/handler.test.ts‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,4 +244,71 @@ describe('VariablesBlockHandler', () => {
244244
isExisting: true,
245245
})
246246
})
247+
248+
it.each([
249+
['a blank variableName', { variableName: ' ', type: 'array', value: '[1]' }],
250+
['a name key instead of variableName', { name: 'issues', type: 'array', value: '[1]' }],
251+
['an unknown variableId and no name', { variableId: 'gone', type: 'boolean', value: false }],
252+
])('fails an assignment that sets a value through %s', async (_label, assignment) => {
253+
const handler = new VariablesBlockHandler()
254+
const ctx = createContext()
255+
256+
await expect(
257+
handler.execute(ctx, createBlock(), {
258+
variables: [{ variableName: 'issues', type: 'array', value: '[0]' }, assignment],
259+
})
260+
).rejects.toThrow('Assignment 2 sets a value but names no variable')
261+
expect(ctx.workflowVariables?.['var-1'].value).toEqual([])
262+
})
263+
264+
it('skips the blank rows the editor creates without failing', async () => {
265+
const handler = new VariablesBlockHandler()
266+
const ctx = createContext()
267+
268+
const output = await handler.execute(ctx, createBlock(), {
269+
variables: [
270+
{ id: 'row-1', variableName: '', type: 'string', value: '', isExisting: false },
271+
{ id: 'row-2', variableName: 'issues', type: 'array', value: '[1]', isExisting: true },
272+
{ id: 'row-3', variableName: '', type: 'number', value: ' ', isExisting: false },
273+
],
274+
})
275+
276+
expect(output).toEqual({ issues: [1] })
277+
expect(ctx.workflowVariables?.['var-1'].value).toEqual([1])
278+
})
279+
280+
it('updates the variable a row selects by variableId when the row carries no name', async () => {
281+
const handler = new VariablesBlockHandler()
282+
const ctx = createContext()
283+
284+
const output = await handler.execute(ctx, createBlock(), {
285+
variables: [{ variableId: 'var-1', type: 'array', value: '[2]' }],
286+
})
287+
288+
expect(output).toEqual({ issues: [2] })
289+
expect(ctx.workflowVariables?.['var-1'].value).toEqual([2])
290+
})
291+
292+
it.each([
293+
['by name', { variableName: 'issues' }],
294+
['by variableId', { variableId: 'var-1' }],
295+
])('keeps the target variable type when a row %s omits type', async (_label, selector) => {
296+
const handler = new VariablesBlockHandler()
297+
const ctx = createContext({
298+
workflowVariables: {
299+
'var-1': { id: 'var-1', name: 'issues', type: 'object', value: {} },
300+
'var-2': { id: 'var-2', name: 'count', type: 'number', value: 0 },
301+
},
302+
})
303+
304+
await handler.execute(ctx, createBlock(), {
305+
variables: [
306+
{ ...selector, value: { open: 2 } },
307+
{ variableName: 'count', value: 1 },
308+
],
309+
})
310+
311+
expect(ctx.workflowVariables?.['var-1'].value).toEqual({ open: 2 })
312+
expect(ctx.workflowVariables?.['var-2'].value).toBe(1)
313+
})
247314
})

‎apps/sim/executor/handlers/variables/handler.ts‎

Lines changed: 64 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { createLogger } from '@sim/logger'
22
import { toError } from '@sim/utils/errors'
3+
import { toRecord, toRecordOrNull } from '@sim/utils/object'
34
import { parseLargeExecutionValue } from '@/lib/execution/payloads/large-execution-value'
45
import { compactWorkflowVariableValue } from '@/lib/execution/payloads/serializer'
56
import type { BlockOutput } from '@/blocks/types'
@@ -56,6 +57,42 @@ function setWorkflowVariableProvenance(
5657
})
5758
}
5859

60+
interface AssignmentTarget {
61+
name: string
62+
type: string
63+
}
64+
65+
function readTrimmedString(value: unknown): string | undefined {
66+
return typeof value === 'string' && value.trim() ? value.trim() : undefined
67+
}
68+
69+
/**
70+
* The variable an assignment row writes: the one it names, else the one its `variableId`
71+
* selects. A row that omits `type` takes the target variable's own type, so a programmatic
72+
* `{ variableName: 'count', value: 1 }` stays a number instead of becoming the string `'1'`.
73+
*/
74+
function resolveAssignmentTarget(
75+
assignment: Record<string, unknown>,
76+
workflowVariables: Record<string, unknown>
77+
): AssignmentTarget | undefined {
78+
const variableId = readTrimmedString(assignment.variableId)
79+
const selected = toRecordOrNull(getWorkflowVariableEntry(workflowVariables, variableId)?.[1])
80+
const name = readTrimmedString(assignment.variableName) ?? readTrimmedString(selected?.name)
81+
if (!name) return undefined
82+
const target =
83+
selected ??
84+
Object.values(workflowVariables)
85+
.map(toRecordOrNull)
86+
.find((variable) => variable?.name === name)
87+
const type = readTrimmedString(assignment.type) ?? readTrimmedString(target?.type) ?? 'string'
88+
return { name, type }
89+
}
90+
91+
function hasAssignedValue(value: unknown): boolean {
92+
if (value === undefined || value === null) return false
93+
return typeof value !== 'string' || value.trim() !== ''
94+
}
95+
5996
export class VariablesBlockHandler implements BlockHandler {
6097
canHandle(block: SerializedBlock): boolean {
6198
const canHandle = block.metadata?.id === BlockType.VARIABLES
@@ -72,7 +109,7 @@ export class VariablesBlockHandler implements BlockHandler {
72109
ctx.workflowVariables = {}
73110
}
74111

75-
const assignments = this.parseAssignments(inputs.variables)
112+
const assignments = this.parseAssignments(inputs.variables, ctx.workflowVariables)
76113

77114
const output: Record<string, any> = {}
78115

@@ -127,8 +164,15 @@ export class VariablesBlockHandler implements BlockHandler {
127164
})
128165
}
129166

167+
/**
168+
* Reads the block's assignment rows. A row targets the variable it names, or the one its
169+
* `variableId` selects. A row with neither is skipped while it is blank (the editor's empty
170+
* row), and fails the block when it carries a value, since that update would otherwise be
171+
* dropped without a trace.
172+
*/
130173
private parseAssignments(
131-
assignmentsInput: any
174+
assignmentsInput: unknown,
175+
workflowVariables: Record<string, unknown>
132176
): Array<{ variableId?: string; variableName: string; type: string; value: any }> {
133177
const result: Array<{ variableId?: string; variableName: string; type: string; value: any }> =
134178
[]
@@ -137,19 +181,25 @@ export class VariablesBlockHandler implements BlockHandler {
137181
return result
138182
}
139183

140-
for (const assignment of assignmentsInput) {
141-
if (assignment?.variableName?.trim()) {
142-
const name = assignment.variableName.trim()
143-
const type = assignment.type || 'string'
144-
const value = this.parseValueByType(assignment.value, type, name)
145-
146-
result.push({
147-
variableId: assignment.variableId,
148-
variableName: name,
149-
type,
150-
value,
151-
})
184+
for (const [index, row] of assignmentsInput.entries()) {
185+
const assignment = toRecord(row)
186+
const target = resolveAssignmentTarget(assignment, workflowVariables)
187+
if (!target) {
188+
if (hasAssignedValue(assignment.value)) {
189+
throw new Error(
190+
`Assignment ${index + 1} sets a value but names no variable. Select a workflow variable for it (variableName), or remove the row.`
191+
)
192+
}
193+
continue
152194
}
195+
const value = this.parseValueByType(assignment.value, target.type, target.name)
196+
197+
result.push({
198+
variableId: readTrimmedString(assignment.variableId),
199+
variableName: target.name,
200+
type: target.type,
201+
value,
202+
})
153203
}
154204

155205
return result

0 commit comments

Comments
 (0)