fix: properly store errors in Iterator.Next() method #1090

GarmashAlex · 2025-04-29T13:58:08Z

This commit fixes the error handling in the Iterator.Next() method
by explicitly storing errors from traversal.next() in the iterator's
error field. Previously, errors were silently dropped, causing them to
be unavailable through the Error() method.

The fix:

Separates the condition checking for errors and nil nodes
Properly stores any returned error in iter.err
Maintains the same behavior of marking the iterator as invalid
in both error and end-of-traversal cases

Summary by CodeRabbit

Bug Fixes
- Improved error handling during iteration to better capture and report errors separately from normal completion.
- Enhanced stability by ensuring iteration stops and reports errors when encountering missing data during traversal.

coderabbitai · 2025-04-29T13:58:16Z

Walkthrough

The Iterator.Next() method was updated to explicitly assign the error returned by the underlying traversal's next() method to the iterator's internal error field. When an error occurs, the iterator records the error, clears the traversal, and marks itself as invalid. A new test was added to verify this error handling behavior by simulating a missing node scenario.

Changes

File(s)	Change Summary
iterator.go	Modified `Iterator.Next()` to assign traversal errors to the iterator's error field and handle error and end-of-iteration cases distinctly.
iterator_test.go	Added `TestIterator_Next_ErrorHandling` to test iterator behavior on node retrieval errors by simulating a missing node; imported `errors` package.

Suggested reviewers

yihuang
aljo242

Poem

In the garden of code, where iterators roam,
Now errors and endings each have their own home.
No more confusion—each case is clear,
The path through the nodes is easier to steer.
With every hop, the logic's refined—
A rabbit's delight in the code you designed! 🐇

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (1.64.8)

Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2
Failed executing command with error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b720618 and 225c562.

📒 Files selected for processing (1)

iterator_test.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

iterator_test.go

✨ Finishing Touches

📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

iterator.go

Co-authored-by: yihuang <[email protected]>

yihuang

Can you fix the code format, I think my code suggestion messed up the spaces and tabs.

GarmashAlex · 2025-05-01T07:20:18Z

Can you fix the code format, I think my code suggestion messed up the spaces and tabs.

corrected,
PS C:\Users...\iavl-master> gofmt -l iterator.go
PS C:\Users....\iavl-master>

GarmashAlex · 2025-05-14T15:07:49Z

@yihuang Hello, is this pull request alright now? Or I need to wait for more reviews?

aljo242 · 2025-05-14T21:12:25Z

This looks good @GarmashAlex could you add a test to verify that this is working?

GarmashAlex · 2025-05-15T19:28:03Z

@aljo242 Done

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

iterator_test.go (2)
386-391: Unused mock implementation.

The mockNodeDB type is defined but not actually used in the test case below. Consider either using this mock or removing it to avoid confusion.
-// mockNodeDB always returns an error for GetNode
-type mockNodeDB struct{}
-
-func (m *mockNodeDB) GetNode(_ []byte) (*Node, error) {
-	return nil, errors.New("mock nodeDB error")
-}
-
393-419: Good test for error handling in Iterator.Next()

This test effectively validates the fix for storing errors in the Iterator.Next() method. It creates a scenario where a node retrieval will fail due to a missing node key, then verifies that the iterator properly captures and exposes the error.

A few minor suggestions:

Add a comment explaining that this test specifically validates the fix in Iterator.Next() for properly storing errors

