Code review:

- refactor(location): dedupe test fixtures and simplify the gate internals - test(location): assert the release, not just its ledger side-effect - test(location): cover the LocationState -> LocationFlow -> RefCountedSession seam
2026-10-06 11:48:24 +00:00 · 2026-07-31 15:19:44 +02:00
parent f09eef1470
commit 2360829e83
8 changed files with 429 additions and 142 deletions
@@ -60,13 +60,12 @@ import kotlinx.coroutines.launch
 * the release on that path; see `releasesTheRegistrationWhenTheSeedThrows` in
 * `LocationFlowTest`. (In principle a collector cancelling mid-seed would also
 * unwind past `awaitClose` the same way, but that path could not be
- * reproduced — `callbackFlow`'s `send` is buffered and returns without
- * suspending, so it never observes the cancellation.)
+ * reproduced — `callbackFlow`'s buffer is empty at this point, so the single
+ * seed send returns without suspending and never observes the cancellation.)
 */
 class LocationFlow(
    private val locationManager: LocationManager,
    private val sdkInt: Int = Build.VERSION.SDK_INT,
-    private val hasFine: Boolean = false,
 ) {
    @SuppressLint("MissingPermission")
    fun get(
@@ -84,51 +83,24 @@ class LocationFlow(
            // One binder call, reused for both the ladder filter and the seed.
            val providers = locationManager.allProviders

-            val candidates = LocationProviderLadder.chooseProviders(sdkInt, hasFine) { it in providers }
+            val candidates = LocationProviderLadder.chooseProviders(sdkInt) { it in providers }

-            var registered: String? = null
-            for (provider in candidates) {
-                try {
-                    locationManager.requestLocationUpdates(
-                        provider,
-                        minTimeMs,
-                        minDistanceM,
-                        locationCallback,
-                        Looper.getMainLooper(),
-                    )
-                    registered = provider
-                    break
-                } catch (e: SecurityException) {
-                    Log.w("LocationFlow", "Provider $provider refused the update request", e)
-                }
-            }
-
-            if (registered == null) {
-                throw SecurityException("No usable location provider. Candidates: $candidates")
-            }
+            val registered =
+                candidates.firstOrNull { requestUpdates(it, minTimeMs, minDistanceM, locationCallback) }
+                    ?: throw SecurityException("No usable location provider. Candidates: $candidates")

            Log.i("LocationFlow") { "Listening on $registered every ${minTimeMs}ms / ${minDistanceM}m" }
            onListening?.invoke(true)

-            // Everything after the acquire runs under try/finally, not under
-            // awaitClose. freshestLastKnownLocation() just below can throw a
-            // non-cancellation exception and unwind before awaitClose is ever
-            // reached — proven by releasesTheRegistrationWhenTheSeedThrows in
-            // LocationFlowTest, which fails if the release moves into
-            // awaitClose. Cleanup parked inside awaitClose would then never
-            // run — the registration would leak and the refcount would stick
-            // at >= 1 for the life of the process, so location.ms would accrue
-            // forever with nothing listening. (In principle a collector
-            // cancelling mid-seed would unwind the same way, but that path
-            // could not be reproduced: callbackFlow's send is buffered and
-            // returns without suspending, so it never observes the
-            // cancellation.) The finally covers both cases regardless.
+            // Cleanup lives in this finally, not in awaitClose — see the class
+            // KDoc, and `releasesTheRegistrationWhenTheSeedThrows` in
+            // LocationFlowTest, which fails if it moves.
            try {
                // Seeded after registration so the no-provider path throws
                // without having emitted anything; seeding first would show the
                // consumer Success -> LackPermission on a device with no
                // compatible provider.
-                freshestLastKnownLocation(providers)?.let {
+                freshestLastKnownLocation(candidates)?.let {
                    Log.d("LocationFlow") { "Last known location is $it" }
                    send(it)
                }
@@ -141,10 +113,35 @@ class LocationFlow(
            }
        }

+    /** True when the registration was accepted; false when the provider refused it. */
+    @SuppressLint("MissingPermission")
+    private fun requestUpdates(
+        provider: String,
+        minTimeMs: Long,
+        minDistanceM: Float,
+        locationCallback: LocationListener,
+    ): Boolean =
+        try {
+            locationManager.requestLocationUpdates(
+                provider,
+                minTimeMs,
+                minDistanceM,
+                locationCallback,
+                Looper.getMainLooper(),
+            )
+            true
+        } catch (e: SecurityException) {
+            Log.w("LocationFlow", "Provider $provider refused the update request", e)
+            false
+        }
+
    /**
-     * The freshest cached fix across every provider. Permission-checked per
-     * provider like the update request is, so each lookup is guarded — on a
-     * device where a provider refuses us, the others should still seed.
+     * The freshest cached fix across the providers the ladder deemed usable.
+     * Sweeping every provider the device reports instead would, on the
+     * coarse-only legacy path, pay a guaranteed-to-throw binder call per
+     * fine-only provider on every flow start. Still guarded per provider, like
+     * the update request is — on a device where one refuses us, the others
+     * should still seed.
     */
    @SuppressLint("MissingPermission")
    private fun freshestLastKnownLocation(providers: List<String>): Location? =
@@ -36,10 +36,13 @@ import android.os.Build
 * Below API 31, `gps`, `passive` and `fused` required `ACCESS_FINE_LOCATION`;
 * only `network` accepted `ACCESS_COARSE_LOCATION`. Approximate location, which
 * lets a coarse-only app request any provider and receive a fuzzed result, is an
- * Android 12 change. Amethyst declares coarse only, so [hasFine] is always false
- * in production — it is a parameter so the function is total over the permission
- * axis and both sides of the API branch are testable, not because fine access is
- * anticipated.
+ * Android 12 change. Amethyst declares coarse only, so the legacy branch is
+ * unconditional below API 31.
+ *
+ * Adding `ACCESS_FINE_LOCATION` later would **not** widen this on its own — the
+ * branch below has no permission input, so pre-31 devices would keep getting
+ * `network` alone and silently lose the precision the new permission was granted
+ * for. Whoever adds it must widen the condition here too.
 *
 * Returns the ordered candidate list rather than a single choice so the caller
 * can fall through to the next rung if a registration is refused. An empty list
@@ -60,15 +63,9 @@ object LocationProviderLadder {

    fun chooseProviders(
        sdkInt: Int,
-        hasFine: Boolean,
        exists: (String) -> Boolean,
    ): List<String> {
-        val ladder =
-            if (sdkInt >= Build.VERSION_CODES.S || hasFine) {
-                FULL_LADDER
-            } else {
-                COARSE_ONLY_LEGACY_LADDER
-            }
+        val ladder = if (sdkInt >= Build.VERSION_CODES.S) FULL_LADDER else COARSE_ONLY_LEGACY_LADDER

        return ladder.filter(exists)
    }
@@ -62,7 +62,7 @@ import kotlinx.coroutines.flow.transformLatest
 */
 class LocationState(
    context: Context,
-    scope: CoroutineScope,
+    private val scope: CoroutineScope,
    private val isForeground: StateFlow<Boolean>,
    /**
     * Resource-ledger hook: true while location updates are actively
@@ -88,9 +88,16 @@ class LocationState(
        /**
         * How long to keep listening after the last activity stops, so a
         * one-second app switch doesn't destroy and rebuild the registration.
-         * Matches the `WhileSubscribed` window below, and is the same intent.
+         * Same intent as [SUBSCRIPTION_STOP_TIMEOUT_MS], on the other axis.
         */
        const val BACKGROUND_GRACE_MS: Long = 5_000L
+
+        /**
+         * How long `stateIn` keeps the upstream alive after the last collector
+         * leaves, so a screen rotation or a tab switch doesn't rebuild the
+         * registration either.
+         */
+        const val SUBSCRIPTION_STOP_TIMEOUT_MS: Long = 5_000L
    }

    sealed class LocationResult {
@@ -107,11 +114,12 @@ class LocationState(

    private var hasLocationPermission = MutableStateFlow(false)

-    // Volatile: R1 below reads these to decide whether to emit Loading, from a
-    // different coroutine than the onEach that writes them.
-    @Volatile private var latestLocation: LocationResult = LocationResult.Loading
+    // Read by R1 below to decide whether to emit Loading, from a different
+    // coroutine than the onEach that writes it — hence a StateFlow rather than
+    // a plain field.
+    private val latestLocation = MutableStateFlow<LocationResult>(LocationResult.Loading)

-    @Volatile private var latestPreciseLocation: LocationResult = LocationResult.Loading
+    private val latestPreciseLocation = MutableStateFlow<LocationResult>(LocationResult.Loading)

    fun setLocationPermission(newValue: Boolean) {
        if (newValue != hasLocationPermission.value) {
@@ -152,60 +160,59 @@ class LocationState(
        }.distinctUntilChanged()

    @OptIn(ExperimentalCoroutinesApi::class)
-    private fun geohashFlow(
+    private fun buildGeohashStateFlow(
        tag: String,
        charsCount: Int,
        minTimeMs: Long,
        minDistanceM: Float,
-        latest: () -> LocationResult,
-        setLatest: (LocationResult) -> Unit,
-    ): Flow<LocationResult> =
-        gate.transformLatest { state ->
-            when (state) {
-                // Deliberately does NOT write to the cache. Today's code emits
-                // LackPermission without touching latestLocation, and wiping it
-                // here would cost a Loading emission — and so an empty-feed
-                // flash — on every permission flap, which is the regression R1
-                // exists to prevent. Consumers already see LackPermission from
-                // the StateFlow; the cache is internal and only decides whether
-                // Loading is emitted.
-                Gate.NoPermission -> emit(LocationResult.LackPermission)
+        cache: MutableStateFlow<LocationResult>,
+    ): StateFlow<LocationResult> =
+        gate
+            .transformLatest { state ->
+                when (state) {
+                    // Deliberately does NOT write to the cache. Today's code emits
+                    // LackPermission without touching the cache, and wiping it
+                    // here would cost a Loading emission — and so an empty-feed
+                    // flash — on every permission flap, which is the regression R1
+                    // exists to prevent. Consumers already see LackPermission from
+                    // the StateFlow; the cache is internal and only decides whether
+                    // Loading is emitted.
+                    Gate.NoPermission -> emit(LocationResult.LackPermission)

-                // Emit nothing: stateIn keeps the last value, so every
-                // synchronous .value reader still sees the last known geohash
-                // while the OS registration is released.
-                Gate.Paused -> Unit
+                    // Emit nothing: stateIn keeps the last value, so every
+                    // synchronous .value reader still sees the last known geohash
+                    // while the OS registration is released.
+                    Gate.Paused -> Unit

-                Gate.Listen -> {
-                    // Only when there is nothing cached. Emitting Loading on
-                    // every foreground return would flash the "Around Me" feed
-                    // empty, because AroundMeFeedFlow.convert maps anything
-                    // that is not Success to an empty geotag set.
-                    if (latest() !is LocationResult.Success) emit(LocationResult.Loading)
+                    Gate.Listen -> {
+                        // Only when there is nothing cached. Emitting Loading on
+                        // every foreground return would flash the "Around Me" feed
+                        // empty, because AroundMeFeedFlow.convert maps anything
+                        // that is not Success to an empty geotag set.
+                        if (cache.value !is LocationResult.Success) emit(LocationResult.Loading)

-                    emitAll(
-                        locationSource(minTimeMs, minDistanceM)
-                            .map { LocationResult.Success(it.toGeoHash(charsCount)) as LocationResult }
-                            .onEach { setLatest(it) }
-                            .catch { e ->
-                                Log.w(tag, "Exception in the flow", e)
-                                setLatest(LocationResult.LackPermission)
-                                emit(LocationResult.LackPermission)
-                            },
-                    )
+                        emitAll(
+                            locationSource(minTimeMs, minDistanceM)
+                                .map { LocationResult.Success(it.toGeoHash(charsCount)) as LocationResult }
+                                .onEach { cache.value = it }
+                                .catch { e ->
+                                    Log.w(tag, "Exception in the flow", e)
+                                    cache.value = LocationResult.LackPermission
+                                    emit(LocationResult.LackPermission)
+                                },
+                        )
+                    }
                }
-            }
-        }
+            }.stateIn(scope, SharingStarted.WhileSubscribed(SUBSCRIPTION_STOP_TIMEOUT_MS), cache.value)

    val geohashStateFlow: StateFlow<LocationResult> by lazy {
-        geohashFlow(
+        buildGeohashStateFlow(
            tag = "GeohashStateFlow",
            charsCount = GeohashPrecision.KM_5_X_5.digits,
            minTimeMs = COARSE_MIN_TIME,
            minDistanceM = COARSE_MIN_DISTANCE,
-            latest = { latestLocation },
-            setLatest = { latestLocation = it },
-        ).stateIn(scope, SharingStarted.WhileSubscribed(5000), latestLocation)
+            cache = latestLocation,
+        )
    }

    /**
@@ -221,13 +228,12 @@ class LocationState(
     * app ever requests `ACCESS_FINE_LOCATION`.
     */
    val preciseGeohashStateFlow: StateFlow<LocationResult> by lazy {
-        geohashFlow(
+        buildGeohashStateFlow(
            tag = "PreciseGeohashStateFlow",
            charsCount = GeohashChannelLevel.BUILDING.chars,
            minTimeMs = PRECISE_MIN_TIME,
            minDistanceM = PRECISE_MIN_DISTANCE,
-            latest = { latestPreciseLocation },
-            setLatest = { latestPreciseLocation = it },
-        ).stateIn(scope, SharingStarted.WhileSubscribed(5000), latestPreciseLocation)
+            cache = latestPreciseLocation,
+        )
    }
 }
@@ -34,17 +34,14 @@ package com.vitorpamplona.amethyst.service.resourceusage
 * off with a holder still active.
 *
 * Takes the setter as a lambda rather than a [SessionTimeIntegrator] because
- * that is all it needs — and because constructing a real integrator drags in a
- * [ResourceUsageAccountant] and a store file to observe one boolean.
+ * that is all it needs, and so tests need no accountant or store file.
 *
- * Reports **transitions only**, not every call. A 1 -> 2 acquire would otherwise
- * re-enter [SessionTimeIntegrator.setActive] with the session already open,
- * splitting one segment into two. That happens to be arithmetically harmless
- * (`account()` adds each piece, and the pieces are contiguous), and it does not
- * inflate a `*.starts` counter either, because [SessionTimeIntegrator] already
- * guards its starts increment on `prev == null`. Transition-only is simply the
- * contract the name implies, and it keeps the class honest for a future caller
- * that reacts to the callback rather than integrating it.
+ * Reports **transitions only**, not every call — the contract the name implies,
+ * and what keeps the class honest for a future caller that reacts to the
+ * callback rather than integrating it. (For [SessionTimeIntegrator] specifically
+ * a 1 -> 2 re-entry would have been harmless: it splits one segment into two
+ * contiguous pieces that `account()` re-adds, and its starts increment is
+ * already guarded on `prev == null`.)
 *
 * Releases must be paired with acquires. This class cannot tell an unpaired
 * release from a real one, so callers guarantee the pairing; see `LocationFlow`,
@@ -38,25 +38,10 @@ import org.junit.Test

@OptIn(ExperimentalCoroutinesApi::class)
 class LocationFlowTest {
-    /**
-     * A LocationManager that reports [providers] and refuses [denied] with a
-     * SecurityException, mimicking the pre-API-31 fine-location requirement.
-     */
    private fun manager(
        providers: List<String>,
        denied: Set<String> = emptySet(),
-    ): LocationManager {
-        val lm = mockk<LocationManager>(relaxed = true)
-        every { lm.allProviders } returns providers
-        every { lm.getLastKnownLocation(any()) } returns null
-        every {
-            lm.requestLocationUpdates(any<String>(), any<Long>(), any<Float>(), any<LocationListener>(), any())
-        } answers {
-            val provider = firstArg<String>()
-            if (provider in denied) throw SecurityException("denied: $provider")
-        }
-        return lm
-    }
+    ) = mockLocationManager(providers, denied)

    @Test
    fun firesNeitherEdgeWhenNoProviderExists() =
@@ -180,6 +165,25 @@ class LocationFlowTest {
            verify(exactly = 0) { lm.getLastKnownLocation(any()) }
        }

+    @Test
+    fun seedsOnlyFromTheProvidersTheLadderDeemedUsable() =
+        runTest {
+            // On the coarse-only legacy path the ladder narrows to `network`,
+            // and gps/passive/fused would each throw SecurityException. Sweeping
+            // every provider the device reports would pay those guaranteed
+            // throws — stack trace plus an eagerly-interpolated Log.w — on every
+            // flow start, i.e. on every foreground return.
+            val lm = manager(providers = listOf("fused", "network", "gps", "passive"))
+            val job = launch { LocationFlow(lm, sdkInt = 30).get(60_000L, 500f).collect { } }
+
+            runCurrent()
+
+            verify(exactly = 1) { lm.getLastKnownLocation("network") }
+            verify(exactly = 0) { lm.getLastKnownLocation(neq("network")) }
+
+            job.cancelAndJoin()
+        }
+
    @Test
    fun registersOnExactlyOneProvider() =
        runTest {
@@ -0,0 +1,243 @@
+/*
+ * Copyright (c) 2025 Vitor Pamplona
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
+ * Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+package com.vitorpamplona.amethyst.service.location
+
+import android.content.Context
+import android.location.LocationListener
+import android.location.LocationManager
+import com.vitorpamplona.amethyst.service.resourceusage.RefCountedSession
+import io.mockk.every
+import io.mockk.mockk
+import io.mockk.verify
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.cancelAndJoin
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.test.UnconfinedTestDispatcher
+import kotlinx.coroutines.test.advanceTimeBy
+import kotlinx.coroutines.test.advanceUntilIdle
+import kotlinx.coroutines.test.runTest
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+/**
+ * Wires the *real* [LocationState] -> [LocationFlow] -> [RefCountedSession]
+ * seam together, exercising [LocationState]'s **default** `locationSource`
+ * rather than overriding it the way [LocationStateTest] and [LocationFlowTest]
+ * do. Each of those files mocks the other layer out, so neither can catch a
+ * regression in the composition itself — e.g. a refcount that gets pinned
+ * open (or closed) because `onListening` fires in an order or multiplicity
+ * the two independent [LocationState] flows didn't anticipate. That failure
+ * mode is unrecoverable at runtime: a pinned-open refcount means
+ * `location.ms` accrues in the resource-usage ledger forever, with nothing
+ * actually listening.
+ *
+ * Same dispatcher note as [LocationStateTest]: `runTest {}`'s default
+ * `StandardTestDispatcher` does not drive this `combine` + `transformLatest` +
+ * `stateIn(WhileSubscribed)` + `backgroundScope.launch { collect }` chain to
+ * completion via `advanceUntilIdle()` alone, so every test here runs on
+ * `UnconfinedTestDispatcher`.
+ */
+@OptIn(ExperimentalCoroutinesApi::class)
+class LocationLedgerCompositionTest {
+    /** A [LocationManager] that reports two providers and never denies registration. */
+    private fun locationManager(): LocationManager = mockLocationManager(providers = listOf("fused", "network"))
+
+    /** A [Context] whose `LOCATION_SERVICE` lookup returns [locationManager]. */
+    private fun contextWithLocationService(locationManager: LocationManager): Context {
+        val context = mockk<Context>(relaxed = true)
+        every { context.getSystemService(Context.LOCATION_SERVICE) } returns locationManager
+        return context
+    }
+
+    /**
+     * Wires a real [LocationState] to a real [RefCountedSession], standing in
+     * for the resource-usage ledger session. Deliberately does NOT override
+     * `locationSource` — that is the whole point of this file.
+     *
+     * Exposes [locationManager] so a test can `verify` against it directly —
+     * e.g. that exactly one of two concurrent registrations was torn down —
+     * rather than inferring release from the ledger alone, which cannot tell
+     * "released" from "never released" on its own (see
+     * `bothFlowsActiveThenOneStopsKeepsTheLedgerOpen`).
+     */
+    private class Harness(
+        scope: CoroutineScope,
+        context: Context,
+        val locationManager: LocationManager,
+    ) {
+        /** Stand-in for `SessionTimeIntegrator.setActive`, i.e. the ledger. */
+        val ledger = mutableListOf<Boolean>()
+        private val refCount = RefCountedSession { ledger.add(it) }
+        val foreground = MutableStateFlow(true)
+        val state =
+            LocationState(
+                context = context,
+                scope = scope,
+                isForeground = foreground,
+                onListening = { refCount.setActive(it) },
+            )
+    }
+
+    private fun harness(scope: CoroutineScope): Harness {
+        val locationManager = locationManager()
+        return Harness(scope, contextWithLocationService(locationManager), locationManager)
+    }
+
+    /**
+     * The interleaving [RefCountedSession] exists for. Two independent
+     * [LocationState] flows both register with the OS; the ledger must open
+     * once, not twice. Stopping one of the two must NOT close the ledger,
+     * because the other is still listening — this half is exactly what a
+     * per-flow (rather than refcounted) `onListening` -> ledger wiring would
+     * get wrong, and what a per-layer test (mocking `locationSource`, or
+     * driving `RefCountedSession` directly with synthetic booleans) cannot
+     * see, because it never lets two real [LocationFlow] registrations race
+     * each other through the shared hook.
+     *
+     * Asserts its premise, not just its consequence. The ledger staying at
+     * `[true]` after the precise flow stops is also what you'd see if that
+     * flow's registration never released at all — `onListening` deleted from
+     * [LocationFlow]'s `finally`, cleanup moved somewhere unreached, or the
+     * [LocationState.SUBSCRIPTION_STOP_TIMEOUT_MS] teardown simply hadn't
+     * elapsed. The
+     * `removeUpdates` verify below rules out the OS-registration side of
+     * that; cancelling the coarse flow too and checking the full `[true,
+     * false]` sequence at the end rules out the `onListening` side, since
+     * `removeUpdates` alone fires unconditionally in the `finally` and would
+     * not by itself notice `onListening` going missing.
+     */
+    @Test
+    fun bothFlowsActiveThenOneStopsKeepsTheLedgerOpen() =
+        runTest(UnconfinedTestDispatcher()) {
+            val harness = harness(backgroundScope)
+            harness.state.setLocationPermission(true)
+
+            val preciseJob = backgroundScope.launch { harness.state.preciseGeohashStateFlow.collect { } }
+            val coarseJob = backgroundScope.launch { harness.state.geohashStateFlow.collect { } }
+            advanceUntilIdle()
+
+            assertEquals(
+                "both flows registering with the OS must open the ledger exactly once, not twice",
+                listOf(true),
+                harness.ledger,
+            )
+
+            preciseJob.cancelAndJoin()
+            // The cancelled collector alone doesn't tear down the upstream
+            // registration — WhileSubscribed keeps it alive for a grace
+            // period first. Advance past it so the release below is
+            // actually observable, not just "not yet due".
+            advanceTimeBy(LocationState.SUBSCRIPTION_STOP_TIMEOUT_MS + 1)
+            advanceUntilIdle()
+
+            // Exactly one of the two OS registrations should have been torn
+            // down at this point — the precise one — while the coarse one is
+            // still live.
+            verify(exactly = 1) { harness.locationManager.removeUpdates(any<LocationListener>()) }
+
+            assertEquals(
+                "the coarse flow is still listening; stopping the precise one alone must not close the ledger",
+                listOf(true),
+                harness.ledger,
+            )
+
+            // Stop the coarse flow too, so the ledger closing here proves
+            // the release path genuinely works end-to-end for this test's
+            // own harness — not merely that the assertions above never
+            // exercised it.
+            coarseJob.cancelAndJoin()
+            advanceTimeBy(LocationState.SUBSCRIPTION_STOP_TIMEOUT_MS + 1)
+            advanceUntilIdle()
+
+            verify(exactly = 2) { harness.locationManager.removeUpdates(any<LocationListener>()) }
+
+            assertEquals(
+                "both flows stopping must close the ledger exactly once, proving the precise flow's earlier release was real",
+                listOf(true, false),
+                harness.ledger,
+            )
+        }
+
+    /**
+     * The full foreground -> background cycle with both flows registered.
+     * The ledger must see exactly `[true, false]`: one open when the first
+     * flow registers (the second joining must not re-open it), one close
+     * once BOTH flows have released (not one close per flow, and not before
+     * the background grace period each flow independently waits out).
+     */
+    @Test
+    fun fullCycleWithBothFlowsOpensAndClosesTheLedgerExactlyOnce() =
+        runTest(UnconfinedTestDispatcher()) {
+            val harness = harness(backgroundScope)
+            harness.state.setLocationPermission(true)
+
+            backgroundScope.launch { harness.state.geohashStateFlow.collect { } }
+            backgroundScope.launch { harness.state.preciseGeohashStateFlow.collect { } }
+            advanceUntilIdle()
+
+            assertEquals(listOf(true), harness.ledger)
+
+            harness.foreground.value = false
+            // Real grace-period wait, not a test artefact: settledForeground
+            // delays BACKGROUND_GRACE_MS before the gate reaches Paused for
+            // either flow. See LocationStateTest for the worked example this
+            // follows.
+            advanceTimeBy(LocationState.BACKGROUND_GRACE_MS + 1)
+            advanceUntilIdle()
+
+            assertEquals(
+                "backgrounding must close the ledger exactly once, not once per flow, and not before the grace period",
+                listOf(true, false),
+                harness.ledger,
+            )
+        }
+
+    /**
+     * After a full open/close cycle, returning to the foreground must reopen
+     * the ledger exactly once — not once per flow, and not a second time on
+     * top of a close that never actually happened.
+     */
+    @Test
+    fun returningToForegroundReopensTheLedgerExactlyOnce() =
+        runTest(UnconfinedTestDispatcher()) {
+            val harness = harness(backgroundScope)
+            harness.state.setLocationPermission(true)
+
+            backgroundScope.launch { harness.state.geohashStateFlow.collect { } }
+            backgroundScope.launch { harness.state.preciseGeohashStateFlow.collect { } }
+            advanceUntilIdle()
+
+            harness.foreground.value = false
+            advanceTimeBy(LocationState.BACKGROUND_GRACE_MS + 1)
+            advanceUntilIdle()
+
+            harness.foreground.value = true
+            advanceUntilIdle()
+
+            assertEquals(
+                "a return to foreground after a full close must reopen the ledger exactly once",
+                listOf(true, false, true),
+                harness.ledger,
+            )
+        }
+}
@@ -30,7 +30,7 @@ class LocationProviderLadderTest {
    fun modernDevicePrefersFusedThenFallsBackInOrder() {
        assertEquals(
            listOf("fused", "network", "gps", "passive"),
-            LocationProviderLadder.chooseProviders(sdkInt = 31, hasFine = false) { it in all },
+            LocationProviderLadder.chooseProviders(sdkInt = 31) { it in all },
        )
    }

@@ -40,25 +40,18 @@ class LocationProviderLadderTest {

        assertEquals(
            listOf("network", "passive"),
-            LocationProviderLadder.chooseProviders(sdkInt = 37, hasFine = false) { it in present },
+            LocationProviderLadder.chooseProviders(sdkInt = 37) { it in present },
        )
    }

    @Test
    fun coarseOnlyBelowApi31GetsNetworkOnly() {
        // gps, passive and fused all required ACCESS_FINE_LOCATION before
-        // Android 12 (see Hypothesis H1 in the design spec).
+        // Android 12 (see Hypothesis H1 in the design spec), and Amethyst
+        // declares ACCESS_COARSE_LOCATION only.
        assertEquals(
            listOf("network"),
-            LocationProviderLadder.chooseProviders(sdkInt = 30, hasFine = false) { it in all },
-        )
-    }
-
-    @Test
-    fun fineBelowApi31GetsTheFullLadder() {
-        assertEquals(
-            listOf("fused", "network", "gps", "passive"),
-            LocationProviderLadder.chooseProviders(sdkInt = 26, hasFine = true) { it in all },
+            LocationProviderLadder.chooseProviders(sdkInt = 30) { it in all },
        )
    }

@@ -68,7 +61,7 @@ class LocationProviderLadderTest {

        assertEquals(
            emptyList<String>(),
-            LocationProviderLadder.chooseProviders(sdkInt = 28, hasFine = false) { it in present },
+            LocationProviderLadder.chooseProviders(sdkInt = 28) { it in present },
        )
    }

@@ -76,7 +69,7 @@ class LocationProviderLadderTest {
    fun noProvidersAtAllGetsNothing() {
        assertEquals(
            emptyList<String>(),
-            LocationProviderLadder.chooseProviders(sdkInt = 37, hasFine = false) { false },
+            LocationProviderLadder.chooseProviders(sdkInt = 37) { false },
        )
    }
 }
@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2025 Vitor Pamplona
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
+ * Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+package com.vitorpamplona.amethyst.service.location
+
+import android.location.LocationListener
+import android.location.LocationManager
+import io.mockk.every
+import io.mockk.mockk
+
+/**
+ * A [LocationManager] that reports [providers], has no cached fix, and refuses
+ * [denied] with a `SecurityException` — mimicking the pre-API-31 fine-location
+ * requirement.
+ *
+ * Shared by [LocationFlowTest] and [LocationLedgerCompositionTest] so the mock's
+ * surface tracks the binder calls [LocationFlow] actually makes in one place.
+ */
+internal fun mockLocationManager(
+    providers: List<String>,
+    denied: Set<String> = emptySet(),
+): LocationManager {
+    val lm = mockk<LocationManager>(relaxed = true)
+    every { lm.allProviders } returns providers
+    every { lm.getLastKnownLocation(any()) } returns null
+    every {
+        lm.requestLocationUpdates(any<String>(), any<Long>(), any<Float>(), any<LocationListener>(), any())
+    } answers {
+        val provider = firstArg<String>()
+        if (provider in denied) throw SecurityException("denied: $provider")
+    }
+    return lm
+}