feat(marqs): optimize batch context fetching in sharedQueueConsumer - #4466
feat(marqs): optimize batch context fetching in sharedQueueConsumer#4466deepshekhardas wants to merge 3 commits into
Conversation
triggerdotdev#2796) When a user removes the machine configuration from a task and redeploys, task.machine becomes undefined. Prisma's create() silently skips undefined fields for Json columns rather than setting them to NULL. This change uses the nullish coalescing operator to explicitly pass null, ensuring the machineConfig column is cleared in the database.
…undWorkerTask create Applied the fix pattern to ensure retryConfig, queueConfig, and payloadSchema are also explicitly cleared when removed from task definition, as suggested in PR feedback.
Efficiently select and map batch relation in AttemptForExecutionGetPayload and _executionFromAttempt to restore batch context during dequeue. Part of Legend Rank mission.
|
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe queue consumer now selects optional task-run batch identifiers and includes the batch friendly ID in execution payloads. Existing work scheduling, deployment selection, and resume payload behavior remain unchanged. Worker task creation now stores absent retry, queue, machine, and payload schema values as ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| console.log("✅ Started the SharedQueueConsumer"); | ||
|
|
||
| this.#doWork().finally(() => {}); | ||
| this.#doWork().finally(() => { }); |
There was a problem hiding this comment.
🟡 Code committed without running the required formatter
Empty callback bodies were rewritten with a stray space inside the braces (() => { } at apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:265 and again at line 420) instead of the formatter's output, so the committed file no longer matches the repository's enforced formatting.
Impact: Formatting checks / pnpm run format will rewrite these lines, adding noise to the diff and to subsequent PRs.
Prettier rule from AGENTS.md and the exact deviations
AGENTS.md ("Coding style") states: "Formatting is enforced using Prettier. Run pnpm run format before committing." Prettier always emits () => {} for an empty arrow body, never () => { }. Both occurrences (apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:265 and apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:420) are purely incidental reformatting unrelated to the PR's purpose.
| this.#doWork().finally(() => { }); | |
| this.#doWork().finally(() => {}); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| batch: attempt.taskRun.batch | ||
| ? { | ||
| id: attempt.taskRun.batch.friendlyId, | ||
| } | ||
| : undefined, |
There was a problem hiding this comment.
🟡 New conditional block is indented differently from what the project formatter produces
The newly added conditional value is written with hand-rolled indentation (batch: attempt.taskRun.batch ? { ... } : undefined at apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:1763-1767) rather than the layout the required formatter produces, so the committed file is not formatter-clean.
Impact: The formatting check will flag the file and re-indent these lines, producing avoidable churn.
Expected Prettier output and other affected spots
AGENTS.md requires running pnpm run format before committing. Prettier indents an object literal inside a ternary branch so its members align under the opening brace:
batch: attempt.taskRun.batch
? {
id: attempt.taskRun.batch.friendlyId,
}
: undefined,
The same unformatted-indentation issue appears in the reflowed deployment ternary (apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:622-627) and in the getResumePayload return type union (apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:1913-1915), both of which are unrelated reformatting of untouched code.
| batch: attempt.taskRun.batch | |
| ? { | |
| id: attempt.taskRun.batch.friendlyId, | |
| } | |
| : undefined, | |
| batch: attempt.taskRun.batch | |
| ? { | |
| id: attempt.taskRun.batch.friendlyId, | |
| } | |
| : undefined, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| retryConfig: task.retry ?? null, | ||
| queueConfig: task.queue ?? null, | ||
| machineConfig: task.machine ?? null, | ||
| triggerSource: task.triggerSource === "schedule" ? "SCHEDULED" : "STANDARD", | ||
| fileId: tasksToBackgroundFiles?.get(task.id) ?? null, | ||
| maxDurationInSeconds: task.maxDuration ? clampMaxDuration(task.maxDuration) : null, | ||
| queueId: queue.id, | ||
| payloadSchema: task.payloadSchema as any, | ||
| payloadSchema: (task.payloadSchema as any) ?? null, |
There was a problem hiding this comment.
🟡 Pull request bundles an unrelated change alongside the stated feature
A separate, unrelated change to how missing task configuration is stored (retryConfig/queueConfig/payloadSchema defaults at apps/webapp/app/v3/services/createBackgroundWorker.server.ts:277-284) is included in a pull request whose stated purpose is optimizing batch context fetching, so the review scope covers two independent issues.
Impact: Reviewers must evaluate two unrelated changes at once, which the project's contribution rules disallow.
Contribution rule and note on the change itself
CONTRIBUTING.md states: "We only accept PRs that address a single issue. Please do not submit PRs containing multiple unrelated fixes or features. If you have multiple contributions, open a separate PR for each one."
Additionally, these fields are only ever supplied to prisma.backgroundWorkerTask.create() for a brand new row, where an omitted (undefined) value already results in a NULL column, so the ?? null additions do not change behaviour; for nullable Json columns Prisma also expects Prisma.DbNull/Prisma.JsonNull rather than a plain null.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Performance optimization for batch context fetching in sharedQueueConsumer.