fix: refactor forest-cli snapshot command#7252
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSnapshot export now runs in detached RPC tasks with temp-file persistence and checksum sidecars, the CLI uses updated status and cancellation handling, and calibnet scripts verify asynchronous export state, cancellation, and concurrent-request rejection. ChangesDetached Export Execution and Concurrency
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
hanabi1224
force-pushed
the
hm/refactor-cli-snapshot-cmd
branch
5 times, most recently
from
31d4dfb to
ff8dac3
Compare
hanabi1224
force-pushed
the
hm/refactor-cli-snapshot-cmd
branch
from
ff8dac3 to
2e1bffd
Compare
…polls' into hm/refactor-cli-snapshot-cmd
hanabi1224
force-pushed
the
hm/refactor-cli-snapshot-cmd
branch
from
e35819a to
bc627d6
Compare
hanabi1224
force-pushed
the
hm/refactor-cli-snapshot-cmd
branch
from
d064942 to
bc7eb69
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/chain.rs (1)
280-280: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not let a read-permission RPC write arbitrary daemon paths.
Both handlers accept
output_pathfrom RPC params and create/persist files on the server while still usingPermission::Read. A read token can overwrite or corrupt files writable by the daemon. Raise the permission level or constrain exports to a configured safe directory with path/symlink validation.Also applies to: 297-308, 343-348, 398-398, 491-491, 526-542
🤖 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` at line 280, The RPC handlers in chain.rs are allowing file writes while still declaring Permission::Read, so a read-only token can persist or overwrite arbitrary daemon files via output_path. Update the affected handler permissions (for example in the relevant PERMISSION constants and export/save flows) to a higher privilege level, or restrict exports through a validated safe directory. Add path and symlink checks in the write/persist logic used by the affected chain RPC methods so only approved destinations can be written.
🧹 Nitpick comments (5)
src/db/car/forest.rs (1)
511-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for the exported path helpers.
These helpers are public API; add short doc comments describing the derived temp/checksum paths. As per coding guidelines,
**/*.rs: Document public functions and structs with doc comments.Suggested docs
+/// Returns the temporary snapshot path used while exporting `output_path`. pub fn tmp_exporting_forest_car_path(output_path: &Path) -> PathBuf { let mut p = output_path.to_owned(); p.add_extension("tmp"); p } +/// Returns the SHA-256 checksum sidecar path for `output_path`. pub fn forest_car_sha256sum_path(output_path: &Path) -> PathBuf { let mut p = output_path.to_owned(); p.add_extension("sha256sum"); p }🤖 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/db/car/forest.rs` around lines 511 - 521, The public helper functions tmp_exporting_forest_car_path and forest_car_sha256sum_path in forest.rs need rustdoc comments. Add short doc comments above each function describing that they derive the temporary export path and the checksum sidecar path from the given output_path, keeping the docs concise and aligned with the existing public API style.Source: Coding guidelines
src/rpc/methods/chain.rs (2)
312-408: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog detached task failures before returning them.
If the client disconnects, the
JoinHandleis dropped and any laterErrfrom the spawned export task is discarded. Wrap the task body and log failures inside the task so kill-resilient exports remain observable.Also applies to: 503-552
🤖 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 312 - 408, The spawned export task in the chain export method can fail after the client disconnects, and those errors are currently only returned through the JoinHandle so they may be dropped. Update the tokio::spawn body in the export flow (and the analogous export path mentioned in the review) to wrap the task work in error logging, so any Err from crate::chain::export / crate::chain::export_v2 or related setup is logged inside the task before being returned. Keep the existing handle.await?? path, but ensure the task itself emits a tracing::error with enough context when it fails.
298-308: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttach path context to filesystem failures.
The new server-side export path has several bare
?filesystem operations, so users can get unactionable I/O errors after long exports. Add.with_context(...)including the affected path. As per coding guidelines,**/*.rs: Useanyhow::Result<T>for most operations and add context with.context()when errors occur.Also applies to: 343-348, 398-398, 526-542
🤖 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 298 - 308, The server-side export flow in chain.rs has several filesystem operations that still use bare `?`, so attach path-aware context to each failure using `.with_context(...)` or `.context(...)` with the affected path. Update the checksum write logic and the other export-related filesystem calls in the same export path (including the code around the snapshot/export helpers) so errors surface which file or directory failed, while keeping the existing anyhow::Result<T> style in functions like the export routines.Source: Coding guidelines
scripts/tests/calibnet_export_check.sh (1)
85-90: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReap the killed CLI process before waiting on server completion.
After sending
SIGKILL, wait for the backgroundforest-cliand ignore its expected non-zero status; this keeps the kill-resilience test from leaving an unreaped child whileexport-status --waitverifies the detached server job.Proposed fix
# Killing the CLI should not cancel the export echo "killing cli command" -kill -KILL $EXPORT_CMD_PID +kill -KILL "$EXPORT_CMD_PID" +wait "$EXPORT_CMD_PID" || true # Wait on the same export job echo "waiting on export-status" $FOREST_CLI_PATH snapshot export-status --wait🤖 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 `@scripts/tests/calibnet_export_check.sh` around lines 85 - 90, The kill-resilience test leaves the background forest-cli process unreaped after SIGKILL; in scripts/tests/calibnet_export_check.sh, update the export-check flow around EXPORT_CMD_PID so the killed CLI is waited on and its expected non-zero exit is ignored before running snapshot export-status --wait. Keep the fix local to the shell test logic by reaping the background job after kill -KILL and before checking the detached server-side export completion.scripts/tests/calibnet_export_diff_check.sh (1)
75-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReap the killed CLI process before waiting on server completion.
After sending
SIGKILL, wait for the backgroundforest-cliand ignore its expected non-zero status; this keeps the kill-resilience test from leaving an unreaped child whileexport-status --waitverifies the detached server job.Proposed fix
# Killing the CLI should not cancel the export -kill -KILL $EXPORT_CMD_PID +kill -KILL "$EXPORT_CMD_PID" +wait "$EXPORT_CMD_PID" || true # Wait on the same export job $FOREST_CLI_PATH snapshot export-status --wait🤖 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 `@scripts/tests/calibnet_export_diff_check.sh` around lines 75 - 78, The kill-resilience test is leaving the background forest-cli child unreaped after SIGKILL. In scripts/tests/calibnet_export_diff_check.sh, after kill -KILL on EXPORT_CMD_PID, explicitly wait on that PID and ignore its expected failure before calling snapshot export-status --wait, so the test reaps the killed CLI while still verifying the detached export job via FOREST_CLI_PATH.
🤖 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/cli/subcommands/snapshot_cmd.rs`:
- Around line 179-180: The `ForestChainExport` flow still runs checksum/persist
logic even when `dry_run` is enabled, which breaks the no-output semantics now
that persistence moved server-side. Update the export handling in `snapshot_cmd`
so the `ApiExportResult::Done` path only proceeds with checksum/persist behavior
when `dry_run` is false, and make sure the `VoidAsyncWriter` branch is
effectively short-circuited before any server-side persistence work. Ensure the
`ApiExportResult` completion path preserves the simplified CLI behavior for dry
runs.
- Around line 327-333: In snapshot_cmd.rs, the progress task cleanup in the
export flow should run even when the RPC call fails. Update the logic around the
ForestChainExportDiff request in the export path so you store the await result
first, then call cancellation_token.cancel(), finish the progress bar, and await
the handle before applying the error with ?. Use the export_result and handle
cleanup sequence to ensure the spawned progress task is always stopped before
bubbling up RPC errors.
In `@src/rpc/methods/chain.rs`:
- Around line 345-399: The dry-run path in the export logic still executes
persistence work after using VoidAsyncWriter, which causes failures and side
effects. Update the chain export success handling in the export match/select
flow so that when dry_run is true it skips both save_checksum and
tmp_path.persist, and only performs those operations in the non-dry-run branch;
use the existing dry_run flag and the chain_export/result handling to gate the
finalization logic.
---
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Line 280: The RPC handlers in chain.rs are allowing file writes while still
declaring Permission::Read, so a read-only token can persist or overwrite
arbitrary daemon files via output_path. Update the affected handler permissions
(for example in the relevant PERMISSION constants and export/save flows) to a
higher privilege level, or restrict exports through a validated safe directory.
Add path and symlink checks in the write/persist logic used by the affected
chain RPC methods so only approved destinations can be written.
---
Nitpick comments:
In `@scripts/tests/calibnet_export_check.sh`:
- Around line 85-90: The kill-resilience test leaves the background forest-cli
process unreaped after SIGKILL; in scripts/tests/calibnet_export_check.sh,
update the export-check flow around EXPORT_CMD_PID so the killed CLI is waited
on and its expected non-zero exit is ignored before running snapshot
export-status --wait. Keep the fix local to the shell test logic by reaping the
background job after kill -KILL and before checking the detached server-side
export completion.
In `@scripts/tests/calibnet_export_diff_check.sh`:
- Around line 75-78: The kill-resilience test is leaving the background
forest-cli child unreaped after SIGKILL. In
scripts/tests/calibnet_export_diff_check.sh, after kill -KILL on EXPORT_CMD_PID,
explicitly wait on that PID and ignore its expected failure before calling
snapshot export-status --wait, so the test reaps the killed CLI while still
verifying the detached export job via FOREST_CLI_PATH.
In `@src/db/car/forest.rs`:
- Around line 511-521: The public helper functions tmp_exporting_forest_car_path
and forest_car_sha256sum_path in forest.rs need rustdoc comments. Add short doc
comments above each function describing that they derive the temporary export
path and the checksum sidecar path from the given output_path, keeping the docs
concise and aligned with the existing public API style.
In `@src/rpc/methods/chain.rs`:
- Around line 312-408: The spawned export task in the chain export method can
fail after the client disconnects, and those errors are currently only returned
through the JoinHandle so they may be dropped. Update the tokio::spawn body in
the export flow (and the analogous export path mentioned in the review) to wrap
the task work in error logging, so any Err from crate::chain::export /
crate::chain::export_v2 or related setup is logged inside the task before being
returned. Keep the existing handle.await?? path, but ensure the task itself
emits a tracing::error with enough context when it fails.
- Around line 298-308: The server-side export flow in chain.rs has several
filesystem operations that still use bare `?`, so attach path-aware context to
each failure using `.with_context(...)` or `.context(...)` with the affected
path. Update the checksum write logic and the other export-related filesystem
calls in the same export path (including the code around the snapshot/export
helpers) so errors surface which file or directory failed, while keeping the
existing anyhow::Result<T> style in functions like the export routines.
🪄 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: 83bd5ff2-56da-44e5-aa72-11989377ac9f
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
scripts/tests/calibnet_export_check.shscripts/tests/calibnet_export_diff_check.shsrc/cli/subcommands/snapshot_cmd.rssrc/db/car/forest.rssrc/lib.rssrc/rpc/methods/chain.rssrc/rpc/types/mod.rssrc/tool/subcommands/archive_cmd.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
That's a breaking change, no? |
| .into_temp_path()) | ||
| } | ||
|
|
||
| pub fn tmp_exporting_forest_car_path(output_path: &Path) -> PathBuf { |
There was a problem hiding this comment.
nit: let's add a simple test here. The code is kind of obvious but obvious things managed to bite us in the past. Plus, we want coverage to be high.
| if !self.cancellation_token.is_cancelled() { | ||
| self.cancellation_token.cancel(); | ||
| } |
There was a problem hiding this comment.
Do we need to check if it's cancelled? Might be a TOCTOU issue
2 participants
Summary of changes
On top of #7249 and #7254
Changes introduced in this pull request:
forest.car.sha256sumtoforest.car.zst.sha256sumReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit