fix(baseapp): make BaseApp.Close idempotent - #26786
Open
pucedoteth wants to merge 1 commit into
Open
Conversation
`server/start.go` defers two cleanups that each call `app.Close()`: `startCmtNode`'s `cleanupFn` (which stops the CometBFT node and then closes the app) and `startApp`'s `appCleanupFn`. Defers run LIFO, so on the normal in-process shutdown path both fire and `Close` runs twice. `Close` closed `app.db` and `app.snapshotManager` unconditionally, so the second call re-closed an already-closed database. Backends differ in how they react: goleveldb returns an error, but pebble panics with `pebble: closed`. The result is that a pebble-backed node panics at the tail end of an otherwise clean stop and exits non-zero. Guard the shutdown with a `sync.Once` and replay the first call's result, so the underlying resources are only ever closed once no matter how many cleanup paths call `Close`. Fixes cosmos#26558 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
PR author is not in the allowed authors list. |
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
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.
Description
Closes #26558
server/start.godefers two cleanups that each callapp.Close():startCmtNodereturns acleanupFnthat stops the CometBFT node and then callsapp.Close()— deferred atserver/start.go:352startAppreturns anappCleanupFnthat shuts telemetry down and then callsapp.Close()— deferred atserver/start.go:252Defers run LIFO, so on the ordinary in-process shutdown path both fire and
Closeruns twice.Closeclosedapp.dbandapp.snapshotManagerunconditionally, so the second call re-closed an already-closed database.Backends disagree about what that means. goleveldb returns an error, which is merely logged. pebble panics with
pebble: closed, so a pebble-backed node panics at the tail end of an otherwise clean stop and exits non-zero.Fix
Guard the shutdown with a
sync.Onceand replay the first call's result. The underlying resources are closed once regardless of how many cleanup paths callClose, and a genuine close error is still surfaced to every caller rather than being swallowed.This is deliberately fixed in
BaseApp.Closerather than by deleting one of the twodefers:Closeis exported, and the SDK itself calls it from four separate places (server/start.golines 328, 424, 649, 830). Making the method idempotent fixes all of them, and any chain doing the same in its own start command.Testing
baseapp/close_test.gowraps adbm.DBso the secondClosepanics the way pebble does, and asserts the backend is only closed once:TestBaseAppCloseIsIdempotent— threeClosecalls, one backend close, no panicTestBaseAppCloseReplaysError— an error from the first close is still returned by later calls, not lostBoth fail on
main(panic: pebble: closed) and pass with this change. Full./baseapp/suite,go vet, andgofmtare clean.Author Checklist
!in the type prefix if API or client breaking changeCHANGELOG.md