diff --git a/app/src/main/kotlin/dev/privacyllc/period/PeriodApplication.kt b/app/src/main/kotlin/dev/privacyllc/period/PeriodApplication.kt index bc3224a..fdc2dbb 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/PeriodApplication.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/PeriodApplication.kt @@ -4,6 +4,10 @@ import android.app.Application import androidx.hilt.work.HiltWorkerFactory import androidx.work.Configuration import dagger.hilt.android.HiltAndroidApp +import kotlinx.coroutines.CoroutineExceptionHandler +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob import javax.inject.Inject /** @@ -31,8 +35,7 @@ class PeriodApplication : Application(), Configuration.Provider { * against the old date is wrong from that moment. Tying this to a ViewModel * would mean it only ran while somebody was looking. */ - private val applicationScope = - kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.SupervisorJob() + kotlinx.coroutines.Dispatchers.Default) + private val applicationScope = applicationScope() override fun onCreate() { super.onCreate() @@ -47,4 +50,45 @@ class PeriodApplication : Application(), Configuration.Provider { // is the kind nobody sees in testing. .setMinimumLoggingLevel(android.util.Log.WARN) .build() + + internal companion object { + + /** + * The process-long scope, and the handler that keeps a throw from ending + * the process. + * + * `SupervisorJob` is the trap this exists for. It stops a failing child + * cancelling its siblings, and it is easy to read that as "failures are + * contained here". It is not: an uncaught throw inside `launch` still + * goes to the [CoroutineExceptionHandler] in the context, and with none + * present it reaches the thread's default handler and takes the whole + * process down. + * + * That matters more here than anywhere else in the app, because this is + * the one scope that runs with **nobody watching**. `onCreate` runs in + * every process, including the ones WorkManager starts after a reboot + * and at the daily reminder — no Activity, no screen, nothing to show an + * error. The symptom a user gets is reminders that stopped. + * + * Both ViewModels already install a handler; `PrivacyViewModel` records + * the lesson in its own KDoc, learned by tapping a button twice. This is + * the same lesson applied to the scope where it costs the most. + * + * ## Why the handler body is empty + * + * It cannot log. `checkNoHealthLogging` forbids logging calls in every + * source file under `app`, and that rule is right here in particular: + * a `Prediction` invariant failure is a plausible cause, and an + * exception message on this path can carry a date derived from + * someone's cycle. So this is a last-resort backstop whose entire job + * is that the process survives. + * + * The place that degrades *usefully* is `ReminderCoordinator`, which + * catches per-flow so one failing collection does not silently take the + * other down with it. + */ + fun applicationScope(): CoroutineScope = CoroutineScope( + SupervisorJob() + Dispatchers.Default + CoroutineExceptionHandler { _, _ -> }, + ) + } } diff --git a/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt b/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt index 57e42e4..6032840 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt @@ -1,9 +1,12 @@ package dev.privacyllc.period.notifications import dev.privacyllc.period.core.data.CycleRepository +import dev.privacyllc.period.core.datastore.UserPreferences import dev.privacyllc.period.core.datastore.UserPreferencesRepository import dev.privacyllc.period.core.notifications.ReminderScheduler import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.launchIn @@ -37,30 +40,84 @@ class ReminderCoordinator @Inject constructor( /** * Watch for anything that changes what a reminder should say or when. * + * The two chains are built by [scheduleUpdates] and [checkInResets] rather + * than inline, so a test can hand them a flow that throws. Without that + * seam the only way to reach the `catch` below is a repository failure, + * and `CycleRepository` is final with an internal constructor — which is + * the right shape for a data boundary and the wrong shape for faking. + */ + fun start(scope: CoroutineScope) { + scheduleUpdates( + forecastDates = repository.forecast.map { it?.mostLikelyStartDate }, + preferences = preferences.preferences, + ).launchIn(scope) + + checkInResets( + confirmedStarts = repository.confirmedPeriods + .map { periods -> periods.maxOfOrNull { it.startDate } }, + ).launchIn(scope) + } + + /** + * Reschedule when the forecast date, the reminder time, or whether anything + * is enabled at all changes. + * * `distinctUntilChanged` on a small key rather than on the whole state: the * forecast object is recreated on every recalculation even when it says the * same thing, and rescheduling on each of those would churn WorkManager for * no reason. + * + * ## Why this catches, and why it does nothing when it does + * + * This is collected on the process-long scope in `PeriodApplication`, in + * processes started by WorkManager with no Activity in existence. Left + * uncaught, a throw here ends the process — see that class's KDoc. + * + * Doing nothing is the deliberate choice rather than the lazy one. The + * alternative — cancelling the scheduled work — would turn a failed *read* + * into reminders silently switched off, and they would stay off until the + * user next touched a notification setting, because those two places are + * the only callers of the scheduler. Leaving the existing schedule alone + * means the worker runs, finds it cannot say anything, and says nothing; + * that is already its behaviour with no history. + * + * It cannot log the failure either: `checkNoHealthLogging` covers the `app` module, + * and the likeliest throw on this path is a `Prediction` invariant failure + * whose message is derived from cycle data. + * + * The flow terminates after a catch, so this degrades for the life of the + * process rather than retrying. That is correct for the failure actually + * expected here — an invariant violation is deterministic, and retrying it + * would spin. */ - fun start(scope: CoroutineScope) { + internal fun scheduleUpdates( + forecastDates: Flow, + preferences: Flow, + ): Flow<*> = combine( - repository.forecast.map { it?.mostLikelyStartDate }, - preferences.preferences.map { it.reminderTime to anyEnabled(it) }, + forecastDates, + preferences.map { it.reminderTime to anyEnabled(it) }, ) { forecastDate, (time, enabled) -> Triple(forecastDate, time, enabled) } .distinctUntilChanged() .onEach { (_, time, enabled) -> apply(time, enabled) } - .launchIn(scope) + .catch { } - // A confirmed period answers the question the check-ins were asking, so - // the count resets and the app is willing to ask again next cycle. - // Without this, somebody who ignored three check-ins once would never be - // asked again — the stopping rule would become permanent. - repository.confirmedPeriods - .map { periods -> periods.maxOfOrNull { it.startDate } } + /** + * A confirmed period answers the question the check-ins were asking, so the + * count resets and the app is willing to ask again next cycle. + * + * Without this, somebody who ignored three check-ins once would never be + * asked again — the stopping rule would become permanent. + * + * Catches for the same reason as [scheduleUpdates], and separately from it: + * one collection failing must not stop the other, which is the whole point + * of them being two chains rather than one. + */ + internal fun checkInResets(confirmedStarts: Flow): Flow<*> = + confirmedStarts .distinctUntilChanged() .onEach { latest -> if (latest != null) onPeriodConfirmed(latest) } - .launchIn(scope) - } + .catch { } private suspend fun apply(time: LocalTime, enabled: Boolean) { if (enabled) scheduler.schedule(time) else scheduler.cancel() @@ -70,7 +127,7 @@ class ReminderCoordinator @Inject constructor( preferences.resetCheckIns() } - private fun anyEnabled(p: dev.privacyllc.period.core.datastore.UserPreferences) = + private fun anyEnabled(p: UserPreferences) = p.periodApproachingEnabled || p.periodExpectedTodayEnabled || p.didItStartEnabled || p.periodEndCheckInEnabled || p.fertileWindowReminderEnabled || p.ovulationReminderEnabled } diff --git a/app/src/test/kotlin/dev/privacyllc/period/ApplicationScopeTest.kt b/app/src/test/kotlin/dev/privacyllc/period/ApplicationScopeTest.kt new file mode 100644 index 0000000..49a6aa7 --- /dev/null +++ b/app/src/test/kotlin/dev/privacyllc/period/ApplicationScopeTest.kt @@ -0,0 +1,90 @@ +package dev.privacyllc.period + +import kotlinx.coroutines.isActive +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference + +/** + * The application scope must survive a throw, because nobody is watching it. + * + * `PeriodApplication.onCreate` runs in **every** process, including the ones + * WorkManager starts after a reboot and at the daily reminder — no Activity, no + * screen, nothing to show an error. An uncaught throw there ends the process, + * and the only symptom a user gets is reminders that stopped. + * + * `SupervisorJob` is what makes this easy to get wrong: it stops a failing + * child cancelling its siblings, which reads like containment and is not. The + * exception still goes to the [kotlinx.coroutines.CoroutineExceptionHandler] in + * the context, and with none present it reaches the thread's default handler. + * + * These tests assert the behaviour rather than the shape. Checking that a + * handler is present in the context would pass over a handler that rethrows. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class ApplicationScopeTest { + + /** + * Installs a recording default handler for the duration, and restores the + * real one afterwards — an escaped exception in a test runner is otherwise + * charged to whichever test happens to be running next. + */ + private fun recordingUncaughtExceptions(block: (AtomicReference) -> T): T { + val original = Thread.getDefaultUncaughtExceptionHandler() + val seen = AtomicReference(null) + Thread.setDefaultUncaughtExceptionHandler { _, e -> seen.compareAndSet(null, e) } + return try { + block(seen) + } finally { + Thread.setDefaultUncaughtExceptionHandler(original) + } + } + + /** Long enough for a scope *without* a handler to have recorded the crash. */ + private fun awaitCrashOrGiveUp(seen: AtomicReference) { + val deadline = System.currentTimeMillis() + 500 + while (seen.get() == null && System.currentTimeMillis() < deadline) Thread.sleep(10) + } + + @Test + fun `a throwing coroutine does not reach the default uncaught handler`() = + recordingUncaughtExceptions { seen -> + val scope = PeriodApplication.applicationScope() + + // The realistic failure: the forecast flow runs the prediction + // engine, and Prediction's init block enforces its window + // invariants with require(). + val job = scope.launch { throw IllegalArgumentException("a prediction invariant failed") } + runBlocking { job.join() } + awaitCrashOrGiveUp(seen) + + assertNull( + "a throw on the application scope reached the default handler, " + + "which ends the process — in the background, with no UI", + seen.get(), + ) + assertTrue("the scope must outlive a failed child", scope.isActive) + } + + @Test + fun `the scope still accepts work after a child has failed`() = + recordingUncaughtExceptions { + val scope = PeriodApplication.applicationScope() + val ranAfterwards = AtomicBoolean(false) + + runBlocking { scope.launch { throw IllegalStateException("the database could not be opened") }.join() } + runBlocking { scope.launch { ranAfterwards.set(true) }.join() } + + // The two reminder collections are siblings on this scope. One + // failing must not take the other with it. + assertTrue("a sibling collection must survive", ranAfterwards.get()) + } +} diff --git a/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt b/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt new file mode 100644 index 0000000..4bacf65 --- /dev/null +++ b/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt @@ -0,0 +1,91 @@ +package dev.privacyllc.period.notifications + +import android.content.Context +import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import androidx.datastore.preferences.preferencesDataStoreFile +import androidx.test.core.app.ApplicationProvider +import dev.privacyllc.period.core.data.CycleData +import dev.privacyllc.period.core.datastore.UserPreferences +import dev.privacyllc.period.core.datastore.UserPreferencesRepository +import dev.privacyllc.period.core.notifications.ReminderScheduler +import dev.privacyllc.period.domain.prediction.PersonalPredictionEngine +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import java.time.Clock + +/** + * The two collections that run on the process-long scope must not throw. + * + * [ApplicationScopeTest][dev.privacyllc.period.ApplicationScopeTest] covers the + * backstop — that the scope survives if something does escape. This covers the + * layer above it: each chain catches its own failure, so one collection dying + * does not silently take the other with it. + * + * The chains are built by `internal` functions taking flows rather than read + * from the repository inline, purely so this test can hand them a flow that + * throws. `CycleRepository` is final with an internal constructor — the right + * shape for a data boundary and the wrong shape for faking — and adding a + * mocking library to reach one `catch` would be a poor trade. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class ReminderCoordinatorTest { + + private lateinit var coordinator: ReminderCoordinator + + @Before fun setUp() { + val context = ApplicationProvider.getApplicationContext() + coordinator = ReminderCoordinator( + repository = CycleData.repository(context, PersonalPredictionEngine(), Clock.systemUTC()), + preferences = UserPreferencesRepository( + PreferenceDataStoreFactory.create { context.preferencesDataStoreFile("coordinator_test") }, + ), + scheduler = ReminderScheduler(context), + ) + } + + /** + * The realistic failure. `repository.forecast` runs the prediction engine + * inside the flow, and `Prediction`'s init block enforces its window + * invariants with `require` — so a bad engine result throws here rather + * than returning something wrong. + */ + @Test fun `a throwing forecast does not escape the schedule chain`() = runBlocking { + coordinator.scheduleUpdates( + forecastDates = flow { throw IllegalArgumentException("a prediction invariant failed") }, + preferences = flowOf(UserPreferences.Defaults), + ).collect { } + } + + @Test fun `a throwing preferences flow does not escape the schedule chain`() = runBlocking { + coordinator.scheduleUpdates( + forecastDates = flowOf(null), + preferences = flow { throw IllegalStateException("the preferences file is unreadable") }, + ).collect { } + } + + @Test fun `a throwing history flow does not escape the check-in chain`() = runBlocking { + coordinator.checkInResets( + confirmedStarts = flow { throw IllegalStateException("the database could not be opened") }, + ).collect { } + } + + /** + * The positive control, so a `catch` that swallowed everything would not + * pass the three tests above by accident. A null latest start is the + * no-history case, which reaches the collector without touching the + * scheduler or the preferences store. + */ + @Test fun `a healthy flow still reaches the collector`() = runBlocking { + var emissions = 0 + coordinator.checkInResets(confirmedStarts = flowOf(null, null)).collect { emissions++ } + assertEquals("distinctUntilChanged should collapse the repeated value", 1, emissions) + } +} diff --git a/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt b/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt index 9413528..99e63c0 100644 --- a/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt +++ b/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt @@ -13,6 +13,7 @@ import dev.privacyllc.period.core.datastore.UserPreferences import dev.privacyllc.period.core.datastore.UserPreferencesRepository import dev.privacyllc.period.domain.prediction.CycleStatusRules import dev.privacyllc.period.domain.prediction.FertilityEstimate +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.first import java.time.Clock import java.time.LocalDate @@ -38,7 +39,35 @@ class ReminderWorker @AssistedInject constructor( private val clock: Clock, ) : CoroutineWorker(appContext, params) { - override suspend fun doWork(): Result { + /** + * A wake-up that cannot read what it needs says nothing, and says so as a + * success. + * + * Without this, a throw anywhere below became `Result.failure()` by way of + * `CoroutineWorker`'s own catch — which is indistinguishable from a + * transient problem, and leaves the reason invisible. Silence is already + * this worker's correct answer when there is nothing to say: with no + * history `CycleStatusRules` returns `NoData` and `ReminderRules` maps it + * to no decision. A failed read lands in the same place for the same + * reason — there is nothing it can honestly tell the user. + * + * `CancellationException` is rethrown rather than swallowed. WorkManager + * cancels this worker by cancelling its coroutine, and catching that would + * report success for work that never ran. + */ + override suspend fun doWork(): Result = + try { + decideAndNotify() + } catch (cancellation: CancellationException) { + throw cancellation + } catch (failure: Exception) { + // Deliberately not logged. §45 applies here as much as to the UI, + // and the likeliest throw on this path is a `Prediction` invariant + // failure whose message is derived from a cycle date. + Result.success() + } + + private suspend fun decideAndNotify(): Result { val prefs = preferences.preferences.first() val today = LocalDate.now(clock) diff --git a/docs/architecture/GUARDS.md b/docs/architecture/GUARDS.md index b9a522a..b77aa7c 100644 --- a/docs/architecture/GUARDS.md +++ b/docs/architecture/GUARDS.md @@ -140,3 +140,42 @@ the operation claims**: Escape hatches are fine, and they have to be asked for by name, never be the default, and say plainly what is being given up. + +## 8. A red is not a proof — read what went red + +`scripts/prove-guard.sh` decides the guard caught the mutation from the runner's +**exit code**. It cannot tell *"the mutation broke the test"* from *"the command +was malformed and nothing ran"*, and both are non-zero. + +Found by using it on the fix for the missing application-scope exception +handler. This looked like a clean proof and was not one: + +```bash +bash scripts/prove-guard.sh app/.../PeriodApplication.kt \ + ' + CoroutineExceptionHandler { _, _ -> }' '' \ + ./gradlew :app:test --tests '*ApplicationScopeTest*' +``` + +`:app:test` is AGP's lifecycle task and takes no `--tests` option, so Gradle +failed with `Unknown command-line option '--tests'` in 544 ms. The mutation was +never compiled and the test never ran — and `prove-guard` reported *"the guard +caught it"*. The concrete task is `:app:testDebugUnitTest`; against that, the +same mutation failed exactly one test and the proof was real. + +Two rules follow, and the first is the general one: + +- **Read the `--- what failed ---` block, every time.** A proof is a proof only + when the *named test* is what went red. An exit code alone cannot distinguish + a caught regression from a typo, and this script is most likely to be run at + the moment you least want to read output — after the code already works. +- **Give it a fail pattern when the runner prints no summary.** The fallback + counts matching log lines, and the default pattern also matches Gradle's + `FAILURE:` and `BUILD FAILED` banners, so one caught violation reads as three + and the script exits 3. Exit 3 is not a pass. The three boundary-guard proofs + in [`README.md`](README.md) carry a `PROVE_GUARD_FAIL_PATTERN` for exactly + this reason; without it they exit 3 while the guard is behaving perfectly, + which is the failure this document exists to prevent — a check whose red you + have learned to ignore. + +The uncomfortable part: the documented proofs had been exiting 3 rather than 0 +since they were written. Nobody had run them and read the last line. diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 0979617..32bfcd2 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -153,7 +153,18 @@ Per [`GUARDS.md`](GUARDS.md) §1 it is not evidence until it has been watched failing. These three are repeatable, each restores the file from a `trap`, and each was run: +The `export` is not optional. Gradle prints no test-style summary line for a +task like this one, so `prove-guard.sh` falls back to counting matching log +lines — and its default pattern also matches Gradle's own `FAILURE:` and +`BUILD FAILED` banners. One correctly-caught violation is then reported as +three failures and the script exits **3**, telling you to narrow a guard that +was already narrow. Exit 3 is not a pass. Pointing the pattern at the guard's +own violation lines makes the count the guard's count, and all three below then +exit 0. See [`GUARDS.md`](GUARDS.md) §8. + ```bash +export PROVE_GUARD_FAIL_PATTERN='^ - .* (does not permit|has no entry|applies an Android plugin)' + # 1. A domain module reaching upward — the leak that would end JVM-only tests bash scripts/prove-guard.sh domain/prediction/build.gradle.kts \ 'implementation(project(":domain:cycle"))' \