refactor(run-engine): route TTL expiration through the batch path only#3554
refactor(run-engine): route TTL expiration through the batch path only#3554ericallam wants to merge 2 commits into
Conversation
TTL expiration on queued runs was being scheduled twice: once via a per-run expireRun worker job (the original implementation) and once via the batch TTL system (added more recently). The per-run job almost always won the race, leaving the batch consumer to observe runs already expired by the older path. Collapse onto the batch path. engine.trigger and delayedRunSystem.enqueueDelayedRun no longer schedule the per-run expireRun job. The TTL sorted set + batch consumer is now the only mechanism that flips queued runs to EXPIRED. Delayed runs get the same coverage by passing includeTtl: true on their post-delay enqueue, so the TTL is armed from the moment the run enters the queue (matching how the old job behaved). The new path intentionally does not re-expire runs that have already been allocated a concurrency slot — TTL is for runs that are queued and have never started.
|
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
🧰 Additional context used📓 Path-based instructions (12)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-engine/src/engine/systems/**/*.ts📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Files:
{apps,internal-packages}/**/*.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{package.json,**/*.{ts,tsx,js}}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx,json,md,css,scss}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/tests/**/*.test.ts📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Files:
**/*.test.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-03-22T13:26:12.060ZApplied to files:
📚 Learning: 2026-03-22T19:24:14.403ZApplied to files:
🔇 Additional comments (4)
WalkthroughThis PR consolidates TTL expiration handling so TTL is armed when a run is enqueued (includeTtl: true) and the TTL consumer batch path is the primary mechanism that marks runs EXPIRED. Legacy per-run expire jobs are retained only for DEVELOPMENT runtime. Tests are updated with ttlSystem.batchMaxWaitMs and concurrency controls and a new contract test confirms re-enqueued runs without TTL arming do not expire. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
…endingVersion re-enqueue Two gaps in the previous commit's collapse onto the batch TTL path: - DEV runs are fast-pathed straight to the worker queue and never enter the TTL sorted set. If the dev CLI isn't running, those runs would sit on the worker queue forever without the batch path ever seeing them. In non-DEV environments runs always execute once they're on the worker queue, so this isn't a concern there. Restore the legacy per-run expireRun job, gated on environment.type === "DEVELOPMENT". - pendingVersionSystem.enqueueRunsForBackgroundWorker re-enqueues runs that were waiting for a matching worker version. That re-enqueue is the first time the run is actually queued for a worker, so pass includeTtl: true to arm TTL on the new message. Adds a DEV TTL test that exercises the fast-path coverage and a pendingVersion test that asserts ttlExpiresAt is present after re-enqueue.
1 participant
Summary
TTL expiration on queued runs was being scheduled twice: once via a per-run
expireRunworker job (the original implementation) and once via the batch TTL system (added more recently). Both paths attempt to flip the same run toEXPIRED. The per-run job almost always won the race, leaving the batch consumer to observe runs already expired by the older path.This collapses TTL expiration onto the batch path so every queued TTLed run goes through a single Redis-backed sorted set + batch consumer instead of also getting its own scheduled redis-worker job.
Design
engine.triggeranddelayedRunSystem.enqueueDelayedRunno longer callttlSystem.scheduleExpireRun. The remainingenqueueSystem.enqueueRun({ includeTtl: true })already adds the run to the TTL sorted set;TtlSystem.expireRunsBatchflips it toEXPIREDwhen the TTL fires.Delayed runs get the same coverage by passing
includeTtl: trueon their post-delay enqueue, so the TTL is armed from the moment the run enters the queue (matching how the old job behaved —parseNaturalLanguageDurationis evaluated at enqueue time).The new path explicitly does not re-expire runs once they have been allocated a concurrency slot. That is intentional: TTL is for runs that are queued and have never started. Once a run has a slot it is on its way to executing.
Test plan
pnpm run test --filter @internal/run-engine ./src/engine/tests/ttl.test.ts— 15 tests, including a new "Re-enqueued runs are not expired by TTL once they have started" that locks in the queued-and-never-started contract.pnpm run test --filter @internal/run-engine ./src/engine/tests/delays.test.ts— 5 tests, including "Delayed run with a ttl" which now also asserts the TTL is armed from queue-enter time, notcreatedAt.pnpm run test --filter @internal/run-engine ./src/engine/tests/lazyWaitpoint.test.ts— 12 tests.pnpm run typecheck --filter @internal/run-engine.