fix(auth): prevent DB connection-pool-closed crash on concurrent login#1071
Merged
Conversation
A cold start with degraded network could race two soft-logins: app startup (FlipcashApp.init) and an FCM push handler (NotificationService) both call AuthManager.init() when no account cluster is established yet. The second login re-runs FlipcashDatabase.init(), which unconditionally closed and rebuilt the per-user database — tearing down the connection pool out from under an active Room query (e.g. CashScreenViewModel's balance Flow) and throwing: IllegalStateException: Cannot perform this operation because the connection pool has been closed. Fixes: - FlipcashDatabase.init() is now @synchronized and idempotent: it early-returns when an instance for the same DB already exists. Keyed on the DB name (not isOpen(), which is false while Room lazily opens the file — exactly the race window). Account-switch still rebuilds. - AuthManager.init() is serialized with a Mutex and short-circuits when already LoggedIn, so a second concurrent soft-login can't churn the DB lifecycle. Adds FlipcashDatabaseInitTest and AuthManager init-guard tests.
A second connection-closed crash was reported (SQLException code 21 "connection is closed") from CashScreenViewModel's Room flow — same family as the connection-pool crash this branch fixes, but a different trigger: logout instead of login re-init. On logout, resetStateForUser() called persistence.close() while the just-dismissed Cash screen's stateIn(WhileSubscribed(5000)) flows were still active (up to 5s), so an in-flight Room query hit the closed connection and threw. Now that FlipcashDatabase.init() is idempotent + @synchronized, it owns the DB lifecycle end to end, so: - resetStateForUser() no longer closes the DB. A same-user re-login reuses the open instance; a different user's login rebuilds it. - PersistenceProvider.openDatabase() no longer short-circuits on isOpen(): with the DB now left open after logout, that guard would hand a different user the previous user's database. It always delegates to the (idempotent, name-keyed) init() instead. Adds PersistenceProviderTest covering same-user reuse and different-user swap-while-open (account isolation). Updates AuthManagerTest to assert the DB is not closed on logout.
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.
Summary
Fixes a rare crash:
IllegalStateException: Cannot perform this operation because the connection pool has been closed.(Bugsnag 6a51b2b4)How it happened
A cold start on degraded network races two soft-logins:
FlipcashApp→AuthManager.init()begins soft-login fix: reset sheet state on collapse #1.NotificationService.onMessageReceived→authenticateIfNeeded { ... }callsAuthManager.init()again becauseuserManager.accountClusteris stillnull(login fix: reset sheet state on collapse #1 hadn't established it yet).Login #2 re-runs
FlipcashDatabase.init(), which unconditionallyclose()d and rebuilt the per-user DB — tearing down the connection pool out from under an active Room query (CashScreenViewModel's balanceFlow, already collecting once login #1 reachedReady).It's rare because it needs a cold start on slow network + a push landing in a ~150ms window + the Cash Flow already collecting.
Changes
FlipcashDatabase.init()— now@Synchronizedand idempotent: early-returns when an instance for the same DB already exists. Keyed on the DB name, notisOpen()— Room opens the file lazily, soisOpen()isfalsein exactly the race window (this is whyPersistenceProvider.openDatabase's existingisOpen()guard didn't catch it). Account-switch (different entropy) still closes + rebuilds.AuthManager.init()— serialized with aMutexand short-circuits when alreadyLoggedIn, so the push handler can't launch a second concurrent soft-login.Tests
FlipcashDatabaseInitTest(new): same entropy → same instance kept alive; different entropy → rebuild.AuthManagerTest(+2): already-LoggedIn→ nolookup/openDatabase; unauthenticated → soft-login proceeds.Both affected suites pass locally.
Risk
Low. Account-switch still rebuilds the DB. The auth guard only skips a redundant re-login when already logged in; explicit
login()(seed input) andlogoutAndSwitchAccountare unaffected.Update — also fixes the logout variant (SQLITE_MISUSE, Bugsnag
6a52e88b)A second report of the same defect family surfaced —
android.database.SQLException: Error code: 21, message: connection is closed— from the same culprit (CashScreenViewModel's Room flow) but a different trigger: logout instead of login re-init.On logout,
AuthManager.resetStateForUser()calledpersistence.close()while the just-dismissed Cash screen'sstateIn(WhileSubscribed(5000))flows were still active (up to 5s), so an in-flight Room query hit the closed connection.Now that
FlipcashDatabase.init()is idempotent +@Synchronized, it owns the DB lifecycle, so this commit:resetStateForUser()no longer closes the DB — a same-user re-login reuses the open instance; a different user's login rebuilds it.PersistenceProvider.openDatabase()no longer short-circuits onisOpen(). With the DB now left open after logout, that guard would hand a different user the previous user's database — so it always delegates to the idempotent, name-keyedinit().Adds
PersistenceProviderTest(same-user reuse + different-user swap-while-open / account isolation); updatesAuthManagerTestto assert the DB is not closed on logout.