Skip to content

Commit bfcec6e

Browse files
committed
fix(workflows): pin a stored block retry policy when loading it
`workflow_blocks.retry` is a jsonb column written verbatim. Three of its writers never validate what they store: the realtime batch-add and replace-state ops take untyped block records, and the admin/superuser import routes persist externally-authored workflow JSON. `load.ts` then asserted the blob was already a `BlockRetryConfig` and handed it straight to the HTTP boundary, where `workflowBlockStateSchema` bounds `maxTries` to 2..5 and `waitBetweenTriesMs` to 0..5000. That schema is shared between the PUT `/state` body, where the bound is right, and the GET `/api/workflows/[id]` and `/state` responses, where it is fatal. The response `.parse` in the shared route builder throws a ZodError, which is not an `OrchestrationError`, so the error policy declines it and it falls through to a 500. One out-of-range or partial stored value therefore made a workflow permanently unopenable, with no in-product repair — every UI write path reads the workflow first. The feature already declares clamp-on-read as its contract: the commit that added it says bounds are clamped on read rather than rejected, the TSDoc on `resolveBlockRetryConfig` says the same, and `block-retry.test.ts` asserts it. Execution has always honoured that. Only the read boundary disagreed, so that is what this fixes: the loader now constructs a real `BlockRetryConfig` from the blob through `normalizeBlockRetryTries` / `normalizeBlockRetryWaitMs`, filling defaults for missing fields and carrying `enabled` across unchanged. `loadWorkflowFromNormalizedTablesRaw` is the single read choke point for both apps — `@sim/workflow-persistence` for the Next app and the realtime server's full-state emit — so one edit repairs every reader, including rows that are already out of range, and the row self-heals on the next save. It matches `clampParallelBatchSize` a few lines below, which already pins a stored subflow value on the same path. Alternatives rejected: - Validating on write. It leaves every existing bad row fatal forever, and it would have to be repeated across three realtime ops plus roughly a dozen `saveWorkflowToNormalizedTables` callers, none of which share a validation seam. - Bounding `BlockRetrySchema` in `@sim/realtime-protocol`. Its own TSDoc is correct that batch-add and replace-state bypass it, so this closes one writer and leaves the 500. - Relaxing the response contract. It stops the 500 but leaves the editor rendering a number execution will never run. A test now pins the write bound so that shortcut fails loudly. - `resolveBlockRetryConfig`. It returns null for a disabled policy, which would erase the numbers a builder configured every time state is read. Tests: six cases in `packages/workflow-persistence/src/load.test.ts` (four red before this change) covering out-of-range enabled, out-of-range disabled with `enabled` preserved, missing fields, a non-boolean flag, an untouched in-range policy, and NULL meaning "runs once"; plus a contract test that the write bound still rejects out-of-range input.
1 parent 892401a commit bfcec6e

3 files changed

Lines changed: 229 additions & 2 deletions

File tree

apps/sim/lib/api/contracts/workflows.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
internalCancelWorkflowExecutionReasonSchema,
77
updateWorkflowBodySchema,
88
workflowListItemSchema,
9+
workflowStateSchema,
910
} from '@/lib/api/contracts/workflows'
1011
import { PRIVATE_SECRET_PROVENANCE_FIELD } from '@/lib/execution/private-tool-metadata'
1112

