diff --git a/apps/docs/content/docs/workflows/blocks/loop.mdx b/apps/docs/content/docs/workflows/blocks/loop.mdx index fe9b8a3ba29..dc3837424c4 100644 --- a/apps/docs/content/docs/workflows/blocks/loop.mdx +++ b/apps/docs/content/docs/workflows/blocks/loop.mdx @@ -61,7 +61,7 @@ For uses the configured count; For Each uses the collection length. `1,000` is a 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. -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. +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. Loops and Parallel containers can nest in either direction. Use the reference picker for the appropriate container scope in a nested flow. diff --git a/apps/docs/content/docs/workflows/blocks/variables.mdx b/apps/docs/content/docs/workflows/blocks/variables.mdx index c68e3e7e5ee..2c3d0bf097c 100644 --- a/apps/docs/content/docs/workflows/blocks/variables.mdx +++ b/apps/docs/content/docs/workflows/blocks/variables.mdx @@ -27,7 +27,7 @@ Then select `retryCount` in a Variables block and set its value to ``. -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. +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. 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. diff --git a/apps/sim/blocks/blocks/variables.ts b/apps/sim/blocks/blocks/variables.ts index f8056ed8029..59cefd3b659 100644 --- a/apps/sim/blocks/blocks/variables.ts +++ b/apps/sim/blocks/blocks/variables.ts @@ -40,7 +40,8 @@ export const VariablesBlock: BlockConfig = { inputs: { variables: { type: 'json', - description: 'Array of variable objects with name and value properties', + description: + "Array of assignments, each { variableName, value }: variableName is an existing workflow variable's name, value its new value", }, }, outputs: { diff --git a/apps/sim/executor/constants.ts b/apps/sim/executor/constants.ts index b21258f64df..7c0a87bcf11 100644 --- a/apps/sim/executor/constants.ts +++ b/apps/sim/executor/constants.ts @@ -196,6 +196,12 @@ export const DEFAULTS = { BLOCK_TITLE: 'Untitled Block', WORKFLOW_NAME: 'Workflow', DEFAULT_LOOP_ITERATIONS: 1000, + /** + * Most iterations a condition-driven loop (While, or Do While with a condition) may run. + * Its body can leave the condition unchanged, and billing-disabled deployments run + * untimed, so without this a stuck condition repeats until the process is stopped. + */ + MAX_CONDITION_LOOP_ITERATIONS: 10_000, MAX_PARALLEL_BRANCHES: 20, MAX_NESTING_DEPTH: 10, /** Maximum child workflow depth for propagating SSE callbacks (block:started, block:completed). */ diff --git a/apps/sim/executor/execution/condition-loop-ceiling.test.ts b/apps/sim/executor/execution/condition-loop-ceiling.test.ts new file mode 100644 index 00000000000..5b2a175a47e --- /dev/null +++ b/apps/sim/executor/execution/condition-loop-ceiling.test.ts @@ -0,0 +1,128 @@ +import type { SessionPrincipal } from '@sim/auth/principal' +import { + createSerializedBlock, + createSerializedWorkflow, +} from '@sim/testing/factories/serialized-block.factory' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { BlockType, DEFAULTS } from '@/executor/constants' +import { DAGExecutor } from '@/executor/execution/executor' +import type { ExecutionResult } from '@/executor/types' +import { hasExecutionResult } from '@/executor/utils/errors' +import type { SerializedLoop, SerializedWorkflow } from '@/serializer/types' + +const { bodyRuns, conditionResult, runaway } = vi.hoisted(() => ({ + bodyRuns: { count: 0 }, + conditionResult: { stopAfter: Number.POSITIVE_INFINITY }, + /** Cancels a loop the ceiling failed to stop, so a regression fails instead of hanging. */ + runaway: { controller: new AbortController(), limit: 30_000 }, +})) + +/** Count conditions resolve to `return Boolean( < )`; user conditions stay opaque. */ +vi.mock('@/lib/execution/isolated-vm', () => ({ + executeInIsolatedVM: vi.fn(async ({ code }: { code: string }) => { + const indexCheck = /^return Boolean\((\d+) < (\d+)\)$/.exec(code) + if (indexCheck) return { result: Number(indexCheck[1]) < Number(indexCheck[2]) } + return { result: bodyRuns.count < conditionResult.stopAfter } + }), +})) + +vi.mock('@/executor/handlers/registry', () => ({ + createBlockHandlers: () => [ + { + canHandle: () => true, + execute: async (_ctx: unknown, block: { id: string }) => { + if (block.id === 'tick') bodyRuns.count++ + if (bodyRuns.count >= runaway.limit) runaway.controller.abort() + return { ok: true } + }, + }, + ], +})) + +const PRINCIPAL: SessionPrincipal = { kind: 'session', userId: 'user-1', sessionId: 'session-1' } +const CEILING = DEFAULTS.MAX_CONDITION_LOOP_ITERATIONS + +function loopWorkflow(loop: Partial>): SerializedWorkflow { + const blocks = [ + createSerializedBlock({ id: 'start', name: 'start', type: BlockType.STARTER }), + createSerializedBlock({ id: 'loop', name: 'loop', type: BlockType.LOOP }), + createSerializedBlock({ id: 'tick', name: 'tick', type: BlockType.FUNCTION }), + ] + return { + ...createSerializedWorkflow(blocks, [ + { source: 'start', target: 'loop' }, + { source: 'loop', target: 'tick', sourceHandle: 'loop-start-source' }, + ]), + loops: { loop: { id: 'loop', nodes: ['tick'], iterations: 0, ...loop } }, + } +} + +/** Runs the workflow and returns its result, including the one a failed run attaches to its throw. */ +async function run(workflow: SerializedWorkflow): Promise { + try { + return await new DAGExecutor({ + workflow, + contextExtensions: { + workspaceId: 'ws', + executionId: 'exec-1', + principal: PRINCIPAL, + abortSignal: runaway.controller.signal, + }, + }).execute('wf') + } catch (error) { + if (hasExecutionResult(error)) return error.executionResult + throw error + } +} + +describe('condition-driven loop iteration ceiling', () => { + beforeEach(() => { + bodyRuns.count = 0 + conditionResult.stopAfter = Number.POSITIVE_INFINITY + runaway.controller = new AbortController() + }) + + it.each([ + ['While', { loopType: 'while', whileCondition: ' < 3' }], + ['Do While', { loopType: 'doWhile', doWhileCondition: ' < 3', iterations: 5 }], + ] as const)( + 'fails a %s loop whose condition never turns false at the ceiling', + async (label, loop) => { + const result = await run(loopWorkflow(loop)) + + expect(bodyRuns.count).toBe(CEILING) + expect(result.success).toBe(false) + expect(result.error).toContain(`${label} loop stopped after 10,000 iterations`) + expect(result.logs?.find((log) => log.blockId === 'loop')).toMatchObject({ + success: false, + error: expect.stringContaining('condition still true'), + }) + }, + 60_000 + ) + + it('ends a While loop normally when its condition turns false on the last allowed iteration', async () => { + conditionResult.stopAfter = CEILING + + const result = await run( + loopWorkflow({ loopType: 'while', whileCondition: ' < 3' }) + ) + + expect(bodyRuns.count).toBe(CEILING) + expect(result.success).toBe(true) + }, 60_000) + + it.each([ + ['For', { loopType: 'for', iterations: CEILING + 5 }], + ['count-driven Do While', { loopType: 'doWhile', iterations: CEILING + 5 }], + ] as const)( + 'leaves a %s loop free to run its configured count past the ceiling', + async (_label, loop) => { + const result = await run(loopWorkflow(loop)) + + expect(result.success).toBe(true) + expect(bodyRuns.count).toBe(CEILING + 5) + }, + 60_000 + ) +}) diff --git a/apps/sim/executor/handlers/variables/handler.test.ts b/apps/sim/executor/handlers/variables/handler.test.ts index 31b47e233c9..3e00e3e43a6 100644 --- a/apps/sim/executor/handlers/variables/handler.test.ts +++ b/apps/sim/executor/handlers/variables/handler.test.ts @@ -244,4 +244,71 @@ describe('VariablesBlockHandler', () => { isExisting: true, }) }) + + it.each([ + ['a blank variableName', { variableName: ' ', type: 'array', value: '[1]' }], + ['a name key instead of variableName', { name: 'issues', type: 'array', value: '[1]' }], + ['an unknown variableId and no name', { variableId: 'gone', type: 'boolean', value: false }], + ])('fails an assignment that sets a value through %s', async (_label, assignment) => { + const handler = new VariablesBlockHandler() + const ctx = createContext() + + await expect( + handler.execute(ctx, createBlock(), { + variables: [{ variableName: 'issues', type: 'array', value: '[0]' }, assignment], + }) + ).rejects.toThrow('Assignment 2 sets a value but names no variable') + expect(ctx.workflowVariables?.['var-1'].value).toEqual([]) + }) + + it('skips the blank rows the editor creates without failing', async () => { + const handler = new VariablesBlockHandler() + const ctx = createContext() + + const output = await handler.execute(ctx, createBlock(), { + variables: [ + { id: 'row-1', variableName: '', type: 'string', value: '', isExisting: false }, + { id: 'row-2', variableName: 'issues', type: 'array', value: '[1]', isExisting: true }, + { id: 'row-3', variableName: '', type: 'number', value: ' ', isExisting: false }, + ], + }) + + expect(output).toEqual({ issues: [1] }) + expect(ctx.workflowVariables?.['var-1'].value).toEqual([1]) + }) + + it('updates the variable a row selects by variableId when the row carries no name', async () => { + const handler = new VariablesBlockHandler() + const ctx = createContext() + + const output = await handler.execute(ctx, createBlock(), { + variables: [{ variableId: 'var-1', type: 'array', value: '[2]' }], + }) + + expect(output).toEqual({ issues: [2] }) + expect(ctx.workflowVariables?.['var-1'].value).toEqual([2]) + }) + + it.each([ + ['by name', { variableName: 'issues' }], + ['by variableId', { variableId: 'var-1' }], + ])('keeps the target variable type when a row %s omits type', async (_label, selector) => { + const handler = new VariablesBlockHandler() + const ctx = createContext({ + workflowVariables: { + 'var-1': { id: 'var-1', name: 'issues', type: 'object', value: {} }, + 'var-2': { id: 'var-2', name: 'count', type: 'number', value: 0 }, + }, + }) + + await handler.execute(ctx, createBlock(), { + variables: [ + { ...selector, value: { open: 2 } }, + { variableName: 'count', value: 1 }, + ], + }) + + expect(ctx.workflowVariables?.['var-1'].value).toEqual({ open: 2 }) + expect(ctx.workflowVariables?.['var-2'].value).toBe(1) + }) }) diff --git a/apps/sim/executor/handlers/variables/handler.ts b/apps/sim/executor/handlers/variables/handler.ts index ec04c2b1584..28e32d0179d 100644 --- a/apps/sim/executor/handlers/variables/handler.ts +++ b/apps/sim/executor/handlers/variables/handler.ts @@ -1,5 +1,6 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' +import { toRecord, toRecordOrNull } from '@sim/utils/object' import { parseLargeExecutionValue } from '@/lib/execution/payloads/large-execution-value' import { compactWorkflowVariableValue } from '@/lib/execution/payloads/serializer' import type { BlockOutput } from '@/blocks/types' @@ -56,6 +57,42 @@ function setWorkflowVariableProvenance( }) } +interface AssignmentTarget { + name: string + type: string +} + +function readTrimmedString(value: unknown): string | undefined { + return typeof value === 'string' && value.trim() ? value.trim() : undefined +} + +/** + * The variable an assignment row writes: the one it names, else the one its `variableId` + * selects. A row that omits `type` takes the target variable's own type, so a programmatic + * `{ variableName: 'count', value: 1 }` stays a number instead of becoming the string `'1'`. + */ +function resolveAssignmentTarget( + assignment: Record, + workflowVariables: Record +): AssignmentTarget | undefined { + const variableId = readTrimmedString(assignment.variableId) + const selected = toRecordOrNull(getWorkflowVariableEntry(workflowVariables, variableId)?.[1]) + const name = readTrimmedString(assignment.variableName) ?? readTrimmedString(selected?.name) + if (!name) return undefined + const target = + selected ?? + Object.values(workflowVariables) + .map(toRecordOrNull) + .find((variable) => variable?.name === name) + const type = readTrimmedString(assignment.type) ?? readTrimmedString(target?.type) ?? 'string' + return { name, type } +} + +function hasAssignedValue(value: unknown): boolean { + if (value === undefined || value === null) return false + return typeof value !== 'string' || value.trim() !== '' +} + export class VariablesBlockHandler implements BlockHandler { canHandle(block: SerializedBlock): boolean { const canHandle = block.metadata?.id === BlockType.VARIABLES @@ -72,7 +109,7 @@ export class VariablesBlockHandler implements BlockHandler { ctx.workflowVariables = {} } - const assignments = this.parseAssignments(inputs.variables) + const assignments = this.parseAssignments(inputs.variables, ctx.workflowVariables) const output: Record = {} @@ -127,8 +164,15 @@ export class VariablesBlockHandler implements BlockHandler { }) } + /** + * Reads the block's assignment rows. A row targets the variable it names, or the one its + * `variableId` selects. A row with neither is skipped while it is blank (the editor's empty + * row), and fails the block when it carries a value, since that update would otherwise be + * dropped without a trace. + */ private parseAssignments( - assignmentsInput: any + assignmentsInput: unknown, + workflowVariables: Record ): Array<{ variableId?: string; variableName: string; type: string; value: any }> { const result: Array<{ variableId?: string; variableName: string; type: string; value: any }> = [] @@ -137,19 +181,25 @@ export class VariablesBlockHandler implements BlockHandler { return result } - for (const assignment of assignmentsInput) { - if (assignment?.variableName?.trim()) { - const name = assignment.variableName.trim() - const type = assignment.type || 'string' - const value = this.parseValueByType(assignment.value, type, name) - - result.push({ - variableId: assignment.variableId, - variableName: name, - type, - value, - }) + for (const [index, row] of assignmentsInput.entries()) { + const assignment = toRecord(row) + const target = resolveAssignmentTarget(assignment, workflowVariables) + if (!target) { + if (hasAssignedValue(assignment.value)) { + throw new Error( + `Assignment ${index + 1} sets a value but names no variable. Select a workflow variable for it (variableName), or remove the row.` + ) + } + continue } + const value = this.parseValueByType(assignment.value, target.type, target.name) + + result.push({ + variableId: readTrimmedString(assignment.variableId), + variableName: target.name, + type: target.type, + value, + }) } return result diff --git a/apps/sim/executor/orchestrators/loop.ts b/apps/sim/executor/orchestrators/loop.ts index 1f4a848c238..945f76ec36e 100644 --- a/apps/sim/executor/orchestrators/loop.ts +++ b/apps/sim/executor/orchestrators/loop.ts @@ -53,6 +53,14 @@ async function replaceLoopConditionReferences( return result + condition.slice(cursor) } +/** While loops, and Do While loops given a condition, stop only when their condition does. */ +function isConditionDriven(scope: LoopScope): boolean { + return ( + scope.loopType === 'while' || + (scope.loopType === 'doWhile' && scope.maxIterations === undefined) + ) +} + export type LoopRoute = typeof EDGE.LOOP_CONTINUE | typeof EDGE.LOOP_EXIT export interface LoopContinuationResult { @@ -312,6 +320,10 @@ export class LoopOrchestrator { return await this.createExitResult(ctx, loopId, scope) } + if (isConditionDriven(scope) && scope.iteration + 1 >= DEFAULTS.MAX_CONDITION_LOOP_ITERATIONS) { + await this.failConditionLoopCeiling(ctx, loopId, scope) + } + scope.iteration++ if (scope.items && scope.iteration < scope.items.length) { @@ -332,6 +344,28 @@ export class LoopOrchestrator { return nextIteration >= scope.maxIterations } + /** + * Fails the run once a condition-driven loop has used its iteration ceiling with the condition + * still true. Exiting quietly instead would hand downstream blocks a truncated result as if + * the condition had ended the loop. + */ + private async failConditionLoopCeiling( + ctx: ExecutionContext, + loopId: string, + scope: LoopScope + ): Promise { + const loopType = scope.loopType === 'doWhile' ? 'doWhile' : 'while' + const label = loopType === 'doWhile' ? 'Do While' : 'While' + const limit = DEFAULTS.MAX_CONDITION_LOOP_ITERATIONS + const errorMessage = `${label} loop stopped after ${limit.toLocaleString('en-US')} iterations with its condition still true. Make sure the loop body changes what the condition checks.` + logger.error('Condition loop reached the iteration ceiling', { loopId, loopType, limit }) + await this.addLoopErrorLog(ctx, loopId, loopType, errorMessage, { + iterations: scope.iteration + 1, + }) + scope.validationError = errorMessage + throw new Error(errorMessage) + } + private async createExitResult( ctx: ExecutionContext, loopId: string, diff --git a/scripts/check-explicit-any.baseline.json b/scripts/check-explicit-any.baseline.json index fe4c7d33dea..951c34082d5 100644 --- a/scripts/check-explicit-any.baseline.json +++ b/scripts/check-explicit-any.baseline.json @@ -159,7 +159,7 @@ "apps/sim/executor/handlers/router/handler.ts": 5, "apps/sim/executor/handlers/shared/response-format.ts": 2, "apps/sim/executor/handlers/trigger/handler.ts": 4, - "apps/sim/executor/handlers/variables/handler.ts": 16, + "apps/sim/executor/handlers/variables/handler.ts": 15, "apps/sim/executor/handlers/wait/handler.test.ts": 3, "apps/sim/executor/handlers/wait/handler.ts": 2, "apps/sim/executor/handlers/workflow/custom-block-tool-runner.test.ts": 2,