Consider making the error message check less brittle by using a more general assertion
 func TestIterator_Next_ErrorHandling(t *testing.T) {
+	// Test that errors from traversal.next() are properly stored in the iterator's error field
 	db := dbm.NewMemDB()
 	ndb := newNodeDB(db, 0, DefaultOptions(), NewNopLogger())
 	tree := &ImmutableTree{ndb: ndb}

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 2cb564b and b720618.

📒 Files selected for processing (1)

iterator_test.go (2 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

iterator_test.go (6)

node.go (1)

Node (59-75)

db/memdb.go (1)

NewMemDB (55-60)

options.go (1)

DefaultOptions (95-97)

logger.go (1)

NewNopLogger (25-27)

immutable_tree.go (1)

ImmutableTree (16-23)

iterator.go (1)

Iterator (172-182)

🔇 Additional comments (2)

iterator_test.go (2)

4-4: Import added appropriately.

The errors package is correctly imported to support the new test case that validates error handling.

415-418: Comprehensive assertions for error handling.

The test correctly verifies all aspects of the iterator's behavior after an error:

The iterator becomes invalid

The error is properly set in the iterator

The error message contains expected content

This provides good coverage for the fix described in the PR.

aljo242 · 2025-05-27T14:53:54Z

@Mergifyio backport release/v1.2.x

aljo242 · 2025-05-27T14:53:58Z

@Mergifyio backport release/v1.3.x

mergify · 2025-05-27T14:54:32Z

backport release/v1.2.x

✅ Backports have been created

#1098 fix: properly store errors in Iterator.Next() method (backport #1090) has been created for branch release/v1.2.x

mergify · 2025-05-27T14:54:32Z

backport release/v1.3.x

✅ Backports have been created

#1099 fix: properly store errors in Iterator.Next() method (backport #1090) has been created for branch release/v1.3.x

GarmashAlex · 2025-05-27T15:27:53Z

@aljo242 corrected lint error

Co-authored-by: yihuang <[email protected]> Co-authored-by: Alex | Interchain Labs <[email protected]> (cherry picked from commit 4027b8d)

fix: properly store errors in Iterator.Next() method

2476099

yihuang approved these changes Apr 29, 2025

View reviewed changes

yihuang reviewed Apr 29, 2025

View reviewed changes

iterator.go Outdated Show resolved Hide resolved

Update iterator.go

4ecc53e

Co-authored-by: yihuang <[email protected]>

GarmashAlex requested a review from yihuang April 30, 2025 14:52

yihuang approved these changes Apr 30, 2025

View reviewed changes

Update iterator.go

35203fd

GarmashAlex requested a review from yihuang May 5, 2025 03:49

yihuang approved these changes May 5, 2025

View reviewed changes

Alex | Interchain Labs and others added 3 commits May 14, 2025 17:12

Merge branch 'master' into hpp1

6495277

Merge branch 'master' into hpp1

2cb564b

add test

b720618

coderabbitai bot reviewed May 15, 2025

View reviewed changes

Merge branch 'master' into hpp1

4e806cc

Update iterator_test.go

225c562

aljo242 enabled auto-merge (squash) May 27, 2025 16:01

aljo242 approved these changes May 27, 2025

View reviewed changes

aljo242 merged commit 4027b8d into cosmos:master May 27, 2025
8 checks passed

mergify bot pushed a commit that referenced this pull request May 27, 2025

fix: properly store errors in Iterator.Next() method (#1090)

226a234

Co-authored-by: yihuang <[email protected]> Co-authored-by: Alex | Interchain Labs <[email protected]> (cherry picked from commit 4027b8d)

mergify bot mentioned this pull request May 27, 2025

fix: properly store errors in Iterator.Next() method (backport #1090) #1098

Open

mergify bot pushed a commit that referenced this pull request May 27, 2025

fix: properly store errors in Iterator.Next() method (#1090)

98829c9

Co-authored-by: yihuang <[email protected]> Co-authored-by: Alex | Interchain Labs <[email protected]> (cherry picked from commit 4027b8d)

mergify bot mentioned this pull request May 27, 2025

fix: properly store errors in Iterator.Next() method (backport #1090) #1099

Open

fix: properly store errors in Iterator.Next() method #1090

fix: properly store errors in Iterator.Next() method #1090

Uh oh!

Conversation

GarmashAlex commented Apr 29, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Suggested reviewers

Poem

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

Uh oh!

yihuang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GarmashAlex commented May 1, 2025

Uh oh!

GarmashAlex commented May 14, 2025

Uh oh!

aljo242 commented May 14, 2025

Uh oh!

GarmashAlex commented May 15, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

aljo242 commented May 27, 2025

Uh oh!

aljo242 commented May 27, 2025

Uh oh!

mergify bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

mergify bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

GarmashAlex commented May 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

GarmashAlex commented Apr 29, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Apr 29, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

yihuang left a comment •

edited

Loading

mergify bot commented May 27, 2025 •

edited

Loading

mergify bot commented May 27, 2025 •

edited

Loading