Handle keystore and VACUUM failures on the database-open path instead of crash-looping (#2213)

app/src/main/java/org/thoughtcrime/securesms/auth/LoginStateRepository.kt

@@ -47,17 +47,17 @@ class LoginStateRepository @Inject constructor(
     private val mutableLoggedInState: MutableStateFlow<LoggedInState?>
 
 
-    init {
-        var initialState = prefs[keyState]?.let { serializedState ->
-            runCatching {
-                json.decodeFromString<LoggedInState>(
-                    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<LoggedInState>(
+                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

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 @@
           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");
+            }
           }
         }
       },
@@ -164,10 +187,10 @@
     db.execSQL(AttachmentDatabase.CREATE_TABLE);
     db.execSQL(ThreadDatabase.CREATE_TABLE);
     db.execSQL(DraftDatabase.CREATE_TABLE);
     db.execSQL(PushDatabase.CREATE_TABLE);
     db.execSQL(GroupDatabase.CREATE_TABLE);
     db.execSQL(RecipientDatabase.CREATE_TABLE);
     db.execSQL(GroupReceiptDatabase.CREATE_TABLE);
     for (String sql : SearchDatabase.CREATE_TABLE) {
       db.execSQL(sql);
     }
@@ -205,12 +228,12 @@
     db.execSQL(LokiMessageDatabase.getUpdateMessageIDTableForType());
     db.execSQL(LokiMessageDatabase.getUpdateMessageMappingTable());
     db.execSQL(SessionContactDatabase.getCreateSessionContactTableCommand());
     db.execSQL(RecipientDatabase.getCreateNotificationTypeCommand());
     db.execSQL(ThreadDatabase.getCreatePinnedCommand());
     db.execSQL(GroupDatabase.getCreateUpdatedTimestampCommand());
     db.execSQL(RecipientDatabase.getCreateApprovedCommand());
     db.execSQL(RecipientDatabase.getCreateApprovedMeCommand());
     db.execSQL(RecipientDatabase.getCreateDisappearingStateCommand());
     db.execSQL(MmsDatabase.CREATE_MESSAGE_REQUEST_RESPONSE_COMMAND);
     db.execSQL(MmsDatabase.CREATE_REACTIONS_UNREAD_COMMAND);
     db.execSQL(SmsDatabase.CREATE_REACTIONS_UNREAD_COMMAND);
@@ -240,13 +263,13 @@
     executeStatements(db, ThreadDatabase.CREATE_INDEXES);
     executeStatements(db, DraftDatabase.CREATE_INDEXS);
     executeStatements(db, GroupDatabase.CREATE_INDEXS);
     executeStatements(db, GroupReceiptDatabase.CREATE_INDEXES);
     executeStatements(db, ReactionDatabase.CREATE_INDEXS);
 
     executeStatements(db, ReactionDatabase.CREATE_REACTION_TRIGGERS);
     executeStatements(db, ReactionDatabase.CREATE_MESSAGE_ID_MMS_INDEX);
     db.execSQL(RecipientDatabase.getAddWrapperHash());
     db.execSQL(RecipientDatabase.getAddBlocksCommunityMessageRequests());
     db.execSQL(LokiAPIDatabase.CREATE_LAST_LEGACY_MESSAGE_TABLE);
 
     db.execSQL(RecipientDatabase.getCreateAutoDownloadCommand());

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,45 @@ 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 currentActivityObserver: CurrentActivityObserver,
+    private val appDisguiseManager: AppDisguiseManager,
+    private val notificationChannelManager: NotificationChannelManager,
+    private val deferredComponents: Provider<DeferredStartupComponents>,
+    @param:ManagerScope private val scope: CoroutineScope,
+) {
+    fun onPostAppStarted() {
+        databaseMigrationManager.onPostAppStarted()
+
+        // 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
+            }
+
+            deferredComponents.get().onPostAppStarted()
+        }
+    }
+}
+
+class DeferredStartupComponents private constructor(
     private val components: List<OnAppStartupComponent>
 ) {
     fun onPostAppStarted() {
@@ -29,36 +70,28 @@ class OnAppStartupComponents private constructor(
         groupPollerManager: GroupPollerManager,
         expiredGroupManager: ExpiredGroupManager,
         openGroupPollerManager: OpenGroupPollerManager,
-        databaseMigrationManager: DatabaseMigrationManager,
         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,
-            databaseMigrationManager,
             tokenManager,
-            currentActivityObserver,
             webRtcCallBridge,
             persistentLogger,
-            appDisguiseManager,
             tokenFetcher,
             emojiIndexLoader,
             subscriptionCoordinator,
             authAwareHandler,
-            snodeClock,
-            notificationChannelManager
+            snodeClock
         ) + subscriptionManagers
     )
 }

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
@@ -17,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
@@ -30,6 +33,7 @@ class DatabaseMigrationManager @Inject constructor(
     private val application: Application,
     private val prefs: TextSecurePreferences,
     private val databaseSecretProvider: DatabaseSecretProvider,
+    private val loginStateRepository: Provider<LoginStateRepository>,
     jsonProvider: Provider<Json>,
     @param:ManagerScope private val scope: CoroutineScope,
 ) : OnAppStartupComponent {
@@ -43,10 +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) {
-            // 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.
+            // 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 }
             }
@@ -90,6 +97,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()
@@ -106,13 +119,58 @@ 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) {
-            mutableMigrationState.value = MigrationState.Error(ec)
+            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.
+     *
+     * 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<android.security.KeyStoreException>()
+            .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")

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,

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,13 +23,17 @@ class DatabaseMigrationStateActivity : FullComposeActivity() {
     @Inject
     lateinit var exporter: LogExporter
 
+    @Inject
+    lateinit var loginStateRepository: LoginStateRepository
+
     @Composable
     override fun ComposeContent() {
         DatabaseMigrationScreen(
             migrationManager = migrationManager,
             fm = supportFragmentManager,
             exporter = exporter,
             clearDataUtils = clearDataUtils,
+            canRestoreAccount = loginStateRepository.peekLoginState() != null,
         )
 
         val state = migrationManager.migrationState.collectAsState().value