fix: drop stale subscription updates#1264
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a crate-visible RemoteAccountProvider accessor on FetchCloner and drops stale forwarded account updates when the provider no longer watches the pubkey; if the account still exists, the cloner enqueues a best-effort removal update. Introduces RemoteAccountProvider::evict_unwatched_with_subscription_lock, wires it into Chainlink's subscribe_account_removals to avoid eviction/subscription races, propagates SubscriptionSource through pubsub plumbing, and updates/extends tests with provider helpers and new cases exercising eviction coordination and unwatched-update behavior. 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: 1
🤖 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 1666-1673: The test currently breaks out immediately because
clone_request_count() can be 0 at baseline; change the test to first observe
that a clone request was created (wait for
ctx.fetch_cloner.cloner().clone_request_count() to become > 0 or use a
deterministic post-send signal from the code path) then wait for it to drop back
to 0; in other words, gate the zero-check behind a confirmed prior increment (or
await a dedicated processing-complete signal/oneshot you add to the async drop
path) so the test actually exercises the async drop processing in tests.rs where
ctx.fetch_cloner.cloner() and clone_request_count() are used.
🪄 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: 6fd6c77b-e2fa-466c-a66d-497b39471bb0
📒 Files selected for processing (4)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/mod.rs
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-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
597-616:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't enqueue removal for intentionally unwatched delegated accounts.
A delegated account can legitimately still be in the bank after its
DirectAccountsubscription was released. In that case, a late forwarded update will now hit this!is_watchingbranch and enqueue removal anyway, which lets the removal listener evict authoritative local delegated state. Please exempt the same local state that later triggerscleanup_direct_subscription_for_delegated_account(...)here as well, and add a regression test for the late-update case.Suggested guard
let update_slot = update.account.slot(); if !self.remote_account_provider.is_watching(&pubkey) { + if self.accounts_bank.get_account(&pubkey).is_some_and(|in_bank| { + in_bank.delegated() && !in_bank.undelegating() + }) { + trace!( + pubkey = %pubkey, + update_slot, + "Dropping late update for intentionally unwatched delegated account" + ); + return; + } trace!( pubkey = %pubkey, update_slot, "Dropping subscription update for account that is no longer watched" );🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 597 - 616, When handling the !self.remote_account_provider.is_watching(&pubkey) branch, avoid enqueueing a removal for accounts that were intentionally unwatched because they are delegated; add a guard that detects the same condition used by cleanup_direct_subscription_for_delegated_account(...) (e.g., consult the delegated subscription marker/state or invoke the same predicate/function) and skip calling self.remote_account_provider.send_removal_update(pubkey). Update the logic around accounts_bank.get_account(&pubkey) to short-circuit when that delegated-unwatched predicate is true, and add a regression test that simulates a late forwarded update after a delegated DirectAccount subscription was released to ensure no removal is enqueued.
🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 597-616: When handling the
!self.remote_account_provider.is_watching(&pubkey) branch, avoid enqueueing a
removal for accounts that were intentionally unwatched because they are
delegated; add a guard that detects the same condition used by
cleanup_direct_subscription_for_delegated_account(...) (e.g., consult the
delegated subscription marker/state or invoke the same predicate/function) and
skip calling self.remote_account_provider.send_removal_update(pubkey). Update
the logic around accounts_bank.get_account(&pubkey) to short-circuit when that
delegated-unwatched predicate is true, and add a regression test that simulates
a late forwarded update after a delegated DirectAccount subscription was
released to ensure no removal is enqueued.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9595bd59-99c2-4f53-a31b-c44b105c1927
📒 Files selected for processing (4)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/mod.rs
Amp-Thread-ID: https://ampcode.com/threads/T-019e9111-adb6-770c-9c05-e368a3bb9102 Co-authored-by: Amp <amp@ampcode.com>
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-chainlink/src/chainlink/fetch_cloner/tests.rs (1)
4524-4531:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAcquire
DirectAccountbefore these non-delegated forwarded updatesBoth tests now send
SubscriptionSource::Accountupdates for plain accounts that are not being watched. Under the new stale-update filter, those updates are dropped, so the assertions can still pass without ever exercising the intended subscription-owner path. Line 4524 and Line 4754 should establish a direct subscription first, like the other updated subscription tests do.Suggested fix
let (subscription_tx, subscription_rx) = mpsc::channel(100); let fetch_cloner = FetchCloner::new( &remote_account_provider, &accounts_bank, &cloner_stub, validator_keypair.insecure_clone(), subscription_rx, None, None, ); + acquire_direct_subscription_for_update( + &remote_account_provider, + &account_pubkey, + ) + .await; + // Send subscription update (this will become the owner). let subscription_account = crate::remote_account_provider::RemoteAccount::from_fresh_account( account.clone(), CURRENT_SLOT, @@ let (subscription_tx, subscription_rx) = mpsc::channel(100); let fetch_cloner = FetchCloner::new( &remote_account_provider, &accounts_bank, &cloner_stub, validator_keypair.insecure_clone(), subscription_rx, None, None, ); + acquire_direct_subscription_for_update( + &remote_account_provider, + &account_pubkey, + ) + .await; + // Send subscription update (becomes owner, will fail). let subscription_account = crate::remote_account_provider::RemoteAccount::from_fresh_account( account.clone(), CURRENT_SLOT,Also applies to: 4754-4761
🤖 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-chainlink/src/chainlink/fetch_cloner/tests.rs` around lines 4524 - 4531, The test sends non-delegated ForwardedSubscriptionUpdate messages (subscription_tx + ForwardedSubscriptionUpdate with SubscriptionSource::Account) before ever establishing a direct subscription, so those updates get dropped by the stale-update filter; fix by first acquiring a DirectAccount subscription for the target pubkey the same way other updated-subscription tests do—i.e., before the lines that send the non-delegated updates, send the helper/direct subscription message (the ForwardedSubscriptionUpdate that creates a DirectAccount subscription or invoke the test helper used elsewhere to subscribe the account) so the subsequent SubscriptionSource::Account forwarded updates exercise the subscription-owner path.
🤖 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-chainlink/src/chainlink/fetch_cloner/tests.rs`:
- Around line 4524-4531: The test sends non-delegated
ForwardedSubscriptionUpdate messages (subscription_tx +
ForwardedSubscriptionUpdate with SubscriptionSource::Account) before ever
establishing a direct subscription, so those updates get dropped by the
stale-update filter; fix by first acquiring a DirectAccount subscription for the
target pubkey the same way other updated-subscription tests do—i.e., before the
lines that send the non-delegated updates, send the helper/direct subscription
message (the ForwardedSubscriptionUpdate that creates a DirectAccount
subscription or invoke the test helper used elsewhere to subscribe the account)
so the subsequent SubscriptionSource::Account forwarded updates exercise the
subscription-owner path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 443cca9e-7dd1-4e9a-9e2e-b7d11dbb667f
📒 Files selected for processing (7)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/remote_account_provider/pubsub_common.rs
* master: fix: reject short account responses (#1290) fix: wait for pubsub listeners before reconnect (#1253) fix: retry failed program subscriptions (#1268) release: 0.12.0 (#1299) Ignore compute unit price in processor fees (#1298) Recover recent pending intents on restart (#1296) chore: adjust log level (#1297) release: v0.11.4 (#1292) fix(scheduler): remove block subscription in the scheduler (#1293) fix(committor): race-condition on cleanup (#1291) Handle ALT extend invalid instruction data (#1287) fix: use provided compute limits instead of defaults (#1289) feat: snapshot accountsdb even in the replica mode (#1282) feat: added vrf ephemeral test queue, delegation record and metadata for mb-test-validator (#1281) fix: use wire size (1232), not encoded size (1644), for tx fit checks! (#1285) chore: simplify, rename out_of_order_slot and add a comment (#1284) fix: execute post-delegation actions after clone (#1278) Handle oversized single-stage committor transactions (#1277) Reduce committor RPC confirmation calls (#1271) fix: preserve streams on optimize failure (#1273)
1 participant
Summary
Prevent stale forwarded subscription updates from recreating local bank state for accounts that are no longer watched by the remote account provider.
Details
magicblock-chainlink
Summary by CodeRabbit
New Features
Bug Fixes
Tests