fix: never resolve a fetch to state older than an observed found result - #1507
Conversation
…umed by an in-flight fetch Reproduces the flash-trade wedge: a subscription update that resolves a pending fetch for an older update of the same account is never forwarded to the fetch cloner, and the resolving fetch discards it when its companion mismatches slots and the RPC-only retry still sees the closed window. The bank placeholder then suppresses every later refetch. The consumed-update test asserts the enforced behavior (the consumed state must be honored) and fails until the min-context floor fix lands.
A pending fetch resolved by a subscription update (e.g. a rapid re-delegation racing the previous incarnation's fetch) could be finalized as NotFound by the slot-matching RPC-only retry when the RPC view lagged the consumed update: the freshest state was silently swallowed, leaving the account permanently stuck behind its bank placeholder (flash-trade wedge, Jul 29-30). try_get_multi_until_slots_match now raises the effective min-context floor to the highest slot of any found result it has observed, so a lagging RPC response cannot finalize an older view; the retry loop waits for the RPC to catch up instead.
|
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 (2)
📝 WalkthroughWalkthroughAdds fetch-cloner tests for missed redelegation updates, placeholder wedges, recovery paths, and in-flight races. Updates remote account retries to raise effective context slots from observed accounts and replay consumed subscription results when RPC resolution fails. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12c31bba59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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-chainlink/src/chainlink/fetch_cloner/tests.rs`:
- Around line 10383-10411: Replace the fixed 150ms, 300ms, and 200ms sleeps in
the re-delegation race test with polling of observable state, using existing
helpers such as fetch_count(), bank contents, or the RPC-fetch counter. Ensure
each transition is explicitly awaited: the stale fetch is in flight, the
notification has been consumed, RPC fetches are unblocked and observed, and the
chain re-delegation is visible before continuing.
- Around line 10400-10404: Update the forward-channel assertions in the test
around the existing `try_recv()` checks, including the assertion near Line
10418, to match specifically on `TryRecvError::Empty` rather than accepting any
error. Ensure `TryRecvError::Disconnected` causes both assertions to fail while
preserving the intended empty-channel behavior.
🪄 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: 75b17cc9-8e97-469c-8a85-8ae65148cd0b
📒 Files selected for processing (3)
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/remote_account_provider/tests.rs
…h fails A found result consumed from the update pipeline to resolve a pending fetch was lost if the fetch subsequently failed, e.g. when the RPC view lags the consumed slot beyond the slot-matching retry budget (~4s, while pubsub commonly leads RPC by several seconds). The account state would then only recover on the next on-chain write. On failure the consumed results now re-enter the subscription update pipeline; the retried resolution rides the lag-tolerant RPC fetch path (RPC_FETCH_MAX_RETRIES) with the consumed slot as its min-context floor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85d55dc886
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… unwatched drop Re-forwarding as Account-sourced could be dropped by the unwatched-account guard if the key's watch was released before the retry processed, for both program- and account-sourced originals. A dedicated SubscriptionSource::Replay variant is processed regardless of watch state and stays excluded from program-only parking.
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-chainlink/src/remote_account_provider/mod.rs`:
- Around line 2428-2438: Ensure consumed subscription resolutions are recorded
during try_get_multi and replayed before every terminal error return, including
initial RPC fetch failures and classification errors propagated by ?. Update the
surrounding remote-account fetch flow, including consumed_subscription_results
and its replay calls, so no subscription-resolved account is lost when any
account fetch or classification fails.
- Around line 2616-2633: Update reforward_consumed_subscription_results so
replay updates are delivered through a bounded outbox/worker or equivalent
lossless, cancellation-aware mechanism instead of awaiting
subscription_forwarder.send directly. Ensure enqueueing does not block terminal
error handling indefinitely under downstream backpressure, while preserving
every consumed update and allowing cancellation to terminate delivery.
🪄 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: 68ae3b76-b47d-4bc8-bd99-7e5430d85ba9
📒 Files selected for processing (4)
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/remote_account_provider/pubsub_common.rsmagicblock-chainlink/src/remote_account_provider/tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6c3bdae81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f04ed45cbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replay sends now run in a detached task so a failing resolution inside a listener-permit task cannot block on the update pipeline's own input capacity. The classification error exit in the slot-matching retry loop re-forwards consumed results like the other failure exits.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5ecfa0dbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replays try_send first; a full pipeline falls back to a detached sender capped by a semaphore (256), beyond which replays are dropped with a warning instead of accumulating unbounded waiters.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf3f57a8d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replays are per-account latest-state snapshots, not an event queue: a Pubkey -> latest-update outbox (newest slot wins) drained by one detached worker is simultaneously lossless, bounded by the number of affected accounts, and never blocks a failing resolution. Mirrors the submux debounce pattern of forwarding only the freshest pending payload.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6580e03baa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
Problem
When a subscription update arrives for an account with a fetch already in flight, the provider consumes the update to resolve that pending fetch instead of forwarding it to the fetch cloner. If the fetch then enters the slot-matching retry (e.g. its companion account resolved at an older slot), the RPC-only refetch could return a view older than the consumed update — a lagging RPC still showing the account as closed — and finalize
NotFound.The freshest state is silently dropped: nothing is forwarded, nothing is cloned, and since the bank now holds a non-delegated entry, every later
ensureskips the remote fetch. An account that is delegated on chain can stay absent from the ER indefinitely, with no error past the initialResolvedAccountCouldNoLongerBeFound.This can be triggered by rapid re-delegation of the same PDA: the re-delegation notification races the fetch resolving the previous incarnation's update.
Solution
try_get_multi_until_slots_matchnow raises its effectivemin_context_slotfloor to the highest slot of any found result it has observed — including results injected by subscription updates. A lagging RPC response can no longer satisfy the slot match below that floor, so the retry loop waits for the RPC view to catch up (within the existing retry budget) and resolves with the fresh state instead of regressing to an older one.Tests
test_repro_redelegation_update_delivered_but_consumed_by_inflight_fetchreproduces the loss deterministically (blocked RPC + update injected through the provider loop) and enforces that the consumed state is honored — red before the fix, green after.test_get_accounts_until_slots_match_refetches_mixed_sources_as_rpc_batchupdated: it previously asserted the regression (accepting an older RPC batch for an account already observed at a newer slot).Summary by CodeRabbit