fix(test): speed up test-release and codecov on CI #6372
Conversation
WalkthroughThis PR introduces a new forest-dev CLI tool for fetching RPC test snapshots with caching and retry logic, refactors CI workflows to manage proof parameters and sccache configuration, consolidates snapshot fetching into a centralized function, and updates proof parameter handling to respect environment variables while maintaining backward compatibility. Changes
Sequence Diagram(s)sequenceDiagram
actor User as CLI User
participant CLI as fetch_rpc_tests
participant Cache as ProjectDirs Cache
participant Fetch as fetch_rpc_test_snapshot
participant Remote as Remote Server
participant FS as Filesystem
User->>CLI: Run fetch-rpc-tests
CLI->>CLI: Prepare proof params
CLI->>CLI: Read test snapshot list
loop For each snapshot (concurrent)
CLI->>Fetch: fetch_rpc_test_snapshot(name)
Fetch->>Cache: Compute cache path
Fetch->>FS: Check if cached locally
alt Snapshot cached
FS-->>Fetch: Return cached path
else Cache miss or invalid
rect rgb(200, 220, 240)
note over Fetch,Remote: Retry loop (up to 5 retries)
Fetch->>Remote: GET snapshot (30s timeout)
alt Success
Remote-->>Fetch: Snapshot data
Fetch->>FS: Write to cache
else Failure
Remote-->>Fetch: Error
Fetch->>Fetch: Delay 1s, retry
end
end
end
Fetch-->>CLI: Return local path or error
end
CLI->>CLI: Log warnings for failed snapshots
CLI-->>User: Report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
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: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/coverage.yml(3 hunks).github/workflows/unit-tests.yml(2 hunks)Makefile(1 hunks)src/utils/proofs_api/parameters.rs(1 hunks)tests/common/mod.rs(1 hunks)tests/config.rs(1 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-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:
tests/common/mod.rssrc/utils/proofs_api/parameters.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:
tests/common/mod.rsMakefile
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
tests/common/mod.rsMakefile
📚 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:
tests/common/mod.rsMakefile
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
tests/common/mod.rsMakefile
📚 Learning: 2025-08-07T13:39:29.732Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:58-58
Timestamp: 2025-08-07T13:39:29.732Z
Learning: In the Forest project, auto-generated files (like those generated by rust2go tool) should be skipped during code review as they are not meant to be manually edited.
Applied to files:
Makefile
⏰ 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: Build forest binaries on Linux AMD64
- GitHub Check: Coverage
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
🔇 Additional comments (10)
src/utils/proofs_api/parameters.rs (1)
93-101: Logic correctly handles empty environment variable values.The implementation properly treats an empty
FIL_PROOFS_PARAMETER_CACHEas unset, falling back to the default path. This aligns with the test changes and ensures consistent behavior..github/workflows/coverage.yml (4)
31-32: AWS credentials properly configured for SCCache.The AWS credentials are correctly sourced from secrets and made available to the SCCache configuration step, which conditionally uses them only when present (protecting against external PR failures).
48-55: SCCache configuration correctly handles external PRs.The conditional check ensures SCCache variables are only set when AWS credentials are available, preventing failures for external contributors who don't have access to secrets.
66-67: Proof parameter pre-fetching should improve CI speed.Fetching proof parameters before running tests should reduce redundant downloads and speed up the overall workflow, which aligns with the PR objectives.
69-69: Verify that package-scoped coverage aligns with project needs.The
make codecovtarget now runs package-specific coverage (-p forest-filecoin --no-default-features) instead of workspace-wide. Ensure this narrower scope provides sufficient coverage visibility for the project's needs.tests/config.rs (1)
58-58: Test correctly verifies empty environment variable handling.Setting
FIL_PROOFS_PARAMETER_CACHEto an empty string properly tests the new behavior where empty values are treated as unset, falling back to the default path.tests/common/mod.rs (1)
44-47: Conditional environment setup properly respects existing values.The updated logic correctly preserves pre-existing
FIL_PROOFS_PARAMETER_CACHEvalues when set and non-empty, while still providing a sensible default for tests. This aligns with the CI workflow changes where the environment variable is set globally..github/workflows/unit-tests.yml (2)
31-32: Environment configuration consistent with coverage workflow.The AWS credentials and
FIL_PROOFS_PARAMETER_CACHEenvironment variables are properly configured, matching the pattern in the coverage workflow.Also applies to: 36-36
45-52: SCCache configuration correctly handles external PRs.The conditional SCCache setup matches the coverage workflow and properly handles cases where external PRs don't have access to secrets.
Makefile (1)
118-118: Clarify coverage scope for interop-tests package.The codecov target now covers only
forest-filecoinwith default features disabled. Confirm:
- Whether
interop-testsintegration tests require coverage reporting (or if they're covered separately)- The disabled features (jemalloc, tokio-console, tracing-loki, tracing-chrome) are non-critical and appropriate to exclude from coverage
.github/workflows/coverage.yml
Outdated
| - uses: taiki-e/install-action@cargo-llvm-cov | ||
| - uses: taiki-e/install-action@nextest | ||
| - name: Fetch proof params | ||
| run: cargo run --bin forest-tool --no-default-features -- fetch-params --keys |
There was a problem hiding this comment.
Would it be faster to fetch them via docker? I don't think there's a need to compile it unless we're reusing the same compilation units in the codecov stage compilation.
There was a problem hiding this comment.
True, I can switch to docker
There was a problem hiding this comment.
It turns out that fetching only parameter files is insufficient, we need to pre-fetch RPC test snapshots as well to save time and reduce flakyness. Switched to forest-dev solution.
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/coverage.yml(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Coverage
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: tests-release
🔇 Additional comments (4)
.github/workflows/coverage.yml (4)
31-32: Verify AWS credentials have minimal IAM permissions.Ensure these credentials are scoped to read/write access only to the SCCache S3 bucket and cannot perform other AWS operations.
38-38: Verify F3 sidecar FFI exclusion doesn't reduce coverage completeness.Opting out of the F3 sidecar FFI build will speed up compilation but ensure the codecov workflow doesn't need coverage data from F3-related code paths.
39-39: Confirm the hard-coded path is appropriate for the runner environment.The path
/var/tmp/filecoin-proof-parameterswill be auto-created by Docker when mounting the volume, but verify this location is suitable for thebuildjet-4vcpu-ubuntu-2204runner (e.g., sufficient disk space, proper permissions).
48-55: No action required. sccache gracefully falls back to local disk caching when remote storage variables are unconfigured, which is the expected behavior for external PRs lacking AWS credentials.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit-tests.yml (2)
76-85: Pin the Docker image version for reproducibility.The Docker image
ghcr.io/chainsafe/forestlacks a version tag, which means it will implicitly uselatest. This can lead to inconsistent builds if the image changes between CI runs.🔎 Suggested fix
- name: Fetch proof params run: | docker run --rm \ -v $FIL_PROOFS_PARAMETER_CACHE:$FIL_PROOFS_PARAMETER_CACHE \ -e FIL_PROOFS_PARAMETER_CACHE=$FIL_PROOFS_PARAMETER_CACHE \ --entrypoint forest-tool \ - ghcr.io/chainsafe/forest \ + ghcr.io/chainsafe/forest:v0.x.x \ fetch-params --keys sudo chmod -R 755 $FIL_PROOFS_PARAMETER_CACHE ls -ahl $FIL_PROOFS_PARAMETER_CACHEReplace
v0.x.xwith a specific version tag or commit SHA for better reproducibility.
76-85: Consider skipping parameter fetch if already cached.The proof parameters fetch step runs unconditionally on every CI run. Adding a check to skip this step when parameters are already present could improve CI performance further.
💡 Optional optimization
- name: Fetch proof params run: | + if [ ! -d "$FIL_PROOFS_PARAMETER_CACHE" ] || [ -z "$(ls -A $FIL_PROOFS_PARAMETER_CACHE)" ]; then docker run --rm \ -v $FIL_PROOFS_PARAMETER_CACHE:$FIL_PROOFS_PARAMETER_CACHE \ -e FIL_PROOFS_PARAMETER_CACHE=$FIL_PROOFS_PARAMETER_CACHE \ --entrypoint forest-tool \ ghcr.io/chainsafe/forest \ fetch-params --keys sudo chmod -R 755 $FIL_PROOFS_PARAMETER_CACHE + else + echo "Proof parameters already cached, skipping fetch" + fi ls -ahl $FIL_PROOFS_PARAMETER_CACHEThis avoids redundant Docker pulls and parameter downloads when the cache is already populated.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/coverage.yml(3 hunks).github/workflows/unit-tests.yml(3 hunks)src/chain_sync/chain_follower.rs(1 hunks)src/utils/rand/mod.rs(1 hunks)tests/lint.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/coverage.yml
🧰 Additional context used
🧠 Learnings (3)
📓 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:11:55.266Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.
Applied to files:
src/chain_sync/chain_follower.rs
📚 Learning: 2025-10-16T11:05:13.586Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6166
File: src/auth/mod.rs:127-150
Timestamp: 2025-10-16T11:05:13.586Z
Learning: In Rust 2024, `std::env::set_var` and `std::env::remove_var` are unsafe functions. They require `unsafe` blocks because they are only safe in single-threaded programs (Windows is an exception). When these functions are used in tests, ensure proper serialization with `#[serial]` or similar mechanisms.
Applied to files:
src/utils/rand/mod.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). (7)
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: Coverage
- GitHub Check: tests-release
🔇 Additional comments (5)
tests/lint.rs (1)
48-48: LGTM! Environment-driven logging configuration for tests.The switch from static
with_max_leveltoEnvFiltermakes test logging configurable via theRUST_LOGenvironment variable while maintaining a DEBUG default. This improves CI flexibility and aligns with the PR's performance optimization goals.Also applies to: 54-59
src/utils/rand/mod.rs (1)
38-39: LGTM! Appropriate test noise reduction.Suppressing the security warning during tests is correct—fixed RNG seeds are intentional in test environments for reproducibility (as seen in
test_fixed_seed_env). The warning remains active in production builds where it serves as an important security alert.src/chain_sync/chain_follower.rs (1)
887-899: LGTM! Consistent test logging improvements.The test setup changes mirror those in
tests/lint.rs:
.without_time()removes timestamp noise from test outputEnvFilter::builder()pattern provides environment-driven log control while maintaining the DEBUG defaultThese improvements support cleaner, more configurable test execution on CI.
.github/workflows/unit-tests.yml (2)
31-37: LGTM! Good performance optimization.The addition of
RUST_LOG=warnreduces logging overhead during test execution, which aligns with the PR's objective to speed up CI. AWS credentials and the proof parameters cache path are correctly configured using secrets and a standard temporary location.
46-53: LGTM! Proper handling of external PRs.The conditional check correctly determines whether SCCache should be configured by testing if AWS credentials are available. External PRs without access to secrets will skip this configuration, which is the appropriate behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yml (1)
76-85: Consider permission scope for the cached parameters.The
sudo chmod -R 755command makes all proof parameters world-readable and executable. While this ensures the runner can access them, verify whether execute permissions (755) are necessary or if644(read-only) would suffice for these data files.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/unit-tests.yml(3 hunks)src/utils/proofs_api/paramfetch.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/proofs_api/paramfetch.rs
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: Coverage
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: All lint checks
🔇 Additional comments (4)
.github/workflows/unit-tests.yml (4)
31-32: LGTM!AWS credentials are properly exposed for SCCache. GitHub's security model ensures these secrets are empty for external PRs, preventing unauthorized S3 access.
36-37: Verify thatRUST_LOG=warndoesn't suppress critical test diagnostics.Setting
RUST_LOG=warnglobally reduces verbosity for performance, but ensure it doesn't hide useful output when debugging CI failures. Consider if failed test runs still provide sufficient context.
46-53: LGTM!The conditional SCCache configuration correctly handles external PRs that lack AWS credentials, preventing errors while allowing internal PRs to benefit from S3-backed caching.
92-92: LGTM!Setting
FOREST_TEST_SKIP_PROOF_PARAM_CHECK=1after pre-fetching parameters eliminates redundant fetch attempts during test execution, directly supporting the PR's performance optimization goal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/unit-tests.yml (2)
46-53: Good security-conscious design for external PRs.The conditional SCCache configuration correctly handles external PRs that lack access to secrets. This ensures the workflow degrades gracefully without failing.
Minor: Line 49's
${{secrets.AWS_ACCESS_KEY_ID}}(no space) differs slightly from line 31's${{ secrets.AWS_ACCESS_KEY_ID }}(with space). Both work, but consistency improves readability.
76-79: Consider adding error handling for the ls command.The
ls -ahl $FIL_PROOFS_PARAMETER_CACHEon line 79 will fail if the directory doesn't exist or isn't created by the cargo command. Since this appears to be for verification/logging purposes, consider making it more resilient:🔎 Proposed fix to prevent step failure
- name: Fetch proof params and RPC test snapshots run: | cargo run --bin forest-dev --no-default-features --profile quick -- fetch-rpc-tests - ls -ahl $FIL_PROOFS_PARAMETER_CACHE + ls -ahl $FIL_PROOFS_PARAMETER_CACHE || echo "Proof parameter cache directory not found or empty"Alternatively, add
continue-on-error: trueto the entire step if the listing is purely informational.src/dev/subcommands/mod.rs (2)
48-54: Consider simplifying comment handling logic.Lines 52 and 54 have overlapping comment handling:
- Line 52 splits on
"#"and takes the first part (removing inline comments)- Line 54 filters out lines starting with
'#'(removing comment-only lines)The line 54 filter is redundant since line 52 already handles
#at the start. Consider simplifying:🔎 Suggested simplification
let tests = include_str!("../../tool/subcommands/api_cmd/test_snapshots.txt") .lines() .map(|i| { // Remove comment i.split("#").next().unwrap().trim().to_string() }) - .filter(|l| !l.is_empty() && !l.starts_with('#')); + .filter(|l| !l.is_empty());
82-82: Consider usingResumabledownload option for better CI performance.The current implementation uses
DownloadFileOption::NonResumable, which restarts downloads from scratch on each retry. Since this PR aims to speed up CI workflows, usingDownloadFileOption::Resumablecould improve performance for larger snapshots by resuming partial downloads on retry.🔎 Suggested change
- download_file_with_cache(&url, &cache_dir, DownloadFileOption::NonResumable).await + download_file_with_cache(&url, &cache_dir, DownloadFileOption::Resumable).await
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/coverage.yml(3 hunks).github/workflows/unit-tests.yml(3 hunks)src/bin/forest-dev.rs(1 hunks)src/dev/main.rs(1 hunks)src/dev/mod.rs(1 hunks)src/dev/subcommands/mod.rs(1 hunks)src/lib.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)src/utils/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tool/subcommands/api_cmd/test_snapshot.rs
🧰 Additional context used
🧠 Learnings (18)
📓 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-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/lib.rssrc/dev/subcommands/mod.rs
📚 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/lib.rssrc/bin/forest-dev.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/lib.rssrc/dev/main.rssrc/bin/forest-dev.rssrc/dev/subcommands/mod.rs
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
src/bin/forest-dev.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/bin/forest-dev.rs
📚 Learning: 2025-08-07T13:39:29.732Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:58-58
Timestamp: 2025-08-07T13:39:29.732Z
Learning: In the Forest project, auto-generated files (like those generated by rust2go tool) should be skipped during code review as they are not meant to be manually edited.
Applied to files:
src/bin/forest-dev.rs
📚 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 pins Rust toolchain to 1.89.0 via rust-toolchain.toml; features stabilized in 1.88 (e.g., let-chains) are acceptable in this codebase.
Applied to files:
src/bin/forest-dev.rs
📚 Learning: 2025-08-11T12:42:42.848Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:186-190
Timestamp: 2025-08-11T12:42:42.848Z
Learning: In the Forest project's test infrastructure (specifically in `src/tool/subcommands/api_cmd/api_run_tests.rs`), the team prefers to use only `ws` (non-secure WebSocket) connections for simplicity in their stateful API test scenarios, as security is not a concern in their testing context.
Applied to files:
src/bin/forest-dev.rssrc/dev/subcommands/mod.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:
.github/workflows/unit-tests.ymlsrc/dev/subcommands/mod.rs
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.
Applied to files:
.github/workflows/unit-tests.ymlsrc/dev/subcommands/mod.rs
📚 Learning: 2025-08-25T09:53:37.443Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:72-76
Timestamp: 2025-08-25T09:53:37.443Z
Learning: hanabi1224 intentionally designs RPC snapshot test cache to be OS/arch-agnostic, sharing cached snapshot files across different platforms using enableCrossOsArchive: true and hash-based cache keys to improve cache efficiency and reduce redundant downloads.
Applied to files:
.github/workflows/unit-tests.yml
📚 Learning: 2025-08-13T09:43:20.301Z
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.
Applied to files:
.github/workflows/coverage.yml
📚 Learning: 2025-09-09T10:37:17.947Z
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.
Applied to files:
.github/workflows/coverage.yml
📚 Learning: 2025-09-17T11:32:44.185Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6074
File: src/rpc/methods/chain.rs:55-56
Timestamp: 2025-09-17T11:32:44.185Z
Learning: In the Forest codebase, hanabi1224 prefers that CodeRabbit should not warn about potential compilation issues (such as Send bounds, async/await compatibility, etc.) since they are already guarded by CI tests. Focus should be on higher-level design and logic issues instead.
Applied to files:
.github/workflows/coverage.yml
📚 Learning: 2025-08-25T09:57:27.412Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:72-76
Timestamp: 2025-08-25T09:57:27.412Z
Learning: hanabi1224's strategy for RPC snapshot cache versioning: branch isolation is not needed currently, but if snapshot generation logic changes in the future, a version string can be added to the cache key to handle the evolution.
Applied to files:
.github/workflows/coverage.yml
📚 Learning: 2025-08-18T03:09:47.932Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
Applied to files:
.github/workflows/coverage.yml
📚 Learning: 2025-08-19T11:25:56.710Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5946
File: src/rpc/methods/state.rs:1459-1463
Timestamp: 2025-08-19T11:25:56.710Z
Learning: hanabi1224 prefers that CodeRabbit should not warn about compilation errors in the Forest codebase since they are already guarded by CI tests. Focus should be on higher-level design and logic issues instead.
Applied to files:
.github/workflows/coverage.yml
🧬 Code graph analysis (5)
src/lib.rs (2)
src/bin/forest-dev.rs (1)
main(5-7)src/dev/main.rs (1)
main(9-18)
src/dev/main.rs (2)
src/cli_shared/logger/mod.rs (1)
setup_minimal_logger(118-127)src/rpc/client.rs (2)
client(115-115)default_or_from_env(45-76)
src/bin/forest-dev.rs (1)
src/dev/main.rs (1)
main(9-18)
src/dev/mod.rs (1)
src/dev/main.rs (1)
main(9-18)
src/dev/subcommands/mod.rs (4)
src/utils/net/download_file.rs (1)
download_file_with_cache(30-89)src/utils/proofs_api/paramfetch.rs (1)
ensure_proof_params_downloaded(62-81)src/utils/proofs_api/parameters.rs (1)
maybe_set_proofs_parameter_cache_dir_env(121-125)src/utils/mod.rs (1)
retry(116-145)
⏰ 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: Build forest binaries on Linux AMD64
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Coverage
- GitHub Check: All lint checks
- GitHub Check: tests-release
🔇 Additional comments (18)
.github/workflows/unit-tests.yml (2)
31-37: LGTM! Environment configuration looks appropriate.The workflow-level AWS credentials enable the conditional SCCache setup (line 49), and external PRs gracefully degrade without them. The proof parameter cache path and log level are sensible defaults.
86-86: LGTM! Optimization correctly skips redundant proof parameter checks.Since proof parameters are pre-fetched in the previous step (lines 76-79), setting
FOREST_TEST_SKIP_PROOF_PARAM_CHECK=1here avoids redundant downloads during test execution. This is the key optimization for speeding up the test run.src/utils/mod.rs (3)
23-26: LGTM: Cleaner imports after retry simplification.The removal of
FutureExt,FusedFuture,pending, andselectaligns well with the simplified retry implementation that no longer requires manual future fusion and selection.
125-144: LGTM: Simplified and more idiomatic retry implementation.The refactored implementation correctly applies the timeout to the entire retry operation (not per-attempt), and leverages
tokio::time::timeoutwhich is cancel-safe. This is much cleaner than the previous manual fusion approach.
183-183: LGTM: Test module correctly importspending.The test module appropriately retains the
pendingimport needed for timeout test cases.src/lib.rs (2)
50-50: LGTM: Dev module declaration.The new
devmodule is properly declared and positioned alphabetically within the module list.
128-128: LGTM: Consistent public export for dev CLI.The
forest_dev_mainexport follows the established pattern for other binary entry points in the codebase.src/dev/mod.rs (1)
4-5: LGTM: Standard module declarations.The public module declarations for
mainandsubcommandsfollow standard Rust conventions.src/bin/forest-dev.rs (1)
4-7: LGTM: Standard binary entrypoint.The
forest-devbinary follows the established pattern used by other Forest binaries, correctly delegating to the library'sforest_dev_mainwith a multi-threaded Tokio runtime.src/dev/main.rs (1)
9-18: LGTM: Well-structured dev CLI entry point.The implementation follows Forest's established CLI pattern with generic argument handling, minimal logging setup, and environment-based RPC client configuration. The RPC client creation at line 16 will fail early if configuration is invalid, which aligns with the fail-fast approach preferred in the codebase.
src/dev/subcommands/mod.rs (4)
18-26: LGTM: Standard Clap CLI structure.The
Clistruct follows Forest's established pattern with dynamic version and metadata extraction.
28-41: LGTM: Clean subcommand structure.The
Subcommandenum andrunmethod follow a clear delegation pattern. The unused_clientparameter maintains consistency with other CLI command interfaces.
55-64: LGTM: Robust concurrent fetch implementation.The use of
JoinSetenables efficient parallel downloading of test snapshots, while the error handling strategy (logging warnings without failing) is appropriate for bulk fetch operations.
75-87: Verify retry configuration is appropriate for CI.The retry configuration specifies:
- 60-second timeout (total for all retries)
- 5 retries with 1-second delays
For CI optimization, consider whether:
- The 60-second total timeout is sufficient for large snapshots in slower CI environments
- The retry count and delay are optimal for transient network issues
Would you like me to check typical snapshot sizes to assess whether these timeouts are adequate?
.github/workflows/coverage.yml (4)
38-40: LGTM - Environment variables support the speed optimization goal.These additions appropriately support faster CI execution:
FOREST_F3_SIDECAR_FFI_BUILD_OPT_OUT=1skips unnecessary F3 sidecar compilation for coverage testingFIL_PROOFS_PARAMETER_CACHEprovides a consistent location for proof parametersRUST_LOG=warnreduces log verbosity in CI
49-56: LGTM - Conditional SCCache setup correctly handles external PRs.The logic properly distinguishes between internal and external PRs by checking the secret value directly. This ensures that:
- Internal workflows benefit from SCCache acceleration
- External PRs without secret access gracefully skip SCCache configuration without errors
The step placement before checkout (line 57) appears intentional to configure the environment early.
67-70: Error handling and caching strategy for proof parameter fetching need verification.The current approach using
cargo run --bin forest-dev --profile quickis intentional—thequickprofile inherits from release withopt-level = 1andlto = "off", designed to balance compilation speed with optimization. However, two areas warrant review:
Error handling: Verify that the
cargo runcommand and directory listing fail the workflow if either step encounters an error. If not, add explicit error handling.Caching: Verify whether
FIL_PROOFS_PARAMETER_CACHEis already cached between runs. If parameters are large and infrequently updated, consider adding GitHub Actions caching to further optimize CI performance.
31-32: Original assumption about empty AWS credentials is incorrect.Secrets are not passed to the runner when a workflow is triggered from a forked repository (with the exception of GITHUB_TOKEN). GitHub Actions does not set these variables to empty strings for external PRs—it simply withholds them entirely as a security measure. Therefore, the concern about tools handling empty credentials gracefully is based on a false premise.
The conditional check of
secrets.AWS_SECRET_ACCESS_KEYin the SCCache setup (lines 49-56) is the correct approach and properly handles the case where secrets are unavailable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/dev/subcommands/mod.rs (3)
52-54: Optimize string processing and remove redundant filter.Two issues with the comment removal logic:
- Line 52 uses
split("#")with a string slice. For single-character splits, prefersplit('#')with a char for better performance.- Line 54's
!l.starts_with('#')condition is redundant because line 52 already removes everything after the first#, so no line can start with#at this point.🔎 Proposed fix
.map(|i| { // Remove comment - i.split("#").next().unwrap().trim().to_string() + i.split('#').next().unwrap().trim().to_string() }) - .filter(|l| !l.is_empty() && !l.starts_with('#')); + .filter(|l| !l.is_empty());
84-84: Fix error message formatting.The error message has incorrect spacing:
":{e}"should be": {e}"(space after colon).🔎 Proposed fix
- .map_err(|e| anyhow::anyhow!("failed to fetch rpc test snapshot {name} :{e}"))? + .map_err(|e| anyhow::anyhow!("failed to fetch rpc test snapshot {name}: {e}"))?
75-83: Consider aligning retry parameters with test configuration patterns.The retry logic uses a 30-second timeout with 5 retries and a 1-second fixed delay. However, the test configuration in
.config/nextest.toml(lines 43, 51) uses a 5-second delay with exponential backoff and jitter for similar network-dependent operations.Consider whether:
- The delay should be increased from 1s to match the 5s delay used in tests
- Exponential backoff would be beneficial (though the current
retryutility may not support it based on the code snippet)- The 30s timeout is per-attempt or for all attempts combined (based on
src/utils/mod.rs, it appears to be for all attempts)The current parameters mean all 5 retries must complete within 30 seconds total, giving approximately 6 seconds per attempt on average. Verify this aligns with expected download times for RPC test snapshots.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.config/nextest.toml(1 hunks)src/dev/subcommands/mod.rs(1 hunks)src/state_manager/utils.rs(2 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-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/state_manager/utils.rssrc/dev/subcommands/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/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/state_manager/utils.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/dev/subcommands/mod.rs
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/dev/subcommands/mod.rs
📚 Learning: 2025-08-11T12:42:42.848Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:186-190
Timestamp: 2025-08-11T12:42:42.848Z
Learning: In the Forest project's test infrastructure (specifically in `src/tool/subcommands/api_cmd/api_run_tests.rs`), the team prefers to use only `ws` (non-secure WebSocket) connections for simplicity in their stateful API test scenarios, as security is not a concern in their testing context.
Applied to files:
src/dev/subcommands/mod.rs
🧬 Code graph analysis (2)
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)
src/dev/subcommands/mod.rs (4)
src/utils/net/download_file.rs (1)
download_file_with_cache(30-89)src/utils/proofs_api/paramfetch.rs (1)
ensure_proof_params_downloaded(62-81)src/utils/proofs_api/parameters.rs (1)
maybe_set_proofs_parameter_cache_dir_env(121-125)src/utils/mod.rs (1)
retry(116-145)
⏰ 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: tests-release
- GitHub Check: Coverage
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
🔇 Additional comments (5)
src/state_manager/utils.rs (2)
200-200: LGTM! Duration import is necessary for retry timing parameters.The import is correctly added to support the retry timeout and delay configuration.
219-234: LGTM! Retry wrapper enhances reliability and aligns with fail-fast approach.The implementation correctly wraps
download_file_with_cachein a retry helper with appropriate parameters:
- 30-second timeout supports the PR objective to speed up CI by failing fast rather than hanging indefinitely
- 5 retries with 1-second delays provide reasonable resilience against transient network issues
NonResumableoption makes sense in a retry context: each retry starts fresh rather than attempting to resume a potentially corrupted partial downloadThe closure correctly captures
&urland&SNAPSHOT_CACHE_DIRby reference, and the retry pattern properly creates a new future on each attempt.Based on learnings, this aligns with your preference for default timeouts and fail-fast behavior to prevent hanging operations.
src/dev/subcommands/mod.rs (2)
59-64: Consider whether all download failures should be non-fatal.The current implementation logs individual snapshot download failures as warnings but still returns
Ok(()). This means the command will succeed even if all snapshots fail to download.Consider whether at least some failures should be fatal, or if the function should return an error when a certain threshold of downloads fails. This would provide better feedback about partial failures and align with the fail-fast preference noted in the learnings.
67-74: Verify the cache directory structure is intentional.The cache directory is set to
project_dir.cache_dir().join("test").join("rpc-snapshots"). According to the codebase learnings,download_file_with_cachepreserves the URL path structure. Since the URL ishttps://.../rpc_test/{name}, the file will be cached atcache_dir/rpc_test/{name}.This means the final path will be:
~/.cache/forest/test/rpc-snapshots/rpc_test/{name}The extra
rpc_testsubdirectory from the URL path might be intentional, but if you want a flatter structure like~/.cache/forest/test/rpc-snapshots/{name}, you would need to adjust either the URL construction or the cache directory path.Based on learnings, hanabi1224's download_file_with_cache function in src/utils/net/download_file.rs preserves URL path structure in local cache directories.
.config/nextest.toml (1)
45-51: The filtertest(state_compute_)correctly targets the intended network-dependent tests:state_compute_calibnetandstate_compute_mainnet. Both are asynchronous tests that download snapshots from the network. The configuration follows the established pattern and the timeout, retry, and backoff settings are appropriate for network-dependent tests.
Makefile
Outdated
|
|
||
| codecov: | ||
| cargo llvm-cov --workspace --codecov --output-path lcov.info | ||
| cargo llvm-cov -p forest-filecoin --no-default-features --codecov --output-path lcov.info |
There was a problem hiding this comment.
--no-default-features saves ~3min
There was a problem hiding this comment.
Shouldn't we run tests with same features (and thus code paths) that the actual binary runs with?
There was a problem hiding this comment.
@LesnyRumcajs Restored Makefile. It's jemalloc being slow to build
| name: Coverage | ||
| if: github.event.pull_request.draft == false && github.actor != 'dependabot[bot]' | ||
| runs-on: buildjet-4vcpu-ubuntu-2204 | ||
| runs-on: ubuntu-24.04-arm |
There was a problem hiding this comment.
codecov runs on hosted runners now.
There was a problem hiding this comment.
Should we add this binary to the docs script?
There was a problem hiding this comment.
I didn't add it because it adds more noise than usefulness for users. What do you think?
There was a problem hiding this comment.
No huge preference here, we can revisit once forest-dev gets larger.
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
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.