diff --git a/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt b/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt index e9af88d80..f5c1722c7 100644 --- a/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt +++ b/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt @@ -32,6 +32,8 @@ import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import javax.inject.Inject import javax.inject.Singleton @@ -73,6 +75,14 @@ class AuthManager @Inject constructor( private var softLoginDisabled: Boolean = false + /** + * Serializes [init] so concurrent callers (app startup and the FCM push + * handler both call it when no account is established) can't launch two + * soft-logins at once — a second login re-inits the per-user database and + * would close the connection pool out from under active Room queries. + */ + private val initMutex = Mutex() + /** * Entropy for the account being switched to. Set before logout so App.kt's * auth guard can navigate to Login(entropy) instead of seedless Login(). @@ -93,18 +103,28 @@ class AuthManager @Inject constructor( fun init(onInitialized: () -> Unit = { }) { launch { - when (val result = credentialManager.lookup() - .also { taggedTrace("lookup result: ${it::class.simpleName}") }) { - is LookupResult.ExistingAccountFound -> { - val token = result.entropy - softLogin(token) - .onSuccess { onInitialized() } + initMutex.withLock { + // A concurrent init already logged us in (e.g. app startup won the + // race against an FCM push). Don't run a second soft-login that + // would re-init the database and close its connection pool mid-query. + if (userManager.authState is AuthState.LoggedIn) { + onInitialized() + return@withLock } - LookupResult.NoAccountFound -> Unit - is LookupResult.TemporaryAccountCreated -> { - userManager.establish(entropy = result.entropy) - userManager.set(AuthState.Onboarding(result.resumePoint)) + when (val result = credentialManager.lookup() + .also { taggedTrace("lookup result: ${it::class.simpleName}") }) { + is LookupResult.ExistingAccountFound -> { + val token = result.entropy + softLogin(token) + .onSuccess { onInitialized() } + } + + LookupResult.NoAccountFound -> Unit + is LookupResult.TemporaryAccountCreated -> { + userManager.establish(entropy = result.entropy) + userManager.set(AuthState.Onboarding(result.resumePoint)) + } } } } @@ -295,7 +315,12 @@ class AuthManager @Inject constructor( notificationManager.cancelAll() userManager.clear() tokenCoordinator.reset() - persistence.close() + // Intentionally do NOT close the Room DB here. Closing it while the just- + // dismissed Cash screen's stateIn(WhileSubscribed(5000)) flows are still + // active tears the connection pool out from under an in-flight Room query + // -> "SQLException: connection is closed" (SQLITE_MISUSE). The DB lifecycle + // is owned by FlipcashDatabase.init(): a same-user re-login reuses the open + // instance, and a different user's login rebuilds it for their entropy. featureFlags.reset() appSettings.reset() userFlags.clearAll() diff --git a/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt b/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt index d7411d366..9668edcef 100644 --- a/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt +++ b/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt @@ -3,6 +3,7 @@ package com.flipcash.app.auth import androidx.core.app.NotificationManagerCompat import com.flipcash.app.appsettings.AppSettingsCoordinator import com.flipcash.app.auth.internal.credentials.AccountMetadata +import com.flipcash.app.auth.internal.credentials.LookupResult import com.flipcash.app.auth.internal.credentials.PassphraseCredentialManager import com.flipcash.app.contacts.ContactCoordinator import com.flipcash.app.featureflags.FeatureFlagController @@ -146,7 +147,10 @@ class AuthManagerTest { verify { userManager.clear() } verify { notificationManager.cancelAll() } coVerify { tokenCoordinator.reset() } - verify { persistence.close() } + // The DB is intentionally left open on logout/reset — closing it races + // still-active Cash screen Room flows. FlipcashDatabase.init() owns the + // lifecycle (reuse on same-user re-login, rebuild for a different user). + verify(exactly = 0) { persistence.close() } verify { featureFlagController.reset() } verify { appSettings.reset() } verify { userFlags.clearAll() } @@ -206,7 +210,8 @@ class AuthManagerTest { verify { userManager.clear() } verify { notificationManager.cancelAll() } coVerify { tokenCoordinator.reset() } - verify { persistence.close() } + // DB stays open on logout — see resetStateForUser; init() owns lifecycle. + verify(exactly = 0) { persistence.close() } verify { featureFlagController.reset() } verify { appSettings.reset() } verify { userFlags.clearAll() } @@ -372,4 +377,35 @@ class AuthManagerTest { assertTrue(result.isSuccess) verify(exactly = 0) { userManager.set(AuthState.Ready) } } + + @Test + fun `init skips soft-login when already logged in`() = runTest { + // Simulates the FCM push handler calling init after app startup already + // authenticated: a second soft-login would re-init the DB and could close + // its connection pool out from under active Room queries. + every { userManager.authState } returns AuthState.Ready + + var initializedCalled = false + authManager.init { initializedCalled = true } + + assertTrue(initializedCalled) + coVerify(exactly = 0) { credentialManager.lookup() } + verify(exactly = 0) { persistence.openDatabase(any()) } + } + + @Test + fun `init soft-logs-in when no account is established`() = runTest { + val entropy = "dGVzdGVudHJvcHkxMjM0NQ==" + val accountMetadata: AccountMetadata = mockk(relaxed = true) + every { accountMetadata.id } returns listOf(1, 2, 3) + + every { userManager.authState } returns AuthState.Unknown + coEvery { credentialManager.lookup() } returns LookupResult.ExistingAccountFound(entropy) + coEvery { credentialManager.login(entropy, any()) } returns Result.success(accountMetadata) + + authManager.init() + + coVerify { credentialManager.lookup() } + verify { persistence.openDatabase(entropy) } + } } diff --git a/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/FlipcashDatabase.kt b/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/FlipcashDatabase.kt index 028c7ba48..1beb27ebd 100644 --- a/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/FlipcashDatabase.kt +++ b/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/FlipcashDatabase.kt @@ -167,11 +167,24 @@ abstract class FlipcashDatabase : RoomDatabase() { fun isOpen() = instance?.isOpen == true + @Synchronized fun init(context: Context, entropyB64: String) { val dbUniqueName = Base58.encode(entropyB64.toByteArray().subByteArray(0, 6)) + val targetName = "$dbNamePrefix-$dbUniqueName.db" + + // Same-user DB already built: don't tear down the existing instance. + // A concurrent or soft re-login (e.g. app startup racing an FCM push + // that also calls AuthManager.init) must not close the DB out from + // under in-flight Room queries such as CashScreenViewModel's balance + // Flow — doing so throws "connection pool has been closed". Key on the + // DB name, not isOpen(): Room opens the file lazily, so the instance + // can be live-but-not-yet-open in exactly this racy window. + // closeDb() nulls `instance`, so a non-null instance here is current. + if (instance != null && dbName == targetName) return + trace("database init start $dbUniqueName", type = TraceType.Process) instance?.close() - dbName = "$dbNamePrefix-$dbUniqueName.db" + dbName = targetName instance = Room.databaseBuilder(context, FlipcashDatabase::class.java, dbName) diff --git a/apps/flipcash/shared/persistence/db/src/test/kotlin/com/flipcash/app/persistence/FlipcashDatabaseInitTest.kt b/apps/flipcash/shared/persistence/db/src/test/kotlin/com/flipcash/app/persistence/FlipcashDatabaseInitTest.kt new file mode 100644 index 000000000..ff8804718 --- /dev/null +++ b/apps/flipcash/shared/persistence/db/src/test/kotlin/com/flipcash/app/persistence/FlipcashDatabaseInitTest.kt @@ -0,0 +1,57 @@ +package com.flipcash.app.persistence + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import kotlin.test.assertNotSame +import kotlin.test.assertSame +import kotlin.test.assertTrue + +@RunWith(RobolectricTestRunner::class) +class FlipcashDatabaseInitTest { + + private val context = ApplicationProvider.getApplicationContext() + + @After + fun tearDown() { + FlipcashDatabase.closeDb() + } + + @Test + fun `re-init with same entropy keeps the same open instance`() { + FlipcashDatabase.init(context, ENTROPY_A) + val first = FlipcashDatabase.requireInstance() + // Force the (lazily-opened) connection pool open, as an active Room query would. + first.openHelper.writableDatabase + assertTrue(first.isOpen) + + // A soft re-login (or a concurrent AuthManager.init) must not tear down + // the live connection pool that active Room queries depend on. + FlipcashDatabase.init(context, ENTROPY_A) + val second = FlipcashDatabase.requireInstance() + + assertSame(first, second) + assertTrue(first.isOpen) + } + + @Test + fun `init with different entropy rebuilds the database`() { + FlipcashDatabase.init(context, ENTROPY_A) + val first = FlipcashDatabase.requireInstance() + + FlipcashDatabase.init(context, ENTROPY_B) + val second = FlipcashDatabase.requireInstance() + + assertNotSame(first, second) + second.openHelper.writableDatabase + assertTrue(second.isOpen) + } + + private companion object { + const val ENTROPY_A = "aaaaaaaaaaaaaaaaaaaaaaaa" + const val ENTROPY_B = "bbbbbbbbbbbbbbbbbbbbbbbb" + } +} diff --git a/apps/flipcash/shared/persistence/provider/build.gradle.kts b/apps/flipcash/shared/persistence/provider/build.gradle.kts index fa7ef1d75..7f3b34024 100644 --- a/apps/flipcash/shared/persistence/provider/build.gradle.kts +++ b/apps/flipcash/shared/persistence/provider/build.gradle.kts @@ -15,4 +15,8 @@ android { dependencies { implementation(project(":apps:flipcash:shared:persistence:db")) + + testImplementation(kotlin("test")) + testImplementation(libs.bundles.unit.testing) + testImplementation(libs.robolectric) } diff --git a/apps/flipcash/shared/persistence/provider/src/main/kotlin/com/flipcash/app/persistence/PersistenceProvider.kt b/apps/flipcash/shared/persistence/provider/src/main/kotlin/com/flipcash/app/persistence/PersistenceProvider.kt index 4e9145489..19f582f47 100644 --- a/apps/flipcash/shared/persistence/provider/src/main/kotlin/com/flipcash/app/persistence/PersistenceProvider.kt +++ b/apps/flipcash/shared/persistence/provider/src/main/kotlin/com/flipcash/app/persistence/PersistenceProvider.kt @@ -10,7 +10,12 @@ class PersistenceProvider @Inject constructor( @ApplicationContext private val context: Context ) { fun openDatabase(entropy: String) { - if (FlipcashDatabase.isOpen()) return + // Always delegate to init(): it is idempotent (no-op when this user's DB + // is already open) and rebuilds when the entropy/DB name differs. We must + // NOT short-circuit on isOpen() — after logout we intentionally leave the + // DB open (see AuthManager.resetStateForUser), so a *different* user + // signing in has to reach init() to swap to their own database. Guarding + // on isOpen() here would hand the new user the previous user's data. FlipcashDatabase.init(context, entropy) } diff --git a/apps/flipcash/shared/persistence/provider/src/test/kotlin/com/flipcash/app/persistence/PersistenceProviderTest.kt b/apps/flipcash/shared/persistence/provider/src/test/kotlin/com/flipcash/app/persistence/PersistenceProviderTest.kt new file mode 100644 index 000000000..f20117aed --- /dev/null +++ b/apps/flipcash/shared/persistence/provider/src/test/kotlin/com/flipcash/app/persistence/PersistenceProviderTest.kt @@ -0,0 +1,51 @@ +package com.flipcash.app.persistence + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import kotlin.test.assertNotSame +import kotlin.test.assertSame + +@RunWith(RobolectricTestRunner::class) +class PersistenceProviderTest { + + private val context = ApplicationProvider.getApplicationContext() + private val provider = PersistenceProvider(context) + + @After + fun tearDown() { + FlipcashDatabase.closeDb() + } + + @Test + fun `openDatabase for the same user reuses the open instance`() { + provider.openDatabase(ENTROPY_A) + val first = FlipcashDatabase.requireInstance() + + provider.openDatabase(ENTROPY_A) + + assertSame(first, FlipcashDatabase.requireInstance()) + } + + @Test + fun `openDatabase for a different user swaps the DB even while one is open`() { + // Regression: after logout the DB is intentionally left open, so + // openDatabase must NOT short-circuit on isOpen() — a different user + // signing in has to reach init() and get their own database, not the + // previous user's. + provider.openDatabase(ENTROPY_A) + val first = FlipcashDatabase.requireInstance() + + provider.openDatabase(ENTROPY_B) + + assertNotSame(first, FlipcashDatabase.requireInstance()) + } + + private companion object { + const val ENTROPY_A = "aaaaaaaaaaaaaaaaaaaaaaaa" + const val ENTROPY_B = "bbbbbbbbbbbbbbbbbbbbbbbb" + } +}