Implement online index backfill - #7408
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughForest adds daemon-hosted index backfilling with resumable checkpoints, cancellation, progress reporting, RPC methods, CLI commands, shared workload coordination, timestamp-aware mappings, and updated specifications and documentation. ChangesIndex backfill
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Forest.IndexBackfill
participant BACKFILL_STATUS
participant run_backfill
participant ChainStore
CLI->>Forest.IndexBackfill: submit range and options
Forest.IndexBackfill->>run_backfill: spawn guarded backfill
run_backfill->>ChainStore: index messages and persist checkpoint
CLI->>BACKFILL_STATUS: poll progress
CLI->>Forest.IndexBackfillCancel: request cancellation
BACKFILL_STATUS-->>CLI: terminal state and counters
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
sudo-shashank
changed the title
![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
🧹 Nitpick comments (6)
src/state_manager/tests.rs (1)
384-394: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the expected error, and consider a success-path case.
Both calls are expected to fail, but
let _ =hides that: ifload_executed_tipset_uncachedever started succeeding here, the test would still pass while no longer covering what its name claims. The cache-untouched invariant is most interesting on the successful load path, which isn't exercised.♻️ Make the premise explicit
- let _ = sm.load_executed_tipset_uncached(&ts, false).await; + assert!(sm.load_executed_tipset_uncached(&ts, false).await.is_err()); assert!(sm.cache.get(ts.key()).is_none());🤖 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/state_manager/tests.rs` around lines 384 - 394, Update the test around load_executed_tipset_uncached to assert that both current calls return the expected error instead of discarding their results. Add a successful uncached-load case with valid executable state, assert it returns successfully, and verify the tipset-state cache remains untouched after that load.src/cli/subcommands/index_cmd.rs (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer letting clap reject
--from --resumerather than silently ignoring--resume.The RPC handler gives
fromprecedence, so--resumebecomes a no-op.#[arg(long, conflicts_with = "from")]surfaces that at parse time instead of in a doc string.🤖 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/cli/subcommands/index_cmd.rs` around lines 41 - 44, Update the `resume` argument definition in the CLI command to declare a Clap conflict with the `from` argument using `conflicts_with = "from"`, and remove the documentation stating that `--resume` is ignored when `--from` is provided.src/daemon/db_util.rs (2)
760-804: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftLong-running blocking DB work now runs on the runtime thread inside the daemon.
start_ts.chain(&db)is a synchronous iterator andprocess_signed_messagesperforms a whole batch of serialization + settings/eth-mapping writes inline. Offline this was a dedicated process; online it occupies a Tokio worker for the duration of each batch, competing with sync and RPC. Consider wrapping the batch commit (and ideally the tipset load) intokio::task::spawn_blocking, or at minimumtokio::task::yield_now().awaitper iteration.As per coding guidelines: "Use
tokio::task::spawn_blockingfor CPU-intensive work such as VM execution and cryptography".🤖 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/daemon/db_util.rs` around lines 760 - 804, Move the synchronous backfill work in the loop around `start_ts.chain(&state_manager.chain_store().db())` and `process_signed_messages` off the Tokio worker, using `tokio::task::spawn_blocking` for each batch commit and, where feasible, tipset loading/processing. Await the blocking tasks while preserving batch boundaries, checkpoint writes, cancellation handling, and report updates.Source: Coding guidelines
861-965: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftGood guard/status coverage;
run_backfillitself is untested.The checkpoint/cancel interplay in
run_backfill(resume epoch written on cancel, checkpoint cleared on success, batch flush boundary) is the part most likely to regress. A small test over a synthetic chain withRangeSpec::NumTipsetsand a pre-cancelled token would cover it.🤖 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/daemon/db_util.rs` around lines 861 - 965, Extend the test module with a focused `run_backfill` test using a synthetic chain, `RangeSpec::NumTipsets`, and a pre-cancelled cancellation token. Verify cancellation preserves or writes the resume checkpoint, successful execution clears the checkpoint, and processing honors the batch flush boundary; reuse existing chain/state-manager helpers and `run_backfill` APIs rather than adding unrelated coverage.src/rpc/methods/chain.rs (1)
682-839: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the new RPC handlers.
None of the three new handlers (
IndexBackfill,IndexBackfillStatus,IndexBackfillCancel) have accompanying tests in this diff.src/rpc/methods/sync.rsalready has a reusablectx()test-harness pattern for building anRPCState; a similar test could exerciseindex_backfill_range_spec's validation branches and the cancel/status handlers againstBACKFILL_STATUSat low cost.🤖 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 682 - 839, Add unit tests for the new IndexBackfill, IndexBackfillStatus, and IndexBackfillCancel handlers using the reusable ctx() RPCState test harness pattern from sync.rs. Cover all validation branches in index_backfill_range_spec, and exercise status and cancellation behavior against BACKFILL_STATUS without introducing unrelated test coverage.docs/docs/users/reference/cli.md (1)
800-874: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFenced code blocks missing language specifiers (MD040).
markdownlint flags the 4 new fenced blocks (802, 819, 854, 866) for no language. This matches the pre-existing convention across the rest of this auto-generated file (e.g., the
snapshot exportblock above), sincecli.shemits plain ``` fences around--helpoutput. A real fix would need to update the generator for all sections, not just the new ones.🤖 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 `@docs/docs/users/reference/cli.md` around lines 800 - 874, Update the generator that emits CLI help documentation, rather than editing only these four blocks, so every generated help-output fence includes an appropriate language specifier. Preserve the existing plain-text rendering and apply the change consistently across all sections produced by the generator, including the existing `snapshot export` output.Source: Linters/SAST tools
🤖 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 `@CHANGELOG.md`:
- Line 34: Update the changelog entry for the index backfill RPC and CLI feature
to reference issue `#7340` using its issue URL instead of pull request `#7408`,
while preserving the existing description.
---
Nitpick comments:
In `@docs/docs/users/reference/cli.md`:
- Around line 800-874: Update the generator that emits CLI help documentation,
rather than editing only these four blocks, so every generated help-output fence
includes an appropriate language specifier. Preserve the existing plain-text
rendering and apply the change consistently across all sections produced by the
generator, including the existing `snapshot export` output.
In `@src/cli/subcommands/index_cmd.rs`:
- Around line 41-44: Update the `resume` argument definition in the CLI command
to declare a Clap conflict with the `from` argument using `conflicts_with =
"from"`, and remove the documentation stating that `--resume` is ignored when
`--from` is provided.
In `@src/daemon/db_util.rs`:
- Around line 760-804: Move the synchronous backfill work in the loop around
`start_ts.chain(&state_manager.chain_store().db())` and
`process_signed_messages` off the Tokio worker, using
`tokio::task::spawn_blocking` for each batch commit and, where feasible, tipset
loading/processing. Await the blocking tasks while preserving batch boundaries,
checkpoint writes, cancellation handling, and report updates.
- Around line 861-965: Extend the test module with a focused `run_backfill` test
using a synthetic chain, `RangeSpec::NumTipsets`, and a pre-cancelled
cancellation token. Verify cancellation preserves or writes the resume
checkpoint, successful execution clears the checkpoint, and processing honors
the batch flush boundary; reuse existing chain/state-manager helpers and
`run_backfill` APIs rather than adding unrelated coverage.
In `@src/rpc/methods/chain.rs`:
- Around line 682-839: Add unit tests for the new IndexBackfill,
IndexBackfillStatus, and IndexBackfillCancel handlers using the reusable ctx()
RPCState test harness pattern from sync.rs. Cover all validation branches in
index_backfill_range_spec, and exercise status and cancellation behavior against
BACKFILL_STATUS without introducing unrelated test coverage.
In `@src/state_manager/tests.rs`:
- Around line 384-394: Update the test around load_executed_tipset_uncached to
assert that both current calls return the expected error instead of discarding
their results. Add a successful uncached-load case with valid executable state,
assert it returns successfully, and verify the tipset-state cache remains
untouched after that load.
🪄 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: 8bea9ac5-23cd-4518-9ba2-53c038bc95a2
⛔ 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 (17)
CHANGELOG.mddocs/docs/users/reference/cli.mddocs/docs/users/reference/cli.shdocs/openrpc-specs/v0.jsondocs/openrpc-specs/v1.jsondocs/openrpc-specs/v2.jsonsrc/chain/store/chain_store.rssrc/cli/subcommands/index_cmd.rssrc/cli/subcommands/mod.rssrc/daemon/db_util.rssrc/daemon/mod.rssrc/db/gc/snapshot.rssrc/ipld/export_status.rssrc/rpc/methods/chain.rssrc/rpc/mod.rssrc/state_manager/state_computation.rssrc/state_manager/tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
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/db/gc/snapshot.rs (1)
185-190: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not use
is_running()as the backfill/GC coordination primitive.This is only a status check: backfill can observe
falseimmediately beforegc_once()claims the run, allowing both workloads to overlap. Use a shared guard/lock for admission and retain this accessor for observability only.🤖 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 185 - 190, Replace uses of SnapshotGc::is_running() for backfill/GC admission with a shared guard or lock that atomically coordinates entry between backfill and gc_once(). Retain is_running() solely as an observability status accessor, and ensure neither workload can begin while the other holds the coordination guard.
🧹 Nitpick comments (1)
src/db/gc/snapshot.rs (1)
220-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context when starting the export guard.
The new error boundary propagates guard-start failures without contextual information, making coordination failures harder to diagnose.
As per coding guidelines, add
.context()when errors occur.Suggested fix
- let chain_export_guard = ChainExportGuard::try_start_export(ChainExportKind::SnapshotGc)?; + let chain_export_guard = + ChainExportGuard::try_start_export(ChainExportKind::SnapshotGc) + .context("failed to start snapshot GC export")?;🤖 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 - 230, Add contextual error information to the ChainExportGuard::try_start_export call in export_snapshot, using anyhow’s .context() before propagating the guard-start failure. Keep the existing export, cleanup, and error-handling flow unchanged.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.
Outside diff comments:
In `@src/db/gc/snapshot.rs`:
- Around line 185-190: Replace uses of SnapshotGc::is_running() for backfill/GC
admission with a shared guard or lock that atomically coordinates entry between
backfill and gc_once(). Retain is_running() solely as an observability status
accessor, and ensure neither workload can begin while the other holds the
coordination guard.
---
Nitpick comments:
In `@src/db/gc/snapshot.rs`:
- Around line 220-230: Add contextual error information to the
ChainExportGuard::try_start_export call in export_snapshot, using anyhow’s
.context() before propagating the guard-start failure. Keep the existing export,
cleanup, and error-handling flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1b41c657-f201-4c53-82de-85787a534099
📒 Files selected for processing (2)
CHANGELOG.mdsrc/db/gc/snapshot.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
1 participant
Summary of changes
Changes introduced in this pull request:
Forest.IndexBackfill/IndexBackfillStatus/IndexBackfillCancelRPC methods and matchingforest-cli index backfill/backfill-status/backfill-cancelcommands to back-fill the chain index (Ethereum mappings, events, block blooms) through the running daemon — no node shutdown required, with progress polling and cancellation.--from/--to/--n-tipsets,--recompute,--allow-near-head,--no-wait, and resumable runs (--resume) backed by a persisted checkpoint in the settings store.Reference issue to close (if applicable)
Closes #7340
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Summary by CodeRabbit
New Features
forest-cli index backfill*commands and new RPC methods:Forest.IndexBackfill,Forest.IndexBackfillStatus, andForest.IndexBackfillCancel.--resume), progress reporting, and cancellation, with a dedicated “index backfill” execution slot.Bug Fixes
Documentation