[Sync framework] Fix sync always pending on Android 14+ (#1463)

* Android 14 and 15 workaround: tell the sync framework to cancel any pending syncs Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Add test which documents wrong pending sync check behaviour Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Exclude android 13 and below Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Cancel only own sync request Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Cancel only after enqueuing sync worker Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Move test to AndroidSyncFrameworkTest Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Reset master sync state Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Remove limited parallelism and increase test timeout Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Rename test method Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Add assert message Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Update comment Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Add sdk suppress annotation Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Use runBlocking to be able to catch the timeout exception Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Extract pending sync cancellation to method Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Enhance sync framework tests for Android versions 9 to 16, verifying correct and incorrect pending states Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Add tests for sync always pending behavior on Android 10 and 11 Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Remove obsolete unmockkAll call Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Make tests a bit more reliable Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Extract cancelSync method call to utils to stub the call more easily Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Remove some unnecessary calls and update stub Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Update expected states lists Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Move cancelSyncInSyncFramework to SyncFrameworkIntegration Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Pass the whole sync extras bundle when cancelling sync Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * [WIP] Initialize pending sync state reporting wrong behaviour Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Optimize SyncAdapterServicesTest * Remove unused property Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Reset master sync state Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Revert "Reset master sync state" This reverts commit 4bfe73a25a. * Revert "Remove unused property" This reverts commit 7c0fdbf392. * Reapply "Reset master sync state" This reverts commit 5f7f0f9bce. * Reapply "Remove unused property" This reverts commit f1d5009f8a. * Increase timeout to 2 min Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * [WIP] Optimize tests * Optimize sync framework tests * SyncAdapterServices FakeSyncAdapter: support interrupting --------- Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> Co-authored-by: Ricki Hirner <hirner@bitfire.at>
2025-12-23 23:17:50 -05:00 · 2025-07-23 13:03:02 +02:00
parent ed7a477d3f
commit 98c0b0c36a
4 changed files with 269 additions and 54 deletions
--- a/app/src/androidTest/kotlin/at/bitfire/davdroid/sync/AndroidSyncFrameworkTest.kt
+++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/sync/AndroidSyncFrameworkTest.kt
@@ -0,0 +1,196 @@
+/*
+ * Copyright © All Contributors. See LICENSE and AUTHORS in the root directory for details.
+ */
+
+package at.bitfire.davdroid.sync
+
+import android.accounts.Account
+import android.content.ContentResolver
+import android.content.Context
+import android.content.SyncRequest
+import android.os.Bundle
+import android.provider.CalendarContract
+import androidx.test.filters.SdkSuppress
+import at.bitfire.davdroid.sync.account.TestAccount
+import dagger.hilt.android.qualifiers.ApplicationContext
+import dagger.hilt.android.testing.HiltAndroidRule
+import dagger.hilt.android.testing.HiltAndroidTest
+import io.mockk.junit4.MockKRule
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.withTimeout
+import org.junit.After
+import org.junit.AfterClass
+import org.junit.Assert.assertEquals
+import org.junit.Before
+import org.junit.BeforeClass
+import org.junit.Rule
+import org.junit.Test
+import java.util.Collections
+import java.util.LinkedList
+import java.util.logging.Logger
+import javax.inject.Inject
+import kotlin.time.Duration.Companion.seconds
+
+@HiltAndroidTest
+class AndroidSyncFrameworkTest {
+
+    @get:Rule
+    val hiltRule = HiltAndroidRule(this)
+
+    @get:Rule
+    val mockkRule = MockKRule(this)
+
+    @Inject
+    @ApplicationContext
+    lateinit var context: Context
+
+    @Inject
+    lateinit var logger: Logger
+
+    lateinit var account: Account
+    val authority = CalendarContract.AUTHORITY
+
+    private lateinit var stateChangeListener: Any
+    private val recordedStates = Collections.synchronizedList(LinkedList<State>())
+
+    @Before
+    fun setUp() {
+        hiltRule.inject()
+
+        account = TestAccount.create()
+
+        // Enable sync globally and for the test account
+        ContentResolver.setIsSyncable(account, authority, 1)
+
+        // Remember states the sync framework reports as pairs of (sync pending, sync active).
+        recordedStates.clear()
+        onStatusChanged(0)      // record first entry (pending = false, active = false)
+        stateChangeListener = ContentResolver.addStatusChangeListener(
+            ContentResolver.SYNC_OBSERVER_TYPE_PENDING or ContentResolver.SYNC_OBSERVER_TYPE_ACTIVE,
+            ::onStatusChanged
+        )
+    }
+
+    @After
+    fun tearDown() {
+        ContentResolver.removeStatusChangeListener(stateChangeListener)
+        TestAccount.remove(account)
+    }
+
+
+    /**
+     * Correct behaviour of the sync framework on Android 13 and below.
+     * Pending state is correctly reflected
+     */
+    @SdkSuppress(maxSdkVersion = 33)
+    @Test
+    fun testVerifySyncAlwaysPending_correctBehaviour_android13() {
+        assertSyncStates(
+            listOf(
+                State(pending = false, active = false),     // no sync pending or active
+                State(pending = true, active = false),      // sync becomes pending
+                State(pending = true, active = true),       // ... and pending and active at the same time
+                State(pending = false, active = true),      // ... and then only active
+                State(pending = false, active = false)      // sync finished
+            )
+        )
+    }
+
+    /**
+     * Wrong behaviour of the sync framework on Android 14+.
+     * Pending state stays true forever (after initial run), active state behaves correctly
+     */
+    @SdkSuppress(minSdkVersion = 34 /*, maxSdkVersion = 36 */)
+    @Test
+    fun testVerifySyncAlwaysPending_wrongBehaviour_android14() {
+        assertSyncStates(
+            listOf(
+                State(pending = false, active = false),     // no sync pending or active
+                State(pending = true, active = false),      // sync becomes pending
+                State(pending = true, active = true),       // ... and pending and active at the same time
+                State(pending = true, active = false)       // ... and finishes, but stays pending
+            )
+        )
+    }
+
+
+    // helpers
+
+    private fun syncRequest() = SyncRequest.Builder()
+        .setSyncAdapter(account, authority)
+        .syncOnce()
+        .setExtras(Bundle())    // needed for Android 9
+        .setExpedited(true)     // sync request will be scheduled at the front of the sync request queue
+        .setManual(true)        // equivalent of setting both SYNC_EXTRAS_IGNORE_SETTINGS and SYNC_EXTRAS_IGNORE_BACKOFF
+        .build()
+
+    /**
+     * Verifies that the given expected states match the recorded states.
+     */
+    private fun assertSyncStates(expectedStates: List<State>) = runBlocking {
+        // We use runBlocking for these tests because it uses the default dispatcher
+        // which does not auto-advance virtual time and we need real system time to
+        // test the sync framework behavior.
+
+        ContentResolver.requestSync(syncRequest())
+
+        // Even though the always-pending-bug is present on Android 14+, the sync active
+        // state behaves correctly, so we can record the state changes as pairs (pending,
+        // active) and expect a certain sequence of state pairs to verify the presence or
+        // absence of the bug on different Android versions.
+        withTimeout(60.seconds) { // Usually takes less than 30 seconds
+            while (recordedStates.size < expectedStates.size) {
+                // verify already known states
+                if (recordedStates.isNotEmpty())
+                    assertEquals(expectedStates.subList(0, recordedStates.size), recordedStates)
+
+                delay(500) // avoid busy-waiting
+            }
+
+            assertEquals(expectedStates, recordedStates)
+        }
+    }
+
+
+    fun onStatusChanged(which: Int) {
+        val state = State(
+            pending = ContentResolver.isSyncPending(account, authority),
+            active = ContentResolver.isSyncActive(account, authority)
+        )
+        synchronized(recordedStates) {
+            if (recordedStates.lastOrNull() != state) {
+                logger.info("$account syncState = $state")
+                recordedStates += state
+            }
+        }
+    }
+
+    data class State(
+        val pending: Boolean,
+        val active: Boolean
+    )
+
+
+    companion object {
+
+        var globalAutoSyncBeforeTest = false
+
+        @BeforeClass
+        @JvmStatic
+        fun before() {
+            globalAutoSyncBeforeTest = ContentResolver.getMasterSyncAutomatically()
+
+            // We'll request syncs explicitly and with SYNC_EXTRAS_IGNORE_SETTINGS
+            ContentResolver.setMasterSyncAutomatically(false)
+        }
+
+        @AfterClass
+        @JvmStatic
+        fun after() {
+            ContentResolver.setMasterSyncAutomatically(globalAutoSyncBeforeTest)
+        }
+
+    }
+
+}
--- a/app/src/androidTest/kotlin/at/bitfire/davdroid/sync/SyncAdapterServicesTest.kt
+++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/sync/SyncAdapterServicesTest.kt
@@ -5,6 +5,7 @@
 package at.bitfire.davdroid.sync

 import android.accounts.Account
+import android.content.ContentResolver
 import android.content.Context
 import android.content.SyncResult
 import android.os.Bundle
@@ -13,17 +14,16 @@ import androidx.hilt.work.HiltWorkerFactory
 import androidx.work.WorkInfo
 import androidx.work.WorkManager
 import at.bitfire.davdroid.TestUtils
-import at.bitfire.davdroid.repository.DavCollectionRepository
-import at.bitfire.davdroid.repository.DavServiceRepository
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.davdroid.sync.account.TestAccount
 import at.bitfire.davdroid.sync.worker.SyncWorkerManager
 import dagger.hilt.android.qualifiers.ApplicationContext
+import dagger.hilt.android.testing.BindValue
 import dagger.hilt.android.testing.HiltAndroidRule
 import dagger.hilt.android.testing.HiltAndroidTest
 import io.mockk.Awaits
 import io.mockk.coEvery
 import io.mockk.every
+import io.mockk.impl.annotations.MockK
 import io.mockk.junit4.MockKRule
 import io.mockk.just
 import io.mockk.mockk
@@ -39,47 +39,34 @@ import org.junit.After
 import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
-import org.junit.rules.Timeout
-import java.util.logging.Logger
 import javax.inject.Inject
+import javax.inject.Provider
 import kotlin.coroutines.cancellation.CancellationException

@HiltAndroidTest
 class SyncAdapterServicesTest {

-    lateinit var account: Account
-
-    @Inject
-    lateinit var accountSettingsFactory: AccountSettings.Factory
-
-    @Inject
-    lateinit var collectionRepository: DavCollectionRepository
-
-    @Inject @ApplicationContext
-    lateinit var context: Context
-
-    @Inject
-    lateinit var logger: Logger
-
-    @Inject
-    lateinit var serviceRepository: DavServiceRepository
-
-    @Inject
-    lateinit var syncConditionsFactory: SyncConditions.Factory
-
-    @Inject
-    lateinit var workerFactory: HiltWorkerFactory
-
    @get:Rule
    val hiltRule = HiltAndroidRule(this)

    @get:Rule
    val mockkRule = MockKRule(this)

-    // test methods should run quickly and not wait 60 seconds for a sync timeout or something like that
-    @get:Rule
-    val timeoutRule: Timeout = Timeout.seconds(5)
+    @Inject @ApplicationContext
+    lateinit var context: Context

+    @Inject
+    lateinit var syncAdapterProvider: Provider<SyncAdapterService.SyncAdapter>
+
+    @BindValue @MockK
+    lateinit var syncWorkerManager: SyncWorkerManager
+
+    @Inject
+    lateinit var workerFactory: HiltWorkerFactory
+
+    lateinit var account: Account
+
+    private var masterSyncStateBeforeTest = ContentResolver.getMasterSyncAutomatically()

    @Before
    fun setUp() {
@@ -87,33 +74,23 @@ class SyncAdapterServicesTest {
        TestUtils.setUpWorkManager(context, workerFactory)

        account = TestAccount.create()
+
+        ContentResolver.setMasterSyncAutomatically(true)
+        ContentResolver.setSyncAutomatically(account, CalendarContract.AUTHORITY, true)
+        ContentResolver.setIsSyncable(account, CalendarContract.AUTHORITY, 1)
    }

    @After
    fun tearDown() {
+        ContentResolver.setMasterSyncAutomatically(masterSyncStateBeforeTest)
        TestAccount.remove(account)
    }


-    private fun syncAdapter(
-        syncWorkerManager: SyncWorkerManager
-    ): SyncAdapterService.SyncAdapter =
-        SyncAdapterService.SyncAdapter(
-            accountSettingsFactory = accountSettingsFactory,
-            collectionRepository = collectionRepository,
-            serviceRepository = serviceRepository,
-            context = context,
-            logger = logger,
-            syncConditionsFactory = syncConditionsFactory,
-            syncWorkerManager = syncWorkerManager
-        )
-
-
    @Test
    fun testSyncAdapter_onPerformSync_cancellation() = runTest {
-        val syncWorkerManager = mockk<SyncWorkerManager>()
-        val syncAdapter = syncAdapter(syncWorkerManager = syncWorkerManager)
        val workManager = WorkManager.getInstance(context)
+        val syncAdapter = syncAdapterProvider.get()

        mockkObject(workManager) {
            // don't actually create a worker
@@ -136,9 +113,8 @@ class SyncAdapterServicesTest {

    @Test
    fun testSyncAdapter_onPerformSync_returnsAfterTimeout() {
-        val syncWorkerManager = mockk<SyncWorkerManager>()
-        val syncAdapter = syncAdapter(syncWorkerManager = syncWorkerManager)
        val workManager = WorkManager.getInstance(context)
+        val syncAdapter = syncAdapterProvider.get()

        mockkObject(workManager) {
            // don't actually create a worker
@@ -158,9 +134,8 @@ class SyncAdapterServicesTest {

    @Test
    fun testSyncAdapter_onPerformSync_runsInTime() {
-        val syncWorkerManager = mockk<SyncWorkerManager>()
-        val syncAdapter = syncAdapter(syncWorkerManager = syncWorkerManager)
        val workManager = WorkManager.getInstance(context)
+        val syncAdapter = syncAdapterProvider.get()

        mockkObject(workManager) {
            // don't actually create a worker
--- a/app/src/main/kotlin/at/bitfire/davdroid/sync/SyncAdapterServices.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/sync/SyncAdapterServices.kt
@@ -13,6 +13,7 @@ import android.content.ContentResolver
 import android.content.Context
 import android.content.Intent
 import android.content.SyncResult
+import android.os.Build
 import android.os.Bundle
 import android.os.IBinder
 import androidx.work.WorkInfo
@@ -57,16 +58,25 @@ abstract class SyncAdapterService: Service() {
    override fun onBind(intent: Intent?): IBinder {
        if (BuildConfig.DEBUG && !syncActive.get()) {
            // only for debug builds/testing: syncActive flag
-            val logger = Logger.getLogger(this@SyncAdapterService::class.java.name)
-            logger.log(Level.WARNING, "SyncAdapterService.onBind() was called but syncActive = false. Ignoring")
+            val logger = Logger.getLogger(SyncAdapterService::class.java.name)
+            logger.warning("SyncAdapterService.onBind() was called but syncActive = false. Returning fake sync adapter")

-            val fakeAdapter = object: AbstractThreadedSyncAdapter(this, false) {
+            val fakeAdapter = object: AbstractThreadedSyncAdapter(this, true) {
                override fun onPerformSync(account: Account, extras: Bundle, authority: String, provider: ContentProviderClient, syncResult: SyncResult) {
                    val message = StringBuilder()
                    message.append("FakeSyncAdapter onPerformSync(account=$account, extras=$extras, authority=$authority, syncResult=$syncResult)")
                    for (key in extras.keySet())
                        message.append("\n\textras[$key] = ${extras[key]}")
                    logger.warning(message.toString())
+
+                    // fake 5 sec sync
+                    try {
+                        Thread.sleep(5000)
+                    } catch (_: InterruptedException) {
+                        logger.warning("FakeSyncAdapter onPerformSync($account) cancelled")
+                    }
+
+                    logger.warning("FakeSyncAdapter onPerformSync($account) finished")
                }
            }
            return fakeAdapter.syncAdapterBinder
@@ -105,6 +115,7 @@ abstract class SyncAdapterService: Service() {
        @ApplicationContext context: Context,
        private val logger: Logger,
        private val syncConditionsFactory: SyncConditions.Factory,
+        private val syncFrameworkIntegration: SyncFrameworkIntegration,
        private val syncWorkerManager: SyncWorkerManager
    ): AbstractThreadedSyncAdapter(
        /* context = */ context,
@@ -160,6 +171,14 @@ abstract class SyncAdapterService: Service() {
            logger.fine("Starting OneTimeSyncWorker for $account $authority and waiting for it")
            val workerName = syncWorkerManager.enqueueOneTime(account, dataType = SyncDataType.fromAuthority(authority), fromUpload = upload)

+            // Android 14+ does not handle pending sync state correctly.
+            // As a defensive workaround, we can cancel specifically this still pending sync only
+            // See: https://github.com/bitfireAT/davx5-ose/issues/1458
+            if (Build.VERSION.SDK_INT >= 34) {
+                logger.fine("Android 14+ bug: Canceling forever pending sync adapter framework sync request for " +
+                        "account=$account authority=$authority upload=$upload")
+                syncFrameworkIntegration.cancelSync(account, authority, extras)
+            }

            /* Because we are not allowed to observe worker state on a background thread, we can not
            use it to block the sync adapter. Instead we use a Flow to get notified when the sync
--- a/app/src/main/kotlin/at/bitfire/davdroid/sync/SyncFrameworkIntegration.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/sync/SyncFrameworkIntegration.kt
@@ -6,7 +6,9 @@ package at.bitfire.davdroid.sync

 import android.accounts.Account
 import android.content.ContentResolver
+import android.content.SyncRequest
 import android.os.Build
+import android.os.Bundle
 import android.provider.CalendarContract
 import androidx.annotation.WorkerThread
 import at.bitfire.davdroid.resource.LocalAddressBookStore
@@ -94,6 +96,29 @@ class SyncFrameworkIntegration @Inject constructor(
            setSyncOnContentChange(account, authority, false)
    }

+    /**
+     * Cancels the sync request in the Sync Framework for Android 14+.
+     * This is a workaround for the bug that the sync framework does not handle pending syncs correctly
+     * on Android 14+ (API level 34+).
+     *
+     * See: https://github.com/bitfireAT/davx5-ose/issues/1458
+     *
+     * @param account The account for which the sync request should be canceled.
+     * @param authority The authority for which the sync request should be canceled.
+     * @param extras The original extras Bundle used to start the sync.
+     */
+    fun cancelSync(account: Account, authority: String, extras: Bundle) {
+        // Recreate the sync request which was used to start this sync
+        val syncRequest = SyncRequest.Builder()
+            .setSyncAdapter(account, authority)
+            .setExtras(extras)
+            .syncOnce()
+            .build()
+
+        // Cancel it
+        ContentResolver.cancelSync(syncRequest)
+    }
+
    /**
     * Enables/disables sync adapter automatic sync (content triggered sync) for the given
     * account and authority. Does *not* call [ContentResolver.setIsSyncable].