fix(server/v2/cometbft): wire app closer #22240
Conversation
github-actions
bot
added
C:server/v2
julienrbrt
requested review from
kocubinski,
a team,
facundomedica,
sontrinh16 and
hieuvubk
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce a new function pointer Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
julienrbrt
changed the title
|
@julienrbrt your pull request is missing a changelog! |
julienrbrt
added
the
backport/v0.52.x
![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
🧹 Outside diff range and nitpick comments (5)
simapp/v2/app_di.go (1)
221-224: Consider the implications of exposing internal state.The
GetStoremethod provides direct access to thestorefield. While this can be useful for testing or debugging, it may violate encapsulation principles. Ensure that this exposure is necessary and doesn't lead to unexpected modifications of the store outside theSimAppstruct.Consider if there are more specific, controlled ways to provide necessary access to store functionality without exposing the entire
RootStore.server/v2/cometbft/server.go (1)
BootstrapStateCmd lacks documentation and tests.
While the
BootstrapStateCmdis properly implemented and integrated into the CLI, it currently lacks necessary documentation and corresponding tests. To ensure maintainability and reliability, please add comprehensive documentation and relevant tests forBootstrapStateCmd.🔗 Analysis chain
Line range hint
271-271: LGTM. Verify implementation and documentation ofBootstrapStateCmd.The addition of
s.BootstrapStateCmd()to the CLI commands expands the server's functionality, likely providing a way to bootstrap the application state.Please ensure that the
BootstrapStateCmdis properly implemented and documented. Run the following script to verify:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and documentation of BootstrapStateCmd # Test 1: Check for the implementation of BootstrapStateCmd echo "Checking BootstrapStateCmd implementation:" ast-grep --lang go --pattern 'func ($_) BootstrapStateCmd() *cobra.Command { $$$ }' # Test 2: Check for documentation of BootstrapStateCmd echo "Checking BootstrapStateCmd documentation:" rg --type go 'BootstrapStateCmd' -B 5 # Test 3: Check for usage examples or tests of BootstrapStateCmd echo "Checking for usage examples or tests of BootstrapStateCmd:" rg --type go 'BootstrapStateCmd'Length of output: 3639
server/v2/cometbft/abci_test.go (1)
675-675: Changes look good, but consider improving the new parameter.The modifications to use
appmanager.Builderand update theNewConsensusfunction call are correct and align with the import changes. However, the new parameterfunc() error { return nil }seems to be a placeholder.Consider replacing the placeholder with a more meaningful function or add a comment explaining its purpose. For example:
func mockAppCloser() error { // This is a placeholder for application closing logic in tests return nil } // Then use mockAppCloser in the NewConsensus callThis would improve code readability and make the intention clearer.
Also applies to: 691-691
server/v2/cometbft/abci.go (2)
46-46: LGTM! Consider adding documentation for the new field.The addition of the
appCloserfield to theConsensusstruct is a good way to manage the application's lifecycle. It follows Go naming conventions and is appropriately unexported.Consider adding a brief comment explaining the purpose of this field, e.g.:
// appCloser is a function used to close the application gracefully appCloser func() error
81-81: LGTM! Consider refactoring for improved readability.The addition of the
appCloserparameter to theNewConsensusfunction and its initialization in theConsensusstruct are correct and consistent with the struct modification.The function signature is becoming quite long. Consider using a configuration struct to group related parameters, which could improve readability and make future additions easier. For example:
type ConsensusConfig[T transaction.Tx] struct { Logger log.Logger AppName string App *appmanager.AppManager[T] AppCloser func() error Mempool mempool.Mempool[T] IndexedEvents map[string]struct{} QueryHandlersMap map[string]appmodulev2.Handler Store types.Store Config Config TxCodec transaction.Codec[T] ChainID string } func NewConsensus[T transaction.Tx](cfg ConsensusConfig[T]) *Consensus[T] { // Use cfg fields to initialize the Consensus struct }This approach would make the function signature more concise and easier to maintain as the number of parameters grows.
Also applies to: 94-94
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- server/v2/cometbft/abci.go (3 hunks)
- server/v2/cometbft/abci_test.go (3 hunks)
- server/v2/cometbft/server.go (1 hunks)
- server/v2/types.go (1 hunks)
- simapp/v2/app_di.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
server/v2/types.go (1)
24-24: LGTM: New method adheres to Go conventions.The addition of the
Close() errormethod to theAppI[T]interface is consistent with Go naming conventions and the Uber Go Style Guide. It follows the common pattern for cleanup methods in Go.However, consider the following suggestions:
- Add a comment describing the purpose and expected behavior of the
Closemethod.- Ensure all existing implementations of
AppI[T]are updated to include this new method.Would you like me to propose a comment for the
Closemethod?To ensure all implementations are updated, run:
simapp/v2/app_di.go (1)
226-233: LGTM! Good error handling and resource management.The
Closemethod effectively overrides the base method to include closing the store. It demonstrates good practices:
- Proper error handling by checking and returning the error from
store.Close().- Ensuring the store is closed before calling the base
Closemethod.- Following the Uber Go Style Guide for error handling and method structure.
This implementation ensures proper resource management during the application shutdown process.
server/v2/cometbft/abci_test.go (2)
22-22: LGTM: Import statement addition is appropriate.The addition of the
appmanagerimport aligns with the changes made in thesetUpConsensusfunction. This change is necessary and correctly implemented.
22-22: Overall changes are well-contained and don't affect existing tests.The modifications to the import statements and the
setUpConsensusfunction are well-contained and don't impact the existing test functions. This is a positive aspect of the changes as it minimizes the risk of unintended side effects.To ensure that the changes don't have any unforeseen impacts, you may want to run the following command to check for any other occurrences of
am.Builderthat might need updating:This will help verify that all instances have been updated consistently.
Also applies to: 675-675, 691-691
✅ Verification successful
Further verification needed to ensure all instances of
am.Builderare addressed.The previous search did not yield any results, which could mean that
am.Builderis no longer used or that the search command had issues. To confirm, please run the following command:
All instances of
am.Builderhave been successfully removed.No remaining occurrences of
am.Builderwere found in the codebase, ensuring that the changes are well-contained and do not impact other parts of the code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg "am\.Builder" --type goLength of output: 28
Script:
# Corrected command to search for 'am.Builder' in Go files rg "am\\.Builder" --type goLength of output: 28
| @@ -107,6 +107,7 @@ func (s *CometBFTServer[T]) Init(appI serverv2.AppI[T], cfg map[string]any, logg | |||
| s.logger, | |||
| appI.Name(), | |||
| appI.GetAppManager(), | |||
| appI.Close, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Detected in NewConsensus Function Signature
The NewConsensus function is being called with an additional appI.Close parameter, but its signature does not reflect this change.
- File: server/v2/cometbft/server.go
- Issue:
NewConsensusfunction signature missing theappI.Closeparameter.
🔗 Analysis chain
LGTM. Verify consistency of appI.Close usage.
The addition of appI.Close to the NewConsensus function call enhances the application's lifecycle management. This change allows for graceful shutdown of the application.
Please ensure that this change is consistent with the NewConsensus function signature and its usage across the codebase. Run the following script to verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of NewConsensus function and appI.Close method
# Test 1: Check the NewConsensus function signature
echo "Checking NewConsensus function signature:"
ast-grep --lang go --pattern 'func NewConsensus($_) $_'
# Test 2: Check for other occurrences of NewConsensus function calls
echo "Checking other NewConsensus function calls:"
rg --type go 'NewConsensus\(' -A 10 -B 2
# Test 3: Check for the definition and usage of Close method in AppI interface
echo "Checking AppI interface for Close method:"
ast-grep --lang go --pattern 'type AppI[T $_] interface {
$$$
Close() $_
$$$
}'
Length of output: 1613
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
store/v2/commitment/store.go (1)
504-505: LGTM with suggestion: UpdatedClosemethodThe addition of
c.db.Close()in theClosemethod is a good improvement for proper resource management. However, consider enhancing the error handling to ensure all resources are closed even if an error occurs.Consider updating the
Closemethod to ensure all resources are closed and errors are properly aggregated:func (c *CommitStore) Close() error { + var errs []error for _, tree := range c.multiTrees { if err := tree.Close(); err != nil { - return err + errs = append(errs, err) } } - return c.db.Close() + if err := c.db.Close(); err != nil { + errs = append(errs, err) + } + return errors.Join(errs...) }This change ensures that all trees and the database are closed, and all errors are reported.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- server/v2/testdata/app.toml (1 hunks)
- store/v2/commitment/metadata.go (1 hunks)
- store/v2/commitment/store.go (3 hunks)
- store/v2/root/factory.go (1 hunks)
- tools/confix/data/v2-app.toml (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- server/v2/testdata/app.toml
- store/v2/commitment/metadata.go
- store/v2/root/factory.go
- tools/confix/data/v2-app.toml
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/commitment/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
store/v2/commitment/store.go (2)
41-41: LGTM: Addition ofdbfield toCommitStorestructThe new
dbfield of typecorestore.KVStoreWithBatchis a good addition to theCommitStorestruct. It provides a clear way to manage the database instance and ensures proper resource management.
50-53: LGTM: UpdatedNewCommitStorefunctionThe
NewCommitStorefunction has been correctly updated to include thedbparameter and initialize it in the returnedCommitStoreinstance. This change is consistent with the struct modification and follows good Go practices.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
store/v2/root/upgrade_test.go (1)
Line range hint
1-168: Summary: Consistent changes torootStoreinitialization. Ensure comprehensive testing.The changes in this file are focused and consistent, modifying the
rootStoreinitialization in both theSetupTestandloadWithUpgradesmethods. While these changes are minimal, they could significantly impact test behavior by altering how therootStoreinteracts with the database.To ensure the integrity of the test suite:
- Run the entire test suite for the
store/v2/rootpackage to catch any potential regressions.- Consider adding specific tests to verify the behavior of the
rootStorewith the new initialization parameters.- If possible, conduct integration tests to ensure these changes don't affect the broader system behavior.
#!/bin/bash # Run all tests in the store/v2/root package go test -v ./store/v2/root/...store/v2/root/migrate_test.go (1)
83-83: LGTM with a minor suggestion for clarity.The change correctly adds an in-memory database as the first argument to the
Newfunction, which is appropriate for testing purposes. This modification aligns with an updatedNewfunction signature in therootpackage.For improved clarity, consider naming the new database instance:
- s.rootStore, err = New(dbm.NewMemDB(), testLog, ss, orgSC, pm, migrationManager, nil) + memDB := dbm.NewMemDB() + s.rootStore, err = New(memDB, testLog, ss, orgSC, pm, migrationManager, nil)This change would make it more explicit that we're using an in-memory database for testing.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- store/v2/root/factory.go (2 hunks)
- store/v2/root/migrate_test.go (1 hunks)
- store/v2/root/store.go (5 hunks)
- store/v2/root/store_test.go (3 hunks)
- store/v2/root/upgrade_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- store/v2/root/factory.go
🧰 Additional context used
📓 Path-based instructions (4)
store/v2/root/migrate_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"store/v2/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/store_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"store/v2/root/upgrade_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (9)
store/v2/root/upgrade_test.go (2)
95-95: LGTM. Consistent with previous change.This modification aligns with the earlier change in the
SetupTestmethod, maintaining consistency in how therootStoreis initialized throughout the test suite. The use ofs.commitDBinstead oftestLogas the first argument in theNewfunction call is appropriate.
53-53: LGTM. Verify the impact of this change on test behavior.The modification to use
s.commitDBinstead oftestLogas the first argument in theNewfunction call appears intentional and aligns with the PR objectives. This change may alter how therootStoreinteracts with the database during tests.To ensure this change doesn't introduce unexpected behavior, please verify:
- All tests in this file still pass.
- The
rootStoreinitialization behaves as expected with this new parameter.store/v2/root/migrate_test.go (1)
Line range hint
1-180: Comprehensive test coverage for store migration.The test file provides thorough coverage of the store migration process:
- It sets up both original and new stores.
- Tests querying during different stages of migration.
- Simulates applying changesets before, during, and after migration.
- Includes checks for version consistency and data integrity.
The use of delays to simulate consensus processes adds realism to the test scenario. The comprehensive coverage across different versions and store keys ensures robustness of the migration process.
Great job on maintaining such a detailed test suite!
store/v2/root/store.go (4)
8-8: LGTM: Import addition is appropriate.The addition of the "io" import is necessary and consistent with the new
dbCloserfield in theStorestruct.
37-39: LGTM: New field addition is appropriate and well-documented.The addition of the
dbCloser io.Closerfield to theStorestruct is a good practice for managing the database instance's lifecycle. The comment above the field clearly explains its purpose.
75-75: LGTM: Function signature update and field initialization are correct.The
Newfunction has been appropriately updated to include thedbCloserparameter, and thedbCloserfield is correctly initialized in the returnedStoreinstance.Also applies to: 84-84
101-101: LGTM: Proper closure of dbCloser added.The
Closemethod has been correctly updated to include the closure of thedbCloser. The use oferrors.Joinfor error handling is a good practice for aggregating multiple errors.store/v2/root/store_test.go (2)
96-96:⚠️ Potential issueConsistent change across methods, but overall impact on tests needs clarification.
This change in
newStoreWithBackendMountis consistent with the modifications inSetupTestandnewStoreWithPruneConfig. However, replacingnoopLogwithdbm.NewMemDB()across all these methods raises concerns about the overall impact on the test suite.
- It changes the type of the first argument from a logger to a database in all
Newfunction calls.- This modification could potentially alter the behavior of various tests, including those for pruning and custom backend mounts.
- The rationale behind using a database where a logger was previously used is unclear and needs explanation.
Please provide a comprehensive explanation of why this change was made across all methods and how it affects the test suite's behavior and reliability. Additionally, ensure that these changes align with any updates made to the
Newfunction in the main implementation.To verify the impact of these changes on the test suite, we can check for any failing tests:
#!/bin/bash # Description: Run the tests in the package and check for any failures go test ./store/v2/root/... -v
62-62:⚠️ Potential issueClarify the rationale for replacing logger with in-memory database.
The change from
noopLogtodbm.NewMemDB()as the first argument in theNewfunction call is unexpected. These serve different purposes - a logger for logging and a database for data storage. This modification could significantly alter the test setup and potentially the behavior of theRootStorein tests. Could you please explain the reasoning behind this change and confirm if this is intentional?To verify the impact of this change, we can check if the
Newfunction signature has been updated in the main implementation:
| @@ -84,7 +84,7 @@ func (s *RootStoreTestSuite) newStoreWithPruneConfig(config *store.PruningOption | |||
|
|
|||
| pm := pruning.NewManager(sc, ss, config, config) | |||
|
|
|||
| rs, err := New(noopLog, ss, sc, pm, nil, nil) | |||
| rs, err := New(dbm.NewMemDB(), noopLog, ss, sc, pm, nil, nil) | |||
There was a problem hiding this comment.
Consistent change, but concerns remain about logger replacement.
This change is consistent with the modification in SetupTest, replacing noopLog with dbm.NewMemDB(). However, the same concerns apply here. Replacing a logger with an in-memory database in the New function call could potentially alter the behavior of pruning tests. Please confirm if this change is intentional and explain how it affects the test setup for pruning configurations.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Improvements
Bug Fixes