fix(batch): add batch queue back into master queue after visibility timeout #2814
Conversation
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change threads a new masterQueueKey through the fair-queue visibility and message-handling paths. VisibilityManager APIs and the internal release/reclaim flows now accept and propagate masterQueueKey so releases, delays, retries, and timed-out reclaim operations update master-queue metadata. The Lua releaseMessage script and its Redis command arity were extended (adds masterQueueKey as a KEY and queueId in ARGV). Several private handlers and tests were updated to pass the new parameter; some error logging calls were reformatted. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Summary of changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,json,md,css,scss}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)packages/redis-worker/src/fair-queue/visibility.ts (2)
⏰ 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). (24)
🔇 Additional comments (4)
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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/redis-worker/src/fair-queue/index.ts (1)
1120-1127: Minor refactor: Error logging consolidated to single object parameter.The error logging call has been reformatted to pass a single structured object parameter rather than multiple string parameters, improving consistency with structured logging practices.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/redis-worker/src/fair-queue/index.tspackages/redis-worker/src/fair-queue/tests/raceConditions.test.tspackages/redis-worker/src/fair-queue/visibility.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/redis-worker/src/fair-queue/tests/raceConditions.test.tspackages/redis-worker/src/fair-queue/index.tspackages/redis-worker/src/fair-queue/visibility.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/redis-worker/src/fair-queue/tests/raceConditions.test.tspackages/redis-worker/src/fair-queue/index.tspackages/redis-worker/src/fair-queue/visibility.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
packages/redis-worker/src/fair-queue/tests/raceConditions.test.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/redis-worker/src/fair-queue/tests/raceConditions.test.tspackages/redis-worker/src/fair-queue/index.tspackages/redis-worker/src/fair-queue/visibility.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Avoid mocks or stubs in tests; use helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for unit tests
Files:
packages/redis-worker/src/fair-queue/tests/raceConditions.test.ts
🧬 Code graph analysis (2)
packages/redis-worker/src/fair-queue/tests/raceConditions.test.ts (1)
packages/redis-worker/src/fair-queue/schedulers/drr.ts (1)
keys(184-187)
packages/redis-worker/src/fair-queue/visibility.ts (1)
packages/redis-worker/src/fair-queue/keyProducer.ts (3)
queueKey(37-39)queueItemsKey(41-43)masterQueueKey(29-31)
⏰ 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). (24)
- GitHub Check: Cursor Bugbot
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
packages/redis-worker/src/fair-queue/visibility.ts (4)
189-225: LGTM! Master queue key properly threaded through release flow.The
releasemethod signature and implementation correctly add themasterQueueKeyparameter and propagate it to thereleaseMessageRedis command. The Lua script will now ensure that released messages update the master queue for proper consumer pickup.
235-301: LGTM! Reclaim flow properly updated to include master queue key.The
reclaimTimedOutmethod signature correctly extends thegetQueueKeyscallback to returnmasterQueueKey, and the implementation properly threads it through to thereleaseMessagecall. This ensures reclaimed messages are correctly tracked in the master queue.
441-477: LGTM! Lua script correctly updates master queue during message release.The updated
releaseMessageLua script correctly:
- Accepts
masterQueueKeyas the 5th key andqueueIdin ARGV- Updates the master queue with the message's score, ensuring the queue priority reflects the oldest message timestamp
- Maintains atomicity by performing all operations (remove from in-flight, add to queue, update master queue) within the same script
The use of the message score for the master queue entry ensures proper scheduling priority.
516-526: LGTM! TypeScript declaration correctly updated.The
releaseMessagecommand declaration properly reflects the updated signature withmasterQueueKeyandqueueIdparameters, matching the Lua script implementation.packages/redis-worker/src/fair-queue/tests/raceConditions.test.ts (1)
446-450: LGTM! Test correctly updated for new master queue key requirement.The test properly supplies
masterQueueKeyusingkeys.masterQueueKey(0), which is correct for this single-shard test scenario (shardCount: 1 on line 400). This aligns with the updatedreclaimTimedOutsignature in visibility.ts.packages/redis-worker/src/fair-queue/index.ts (6)
818-825: LGTM! Release call correctly updated with master queue key.The release call after failed concurrency reservation properly includes the
masterQueueKeyparameter, ensuring the master queue is updated when messages are returned to the queue. This maintains consistency in the two-stage processing flow.
1065-1072: LGTM! Direct processing release call properly updated.The release call in the direct processing path correctly includes the
masterQueueKeyparameter, mirroring the two-stage processing changes. This ensures consistent master queue updates across both processing modes.
1189-1197: LGTM! Handler context release properly updated.The release callback in the message handler context correctly passes
masterQueueKeyto the#releaseMessagehelper, ensuring user-initiated releases update the master queue.
1271-1298: LGTM! Private release helper correctly updated.The
#releaseMessagemethod signature and implementation properly add themasterQueueKeyparameter and thread it through to the visibility manager's release call. The use ofDate.now()for the score ensures the message goes to the back of the queue as expected.
1329-1337: LGTM! Retry flow release properly includes master queue key.The release call in the retry flow correctly includes
masterQueueKeyand usesDate.now() + nextDelayas the score, ensuring delayed retries are properly tracked in the master queue with the correct scheduling priority.
1458-1462: LGTM! Reclaim call correctly calculates master queue key per queue.The
reclaimTimedOutcallback properly includesmasterQueueKeycomputed using the queue's shard ID. This ensures reclaimed messages update the correct shard's master queue, which is crucial for proper sharded queue management.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
No description provided.