feat: subscribe to accounts in parallel #805
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThe PR makes subscription and update-handling more concurrent. ATA projection collects ATA/eATA pubkeys during iteration and issues all subscription requests in parallel with 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 |
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @magicblock-chainlink/src/remote_account_provider/mod.rs:
- Around line 810-817: The use of try_join_all over
subscribe_and_fetch.iter().map(|(pubkey, _)| self.subscribe(pubkey)) must be
replaced with non-fail-fast logic: call join_all (or otherwise spawn all
subscribe futures) so every subscribe attempt is executed, collect
per-subscription results, log individual errors instead of returning early from
setup_subscriptions/self.subscribe, and ensure failed subscriptions clean up any
entries in fetching_accounts and the pubsub_client/LRU cache or schedule a
cleanup task so senders are not leaked; update the code paths around
try_get_multi and fetch to only proceed after all subscribe attempts are
resolved (success or logged failure) so receivers aren’t left waiting and ensure
any in-flight subscribe futures are observed for completion rather than relying
on being dropped.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/remote_account_provider/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
🧠 Learnings (9)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
📚 Learning: 2025-12-17T12:46:36.207Z
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
🧬 Code graph analysis (1)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
magicblock-core/src/token_programs.rs (1)
try_derive_eata_address_and_bump(81-89)
🔇 Additional comments (3)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (3)
46-72: LGTM: Clean separation of subscription batching from fetch logic.The refactoring to collect pubkeys first and defer subscriptions is well-structured:
- Maintains existing fetch task spawning via
JoinSet- Enables parallel subscription execution (lines 75-80)
- Preserves the logic for deriving eATA addresses and determining effective slots
75-80: LGTM:join_allprovides better error handling thantry_join_all.Using
join_allhere is the right choice because:
- It waits for all subscription futures to complete, rather than failing fast on the first error
- Allows individual error logging without aborting the entire operation
- Consistent with the learning that eATA subscriptions should be kept active to watch for future creation/delegation (per maintainer GabrielePicco)
- Prevents the partial subscription registration issues that
try_join_allcan cause (as seen in the other file under review)Based on learnings, eATA subscriptions are intentionally kept active even when accounts don't exist.
83-89: LGTM: Appropriate error logging without blocking the cloning pipeline.The error handling approach correctly:
- Logs individual subscription failures for debugging and monitoring
- Continues processing even if some subscriptions fail
- Allows the fetch and delegation projection logic to proceed regardless of subscription errors
This resilient approach ensures that temporary subscription failures don't block the account cloning pipeline.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
📚 Learning: 2025-12-17T12:46:36.207Z
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
🧬 Code graph analysis (1)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
account_still_undelegating_on_chain(22-89)
🔇 Additional comments (1)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
227-336: LGTM!The
process_subscription_updatehelper cleanly encapsulates the per-update processing logic. The implementation correctly:
- Handles out-of-order updates by comparing remote slots
- Checks undelegation state using
account_still_undelegating_on_chain- Unsubscribes from delegated accounts to avoid redundant updates
- Logs errors without propagating them (appropriate for background tasks)
The decoupling of update processing into spawned tasks improves parallelism while maintaining correctness.
GabrielePicco
force-pushed
the
chore/refactor-cloning-pipeline-v3
branch
from
253e5f7 to
38259f9
Compare
GabrielePicco
force-pushed
the
chore/refactor-cloning-pipeline-v2
branch
from
de09c36 to
381c479
Compare
GabrielePicco
force-pushed
the
chore/refactor-cloning-pipeline-v3
branch
from
38259f9 to
f043ad0
Compare
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
🧠 Learnings (9)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-12-17T12:46:36.207Z
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
🧬 Code graph analysis (1)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
magicblock-core/src/token_programs.rs (1)
try_derive_eata_address_and_bump(81-89)
🔇 Additional comments (3)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
74-89: LGTM! Correct parallel subscription pattern.The implementation properly:
- Attempts all subscriptions concurrently using
join_all(line 75)- Logs individual failures without stopping the operation (lines 82-89)
- Continues execution even if some subscriptions fail
This pattern maintains operational resilience by treating subscription errors as non-fatal optimization failures while ensuring the core fetch/clone operations proceed.
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (2)
157-222: Well-structured concurrency refactor.The refactoring to use
JoinSetfor processing subscription updates concurrently (with non-blocking draining) is a solid improvement that:
- Enables parallel processing of updates without blocking on delegation record fetches
- Properly handles channel closure scenarios
- Extracts update logic into a dedicated function for better maintainability
788-947: LGTM! Proper deduplication implementation.The
fetch_and_clone_accounts_with_dedupmethod correctly implements request deduplication:
- Synchronous bank checks avoid unnecessary fetches
- Proper handling of pending requests using oneshot channels
- Race-condition-safe entry management in
pending_requests- Cleanup of pending entries after fetch completion
Comment on lines
+190
to
+194
| let Some(update) = maybe_update else { | ||
| // Channel closed, drain remaining tasks and exit | ||
| while pending_tasks.join_next().await.is_some() {} | ||
| break; | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent panic handling in channel-close drain loops.
The drain loops at lines 192 and 216 silently discard task results (including panics), while the drain loops at lines 178-184 and 206-208 properly log panicked tasks. This inconsistency could hide critical issues when the subscription channel closes.
🔎 Proposed fix to log panics during drain
let Some(update) = maybe_update else {
// Channel closed, drain remaining tasks and exit
- while pending_tasks.join_next().await.is_some() {}
+ while let Some(result) = pending_tasks.join_next().await {
+ if let Err(e) = result {
+ error!("Subscription update task panicked during drain: {e:?}");
+ }
+ }
break;
}; Err(
tokio::sync::mpsc::error::TryRecvError::Disconnected,
) => {
// Channel closed, drain remaining tasks and exit
- while pending_tasks.join_next().await.is_some() {}
+ while let Some(result) = pending_tasks.join_next().await {
+ if let Err(e) = result {
+ error!("Subscription update task panicked during drain: {e:?}");
+ }
+ }
break;
}Also applies to: 215-218
Comment on lines
+832
to
843
| // Fail if ANY subscription failed | ||
| if !errors.is_empty() { | ||
| return Err( | ||
| RemoteAccountProviderError::AccountSubscriptionsTaskFailed( | ||
| format!( | ||
| "{} subscription(s) failed: [{}]", | ||
| errors.len(), | ||
| errors.join(", ") | ||
| ), | ||
| ), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Returning error on subscription failure causes memory leak in fetching_accounts.
While switching to join_all (line 814) correctly ensures all subscriptions are attempted, the code still returns an error if ANY subscription fails (lines 832-843). This creates a critical resource leak:
- Oneshot channels created (lines 716-726): For each pubkey, a sender/receiver pair is created and the sender is stored in
fetching_accounts. - Early return on error (line 730
await?): Ifsetup_subscriptionsreturns an error, the function exits immediately. - Fetch never called (lines 734-740): The fetch task that would send results through the channels is never spawned.
- Memory leak: The senders remain in
fetching_accountsindefinitely. The receivers are dropped, but nothing cleans up the senders.
The correct pattern (as shown in ata_projection.rs lines 82-89) is to log subscription errors but continue to fetch rather than returning an error. Subscriptions are an optimization for receiving updates; their failure should not prevent the initial fetch.
Based on learnings, the project maintains subscription resilience where partial failures are tolerated and reconnection logic retries subscriptions. Failing the entire fetch operation over subscription errors contradicts this pattern.
🔧 Recommended fix: Log errors but allow fetch to proceed
// Collect errors and log each individual failure
- let mut errors = Vec::new();
for (result, (pubkey, _)) in
subscription_results.iter().zip(subscribe_and_fetch.iter())
{
if let Err(err) = result {
error!("Failed to subscribe to account {}: {}", pubkey, err);
- errors.push(format!("{}: {}", pubkey, err));
}
}
- // Fail if ANY subscription failed
- if !errors.is_empty() {
- return Err(
- RemoteAccountProviderError::AccountSubscriptionsTaskFailed(
- format!(
- "{} subscription(s) failed: [{}]",
- errors.len(),
- errors.join(", ")
- ),
- ),
- );
- }
-
Ok(())Committable suggestion skipped: line range outside the PR's diff.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Compatibility
Testing
Checklist
Summary by CodeRabbit
Bug Fixes & Performance
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.