fix: retry failed intents - #1501
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRecovery persistence now stores and migrates 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: 6
🤖 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/persist/commit_persister.rs`:
- Around line 814-838: Extend the recoverable_intents test coverage around
recoverable_intents and
test_recoverable_intents_pairs_bundle_with_its_commit_ids by inserting
FailedFinalize or FailedProcess rows, including a mixed pending-and-failed
bundle. Assert the failed row is included in the recovered bundle and that its
commit ID is present in RecoveredIntent.commit_ids alongside the pending rows.
- Line 177: Replace the expect-based lock acquisition in the recovery path with
recoverable error handling: map a poisoned `commits_db` mutex lock into the
existing `CommitPersistError` and propagate it from the enclosing method. Follow
the surrounding pre-existing mutex error-mapping pattern and preserve normal
lock usage when acquisition succeeds.
- Around line 173-184: Extract the shared “lock commits_db, load pending
statuses, extend with failed statuses, then release the lock” sequence from
recoverable_intents and pending_intent_bundles into a single helper for loading
recoverable statuses. Update both methods to use that helper while retaining
their existing distinct conversion functions, and remove
pending_rows_to_scheduled_intent_bundles if it is no longer used outside tests.
In `@magicblock-committor-service/src/persist/db.rs`:
- Around line 618-645: Update get_failed_commit_statuses to derive both
failed-status query parameters from CommitStatus::FailedFinalize(...).as_str()
and CommitStatus::FailedProcess(None).as_str() instead of hard-coded string
literals, preserving the existing query and filtering behavior.
In `@magicblock-committor-service/src/service.rs`:
- Around line 388-497: Add unit tests for the replay-safety gates
is_same_delegation_session and is_valid_nonce using stubbed chainlink and
task-info-fetcher dependencies. Cover same-session acceptance,
redelegated-session rejection, equal nonce rejection, greater nonce acceptance,
and missing current-nonce rejection, including the commit_id == 0 boundary where
applicable.
- Around line 492-496: Update the validity predicate in the recovered-intent
flow around recovered.commit_ids so commit_id == 0 is treated as valid for
pending-intent recovery instead of compared against current_nonces. Preserve the
existing commit_id > current_nonce validation for assigned nonzero commit IDs,
and keep the pubkey lookup behavior unchanged.
🪄 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 Plus
Run ID: f92bfc66-fc09-4e98-9f1b-d2e2bd9af4bf
📒 Files selected for processing (5)
magicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/persist/db.rsmagicblock-committor-service/src/persist/mod.rsmagicblock-committor-service/src/service.rs
| /// Filters recovered intents down to the ones still safe to replay, | ||
| /// checking up to `JOIN_CHUNK_SIZE` at a time in parallel. | ||
| async fn retain_recoverable_intents( | ||
| &self, | ||
| bundles: &mut Vec<ScheduledIntentBundle>, | ||
| ) { | ||
| let results = join_all( | ||
| bundles.iter().map(|b| b.get_all_committed_pubkeys()).map( | ||
| |pubkeys| async move { | ||
| #[allow(deprecated)] | ||
| let result = self | ||
| .chainlink | ||
| .accounts_delegated_on_base_and_er( | ||
| &pubkeys, | ||
| AccountFetchContext::internal( | ||
| AccountFetchReason::RequestedAccount, | ||
| ), | ||
| ) | ||
| .await; | ||
| result | ||
| }, | ||
| ), | ||
| ) | ||
| .await; | ||
|
|
||
| let mut results_iter = results.into_iter(); | ||
| bundles.retain(|bundle| { | ||
| let Some(result) = results_iter.next() else { | ||
| error!("Results and bundles must have equal length"); | ||
| return false; | ||
| }; | ||
| match result { | ||
| Ok(delegated) if delegated.iter().all(|d| *d) => true, | ||
| Ok(_) => { | ||
| warn!( | ||
| intent_id = bundle.id, | ||
| "Skipping recovered commit intent because not all accounts are delegated on base and ER" | ||
| ); | ||
| false | ||
| } | ||
| Err(err) => { | ||
| error!( | ||
| intent_id = bundle.id, | ||
| error = ?err, | ||
| "Failed to verify recovered commit intent accounts" | ||
| ); | ||
| false | ||
| } | ||
| } | ||
| recovered: Vec<RecoveredIntent>, | ||
| ) -> Vec<ScheduledIntentBundle> { | ||
| const JOIN_CHUNK_SIZE: usize = 10; | ||
|
|
||
| let mut to_keep = Vec::with_capacity(recovered.len()); | ||
| for chunk in recovered.chunks(JOIN_CHUNK_SIZE) { | ||
| let iter = chunk.iter().map(|recovered| async move { | ||
| self.is_intent_retriable(recovered).await | ||
| }); | ||
| to_keep.extend(join_all(iter).await); | ||
| } | ||
|
|
||
| recovered | ||
| .into_iter() | ||
| .enumerate() | ||
| .filter(|(i, _)| to_keep[*i]) | ||
| .map(|(_, el)| el.bundle) | ||
| .collect() | ||
| } | ||
|
|
||
| /// An intent is retriable only if its delegation session is unchanged | ||
| /// and its commit nonce hasn't already landed on chain. | ||
| async fn is_intent_retriable(&self, recovered: &RecoveredIntent) -> bool { | ||
| let pubkeys = recovered.bundle.get_all_committed_pubkeys(); | ||
| let result = self | ||
| .is_same_delegation_session(&pubkeys, recovered) | ||
| .await | ||
| .inspect_err(|err| { | ||
| error!(intent_id = recovered.bundle.id, error = ?err, "Failed to check delegation session for recovery"); | ||
| }) | ||
| .unwrap_or(false); | ||
|
|
||
| if !result { | ||
| false | ||
| } else { | ||
| self.is_valid_nonce(&pubkeys, recovered) | ||
| .await | ||
| .inspect_err(|err| { | ||
| error!(intent_id = recovered.bundle.id, error = ?err, "Failed to check commit nonce for recovery"); | ||
| }) | ||
| .unwrap_or(false) | ||
| } | ||
| } | ||
|
|
||
| /// True if every committed account is still delegated (or undelegating) | ||
| /// at the same `remote_slot` recorded when the intent was scheduled. | ||
| async fn is_same_delegation_session( | ||
| &self, | ||
| pubkeys: &[Pubkey], | ||
| recovered: &RecoveredIntent, | ||
| ) -> ChainlinkResult<bool> { | ||
| let recovered_accounts = recovered.bundle.get_all_committed_accounts(); | ||
| let current_accounts = self | ||
| .chainlink | ||
| .fetch_accounts( | ||
| pubkeys, | ||
| AccountFetchContext::internal( | ||
| AccountFetchReason::RequestedAccount, | ||
| ), | ||
| ) | ||
| .await? | ||
| .into_iter() | ||
| .collect::<Option<Vec<_>>>(); | ||
| let Some(current_accounts) = current_accounts else { | ||
| return Ok(false); | ||
| }; | ||
| if current_accounts.len() != pubkeys.len() { | ||
| return Ok(false); | ||
| } | ||
|
|
||
| let same_session = recovered_accounts | ||
| .into_iter() | ||
| .zip(current_accounts) | ||
| .all(|(recovered, actual)| { | ||
| // 1. If account delegated using remote_slot we ensure | ||
| // that account is within same delegation "session" and wasn't redelegated | ||
| // 2. We handle `undelegating` state as well because undelegation could fail | ||
| // If that is the case we want to retry undelegate intent | ||
| // We need to make sure that is within same session as well | ||
| // NOTE: here we rely `validate_not_delegated` & `should_refresh_undelegating_in_bank_account` | ||
| // If account delegated - remote_slot doesn't change | ||
| // If account undelegating - until it is remote_slot doesn't change | ||
| (actual.delegated() || actual.undelegating()) | ||
| && recovered.remote_slot == actual.remote_slot() | ||
| }); | ||
| Ok(same_session) | ||
| } | ||
|
|
||
| /// False if any committed account's persisted `commit_id` has already | ||
| /// been consumed on chain, or couldn't be verified. | ||
| async fn is_valid_nonce( | ||
| &self, | ||
| pubkeys: &[Pubkey], | ||
| recovered: &RecoveredIntent, | ||
| ) -> TaskInfoFetcherResult<bool> { | ||
| let current_nonces = self | ||
| .processor | ||
| .fetch_current_commit_nonces(pubkeys, 0) | ||
| .await?; | ||
|
|
||
| let valid = recovered.commit_ids.iter().all(|(pubkey, commit_id)| { | ||
| current_nonces | ||
| .get(pubkey) | ||
| .is_some_and(|current_nonce| commit_id > current_nonce) | ||
| }); | ||
| Ok(valid) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No tests cover the new replay-safety filters.
is_same_delegation_session and is_valid_nonce are the gates that prevent re-committing an intent into a new delegation session or double-committing an already-landed nonce, yet neither has coverage. The nonce boundary in particular (commit_id == current_nonce, commit_id == 0, pubkey absent from current_nonces) is exactly where an off-by-one silently becomes a duplicate commit.
Worth adding unit tests with a stubbed chainlink/task-info-fetcher for: same-session accept, redelegated-session reject, nonce-equal reject, nonce-greater accept, and missing-nonce reject.
🤖 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/service.rs` around lines 388 - 497, Add unit
tests for the replay-safety gates is_same_delegation_session and is_valid_nonce
using stubbed chainlink and task-info-fetcher dependencies. Cover same-session
acceptance, redelegated-session rejection, equal nonce rejection, greater nonce
acceptance, and missing current-nonce rejection, including the commit_id == 0
boundary where applicable.
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
Aside from the remaining coderabbit issues I found a few issues as well.
The recovery path has two blocking safety and durability issues: it can approve stale local delegated state after an offline re-delegation, and the schema migration makes all legacy pending intents unrecoverable.
Failed-finalize rows also cannot pass the new nonce gate, and nonce validation uses an unfenced RPC read that is then cached. These issues conflict with the base-layer synchronization, restart durability, and commit lifecycle invariants.
| let recovered_accounts = recovered.bundle.get_all_committed_accounts(); | ||
| let current_accounts = self | ||
| .chainlink | ||
| .fetch_accounts( |
There was a problem hiding this comment.
This does not necessarily fetch current base-layer state.
For a delegated, non-undelegating account already in the bank, chainlink treats the local copy as a hit and skips the remote fetch.
Startup also preserves delegated bank accounts.
After an offline undelegate/redelegate cycle, this can therefore compare the persisted slot with the same stale local slot and approve replay into a new delegation session.
If the nonce reset, the old account state can then be committed into that new session.
Please verify the session against current base-layer delegation metadata rather than the protected local account copy.
There was a problem hiding this comment.
For a delegated, non-undelegating account already in the bank, chainlink treats the local copy as a hit and skips the remote fetch.
That what we want. If it is in the bank and delegated it is owned by ER and remote_slot allows us to check delegation "session". If intent.remote_slot == current.remote_slot it is same session. If not(higher likely) it means account was undelegated and then delegated.
After an offline undelegate/redelegate cycle, this can therefore compare the persisted slot with the same stale local slot and approve replay into a new delegation session.
- If account is delegated it's remote_slot is not stale.
- It can't approve replay into new session as we compare remote_slots and distinguish sessions using it
For !delegated or undelegatingchainlink will fetch new data if as we want.
| if has_column { | ||
| return Ok(()); | ||
| } | ||
| self.conn.execute_batch( |
There was a problem hiding this comment.
Every pre-upgrade pending row is backfilled with remote_slot = 0, but recovery later requires exact equality with the current account's normally nonzero slot.
That silently drops all pending settlement work present during an upgrade, while the previous implementation would recover it after checking delegation.
If this is not acceptable then provide a safe legacy reconciliation path, or record an explicit durable failure outcome instead of making these rows unrecoverable.
There was a problem hiding this comment.
We can't replay intents we're not sure about.
Atm we have:
- No failed intents on nodes
- Failed intents can be resolved via script and then ignoreing here is safe
All new failed intent until Outbox is merged will have correctly identified remote_slot
| .fetch_current_commit_nonces(pubkeys, 0) | ||
| .await?; | ||
|
|
||
| let valid = recovered.commit_ids.iter().all(|(pubkey, commit_id)| { |
There was a problem hiding this comment.
A FailedFinalize row is written only after the commit stage returned a commit signature, so its persisted commit ID has normally already landed and equals the current on-chain nonce.
Such a row can never pass this predicate, even though the new query explicitly loads FailedFinalize for retry.
Retrying the full commit would also be wrong after that stage landed.
We need to distinguish this state and resume only the remaining finalize work, or exclude it from the claimed recovery set with an explicit durable outcome.
There was a problem hiding this comment.
Note, intent execution durability is question of a different PR and saga here. Here we try to replay intents that we can and that are safe to do. Everything else will be covered in big PR mentioned above.
With this said, relaxed nonce comparison so it would cover both SingleStage and TwoStage. That could lead to intent executed twice but it is fine as states are the same
| pubkeys: &[Pubkey], | ||
| recovered: &RecoveredIntent, | ||
| ) -> TaskInfoFetcherResult<bool> { | ||
| let current_nonces = self |
There was a problem hiding this comment.
Using min_context_slot = 0 allows a lagging RPC response to pass this replay gate.
The returned nonce is cached, so the later bundle refresh does not make task execution refetch it at the newer account slot.
That can seed the retry with a stale nonce and produce another avoidable failure. Please fence this read at the verified base-layer slot and preserve that freshness through execution.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/service.rs (1)
431-444: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not permanently discard intents when replay validation is indeterminate.
Line 434 converts transient delegation/nonce lookup failures into
false; the bundle is then dropped, and recovery runs only once before the accept loop. Preserve invalid intents as rejected, but propagate or retry indeterminate checks with backoff so RPC failures do not disable recovery until restart.🤖 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/service.rs` around lines 431 - 444, Update the recovery validation flow around the delegation check and self.is_valid_nonce so transient lookup errors are not converted to false and permanently discarded. Distinguish definitively invalid intents from indeterminate RPC failures, and propagate or retry the latter with backoff so recovery can continue after temporary failures; keep valid intents accepted and invalid intents rejected.
🤖 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.
Outside diff comments:
In `@magicblock-committor-service/src/service.rs`:
- Around line 431-444: Update the recovery validation flow around the delegation
check and self.is_valid_nonce so transient lookup errors are not converted to
false and permanently discarded. Distinguish definitively invalid intents from
indeterminate RPC failures, and propagate or retry the latter with backoff so
recovery can continue after temporary failures; keep valid intents accepted and
invalid intents rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c82b94b2-fdb8-4e04-8b11-875348cf6840
📒 Files selected for processing (2)
magicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/service.rs
|
@thlorenz
Delegated accounts can't be re-delegated offline as they have to be undelegated first. More detailed answer here.
Legacy intents will be addressed via script and post migration will be recovered via current flow.
Fixed
Majority of intents are executed in 1 tx for which this isn't true. For example on devnet-asia: 143 single stage intents an 0 are executed in 2 txs. Tho I will think more on how that can be addressed |
2 participants
Summary
Reintroduces retrying failed intents on restart (previously reverted after a bug where a replayed intent could apply stale state if the account had been undelegated/redelegated since the intent was originally scheduled).
Before replaying a failed intent, we now verify:
Only intents that pass both checks are retried. Pending intents (still in-flight, most recent) are recovered as before, without these checks.
To support the session check, we now persist the base chain slot observed when an intent was scheduled - previously dropped on reload, defaulting to 0. Includes a DB migration for existing rows.
Note: intent execution durability is a question of a different PR and saga — tracked separately. Here we only try to replay intents that we can and that are safe to replay; everything else is covered by that larger effort.
With this in mind, the nonce comparison was relaxed so it covers both single-stage and two-stage execution. This could lead to an intent being executed twice, but that's fine since the resulting state is the same either way.
Breaking Changes
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes
remote_slotfor older commit-status rows.remote_slotcorrectly to prevent scheduling from stale sessions or reused nonces.