fix(coordinator): fix the error in the incorrect call during database… #1576
Conversation
WalkthroughThe pull request introduces a modification in the Changes
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
235-237: Consider adding tests for attempt recoverySince this fixes a bug in attempt management, it would be valuable to add tests verifying:
- Correct decrease of active attempts for batch tasks
- Error handling behavior
Would you like me to help create test cases for this functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
coordinator/internal/logic/provertask/batch_prover_task.go(1 hunks)
🔇 Additional comments (2)
coordinator/internal/logic/provertask/batch_prover_task.go (2)
235-237: LGTM! Correct ORM usage for batch operations
The change from chunkOrm to batchOrm is correct as we're operating on batch-level attempts. This fixes a logical error where chunk-level operations were being used for batch-level state management.
235-237: Verify consistent attempt management across codebase
Let's verify there are no other inconsistent usages of attempt management between chunk and batch operations.
✅ Verification successful
Based on the search results, I can see a consistent pattern in how attempt management is handled across the codebase:
- All three types (chunk, batch, bundle) implement
DecreaseActiveAttemptsByHashwith similar behavior - The attempt management is used consistently in error paths:
- In proof submission (
submitproof/proof_receiver.go) - In proof collection (
controller/cron/collect_proof.go) - In prover task handling (
provertask/*.go)
- In proof submission (
- The error handling and logging patterns are consistent across all implementations
The code under review follows the same pattern as other similar operations in the codebase. The attempt management is properly synchronized with the task lifecycle and error handling paths.
Attempt management implementation is consistent
The implementation follows the established patterns in the codebase and properly handles attempt management for batch operations, consistent with how it's done for chunks and bundles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other potential mismatches in attempt management
# Look for attempt-related operations on chunks and batches
rg -A 3 "DecreaseActiveAttempts|UpdateAttempts|GetAssigned.*Attempts" --type go
# Look for potential error recovery paths that might need similar fixes
ast-grep --pattern 'if err := $$$; err != nil {
$$$
return $$$, err
}'
Length of output: 34577
scroll-tech#1576) Co-authored-by: xiaoranlu <[email protected]>
2 participants
Purpose or design rationale of this PR
BatchProverTask.recoverActiveAttempts()should call batchOrmPR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Refactor
BatchProverTask.