fix(run-engine): stop task retries consuming the queue nack budget - #4810
Conversation
A task retry whose delay is long enough to go back through the queue was counted as a failed dequeue, so after enough retries the run was dead-lettered and failed with TASK_RUN_DEQUEUED_MAX_RETRIES even though every attempt had actually executed. The queue attempt counter is now reset on that path, since a completed attempt proves the run can start.
|
WalkthroughTask retries now reset the queue dequeue-attempt counter when the worker reports a failure. Engine-detected stalls and dequeue failures keep consuming the redelivery budget. Merge Risk: 🔵 Low · up to The change prevents worker-reported task retries from exhausting the queue redelivery budget while retaining limits for genuine stalls. It is mergeable with owner awareness that separate telemetry for reset versus consumed queue attempts would improve production visibility; no merge-blocking correctness or availability risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4 files. Full details: Description checkExplanation The description provides clear summary, root cause, fix details, and test coverage. It does not use all template headings or include an issue reference, but it contains the main information required for review. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The requeue-processing job is debounced to fire before the retry becomes due and no consumer runs after it in this test, so a dequeue that happened to run first found nothing.
Only a failure the worker reported itself resets the counter. A heartbeat timeout is requeued through the same path, and a run that keeps stalling still has to be bounded by the queue redelivery limit.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (2)
1129-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd crumbs to the new retry-budget path.
Mark the new reset-versus-consume logic with
//@crumbs`` or an@crumbsregion. The markers are removed by `agentcrumbs strip` before merge.As per coding guidelines: Add crumbs as you write code, not just when debugging.
Also applies to: 1289-1289
Source: Coding guidelines
1129-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an OTEL metric for the queue-attempt outcome.
RunAttemptSystemforwards the reset decision toRunQueue.nackMessage, which either resets or consumes the attempt budget. Existing queue metrics emit only genericnackanddlqoperations. Add a counter with boundedoutcomevalues:resetandconsumed. Do not include IDs or error text as attributes.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd2fb169-06df-4a8d-bd95-6fe750a2b9a1
📒 Files selected for processing (1)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (20)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: code-quality / code-quality
🧰 Additional context used
📓 Path-based instructions (6)
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
Integrate OpenTelemetry tracer and meter instrumentation in RunEngine systems for observability
📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
🔇 Additional comments (1)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
1253-1253: 🗄️ Data Integrity & IntegrationNo change is needed for
resetQueueAttempts.The worker failure path passes
resetQueueAttempts: !forceRequeue; ordinary retries passtrue. Stalled and dequeue failure paths omit the option and retainfalse.
3 participants
Summary
A run whose task retries were delayed long enough to go back through the queue could end up failed with
TASK_RUN_DEQUEUED_MAX_RETRIESand statusSYSTEM_FAILUREeven though every attempt had actually executed. The real failure from the final attempt was replaced by that placeholder error, and tasks configured for more retries than the queue redelivery limit never got them.Root cause
Retries with a delay at or above the warm-start threshold are requeued via
tryNackAndRequeue, which nacks the queue message.nackMessageincrements the message attempt counter by default and dead-letters the message once it reaches the queue retry limit. That counter is meant to bound redeliveries of a run that never comes back healthy; a task retry after a worker-reported attempt failure was being charged against it anyway, so a long-backoff retry schedule exhausted it.Fix
nackMessagegains aresetAttemptCountoption that zeroes the counter instead of incrementing it.tryNackAndRequeueexposes it asresetQueueAttempts, and the attempt-retry path passes it only when the worker reported the failure itself (!forceRequeue). Engine-detected stalls (heartbeat timeouts, which are requeued through the same path withforceRequeue), dequeue failures and stalledPENDING_EXECUTINGsnapshots keep consuming the budget, so a run that keeps stalling is still bounded by the queue redelivery limit as before.Tests cover the queue-level reset (no dead-letter at the limit) and an engine-level run that retries past the queue limit and finishes with its own error rather than a system failure. The existing heartbeat timeout tests pin the stall path.