From e8c4fc1ea14ab50474a3bb9de27539ff3e9031ad Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 02:27:33 -0500 Subject: [PATCH] fix(sync): re-enqueue periodic work with UPDATE so upgrades re-apply the schedule Periodic sync, backfill, and prune were enqueued with ExistingPeriodicWorkPolicy.KEEP, so a newer app version's interval or constraint change never reached already-installed devices: KEEP pins the job to the spec from whichever version first scheduled it. Switch the three periodic schedulers to UPDATE (WorkManager 2.8+; 2.11.2 in use), which re-applies the current spec on each app-start re-enqueue while preserving the running period's progress. An unchanged spec is effectively a no-op, so this never resets the schedule on launch the way REPLACE (cancel + re-enqueue) would. The one-shot kicks (syncNow/backfillNow/pruneNow) keep their existing policies -- they are a separate concern from #96. SyncScheduler now injects Provider (via a new WorkManagerModule) instead of calling the WorkManager.getInstance() static directly, so the policy is unit-testable with MockK; the Provider keeps resolution lazy to preserve the previous initialization timing. Co-Authored-By: Claude Fable 5 --- .../ui/settings/AccountSettingsScreenTest.kt | 4 +- .../ui/settings/SettingsScreenTest.kt | 4 +- .../org/libremail/data/sync/SyncScheduler.kt | 28 +++-- .../org/libremail/di/WorkManagerModule.kt | 24 ++++ .../kotlin/org/libremail/push/IdleService.kt | 5 +- .../libremail/data/sync/SyncSchedulerTest.kt | 107 ++++++++++++++++++ 6 files changed, 160 insertions(+), 12 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt create mode 100644 app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt index 36b3ed2..20e1173 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt @@ -10,6 +10,7 @@ import androidx.lifecycle.SavedStateHandle import androidx.room.Room import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.work.WorkManager import kotlinx.coroutines.runBlocking import org.junit.Rule import org.junit.Test @@ -27,6 +28,7 @@ import org.libremail.domain.model.ServerConfig import org.libremail.ui.FakeAccountRepository import org.libremail.ui.navigation.Routes import org.libremail.ui.theme.LibreMailTheme +import javax.inject.Provider /** * End-to-end test for the per-account settings screen: editing the signature and toggling the @@ -68,7 +70,7 @@ class AccountSettingsScreenTest { accountRepository = FakeAccountRepository(accounts = listOf(account)), accountSettingsRepository = repository, signatureRepository = SignatureRepository(db.signatureDao()), - syncScheduler = SyncScheduler(context), + syncScheduler = SyncScheduler(Provider { WorkManager.getInstance(context) }), ) composeTestRule.setContent { LibreMailTheme(darkTheme = false, dynamicColor = false) { diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt index 84d2832..4db2142 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt @@ -8,6 +8,7 @@ import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollTo import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry +import androidx.work.WorkManager import kotlinx.coroutines.runBlocking import org.junit.Rule import org.junit.Test @@ -25,6 +26,7 @@ import org.libremail.data.sync.SyncScheduler import org.libremail.push.BatteryOptimizationManager import org.libremail.ui.FakeAccountRepository import org.libremail.ui.theme.LibreMailTheme +import javax.inject.Provider /** * End-to-end test for the top-level message-downloading setting: tapping a policy must round-trip @@ -54,7 +56,7 @@ class SettingsScreenTest { appLockManager, keyStore, BatteryOptimizationManager(context), - SyncScheduler(context), + SyncScheduler(Provider { WorkManager.getInstance(context) }), ) composeTestRule.setContent { diff --git a/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt b/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt index 5cd91b4..d23806f 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt @@ -1,7 +1,6 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.sync -import android.content.Context import androidx.work.Constraints import androidx.work.ExistingPeriodicWorkPolicy import androidx.work.ExistingWorkPolicy @@ -10,15 +9,20 @@ import androidx.work.OneTimeWorkRequestBuilder import androidx.work.OutOfQuotaPolicy import androidx.work.PeriodicWorkRequestBuilder import androidx.work.WorkManager -import dagger.hilt.android.qualifiers.ApplicationContext import java.util.concurrent.TimeUnit import javax.inject.Inject +import javax.inject.Provider import javax.inject.Singleton /** Schedules background mail sync, full-history backfill, and retention pruning via WorkManager. */ @Singleton -class SyncScheduler @Inject constructor(@ApplicationContext private val context: Context) { - private val workManager get() = WorkManager.getInstance(context) +class SyncScheduler @Inject constructor( + // A Provider (not the WorkManager itself) so WorkManager.getInstance() is resolved lazily at + // schedule time — never during Hilt's Application field injection, which can run before the + // HiltWorkerFactory that on-demand WorkManager initialization needs is set. + private val workManagerProvider: Provider, +) { + private val workManager get() = workManagerProvider.get() private val networkConstraint = Constraints.Builder() .setRequiredNetworkType(NetworkType.CONNECTED) @@ -41,7 +45,7 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: val request = PeriodicWorkRequestBuilder(15, TimeUnit.MINUTES) .setConstraints(networkConstraint) .build() - workManager.enqueueUniquePeriodicWork(PERIODIC_WORK, ExistingPeriodicWorkPolicy.KEEP, request) + workManager.enqueueUniquePeriodicWork(PERIODIC_WORK, PERIODIC_POLICY, request) } /** One-shot sync, e.g. right after an account is added. */ @@ -55,13 +59,13 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: /** * Periodic full-history backfill (issue #12). Each run pages a bounded slice and persists its - * boundary, so history fills in over successive runs; KEEP preserves an already-scheduled cadence. + * boundary, so history fills in over successive runs. */ fun schedulePeriodicBackfill() { val request = PeriodicWorkRequestBuilder(30, TimeUnit.MINUTES) .setConstraints(backfillConstraint) .build() - workManager.enqueueUniquePeriodicWork(PERIODIC_BACKFILL, ExistingPeriodicWorkPolicy.KEEP, request) + workManager.enqueueUniquePeriodicWork(PERIODIC_BACKFILL, PERIODIC_POLICY, request) } /** Kicks an immediate backfill slice (e.g. just after an account is added) without waiting for the cadence. */ @@ -79,7 +83,7 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: val request = PeriodicWorkRequestBuilder(12, TimeUnit.HOURS) .setConstraints(pruneConstraint) .build() - workManager.enqueueUniquePeriodicWork(PERIODIC_PRUNE, ExistingPeriodicWorkPolicy.KEEP, request) + workManager.enqueueUniquePeriodicWork(PERIODIC_PRUNE, PERIODIC_POLICY, request) } /** Runs pruning promptly, e.g. right after the user tightens a retention limit. */ @@ -95,5 +99,13 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: const val ONESHOT_BACKFILL = "libremail_oneshot_backfill" const val PERIODIC_PRUNE = "libremail_periodic_prune" const val ONESHOT_PRUNE = "libremail_oneshot_prune" + + // UPDATE, not KEEP (issue #96). These periodic jobs are re-enqueued at every app start, so KEEP + // pinned an already-installed device to the interval/constraints from the version that first + // scheduled it — later tuning never reached upgraders. UPDATE re-applies the current spec while + // preserving the running period's progress: an unchanged spec is effectively a no-op, so this + // does NOT reset the schedule on launch the way REPLACE (cancel + re-enqueue) would. UPDATE is + // available since WorkManager 2.8. + val PERIODIC_POLICY = ExistingPeriodicWorkPolicy.UPDATE } } diff --git a/app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt b/app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt new file mode 100644 index 0000000..5df1c41 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.di + +import android.content.Context +import androidx.work.WorkManager +import dagger.Module +import dagger.Provides +import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext +import dagger.hilt.components.SingletonComponent +import javax.inject.Singleton + +/** + * Provides the process-wide [WorkManager] so schedulers can inject it (and unit tests can substitute + * a fake) instead of reaching for the [WorkManager.getInstance] static directly. + */ +@Module +@InstallIn(SingletonComponent::class) +object WorkManagerModule { + + @Provides + @Singleton + fun provideWorkManager(@ApplicationContext context: Context): WorkManager = WorkManager.getInstance(context) +} diff --git a/app/src/main/kotlin/org/libremail/push/IdleService.kt b/app/src/main/kotlin/org/libremail/push/IdleService.kt index a65307c..336dee2 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -134,8 +134,9 @@ class IdleService : Service() { startAsForeground(mode) if (mode == PushMode.POLLING) { AppLog.i(TAG, "Battery low: pausing IMAP IDLE; mail arrives via 15-minute periodic sync") - // The periodic fallback is scheduled at every app start with KEEP, so this is normally a - // no-op — re-asserted here so the fallback provably exists whenever push is paused. + // The periodic fallback is already scheduled at every app start (UPDATE), so re-asserting it + // here is effectively a no-op — done anyway so the fallback provably exists whenever push is + // paused, without disturbing the running period. syncScheduler.schedulePeriodicSync() } else { AppLog.i(TAG, "Battery recovered: resuming IMAP IDLE push") diff --git a/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt new file mode 100644 index 0000000..24092ed --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import androidx.work.ExistingPeriodicWorkPolicy +import androidx.work.ExistingWorkPolicy +import androidx.work.OneTimeWorkRequest +import androidx.work.PeriodicWorkRequest +import androidx.work.WorkManager +import io.mockk.mockk +import io.mockk.verify +import org.junit.Test +import javax.inject.Provider + +/** + * The enqueue methods are thin wrappers over WorkManager, so these tests pin the one thing that carries + * behaviour: the existing-work policy each unique job is enqueued with. + */ +class SyncSchedulerTest { + + private val workManager = mockk(relaxed = true) + private val scheduler = SyncScheduler(Provider { workManager }) + + // Issue #96: periodic jobs must re-enqueue with UPDATE (not KEEP). At every app start KEEP would pin + // an already-installed device to the interval/constraints of whichever version first scheduled the + // job, so cadence/constraint tuning never reached upgraders; UPDATE re-applies the current spec. + + @Test + fun `periodic sync is enqueued with UPDATE so upgrades re-apply its schedule`() { + scheduler.schedulePeriodicSync() + + verify { + workManager.enqueueUniquePeriodicWork( + "libremail_periodic_sync", + ExistingPeriodicWorkPolicy.UPDATE, + any(), + ) + } + } + + @Test + fun `periodic backfill is enqueued with UPDATE so upgrades re-apply its schedule`() { + scheduler.schedulePeriodicBackfill() + + verify { + workManager.enqueueUniquePeriodicWork( + "libremail_periodic_backfill", + ExistingPeriodicWorkPolicy.UPDATE, + any(), + ) + } + } + + @Test + fun `periodic prune is enqueued with UPDATE so upgrades re-apply its schedule`() { + scheduler.schedulePeriodicPrune() + + verify { + workManager.enqueueUniquePeriodicWork( + "libremail_periodic_prune", + ExistingPeriodicWorkPolicy.UPDATE, + any(), + ) + } + } + + // The one-shot kicks are a separate concern from #96 and keep their existing policies: syncNow and + // pruneNow REPLACE for a clean immediate attempt; backfillNow KEEPs an in-flight all-account run. + + @Test + fun `syncNow replaces any pending one-shot sync`() { + scheduler.syncNow() + + verify { + workManager.enqueueUniqueWork( + "libremail_oneshot_sync", + ExistingWorkPolicy.REPLACE, + any(), + ) + } + } + + @Test + fun `backfillNow keeps an already-running backfill`() { + scheduler.backfillNow() + + verify { + workManager.enqueueUniqueWork( + "libremail_oneshot_backfill", + ExistingWorkPolicy.KEEP, + any(), + ) + } + } + + @Test + fun `pruneNow replaces any pending one-shot prune`() { + scheduler.pruneNow() + + verify { + workManager.enqueueUniqueWork( + "libremail_oneshot_prune", + ExistingWorkPolicy.REPLACE, + any(), + ) + } + } +}