handle badger iterator panics on app stop #1355
Conversation
WalkthroughThe recent changes improve error handling in the BadgerDB usage within the Changes
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- pkg/lib/localstore/objectstore/badger_helpers.go (6 hunks)
- pkg/lib/localstore/objectstore/queries.go (1 hunks)
- pkg/lib/localstore/stores.go (1 hunks)
- util/persistentqueue/storage.go (1 hunks)
Additional context used
golangci-lint
pkg/lib/localstore/objectstore/queries.go
[error] 39-39: composites: github.com/anyproto/anytype-heart/pkg/lib/database.FilterAllIn struct literal uses unkeyed fields
(govet)
Additional comments not posted (5)
util/persistentqueue/storage.go (1)
37-42: LGTM!The deferred function to handle panics during iteration is correctly placed and will help in catching and reporting panics.
pkg/lib/localstore/objectstore/badger_helpers.go (2)
40-45: LGTM!The deferred function to handle panics during iteration is correctly placed and will help in catching and reporting panics.
81-86: LGTM!The deferred function to handle panics during iteration is correctly placed and will help in catching and reporting panics.
pkg/lib/localstore/stores.go (1)
241-245: LGTM!The deferred function to handle panics during iteration is correctly placed and will help in catching and reporting panics.
pkg/lib/localstore/objectstore/queries.go (1)
96-104: LGTM!The deferred function to handle panics during iteration is correctly placed and will help in catching and reporting panics.
| ) | ||
|
|
||
| err := s.db.View(func(txn *badger.Txn) error { | ||
| defer func() { |
There was a problem hiding this comment.
Use keyed fields in struct literals.
The github.com/anyproto/anytype-heart/pkg/lib/database.FilterAllIn struct literal uses unkeyed fields. This can lead to maintainability issues.
- optionFilter := database.FilterAllIn{relationKey, listValue.GetListValue()}
+ optionFilter := database.FilterAllIn{
+ Key: relationKey,
+ Value: listValue.GetListValue(),
+}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.
| defer func() { | |
| defer func() { | |
| optionFilter := database.FilterAllIn{ | |
| Key: relationKey, | |
| Value: listValue.GetListValue(), | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- pkg/lib/localstore/objectstore/badger_helpers.go (6 hunks)
- pkg/lib/localstore/objectstore/queries.go (1 hunks)
- pkg/lib/localstore/stores.go (1 hunks)
- util/persistentqueue/storage.go (1 hunks)
Additional context used
golangci-lint
pkg/lib/localstore/objectstore/queries.go
[error] 39-39: composites: github.com/anyproto/anytype-heart/pkg/lib/database.FilterAllIn struct literal uses unkeyed fields
(govet)
Additional comments not posted (5)
util/persistentqueue/storage.go (1)
37-42: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all calls to
Listhandle the potential error correctly.pkg/lib/localstore/objectstore/badger_helpers.go (2)
40-45: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all calls to
iterateKeysByPrefixBatchedTxhandle the potential error correctly.Verification successful
Proper Error Handling Verified
The
runFullTextIndexerfunction correctly handles the error returned byBatchProcessFullTextQueueby logging the error and returning early if an error occurs. This ensures that any issues encountered during the batch processing of full-text queues are appropriately managed.
- Location:
core/indexer/fulltext.go, lines 102-105Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `iterateKeysByPrefixBatchedTx` handle the potential error correctly. # Test: Search for the function usage. Expect: Proper error handling in all occurrences. rg --type go $'iterateKeysByPrefixBatchedTx'Length of output: 249
Script:
#!/bin/bash # Description: Inspect error handling at the call site of `iterateKeysByPrefixBatchedTx` within `pkg/lib/localstore/objectstore/badger_helpers.go`. # Extract the lines around the call to `iterateKeysByPrefixBatchedTx` to inspect error handling. rg --type go -C 5 'iterateKeysByPrefixBatchedTx' pkg/lib/localstore/objectstore/badger_helpers.goLength of output: 440
Script:
#!/bin/bash # Description: Search for the function that calls the outer function which calls `iterateKeysByPrefixBatchedTx` and inspect how its error is handled. # Search for the function that calls the outer function within the same file. rg --type go -C 5 'db.View' pkg/lib/localstore/objectstore/badger_helpers.goLength of output: 602
Script:
#!/bin/bash # Description: Search for the function that calls `iterateKeysByPrefixBatched` and inspect how its error is handled. # Search for the function that calls `iterateKeysByPrefixBatched` within the codebase. rg --type go -C 5 'iterateKeysByPrefixBatched'Length of output: 2603
Script:
#!/bin/bash # Description: Search for the function that calls `BatchProcessFullTextQueue` and inspect how its error is handled. # Search for the function that calls `BatchProcessFullTextQueue` within the codebase. rg --type go -C 5 'BatchProcessFullTextQueue'Length of output: 11162
Script:
#!/bin/bash # Description: Extract the lines around the call to `BatchProcessFullTextQueue` in `core/indexer/fulltext.go` to inspect how the error `err` is handled. # Extract the lines around the call to `BatchProcessFullTextQueue`. rg --type go -C 10 'BatchProcessFullTextQueue' core/indexer/fulltext.goLength of output: 768
Script:
#!/bin/bash # Description: Extract the complete `runFullTextIndexer` function from `core/indexer/fulltext.go` to inspect how the error `err` is handled. # Extract the complete `runFullTextIndexer` function. ast-grep --lang go --pattern $'func (i *indexer) runFullTextIndexer(ctx context.Context) { $$$ }' core/indexer/fulltext.goLength of output: 2437
81-86: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all calls to
iterateKeysByPrefixTxhandle the potential error correctly.Verification successful
Function usage verified and no issues found.
The calls to
iterateKeysByPrefixTxin the codebase handle the potential error correctly.
pkg/lib/localstore/objectstore/objects.go: The error is returned directly after the function call.pkg/lib/localstore/objectstore/delete.go: The error is checked immediately after the function call and handled within anifstatement.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `iterateKeysByPrefixTx` handle the potential error correctly. # Test: Search for the function usage. Expect: Proper error handling in all occurrences. rg --type go $'iterateKeysByPrefixTx'Length of output: 505
Script:
#!/bin/bash # Description: Inspect the code surrounding the calls to `iterateKeysByPrefixTx` in objects.go and delete.go # Check the surrounding code in objects.go echo "Checking objects.go:" rg --context 5 'iterateKeysByPrefixTx' pkg/lib/localstore/objectstore/objects.go # Check the surrounding code in delete.go echo "Checking delete.go:" rg --context 5 'iterateKeysByPrefixTx' pkg/lib/localstore/objectstore/delete.goLength of output: 954
pkg/lib/localstore/stores.go (1)
241-245: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all calls to
GetKeyshandle the potential error correctly.pkg/lib/localstore/objectstore/queries.go (1)
96-104: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all calls to
queryRawhandle the potential error correctly.Verification successful
Verification successful.
The errors from
queryRaware correctly handled in all instances.
getObjectsWithObjectInRelationerror is logged.QueryRawerrors are wrapped and returned in all instances.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `queryRaw` handle the potential error correctly. # Test: Search for the function usage. Expect: Proper error handling in all occurrences. rg --type go $'queryRaw'Length of output: 420
Script:
#!/bin/bash # Extract context around the lines where `queryRaw` is called to verify error handling. # Extract 5 lines before and after the return statements involving `queryRaw`. rg --type go -C 5 $'return s.queryRaw' pkg/lib/localstore/objectstore/queries.goLength of output: 1203
Script:
#!/bin/bash # Search for the usage of `getObjectsWithObjectInRelation` and `QueryRaw` to verify error handling. # Search for `getObjectsWithObjectInRelation` usage rg --type go $'getObjectsWithObjectInRelation' # Search for `QueryRaw` usage rg --type go $'QueryRaw'Length of output: 6218
Script:
#!/bin/bash # Extract context around the lines where `getObjectsWithObjectInRelation` and `QueryRaw` are called to verify error handling. # Extract 5 lines before and after the `getObjectsWithObjectInRelation` call. rg --type go -C 5 $'getObjectsWithObjectInRelation' pkg/lib/localstore/objectstore/queries.go # Extract 5 lines before and after the `QueryRaw` calls in various files. rg --type go -C 5 $'QueryRaw' pkg/lib/database/filter.go rg --type go -C 5 $'QueryRaw' core/subscription/service.go rg --type go -C 5 $'QueryRaw' core/kanban/group_tag.goLength of output: 2413
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- pkg/lib/localstore/objectstore/badger_helpers.go (6 hunks)
- pkg/lib/localstore/objectstore/queries.go (1 hunks)
- pkg/lib/localstore/stores.go (1 hunks)
- util/persistentqueue/storage.go (1 hunks)
| var err error | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = fmt.Errorf("badger iterator panic: %v", r) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Ensure err is returned if a panic occurs.
The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
+ return
}
}()Committable suggestion was skipped due to low confidence.
| var err error | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = fmt.Errorf("badger iterator panic: %v", r) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Ensure err is returned if a panic occurs in iterateKeysByPrefixBatchedTx.
The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
+ return
}
}()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.
| var err error | |
| defer func() { | |
| if r := recover(); r != nil { | |
| err = fmt.Errorf("badger iterator panic: %v", r) | |
| } | |
| }() | |
| var err error | |
| defer func() { | |
| if r := recover(); r != nil { | |
| err = fmt.Errorf("badger iterator panic: %v", r) | |
| return | |
| } | |
| }() |
| var err error | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = fmt.Errorf("badger iterator panic: %v", r) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Ensure err is returned if a panic occurs in iterateKeysByPrefixTx.
The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
+ return
}
}()Committable suggestion was skipped due to low confidence.
| defer func() { | ||
| if r := recover(); r != nil { | ||
| log.Errorf("badger iterator panic: %v", r) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Ensure the panic handling is effective in GetKeys.
The defer function correctly captures and logs the panic, but it would be better to also return an error if a panic is detected.
defer func() {
if r := recover(); r != nil {
log.Errorf("badger iterator panic: %v", r)
+ err = fmt.Errorf("badger iterator panic: %v", r)
+ return
}
}()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.
| defer func() { | |
| if r := recover(); r != nil { | |
| log.Errorf("badger iterator panic: %v", r) | |
| } | |
| }() | |
| defer func() { | |
| if r := recover(); r != nil { | |
| log.Errorf("badger iterator panic: %v", r) | |
| err = fmt.Errorf("badger iterator panic: %v", r) | |
| return | |
| } | |
| }() |
| err error | ||
| ) | ||
|
|
||
| err := s.db.View(func(txn *badger.Txn) error { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = fmt.Errorf("badger iterator panic: %v", r) | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
Ensure err is returned if a panic occurs in queryRaw.
The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
+ return
}
}()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.
| err error | |
| ) | |
| err := s.db.View(func(txn *badger.Txn) error { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| err = fmt.Errorf("badger iterator panic: %v", r) | |
| } | |
| }() | |
| err error | |
| ) | |
| defer func() { | |
| if r := recover(); r != nil { | |
| err = fmt.Errorf("badger iterator panic: %v", r) | |
| return | |
| } | |
| }() |
requilence
deleted the
go-3454-badger-do-not-panic-when-calling-closed-badger
branch
No description provided.