perf(run-engine): optimize waitpoint mapping in executionSnapshotSystem - #4467
perf(run-engine): optimize waitpoint mapping in executionSnapshotSystem#4467deepshekhardas wants to merge 4 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.
Refactor enhanceExecutionSnapshotWithWaitpoints to use an Index Map for O(N+M) complexity, replacing a quadratic nested loop. Improves performance for runs with large numbers of waitpoints. Part of Mythic 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 (3)
WalkthroughThe queue consumer now retrieves task run batch identifiers and includes batch metadata in returned executions. Background worker task creation stores absent configuration 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 Warning |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 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.
🔴 Deployed task registration fails when a task has no retry, queue, machine or schema settings
Task records are created with explicit empty values (task.retry ?? null at apps/webapp/app/v3/services/createBackgroundWorker.server.ts:277-279 and 284) instead of simply leaving those settings out, and the database layer rejects that empty value, so registering such tasks silently fails.
Impact: Tasks that don't declare retry/queue/machine/payload-schema options can fail to be registered during deploy or dev registration, and the failure is only logged, leaving those tasks missing from the deployed worker.
Prisma rejects plain null for nullable Json fields
retryConfig, queueConfig, machineConfig and payloadSchema are Json? columns (internal-packages/database/prisma/schema.prisma:559-570). For nullable Json fields Prisma's create input is NullableJsonNullValueInput | InputJsonValue, i.e. Prisma.DbNull / Prisma.JsonNull — passing a bare null is a type error and, at runtime, produces a PrismaClientValidationError ("Invalid value provided. Expected NullableJsonNullValueInput or Json, provided null"). That error is swallowed by the surrounding try/catch in createWorkerTask (apps/webapp/app/v3/services/createBackgroundWorker.server.ts:287-322), which only logs, so the backgroundWorkerTask row is never created.
Also note this is a create, not an update: omitting the field (the previous undefined behaviour) already stores SQL NULL, so the ?? null change provides no benefit.
| 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, | |
| retryConfig: task.retry, | |
| queueConfig: task.queue, | |
| machineConfig: task.machine, | |
| 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, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| console.log("✅ Started the SharedQueueConsumer"); | ||
|
|
||
| this.#doWork().finally(() => {}); | ||
| this.#doWork().finally(() => { }); |
There was a problem hiding this comment.
🟡 Several touched files are no longer formatted per the repository's required formatter
Code in the changed files is re-indented into a style the project's mandated formatter does not produce (e.g. () => { } at apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:265), so the required formatting check is violated.
Impact: The repository's formatting requirement is broken, producing noisy unrelated diffs and failing format checks.
Prettier-enforced style per AGENTS.md
AGENTS.md ("Coding style") states: "Formatting is enforced using Prettier. Run pnpm run format before committing." The PR introduces non-Prettier output in multiple places: .finally(() => { }) at apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:265 and :420, the re-indented nested ternary at apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:623-624 and :1763-1767, the return-type object at :1913-1915, and the object-literal re-indentation in internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts:89-104 and :233-245. Running pnpm run format will revert these.
| this.#doWork().finally(() => { }); | |
| this.#doWork().finally(() => {}); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| batch: { | ||
| select: { | ||
| id: true, | ||
| friendlyId: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🔍 PR contains three unrelated changes across two subsystems
The stated scope is a waitpoint-mapping optimization in the run engine, but the branch also re-adds batch context to the legacy shared-queue execution payload (apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts:1653-1658, :1763-1767) and changes nullable Json handling in apps/webapp/app/v3/services/createBackgroundWorker.server.ts:277-284. CONTRIBUTING.md explicitly asks for one issue per PR; splitting would also make the batch-relation change (which reverses a deliberate // TODO: Removing this for now until we can do it more efficiently) reviewable on its own merits, including the extra join cost per dequeue.
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.
🔍 Batch id semantics in the restored execution payload match the attempt-creation path
batch.id is populated from attempt.taskRun.batch.friendlyId, which matches the convention used when the execution payload is built at attempt-creation time (apps/webapp/app/v3/services/createTaskRunAttempt.server.ts:244-246 uses batchTaskRun.friendlyId as id). One difference worth noting: that path resolves the batch through batchItems/BatchTaskRunItem, while this one uses the direct TaskRun.batchId relation, so runs whose batch association only exists via BatchTaskRunItem would report batch: undefined here, producing inconsistent ctx.batch between initial execution and resume.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Performance optimization for waitpoint mapping in executionSnapshotSystem.