From 6514564df67dbe65104b3a25dd4f73da08bc0453 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 21 Sep 2026 06:42:20 +1000 Subject: [PATCH 1/7] Show the database error screen when the keystore secret can't be unsealed (#2213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit KeyStoreHelper.unseal rethrows every crypto failure as AssertionError, and the database secret was only ever dereferenced from the openHelper lazy, outside any handler. The process died on every launch instead of reaching the error screen that already exists. Resolve the secret inside the migration flow and catch Throwable so the failure becomes MigrationState.Error, and end openHelper's wait on Error as well as Completed — waiting for Completed alone would have hung every database caller once that state was reachable. "Clear Device and Restore" is now hidden when there is no login state, because it restores from the in-memory state and would otherwise silently behave as "clear and restart" after the user accepted a warning promising an account recovery. Keystore failures now log the KeyStoreException error code, which is the only thing distinguishing a transient fault, where the data is intact and a retry may succeed, from a key that can no longer decrypt what it sealed. --- .../migration/DatabaseMigrationManager.kt | 61 ++++++++++++++++--- .../migration/DatabaseMigrationScreen.kt | 21 +++++-- .../DatabaseMigrationStateActivity.kt | 5 ++ 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt index 386ae2ba9f..1989120745 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt @@ -1,7 +1,9 @@ package org.thoughtcrime.securesms.migration import android.app.Application +import android.os.Build import android.os.SystemClock +import androidx.annotation.RequiresApi import androidx.annotation.StringRes import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -43,12 +45,21 @@ class DatabaseMigrationManager @Inject constructor( // First perform a cheap check to see if the migration is done, if so we can skip the wait. if (mutableMigrationState.value != MigrationState.Completed) { - // Wait until the migration is done. This is a semi-expensive call but it's necessary - // to block the callers from accessing the database until we have sorted out the migration - // process. Note that we don't pass in errors here as the callers of this function - // don't expect the exceptions at all so we have no choice but to block them. - runBlocking { - migrationState.first { it == MigrationState.Completed } + // Blocking is deliberate: callers don't expect to handle a half-migrated database, so + // they wait here instead. Error has to end the wait as well as Completed, because the + // state only leaves Error on an explicit retry and waiting for Completed alone hangs + // every caller until the process dies. + val finalState = runBlocking { + migrationState.first { + it == MigrationState.Completed || it is MigrationState.Error + } + } + + if (finalState is MigrationState.Error) { + throw IllegalStateException( + "Database unavailable: migration failed", + finalState.throwable + ) } } @@ -90,6 +101,12 @@ class DatabaseMigrationManager @Inject constructor( mutableMigrationState.value = MigrationState.Migrating(steps.toList()) try { + // Resolving the secret here is what puts a failed KeyStoreHelper.unseal into + // MigrationState.Error, where the user is offered retry and log export. Its only other + // dereference is in openHelper, which sits outside every handler on this path, so left + // to happen there the AssertionError kills the process on every launch instead (#2213). + dbSecret + for ((index, desc) in stepDescriptors.withIndex()) { Log.d(TAG, "Starting migration step: ${desc.name}") val stepStartedAt = SystemClock.elapsedRealtime() @@ -107,12 +124,42 @@ class DatabaseMigrationManager @Inject constructor( } mutableMigrationState.value = MigrationState.Completed - } catch (ec: Exception) { + } catch (ec: Throwable) { + // Throwable, not Exception: KeyStoreHelper reports every crypto failure as an + // AssertionError, which is an Error and would otherwise pass straight through here. + logKeyStoreFailure(ec) mutableMigrationState.value = MigrationState.Error(ec) return } } + /** + * Records the keystore error code behind a crypto failure, if there is one. + * + * The code distinguishes a transient keystore fault, where the data is intact and a retry may + * succeed, from a key that can no longer decrypt what it sealed. Nothing else in the crash + * reaching us carries that distinction. + */ + private fun logKeyStoreFailure(error: Throwable) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + logKeyStoreExceptionDetails(error) + } + } + + @RequiresApi(Build.VERSION_CODES.TIRAMISU) + private fun logKeyStoreExceptionDetails(error: Throwable) { + val keyStoreError = generateSequence(error) { it.cause } + .filterIsInstance() + .firstOrNull() ?: return + + Log.w( + TAG, + "Keystore failure: code=${keyStoreError.numericErrorCode}, " + + "transient=${keyStoreError.isTransientFailure}, " + + "systemError=${keyStoreError.isSystemError}" + ) + } + private fun migrateCipherSettings(fromRetry: Boolean) { if (prefs.migratedToDisablingKDF) { Log.i(TAG, "Already migrated to latest cipher settings") diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt index ceff850492..5c31039b7e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationScreen.kt @@ -56,12 +56,14 @@ fun DatabaseMigrationScreen( clearDataUtils: ClearDataUtils, exporter: LogExporter, fm: FragmentManager, + canRestoreAccount: Boolean, ) { val scope = rememberCoroutineScope() var showExportLogDialog by retain { mutableStateOf(false) } DatabaseMigration( state = migrationManager.migrationState.collectAsState().value, + canRestoreAccount = canRestoreAccount, onRetry = { migrationManager.requestMigration(fromRetry = true) }, @@ -95,6 +97,7 @@ fun DatabaseMigrationScreen( private fun DatabaseMigration( @PreviewParameter(DatabaseMigrationStateProvider::class) state: DatabaseMigrationManager.MigrationState, + canRestoreAccount: Boolean = true, onRetry: () -> Unit = {}, onExportLogs: () -> Unit = {}, onClearData: () -> Unit = {}, @@ -164,12 +167,18 @@ private fun DatabaseMigration( Spacer(Modifier.size(LocalDimensions.current.mediumSpacing)) - OutlineButton( - text = stringResource(R.string.clearDeviceRestore), - color = LocalColors.current.danger, - onClick = { showingClearDeviceRestoreWarning = true } - ) - Spacer(Modifier.size(LocalDimensions.current.xsSpacing)) + // Restoring replays the account from the network using the login state + // held in memory, so with no login state there is nothing to restore with + // and this would silently do what "clear and restart" does. Offering it + // then would promise an account recovery that cannot happen. + if (canRestoreAccount) { + OutlineButton( + text = stringResource(R.string.clearDeviceRestore), + color = LocalColors.current.danger, + onClick = { showingClearDeviceRestoreWarning = true } + ) + Spacer(Modifier.size(LocalDimensions.current.xsSpacing)) + } OutlineButton( text = stringResource(R.string.clearDeviceRestart), color = LocalColors.current.danger, diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt index adcb7c86f4..416c9dbf9f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationStateActivity.kt @@ -7,6 +7,7 @@ import androidx.compose.runtime.collectAsState import androidx.core.content.IntentCompat import dagger.hilt.android.AndroidEntryPoint import org.thoughtcrime.securesms.FullComposeActivity +import org.thoughtcrime.securesms.auth.LoginStateRepository import org.thoughtcrime.securesms.ui.components.LogExporter import org.thoughtcrime.securesms.util.ClearDataUtils import javax.inject.Inject @@ -22,6 +23,9 @@ class DatabaseMigrationStateActivity : FullComposeActivity() { @Inject lateinit var exporter: LogExporter + @Inject + lateinit var loginStateRepository: LoginStateRepository + @Composable override fun ComposeContent() { DatabaseMigrationScreen( @@ -29,6 +33,7 @@ class DatabaseMigrationStateActivity : FullComposeActivity() { fm = supportFragmentManager, exporter = exporter, clearDataUtils = clearDataUtils, + canRestoreAccount = loginStateRepository.peekLoginState() != null, ) val state = migrationManager.migrationState.collectAsState().value From c2eeb47fa9270164991f009296cb29e08264c866 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 21 Sep 2026 11:37:05 +1000 Subject: [PATCH 2/7] Hold database-backed startup work until the migration has a usable secret (#2213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Routing the keystore failure into MigrationState.Error was not enough on its own: the database is reached during startup by components that construct themselves eagerly and by flows that start on login state, and the first of them to be handed an exception took the process down before the migration screen could appear. Split the startup components so everything that reaches the database is resolved through a Provider that is only asked for once the migration reports Completed, which defers construction as well as the callback — the pollers start their own work from their constructors, so deferring onPostAppStarted alone would have started them anyway. openHelper goes back to waiting rather than throwing. Parking a caller that cannot proceed is better than handing it a failure it does not expect, and the callers that reach the database outside the startup sequence are not enumerable from one place. A retry that succeeds reaches Completed and releases everyone waiting. Verified on an emulator by corrupting the sealed secret in place: the app now survives and shows the database error screen with Retry, Export Logs and Clear Device and Restart, logs "Keystore failure: code=10, transient=false, systemError=false", drops Clear Device and Restore when the login state is unsealable too, and starts normally with no keystore failures once the secret is sound. --- .../dependencies/OnAppStartupComponents.kt | 33 +++++++++++++++++-- .../migration/DatabaseMigrationManager.kt | 22 +++++-------- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt b/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt index 1f1aa4b7f3..9e3c1e3b4b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt @@ -1,5 +1,9 @@ package org.thoughtcrime.securesms.dependencies +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch import org.session.libsession.messaging.notifications.TokenFetcher import org.session.libsession.messaging.sending_receiving.pollers.OpenGroupPollerManager import org.session.libsession.network.SnodeClock @@ -17,8 +21,33 @@ import org.thoughtcrime.securesms.tokenpage.TokenDataManager import org.thoughtcrime.securesms.util.CurrentActivityObserver import org.thoughtcrime.securesms.webrtc.WebRtcCallBridge import javax.inject.Inject +import javax.inject.Provider -class OnAppStartupComponents private constructor( +class OnAppStartupComponents @Inject constructor( + private val databaseMigrationManager: DatabaseMigrationManager, + private val databaseBackedComponents: Provider, + @param:ManagerScope private val scope: CoroutineScope, +) { + fun onPostAppStarted() { + databaseMigrationManager.onPostAppStarted() + + // Everything in DatabaseBackedStartupComponents reaches the database, several of them from + // their own constructors, and the open helper has nothing to hand out until the migration + // has produced a usable secret. Resolving them before then means whichever touches it first + // dies on a failure the user could otherwise have retried from, taking the process with it + // and leaving the migration screen no chance to appear (#2213). A retry that succeeds still + // reaches Completed, so this also starts them after a recovery. + scope.launch(Dispatchers.Main) { + databaseMigrationManager.migrationState.first { + it == DatabaseMigrationManager.MigrationState.Completed + } + + databaseBackedComponents.get().onPostAppStarted() + } + } +} + +class DatabaseBackedStartupComponents private constructor( private val components: List ) { fun onPostAppStarted() { @@ -29,7 +58,6 @@ class OnAppStartupComponents private constructor( groupPollerManager: GroupPollerManager, expiredGroupManager: ExpiredGroupManager, openGroupPollerManager: OpenGroupPollerManager, - databaseMigrationManager: DatabaseMigrationManager, tokenManager: TokenDataManager, currentActivityObserver: CurrentActivityObserver, webRtcCallBridge: WebRtcCallBridge, @@ -47,7 +75,6 @@ class OnAppStartupComponents private constructor( groupPollerManager, expiredGroupManager, openGroupPollerManager, - databaseMigrationManager, tokenManager, currentActivityObserver, webRtcCallBridge, diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt index 1989120745..57277c18fe 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt @@ -46,20 +46,14 @@ class DatabaseMigrationManager @Inject constructor( // First perform a cheap check to see if the migration is done, if so we can skip the wait. if (mutableMigrationState.value != MigrationState.Completed) { // Blocking is deliberate: callers don't expect to handle a half-migrated database, so - // they wait here instead. Error has to end the wait as well as Completed, because the - // state only leaves Error on an explicit retry and waiting for Completed alone hangs - // every caller until the process dies. - val finalState = runBlocking { - migrationState.first { - it == MigrationState.Completed || it is MigrationState.Error - } - } - - if (finalState is MigrationState.Error) { - throw IllegalStateException( - "Database unavailable: migration failed", - finalState.throwable - ) + // they wait here instead, and that includes waiting through a failure. Throwing would + // be worse than parking them: the database is reached from background flows that start + // on login state rather than through any startup sequence, none of them expect this + // call to fail, and the first one to be handed an exception takes the process down + // before the migration screen can offer a retry (#2213). A retry that succeeds reaches + // Completed and releases everyone waiting here. + runBlocking { + migrationState.first { it == MigrationState.Completed } } } From 4d4e0bae87de305e553dbfec3c56a277912a47f8 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 22 Sep 2026 12:04:05 +1000 Subject: [PATCH 3/7] Narrow the migration catch so a dying VM is not offered a retry button catch (Throwable) also caught OutOfMemoryError, StackOverflowError and LinkageError, turning a process that is already lost into a database error screen offering Retry and Clear Data. Catch Exception and AssertionError instead: AssertionError is the one Error on this path thrown deliberately, as KeyStoreHelper's way of reporting a crypto failure, which is what makes singling it out defensible rather than arbitrary. --- .../migration/DatabaseMigrationManager.kt | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt index 57277c18fe..9901e3f99a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt @@ -118,15 +118,24 @@ class DatabaseMigrationManager @Inject constructor( } mutableMigrationState.value = MigrationState.Completed - } catch (ec: Throwable) { - // Throwable, not Exception: KeyStoreHelper reports every crypto failure as an - // AssertionError, which is an Error and would otherwise pass straight through here. - logKeyStoreFailure(ec) - mutableMigrationState.value = MigrationState.Error(ec) + } catch (ec: Exception) { + recordMigrationFailure(ec) + return + } catch (ec: AssertionError) { + // AssertionError is caught alongside Exception because KeyStoreHelper throws it on + // purpose — it is how every crypto failure is reported, a deliberate signal rather than + // a symptom of a dying VM. Errors that do mean the process is already lost + // (OutOfMemoryError, StackOverflowError, LinkageError) are deliberately left to kill it. + recordMigrationFailure(ec) return } } + private fun recordMigrationFailure(error: Throwable) { + logKeyStoreFailure(error) + mutableMigrationState.value = MigrationState.Error(error) + } + /** * Records the keystore error code behind a crypto failure, if there is one. * From 27261fd25963a258637c376a5c303bfb2ce0f071 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 22 Sep 2026 12:04:05 +1000 Subject: [PATCH 4/7] Stop a weekly VACUUM from being able to prevent the app starting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The VACUUM in postKey runs on the database-open path, unwrapped, so any failure reached the caller as a database that would not open rather than as failed maintenance. It also recorded only success, so a VACUUM that threw was retried on every open from then on — one full disk became a launch that never worked again. Record the attempt before making it, skip it when there is less than twice the database file's size free since that is roughly what rebuilding it needs, and catch what is left. Note the timestamp is persisted with apply(), so on a genuinely full disk the write can be dropped and the attempt repeats next launch; the catch, not the ordering, is what stops that being fatal. Verified only in part: the block is entered when due and the timestamp now advances on entry, and startup is unaffected across repeated runs. The skip and catch branches were not observed executing — a full emulator disk reclaims cache at exactly the boundary that would trigger them — so those two paths are reasoned, not tested. --- .../database/helpers/SQLCipherOpenHelper.java | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java index ce53ca37ba..91aff24375 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java @@ -45,6 +45,8 @@ import org.thoughtcrime.securesms.pro.db.ProDatabase; import org.thoughtcrime.securesms.util.ConfigurationMessageUtilities; +import java.io.File; + import javax.inject.Provider; import kotlinx.serialization.json.Json; @@ -141,8 +143,29 @@ public void postKey(SQLiteConnection connection) { long currentTime = System.currentTimeMillis(); // 7 days if (currentTime - TextSecurePreferences.getLastVacuumTime(context) > 604_800_000) { - connection.execute("VACUUM;", null, null); + // Recorded before the attempt rather than after it: a VACUUM that throws would + // otherwise leave the timestamp untouched and be retried on every open from then on, + // which is what turns one unlucky moment into a launch that never succeeds again. TextSecurePreferences.setLastVacuumNow(context); + + // VACUUM rebuilds the database into a temporary copy before replacing the original, so + // it needs roughly twice the file's size free. Below that it is going to fail, and + // failing takes as long as the copy it got through first. + File database = context.getDatabasePath(DATABASE_NAME); + long requiredSpace = database.length() * 2; + + if (database.getUsableSpace() > requiredSpace) { + // This is maintenance, and it runs on the database-open path, where an exception + // does not reach the caller as a failed VACUUM — it reaches them as a database that + // would not open. Nothing here is worth not starting the app for. + try { + connection.execute("VACUUM;", null, null); + } catch (Exception e) { + Log.w(TAG, "Vacuum failed, continuing", e); + } + } else { + Log.i(TAG, "Skipping vacuum: needs " + requiredSpace + " bytes free"); + } } } }, From 0af82004597dd865bb2dc4701d8c08ddf31ff277 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 13:57:26 +1000 Subject: [PATCH 5/7] Re-read a login state that was unreadable once the keystore is working again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The login state is unsealed once, when its repository is built, and a failure is indistinguishable from having no account: both leave it null. Surviving a keystore fault instead of crashing turns that into data loss. The process now stays alive with the account pinned to null, routing sends the user to the welcome screen on top of their own data, and creating an account there re-seals a new seed over the old one. Crashing was self-healing by accident — every relaunch read both secrets again. Record that the state was unreadable rather than absent, and re-attempt the read when the migration reaches Completed, which is the point where there is fresh evidence the keystore is working. A recovered read repopulates the state before anything routes on it; the flag keeps it a no-op in the ordinary case. Verified on an emulator: the re-attempt fires with the flag set and genuinely re-reads (a corrupt blob logs the unseal failure twice, once per attempt), stays a no-op when the first read succeeded, and leaves healthy startup unchanged. The recovery branch itself is not covered — a transient keystore fault cannot be induced, and a corrupt blob fails identically on every attempt. --- .../securesms/auth/LoginStateRepository.kt | 51 +++++++++++++++---- .../migration/DatabaseMigrationManager.kt | 8 +++ 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt index 5cb74ee239..c079c68a6e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt @@ -47,17 +47,17 @@ class LoginStateRepository @Inject constructor( private val mutableLoggedInState: MutableStateFlow - init { - var initialState = prefs[keyState]?.let { serializedState -> - runCatching { - json.decodeFromString( - KeyStoreHelper.unseal(serializedState).toString(Charsets.UTF_8) - ) + /** + * Set when the stored login state existed but could not be unsealed, which is not the same thing + * as having no account. Callers cannot tell the two apart from a null state, and treating an + * unreadable account as an absent one sends the user to onboarding, where creating an account + * overwrites the seed that was only unreadable. + */ + @Volatile + private var sealedStateUnreadable = false - }.onFailure { - Log.e(TAG, "Unable to unseal login state", it) - }.getOrNull() - } + init { + var initialState = readSealedState() if (initialState == null) { initialState = runCatching { @@ -179,6 +179,37 @@ class LoginStateRepository @Inject constructor( mutableLoggedInState.value = null } + /** + * Re-attempts a sealed state read that previously failed, and reports whether an account was + * recovered. + * + * Keystore faults are frequently transient, but the read is only performed once when this + * singleton is built, so a fault at startup leaves the account unreadable for the rest of the + * process even after the keystore recovers. Call this wherever there is fresh reason to believe + * it has. + */ + fun reloadUnreadableState(): Boolean { + if (!sealedStateUnreadable || mutableLoggedInState.value != null) return false + + val recovered = readSealedState() ?: return false + + Log.i(TAG, "Recovered a login state that was previously unreadable") + sealedStateUnreadable = false + mutableLoggedInState.value = recovered + return true + } + + private fun readSealedState(): LoggedInState? = prefs[keyState]?.let { serializedState -> + runCatching { + json.decodeFromString( + KeyStoreHelper.unseal(serializedState).toString(Charsets.UTF_8) + ) + }.onFailure { + sealedStateUnreadable = true + Log.e(TAG, "Unable to unseal login state", it) + }.getOrNull() + } + /** * Updates the current [LoggedInState] using the provided [updater] function. * The [org.thoughtcrime.securesms.auth.LoginStateRepository] will manage the persistence diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt index 9901e3f99a..b51366e102 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt @@ -19,6 +19,7 @@ import net.zetetic.database.sqlcipher.SQLiteDatabaseHook import network.loki.messenger.R import org.session.libsession.utilities.TextSecurePreferences import org.session.libsignal.utilities.Log +import org.thoughtcrime.securesms.auth.LoginStateRepository import org.thoughtcrime.securesms.crypto.DatabaseSecretProvider import org.thoughtcrime.securesms.database.helpers.SQLCipherOpenHelper import org.thoughtcrime.securesms.dependencies.ManagerScope @@ -32,6 +33,7 @@ class DatabaseMigrationManager @Inject constructor( private val application: Application, private val prefs: TextSecurePreferences, private val databaseSecretProvider: DatabaseSecretProvider, + private val loginStateRepository: Provider, jsonProvider: Provider, @param:ManagerScope private val scope: CoroutineScope, ) : OnAppStartupComponent { @@ -117,6 +119,12 @@ class DatabaseMigrationManager @Inject constructor( mutableMigrationState.value = MigrationState.Migrating(steps.toList()) } + // Reaching Completed after a failure means the keystore is working again, and the login + // state is only read once when its repository is built — so an account left unreadable + // by the same fault has to be re-read before anything routes on it, or a recovered user + // is sent to onboarding on top of their own data. + loginStateRepository.get().reloadUnreadableState() + mutableMigrationState.value = MigrationState.Completed } catch (ec: Exception) { recordMigrationFailure(ec) From 868930da500c31549e8dfeb955ec5bdf58e3c012 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 14:14:00 +1000 Subject: [PATCH 6/7] Stop deferring startup components that take no database dependency CurrentActivityObserver, AppDisguiseManager and NotificationChannelManager were held back with the rest, and the comment claimed everything in the group reached the database, which was not true of any of the three. CurrentActivityObserver registers the activity lifecycle callbacks in its own constructor, so deferring it dropped the events arriving before the migration finished. They now start immediately. The remainder still waits as a group rather than being classified one by one: transitive reach through injected dependencies is not cheap to establish per component, and starting one too early costs more than starting one too late. Renamed to match what the group now claims about itself. --- .../dependencies/OnAppStartupComponents.kt | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt b/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt index 9e3c1e3b4b..16c0355047 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/dependencies/OnAppStartupComponents.kt @@ -25,29 +25,41 @@ import javax.inject.Provider class OnAppStartupComponents @Inject constructor( private val databaseMigrationManager: DatabaseMigrationManager, - private val databaseBackedComponents: Provider, + private val currentActivityObserver: CurrentActivityObserver, + private val appDisguiseManager: AppDisguiseManager, + private val notificationChannelManager: NotificationChannelManager, + private val deferredComponents: Provider, @param:ManagerScope private val scope: CoroutineScope, ) { fun onPostAppStarted() { databaseMigrationManager.onPostAppStarted() - // Everything in DatabaseBackedStartupComponents reaches the database, several of them from - // their own constructors, and the open helper has nothing to hand out until the migration - // has produced a usable secret. Resolving them before then means whichever touches it first - // dies on a failure the user could otherwise have retried from, taking the process with it - // and leaving the migration screen no chance to appear (#2213). A retry that succeeds still - // reaches Completed, so this also starts them after a recovery. + // These three take no database dependency, and waiting costs something: CurrentActivityObserver + // registers the activity lifecycle callbacks in its own constructor, so deferring it drops the + // events that arrive before the migration finishes. + currentActivityObserver.onPostAppStarted() + appDisguiseManager.onPostAppStarted() + notificationChannelManager.onPostAppStarted() + + // The rest wait because some of them reach the database, several from their own constructors, + // and the open helper has nothing to hand out until the migration has produced a usable secret. + // Whichever touches it first otherwise dies on a failure the user could have retried from, + // taking the process with it and leaving the migration screen no chance to appear (#2213). + // Deferring the whole remainder is the conservative choice: transitive reach through injected + // dependencies is not cheap to establish per component, and starting one too early costs more + // than starting one too late. A retry that succeeds still reaches Completed, so this also + // starts them after a recovery. scope.launch(Dispatchers.Main) { databaseMigrationManager.migrationState.first { it == DatabaseMigrationManager.MigrationState.Completed } - databaseBackedComponents.get().onPostAppStarted() + deferredComponents.get().onPostAppStarted() } } } -class DatabaseBackedStartupComponents private constructor( +class DeferredStartupComponents private constructor( private val components: List ) { fun onPostAppStarted() { @@ -59,33 +71,27 @@ class DatabaseBackedStartupComponents private constructor( expiredGroupManager: ExpiredGroupManager, openGroupPollerManager: OpenGroupPollerManager, tokenManager: TokenDataManager, - currentActivityObserver: CurrentActivityObserver, webRtcCallBridge: WebRtcCallBridge, persistentLogger: PersistentLogger, - appDisguiseManager: AppDisguiseManager, tokenFetcher: TokenFetcher, emojiIndexLoader: EmojiIndexLoader, subscriptionCoordinator: SubscriptionCoordinator, authAwareHandler: AuthAwareComponentsHandler, snodeClock: SnodeClock, subscriptionManagers: Set<@JvmSuppressWildcards SubscriptionManager>, - notificationChannelManager: NotificationChannelManager, ): this( components = listOf( groupPollerManager, expiredGroupManager, openGroupPollerManager, tokenManager, - currentActivityObserver, webRtcCallBridge, persistentLogger, - appDisguiseManager, tokenFetcher, emojiIndexLoader, subscriptionCoordinator, authAwareHandler, - snodeClock, - notificationChannelManager + snodeClock ) + subscriptionManagers ) } From fe2ecd5d9baccfae949b116fa8d32b11e6681c8b Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 14:14:00 +1000 Subject: [PATCH 7/7] Describe what the openHelper wait does rather than defending it The comment argued that blocking beat throwing, which was the reasoning at the time and is not a property of the code. What a reader needs is the hazard: Error is terminal and is not a terminating condition for the wait, so a caller arriving after a failed migration blocks for the life of the process and its thread is never returned. --- .../migration/DatabaseMigrationManager.kt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt index b51366e102..7ada19885a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migration/DatabaseMigrationManager.kt @@ -47,13 +47,13 @@ class DatabaseMigrationManager @Inject constructor( // First perform a cheap check to see if the migration is done, if so we can skip the wait. if (mutableMigrationState.value != MigrationState.Completed) { - // Blocking is deliberate: callers don't expect to handle a half-migrated database, so - // they wait here instead, and that includes waiting through a failure. Throwing would - // be worse than parking them: the database is reached from background flows that start - // on login state rather than through any startup sequence, none of them expect this - // call to fail, and the first one to be handed an exception takes the process down - // before the migration screen can offer a retry (#2213). A retry that succeeds reaches - // Completed and releases everyone waiting here. + // Callers cannot do anything sensible with a half-migrated database, so they wait + // here rather than be handed one, and a retry that succeeds releases them. + // + // Known hazard: Error is terminal and is not a terminating condition for this wait, so + // a caller arriving after a failed migration blocks for the life of the process and its + // thread is never returned. Deferring database-backed startup work keeps the usual + // arrivals away from it, but callers reached from login-state flows are not covered. runBlocking { migrationState.first { it == MigrationState.Completed } }