fix: load actor bundles in forest-tool archive diff
#6060
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAn asynchronous preload of actor bundles was added in the archive subcommand’s tipset diff path. After deriving the network from genesis, the code now calls load_actor_bundles(&store, &network).await? before proceeding with timestamp extraction and diff logic. Only one file was changed; no public APIs were modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Archive as show_tipset_diff
participant Store as Blockstore
participant Bundles as load_actor_bundles
CLI->>Archive: run tipset diff
Archive->>Store: compute genesis / derive network
Archive->>Bundles: load_actor_bundles(store, network) (async)
alt success
Bundles-->>Archive: Ok
Archive->>Archive: proceed to extract timestamps & diff
Archive-->>CLI: output diff
else error
Bundles-->>Archive: Err
Archive-->>CLI: propagate error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
LesnyRumcajs
approved these changes
Sep 10, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tool/subcommands/archive_cmd.rs (1)
785-790: Add failure context to bundle loading for easier diagnosis.
Wrap the await with anyhow::Context so users see which network/path failed.- load_actor_bundles(&store, &network).await?; + load_actor_bundles(&store, &network) + .await + .with_context(|| format!("failed to load actor bundles for network {network}"))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tool/subcommands/archive_cmd.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#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.
🧬 Code graph analysis (1)
src/tool/subcommands/archive_cmd.rs (1)
src/daemon/bundle.rs (1)
load_actor_bundles(22-39)
⏰ 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
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
🔇 Additional comments (2)
src/tool/subcommands/archive_cmd.rs (2)
36-36: LGTM: import is appropriate and scoped to Diff path.
785-790: VerifyPersistentStoreimplementation and loader idempotency
- Confirm the concrete
storetype (e.g. ManyCar) implementsPersistentStorewith writableputsemantics soload_actor_bundlescan succeed at compile- and run-time.- Ensure
load_actor_bundlesshort-circuits when bundles already exist (idempotent/offline-safe).
Member
akaladarshi
approved these changes
Sep 15, 2025
This was referenced Sep 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
This PR tries a avoid
actor missing errorinforest-tool archive difffor scenarios like testing an upcoming state migration at a dummy epoche.g. to test GoldenWeek upgrade at 5283321
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Bug Fixes
Performance