fix(run-engine,webapp): guard run finalization against lost resume signals - #4849
Conversation
…gnals A database error between a run's finish commit and its waitpoint completion could permanently strand a parent blocked on triggerAndWait or batchTriggerAndWait: the child's retries hit the already-finished guard and nothing ever re-delivered the completion signal. Every finalizing path now enqueues a durable ensureRunFinalized job before the finish commit and acks it once the inline side effects (waitpoint completion, unblock fan-out, batch nudge) all succeed. If the inline path dies in between, the guard re-derives the lost signal from the run row. All legs are idempotent, and the job never dead-letters so it outlives any database outage.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (33)
🧰 Additional context used📓 Path-based instructions (9)We use vitest exclusively. **Never mock anything** - use testcontainers instead.📄 CodeRabbit inference engine (AGENTS.md) Files:
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:📄 CodeRabbit inference engine (AGENTS.md) Files:
Add crumbs as you write code — not just when debugging. Mark lines with📄 CodeRabbit inference engine (AGENTS.md) Files:
Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md) Files:
Integrate OpenTelemetry tracer and meter instrumentation in RunEngine systems for observability📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md) Files:
Use vitest for all tests in the Trigger.dev repository📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
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:
🔇 Additional comments (4)
WalkthroughAdds a delayed Merge Risk: 🟡 Moderate · up to Terminal-run recovery can still resume dependent workflows before canceled execution has fully stopped, while a runtime-environment lookup failure can leave queue or concurrency capacity unreleased. These bounded failure paths can cause inconsistent workflow behavior or reduced availability and require explicit owner acceptance or fixes before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed and relevant summary, fix explanation, test coverage, and known edge case, but it does not follow the repository template. It omits the issue reference, checklist, explicit Testing section with test steps, Changelog section, and Screenshots section. Resolution Add the required template sections. Include the issue number after "Closes #", complete the checklist, document the exact testing steps and commands, add a short changelog entry, and include screenshots or state that screenshots are not applicable. ✨ Finishing Touches 💡 1📝 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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.
…queue failure A guard surviving a failed finish commit could fire after a later cancellation while the worker was still executing in PENDING_CANCEL, resuming the parent before execution actually stopped. CANCELED is the one terminal status where execution may still be in flight, so the guard now re-delivers for a canceled run only once its latest snapshot is FINISHED; the cancellation finalize path owns the window before that. Also documents the real dead-letter behavior (about five weeks of retries at the capped backoff, then DLQ for visibility) and adds tests for the failed-guard-enqueue and cancel-race shapes.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Three hardening changes to the run finalization guard: The cancel path enqueued the guard after the CANCELED status commit, so a crash between the two could still strand a waiting parent. The enqueue now happens before the status write, restoring the write-ahead invariant on every finalizing path. A guard that finds a canceled run whose execution has not reached FINISHED now re-arms itself instead of exiting, so the watcher survives the PENDING_CANCEL window without resuming the parent early. The guard now also re-delivers the queue ack, releasing the queue message and concurrency slot when the inline path died before acknowledging, and TTL expiry gets the same guard: both the per-run path (whose worker retry was defeated by the not-PENDING early return) and the batch path (which swallowed a failed waitpoint-job enqueue) now arm it before the EXPIRED commit.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The CANCELED gate treated any non-FINISHED snapshot as a cancellation in flight, but only the executing, pending-executing and pending-cancel states carry heartbeats that force the finalize path. A canceled run crashed while queued, delayed, created or suspended has nobody left to produce a FINISHED snapshot, so the guard re-armed forever and the waiting parent stayed stranded. The deferral now matches the worker-owned states exactly and the guard delivers in every other snapshot state. Adds a crash-recovery test for a queued child canceled just before its FINISHED snapshot.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The guard's watch on an in-flight cancellation relied on the snapshot heartbeat to force the finalize path, but the heartbeat enqueue is a separate write after the snapshot commit and can itself be lost, leaving the guard deferring forever. The deferral now carries a counter and delivers after ten cycles: resuming the parent with the identical cancellation error beats an invisible infinite watch, and the budget exhaustion is logged and counted.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
Summary
A run's finish commit and its follow-up side effects (completing the associated waitpoint, waking blocked parents, releasing the queue slot, nudging batch completion) are separate writes across Postgres and Redis. If a database error landed between them, the child run was already finished, so the runner's retries hit the "Run is already finished" guard and the completion signal was lost for good. A parent blocked on
triggerAndWaitorbatchTriggerAndWaitthen stayed waiting forever. TTL expiry had the same shape: its worker retry returned early on a non-pending run, and the batch expiry path swallowed a failed waitpoint-job enqueue.Fix
Every finalizing path (attempt success, permanent failure, cancellation, TTL expiry) now enqueues a durable
ensureRunFinalizedjob before the finish commit, and acks it once the inline side effects all succeed. In steady state the guard never executes; the cost is one Redis enqueue and ack per completion.When the inline path dies in between, the guard fires after a short delay and re-derives everything from current state: it releases the run's queue message and concurrency slot, completes a still-pending associated waitpoint from the run row's output or error, re-runs the blocked-run fan-out (covering a lost unblock enqueue even after the waitpoint committed), and re-schedules the batch completion check. Every leg is idempotent, so racing the inline path is a no-op. The job retries with a capped backoff for roughly five weeks before dead-lettering, so it outlives any database outage while a genuinely poisoned item still becomes visible.
Cancellation gets special handling: CANCELED is the only terminal run status where execution can still be in flight, so the guard only re-delivers for a canceled run once its execution snapshot is FINISHED, re-arming itself until then rather than resuming the parent while the child is still winding down.
A
finalization_rederivationscounter increments whenever the guard actually re-delivers a lost signal; it should stay at zero in a healthy system.Tests cover six shapes: waitpoint completion lost after the finish commit, unblock fan-out lost after the waitpoint completed, a failed guard enqueue failing the completion request with nothing committed, a stale guard held back during an in-flight cancellation, waitpoint completion lost during TTL expiry, and the happy path where the guard is acked and never runs.
Known accepted edge: a guard re-run after a partial inline completion can re-emit a cached-run completion event for the same span; this only happens during failure recovery and is bounded to duplicate trace events.