feat(run-engine): trigger tasks pinned to an external deployment id - #4664
feat(run-engine): trigger tasks pinned to an external deployment id#46640ski wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: bb5d381 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The SDK discovers an external deployment id at runtime (explicit TRIGGER_EXTERNAL_DEPLOYMENT_ID always; platform commit-SHA variables and generic fallbacks when TRIGGER_AUTOMATIC_SKEW_VERSION_PROTECTION=1) and sends it alongside lockToVersion; the server resolves precedence (version > external id > current). An id held by a deployed deployment pins the run to that worker; an in-flight or unknown id parks the run in PENDING_VERSION with the id in TaskRun.annotations, wakes it pinned when a deployment carrying the id finalizes (ClickHouse candidates, Postgres authoritative), and expires it after a deadline that re-checks Postgres before acting. Parking outranks delaying and preserves delayUntil. The id is projected to ClickHouse task_runs_v2.external_deployment_id during replication. Redis cache for id-to-worker resolution, guarded version-aware writes. Ids are not unique. Several deployments can hold one id - a --force rebuild is the ordinary way to get there - so resolution always picks the highest version among the candidates, never the newest by timestamp. The rule is applied identically on both paths that can bind a run to a worker: resolveExternalDeployment at trigger time, and PendingVersionSystem when a landing deployment wakes a parked run. Version comparison is numeric on the counter half, so 20260807.10 outranks 20260807.9. A run whose id never lands expires at the deadline with EXTERNAL_DEPLOYMENT_NOT_FOUND and an error naming the id it waited for, which is what a failed build or a typo looks like from the caller. Default deadline is one hour (EXTERNAL_DEPLOYMENT_PARK_DEADLINE_MS). Debounce registration happens in both the parked and the delayed branch through one helper, so a debounced run that parks still binds its debounce key; without it every later trigger for the same key created another parked run, and all of them executed when the deployment landed. The two DELAYED-only status checks in DebounceSystem also accept PENDING_VERSION, without which the lock-contention fallback would rethrow a 5xx the SDK retries and amplifies, and the fast path would push every trigger on a parked key through the redlock. Resolution is skipped in development. A dev environment cannot hold a WorkerDeployment - trigger dev registers a BackgroundWorker with nothing behind it, and deploy --env refuses dev - so an external deployment id there could only ever park, and the parked run then expired against the dev TTL while a connected dev worker sat idle. The id is still annotated so the dashboard shows what the app sent (TRI-13000).
8f7617a to
bb5d381
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
| if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === "P2025") { | ||
| return { count: 0 }; |
There was a problem hiding this comment.
🟡 Runs that expire and get released at the same moment can crash the background job instead of quietly doing nothing
The "row no longer matches" case is only recognised when the error object comes from the control-plane database client (error instanceof Prisma.PrismaClientKnownRequestError at internal-packages/run-store/src/PostgresRunStore.ts:1381), so on the split database setup the harmless race is rethrown as a hard failure instead of being treated as a no-op.
Impact: When a waiting run is released by a landing deployment at the same instant its wait deadline fires, the deadline job errors out and retries repeatedly, filling logs with spurious failures.
Cross-generation Prisma error classes make the P2025 guard ineffective on the dedicated run-ops client
expireParkedRun issues prisma.taskRun.update({ where: { id, status: "PENDING_VERSION" } }) directly (not through #updateTaskRunWithSelect, which routes errors through normalizeRunOpsError at internal-packages/run-store/src/PostgresRunStore.ts:554-559). When the run has already been promoted out of PENDING_VERSION, Prisma raises P2025. The store's own comment block at internal-packages/run-store/src/PostgresRunStore.ts:485-500 documents that the run-ops generated client has its OWN PrismaClientKnownRequestError class object, so error instanceof Prisma.PrismaClientKnownRequestError is false for errors raised by that client. The return { count: 0 } branch is therefore skipped and the error escapes PendingVersionSystem.expireParkedExternalDeploymentRun, failing the expireParkedExternalDeploymentRun worker job instead of returning cleanly.
The existing helpers key on the runtime name + string code (isForeignPrismaKnownRequestError) precisely for this reason; the new guard should do the same (or route the write through the normalizing helper).
Prompt for agents
In PostgresRunStore.expireParkedRun the P2025 catch uses `error instanceof Prisma.PrismaClientKnownRequestError`. As documented near isForeignPrismaKnownRequestError/normalizeRunOpsError in the same file, errors raised by the run-ops generated client are instances of a DIFFERENT PrismaClientKnownRequestError class, so instanceof is false and the intended `return { count: 0 }` no-op is skipped — the error escapes and fails the expireParkedExternalDeploymentRun worker job whenever the run was concurrently promoted out of PENDING_VERSION. Detect the P2025 in a generation-agnostic way (e.g. reuse the existing foreign-error detection that keys on the runtime `name` plus the string `code`, or route the update through the helper that already normalizes errors) so both schema variants behave identically.
Was this helpful? React with 👍 or 👎 to provide feedback.
| prisma | ||
| ); | ||
| if (!probe || probe.status !== "DELAYED" || !probe.delayUntil) { | ||
| if (!probe || !DEBOUNCEABLE_RUN_STATUSES.includes(probe.status) || !probe.delayUntil) { |
There was a problem hiding this comment.
🟡 Repeated triggers on a waiting, debounced run all take the slow contended path they were meant to skip
The quick check that lets a repeated debounce trigger return the existing run without taking the distributed lock was widened to include runs waiting on a deployment (DEBOUNCEABLE_RUN_STATUSES.includes(probe.status) at internal-packages/run-engine/src/engine/systems/debounceSystem.ts:649) but the second half of the same check was left as before, so such runs always fall through to the lock anyway.
Impact: A hot debounce key whose run is waiting on a deployment funnels every trigger through the shared lock, adding latency and lock contention on the busiest code path.
The follow-up full-run read still hard-codes DELAYED
#tryFastPathSkip was updated in two places (the probe at internal-packages/run-engine/src/engine/systems/debounceSystem.ts:649 and the lock-contention fallback at :707), but the full-run re-read a few lines later still reads if (!fullRun || fullRun.status !== "DELAYED") return null; (internal-packages/run-engine/src/engine/systems/debounceSystem.ts:672). A run parked with PENDING_VERSION passes the probe and then always returns null there, so the fast path can never short-circuit for parked keys — the exact case the change set out to cover. Behaviour stays correct (the locked path re-checks and skips rescheduling), but the redlock is acquired on every trigger for that key.
Prompt for agents
In DebounceSystem.#tryFastPathSkip the initial probe now accepts both DELAYED and PENDING_VERSION via DEBOUNCEABLE_RUN_STATUSES, but the subsequent full-run read still bails on `fullRun.status !== "DELAYED"`, so the unlocked fast path never short-circuits for a run parked on an external deployment id and every trigger on that debounce key takes the redlock. Make the second status check use the same DEBOUNCEABLE_RUN_STATUSES set so the two checks agree.
Was this helpful? React with 👍 or 👎 to provide feedback.
| expect(byRunId.get(withId.id)).toBe("fa1eade47b73733d6312d5abfad33ce9e4068081"); | ||
| expect(byRunId.get(withoutId.id)).toBe(""); | ||
| expect(byRunId.get(withUnparseableAnnotations.id)).toBe("commit-on-a-broken-blob"); |
There was a problem hiding this comment.
🔍 New replication test contradicts the statusReason gate and will fail
The new ClickHouse projection is deliberately gated on run.statusReason === PARKED_ON_EXTERNAL_DEPLOYMENT_STATUS_REASON (apps/webapp/app/services/runsReplicationService.server.ts:1369), which is required for correctness: the pending-version lookup filters external_deployment_id = '' when the landing deployment carries no external id (apps/webapp/app/v3/services/clickhousePendingVersionLookup.server.ts:87), so a run that merely reports an id in annotations but is parked for NO_WORKER must project an empty column or it would never be released — exactly the case the engine test "releases a run that reports an external deployment id but is parked for an unrelated reason" asserts.
The new replication test, however, creates all three runs with statusReason unset and asserts the id is projected anyway (apps/webapp/test/runsReplicationServiceExternalDeploymentId.test.ts:147,149). With the gate in place those rows project "", so the test fails. Either the test needs to set statusReason: "EXTERNAL_DEPLOYMENT_PENDING" on the two rows expected to carry an id, or the intended semantics differ from the code — worth confirming which before merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
The SDK discovers an external deployment id at runtime (explicit TRIGGER_EXTERNAL_DEPLOYMENT_ID always; platform commit-SHA variables and generic fallbacks when TRIGGER_AUTOMATIC_SKEW_VERSION_PROTECTION=1) and sends it alongside lockToVersion; the server resolves precedence (version > external id > current). An id held by a deployed deployment pins the run to that worker; an in-flight or unknown id parks the run in PENDING_VERSION with the id in TaskRun.annotations, wakes it pinned when a deployment carrying the id finalizes (ClickHouse candidates, Postgres authoritative), and expires it after a deadline that re-checks Postgres before acting. Parking outranks delaying and preserves delayUntil. The id is projected to ClickHouse task_runs_v2.external_deployment_id during replication. Redis cache for id-to-worker resolution, guarded version-aware writes.
Ids are not unique. Several deployments can hold one id - a --force rebuild is the ordinary way to get there - so resolution always picks the highest version among the candidates, never the newest by timestamp. The rule is applied identically on both paths that can bind a run to a worker: resolveExternalDeployment at trigger time, and PendingVersionSystem when a landing deployment wakes a parked run. Version comparison is numeric on the counter half, so 20260807.10 outranks 20260807.9.
A run whose id never lands expires at the deadline with EXTERNAL_DEPLOYMENT_NOT_FOUND and an error naming the id it waited for, which is what a failed build or a typo looks like from the caller. Default deadline is one hour (EXTERNAL_DEPLOYMENT_PARK_DEADLINE_MS).
Debounce registration happens in both the parked and the delayed branch through one helper, so a debounced run that parks still binds its debounce key; without it every later trigger for the same key created another parked run, and all of them executed when the deployment landed. The two DELAYED-only status checks in DebounceSystem also accept PENDING_VERSION, without which the lock-contention fallback would rethrow a 5xx the SDK retries and amplifies, and the fast path would push every trigger on a parked key through the redlock.
Resolution is skipped in development. A dev environment cannot hold a WorkerDeployment - trigger dev registers a BackgroundWorker with nothing behind it, and deploy --env refuses dev - so an external deployment id there could only ever park, and the parked run then expired against the dev TTL while a connected dev worker sat idle. The id is still annotated so the dashboard shows what the app sent (TRI-13000).