[CP-590] fix(store): gaskv iterator panic during first seek not releasing parent iterator #65
Conversation
WalkthroughAdded panic-safety to GStore.iterator by wrapping parent iteration with a defer-recover that closes the parent iterator on panic and re-panics with a joined error. Imported errors package to support errors.Join. No exported API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Consumer
participant G as GStore.iterator
participant P as Parent Iterator
Note over G: Setup defer-recover for panic
C->>G: Next()/Use iterator
G->>P: Iterate / consume gas
alt Normal flow
P-->>G: Items / EOF
G-->>C: Results
else Panic occurs
note over G: recover(p)
G->>P: Close()
alt Close returns error
note over G: Re-panic with errors.Join(p, closeErr)
else Close succeeds
note over G: Re-panic with original panic
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@kakysha your pull request is missing a changelog! |
![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 (1)
store/gaskv/store.go (1)
136-144: Optional: clarify recovery scope and add test coverage.This defer only guards panics occurring after parent is created (e.g., during consumeSeekGas). It won’t catch panics inside gs.parent.Iterator/ReverseIterator. That seems aligned with the PR intent, but please add a test that simulates a parent iterator that panics on Valid/Key/Value to assert Close() is invoked.
I can provide a minimal fake iterator and a test that ensures Close() is called on panic during the first seek. Want me to draft it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
store/gaskv/store.go(2 hunks)
⏰ 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). (13)
- GitHub Check: tests (01)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: tests (02)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-simapp
- GitHub Check: test-integration
- GitHub Check: test-e2e
- GitHub Check: liveness-test
- GitHub Check: build (arm64)
- GitHub Check: build (arm)
- GitHub Check: Gosec
- GitHub Check: Analyze
🔇 Additional comments (2)
store/gaskv/store.go (2)
4-4: Import looks good.Using the stdlib errors package is appropriate for errors.Join.
4-4: Verified Go version supports errors.Join.
The go.mod specifies Go version 1.23.5, which exceeds the minimum requirement of 1.20 for using errors.Join. No changes needed.
| // release parent open iterator if we panic during consumeSeekGas() and re-panic | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| if err := parent.Close(); err != nil { | ||
| r = errors.Join(r.(error), err) | ||
| } | ||
| panic(r) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Fix unsafe type assertion in panic path (can double-panic).
If the panic value r is not an error (common: string), r.(error) will itself panic, obscuring the original failure. Convert non-error panics before joining.
Apply this diff:
- if err := parent.Close(); err != nil {
- r = errors.Join(r.(error), err)
- }
+ if err := parent.Close(); err != nil {
+ if e, ok := r.(error); ok {
+ r = errors.Join(e, err)
+ } else {
+ // convert arbitrary panic payloads (e.g., strings) to error before join
+ r = errors.Join(fmt.Errorf("%v", r), err)
+ }
+ }And add the missing import:
import (
+ "fmt"
"errors"
"io"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // release parent open iterator if we panic during consumeSeekGas() and re-panic | |
| defer func() { | |
| if r := recover(); r != nil { | |
| if err := parent.Close(); err != nil { | |
| r = errors.Join(r.(error), err) | |
| } | |
| panic(r) | |
| } | |
| }() | |
| // release parent open iterator if we panic during consumeSeekGas() and re-panic | |
| defer func() { | |
| if r := recover(); r != nil { | |
| if err := parent.Close(); err != nil { | |
| if e, ok := r.(error); ok { | |
| r = errors.Join(e, err) | |
| } else { | |
| // convert arbitrary panic payloads (e.g., strings) to error before join | |
| r = errors.Join(fmt.Errorf("%v", r), err) | |
| } | |
| } | |
| panic(r) | |
| } | |
| }() |
🤖 Prompt for AI Agents
In store/gaskv/store.go around lines 136-144, the deferred panic handler
unsafely asserts r.(error) which can itself panic if r isn't an error; change it
to normalize r into an error before joining: if r is already an error use it,
otherwise convert with fmt.Errorf("%v", r), then if parent.Close() returns an
error use errors.Join(normalizedError, closeErr) and re-panic the joined error;
also add the required imports for "errors" and "fmt".
Summary by CodeRabbit
New Features
Bug Fixes