fix(workflows): pin a stored block retry policy when loading it - #6614
Conversation
`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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview In Adds load-path coverage for out-of-range, disabled, incomplete, and null policies, plus a contract test that the write schema still rejects unbounded retry input. Reviewed by Cursor Bugbot for commit bfcec6e. Configure here. |
Greptile SummaryThe PR normalizes retry policies loaded from normalized workflow storage so malformed or out-of-range persisted values satisfy the strict API response contract and match executor behavior.
Confidence Score: 5/5The PR appears safe to merge with no actionable correctness, security, or repository-rule issues identified. The loader now produces schema-valid retry configurations using the executor’s established normalization helpers, preserves disabled settings, and retains strict validation at the write boundary.
|
| Filename | Overview |
|---|---|
| packages/workflow-persistence/src/load.ts | Adds centralized read-time retry-policy normalization using the same numeric bounds and coercion semantics as execution. |
| packages/workflow-persistence/src/load.test.ts | Covers bounded, disabled, incomplete, non-boolean, valid, and absent persisted retry policies. |
| apps/sim/lib/api/contracts/workflows.test.ts | Verifies that read tolerance does not weaken the workflow-state write contract. |
Reviews (1): Last reviewed commit: "fix(workflows): pin a stored block retry..." | Re-trigger Greptile
Summary
A single out-of-range value in
workflow_blocks.retrymakesGET /api/workflows/[id]return 500 forever, bricking the workflow in the editor with no in-product repair. This makes the read tolerant while leaving the write strict.retryis barejsonbtyped asunknown, andload.tsasserted the stored blob was already aBlockRetryConfig:It then handed that straight to the HTTP boundary, where
workflowBlockStateSchemaboundsmaxTriesto 2..5 andwaitBetweenTriesMsto 0..5000. That schema is shared between the PUT/statebody — where the bound is right — and the GET/api/workflows/[id]and/stateresponses, where it is fatal. The response.parsein the shared route builder throws aZodError, which is not anOrchestrationError, so the error policy declines it and it falls through to a 500. Every UI write path reads the workflow first, so there is no way to fix the row from inside the product.The fix constructs a real
BlockRetryConfigfrom the blob using the feature's ownnormalizeBlockRetryTries/normalizeBlockRetryWaitMs, filling defaults for missing fields and carryingenabledacross unchanged.This lands ahead of the data. The
retrycolumn itself is brand new — migration0288_workflow_blocks_retrylanded two days ago in #6458 — so this is a pre-emptive guard arriving essentially alongside the column, not a repair of existing rows. That is the ideal time to add it: once a row goes out of range, the workflow is unopenable and there is no in-product way to fix it.Why the read is the right seam
loadWorkflowFromNormalizedTablesRawis the single read choke point for both apps —@sim/workflow-persistencefor the Next app and the realtime server's full-state emit — so one edit covers every reader, and a normalized row self-heals on the next save. It matchesclampParallelBatchSizea few lines below, which already pins a stored subflow value on the same path.The feature already declares clamp-on-read as its contract: the TSDoc on
resolveBlockRetryConfigsays bounds are clamped on read rather than rejected, andblock-retry.test.tsasserts it. Execution has always honoured that. Only the read boundary disagreed.Six writers persist this column without bounding it:
workflow:batch-add-blocks,workflow:replace-state, andworkflow:update-block-retryon the realtime server — the first two take untyped block recordsv1/admin/workflows/import,v1/admin/workspaces/[id]/import, andsuperuser/import-workflow, which persist externally-authored workflow JSON server-side"Doesn't this silently change a value the user configured?"
No — it makes the UI stop lying. The executor already clamps:
resolveBlockRetryConfigpins the same bounds before a retry runs. Before this change, a block storingmaxTries: 999displayed999in the editor while execution ran 5. Now the editor shows 5, which is what actually happens. The displayed value and the executed value agree for the first time.enabledis carried across rather than resolved, so the numbers a builder configured survive switching retry off and back on.Alternatives rejected
saveWorkflowToNormalizedTablescallers, none of which share a validation seam.BlockRetrySchemain@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.resolveBlockRetryConfig. It returnsnullfor a disabled policy, so it would erase the numbers a builder configured on every read. A dedicated test pins the opposite behavior.Type of Change
Testing
Six cases in
packages/workflow-persistence/src/load.test.ts, four of which are red without theload.tschange (verified by reverting the file and re-running):resolveBlockRetryConfig-based patch ship green and be wrongenabledresolved the way execution reads itNULLreported as no policy (block runs once)Plus a contract test in
apps/sim/lib/api/contracts/workflows.test.tsthat the write bound still rejects out-of-range input.bun run type-checkclean inapps/simandpackages/workflow-persistence;bun run check:api-validationpasses; biome clean.Checklist