fix: log full context of anyhow::Error#6795
Merged
hanabi1224 merged 7 commits intomainfrom Mar 26, 2026
Merged
Conversation
Contributor
|
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 (3)
✅ Files skipped from review due to trivial changes (2)
WalkthroughStandardized many function error-return types to the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
hanabi1224
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/chain_sync/network_context.rs (1)
214-254:⚠️ Potential issue | 🟡 MinorSimplify error handling by removing unnecessary anyhow error wrapping in callers.
The calls to
chain_exchange_full_tipset()andchain_exchange_full_tipsets()insrc/chain_sync/chain_follower.rs(lines 443 and 463) use.map_err(|e| anyhow::anyhow!(e))?to handleanyhow::Result<T>. This pattern wraps an already-wrappedanyhow::Error, converting it to a string representation and losing the original error chain.Since both methods already return
anyhow::Result<T>, simplify these to:
await?for direct propagation, or.await.context("additional context")?if additional context is needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain_sync/network_context.rs` around lines 214 - 254, The callers of chain_exchange_full_tipset() and chain_exchange_full_tipsets() are wrapping an anyhow::Error into a new anyhow::Error via .map_err(|e| anyhow::anyhow!(e))? which loses the original error chain; locate the calls to chain_exchange_full_tipset and chain_exchange_full_tipsets in chain_follower.rs and replace .await.map_err(|e| anyhow::anyhow!(e))? with a simple .await? to propagate the existing anyhow::Error, or use .await.context("additional context")? if you need to add context.
🧹 Nitpick comments (3)
src/utils/sqlite/mod.rs (1)
150-158: Consider using{e:#}for error logging to align with PR objective.These
tracing::warn!calls still use{e}format, which only displays the top-level error message. Given this PR's objective is to show full error context/chain using the{:#}formatter, these lines should be updated as well.♻️ Suggested change
if let Err(e) = sqlx::query("VACUUM").execute(db).await { - tracing::warn!("error vacuuming {name} database: {e}") + tracing::warn!("error vacuuming {name} database: {e:#}") } if let Err(e) = sqlx::query("PRAGMA wal_checkpoint(TRUNCATE)") .execute(db) .await { - tracing::warn!("error checkpointing {name} database wal: {e}") + tracing::warn!("error checkpointing {name} database wal: {e:#}") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/sqlite/mod.rs` around lines 150 - 158, Update the tracing warnings in src/utils/sqlite/mod.rs to use the pretty/error-chain formatter so full error context is logged: replace the "{e}" interpolations with "{e:#}" in the two tracing::warn! calls associated with the sqlx::query("VACUUM") execute error and the sqlx::query("PRAGMA wal_checkpoint(TRUNCATE)") execute error (the two blocks that currently bind Err(e) and call tracing::warn!). Ensure both messages now use {e:#} so the complete error chain is emitted.src/db/blockstore_with_write_buffer.rs (1)
66-66: LGTM! Error formatting improvement applied correctly.The change from
{e}to{e:#}correctly applies the alternate formatter to display the full error context chain, which aligns with the PR objectives and improves debuggability.Optional: Consider adding descriptive context to the warning message.
The warning currently logs only the error itself. Adding a brief description would improve log readability:
- tracing::warn!("{e:#}"); + tracing::warn!("Failed to flush buffer on drop: {e:#}");This makes it immediately clear what operation failed, especially useful when scanning logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/blockstore_with_write_buffer.rs` at line 66, The current warning call uses tracing::warn!("{e:#}") which only prints the error chain; update that tracing::warn invocation to include a short descriptive context string (e.g., "failed to flush write buffer" or "error writing blockstore") combined with the error variable e so logs show both what operation failed and the error chain; locate the tracing::warn!("{e:#}") line and modify the message to include the operation context while keeping {e:#} for full error formatting.src/rpc/methods/eth/trace/state_diff.rs (1)
219-219: Inconsistent error formatting within the same function.Line 219 now uses
{e:#}for richer error context, but lines 230 and 240 still use{e}. For consistency with the PR's goal of printing full error chains, consider updating those as well.Suggested diff for consistency
let kamt: EvmStorageKamt<&DB> = match Kamt::load_with_config(&storage_cid, store, config) { Ok(k) => k, Err(e) => { - debug!("failed to load storage KAMT: {e}"); + debug!("failed to load storage KAMT: {e:#}"); return HashMap::default(); } }; let mut entries = HashMap::default(); if let Err(e) = kamt.for_each(|key, value| { entries.insert(key.to_big_endian(), *value); Ok(()) }) { - debug!("failed to iterate storage KAMT: {e}"); + debug!("failed to iterate storage KAMT: {e:#}"); return HashMap::default(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/trace/state_diff.rs` at line 219, The debug log in state_diff.rs uses rich error formatting in one place (debug!("failed to load EVM state for storage extraction: {e:#}")) but other debug/error logs in the same function still use plain "{e}"; update the remaining debug/error calls in the same function (look for the other debug/error statements around the storage extraction logic inside the function handling EVM state — e.g., the other debug lines after the initial load attempt) to use "{e:#}" so all logs print full error chains consistently, keeping the same message text and log level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libp2p/chain_exchange/message.rs`:
- Around line 140-143: The closure that maps TipsetBundle into T currently
discards the original conversion error by formatting a fresh string; instead
wrap the original error into an anyhow::Error and add context so the error chain
is preserved (e.g., replace the map_err(|e| anyhow::anyhow!("failed to convert
from tipset bundle: {e}")) with something that uses
anyhow::Error::new(e).context("failed to convert from tipset bundle") so the
original source (from T::try_from / FullTipset::try_from) is retained).
---
Outside diff comments:
In `@src/chain_sync/network_context.rs`:
- Around line 214-254: The callers of chain_exchange_full_tipset() and
chain_exchange_full_tipsets() are wrapping an anyhow::Error into a new
anyhow::Error via .map_err(|e| anyhow::anyhow!(e))? which loses the original
error chain; locate the calls to chain_exchange_full_tipset and
chain_exchange_full_tipsets in chain_follower.rs and replace .await.map_err(|e|
anyhow::anyhow!(e))? with a simple .await? to propagate the existing
anyhow::Error, or use .await.context("additional context")? if you need to add
context.
---
Nitpick comments:
In `@src/db/blockstore_with_write_buffer.rs`:
- Line 66: The current warning call uses tracing::warn!("{e:#}") which only
prints the error chain; update that tracing::warn invocation to include a short
descriptive context string (e.g., "failed to flush write buffer" or "error
writing blockstore") combined with the error variable e so logs show both what
operation failed and the error chain; locate the tracing::warn!("{e:#}") line
and modify the message to include the operation context while keeping {e:#} for
full error formatting.
In `@src/rpc/methods/eth/trace/state_diff.rs`:
- Line 219: The debug log in state_diff.rs uses rich error formatting in one
place (debug!("failed to load EVM state for storage extraction: {e:#}")) but
other debug/error logs in the same function still use plain "{e}"; update the
remaining debug/error calls in the same function (look for the other debug/error
statements around the storage extraction logic inside the function handling EVM
state — e.g., the other debug lines after the initial load attempt) to use
"{e:#}" so all logs print full error chains consistently, keeping the same
message text and log level.
In `@src/utils/sqlite/mod.rs`:
- Around line 150-158: Update the tracing warnings in src/utils/sqlite/mod.rs to
use the pretty/error-chain formatter so full error context is logged: replace
the "{e}" interpolations with "{e:#}" in the two tracing::warn! calls associated
with the sqlx::query("VACUUM") execute error and the sqlx::query("PRAGMA
wal_checkpoint(TRUNCATE)") execute error (the two blocks that currently bind
Err(e) and call tracing::warn!). Ensure both messages now use {e:#} so the
complete error chain is emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 469e916d-5db1-4eb6-807f-b24ddb876385
📒 Files selected for processing (57)
src/beacon/drand.rssrc/beacon/mock_beacon.rssrc/blocks/header.rssrc/blocks/mod.rssrc/chain/store/chain_store.rssrc/chain/store/errors.rssrc/chain_sync/chain_follower.rssrc/chain_sync/network_context.rssrc/chain_sync/tipset_syncer.rssrc/cli/subcommands/f3_cmd.rssrc/cli_shared/cli/mod.rssrc/daemon/context.rssrc/daemon/db_util.rssrc/daemon/mod.rssrc/db/blockstore_with_write_buffer.rssrc/db/car/forest.rssrc/db/gc/snapshot.rssrc/db/parity_db.rssrc/dev/subcommands/mod.rssrc/dev/subcommands/update_checkpoints_cmd.rssrc/fil_cns/mod.rssrc/fil_cns/validation.rssrc/genesis/mod.rssrc/interpreter/mod.rssrc/interpreter/vm.rssrc/libp2p/behaviour.rssrc/libp2p/chain_exchange/message.rssrc/libp2p/discovery.rssrc/libp2p/service.rssrc/libp2p_bitswap/request_manager.rssrc/message/mod.rssrc/message_pool/config.rssrc/message_pool/errors.rssrc/message_pool/msgpool/selection.rssrc/rpc/error.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/trace/state_diff.rssrc/rpc/methods/gas.rssrc/rpc/methods/state.rssrc/shim/actors/builtin/market/mod.rssrc/shim/actors/builtin/miner/mod.rssrc/shim/crypto.rssrc/shim/state_tree.rssrc/state_manager/circulating_supply.rssrc/state_manager/errors.rssrc/state_manager/mod.rssrc/state_manager/utils.rssrc/state_migration/common/state_migration.rssrc/state_migration/nv19/power.rssrc/state_migration/type_migrations/init/state_v9_to_v10.rssrc/statediff/mod.rssrc/statediff/resolve.rssrc/tool/subcommands/api_cmd.rssrc/tool/subcommands/snapshot_cmd.rssrc/utils/proofs_api/paramfetch.rssrc/utils/sqlite/mod.rs
akaladarshi
reviewed
Mar 25, 2026
akaladarshi
reviewed
Mar 25, 2026
akaladarshi
reviewed
Mar 25, 2026
akaladarshi
reviewed
Mar 25, 2026
Collaborator
akaladarshi
left a comment
akaladarshi
left a comment
There was a problem hiding this comment.
Mostly LGTM, but there are couple of concerns
akaladarshi
approved these changes
Mar 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Changes introduced in this pull request:
:#) foranyhow::Errorin log to show full context. Check out this exampleReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
Refactor