@@ -148,4 +149,45 @@ describe('workflow contracts', () => {
148149
expect(cancelWorkflowExecutionReasonSchema.options).not.toContain(reason)
149150
}
150151
})
152+
153+
/**
154+
* `workflowStateSchema` is the PUT `/api/workflows/[id]/state` body and also
155+
* the `state` slot of the GET response. A stored value outside these bounds
156+
* used to 500 the read, which is now prevented by pinning the policy when the
157+
* normalized tables are loaded — not by widening the write contract. Relaxing
158+
* these bounds would let a caller persist a policy the executor will not run.
159+
*/
160+
it('rejects a retry policy outside the bounds on the write contract', () => {
161+
const stateWith = (retry: Record<string, unknown>) => ({
162+
blocks: {
163+
'block-1': {
164+
id: 'block-1',
165+
type: 'api',
166+
name: 'API',
167+
position: { x: 0, y: 0 },
168+
subBlocks: {},
169+
outputs: {},
170+
enabled: true,
171+
retry,
172+
},
173+
},
174+
edges: [],
175+
})
176+
177+
expect(
178+
workflowStateSchema.safeParse(
179+
stateWith({ enabled: true, maxTries: 999, waitBetweenTriesMs: 0 })
180+
).success
181+
).toBe(false)
182+
expect(
183+
workflowStateSchema.safeParse(
184+
stateWith({ enabled: true, maxTries: 3, waitBetweenTriesMs: 10_000_000 })
185+
).success
186+
).toBe(false)
187+
expect(
188+
workflowStateSchema.safeParse(
189+
stateWith({ enabled: true, maxTries: 3, waitBetweenTriesMs: 0 })
190+
).success
191+
).toBe(true)
192+
})
151193
})
Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
type Row = Record<string, unknown>
4+
5+
const tables = vi.hoisted(() => ({
6+
workflow: { name: 'workflow' as const },
7+
workflowBlocks: {
8+
name: 'workflowBlocks' as const,
9+
workflowId: 'workflow_id',
10+
updatedAt: 'updated_at',
11+
},
12+
workflowEdges: { name: 'workflowEdges' as const, workflowId: 'workflow_id' },
13+
workflowSubflows: { name: 'workflowSubflows' as const, workflowId: 'workflow_id' },
14+
}))
15+
16+
vi.mock('@sim/db', () => ({
17+
db: {},
18+
workflow: tables.workflow,
19+
workflowBlocks: tables.workflowBlocks,
20+
workflowEdges: tables.workflowEdges,
21+
workflowSubflows: tables.workflowSubflows,
22+
}))
23+
24+
vi.mock('@sim/logger', () => ({
25+
createLogger: () => ({ debug: vi.fn(), error: vi.fn(), info: vi.fn(), warn: vi.fn() }),
26+
}))
27+
28+
vi.mock('drizzle-orm', () => ({
29+
and: vi.fn(),
30+
eq: vi.fn(),
31+
getTableColumns: vi.fn(() => ({})),
32+
isNull: vi.fn(),
33+
sql: vi.fn(() => 'updated_at::text'),
34+
}))
35+
36+
import { loadWorkflowFromNormalizedTablesRaw } from './load'
37+
38+
/**
39+
* Minimal stand-in for the drizzle query builder the loader uses: every chain
40+
* ends in the rows registered for the table named by `.from(...)`, and the
41+
* chain is awaitable both with and without a trailing `.limit(...)`.
42+
*/
43+
function createTx(rowsByTable: Record<string, Row[]>) {
44+
const resultFor = (rows: Row[]) => {
45+
const result = {
46+
where: () => result,
47+
limit: () => Promise.resolve(rows),
48+
then: (onFulfilled: (rows: Row[]) => unknown) => Promise.resolve(rows).then(onFulfilled),
49+
}
50+
return result
51+
}
52+
53+
return {
54+
select: () => ({
55+
from: (table: { name: string }) => resultFor(rowsByTable[table.name] ?? []),
56+
}),
57+
}
58+
}
59+
60+
function blockRow(retry: unknown): Row {
61+
return {
62+
id: 'block-1',
63+
type: 'api',
64+
name: 'API',
65+
positionX: '0',
66+
positionY: '0',
67+
enabled: true,
68+
horizontalHandles: true,
69+
advancedMode: false,
70+
errorEnabled: false,
71+
retry,
72+
triggerMode: false,
73+
height: '0',
74+
subBlocks: {},
75+
outputs: {},
76+
data: {},
77+
locked: false,
78+
updatedAtText: '2026-01-01 00:00:00.000000',
79+
}
80+
}
81+
82+
async function loadRetry(retry: unknown) {
83+
const tx = createTx({
84+
workflowBlocks: [blockRow(retry)],
85+
workflowEdges: [],
86+
workflowSubflows: [],
87+
workflow: [{ workspaceId: 'workspace-1' }],
88+
})
89+
90+
const loaded = await loadWorkflowFromNormalizedTablesRaw(
91+
'workflow-1',
92+
tx as unknown as Parameters<typeof loadWorkflowFromNormalizedTablesRaw>[1]
93+
)
94+
95+
return loaded?.blocks['block-1'].retry
96+
}
97+
98+
describe('loadWorkflowFromNormalizedTablesRaw retry normalization', () => {
99+
beforeEach(() => {
100+
vi.clearAllMocks()
101+
})
102+
103+
/**
104+
* The `retry` column is jsonb written verbatim by writers that never bound it
105+
* (realtime batch-add and replace-state, the admin/superuser import routes),
106+
* so a stored value can sit outside the range the HTTP read contract demands.
107+
*/
108+
it('pins an out-of-range enabled policy to the bounds', async () => {
109+
expect(
110+
await loadRetry({ enabled: true, maxTries: 999, waitBetweenTriesMs: 10_000_000 })
111+
).toEqual({ enabled: true, maxTries: 5, waitBetweenTriesMs: 5000 })
112+
})
113+
114+
/**
115+
* A disabled policy keeps its configured numbers so switching retry off and
116+
* back on restores them. This is what `resolveBlockRetryConfig` would destroy.
117+
*/
118+
it('pins a disabled policy without discarding it', async () => {
119+
expect(await loadRetry({ enabled: false, maxTries: 99, waitBetweenTriesMs: -4 })).toEqual({
120+
enabled: false,
121+
maxTries: 5,
122+
waitBetweenTriesMs: 0,
123+
})
124+
})
125+
126+
it('fills the defaults for a policy stored with fields missing', async () => {
127+
expect(await loadRetry({ enabled: true })).toEqual({
128+
enabled: true,
129+
maxTries: 3,
130+
waitBetweenTriesMs: 1000,
131+
})
132+
})
133+
134+
it('resolves a non-boolean enabled flag the way execution reads it', async () => {
135+
expect(await loadRetry({ enabled: 'yes', maxTries: 3, waitBetweenTriesMs: 1000 })).toEqual({
136+
enabled: true,
137+
maxTries: 3,
138+
waitBetweenTriesMs: 1000,
139+
})
140+
})
141+
142+
it('leaves an in-range policy untouched', async () => {
143+
expect(await loadRetry({ enabled: true, maxTries: 4, waitBetweenTriesMs: 250 })).toEqual({
144+
enabled: true,
145+
maxTries: 4,
146+
waitBetweenTriesMs: 250,
147+
})
148+
})
149+
150+
/** NULL is reserved for a block that never had a policy: it runs once. */
151+
it('reports no policy for a block that never had one', async () => {
152+
expect(await loadRetry(null)).toBeUndefined()
153+
})
154+
})

