From 424a513336e0e7e8de9da126c1a81bc1c05c4a82 Mon Sep 17 00:00:00 2001 From: null Date: Wed, 19 Aug 2026 03:06:10 -0500 Subject: [PATCH] fix: a throw in a background flow no longer kills the process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The application scope in PeriodApplication was built with SupervisorJob and no CoroutineExceptionHandler, and ReminderCoordinator launchIns two Room flows on it. SupervisorJob stops a failing child cancelling its siblings; it does not stop the exception, which reaches the thread's default handler and ends the process. That scope is the one that runs with nobody watching. Application.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. Both ViewModels already install a handler; the one place a crash is invisible did not. The trigger is real rather than theoretical: repository.forecast runs the prediction engine inside the flow, and Prediction's init block enforces its window invariants with require. Three layers, outermost last: - ReminderCoordinator catches per chain, so one failing collection cannot take the other down. Doing nothing on failure is deliberate — cancelling the schedule would turn a failed read into reminders silently switched off until the user next touched a notification setting. - ReminderWorker returns success and posts nothing when it cannot read what it needs, which is already its behaviour with no history. Cancellation is rethrown rather than swallowed. - The scope handler is a backstop whose only job is that the process lives. It cannot log: checkNoHealthLogging covers this module, and an exception message here can carry a date derived from a cycle. The chains moved into internal functions taking flows so the catch is reachable from a test. CycleRepository is final with an internal constructor, which is right for a data boundary and wrong for faking, and adding a mocking library to reach one catch would have been the worse trade. Proved to fail, per GUARDS.md §1: removing the handler fails exactly one test (ApplicationScopeTest.kt:69), and removing either catch fails exactly its own. GUARDS.md gains §8. prove-guard.sh decides a guard caught the mutation from the runner's exit code, and cannot tell a broken test from a malformed command. Its first use here reported a clean catch when Gradle had actually rejected `:app:test --tests` as an unknown option and run nothing. The same tool's line-counting fallback also means the three documented boundary proofs in architecture/README.md have been exiting 3 rather than 0 since they were written; they now carry the fail pattern that makes them exit 0. closes #45 Co-Authored-By: Claude Opus 5 (1M context) --- .../privacyllc/period/PeriodApplication.kt | 48 +++++++++- .../notifications/ReminderCoordinator.kt | 83 ++++++++++++++--- .../privacyllc/period/ApplicationScopeTest.kt | 90 ++++++++++++++++++ .../notifications/ReminderCoordinatorTest.kt | 91 +++++++++++++++++++ .../core/notifications/ReminderWorker.kt | 31 ++++++- docs/architecture/GUARDS.md | 39 ++++++++ docs/architecture/README.md | 11 +++ 7 files changed, 377 insertions(+), 16 deletions(-) create mode 100644 app/src/test/kotlin/dev/privacyllc/period/ApplicationScopeTest.kt create mode 100644 app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt 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"))' \