[v4] Fix for completing batches (when there are idempotent runs) #1985
Conversation
|
WalkthroughThe change updates the batch processing logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant Service as BatchTriggerService
participant DB as Database
Service->>DB: Fetch batch record (select processingJobsCount, runCount)
Service->>DB: Increment processingJobsCount by processed run IDs
Service->>Service: Check if processingJobsCount == runCount
alt All runs processed
Service->>Service: Run batch completion logic
end
Service->>Service: Check if more items to process
alt More items remain
Service->>Service: Requeue batch for further processing
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
apps/webapp/app/runEngine/services/batchTrigger.server.ts (1)
560-589: Consider adding a migration strategy for existing batch records.While the implementation is solid, there's no explicit handling for existing batch records that might not have the
processingJobsCountfield initialized.Consider adding a data migration strategy or handling null/undefined values in the completion check:
if (updatedBatch.processingJobsCount >= updatedBatch.runCount) { //if all the runs were idempotent, it's possible the batch is already completed await this._engine.tryCompleteBatch({ batchId: batch.id }); }You might also want to add a consistency check to ensure that
processingJobsCountnever exceedsrunCount:// Add a consistency check to make sure processingJobsCount doesn't exceed runCount if (updatedBatch.processingJobsCount > updatedBatch.runCount) { logger.warn("[RunEngineBatchTrigger] processingJobsCount exceeds runCount", { batchId: batch.id, processingJobsCount: updatedBatch.processingJobsCount, runCount: updatedBatch.runCount, }); } if (updatedBatch.processingJobsCount >= updatedBatch.runCount) { //if all the runs were idempotent, it's possible the batch is already completed await this._engine.tryCompleteBatch({ batchId: batch.id }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/runEngine/services/batchTrigger.server.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/runEngine/services/batchTrigger.server.ts (4)
567-570: Good addition of atomic counter for processing jobs.The implementation of an atomic increment operation for
processingJobsCountis a good approach for handling concurrency. This ensures accurate tracking when multiple workers process the same batch simultaneously, which directly addresses the issue with idempotent runs mentioned in the PR objectives.
571-574: Appropriate field selection for batch completion check.Selecting both
processingJobsCountandrunCountfields ensures the completion check has the necessary data to determine if all runs have been triggered, without needing to fetch the entire batch record.
578-581: Excellent fix for idempotent run completion.This change is the core fix - comparing
processingJobsCountagainstrunCountinstead of relying on the length ofrunIds. This properly addresses the issue where batches with idempotent runs would fail to complete because idempotent runs might not add new entries to therunIdsarray.
583-587: Correct repositioning of the requeue check.Moving the requeue check to after the batch completion check ensures that the batch completion logic runs before deciding whether to requeue. This is a critical ordering fix that prevents the situation where a batch might be requeued unnecessarily when it's actually complete.
If there were batches in v4 with lots or all idempotent runs, the batch wouldn't get completed.
This impacted the dashboard Batches page, but more important when using
batchTriggerAndWaitit could mean the parent run never continued.This changes the strategy so the tracking of how many items have been processed is atomic using an increment. The logic has also been moved up to the correct place.
Summary by CodeRabbit