Repository navigation
fix(run-engine): record system failure when dequeue recovery nack dead-letters run (#5106) - #5113
Conversation
…d-letters run (triggerdotdev#5106) In DequeueSystem.dequeueFromWorkerQueue recovery path, when the run read or resolveEnv fails (e.g. during a datastore outage or when runStore.findRun rejects), the code previously called runQueue.nackMessage without inspecting the return value. When attempt >= maxAttempts, nackMessage returns false and dead-letters the message into DLQ, leaving the run indefinitely in QUEUED/PENDING status with no failure recorded. This fix: 1. Wraps controlPlaneResolver.resolveEnv in tryCatch to prevent unhandled rejections from escaping the recovery lock. 2. Checks the boolean result of runQueue.nackMessage: if requeued === false, calls runAttemptSystem.systemFailure with dequeueMaxRetriesError (matching the behavior of the snapshot read recovery branch). 3. Adds regression test dequeueRecoveryNackDlq.test.ts verifying that dead-lettered runs transition to SYSTEM_FAILURE and snapshot to FINISHED.
|
|
Caution Review failedThe pull request is closed.
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. |
|
Hi @granbellllc-png, 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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const store = new Proxy(base, { | ||
| get(target, prop) { | ||
| const value = Reflect.get(target, prop, target); | ||
| if (prop === "findRun") { | ||
| return (...args: any[]) => { | ||
| // Dequeue preparation (dequeueRunSelect) and the recovery read both select | ||
| // runtimeEnvironmentId; the snapshot helper's fallback read only selects { id }. | ||
| // The real findRun is async, so a store outage is a rejected promise, never a sync throw. | ||
| if (armed && args[1]?.select?.runtimeEnvironmentId) { | ||
| injected++; | ||
| return Promise.reject(new Error("injected run-store outage")); | ||
| } | ||
| return (value as Function).apply(target, args); | ||
| }; | ||
| } | ||
| return typeof value === "function" ? value.bind(target) : value; | ||
| }, |
There was a problem hiding this comment.
🟡 Regression test uses a stubbed store
When armed is true, the test's store fabricates rejected findRun calls. The repository forbids stubs and fakes in tests, even alongside real containers.
Learn more
The test passes a proxy as its run store. Its findRun override fabricates a failure while the other methods delegate to the real store. Test rules prohibit mocks, stubs and fakes; AGENTS.md requires using testcontainers instead. The failure injection is the only way this test reaches its target branch, so replacing only the proxy lines will not preserve its coverage.
Example: Once armed becomes true, the test's findRun rejects without querying PostgreSQL; the run reaches recovery because of a simulated store error, not a real store failure.
Recommended fix: Exercise recovery with a genuine database or resolver failure using container-backed dependencies, without replacing findRun behavior with a stub. Preserve the DLQ and final-state assertions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| describe("dequeue recovery nack (#5106)", () => { | ||
| containerTest( | ||
| "a run whose message is dead-lettered by the recovery nack is failed, not left QUEUED", |
There was a problem hiding this comment.
🟡 Regression test omits required comment
The new regression test lacks the mandatory one-line // Catches: <bug> comment. Its reproduction header does not use the required format.
| describe("dequeue recovery nack (#5106)", () => { | |
| containerTest( | |
| "a run whose message is dead-lettered by the recovery nack is failed, not left QUEUED", | |
| // Catches: dead-lettered dequeue recovery leaves the run queued instead of recording system failure. | |
| describe("dequeue recovery nack (#5106)", () => { | |
| containerTest( | |
| "a run whose message is dead-lettered by the recovery nack is failed, not left QUEUED", |
Was this helpful? React with 👍 or 👎 to provide feedback.
Resolves #5106
Summary
In
DequeueSystem.dequeueFromWorkerQueue, when preparing a run for dequeue encounters an error, the recoverycatchpath contains two fallback branches that invokethis.$.runQueue.nackMessage:nackMessage:findRunfails or the environment cannot be resolved) previously dropped the return value ofnackMessage:Because
nackMessagemoves the message to the dead-letter queue (DLQ) and returnsfalseonceattempt >= maxAttempts, ignoring the return value left the run indefinitely inQUEUED/PENDINGstatus with no queue message and no failure recorded. Additionally,controlPlaneResolver.resolveEnvwas awaited outside oftryCatch, risking unhandled rejections that could escape the recovery lock without triggering a nack.Solution
DequeueSystem.dequeueFromWorkerQueue:controlPlaneResolver.resolveEnvintryCatchso environment resolution rejections are handled safely within the recovery branch.const requeued = await this.$.runQueue.nackMessage({ orgId, messageId: runId }).requeued === false(dead-lettered), callthis.runAttemptSystem.systemFailurewithdequeueMaxRetriesErrorandtx: prisma, ensuring the run transitions toSYSTEM_FAILUREand the snapshot toFINISHED.dequeueRecoveryNackDlq.test.tsverifying that whenfindRunrejects and the message is moved to DLQ, the run is transitioned toSYSTEM_FAILURE.