Use BackupManagerMonitor to handle K/V with no data changed

The fake package manager package is essential for the backup, but when its data doesn't change and we request a normal incremental backup, it doesn't get included, because our transport doesn't even get called for it. Only the BackupMonitor gets a hint that it had no (new?) data via LOG_EVENT_ID_NO_DATA_TO_SEND.
This behavior started with Android 15 that fixed a bug that caused @pm@ to always backup. However, other K/V apps were probably affected before.

app/src/main/java/com/stevesoltys/seedvault/BackupMonitor.kt

@@ -17,18 +17,26 @@ import android.util.Log.DEBUG
 
 private val TAG = BackupMonitor::class.java.name
 
-class BackupMonitor : IBackupManagerMonitor.Stub() {
+open class BackupMonitor : IBackupManagerMonitor.Stub() {
 
     override fun onEvent(bundle: Bundle) {
-        val id = bundle.getInt(EXTRA_LOG_EVENT_ID)
+        onEvent(
-        val packageName = bundle.getString(EXTRA_LOG_EVENT_PACKAGE_NAME, "?")
+            id = bundle.getInt(EXTRA_LOG_EVENT_ID),
+            category = bundle.getInt(EXTRA_LOG_EVENT_CATEGORY),
+            packageName = bundle.getString(EXTRA_LOG_EVENT_PACKAGE_NAME)
+                ?: error("no package name for $bundle"),
+            bundle = bundle,
+        )
+    }
+
+    open fun onEvent(id: Int, category: Int, packageName: String, bundle: Bundle) {
         if (id == LOG_EVENT_ID_ERROR_PREFLIGHT) {
             val preflightResult = bundle.getLong(EXTRA_LOG_PREFLIGHT_ERROR, -1)
             Log.w(TAG, "Pre-flight error from $packageName: $preflightResult")
         }
         if (!Log.isLoggable(TAG, DEBUG)) return
         Log.d(TAG, "ID: $id")
-        Log.d(TAG, "CATEGORY: " + bundle.getInt(EXTRA_LOG_EVENT_CATEGORY, -1))
+        Log.d(TAG, "CATEGORY: $category")
         Log.d(TAG, "PACKAGE: $packageName")
     }
 

app/src/main/java/com/stevesoltys/seedvault/metadata/Metadata.kt

@@ -121,11 +121,7 @@ data class PackageMetadata(
     companion object {
         fun fromSnapshot(app: Snapshot.App) = PackageMetadata(
             time = app.time,
-            backupType = when (app.type) {
+            backupType = app.type.toBackupType(),
-                Snapshot.BackupType.FULL -> BackupType.FULL
-                Snapshot.BackupType.KV -> BackupType.KV
-                else -> null
-            },
             name = app.name,
             chunkIds = app.chunkIdsList.hexFromProto(),
             system = app.system,
@@ -153,6 +149,12 @@ data class PackageMetadata(
                 it.isNotEmpty()
             },
         )
+
+        fun Snapshot.BackupType.toBackupType() = when (this) {
+            Snapshot.BackupType.FULL -> BackupType.FULL
+            Snapshot.BackupType.KV -> BackupType.KV
+            else -> null
+        }
     }
 
     val isInternalSystem: Boolean = system && !isLaunchableSystemApp

app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt

@@ -53,16 +53,13 @@ internal class MetadataManager(
     /**
      * Call this after a package has been backed up successfully.
      *
-     * It updates the packages' metadata
+     * It updates the packages' metadata.
-     * and writes it encrypted to the given [OutputStream] as well as the internal cache.
-     *
-     * Closing the [OutputStream] is the responsibility of the caller.
      */
     @Synchronized
     @Throws(IOException::class)
     fun onPackageBackedUp(
         packageInfo: PackageInfo,
-        type: BackupType,
+        type: BackupType?,
         size: Long?,
     ) {
         val packageName = packageInfo.packageName

app/src/main/java/com/stevesoltys/seedvault/repo/SnapshotCreator.kt

@@ -16,9 +16,11 @@ import android.provider.Settings
 import android.provider.Settings.Secure.ANDROID_ID
 import com.google.protobuf.ByteString
 import com.stevesoltys.seedvault.Clock
+import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
 import com.stevesoltys.seedvault.header.VERSION
 import com.stevesoltys.seedvault.metadata.BackupType
 import com.stevesoltys.seedvault.metadata.MetadataManager
+import com.stevesoltys.seedvault.metadata.PackageMetadata.Companion.toBackupType
 import com.stevesoltys.seedvault.proto.Snapshot
 import com.stevesoltys.seedvault.proto.Snapshot.Apk
 import com.stevesoltys.seedvault.proto.Snapshot.App
@@ -28,6 +30,7 @@ import com.stevesoltys.seedvault.transport.backup.isSystemApp
 import io.github.oshai.kotlinlogging.KotlinLogging
 import org.calyxos.seedvault.core.backends.AppBackupFileType
 import org.calyxos.seedvault.core.toHexString
+import java.util.concurrent.ConcurrentHashMap
 
 /**
  * Assembles snapshot information over the course of a single backup run
@@ -43,8 +46,8 @@ internal class SnapshotCreator(
     private val log = KotlinLogging.logger { }
 
     private val snapshotBuilder = Snapshot.newBuilder()
-    private val appBuilderMap = mutableMapOf<String, App.Builder>()
+    private val appBuilderMap = ConcurrentHashMap<String, App.Builder>()
-    private val blobsMap = mutableMapOf<String, Blob>()
+    private val blobsMap = ConcurrentHashMap<String, Blob>()
 
     private val launchableSystemApps by lazy {
         // as we can't ask [PackageInfo] for this, we keep a set of packages around
@@ -103,6 +106,50 @@ internal class SnapshotCreator(
         metadataManager.onPackageBackedUp(packageInfo, backupType, backupData.size)
     }
 
+    /**
+     * Call this when the given [packageName] may not call our transport at all in this run,
+     * but we need to include data for the package in the current snapshot.
+     * This may happen for K/V apps like @pm@ that don't call us when their data didn't change.
+     *
+     * If we do *not* have data for the given [packageName],
+     * we try to extract data from the given [snapshot] (ideally we latest we have) and
+     * add it to the current snapshot under construction.
+     */
+    fun onNoDataInCurrentRun(snapshot: Snapshot, packageName: String) {
+        log.info { "onKvPackageNotChanged(${snapshot.token}, $packageName)" }
+
+        if (appBuilderMap.containsKey(packageName)) {
+            // the system backs up K/V apps repeatedly, e.g. @pm@
+            log.info { "  Already have data for $packageName in current snapshot, not touching it" }
+            return
+        }
+        val app = snapshot.appsMap[packageName]
+        if (app == null) {
+            log.error { "  No changed data for $packageName, but we had no data for it" }
+            return
+        }
+
+        // get chunkIds from last snapshot
+        val chunkIds = app.chunkIdsList.hexFromProto() +
+            app.apk.splitsList.flatMap { it.chunkIdsList }.hexFromProto()
+
+        // get blobs for chunkIds
+        val blobMap = mutableMapOf<String, Blob>()
+        chunkIds.forEach { chunkId ->
+            val blob = snapshot.blobsMap[chunkId]
+            if (blob == null) log.error { "  No blob for $packageName chunk $chunkId" }
+            else blobMap[chunkId] = blob
+        }
+
+        // add info to current snapshot
+        appBuilderMap[packageName] = app.toBuilder()
+        blobsMap.putAll(blobMap)
+
+        // record local metadata
+        val packageInfo = PackageInfo().apply { this.packageName = packageName }
+        metadataManager.onPackageBackedUp(packageInfo, app.type.toBackupType(), app.size)
+    }
+
     /**
      * Call this after all blobs for the app icons have been saved to the backend.
      */
@@ -134,6 +181,8 @@ internal class SnapshotCreator(
             putAllApps(appBuilderMap.mapValues { it.value.build() })
             putAllBlobs(this@SnapshotCreator.blobsMap)
         }.build()
+        // may as well fail the backup, if @pm@ isn't in it
+        check(MAGIC_PACKAGE_MANAGER in snapshot.appsMap) { "No metadata for @pm@" }
         appBuilderMap.clear()
         snapshotBuilder.clear()
         blobsMap.clear()

app/src/main/java/com/stevesoltys/seedvault/repo/SnapshotManager.kt

@@ -39,6 +39,7 @@ internal class SnapshotManager(
      * The latest [Snapshot]. May be stale if [onSnapshotsLoaded] has not returned
      * or wasn't called since new snapshots have been created.
      */
+    @Volatile
     var latestSnapshot: Snapshot? = null
         private set
 

app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupModule.kt

@@ -9,6 +9,7 @@ import org.koin.android.ext.koin.androidContext
 import org.koin.dsl.module
 
 val backupModule = module {
+    factory { BackupTransportMonitor(get(), get()) }
     single { BackupInitializer(get()) }
     single { InputFactory() }
     single {

app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupTransportMonitor.kt

@@ -0,0 +1,43 @@
+/*
+ * SPDX-FileCopyrightText: 2024 The Calyx Institute
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package com.stevesoltys.seedvault.transport.backup
+
+import android.app.backup.BackupManagerMonitor.LOG_EVENT_CATEGORY_BACKUP_MANAGER_POLICY
+import android.app.backup.BackupManagerMonitor.LOG_EVENT_ID_NO_DATA_TO_SEND
+import android.os.Bundle
+import com.stevesoltys.seedvault.BackupMonitor
+import com.stevesoltys.seedvault.repo.AppBackupManager
+import com.stevesoltys.seedvault.repo.SnapshotManager
+import io.github.oshai.kotlinlogging.KotlinLogging
+
+internal class BackupTransportMonitor(
+    private val appBackupManager: AppBackupManager,
+    private val snapshotManager: SnapshotManager,
+) : BackupMonitor() {
+
+    private val log = KotlinLogging.logger { }
+
+    override fun onEvent(id: Int, category: Int, packageName: String, bundle: Bundle) {
+        super.onEvent(id, category, packageName, bundle)
+        if (id == LOG_EVENT_ID_NO_DATA_TO_SEND &&
+            category == LOG_EVENT_CATEGORY_BACKUP_MANAGER_POLICY
+        ) {
+            sendNoDataChanged(packageName)
+        }
+    }
+
+    private fun sendNoDataChanged(packageName: String) {
+        log.info { "sendNoDataChanged($packageName)" }
+
+        val snapshot = snapshotManager.latestSnapshot
+        if (snapshot == null) {
+            log.error { "No latest snapshot!" }
+        } else {
+            val snapshotCreator = appBackupManager.snapshotCreator ?: error("No SnapshotCreator")
+            snapshotCreator.onNoDataInCurrentRun(snapshot, packageName)
+        }
+    }
+}

app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt

@@ -14,6 +14,7 @@ import android.app.backup.BackupTransport.TRANSPORT_OK
 import android.content.pm.PackageInfo
 import android.os.ParcelFileDescriptor
 import android.util.Log
+import com.stevesoltys.seedvault.NO_DATA_END_SENTINEL
 import com.stevesoltys.seedvault.repo.BackupData
 import com.stevesoltys.seedvault.repo.BackupReceiver
 import java.io.IOException
@@ -52,6 +53,8 @@ internal class KVBackup(
             else -> Log.i(TAG, "Performing K/V backup for $packageName")
         }
         check(state == null) { "Have unexpected state for ${state?.packageInfo?.packageName}" }
+        // This fake package name just signals that we've seen all packages without new data
+        if (packageName == NO_DATA_END_SENTINEL) return TRANSPORT_OK
 
         // initialize state
         state = KVBackupState(packageInfo = packageInfo, db = dbManager.getDb(packageName))

app/src/main/java/com/stevesoltys/seedvault/transport/backup/PackageService.kt

@@ -59,18 +59,17 @@ internal class PackageService(
                 logPackages(packages)
             }
 
-            val eligibleApps = packages.filter(::shouldIncludeAppInBackup).toTypedArray()
+            val eligibleApps = packages.filter(::shouldIncludeAppInBackup).toMutableList()
             // log eligible packages
             if (Log.isLoggable(TAG, INFO)) {
                 Log.i(TAG, "Filtering left ${eligibleApps.size} eligible packages:")
-                logPackages(eligibleApps.toList())
+                logPackages(eligibleApps)
             }
 
             // add magic @pm@ package (PACKAGE_MANAGER_SENTINEL) which holds package manager data
-            val packageArray = eligibleApps.toMutableList()
+            eligibleApps.add(0, MAGIC_PACKAGE_MANAGER)
-            packageArray.add(MAGIC_PACKAGE_MANAGER)
 
-            return packageArray
+            return eligibleApps
         }
 
     /**

app/src/main/java/com/stevesoltys/seedvault/worker/BackupRequester.kt

@@ -17,7 +17,7 @@ import androidx.core.content.ContextCompat.startForegroundService
 import com.stevesoltys.seedvault.settings.SettingsManager
 import com.stevesoltys.seedvault.storage.StorageBackupService
 import com.stevesoltys.seedvault.storage.StorageBackupService.Companion.EXTRA_START_APP_BACKUP
-import com.stevesoltys.seedvault.BackupMonitor
+import com.stevesoltys.seedvault.transport.backup.BackupTransportMonitor
 import com.stevesoltys.seedvault.transport.backup.PackageService
 import com.stevesoltys.seedvault.ui.notification.BackupNotificationManager
 import com.stevesoltys.seedvault.ui.notification.NotificationBackupObserver
@@ -38,6 +38,7 @@ internal const val NUM_PACKAGES_PER_TRANSACTION = 100
 internal class BackupRequester(
     context: Context,
     private val backupManager: IBackupManager,
+    private val monitor: BackupTransportMonitor,
     val packageService: PackageService,
 ) : KoinComponent {
 
@@ -72,7 +73,6 @@ internal class BackupRequester(
         backupRequester = this,
         requestedPackages = packages.size,
     )
-    private val monitor = BackupMonitor()
 
     /**
      * The current package index.

app/src/main/java/com/stevesoltys/seedvault/worker/WorkerModule.kt

@@ -13,6 +13,7 @@ val workerModule = module {
         BackupRequester(
             context = androidContext(),
             backupManager = get(),
+            monitor = get(),
             packageService = get(),
         )
     }

app/src/test/java/com/stevesoltys/seedvault/repo/SnapshotCreatorTest.kt

@@ -12,12 +12,19 @@ import android.content.pm.ApplicationInfo.FLAG_SYSTEM
 import android.content.pm.ResolveInfo
 import androidx.test.core.app.ApplicationProvider
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
 import com.stevesoltys.seedvault.TestApp
 import com.stevesoltys.seedvault.header.VERSION
 import com.stevesoltys.seedvault.metadata.BackupType
+import com.stevesoltys.seedvault.metadata.BackupType.KV
 import com.stevesoltys.seedvault.proto.Snapshot
+import com.stevesoltys.seedvault.proto.SnapshotKt.apk
+import com.stevesoltys.seedvault.proto.SnapshotKt.app
+import com.stevesoltys.seedvault.proto.SnapshotKt.split
+import com.stevesoltys.seedvault.proto.copy
 import com.stevesoltys.seedvault.transport.TransportTest
 import com.stevesoltys.seedvault.transport.backup.PackageService
+import com.stevesoltys.seedvault.worker.BASE_SPLIT
 import io.mockk.Runs
 import io.mockk.every
 import io.mockk.just
@@ -25,6 +32,7 @@ import io.mockk.mockk
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
 import org.junit.Test
+import org.junit.jupiter.api.assertThrows
 import org.junit.runner.RunWith
 import org.robolectric.annotation.Config
 import kotlin.random.Random
@@ -40,12 +48,18 @@ internal class SnapshotCreatorTest : TransportTest() {
     private val packageService: PackageService = mockk()
     private val snapshotCreator = SnapshotCreator(ctx, clock, packageService, metadataManager)
 
+    init {
+        every { packageService.launchableSystemApps } returns emptyList()
+        every { metadataManager.onPackageBackedUp(pmPackageInfo, any(), any()) } just Runs
+    }
+
     @Test
     fun `test onApkBackedUp`() {
         every { applicationInfo.loadLabel(any()) } returns name
         every { clock.time() } returns token
 
         snapshotCreator.onApkBackedUp(packageInfo, apk, blobMap)
+        snapshotCreator.onPackageBackedUp(pmPackageInfo, KV, BackupData(emptyList(), emptyMap()))
         val s = snapshotCreator.finalizeSnapshot()
 
         assertEquals(apk, s.appsMap[packageName]?.apk)
@@ -72,6 +86,7 @@ internal class SnapshotCreatorTest : TransportTest() {
         every { packageService.launchableSystemApps } returns listOf(resolveInfo)
 
         snapshotCreator.onPackageBackedUp(packageInfo, BackupType.FULL, apkBackupData)
+        snapshotCreator.onPackageBackedUp(pmPackageInfo, KV, BackupData(emptyList(), emptyMap()))
         val s = snapshotCreator.finalizeSnapshot()
 
         assertEquals(name, s.appsMap[packageName]?.name)
@@ -94,14 +109,115 @@ internal class SnapshotCreatorTest : TransportTest() {
         every { packageService.launchableSystemApps } returns emptyList()
 
         snapshotCreator.onPackageBackedUp(packageInfo, BackupType.FULL, apkBackupData)
+        snapshotCreator.onPackageBackedUp(pmPackageInfo, KV, BackupData(emptyList(), emptyMap()))
         snapshotCreator.finalizeSnapshot()
     }
 
+    @Test
+    fun `test onNoDataInCurrentRun is no-op if no data in last snapshot`() {
+        snapshotCreator.onNoDataInCurrentRun(snapshot, MAGIC_PACKAGE_MANAGER)
+
+        every { clock.time() } returns token
+
+        // finalizing complains about not having @pm@
+        val e = assertThrows<IllegalStateException> {
+            snapshotCreator.finalizeSnapshot()
+        }
+        assertTrue(e.message?.contains(MAGIC_PACKAGE_MANAGER) == true)
+    }
+
+    @Test
+    fun `test onNoDataInCurrentRun doesn't overwrite existing data`() {
+        val snapshot1 = snapshot.copy {
+            apps[MAGIC_PACKAGE_MANAGER] = app {
+                system = true
+                type = Snapshot.BackupType.KV
+                size = 42L
+                chunkIds.addAll(listOf(chunkId1).forProto())
+            }
+            blobs.clear()
+            blobs[chunkId1] = blob1
+        }
+        val snapshot2 = snapshot.copy {
+            apps[MAGIC_PACKAGE_MANAGER] = app {
+                system = true
+                type = Snapshot.BackupType.KV
+                size = 1337L
+                chunkIds.addAll(listOf(chunkId2).forProto())
+            }
+            blobs.clear()
+            blobs[chunkId2] = blob2
+        }
+
+        every {
+            metadataManager.onPackageBackedUp(match {
+                it.packageName == MAGIC_PACKAGE_MANAGER
+            }, KV, 42L) // doesn't get run for size of snapshot2
+        } just Runs
+
+        // We just call the same method twice for ease of testing,
+        // but in reality, the existing data could come from other calls.
+        // Important is that existing data doesn't get replaced with data from old snapshots.
+        snapshotCreator.onNoDataInCurrentRun(snapshot1, MAGIC_PACKAGE_MANAGER)
+        snapshotCreator.onNoDataInCurrentRun(snapshot2, MAGIC_PACKAGE_MANAGER)
+
+        every { clock.time() } returns token
+
+        // finalizing includes @pm@ app and its blobs
+        snapshotCreator.finalizeSnapshot().also { s ->
+            // data from snapshot1 is used, not from snapshot2
+            assertEquals(snapshot1.appsMap[MAGIC_PACKAGE_MANAGER], s.appsMap[MAGIC_PACKAGE_MANAGER])
+            // only first blob is in map
+            assertEquals(1, s.blobsMap.size)
+            assertEquals(blob1, s.blobsMap[chunkId1])
+        }
+    }
+
+    @Test
+    fun `test onNoDataInCurrentRun`() {
+        val snapshot = snapshot.copy {
+            apps[MAGIC_PACKAGE_MANAGER] = app {
+                system = true
+                type = Snapshot.BackupType.KV
+                size = 42L
+                chunkIds.addAll(listOf(chunkId1).forProto())
+                apk = apk { // @pm@ doesn't have an APK, but we just add one for testing
+                    val split = split {
+                        this.name = BASE_SPLIT
+                        this.chunkIds.addAll(listOf(chunkId2).forProto())
+                    }
+                    splits.add(split)
+                }
+            }
+            blobs.clear()
+            blobs[chunkId1] = blob1
+            blobs[chunkId2] = blob2
+        }
+
+        every {
+            metadataManager.onPackageBackedUp(match {
+                it.packageName == MAGIC_PACKAGE_MANAGER
+            }, KV, 42L)
+        } just Runs
+
+        snapshotCreator.onNoDataInCurrentRun(snapshot, MAGIC_PACKAGE_MANAGER)
+
+        every { clock.time() } returns token
+
+        // finalizing includes @pm@ app and its blobs
+        snapshotCreator.finalizeSnapshot().also { s ->
+            assertEquals(snapshot.appsMap[MAGIC_PACKAGE_MANAGER], s.appsMap[MAGIC_PACKAGE_MANAGER])
+            assertEquals(blob1, s.blobsMap[chunkId1])
+            assertEquals(blob2, s.blobsMap[chunkId2])
+        }
+    }
+
     @Test
     fun `test onIconsBackedUp`() {
         every { clock.time() } returns token andThen token + 1
 
         snapshotCreator.onIconsBackedUp(apkBackupData)
+        snapshotCreator.onPackageBackedUp(pmPackageInfo, KV, BackupData(emptyList(), emptyMap()))
         val s = snapshotCreator.finalizeSnapshot()
 
         assertEquals(apkBackupData.chunkIds.forProto(), s.iconChunkIdsList)
@@ -112,6 +228,7 @@ internal class SnapshotCreatorTest : TransportTest() {
     fun `test finalize`() {
         every { clock.time() } returns token
 
+        snapshotCreator.onPackageBackedUp(pmPackageInfo, KV, BackupData(emptyList(), emptyMap()))
         val s = snapshotCreator.finalizeSnapshot()
 
         assertEquals(VERSION, s.version.toByte())
@@ -122,7 +239,7 @@ internal class SnapshotCreatorTest : TransportTest() {
         assertEquals(34, s.sdkInt) // as per config above, needs bump once possible
         assertEquals("unknown", s.androidIncremental)
         assertTrue(s.d2D)
-        assertEquals(0, s.appsCount)
+        assertEquals(1, s.appsCount)
         assertEquals(0, s.iconChunkIdsCount)
         assertEquals(emptyMap<String, Snapshot.Blob>(), s.blobsMap)
     }