fix: remove unnecessary select! macro #6374
Conversation
WalkthroughRefactors async control flow and standardizes retry logic across multiple modules: removes use of the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-08-08T12:10:45.218ZApplied to files:
📚 Learning: 2025-10-17T09:36:15.757ZApplied to files:
📚 Learning: 2025-08-28T12:52:46.927ZApplied to files:
🧬 Code graph analysis (1)src/daemon/mod.rs (2)
⏰ 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). (7)
🔇 Additional comments (6)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state_migration/tests/mod.rs (1)
84-89: Potential timeout/retry mismatch.The
timeoutof 5 seconds is the overall timeout for the entire retry operation (as per the refactoredretryfunction), not a per-attempt timeout. With 15 retries, most won't execute before the 5-second overall timeout expires. The per-request timeout on line 94 (global_http_client().get(...).timeout(timeout)) also uses the same 5-second value.Consider whether the intent is:
- Overall 5s timeout with up to 15 quick retries (current behavior), or
- A longer overall timeout (e.g., 30s or more) to allow multiple retries to complete
Based on learnings, hanabi1224 prefers fail-fast with reasonable timeouts. If long downloads are expected, consider increasing the overall timeout proportionally to the retry count.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/daemon/mod.rs(2 hunks)src/state_manager/utils.rs(2 hunks)src/state_migration/tests/mod.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)src/utils/mod.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
src/daemon/mod.rs
📚 Learning: 2025-10-17T09:36:15.757Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6128
File: src/ipld/util.rs:23-30
Timestamp: 2025-10-17T09:36:15.757Z
Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.
Applied to files:
src/daemon/mod.rs
📚 Learning: 2025-08-28T12:52:46.927Z
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6011
File: src/cli/main.rs:18-25
Timestamp: 2025-08-28T12:52:46.927Z
Learning: In Forest CLI (src/cli/main.rs), the early RPC network check before Cli::parse_from() does not block help/version commands because clap processes these internally before reaching the RPC call. LesnyRumcajs confirmed this implementation works correctly and that RPC call failures are acceptable in this context.
Applied to files:
src/daemon/mod.rs
📚 Learning: 2025-08-25T13:35:24.230Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5969
File: src/tool/subcommands/snapshot_cmd.rs:412-412
Timestamp: 2025-08-25T13:35:24.230Z
Learning: In src/tool/subcommands/snapshot_cmd.rs, the +1 in `last_epoch = ts.epoch() - epochs as i64 + 1` fixes an off-by-1 bug where specifying --check-stateroots=N would validate N+1 epochs instead of N epochs, causing out-of-bounds errors when the snapshot contains only N recent state roots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshot.rs
📚 Learning: 2025-08-25T14:17:09.129Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:0-0
Timestamp: 2025-08-25T14:17:09.129Z
Learning: hanabi1224's download_file_with_cache function in src/utils/net/download_file.rs preserves URL path structure in local cache directories by using cache_dir.join(url.path().strip_prefix('/').unwrap_or_else(|| url.path())), so snapshots from https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/ are cached locally at ~/.cache/forest/test/rpc-snapshots/rpc_test/ (including the rpc_test subdirectory from the URL path).
Applied to files:
src/tool/subcommands/api_cmd/test_snapshot.rssrc/state_manager/utils.rs
📚 Learning: 2025-08-25T14:17:09.129Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:0-0
Timestamp: 2025-08-25T14:17:09.129Z
Learning: hanabi1224's download_file_with_cache function preserves URL path structure in local cache directories, so snapshots from https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/ are cached locally at ~/.cache/forest/test/rpc-snapshots/rpc_test (including the rpc_test subdirectory from the URL path).
Applied to files:
src/tool/subcommands/api_cmd/test_snapshot.rssrc/state_manager/utils.rs
🧬 Code graph analysis (2)
src/tool/subcommands/api_cmd/test_snapshot.rs (1)
src/utils/net/download_file.rs (1)
download_file_with_cache(30-89)
src/state_manager/utils.rs (2)
src/utils/mod.rs (1)
retry(116-145)src/utils/net/download_file.rs (1)
download_file_with_cache(30-89)
⏰ 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). (7)
- GitHub Check: Coverage
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
🔇 Additional comments (4)
src/utils/mod.rs (1)
125-144: Clean refactoring of the retry logic.The new implementation is simpler and more readable. The timeout now correctly wraps the entire retry operation (including all retries and delays), which is the expected behavior. The removal of
select!macro and associated futures machinery is a good simplification.src/state_manager/utils.rs (1)
219-234: Good addition of retry logic with consistent parameters.The retry configuration (30s timeout, 5 retries, 1s delay) aligns well with the pattern used elsewhere in the codebase. The closure correctly returns the future directly without awaiting, which is the expected pattern for the refactored
retryfunction.src/daemon/mod.rs (1)
741-747: Clean replacement of select! with explicit join_next loop.The refactored
propagate_errorcorrectly iterates through completed services and returns the first error. Thepending().awaitensures the function never returns if all services complete successfully, matching the documented behavior.Note: Task panics (
Errfromjoin_next) are silently ignored. This appears to be pre-existing behavior, but you may want to consider logging or handling panics in a follow-up.src/tool/subcommands/api_cmd/test_snapshot.rs (1)
218-228: Consistent retry configuration with other download operations.The updated parameters (30s timeout, 5 retries, 1s delay) align with the pattern established in
src/state_manager/utils.rs. The closure correctly returns the future directly, matching the expected signature for the refactoredretryfunction.
hanabi1224
force-pushed
the
hm/remove-unnecessary-select
branch
from
9e1d214 to
bf4e33e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.