packages/workflow-persistence/src/load.ts

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { db, workflow, workflowBlocks, workflowEdges, workflowSubflows } from '@sim/db'
22
import { createLogger } from '@sim/logger'
3-
import type { BlockState, Loop, Parallel } from '@sim/workflow-types/workflow'
3+
import type { BlockRetryConfig, BlockState, Loop, Parallel } from '@sim/workflow-types/workflow'
44
import {
5+
normalizeBlockRetryTries,
6+
normalizeBlockRetryWaitMs,
57
normalizeWorkflowEdgeSourceHandle,
68
normalizeWorkflowEdgeTargetHandle,
79
SUBFLOW_TYPES,
@@ -13,6 +15,35 @@ import type { DbOrTx, NormalizedWorkflowData } from './types'
1315

1416
const logger = createLogger('WorkflowPersistenceLoad')
1517

18+
/**
19+
* Rebuilds a stored retry policy as a real {@link BlockRetryConfig} instead of
20+
* asserting that the raw `jsonb` blob already is one.
21+
*
22+
* The column is written verbatim by writers that never validate its contents —
23+
* the realtime batch-add and replace-state ops take untyped block records, and
24+
* the admin/superuser import routes persist externally-authored workflow JSON —
25+
* so a row can hold an out-of-range number, a missing field, or a non-boolean
26+
* flag. Pinning the values here is the policy the feature declares (see
27+
* `resolveBlockRetryConfig`) and applies it to every reader at once: the editor
28+
* renders exactly the numbers execution will use, and the strict HTTP contract
29+
* that serves this state can never reject a workflow it is meant to open.
30+
*
31+
* `enabled` is carried across rather than resolved, so the numbers a builder
32+
* configured survive switching retry off and back on. That is why
33+
* `resolveBlockRetryConfig` — which collapses a disabled policy to `null` —
34+
* must not be used on this path.
35+
*/
36+
function normalizeStoredBlockRetry(stored: unknown): BlockRetryConfig | undefined {
37+
if (stored == null || typeof stored !== 'object') return undefined
38+
const retry = stored as Record<string, unknown>
39+
40+
return {
41+
enabled: Boolean(retry.enabled),
42+
maxTries: normalizeBlockRetryTries(retry.maxTries),
43+
waitBetweenTriesMs: normalizeBlockRetryWaitMs(retry.waitBetweenTriesMs),
44+
}
45+
}
46+
1647
export interface RawNormalizedWorkflow extends NormalizedWorkflowData {
1748
workspaceId: string
1849
/**
@@ -87,7 +118,7 @@ export async function loadWorkflowFromNormalizedTablesRaw(
87118
horizontalHandles: block.horizontalHandles,
88119
advancedMode: block.advancedMode,
89120
errorEnabled: block.errorEnabled,
90-
retry: (block.retry as BlockState['retry']) ?? undefined,
121+
retry: normalizeStoredBlockRetry(block.retry),
91122
triggerMode: block.triggerMode,
92123
height: Number(block.height),
93124
subBlocks: (block.subBlocks as BlockState['subBlocks']) || {},

0 commit comments

Comments
 (0)