fix: a throw in a background flow no longer kills the process

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) <noreply@anthropic.com>
This commit is contained in:
null 2026-08-19 03:06:10 -05:00
parent 5ff9f00f04
commit 424a513336
7 changed files with 377 additions and 16 deletions

View File

@ -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 { _, _ -> },
)
}
}

View File

@ -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<LocalDate?>,
preferences: Flow<UserPreferences>,
): 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<LocalDate?>): 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
}

View File

@ -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 <T> recordingUncaughtExceptions(block: (AtomicReference<Throwable?>) -> T): T {
val original = Thread.getDefaultUncaughtExceptionHandler()
val seen = AtomicReference<Throwable?>(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<Throwable?>) {
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())
}
}

View File

@ -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<Context>()
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)
}
}

View File

@ -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)

View File

@ -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.

View File

@ -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"))' \