hotfix: load failed intents on restart & reschedule#1371
Conversation
📝 WalkthroughWalkthroughThe change extends commit recovery to include failed (FailedFinalize, FailedProcess) commit status rows alongside pending ones. A new Suggested reviewers
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-committor-service/src/committor_processor.rs`:
- Around line 183-185: The recovery path in committor_processor’s row loading is
incorrectly unioning separately filtered Pending and Failed* results, which can
reconstruct only part of a bundle for the same message_id. Update the logic
around self.persister.get_pending_commit_statuses and
self.persister.get_failed_commit_statuses so recovery eligibility is computed
once across the full recoverable-status set, then load all rows for eligible
message_ids together before passing them to rows_to_scheduled_intent_bundles.
In `@magicblock-committor-service/src/persist/commit_persister.rs`:
- Around line 264-272: The new recovery helper get_failed_commit_statuses
currently panics on a poisoned DB mutex via .expect(POISONED_MUTEX_MSG), which
violates the non-panicking persistence path requirement. Update
get_failed_commit_statuses in commit_persister.rs to handle the lock acquisition
failure explicitly by mapping it into a CommitPersistResult error instead of
aborting the committor. Reuse the existing CommitPersistResult and commits_db
access pattern used by the other persistence methods so the failure is reported
consistently, and only keep a panic if you add an explicit invariant comment
justifying it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b5fee1a-dfd3-4ee9-9faa-e1be91cd721b
📒 Files selected for processing (4)
magicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/persist/db.rsmagicblock-committor-service/src/service.rs
| let mut rows = | ||
| self.persister.get_pending_commit_statuses(min_created_at)?; | ||
| rows.extend(self.persister.get_failed_commit_statuses(min_created_at)?); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't union independently filtered recovery rows.
Line 184 and Line 185 can recover a partial bundle. If one message_id has both Pending and Failed* rows with different created_at values, one query can include it while the other excludes it, and rows_to_scheduled_intent_bundles then rebuilds only the surviving subset of accounts. Compute recovery eligibility once across the full recoverable-status set and load all eligible rows together.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magicblock-committor-service/src/committor_processor.rs` around lines 183 -
185, The recovery path in committor_processor’s row loading is incorrectly
unioning separately filtered Pending and Failed* results, which can reconstruct
only part of a bundle for the same message_id. Update the logic around
self.persister.get_pending_commit_statuses and
self.persister.get_failed_commit_statuses so recovery eligibility is computed
once across the full recoverable-status set, then load all rows for eligible
message_ids together before passing them to rows_to_scheduled_intent_bundles.
| fn get_failed_commit_statuses( | ||
| &self, | ||
| min_created_at: u64, | ||
| ) -> CommitPersistResult<Vec<CommitStatusRow>> { | ||
| self.commits_db | ||
| .lock() | ||
| .expect(POISONED_MUTEX_MSG) | ||
| .get_failed_commit_statuses(min_created_at) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid panicking on the DB mutex in this new method.
Line 270 uses .expect(POISONED_MUTEX_MSG), so a poisoned mutex aborts the committor instead of returning a persistence error from this recovery path. Please map the lock failure into CommitPersistResult or justify the panic invariant explicitly. As per path instructions, {magicblock-*,programs,storage-proto}/**: Treat any usage of .unwrap() or .expect() in production Rust code as a MAJOR issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magicblock-committor-service/src/persist/commit_persister.rs` around lines
264 - 272, The new recovery helper get_failed_commit_statuses currently panics
on a poisoned DB mutex via .expect(POISONED_MUTEX_MSG), which violates the
non-panicking persistence path requirement. Update get_failed_commit_statuses in
commit_persister.rs to handle the lock acquisition failure explicitly by mapping
it into a CommitPersistResult error instead of aborting the committor. Reuse the
existing CommitPersistResult and commits_db access pattern used by the other
persistence methods so the failure is reported consistently, and only keep a
panic if you add an explicit invariant comment justifying it.
Source: Path instructions
2 participants
Summary
Loads failed intents on restart and reschedules them. Proper recovery is coming with introducion of outbox accounts following MIMD-0025
Breaking Changes
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes