fix: tokio::spawn futures for FuturesOrdered#7104
Merged
Merged
Conversation
Contributor
|
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:
WalkthroughWrap ParsedFilter in Arc across RPC filter paths, update filter module signatures and Matcher imports, spawn per-tipset tokio tasks with abort-handle tracking, migrate proofs parameter downloads to JoinSet, and add an AbortHandles utility. ChangesParsedFilter Arc wrapping and concurrent task coordination
Sequence Diagram(s)sequenceDiagram
participant Client
participant EthRPC
participant EthFilterModule
participant TokioTask
participant StateManager
Client->>EthRPC: request logs/filter
EthRPC->>EthFilterModule: call get_events_for_parsed_filter(&Arc<ParsedFilter>)
EthFilterModule->>TokioTask: spawn per-tipset tokio::spawn(task with cloned matcher/state_manager)
TokioTask->>StateManager: load/compute state for tipset
StateManager-->>TokioTask: return events/results
TokioTask-->>EthFilterModule: task result (via JoinHandle)
EthFilterModule->>EthRPC: aggregate results (enforce cap) and return
EthRPC->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
hanabi1224
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/proofs_api/paramfetch.rs (1)
99-117:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix swallowed parameter download errors in
get_params
src/utils/proofs_api/paramfetch.rsawaitsJoinSet::from_iter(...).join_all().awaitbut discards the returned results. Sincefetch_verify_params(...) -> anyhow::Result<()>, anyErrfrom a failed download/verification is ignored andget_paramsstill returnsOk(())—soensure_proof_params_downloaded()will markRUN_ONCEas completed and won’t retry.Propagate task + inner errors
- JoinSet::from_iter( + let results = JoinSet::from_iter( params .into_iter() .filter(|(name, info)| match storage_size { SectorSizeOpt::Keys => !name.ends_with("params"), SectorSizeOpt::Size(size) => { size as u64 == info.sector_size || !name.ends_with(".params") } SectorSizeOpt::All => true, }) .map(|(name, info)| { let data_dir = data_dir.to_owned(); async move { fetch_verify_params(&data_dir, &name, Arc::new(info)).await } }), ) .join_all() .await; + for result in results { + result.context("parameter fetch task panicked")??; + } + Ok(())🤖 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/utils/proofs_api/paramfetch.rs` around lines 99 - 117, The current get_params call creates a JoinSet of async tasks that call fetch_verify_params but discards the Vec of results from .join_all().await, swallowing both task JoinErrors and the inner anyhow::Result errors; change get_params to capture the join_all result into a variable, iterate over each join result and propagate failures (return Err) instead of ignoring them—handle the JoinError (task panics/cancellation) and then unwrap or propagate the inner anyhow::Result from fetch_verify_params (use ? or map_err to convert into the function's anyhow::Result). Locate the block that builds JoinSet::from_iter(...).map(|(name, info)| async move { fetch_verify_params(&data_dir, &name, Arc::new(info)).await }) and replace the discard with proper error checking so ensure_proof_params_downloaded() only marks RUN_ONCE when all tasks succeed.
🤖 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/rpc/methods/eth/filter/mod.rs`:
- Around line 299-320: collect_events_for_tipsets currently spawns per-tipset
tasks with tokio::spawn and collects them via FuturesOrdered, but on early
returns (e.g., when tasks.try_next().await? or ensure_filter_cap(...)?) the
remaining JoinHandles are simply dropped and keep running; change this to a
cancellation-aware pattern: replace the tokio::spawn + FuturesOrdered approach
in collect_events_for_tipsets with a JoinSet (or create AbortHandle/Abortable
per task) so you can call joinset.abort_all() (or AbortHandle::abort()) on any
early error/return path, and ensure all spawned tasks (the closures that call
Self::collect_events) are awaited/aborted before returning to avoid background
scans continuing; locate references to tokio::spawn, FuturesOrdered, and the
async closures that call Self::collect_events to implement this.
---
Outside diff comments:
In `@src/utils/proofs_api/paramfetch.rs`:
- Around line 99-117: The current get_params call creates a JoinSet of async
tasks that call fetch_verify_params but discards the Vec of results from
.join_all().await, swallowing both task JoinErrors and the inner anyhow::Result
errors; change get_params to capture the join_all result into a variable,
iterate over each join result and propagate failures (return Err) instead of
ignoring them—handle the JoinError (task panics/cancellation) and then unwrap or
propagate the inner anyhow::Result from fetch_verify_params (use ? or map_err to
convert into the function's anyhow::Result). Locate the block that builds
JoinSet::from_iter(...).map(|(name, info)| async move {
fetch_verify_params(&data_dir, &name, Arc::new(info)).await }) and replace the
discard with proper error checking so ensure_proof_params_downloaded() only
marks RUN_ONCE when all tasks succeed.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7e19912-5946-4d14-b3e2-2c548b12b4ea
📒 Files selected for processing (5)
src/rpc/methods/eth.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/misc.rssrc/rpc/methods/state.rssrc/utils/proofs_api/paramfetch.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
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 `@src/utils/task/mod.rs`:
- Line 6: Update the doc comment that currently reads "Holds a collection of
[`AbortHandle`] and abort them automatically on drop" to use correct
subject-verb agreement: change "abort them" to "aborts them" so it reads "Holds
a collection of [`AbortHandle`] and aborts them automatically on drop"; this is
the documentation comment associated with the type that stores [`AbortHandle`]
instances in src/utils/task/mod.rs—edit that comment accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0ce24220-6753-4e02-8c3d-408e8f1a327b
📒 Files selected for processing (3)
src/rpc/methods/eth/filter/mod.rssrc/utils/mod.rssrc/utils/task/mod.rs
hanabi1224
force-pushed
the
hm/tokio-spawn-futures-ordered
branch
2 times, most recently
from
c477ec1 to
ae08cee
Compare
Contributor
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)
src/utils/proofs_api/paramfetch.rs (1)
99-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore fail-fast behavior for spawned proof parameter downloads
src/utils/proofs_api/paramfetch.rsusesJoinSet::join_all().await, which waits for allfetch_verify_paramstasks to finish before returning, so the firstanyhow::Result<()>error is reported late and the remaining downloads keep running. Prefer ajoin_next()loop so the function returns on the first taskErr(dropping theJoinSetcancels the rest), and add context for easier debugging.💡 Suggested change
- for task in JoinSet::from_iter( - params - .into_iter() - .filter(|(name, info)| match storage_size { - SectorSizeOpt::Keys => !name.ends_with("params"), - SectorSizeOpt::Size(size) => { - size as u64 == info.sector_size || !name.ends_with(".params") - } - SectorSizeOpt::All => true, - }) - .map(|(name, info)| { - let data_dir = data_dir.to_owned(); - async move { fetch_verify_params(&data_dir, &name, Arc::new(info)).await } - }), - ) - .join_all() - .await - { - _ = task?; - } + let mut tasks: JoinSet<anyhow::Result<()>> = params + .into_iter() + .filter(|(name, info)| match storage_size { + SectorSizeOpt::Keys => !name.ends_with("params"), + SectorSizeOpt::Size(size) => size as u64 == info.sector_size || !name.ends_with(".params"), + SectorSizeOpt::All => true, + }) + .map(|(name, info)| { + let data_dir = data_dir.to_owned(); + async move { fetch_verify_params(&data_dir, &name, Arc::new(info)).await } + }) + .collect(); + + while let Some(task) = tasks.join_next().await { + task.context("proof parameter download task failed")??; + }🤖 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/utils/proofs_api/paramfetch.rs` around lines 99 - 118, The current use of JoinSet::from_iter(...).join_all().await in paramfetch.rs causes the function to wait for all fetch_verify_params tasks and delays error reporting; replace the join_all() pattern with constructing a JoinSet, spawning each async task (calling fetch_verify_params with data_dir/name/Arc::new(info)), then loop using join_next().await to handle each completed task and return early on the first Err (dropping the JoinSet will cancel remaining tasks); when propagating errors wrap them with context (e.g., include the parameter name or storage_size info) to aid debugging.
🤖 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 `@src/utils/proofs_api/paramfetch.rs`:
- Around line 99-118: The current use of
JoinSet::from_iter(...).join_all().await in paramfetch.rs causes the function to
wait for all fetch_verify_params tasks and delays error reporting; replace the
join_all() pattern with constructing a JoinSet, spawning each async task
(calling fetch_verify_params with data_dir/name/Arc::new(info)), then loop using
join_next().await to handle each completed task and return early on the first
Err (dropping the JoinSet will cancel remaining tasks); when propagating errors
wrap them with context (e.g., include the parameter name or storage_size info)
to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1dbb9a0d-d93c-4393-a0e9-c548463a3134
📒 Files selected for processing (4)
src/rpc/methods/eth/filter/mod.rssrc/utils/mod.rssrc/utils/proofs_api/paramfetch.rssrc/utils/task/mod.rs
hanabi1224
force-pushed
the
hm/tokio-spawn-futures-ordered
branch
from
ae08cee to
599a9c1
Compare
hanabi1224
force-pushed
the
hm/tokio-spawn-futures-ordered
branch
from
599a9c1 to
34c5653
Compare
LesnyRumcajs
previously approved these changes
May 26, 2026
Co-authored-by: Hubert <lesny.rumcajs+github@gmail.com>
LesnyRumcajs
approved these changes
May 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Refactor
New Features