fix: disable overflow in head change broadcast channel - #7462
fix: disable overflow in head change broadcast channel#7462hanabi1224 wants to merge 13 commits into
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:
WalkthroughThe PR migrates chain head-change notifications from Tokio broadcast to ChangesHead-change broadcast migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ChainStore
participant Indexer
participant MessagePool
participant RPCSubscriber
ChainStore->>Indexer: publish head-change stream events
ChainStore->>MessagePool: publish head-change stream events
ChainStore->>RPCSubscriber: publish head-change stream events
Indexer-->>Indexer: process applied tipsets
MessagePool-->>MessagePool: apply head changes
RPCSubscriber-->>RPCSubscriber: publish feed updates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 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 `@src/chain/store/chain_store.rs`:
- Around line 258-259: Update the active-subscriber check in the head-change
publication logic to use self.head_changes_tx.receiver_count() > 0. Remove the
comparison with inactive_receiver_count() so inactive receivers cannot suppress
publication to active receivers.
- Around line 144-146: Retain a deactivated InactiveReceiver for the broadcast
channel lifetime in ChainStore at src/chain/store/chain_store.rs:144-146, and in
both TestApi constructors at src/message_pool/msgpool/test_provider.rs:42 and
:56; configure the TestApi senders with await_active disabled, handle
broadcast_blocking send errors instead of unwrapping when no active receiver
exists, and add a regression test proving subscribers created after construction
receive head changes.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04beca37-e23d-4ec4-817e-f05d2501f9cf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlsrc/chain/store/chain_store.rssrc/daemon/db_util.rssrc/daemon/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/provider.rssrc/message_pool/msgpool/test_provider.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth/pubsub.rssrc/state_manager/message_search.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
b19d888 to
8a6626a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
8a6626a to
aa2d402
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
aa2d402 to
e4de3e2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/rpc/methods/chain.rs (1)
2061-2063: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for
PathChanges::is_empty.
PathChanges::is_emptyis a new public function without a rustdoc comment. Document that it returnstruewhen bothrevertsandappliesare empty.As per coding guidelines, public functions and structs must have doc comments.
Proposed documentation
impl<T> PathChanges<T> { + /// Returns `true` when both revert and apply lists are empty. pub fn is_empty(&self) -> bool {🤖 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 `@src/rpc/methods/chain.rs` around lines 2061 - 2063, Add a rustdoc comment directly above PathChanges::is_empty describing that it returns true only when both reverts and applies are empty, following the project’s documentation style for public functions.Source: Coding guidelines
🤖 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 `@src/chain/store/chain_store.rs`:
- Around line 981-1024: Extend test_head_changes to publish more than
HEAD_CHANGE_BROADCAST_CHANNEL_CAP changes while the receiver is paused, then
drain and assert all changes arrive in order, including the expected behavior
for retained unread changes. Add a separate shutdown scenario that leaves
publication blocked by a full channel, drops or closes the receiver, and
verifies the publisher task exits cleanly.
- Around line 281-285: Update set_heaviest_tipset to avoid calling
broadcast_blocking on the Tokio worker. Route non-empty changes through a single
ordered asynchronous publisher with shutdown cancellation, preserving lossless
delivery and existing receiver checks; do not replace it with a dropping or
best-effort send.
In `@src/rpc/methods/chain.rs`:
- Around line 1142-1147: Update chain_get_path to load and validate the tipset
key before the from == to empty-path shortcut, so an unknown identical key
returns the appropriate error while valid identical keys still return an empty
path. Add a regression test covering an unknown key used for both from and to.
---
Nitpick comments:
In `@src/rpc/methods/chain.rs`:
- Around line 2061-2063: Add a rustdoc comment directly above
PathChanges::is_empty describing that it returns true only when both reverts and
applies are empty, following the project’s documentation style for public
functions.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 280f51ff-72b1-4f71-a6cb-02b7904b03bb
📒 Files selected for processing (2)
src/chain/store/chain_store.rssrc/rpc/methods/chain.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
1 participant
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #7442
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
Tests