Uh oh!
There was an error while loading. Please reload this page.
fix: refactor snapshot export locking / cancelling logic - #7249
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)
WalkthroughAdds ChangesChainExportGuard refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
d6a7c87 to
52594deCompareThere 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 (3)
src/ipld/util.rs (1)
130-134: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t mark the export ended from
cancel_export().Line 132 sets
exporting=falsewhile theChainExportGuardmay still be in scope. In the RPC path, a second export can start in that window, then the old guard’sDroprunsend_export()and clears the new export’s status/token. LetDropown the end transition; cancellation should only markcancelledand signal the token.fn cancel_export() { let status = &*CHAIN_EXPORT_STATUS; - status.exporting.store(false, atomic::Ordering::Relaxed); status.cancelled.store(true, atomic::Ordering::Relaxed); }🤖 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/ipld/util.rs` around lines 130 - 134, The cancel_export helper is ending the export too early by clearing the exporting state before ChainExportGuard is dropped, which can let a new export start and then get wiped out by the old guard’s Drop. Update cancel_export to only mark the export as cancelled and trigger the cancellation token, and leave the end_export transition entirely to ChainExportGuard::drop so the active export lifecycle stays consistent.src/db/gc/snapshot.rs (1)
220-275: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEither observe cancellation here or don’t expose GC exports as cancellable.
This guard publishes a cancellation token through
CHAIN_EXPORT_STATUS, soForestChainExportCancelcan returntrueduring GC snapshot export. Butexport_snapshotnever awaits the token, socompute_tipset_state/chain::exportcontinue after cancellation.Possible direction
- let _guard = ChainExportGuard::try_start_export()?;+ let chain_export_guard = ChainExportGuard::try_start_export()?; @@ - let _ = crate::chain::export::<Sha256, _>(+ let export = crate::chain::export::<Sha256, _>( db, &head_ts, self.recent_state_roots, file, @@ - )- .await?;+ );++ tokio::select! {+ result = export => {+ let _ = result?;+ }+ _ = chain_export_guard.cancellation_token().cancelled() => {+ chain_export_guard.cancel_export();+ anyhow::bail!("snapshot GC export was cancelled");+ }+ }🤖 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/gc/snapshot.rs` around lines 220 - 275, In export_snapshot, the ChainExportGuard currently exposes a cancel token via CHAIN_EXPORT_STATUS, but the export path never checks it, so cancellation is ineffective during GC snapshot export. Update the export flow around compute_tipset_state and chain::export to observe the guard’s cancellation state and abort promptly when canceled, using the existing ChainExportGuard/ForestChainExportCancel path; otherwise, make GC exports non-cancellable by not publishing the token for this code path.src/rpc/methods/chain.rs (1)
489-522: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake ChainExportDiff observe the guard cancellation token.
ForestChainExportCancelcancels the status token and returnstrue, but this diff export holds aChainExportGuardwithout selecting oncancellation_token().cancelled(), so cancellation reports success whiledo_exportcontinues.Proposed fix
- let _guard = ChainExportGuard::try_start_export()?;+ let chain_export_guard = ChainExportGuard::try_start_export()?; @@ - crate::tool::subcommands::archive_cmd::do_export(+ let export = crate::tool::subcommands::archive_cmd::do_export( ctx.chain_index().db(), start_ts, output_path, None, depth, Some(to), Some(chain_finality), true, - )- .await?;+ );++ tokio::select! {+ result = export => result?,+ _ = chain_export_guard.cancellation_token().cancelled() => {+ chain_export_guard.cancel_export();+ tracing::warn!("Snapshot export diff was cancelled");+ return Ok(());+ },+ } 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/rpc/methods/chain.rs` around lines 489 - 522, ChainExportDiff currently starts a ChainExportGuard but never watches the guard’s cancellation token, so the export keeps running after ForestChainExportCancel reports success. Update the ChainExportDiff flow in chain.rs to select on ChainExportGuard::cancellation_token().cancelled() while awaiting archive_cmd::subcommands::archive_cmd::do_export, and ensure the export returns early/cancels cleanly when the token is triggered.
🧹 Nitpick comments (1)
src/ipld/util.rs (1)
67-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the RAII lifecycle contract.
ChainExportGuardis public and its Drop/cancellation side effects are non-obvious; please add doc comments for the guard and its public methods. As per coding guidelines,Document public functions and structs with doc comments.🤖 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/ipld/util.rs` around lines 67 - 92, Add doc comments for the public RAII type and its API in ChainExportGuard: document that try_start_export starts the export lifecycle and returns a guard that owns the cancellation token, cancel_export explicitly ends the active export, and cancellation_token exposes the underlying token for coordination. Keep the docs on the struct and each public method concise and aligned with the lifecycle/cancellation behavior so the contract is clear to callers.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/ipld/util.rs`:
- Around line 72-82: Make the export acquisition in try_start_export and
start_export atomic so concurrent callers cannot both observe exporting as false
and create multiple active guards. Move the full state transition into one
synchronized path, using a compare_exchange-style CAS or a lock around the
multi-field CHAIN_EXPORT_STATUS update, and remove the separate
exporting.store(true, ...) step from start_export if it is now handled by the
atomic transition helper.
- Around line 123-128: Clear the export start timestamp when finishing an export
in end_export, since ChainExportStatus still reads it after completion. Update
the end_export logic to reset start_time alongside cancellation_token and
exporting so completed exports no longer report the previous active export’s
start time.
In `@src/rpc/methods/chain.rs`:
- Line 301: Make the export guard acquisition atomic:
ChainExportGuard::try_start_export() currently does a check-then-set through
exporting() and start_export(), which can race and allow two concurrent RPC
exports to start. Update the export state claim in src/ipld/util.rs to use an
atomic compare_exchange or swap-based transition before marking exporting, and
keep try_start_export() as the caller-facing entry point that relies on the
atomic claim.
---
Outside diff comments:
In `@src/db/gc/snapshot.rs`:
- Around line 220-275: In export_snapshot, the ChainExportGuard currently
exposes a cancel token via CHAIN_EXPORT_STATUS, but the export path never checks
it, so cancellation is ineffective during GC snapshot export. Update the export
flow around compute_tipset_state and chain::export to observe the guard’s
cancellation state and abort promptly when canceled, using the existing
ChainExportGuard/ForestChainExportCancel path; otherwise, make GC exports
non-cancellable by not publishing the token for this code path.
In `@src/ipld/util.rs`:
- Around line 130-134: The cancel_export helper is ending the export too early
by clearing the exporting state before ChainExportGuard is dropped, which can
let a new export start and then get wiped out by the old guard’s Drop. Update
cancel_export to only mark the export as cancelled and trigger the cancellation
token, and leave the end_export transition entirely to ChainExportGuard::drop so
the active export lifecycle stays consistent.
In `@src/rpc/methods/chain.rs`:
- Around line 489-522: ChainExportDiff currently starts a ChainExportGuard but
never watches the guard’s cancellation token, so the export keeps running after
ForestChainExportCancel reports success. Update the ChainExportDiff flow in
chain.rs to select on ChainExportGuard::cancellation_token().cancelled() while
awaiting archive_cmd::subcommands::archive_cmd::do_export, and ensure the export
returns early/cancels cleanly when the token is triggered.
---
Nitpick comments:
In `@src/ipld/util.rs`:
- Around line 67-92: Add doc comments for the public RAII type and its API in
ChainExportGuard: document that try_start_export starts the export lifecycle and
returns a guard that owns the cancellation token, cancel_export explicitly ends
the active export, and cancellation_token exposes the underlying token for
coordination. Keep the docs on the struct and each public method concise and
aligned with the lifecycle/cancellation behavior so the contract is clear to
callers.
🪄 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: 3d4da359-b41d-4290-a232-b8ec4d9542f6
📒 Files selected for processing (3)
src/db/gc/snapshot.rssrc/ipld/util.rssrc/rpc/methods/chain.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
LesnyRumcajs
left a comment
There was a problem hiding this comment.
@hanabi1224 Do you think Ordering::Relaxed is enough here?
hanabi1224
commented
Jun 29, 2026
I think it's fine |
Uh oh!
There was an error while loading. Please reload this page.
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