-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix(cjson): santize bullmq payloads accurately #3892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
93efac6
6cc1233
584d591
c4987da
6b9a1bc
d97f27e
72d8a68
f52ab39
d34d6f4
9fc22a5
07716cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| import type { ConnectionOptions } from 'bullmq' | ||
| import { env, isTruthy } from '@/lib/core/config/env' | ||
| import { env } from '@/lib/core/config/env' | ||
|
|
||
| export function isBullMQEnabled(): boolean { | ||
| return isTruthy(env.CONCURRENCY_CONTROL_ENABLED) && Boolean(env.REDIS_URL) | ||
| return Boolean(env.REDIS_URL) | ||
| } | ||
|
Comment on lines
4
to
6
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The old guard ( If the goal is to consolidate configuration, consider keeping |
||
|
|
||
| export function getBullMQConnectionOptions(): ConnectionOptions { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,22 +28,22 @@ function getQueueDefaultOptions(type: JobType) { | |
| return { | ||
| attempts: 3, | ||
| backoff: { type: 'exponential' as const, delay: 1000 }, | ||
| removeOnComplete: { age: 24 * 60 * 60 }, | ||
| removeOnFail: { age: 7 * 24 * 60 * 60 }, | ||
| removeOnComplete: { age: 2 * 60 * 60, count: 1000 }, | ||
| removeOnFail: { age: 2 * 60 * 60, count: 500 }, | ||
| } | ||
| case 'webhook-execution': | ||
| return { | ||
|
Comment on lines
+31
to
35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All BullMQ queue retention has been reduced from 24 h / 7 days down to 2 hours, and the same change applies to
The previous |
||
| attempts: 2, | ||
| backoff: { type: 'exponential' as const, delay: 2000 }, | ||
| removeOnComplete: { age: 24 * 60 * 60 }, | ||
| removeOnFail: { age: 3 * 24 * 60 * 60 }, | ||
| removeOnComplete: { age: 2 * 60 * 60, count: 1000 }, | ||
| removeOnFail: { age: 2 * 60 * 60, count: 500 }, | ||
| } | ||
| case 'schedule-execution': | ||
| return { | ||
| attempts: 2, | ||
| backoff: { type: 'exponential' as const, delay: 5000 }, | ||
| removeOnComplete: { age: 24 * 60 * 60 }, | ||
| removeOnFail: { age: 3 * 24 * 60 * 60 }, | ||
| removeOnComplete: { age: 2 * 60 * 60, count: 1000 }, | ||
| removeOnFail: { age: 2 * 60 * 60, count: 500 }, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -69,8 +69,8 @@ function createNamedQueue( | |
| defaultJobOptions: { | ||
| attempts: 3, | ||
| backoff: { type: 'exponential', delay: 5000 }, | ||
| removeOnComplete: { age: 24 * 60 * 60 }, | ||
| removeOnFail: { age: 7 * 24 * 60 * 60 }, | ||
| removeOnComplete: { age: 2 * 60 * 60, count: 500 }, | ||
| removeOnFail: { age: 2 * 60 * 60, count: 200 }, | ||
| }, | ||
| }) | ||
| case KNOWLEDGE_DOCUMENT_PROCESSING_QUEUE: | ||
|
|
@@ -79,26 +79,26 @@ function createNamedQueue( | |
| defaultJobOptions: { | ||
| attempts: 3, | ||
| backoff: { type: 'exponential', delay: 1000 }, | ||
| removeOnComplete: { age: 24 * 60 * 60 }, | ||
| removeOnFail: { age: 7 * 24 * 60 * 60 }, | ||
| removeOnComplete: { age: 2 * 60 * 60, count: 500 }, | ||
| removeOnFail: { age: 2 * 60 * 60, count: 200 }, | ||
| }, | ||
| }) | ||
| case MOTHERSHIP_JOB_EXECUTION_QUEUE: | ||
| return new Queue(name, { | ||
| connection: getBullMQConnectionOptions(), | ||
| defaultJobOptions: { | ||
| attempts: 1, | ||
| removeOnComplete: { age: 24 * 60 * 60 }, | ||
| removeOnFail: { age: 7 * 24 * 60 * 60 }, | ||
| removeOnComplete: { age: 2 * 60 * 60, count: 500 }, | ||
| removeOnFail: { age: 2 * 60 * 60, count: 200 }, | ||
| }, | ||
| }) | ||
| case WORKSPACE_NOTIFICATION_DELIVERY_QUEUE: | ||
| return new Queue(name, { | ||
| connection: getBullMQConnectionOptions(), | ||
| defaultJobOptions: { | ||
| attempts: 1, | ||
| removeOnComplete: { age: 24 * 60 * 60 }, | ||
| removeOnFail: { age: 7 * 24 * 60 * 60 }, | ||
| removeOnComplete: { age: 2 * 60 * 60, count: 500 }, | ||
| removeOnFail: { age: 2 * 60 * 60, count: 200 }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| /** | ||
| * Sanitization for JSON data round-tripped through Redis Lua cjson. | ||
| * | ||
| * Lua's cjson library cannot distinguish between empty arrays `[]` and empty objects `{}`. | ||
| * Both serialize to `{}` in Lua tables. When BullMQ's internal Lua scripts touch job data, | ||
| * any empty array in the payload silently becomes `{}`. | ||
| * | ||
| * Applied once at the worker boundary before data enters the execution engine. | ||
| */ | ||
|
|
||
| /** | ||
| * Returns `value` if it's an array, otherwise `[]`. | ||
| */ | ||
| export function ensureArray<T>(value: unknown): T[] { | ||
| return Array.isArray(value) ? value : [] | ||
| } | ||
|
|
||
| const EXECUTION_STATE_ARRAY_FIELDS = [ | ||
| 'executedBlocks', | ||
| 'blockLogs', | ||
| 'completedLoops', | ||
| 'activeExecutionPath', | ||
| 'pendingQueue', | ||
| 'remainingEdges', | ||
| 'completedPauseContexts', | ||
| ] | ||
|
|
||
| /** | ||
| * Normalizes all known array fields on a BullMQ-deserialized workflow execution payload. | ||
| * Mutates in place — call once before passing into the execution engine. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function sanitizeBullMQPayload(payload: any): void { | ||
| if (!payload) return | ||
|
|
||
| payload.selectedOutputs = ensureArray(payload.selectedOutputs) | ||
|
|
||
| if (payload.metadata) { | ||
| payload.metadata.callChain = ensureArray(payload.metadata.callChain) | ||
|
|
||
| if (payload.metadata.pendingBlocks !== undefined) { | ||
| payload.metadata.pendingBlocks = ensureArray(payload.metadata.pendingBlocks) | ||
| } | ||
|
|
||
| if (payload.metadata.workflowStateOverride?.edges !== undefined) { | ||
| payload.metadata.workflowStateOverride.edges = ensureArray( | ||
| payload.metadata.workflowStateOverride.edges | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| if (payload.runFromBlock?.sourceSnapshot) { | ||
| const state = payload.runFromBlock.sourceSnapshot | ||
| for (const field of EXECUTION_STATE_ARRAY_FIELDS) { | ||
| if (field in state && !Array.isArray(state[field])) { | ||
| state[field] = [] | ||
| } | ||
| } | ||
|
|
||
| if (state.dagIncomingEdges && typeof state.dagIncomingEdges === 'object') { | ||
| for (const key of Object.keys(state.dagIncomingEdges)) { | ||
| if (!Array.isArray(state.dagIncomingEdges[key])) { | ||
| state.dagIncomingEdges[key] = [] | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+33
to
+67
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The PR checklist marks "Tests added/updated and passing", but no test file for Consider adding a dedicated test file that exercises:
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isTriggerDevEnabledused inconsistently as a value vs. function callisBullMQEnabled()is called as a function (parentheses), whileisTriggerDevEnabledis referenced as a constant (no parentheses). Both are correct —isBullMQEnabledis a function andisTriggerDevEnabledis a module-level constant — but the asymmetry can confuse readers who might expect both to be function calls given the naming conventionisFoo.The same pattern appears at
apps/sim/app/api/workflows/[id]/execute/route.ts(line 246) andapps/sim/lib/webhooks/processor.ts(line 1268). No code change is strictly necessary, but adding an inline comment clarifying thatisTriggerDevEnabledis a constant (evaluated at module load) would aid readability.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!