fix: don't increment subscription id for dups #610
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Sub as Subscribe Handler
participant WD as WsDispatcher
participant Unsubs as per-connection unsubs Map
rect rgb(240,248,255)
Note over Sub,Unsubs: Old flow (before)
Sub->>Unsubs: insert(handle.id -> handle.cleanup)
Sub-->>Sub: return SubResult::SubId(handle.id)
end
rect rgb(245,255,240)
Note over Sub,WD: New flow (after)
Sub->>WD: register_unsub(handle)
WD->>Unsubs: insert(handle.id -> handle.cleanup) (drop prev)
WD-->>Sub: ack
Sub-->>Sub: return SubResult::SubId(handle.id)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
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 |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
magicblock-aperture/src/encoder.rs(6 hunks)magicblock-aperture/src/requests/websocket/account_subscribe.rs(1 hunks)magicblock-aperture/src/requests/websocket/log_subscribe.rs(1 hunks)magicblock-aperture/src/requests/websocket/program_subscribe.rs(1 hunks)magicblock-aperture/src/requests/websocket/slot_subscribe.rs(1 hunks)magicblock-aperture/src/server/websocket/dispatch.rs(2 hunks)magicblock-aperture/src/state/subscriptions.rs(2 hunks)magicblock-aperture/src/utils.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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
📚 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-aperture/src/requests/websocket/log_subscribe.rsmagicblock-aperture/src/requests/websocket/account_subscribe.rsmagicblock-aperture/src/requests/websocket/slot_subscribe.rsmagicblock-aperture/src/state/subscriptions.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-aperture/src/encoder.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (11)
magicblock-aperture/src/requests/websocket/program_subscribe.rs (1)
46-50: LGTM: Centralized lifecycle management.The refactored subscription lifecycle using
register_unsubcorrectly implements the duplicate subscription ID fix. When the same connection subscribes to the same program with the same encoder,subscribe_to_programwill return a handle with the existing subscription ID, andregister_unsubwill properly replace any old cleanup callback.magicblock-aperture/src/requests/websocket/account_subscribe.rs (1)
35-39: LGTM: Consistent lifecycle management pattern.The subscription lifecycle refactoring correctly prevents duplicate subscription ID allocation when the same connection subscribes multiple times to the same account with the same encoder.
magicblock-aperture/src/requests/websocket/log_subscribe.rs (1)
38-42: LGTM: Lifecycle management correctly applied.The refactoring correctly uses
register_unsubto manage subscription cleanup, aligning with the duplicate ID prevention mechanism.magicblock-aperture/src/requests/websocket/slot_subscribe.rs (1)
10-14: LGTM: Consistent refactoring.The subscription lifecycle management is correctly implemented using the centralized
register_unsubmethod.magicblock-aperture/src/state/subscriptions.rs (2)
288-308: LGTM: Core deduplication logic.The
add_subscribermethod correctly implements the duplicate subscription fix. When a subscriber with the same encoder already exists (line 295), it reuses the existingsubscriber.id(line 298) rather than generating a new one. This is the key mechanism that prevents subscription ID increment for duplicates.
387-387: LGTM: Necessary visibility change.Making
CleanUppub(crate) is required for the newregister_unsubmethod in dispatch.rs to accesshandle.cleanup.magicblock-aperture/src/utils.rs (1)
52-59: LGTM: Improved debuggability.Adding
Debugderives toProgramFilterandProgramFiltersenhances diagnostics without changing functionality. This aligns with the broader Debug trait additions across encoder types.magicblock-aperture/src/server/websocket/dispatch.rs (1)
15-17: LGTM: Necessary import addition.Adding
SubscriptionHandleto the imports enables the newregister_unsubmethod.magicblock-aperture/src/encoder.rs (3)
1-1: LGTM: Required import for Debug trait bound.The
std::fmt::Debugimport supports the new Debug trait bound on the Encoder trait.
25-25: LGTM: Debug trait bound improves observability.Adding
Debugas a required bound on theEncodertrait enables better diagnostics and debugging of subscription state, which is valuable for a WebSocket subscription system.
36-177: LGTM: Consistent Debug derives.All encoder types correctly derive
Debugto satisfy the trait bound. This enables debugging of subscription state across all encoder variants.
Summary by CodeRabbit
Refactor
Chores