From 3fe936bedbad70f6bb15aed800a8d740ef154b48 Mon Sep 17 00:00:00 2001 From: davotoula Date: Wed, 29 Jul 2026 16:41:02 +0200 Subject: [PATCH] docs --- ...026-07-29-location-foreground-gate-impl.md | 1457 +++++++++++++++++ .../2026-07-29-location-foreground-gate.md | 773 +++++++++ 2 files changed, 2230 insertions(+) create mode 100644 amethyst/plans/2026-07-29-location-foreground-gate-impl.md create mode 100644 amethyst/plans/2026-07-29-location-foreground-gate.md diff --git a/amethyst/plans/2026-07-29-location-foreground-gate-impl.md b/amethyst/plans/2026-07-29-location-foreground-gate-impl.md new file mode 100644 index 0000000000..2f4afdb777 --- /dev/null +++ b/amethyst/plans/2026-07-29-location-foreground-gate-impl.md @@ -0,0 +1,1457 @@ +# Location Foreground Gate Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop Amethyst holding a location registration when no activity is started, register on one appropriate provider instead of four, and make `location.ms` measure real listening time. + +**Architecture:** A three-state gate over *permission × foreground* inside `LocationState` replaces the permission-only gate, so the registration is released whenever the app is backgrounded. `LocationFlow` selects one provider from an ordered ladder instead of shotgunning `allProviders`, and owns the `onListening` hook so accounting cannot start without a live registration. A `RefCountedSession` serialises the meter's refcount with the transition it drives. + +**Tech Stack:** Kotlin, kotlinx.coroutines Flow/StateFlow, `android.location.LocationManager` (no Play Services), JUnit 4 + MockK + kotlinx-coroutines-test. + +**Design spec:** `amethyst/plans/2026-07-29-location-foreground-gate.md` — read it before starting. This plan implements it; where the two disagree, the spec is wrong and should be corrected. + +## Global Constraints + +- Module is `amethyst` (Android only). Package root `com.vitorpamplona.amethyst`. +- `minSdk = 26`, `targetSdk = 37`, `compileSdk = 37` (`gradle/libs.versions.toml:12-14`). +- Every new `.kt` file starts with the MIT licence header — copy it verbatim from `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt:1-20`, changing nothing. +- Logging uses `com.vitorpamplona.quartz.utils.Log`, never `android.util.Log`. Use the lambda overload (`Log.d("Tag") { "msg $x" }`) when the message interpolates **and there is no throwable**. When a throwable must be logged, use the three-argument form `Log.w("Tag", "message", e)` — the lambda overloads take no `Throwable` (`quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/utils/Log.kt:74-79`), so "converting" such a call to a lambda silently discards the stack trace. That is the exact anti-pattern the repo's `find-non-lambda-logs` skill flags. +- Never pass `--no-verify` to `git commit`. The pre-commit hook runs `./gradlew spotlessCheck` and nothing else — no compilation — so it passes even at the two points in this plan where the module is temporarily red. +- No new third-party dependencies. All libraries used here are already declared. +- Never write a fully-qualified class name inline in a function body — add an `import`. +- Run `./gradlew spotlessApply` before every commit. +- Do not push and do not open a PR. Commit locally only. +- Unit tests live in `amethyst/src/test/java/...`, run with `./gradlew :amethyst:testPlayDebugUnitTest`. + +## Naming contract + +These names are used across tasks. Use them exactly. + +| Name | Defined in | Signature | +|---|---|---| +| `RefCountedSession` | Task 2 | `class RefCountedSession(setSessionActive: (Boolean) -> Unit)`, method `fun setActive(active: Boolean)` | +| `LocationProviderLadder.chooseProviders` | Task 3 | `fun chooseProviders(sdkInt: Int, hasFine: Boolean, exists: (String) -> Boolean): List` | +| `LocationFlow` | Task 4 | `class LocationFlow(locationManager: LocationManager, sdkInt: Int = Build.VERSION.SDK_INT, hasFine: Boolean = false)`, method `fun get(minTimeMs: Long, minDistanceM: Float, onListening: ((Boolean) -> Unit)? = null): Flow` | +| `LocationState` | Task 5 | `class LocationState(context: Context, scope: CoroutineScope, isForeground: StateFlow, onListening: ((Boolean) -> Unit)? = null, locationSource: (Long, Float) -> Flow = …)` | +| `LocationState.COARSE_MIN_TIME` | Task 5 | `const val = 60_000L` | +| `LocationState.COARSE_MIN_DISTANCE` | Task 5 | `const val = 500.0f` | +| `LocationState.PRECISE_MIN_TIME` | Task 5 | `const val = 10_000L` | +| `LocationState.PRECISE_MIN_DISTANCE` | Task 5 | `const val = 100.0f` | +| `LocationState.BACKGROUND_GRACE_MS` | Task 5 | `const val = 5_000L` | + +`LocationState.MIN_TIME` and `LocationState.MIN_DISTANCE` are **deleted** in Task 5. Their only readers are `LocationState.kt:80`, `:119` and `LocationFlow.kt:29-30,42-43`, all rewritten by Tasks 4–5. + +## File structure + +| File | Task | Responsibility | +|---|---|---| +| `amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt` | 2 | Serialise a refcount with the boolean transition it drives | +| `amethyst/src/test/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSessionTest.kt` | 2 | ↑ | +| `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt` | 3 | Pure provider-selection policy | +| `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt` | 3 | ↑ | +| `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt` | 4 | Own the OS registration and the paired listening hook | +| `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt` | 4 | ↑ | +| `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt` | 5 | Gate on permission × foreground; expose the two geohash StateFlows | +| `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationStateTest.kt` | 5 | ↑ | +| `amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt` | 6 | Wire the foreground signal and the refcounted meter | + +--- + +### Task 1: Verify Hypothesis H1 on an API 26 emulator + +The spec's §H1 predicts that today's `allProviders` loop throws `SecurityException` on `gps`, `passive` and `fused` below API 31, because those required `ACCESS_FINE_LOCATION` before Android 12 and Amethyst holds coarse only. If true, location is **already broken** on Android 8–11 and this change fixes it — which belongs in the PR description. If false, the PR must not claim it. + +This task changes no production code. It is first because the spec says to verify before implementing, and because the answer decides one paragraph of the PR. + +**Files:** +- Modify (temporarily, reverted in Step 5): `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt:55` + +**Interfaces:** +- Consumes: nothing +- Produces: a written observation recorded in Step 6; no code + +- [ ] **Step 1: Boot the API 26 emulator** + +```bash +emulator -avd Medium_Phone_API_26_8_ -no-snapshot-load & +adb wait-for-device +adb shell getprop ro.build.version.sdk +``` + +Expected: prints `26`. + +If the AVD fails to boot, stop and report it. Do not substitute a different API level without saying so — the claim is specifically about API 26–30. + +- [ ] **Step 2: Add a temporary log of the provider order** + +`LocationFlow.kt`, immediately before the `locationManager.allProviders.forEach {` line (currently line 55), insert: + +```kotlin + Log.w("LocationFlowH1") { "allProviders order = ${locationManager.allProviders}" } +``` + +This is the ordering evidence the spec requires: the leak consequence only occurs if `network` precedes a fine-only provider, so the PR cannot claim a leak without seeing the order. + +- [ ] **Step 3: Install and grant coarse location only** + +```bash +./gradlew :amethyst:installPlayDebug +adb shell pm grant com.vitorpamplona.amethyst android.permission.ACCESS_COARSE_LOCATION +adb logcat -c +``` + +- [ ] **Step 4: Trigger a location subscription and capture the result** + +Launch the app, sign in to any account, and open the top-nav filter spinner on Home, selecting "Around Me". Then: + +```bash +adb logcat -d | grep -E "LocationFlowH1|SecurityException|LocationFlow" +``` + +Record verbatim: +1. the `allProviders order = [...]` line +2. whether a `SecurityException` appears, and which provider it names +3. whether any `Requesting Updates` line precedes the exception + +- [ ] **Step 5: Revert the temporary log** + +```bash +git checkout -- amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt +git status --short +``` + +Expected: no changes to `LocationFlow.kt`. + +- [ ] **Step 6: Record the observation in the spec** + +Append to the `## Hypothesis H1` section of `amethyst/plans/2026-07-29-location-foreground-gate.md`, replacing `<...>` with what Step 4 actually showed: + +```markdown +### H1 verification result (2026-07-29, Medium_Phone_API_26_8_, API 26) + +- `allProviders` order: `` +- `SecurityException`: `` +- Registration attempted before the throw: `` + +Conclusion: location is `` on API 26 with coarse-only +permission; registrations `` leak. +``` + +- [ ] **Step 7: If H1 is FALSE, amend the plan before continuing** + +The spec says the design is "correct either way", which is true only in one +direction. Task 3 hard-codes H1's conclusion in `COARSE_ONLY_LEGACY_LADDER` and +two of its tests. If Step 4 showed **no** `SecurityException`, then coarse +permission does reach `gps`/`fused`/`passive` below API 31 on this build, and +restricting pre-31 devices to `network` would be a conclusion drawn from +evidence that contradicts it. It is safe for `geohashStateFlow` — `network` is +the right provider for a 5 km cell either way — but it needlessly denies +`preciseGeohashStateFlow` any provider capable of building-level precision on +Android 8–11. + +So, **only if no `SecurityException` appeared**, make these three amendments +before starting Task 3, and say in the commit message that H1 was disproved: + +1. In Task 3's implementation, delete `COARSE_ONLY_LEGACY_LADDER` and the + `sdkInt`/`hasFine` branch. `chooseProviders` becomes + `fun chooseProviders(exists: (String) -> Boolean): List = FULL_LADDER.filter(exists)`. +2. In Task 3's test, delete `coarseOnlyBelowApi31GetsNetworkOnly` and + `coarseOnlyBelowApi31WithNoNetworkProviderGetsNothing`, and drop the + `sdkInt`/`hasFine` arguments from the remaining four. +3. In Task 4, drop the `sdkInt` and `hasFine` constructor parameters from + `LocationFlow` and the corresponding arguments from its six tests. +4. In Task 4's `get`, update the call site to match amendment 1: + `LocationProviderLadder.chooseProviders(sdkInt, hasFine) { it in providers }` + becomes `LocationProviderLadder.chooseProviders { it in providers }`. + +The per-rung `SecurityException` fall-through stays in **both** cases — it is +what makes the ladder robust to whatever the runtime check actually does, and +it is why H1 being wrong costs nothing. + +- [ ] **Step 8: Commit** + +```bash +git add amethyst/plans/2026-07-29-location-foreground-gate.md +git commit -m "docs(amethyst): record H1 verification result on API 26" +``` + +--- + +### Task 2: RefCountedSession + +**Files:** +- Create: `amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt` +- Test: `amethyst/src/test/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSessionTest.kt` + +**Interfaces:** +- Consumes: nothing +- Produces: `class RefCountedSession(setSessionActive: (Boolean) -> Unit)` with `fun setActive(active: Boolean)`. Task 6 constructs it as `RefCountedSession(locationSession::setActive)`. + +- [ ] **Step 1: Write the failing test** + +Create `amethyst/src/test/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSessionTest.kt` with the MIT header (copy `LocationState.kt:1-20` verbatim), then: + +```kotlin +package com.vitorpamplona.amethyst.service.resourceusage + +import org.junit.Assert.assertEquals +import org.junit.Test + +class RefCountedSessionTest { + @Test + fun overlappingHoldersKeepTheSessionOpenAndReportOnlyTransitions() { + val calls = mutableListOf() + val session = RefCountedSession { calls.add(it) } + + session.setActive(true) // holders 1 — inactive -> active + session.setActive(true) // holders 2 — a second listener joins, no transition + session.setActive(false) // holders 1 — the first one leaves, still active + + assertEquals("only the 0 -> 1 edge is a transition", listOf(true), calls) + + session.setActive(false) // holders 0 — the last one leaves + + assertEquals(listOf(true, false), calls) + } + + @Test + fun unmatchedReleaseDoesNotDriveTheCountNegative() { + val calls = mutableListOf() + val session = RefCountedSession { calls.add(it) } + + session.setActive(false) + session.setActive(false) + + assertEquals("releasing an idle session is a no-op", emptyList(), calls) + + // If the count had gone to -2, one acquire would leave it at -1 and + // report inactive. It must open the session instead. + session.setActive(true) + + assertEquals(listOf(true), calls) + } + + @Test + fun aSingleHolderOpensAndClosesTheSession() { + val calls = mutableListOf() + val session = RefCountedSession { calls.add(it) } + + session.setActive(true) + session.setActive(false) + + assertEquals(listOf(true, false), calls) + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*RefCountedSessionTest*' +``` + +Expected: FAIL — compilation error, `Unresolved reference: RefCountedSession`. + +- [ ] **Step 3: Write the implementation** + +Create `amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt` with the MIT header, then: + +```kotlin +package com.vitorpamplona.amethyst.service.resourceusage + +/** + * Refcounts a boolean session so overlapping holders don't close each other's + * segment. [LocationState][com.vitorpamplona.amethyst.service.location.LocationState] + * exposes two independent location flows that can both be listening at once — + * the "Around Me" feed plus an open geohash chat — and a bare + * [SessionTimeIntegrator] would close the segment when either one stops. + * + * The count and the transition it drives are taken under one lock. An + * [java.util.concurrent.atomic.AtomicInteger] beside an unsynchronised call is + * not enough: two threads can leave the counter at 1 while the last + * `setActive(false)` lands after the `setActive(true)`, latching the session + * 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. + * + * 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. + * + * Releases must be paired with acquires. This class cannot tell an unpaired + * release from a real one, so callers guarantee the pairing; see `LocationFlow`, + * which throws rather than reaching `awaitClose` when nothing registered. + */ +class RefCountedSession( + private val setSessionActive: (Boolean) -> Unit, +) { + private val lock = Any() + private var holders = 0 + + fun setActive(active: Boolean) { + synchronized(lock) { + val wasActive = holders > 0 + holders = if (active) holders + 1 else (holders - 1).coerceAtLeast(0) + val isActive = holders > 0 + if (isActive != wasActive) setSessionActive(isActive) + } + } +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*RefCountedSessionTest*' +``` + +Expected: PASS, 3 tests. + +- [ ] **Step 5: Format and commit** + +```bash +./gradlew spotlessApply +git add amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt \ + amethyst/src/test/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSessionTest.kt +git commit -m "feat(amethyst): add RefCountedSession for overlapping ledger holders" +``` + +--- + +### Task 3: Provider ladder + +**Files:** +- Create: `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt` +- Test: `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt` + +**Interfaces:** +- Consumes: nothing +- Produces: `LocationProviderLadder.chooseProviders(sdkInt: Int, hasFine: Boolean, exists: (String) -> Boolean): List`. Task 4 calls it. + +- [ ] **Step 1: Write the failing test** + +Create `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt` with the MIT header, then: + +```kotlin +package com.vitorpamplona.amethyst.service.location + +import org.junit.Assert.assertEquals +import org.junit.Test + +class LocationProviderLadderTest { + private val all = setOf("fused", "network", "gps", "passive") + + @Test + fun modernDevicePrefersFusedThenFallsBackInOrder() { + assertEquals( + listOf("fused", "network", "gps", "passive"), + LocationProviderLadder.chooseProviders(sdkInt = 31, hasFine = false) { it in all }, + ) + } + + @Test + fun missingProvidersAreFilteredOutButOrderIsKept() { + val present = setOf("network", "passive") + + assertEquals( + listOf("network", "passive"), + LocationProviderLadder.chooseProviders(sdkInt = 37, hasFine = false) { 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). + 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 }, + ) + } + + @Test + fun coarseOnlyBelowApi31WithNoNetworkProviderGetsNothing() { + val present = setOf("gps", "passive") + + assertEquals( + emptyList(), + LocationProviderLadder.chooseProviders(sdkInt = 28, hasFine = false) { it in present }, + ) + } + + @Test + fun noProvidersAtAllGetsNothing() { + assertEquals( + emptyList(), + LocationProviderLadder.chooseProviders(sdkInt = 37, hasFine = false) { false }, + ) + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*LocationProviderLadderTest*' +``` + +Expected: FAIL — compilation error, `Unresolved reference: LocationProviderLadder`. + +- [ ] **Step 3: Write the implementation** + +Create `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt` with the MIT header, then: + +```kotlin +package com.vitorpamplona.amethyst.service.location + +import android.location.LocationManager +import android.os.Build + +/** + * Picks which location providers to try, in order. + * + * Deliberately selects on **provider existence**, never on + * [LocationManager.isProviderEnabled]. A registration on a disabled provider + * goes live by itself when the user enables location — including from the + * quick-settings shade without leaving the app, which is exactly what someone + * does after seeing an empty "Around Me" feed. An enabled-state guard evaluated + * once at subscription start would lose that. + * + * 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. + * + * 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 + * means no compatible provider exists. + */ +object LocationProviderLadder { + // Compile-time String constants, inlined by the compiler, so naming + // FUSED_PROVIDER (added in API 31) is safe on older runtimes. + private val FULL_LADDER = + listOf( + LocationManager.FUSED_PROVIDER, + LocationManager.NETWORK_PROVIDER, + LocationManager.GPS_PROVIDER, + LocationManager.PASSIVE_PROVIDER, + ) + + private val COARSE_ONLY_LEGACY_LADDER = listOf(LocationManager.NETWORK_PROVIDER) + + fun chooseProviders( + sdkInt: Int, + hasFine: Boolean, + exists: (String) -> Boolean, + ): List { + val ladder = + if (sdkInt >= Build.VERSION_CODES.S || hasFine) { + FULL_LADDER + } else { + COARSE_ONLY_LEGACY_LADDER + } + + return ladder.filter(exists) + } +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*LocationProviderLadderTest*' +``` + +Expected: PASS, 6 tests. + +- [ ] **Step 5: Format and commit** + +```bash +./gradlew spotlessApply +git add amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt \ + amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt +git commit -m "feat(amethyst): add location provider ladder" +``` + +--- + +### Task 4: LocationFlow — one provider, paired listening hook + +**Files:** +- Modify (full rewrite of the class body): `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt` +- Test: `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt` + +**Interfaces:** +- Consumes: `LocationProviderLadder.chooseProviders` (Task 3) +- Produces: `class LocationFlow(locationManager: LocationManager, sdkInt: Int = Build.VERSION.SDK_INT, hasFine: Boolean = false)` with `fun get(minTimeMs: Long, minDistanceM: Float, onListening: ((Boolean) -> Unit)? = null): Flow`. Task 5 constructs it. + +The constructor takes a `LocationManager`, **not** a `Context`, so it can be tested with a MockK stub. `LocationState` does the `getSystemService` lookup. + +- [ ] **Step 1: Write the failing test** + +Create `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt` with the MIT header, then: + +```kotlin +package com.vitorpamplona.amethyst.service.location + +import android.location.Location +import android.location.LocationListener +import android.location.LocationManager +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.flow.collect +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +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, + denied: Set = emptySet(), + ): LocationManager { + val lm = mockk(relaxed = true) + every { lm.allProviders } returns providers + every { lm.getLastKnownLocation(any()) } returns null + every { + lm.requestLocationUpdates(any(), any(), any(), any(), any()) + } answers { + val provider = firstArg() + if (provider in denied) throw SecurityException("denied: $provider") + } + return lm + } + + @Test + fun firesNeitherEdgeWhenNoProviderExists() = + runTest { + val edges = mutableListOf() + val flow = LocationFlow(manager(providers = emptyList()), sdkInt = 37).get(60_000L, 500f) { edges.add(it) } + + val failure = runCatching { flow.collect { } }.exceptionOrNull() + + assertTrue("expected SecurityException, got $failure", failure is SecurityException) + assertEquals(emptyList(), edges) + } + + @Test + fun firesNeitherEdgeWhenEveryRungIsDenied() = + runTest { + val edges = mutableListOf() + val lm = manager(providers = listOf("fused", "network"), denied = setOf("fused", "network")) + val flow = LocationFlow(lm, sdkInt = 37).get(60_000L, 500f) { edges.add(it) } + + val failure = runCatching { flow.collect { } }.exceptionOrNull() + + assertTrue("expected SecurityException, got $failure", failure is SecurityException) + assertEquals(emptyList(), edges) + } + + @Test + fun fallsThroughToTheNextRungWhenOneIsDenied() = + runTest { + val edges = mutableListOf() + val lm = manager(providers = listOf("fused", "network"), denied = setOf("fused")) + val job = launch { LocationFlow(lm, sdkInt = 37).get(60_000L, 500f) { edges.add(it) }.collect { } } + + runCurrent() + + assertEquals(listOf(true), edges) + verify { lm.requestLocationUpdates("network", 60_000L, 500f, any(), any()) } + + job.cancelAndJoin() + } + + @Test + fun pairsTheListeningEdgesAroundASuccessfulRegistration() = + runTest { + val edges = mutableListOf() + val lm = manager(providers = listOf("network")) + val job = launch { LocationFlow(lm, sdkInt = 30).get(60_000L, 500f) { edges.add(it) }.collect { } } + + runCurrent() + assertEquals(listOf(true), edges) + + job.cancelAndJoin() + + assertEquals(listOf(true, false), edges) + verify { lm.removeUpdates(any()) } + } + + @Test + fun releasesTheRegistrationWhenCancelledDuringTheSeed() = + runTest { + // `send` in the seed suspends, so a collector cancelling while the + // getLastKnownLocation sweep is in flight unwinds the producer + // there. Cleanup must still run, or the refcount sticks at >= 1 + // forever and the OS registration leaks. + val edges = mutableListOf() + val lm = mockk(relaxed = true) + every { lm.allProviders } returns listOf("network") + + lateinit var job: Job + every { lm.getLastKnownLocation(any()) } answers { + // Cancel from inside the sweep, so the subsequent send() throws. + job.cancel() + mockk { every { time } returns 1L } + } + + job = launch { LocationFlow(lm, sdkInt = 30).get(60_000L, 500f) { edges.add(it) }.collect { } } + runCurrent() + job.join() + + assertEquals("the acquire must be released even on cancellation", listOf(true, false), edges) + verify { lm.removeUpdates(any()) } + } + + @Test + fun registersOnExactlyOneProvider() = + runTest { + val lm = manager(providers = listOf("fused", "network", "gps", "passive")) + val job = launch { LocationFlow(lm, sdkInt = 37).get(60_000L, 500f).collect { } } + + runCurrent() + + verify(exactly = 1) { + lm.requestLocationUpdates(any(), any(), any(), any(), any()) + } + + job.cancelAndJoin() + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*LocationFlowTest*' +``` + +Expected: FAIL — compilation error. `LocationFlow` currently takes a `Context`, and `get` has no `onListening` parameter. + +- [ ] **Step 3: Rewrite LocationFlow** + +Replace everything **below** the MIT header in `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt` with: + +```kotlin +package com.vitorpamplona.amethyst.service.location + +import android.annotation.SuppressLint +import android.location.Location +import android.location.LocationListener +import android.location.LocationManager +import android.os.Build +import android.os.Looper +import com.vitorpamplona.quartz.utils.Log +import kotlinx.coroutines.channels.awaitClose +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.callbackFlow +import kotlinx.coroutines.launch + +/** + * Wraps [LocationManager] update registration as a cold [Flow]. + * + * Registers on **one** provider, chosen by [LocationProviderLadder], rather than + * on every provider the device reports. The previous shotgun cost four + * simultaneous registrations — passive, network, fused and gps, the last at + * HIGH_ACCURACY — to produce a 5 km geohash. + * + * Takes a [LocationManager] rather than a `Context` so the registration + * behaviour is unit-testable; the caller does the `getSystemService` lookup. + * + * [onListening] is fired from inside the flow, after a registration succeeds and + * again from `awaitClose`, never as an `onStart`/`onCompletion` pair on the + * returned flow. The distinction matters: an `onStart` fires on collection even + * when nothing registered, so a device with no usable provider would accrue + * location time with no location running, and — because the ledger refcounts the + * two [LocationState] flows together — the unpaired close would steal the other + * flow's holder. + * + * The pair is kept honest from both ends. The acquire cannot fire without a + * registration, because a failure to register throws before reaching it. The + * release cannot be skipped, because everything after the acquire runs inside a + * `try`/`finally` rather than inside `awaitClose` — `send` suspends, so a + * collector that cancels mid-seed would otherwise unwind past an `awaitClose` + * that never ran. + */ +class LocationFlow( + private val locationManager: LocationManager, + private val sdkInt: Int = Build.VERSION.SDK_INT, + private val hasFine: Boolean = false, +) { + @SuppressLint("MissingPermission") + fun get( + minTimeMs: Long, + minDistanceM: Float, + onListening: ((Boolean) -> Unit)? = null, + ): Flow = + callbackFlow { + val locationCallback = + LocationListener { location -> + Log.d("LocationFlow") { "onLocationChanged $location" } + launch { send(location) } + } + + // 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 } + + 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") + } + + 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. `send` below suspends, so it is a cancellation point: + // if the collector cancels while the seed is mid-flight, the + // producer throws there and `awaitClose` is never entered. 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. The finally covers normal close and + // cancellation-during-send alike. + 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 { + Log.d("LocationFlow") { "Last known location is $it" } + send(it) + } + + awaitClose { } + } finally { + Log.i("LocationFlow") { "Stopped listening on $registered" } + locationManager.removeUpdates(locationCallback) + onListening?.invoke(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. + */ + @SuppressLint("MissingPermission") + private fun freshestLastKnownLocation(providers: List): Location? = + providers + .mapNotNull { provider -> + try { + locationManager.getLastKnownLocation(provider) + } catch (e: SecurityException) { + Log.w("LocationFlow", "No permission to read the last known location of $provider", e) + null + } + }.maxByOrNull { it.time } +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*LocationFlowTest*' +``` + +Expected: PASS, 6 tests. + +If `releasesTheRegistrationWhenCancelledDuringTheSeed` fails to *fail* against a version without the `try`/`finally` — i.e. it passes either way — the cancellation is not reaching `send` as expected. Do not delete the test: replace the in-answer `job.cancel()` with a `trySend` on a channel the test awaits, or fall back to asserting the same invariant from `LocationStateTest` by cancelling the collector mid-gate. The invariant is what matters, not this particular provocation. + +`LocationState.kt` will not compile yet — it still calls the old `LocationFlow(context)` and `MIN_TIME`. That is Task 5. If the Gradle run fails on `LocationState.kt` compilation rather than on the tests, that is expected; proceed to Task 5 and re-run both suites there. + +- [ ] **Step 5: Commit** + +The module is red until Task 5 lands. That does not block the commit: the pre-commit hook runs `spotlessCheck` only, which does not compile. + +```bash +./gradlew spotlessApply +git add amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt \ + amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt +git commit -m "feat(amethyst): register one location provider with a paired listening hook" +``` + +--- + +### Task 5: LocationState — the foreground gate + +**Files:** +- Modify (full rewrite of the class body): `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt` +- Test: `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationStateTest.kt` + +**Interfaces:** +- Consumes: `LocationFlow` (Task 4) +- Produces: `class LocationState(context, scope, isForeground, onListening, locationSource)` and the five `const val`s in the naming contract. Task 6 constructs it. + +- [ ] **Step 1: Write the failing test** + +Create `amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationStateTest.kt` with the MIT header, then: + +```kotlin +package com.vitorpamplona.amethyst.service.location + +import android.content.Context +import android.location.Location +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.FlowCollector +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.onCompletion +import kotlinx.coroutines.flow.onStart +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +@OptIn(ExperimentalCoroutinesApi::class) +class LocationStateTest { + private fun locationAt( + lat: Double, + lon: Double, + ): Location = + mockk { + every { latitude } returns lat + every { longitude } returns lon + } + + /** Counts subscriptions and completions of the underlying location source. */ + private class SourceProbe( + private val body: suspend FlowCollector.() -> Unit, + ) { + var subscriptions = 0 + private set + var completions = 0 + private set + + val live: Int get() = subscriptions - completions + + fun source(): (Long, Float) -> Flow = + { _, _ -> + flow(body) + .onStart { subscriptions++ } + .onCompletion { completions++ } + } + } + + private fun neverEmits() = SourceProbe { awaitCancellation() } + + private fun emitsOnceThenHangs( + lat: Double, + lon: Double, + ) = SourceProbe { + emit(locationAt(lat, lon)) + awaitCancellation() + } + + private fun stateWith( + scope: CoroutineScope, + foreground: MutableStateFlow, + probe: SourceProbe, + ) = LocationState( + context = mockk(relaxed = true), + scope = scope, + isForeground = foreground, + locationSource = probe.source(), + ) + + @Test + fun doesNotListenWhileBackgrounded() = + runTest { + val probe = neverEmits() + val foreground = MutableStateFlow(false) + val state = stateWith(backgroundScope, foreground, probe) + state.setLocationPermission(true) + + backgroundScope.launch { state.geohashStateFlow.collect { } } + advanceUntilIdle() + + assertEquals(0, probe.subscriptions) + } + + @Test + fun listensOnceWhileForegrounded() = + runTest { + val probe = neverEmits() + val foreground = MutableStateFlow(true) + val state = stateWith(backgroundScope, foreground, probe) + state.setLocationPermission(true) + + backgroundScope.launch { state.geohashStateFlow.collect { } } + advanceUntilIdle() + + assertEquals(1, probe.subscriptions) + assertEquals(1, probe.live) + } + + @Test + fun releasesTheSourceAfterTheGracePeriodAndKeepsTheLastFix() = + runTest { + val probe = emitsOnceThenHangs(56.048839, 12.721029) + val foreground = MutableStateFlow(true) + val state = stateWith(backgroundScope, foreground, probe) + state.setLocationPermission(true) + + backgroundScope.launch { state.geohashStateFlow.collect { } } + advanceUntilIdle() + + val fixWhileForeground = state.geohashStateFlow.value + assertTrue("expected a Success, got $fixWhileForeground", fixWhileForeground is LocationState.LocationResult.Success) + + foreground.value = false + advanceUntilIdle() + + assertEquals("source must be released once backgrounded", 0, probe.live) + assertEquals( + "the last geohash must survive the release for the ~60 synchronous .value readers", + fixWhileForeground, + state.geohashStateFlow.value, + ) + } + + @Test + fun keepsListeningAcrossABackgroundEdgeShorterThanTheGracePeriod() = + runTest { + val probe = neverEmits() + val foreground = MutableStateFlow(true) + val state = stateWith(backgroundScope, foreground, probe) + state.setLocationPermission(true) + + backgroundScope.launch { state.geohashStateFlow.collect { } } + advanceUntilIdle() + assertEquals(1, probe.subscriptions) + + foreground.value = false + advanceTimeBy(LocationState.BACKGROUND_GRACE_MS / 2) + foreground.value = true + advanceUntilIdle() + + assertEquals("a brief app switch must not tear down the registration", 1, probe.subscriptions) + assertEquals(1, probe.live) + } + + @Test + fun doesNotReemitLoadingWhenReturningToForegroundWithACachedFix() = + runTest { + val probe = emitsOnceThenHangs(56.048839, 12.721029) + val foreground = MutableStateFlow(true) + val state = stateWith(backgroundScope, foreground, probe) + state.setLocationPermission(true) + + val seen = mutableListOf() + backgroundScope.launch { state.geohashStateFlow.collect { seen.add(it) } } + advanceUntilIdle() + + foreground.value = false + advanceUntilIdle() + val afterBackground = seen.size + + foreground.value = true + advanceUntilIdle() + + assertTrue( + "returning to foreground must not flash Loading — AroundMeFeedFlow renders an empty feed for it. Saw: ${seen.drop(afterBackground)}", + seen.drop(afterBackground).none { it is LocationState.LocationResult.Loading }, + ) + } + + @Test + fun emitsLoadingOnTheFirstForegroundWhenThereIsNoCachedFix() = + runTest { + val probe = neverEmits() + val foreground = MutableStateFlow(true) + val state = stateWith(backgroundScope, foreground, probe) + state.setLocationPermission(true) + + val seen = mutableListOf() + backgroundScope.launch { state.geohashStateFlow.collect { seen.add(it) } } + advanceUntilIdle() + + assertTrue("expected Loading, saw $seen", seen.any { it is LocationState.LocationResult.Loading }) + } + + @Test + fun reportsLackPermissionRegardlessOfForeground() = + runTest { + val probe = neverEmits() + val foreground = MutableStateFlow(true) + val state = stateWith(backgroundScope, foreground, probe) + state.setLocationPermission(false) + + backgroundScope.launch { state.geohashStateFlow.collect { } } + advanceUntilIdle() + + assertEquals(LocationState.LocationResult.LackPermission, state.geohashStateFlow.value) + assertEquals(0, probe.subscriptions) + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*LocationStateTest*' +``` + +Expected: FAIL — compilation error. `LocationState` has no `isForeground` or `locationSource` parameter and no `BACKGROUND_GRACE_MS`. + +- [ ] **Step 3: Rewrite LocationState** + +Replace everything **below** the MIT header in `amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt` with: + +```kotlin +package com.vitorpamplona.amethyst.service.location + +import android.content.Context +import android.location.Location +import android.location.LocationManager +import com.vitorpamplona.quartz.experimental.bitchat.geohash.GeohashChannelLevel +import com.vitorpamplona.quartz.nip01Core.tags.geohash.GeoHash +import com.vitorpamplona.quartz.nip01Core.tags.geohash.GeohashPrecision +import com.vitorpamplona.quartz.utils.Log +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.FlowCollector +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.catch +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.emitAll +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.flow.transformLatest + +// `toGeoHash` is an extension on Location declared in LocationGeoHash.kt, same +// package, so it needs no import. + +/** + * Turns the device's location into geohashes, listening **only while the app is + * in the foreground**. + * + * The gate is not an optimisation of last resort: `Account` builds 30 + * `SharingStarted.Eagerly` top-nav filter states on the account scope, and + * `AccountSettings.defaultProductsFollowList` ships as `TopFilter.AroundMe`, so + * without it every user with location permission holds a registration for the + * life of the process. See `amethyst/plans/2026-07-29-location-foreground-gate.md`. + * + * Switching the *consumers* to `WhileSubscribed` is not an option: roughly 60 + * call sites read `account.live*FollowLists.value` synchronously rather than + * collecting, and would silently serve a stale or initial value. + */ +class LocationState( + context: Context, + scope: CoroutineScope, + private val isForeground: StateFlow, + /** + * Resource-ledger hook: true while location updates are actively + * registered. Reaches the OS only through the default [locationSource], + * which hands it to [LocationFlow] — a caller that overrides + * [locationSource] (the tests do) is responsible for firing it, or not. + */ + private val onListening: ((Boolean) -> Unit)? = null, + private val locationSource: (Long, Float) -> Flow = { minTimeMs, minDistanceM -> + LocationFlow(context.getSystemService(Context.LOCATION_SERVICE) as LocationManager) + .get(minTimeMs, minDistanceM, onListening) + }, +) { + companion object { + /** A 5 km cell takes 2.5 minutes to cross at 120 km/h; 60s/500m is ample. */ + const val COARSE_MIN_TIME: Long = 60_000L + const val COARSE_MIN_DISTANCE: Float = 500.0f + + /** Building-level geohashes need the tighter profile. */ + const val PRECISE_MIN_TIME: Long = 10_000L + const val PRECISE_MIN_DISTANCE: Float = 100.0f + + /** + * 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. + */ + const val BACKGROUND_GRACE_MS: Long = 5_000L + } + + sealed class LocationResult { + data class Success( + val geoHash: GeoHash, + ) : LocationResult() + + object LackPermission : LocationResult() + + object Loading : LocationResult() + } + + private enum class Gate { NoPermission, Paused, Listen } + + 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 + + @Volatile private var latestPreciseLocation: LocationResult = LocationResult.Loading + + fun setLocationPermission(newValue: Boolean) { + if (newValue != hasLocationPermission.value) { + hasLocationPermission.tryEmit(newValue) + } + } + + /** + * Foreground with an asymmetric delay: leaving the foreground waits out + * [BACKGROUND_GRACE_MS], returning to it is immediate. + * + * `debounce(5000)` would delay both edges, and the duration-selector + * overload that allows an asymmetric delay is `@FlowPreview`. + * `transformLatest` cancels the pending `delay` when foreground returns + * first, which is exactly the semantics wanted, with no preview opt-in. + * + * Known and harmless: [ForegroundTracker] starts at `false`, so on a + * process that starts backgrounded the first emission — and therefore the + * first gate verdict, including `LackPermission` — is delayed by + * [BACKGROUND_GRACE_MS]. Nothing renders while backgrounded, and a process + * that starts into the foreground emits immediately, because the activity's + * `onStart` cancels the pending delay. + */ + @OptIn(ExperimentalCoroutinesApi::class) + private val settledForeground: Flow = + isForeground.transformLatest { foreground -> + if (!foreground) delay(BACKGROUND_GRACE_MS) + emit(foreground) + } + + private val gate: Flow = + combine(hasLocationPermission, settledForeground) { permitted, foreground -> + when { + !permitted -> Gate.NoPermission + foreground -> Gate.Listen + else -> Gate.Paused + } + }.distinctUntilChanged() + + @OptIn(ExperimentalCoroutinesApi::class) + private fun geohashFlow( + tag: String, + charsCount: Int, + minTimeMs: Long, + minDistanceM: Float, + latest: () -> LocationResult, + setLatest: (LocationResult) -> Unit, + ): Flow = + 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) + + // 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) + + 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) + }, + ) + } + } + } + + val geohashStateFlow: StateFlow by lazy { + geohashFlow( + 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) + } + + /** + * Like [geohashStateFlow] but at building-level precision + * ([GeohashChannelLevel.BUILDING] = 8 chars). Location channels truncate this + * to every coarser level (a geohash is a prefix code), so one fix yields the + * whole region→building ladder. Kept separate so the coarser + * [geohashStateFlow] the "around me" feed relies on is unchanged. + * + * Note that Amethyst declares only `ACCESS_COARSE_LOCATION`, so Android + * fuzzes every fix to roughly a 3 km grid and this is not in fact + * building-level today. The profile is kept so the intent survives if the + * app ever requests `ACCESS_FINE_LOCATION`. + */ + val preciseGeohashStateFlow: StateFlow by lazy { + geohashFlow( + 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) + } +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +```bash +./gradlew :amethyst:testPlayDebugUnitTest --tests '*LocationStateTest*' --tests '*LocationFlowTest*' +``` + +Expected: PASS, 7 + 6 tests. `AppModules.kt` still will not compile — it calls the three-argument `LocationState` constructor. That is Task 6. + +- [ ] **Step 5: Commit** + +```bash +./gradlew spotlessApply +git add amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt \ + amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationStateTest.kt +git commit -m "feat(amethyst): gate location listening on foreground" +``` + +`AppModules.kt` lands in Task 6, and the module compiles from there on. + +--- + +### Task 6: Wire it up in AppModules + +**Files:** +- Modify: `amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt` — two edits, anchored on declaration text rather than line numbers, because Step 1 shifts everything below it + +**Interfaces:** +- Consumes: `RefCountedSession` (Task 2), `LocationState` (Task 5) +- Produces: nothing downstream + +- [ ] **Step 1: Add the refcounted session next to the integrator** + +Find the line beginning `private val locationSession = SessionTimeIntegrator(resourceUsage, UsageKeys.LOCATION_MS)` (currently line 369) and insert immediately after it: + +```kotlin + + // LocationState exposes two independent flows that can both be listening at + // once (the "Around Me" feed plus an open geohash chat). Refcounting keeps + // either one stopping from closing the other's segment. + private val locationRefCount = RefCountedSession(locationSession::setActive) +``` + +Add the import alongside the other `service.resourceusage` imports: + +```kotlin +import com.vitorpamplona.amethyst.service.resourceusage.RefCountedSession +``` + +- [ ] **Step 2: Pass the foreground signal and the refcount into LocationState** + +Find the `val locationManager by lazy` declaration (near line 249, unchanged by Step 1 since that insert was below it) and replace this exact block: + +```kotlin + // App services that should be run as soon as there are subscribers to their flows + val locationManager by lazy { + Log.d("AppModules", "LocationManager Init") + LocationState(appContext, applicationIOScope, onListening = { locationSession.setActive(it) }) + } +``` + +with: + +```kotlin + // App services that should be run as soon as there are subscribers to their + // flows. Location additionally releases its OS registration whenever no + // activity is started — see the foreground gate inside LocationState. + val locationManager by lazy { + Log.d("AppModules", "LocationManager Init") + LocationState( + appContext, + applicationIOScope, + isForeground = foregroundTracker.isForeground, + onListening = { locationRefCount.setActive(it) }, + ) + } +``` + +Both `foregroundTracker` (line 333) and `locationRefCount` (added in Step 1) are declared after `locationManager`, which is fine — `locationManager` is `by lazy`, so the references resolve on first access. `locationSession` was already referenced this way. + +- [ ] **Step 3: Verify the whole module compiles, tests pass, and lint is clean** + +```bash +./gradlew :amethyst:compilePlayDebugKotlin +./gradlew :amethyst:testPlayDebugUnitTest +./gradlew :amethyst:lintPlayDebug +``` + +Expected: BUILD SUCCESSFUL for all three. + +Lint is not optional here. This change names `LocationManager.FUSED_PROVIDER` (API 31) on `minSdk = 26`, and keeps `@SuppressLint("MissingPermission")` on a rewritten method. The ladder's KDoc argues the constant is inlined by the compiler and therefore safe on older runtimes — correct, but an argument, and `NewApi` is exactly the check that settles it. If lint flags `NewApi` on `FUSED_PROVIDER`, replace the constant with the literal `"fused"` and keep the explanatory comment. + +If `compilePlayDebugKotlin` reports an unresolved `MIN_TIME` or `MIN_DISTANCE`, a caller was missed — grep for it and fix. + +```bash +grep -rn --include='*.kt' "MIN_TIME\|MIN_DISTANCE" amethyst/src commons/src desktopApp/src +``` + +Expected: only the four `COARSE_*`/`PRECISE_*` constants in `LocationState.kt` and their uses. + +- [ ] **Step 4: Commit** + +```bash +./gradlew spotlessApply +git add amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt +git commit -m "feat(amethyst): wire the location foreground gate and refcounted meter" +``` + +--- + +### Task 7: Verify the acceptance criteria on device + +**Files:** none — this is measurement. + +**Interfaces:** +- Consumes: the whole change +- Produces: the evidence block for the PR description + +- [ ] **Step 1: Install on the Pixel 9a** + +```bash +adb devices -l +./gradlew :amethyst:installPlayDebug +``` + +Expected: one device listed (`model:Pixel_9a`), BUILD SUCCESSFUL. + +- [ ] **Step 2: Record the foreground baseline** + +Open the app to Home, leave it in the foreground, then: + +```bash +adb shell dumpsys location | sed -n '/Location Providers:/,/Historical/p' | grep -c "com.vitorpamplona.amethyst" +adb shell dumpsys location | sed -n '/Location Providers:/,/Historical/p' | grep -A1 "com.vitorpamplona.amethyst" +``` + +Expected: the count is **1** in the steady state where only the "Around Me" feed is live (at most 2 if a geohash chat is also open — the two flows are independent, which is why the ledger refcounts). Before this change it was 4. + +Expected in the request line: `@+60s0ms` and `minUpdateDistance=500.0`. Before this change: `@+10s0ms` and `minUpdateDistance=100.0`. + +- [ ] **Step 3: Verify the registration is released when backgrounded** + +Press Home, wait more than `BACKGROUND_GRACE_MS` (5 s), then: + +```bash +adb shell dumpsys location | sed -n '/Location Providers:/,/Historical/p' | grep -c "com.vitorpamplona.amethyst" +adb shell dumpsys location | grep "com.vitorpamplona.amethyst" | tail -5 +``` + +Expected: the count is **0**, and the last recent-event lines are `-registration` entries timestamped when you pressed Home. + +- [ ] **Step 4: Verify the feed does not flash empty** + +Foreground the app on Home with the top-nav filter set to "Around Me". Note the geohash shown in the spinner. Press Home, wait 10 s, reopen the app. + +Expected: the same geohash is displayed immediately, with no "Loading" state and no empty feed. This is R1; a flash here means the cached-`Success` check is wrong. + +- [ ] **Step 5: Check the ledger invariant after a day of use** + +Open the in-app Resource Usage Report and compare: + +``` +location.ms ≤ app.fgms + 5000 × (number of times the app was backgrounded) +``` + +Both counters are driven by the same `foregroundTracker.isForeground`, so the grace period is the only expected slack. A violation much larger than that indicts `app.fgms` (Finding 4 of the source analysis suspects it of under-reporting) rather than this change. + +- [ ] **Step 6: Record the evidence** + +Append the observed numbers from Steps 2, 3 and 5 to the `## Acceptance criteria` section of `amethyst/plans/2026-07-29-location-foreground-gate.md` under a `### Verified` heading, then: + +```bash +git add amethyst/plans/2026-07-29-location-foreground-gate.md +git commit -m "docs(amethyst): record on-device verification of the location gate" +``` + +--- + +## Self-review notes + +Spec coverage: §A gate → Task 5 (R1 cached-`Success` check, R2 `settledForeground`, R3 `Gate.Paused` emitting nothing, R4 `@Volatile`). §B request shape → Tasks 3 and 4. §C meter → Tasks 2, 4 (R5 pairing via throw) and 6 (R6 wiring). H1 → Task 1. Testing → the test steps of Tasks 2–5. Acceptance criteria → Task 7. + +Not covered by design, and correctly so: the two `Unavailable`-state and Products-default items are Non-goals; the `GeohashChatScreen.kt:161-163` permission latch is a Follow-up. + +Tasks 4 and 5 leave the module temporarily uncompilable. That is a deliberate split — one task spanning `LocationFlow`, `LocationState` and `AppModules` would review far worse — and it costs nothing, because the pre-commit hook is `spotlessCheck` alone. Task 6 restores a green build. diff --git a/amethyst/plans/2026-07-29-location-foreground-gate.md b/amethyst/plans/2026-07-29-location-foreground-gate.md new file mode 100644 index 0000000000..5c844bc117 --- /dev/null +++ b/amethyst/plans/2026-07-29-location-foreground-gate.md @@ -0,0 +1,773 @@ +# Location: foreground-gate the listener, trim the request, fix the meter + +Date: 2026-07-29 +Module: `amethyst` +Origin: Finding 1 of `2026-07-29-resource-report-1.13.0-analysis.md` (1.13.0-PLAY, +Pixel 9a / Android 17) +Revision: 2 — incorporates spec review of 2026-07-29 + +## Context + +The 1.13.0 resource report showed **7.13 h of location listening against 9.1 +seconds of app foreground**, and the accompanying analysis called it the leading +suspect for that day's 11 pp of background battery drain. Root cause given: 30 +`SharingStarted.Eagerly` top-nav filter states on the account scope +(`Account.kt:843-933`), one of which is always `TopFilter.AroundMe` because +`AccountSettings.kt:256` ships that as the Products default. The `AroundMe` +branch collects `locationFlow()`, and an `Eagerly`-shared subscriber holds +`LocationState`'s `WhileSubscribed(5000)` open for the life of the process. + +That chain is real. **The battery conclusion drawn from it is not**, and this +spec is written against the measurement rather than the inference. + +### What the device reports + +`amethyst/src/main/AndroidManifest.xml:80` declares **only** +`ACCESS_COARSE_LOCATION` — no `ACCESS_FINE_LOCATION`, no +`ACCESS_BACKGROUND_LOCATION` — and no service declares +`foregroundServiceType="location"` (the declared types are `mediaPlayback`, +`microphone`, `camera`, `phoneCall`, `shortService`, `dataSync`, `specialUse`). +`targetSdk = 37`. + +`adb shell dumpsys location`, Pixel 9a, 21 d 7 h of uptime. **These are the +`com.vitorpamplona.amethyst` rows** of the per-provider *Historical Aggregate +Location Provider Data* block — not system-wide totals: + +| provider | registration held | **active** | **foreground** | fixes | +|---|---|---|---|---| +| passive | 9 d 14 h 20 m | 8 h 26 m 14 s | 8 h 14 m 50 s | 107 | +| network | 9 d 14 h 20 m | 8 h 26 m 13 s | 8 h 14 m 49 s | 95 | +| fused | 9 d 14 h 20 m | 8 h 26 m 12 s | 8 h 14 m 48 s | 95 | +| gps | 9 d 14 h 20 m | 8 h 26 m 11 s | 8 h 14 m 47 s | 98 | + +Roughly **11 minutes of background-active location in three weeks**, and about +100 delivered fixes per provider. With the app backgrounded at the time of the +dump: `gps provider: service: ProviderRequest[OFF]`, `gps_hardware: +mStarted=false`. + +The cleanest assumption-free comparison: the ledger's **two-day** `location.ms` +total (3.57 h + 7.13 h = 10.70 h) **exceeds the OS's three-week active total** +(8 h 26 m) by 27 %. `location.ms` is not measuring what its label implies. + +**One figure does not reconcile, and is left open.** Registration-held is +9 d 14 h over 21 d 7 h ≈ 10.8 h/day, whereas `location.ms` averages 5.35 h/day +across the two ledger days. If `location.ms` measured subscription existence +these should agree; they are ~2× apart. Candidates: segments open at process +death are lost (the pre-flush hook does not run on a kill, and the report shows +6 process starts across the two days); the ledger covers 2 days while dumpsys +spans 21 with different usage. Not investigated. It does not affect the +conclusion below, which rests on `location.ms` versus OS-*active* time, not +versus registration-held. + +### How far the "no background location" claim generalises + +This matters because the "no behaviour change" argument rests on it, so it is +scoped rather than asserted universally: + +- **API 29+ (Android 10 and up):** `ACCESS_BACKGROUND_LOCATION` gates background + access, and for `targetSdk ≥ 29` an FGS additionally needs + `foregroundServiceType="location"`. Amethyst has neither, so background + registrations are suspended. This is the case the Pixel 9a measurement above + covers. +- **API 26–28 (Android 8–9):** `ACCESS_BACKGROUND_LOCATION` does not exist. + Amethyst *can* receive background location there, throttled by the platform to + a few updates per hour. The gate is a genuine, if small, improvement on these + releases rather than a no-op. + +So "structural" applies to API 29+; on 26–28 the change has real effect. + +### What is actually wrong + +1. **The meter lies.** `location.ms` measures how long a *subscription* existed, + not how long anything listened. That is what made Finding 1 read as the + top-priority battery bug. +2. **The request is 4× redundant.** `LocationFlow.kt:55` iterates + `locationManager.allProviders` and calls `requestLocationUpdates` on each — + passive, network, fused **and** gps (the last tagged `HIGH_ACCURACY`) — every + one at `@+10s0ms, minUpdateDistance=100.0`, to produce a **5 km** geohash. + This burns during the 8 h 14 m the app genuinely is foreground. +3. **The subscription is held for 45 % of device uptime** doing nothing, because + `Eagerly` never lets go. +4. **Every user is exposed**, since Products defaults to `AroundMe` and needs no + opt-in. +5. **Location may be entirely broken on Android 8–11** — see Hypothesis H1. + +## Hypothesis H1 — location is dead below API 31 (unverified) + +Through Android 11, AOSP's `getMinimumPermissionForProvider` required +`ACCESS_FINE_LOCATION` for the `gps`, `passive` and `fused` providers; 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 (API 31) change. + +Amethyst holds coarse only. So on API 26–30 today's `allProviders` loop should +throw `SecurityException` on three of the four providers. Two consequences: + +- The throw escapes the `callbackFlow` builder → `.catch` in `LocationState` → + `LackPermission`. Location would be **non-functional** on Android 8–11. +- The builder aborting means `awaitClose` never runs, so `removeUpdates` is + never called and any registration made before the throw **leaks** for the life + of the process. + +**The leak is iteration-order dependent, and may not occur at all.** +`getLastKnownLocation` is permission-checked per provider too, and +`LocationFlow.kt:56-68` calls it *before* `requestLocationUpdates` on each +iteration. If a fine-only provider comes first in `allProviders` — `passive` +does, in the common AOSP ordering — the throw lands before any registration +exists: dead, but not leaking. A leak requires `network` to precede a fine-only +provider. + +`@SuppressLint("MissingPermission")` at `LocationFlow.kt:40` suppresses the lint +warning, not the runtime check, so this would not have been caught statically. + +**Not reproduced.** Verify before implementing, on the existing +`Medium_Phone_API_26_8_` AVD: grant coarse only, open a screen that subscribes, +and capture **both** the `SecurityException` *and* the actual +`locationManager.allProviders` order (log it). The PR should claim only what that +run observed — "location is dead on Android 8–11" and "registrations leak" are +separate claims and the second may not hold. + +The design below is written to be correct either way (§B excludes +permission-incompatible providers by API level, and catches `SecurityException` +per provider). If H1 holds, this change also **fixes location on Android 8–11**, +which should be called out in the PR. + +### H1 verification result (2026-07-30, Pixel 9a, API 37) + +Partial. The API-level claim could **not** be tested on this hardware: API 31+ +grants coarse-only apps access to every provider, so no `SecurityException` can +appear regardless of whether H1 is true. Only an API ≤ 30 image can settle it. + +What *was* settled is the ordering, which decides the leak sub-claim. +`dumpsys location` recent-events shows the same iteration order on every +registration cycle across two days, all four sharing one registration id +(`88A8E679`), confirming a single `LocationFlow` subscription: + +``` +07-30 07:09:58.282: passive provider +registration .../88A8E679 +07-30 07:09:58.291: network provider +registration .../88A8E679 +07-30 07:09:58.293: fused provider +registration .../88A8E679 +07-30 07:09:58.299: gps provider +registration .../88A8E679 +``` + +`allProviders` yields **passive first**, and `passive` is one of the fine-only +providers below API 31. So on Android 8–11 the throw would land on the first +iteration, before any registration exists: + +- "location is dead on Android 8–11" — **still unverified**, needs API ≤ 30. +- "registrations leak" — **disproved for this ordering**. Dead, but not leaking. + +The PR must not claim the leak. Caveat: the ordering is observed on API 37 and +`getAllProviders()` could order differently on API 26. + +**Unrelated but decisive "before" datum, same session:** with +`mWakefulness=Dozing` (screen off, device dozing) and `MainActivity` sitting in +`mLastPausedActivity`, Amethyst held **four** live registrations at +`@+10s0ms / minUpdateDistance=100.0`. That is the state §A's gate exists to +eliminate, captured on the owner's daily-driver device rather than an emulator. + +## Goals + +- Release the location registration whenever no activity is started. +- Register on one appropriate, permission-compatible provider at an interval + matched to the precision actually needed. +- Make `location.ms` reflect real listening time, correctly, under concurrency. +- No user-visible behaviour change to the "Around Me" feed or geohash chats. + +## Non-goals + +- Changing the Products `AroundMe` default (`AccountSettings.kt:256`). With the + gate in place its cost is bounded to foreground use. Worth revisiting + separately as a product decision. +- Requesting `ACCESS_FINE_LOCATION` or `ACCESS_BACKGROUND_LOCATION`. +- Findings 2–6 of the source analysis. Finding 2 (the relay reconnect storm, + 1.65 GB/day at a 75 % dial-failure rate) is the more likely explanation for + the background battery drain and should be taken next. + +## Design + +### A. The gate + +`LocationState` gains an `isForeground: StateFlow` parameter, wired in +`AppModules.kt:251` from the existing `foregroundTracker` (`AppModules.kt:333`, +registered at `Amethyst.kt:122`). `locationManager` is `by lazy`, so +initialisation order is safe. + +Today's `hasLocationPermission.transformLatest { … }` becomes a three-state gate +over *permission × foreground*, applied identically to `geohashStateFlow` and +`preciseGeohashStateFlow`: + +| gate state | behaviour | +|---|---| +| no permission | emit `LackPermission` (unchanged) | +| permitted, foreground | **R1**: emit `Loading` *only if* no `Success` is cached; then `emitAll(locationSource(…))` | +| permitted, backgrounded | emit nothing; the registration is released and the `StateFlow` retains its last value | + +**R1 is a requirement, not an improvement.** `AroundMeFeedFlow.convert` collapses +to `geotags = emptySet()` for anything that is not `Success`. Without R1 the gate +would make the "Around Me" feed flash empty on **every** return to foreground — a +new, frequent, user-visible regression introduced by this change. (It also fixes +the same flash on permission grant, which exists today.) + +**R1 corollary: the `NoPermission` branch must not clear the cache.** Today's +code emits `LackPermission` without touching `latestLocation` +(`LocationState.kt:94-96`), and that stays. Clearing it is superficially +attractive — a revoked permission arguably should not leave a fix readable — but +consumers already see `LackPermission` from the `StateFlow`; `latestLocation` is +private and its only jobs are seeding `stateIn` and deciding whether `Loading` is +emitted. Clearing it would therefore buy no privacy and would cost an +empty-feed flash on every permission flap, which is precisely what R1 exists to +prevent. The cache is in-memory and dies with the process regardless. + +The `.catch` branch **does** clear the cache, and keeps doing so. That asymmetry +looks arbitrary next to the paragraph above, so to be explicit: it is inherited, +not introduced. Both branches preserve today's behaviour exactly +(`LocationState.kt:87-91` clears on failure, `:94-96` does not clear on missing +permission). This corollary argues against *adding* a clear, not for removing +the existing one — changing it would be an unmotivated behaviour change. The +asymmetry is also defensible on its own terms: a source that failed mid-stream +says something about the fix's provenance, whereas a permission known to be +absent says nothing about a fix already taken. + +**R2 — grace period on the background edge.** The gate must delay the +`foreground → background` transition by **5 s** before tearing down. Without it a +one-second app switch destroys and rebuilds the registration, including a full +`getLastKnownLocation` sweep, so a user flipping between apps pays more than the +steady state. 5 s matches the existing `WhileSubscribed(5000)` and is the same +intent. The `background → foreground` edge is **not** delayed. + +Mechanism, stated because the obvious operator is the wrong one: `debounce(5000)` +delays both edges, and the duration-selector overload that would allow an +asymmetric delay is `@FlowPreview`. Use `transformLatest`, already in this file +and already opted into via `@OptIn(ExperimentalCoroutinesApi::class)`: + +```kotlin +isForeground.transformLatest { fg -> + if (!fg) delay(BACKGROUND_GRACE_MS) + emit(fg) +} +``` + +`transformLatest` cancels the pending `delay` if foreground returns first, which +is exactly the stated semantics, with no preview opt-in. + +**R3 — the retained-value contract.** The "emit nothing" branch is what keeps +this behaviour-neutral: `stateIn` holds the last `Success`, so the ~60 +synchronous `.value` reads across the feed filters +(`HomeNewThreadFeedFilter.kt`, `VideoFeedFilter.kt`, +`DiscoverLongFormFeedFilter.kt`, …) keep seeing the last known geohash. A 5 km +cell does not meaningfully decay while backgrounded. + +**R4 — memory visibility.** `latestLocation` and `latestPreciseLocation` +(`LocationState.kt:63-64`) are plain `var`s today, used only as `stateIn` initial +values. R1 promotes them to control flow, read from a different coroutine than +the `onEach` that writes them. They must become `@Volatile` (or +`MutableStateFlow`). + +**Rejected alternative:** switching `FeedTopNavFilterState.flow` from `Eagerly` +to `WhileSubscribed`. Roughly 60 call sites read +`account.live*FollowLists.value` synchronously rather than collecting; under +`WhileSubscribed` those reads would silently serve a stale or initial value +whenever no collector happened to be active. That is a correctness regression, +not a battery fix. + +**Rejected alternative:** gating only at the `AppModules` wiring point +(`geolocationFlow = { … }`). Smaller diff, but it leaves the raw +`geohashStateFlow` as a loaded gun for the next eager consumer, does nothing for +`preciseGeohashStateFlow`, and introduces a second `StateFlow` layer over the +same data. + +### B. Request shape + +`LocationFlow.get` registers on **one** provider, chosen by a ladder over +**provider existence and permission compatibility** — both static facts: + +``` +chooseProviders(sdkInt, hasFine, exists) -> List: + API 31+ or hasFine → [FUSED, NETWORK, GPS, PASSIVE] filtered by exists + API < 31, coarse → [NETWORK] filtered by exists (see H1) +``` + +It returns the **ordered candidate list**, not a single choice, because the +per-provider `SecurityException` fall-through below needs somewhere to fall to. +An empty list means no compatible provider exists. + +**The ladder deliberately does not consult `isProviderEnabled`.** Today's code +registers regardless of enabled state, and such a registration goes live by +itself when the user enables location — including from the quick-settings shade +without leaving the app, which is exactly what someone does after seeing "Around +Me" empty. A guard evaluated once at subscription start would lose that, and the +foreground-transition restart does not cover the in-app path. Selecting on +existence keeps the property with no `PROVIDERS_CHANGED_ACTION` receiver. If +field reports show dead feeds on devices where the chosen provider exists but is +disabled while another is enabled, adding that receiver is the follow-up. + +`requestLocationUpdates` is wrapped in a per-provider `SecurityException` catch +that falls through to the next rung, so H1 cannot abort the builder and leak +registrations regardless of how the AOSP check actually behaves. + +**When no provider can be registered** — the candidate list was empty, or every +rung threw — `LocationFlow` **throws** `SecurityException`. It cannot emit +`LackPermission`: the seam is `(Long, Float) -> Flow`, and +`LackPermission` is a `LocationState.LocationResult`, which `LocationFlow` has no +way to express. Throwing routes it through the `.catch` already present in +`LocationState` (`LocationState.kt:87-91`, `:126-130`), which sets +`latestLocation = LackPermission` and emits it — the existing, unchanged path. + +Throwing covers **both** failure cases, and it subsumes R5's `registered`-flag +guard: the throw happens before the acquire, so `onListening(true)` cannot fire +without a live registration and no separate flag is needed. That is only half of +R5's pairing, though — see R5 for the release half, which the throw does **not** +cover and which needs `try`/`finally`. + +**Stated decision: `LackPermission` stays conflated with "no usable provider".** +That value renders `R.string.lack_location_permissions` — "No Location +Permissions" — at `DisplayLocationObserver.kt:49` and `FeedFilterSpinner.kt:224`, +which is wrong for a coarse-only pre-31 device that has no `network` provider. +The conflation is accepted rather than introduced: if H1 holds, today's +`SecurityException` already lands in the same `.catch` and shows the same wrong +message. Adding an `Unavailable` state would ripple through four UI `when`s plus +`LocationState` (10 references across 5 files) and belongs with the H1 fix +messaging, not here. Recorded as a follow-up. + +The `getLastKnownLocation` seed stays a sweep across all providers, taking the +freshest result. It requires no registration and is what makes the first geohash +appear immediately rather than after a fix. + +`MIN_TIME` / `MIN_DISTANCE` split into two profiles, passed per call: + +| flow | precision | interval / distance | provider set | +|---|---|---|---| +| `geohashStateFlow` | `KM_5_X_5` | 10 s / 100 m → **60 s / 500 m** | 4 → 1 | +| `preciseGeohashStateFlow` | `BUILDING` (8 chars) | 10 s / 100 m (kept) | 4 → 1 | + +Both rows change: the ladder narrows the precise flow's provider set too, and +below API 31 that means `network` only, no GPS. Academic while the app holds +coarse only (see Follow-ups), but it is not "unchanged". + +At 120 km/h a 5 km cell takes 2.5 minutes to cross, so 60 s / 500 m has no +observable effect on the feed. + +**Rejected alternative — one shared source at the fine profile,** deriving the +coarse geohash by prefix truncation. It halves registrations and removes the need +for `RefCountedSession` entirely, but it upgrades the **common** case — the +"Around Me" feed alone, which is always on via the Products default — from +60 s/500 m to 10 s/100 m. That trades the change's main win for a rarer one. + +**Rejected alternative — one shared source whose profile tracks the finest +active subscriber.** Recovers the above and is the best of the three on both +axes, but it is refcounting with the counter moved from the meter into the +request path, for a benefit bounded by how often the two flows overlap. They +overlap only while one of three composable-scoped, foreground-only screens is +open (`GeohashChatScreen`, `NewGeohashChatScreen`, +`GeohashLocationPickerDialog`). Not worth the machinery; revisit if that changes. + +### C. The meter + +`AppModules.kt:251` hands both flows the same non-refcounted +`SessionTimeIntegrator`, so `setActive(false)` from either closes the segment +while the other is still listening. Both can be live at once — the "Around Me" +feed plus an open geohash chat. + +**R5 — the hook moves inside `LocationFlow`, and both edges are paired.** Today +`onListening(true)` is an `onStart` on the flow returned by `LocationFlow.get`, +so it fires on *collection* whether or not anything was registered — meaning a +device with no usable provider accrues `location.ms` with nothing listening, +reintroducing the exact defect this section exists to fix. The hook must instead +fire from inside the `callbackFlow`, after `requestLocationUpdates` returns +without throwing, and again on the way out. + +The obvious "way out" is `awaitClose`, and that would introduce a worse bug than +it fixes — twice over. First, `awaitClose` runs on every normal +completion, including one where no rung ever registered, so it would fire an +**unpaired** `onListening(false)`. With R6 that does not merely under-count — it +decrements a holder it never acquired, stealing another flow's. Concretely: +`geohashStateFlow` registers (`holders = 1`), `preciseGeohashStateFlow` fails to +register and closes (`holders = 0`), and the session latches off while the coarse +flow is still listening. `coerceAtLeast(0)` does not help; the count never went +negative. + +Second — and this is the one that survives fixing the first — `awaitClose` also +fails to run at all on some paths that *did* register. See below. + +The pair therefore has to be guaranteed from **both** ends, and the two ends need +different mechanisms. + +*No acquire without a registration* is §B's **throw**: if no rung registers, the +builder throws before reaching the acquire at all. + +*No acquire without a release* needs `try`/`finally`, **not** `awaitClose`. This +is the subtlest point in the document, so the justification below is the one that +was **demonstrated**, not the one that sounds most obvious. + +Anything between the acquire and `awaitClose` that unwinds skips cleanup parked +inside `awaitClose`, because `awaitClose` is never reached to register it. The +registration then leaks and the refcount sticks at ≥ 1 for the life of the +process, so `location.ms` accrues forever with nothing listening — this exact +defect, arrived at from the other direction, and unrecoverable once hit. + +The **proven** path is the seed throwing a non-cancellation exception: +`getLastKnownLocation` is a binder call and can fail. The regression test +`releasesTheRegistrationWhenTheSeedThrows` provokes exactly this and was watched +failing against an `awaitClose`-only implementation +(`expected:<[true, false]> but was:<[true]>`). + +A cancellation during the seed is *in principle* a second such path, since `send` +is a suspending call. Recorded honestly: **this one could not be reproduced.** +Two attempts during implementation both produced tests that passed against a +deliberately broken implementation, because `callbackFlow`'s channel is buffered, +so `send` returns without suspending and never observes the cancel. Do not treat +the cancellation story as the reason for the `try`/`finally`; a future reader who +tries to reproduce it, fails, and concludes the guard is unnecessary would +reintroduce the leak. + +```kotlin +var registered: String? = null +for (provider in candidates) { + try { + locationManager.requestLocationUpdates(provider, minTimeMs, minDistanceM, callback, Looper.getMainLooper()) + registered = provider + break + } catch (e: SecurityException) { /* next rung */ } +} +if (registered == null) throw SecurityException("no usable location provider") + +onListening?.invoke(true) // cannot fire without a registration +try { + freshestLastKnownLocation(providers)?.let { send(it) } // suspends — cancellable + awaitClose { } // only to satisfy callbackFlow's contract +} finally { + locationManager.removeUpdates(callback) + onListening?.invoke(false) // cannot be skipped +} +``` + +`onListening?.invoke(true)` sits immediately before the `try`, with no suspension +between them, so the acquire cannot happen outside the block that guarantees its +release. + +`trySend` for the seed would also close this particular hole, being +non-suspending. It is rejected because it leaves the invariant resting on nobody +adding a suspending call to that block later — vigilance rather than +impossibility, which is the standard the rest of R5 is held to. + +**R6 — refcounting.** An `AtomicInteger` beside the `setActive` call is not +sufficient: two threads can leave the counter at 1 while the last +`setActive(false)` lands after the `setActive(true)`, latching the session off. +The count and the transition must move under one lock. New class in +`service/resourceusage/`: + +```kotlin +class RefCountedSession(private val setSessionActive: (Boolean) -> Unit) { + private val lock = Any() + private var holders = 0 + + fun setActive(active: Boolean) = + synchronized(lock) { + holders = if (active) holders + 1 else (holders - 1).coerceAtLeast(0) + setSessionActive(holders > 0) + } +} +``` + +It takes the setter as a lambda rather than a `SessionTimeIntegrator` because +that is all it needs, and because constructing a real integrator in a unit test +would drag in a `ResourceUsageAccountant`, a `ResourceUsageStore` and a temp +file to observe one boolean. + +`AppModules` wires `RefCountedSession(locationSession::setActive)` and passes +`onListening = { locationRefCount.setActive(it) }`. The outer +lock serialises entry into `SessionTimeIntegrator.setActive`, whose own lock is +then nested but never acquired in the reverse order, so there is no deadlock. +`coerceAtLeast(0)` guards an unmatched release. + +### What `location.ms` means after this change + +Stated plainly, because the finding that opened this spec is "the meter lies" +and the next reader should not over-trust the fixed number the way the last one +over-trusted the broken one: + +> `location.ms` measures **how long a location registration was held while the +> app was in the foreground**. It is not radio-on time and not an energy +> figure. A `network`-provider registration at 60 s costs close to nothing; a +> `gps` registration at 10 s costs a great deal. The counter cannot tell them +> apart. + +Reading it as a battery signal requires knowing which provider was chosen — +which the ledger does not record. Recording the chosen provider as a separate +counter is a possible follow-up. + +## Testing + +JVM unit tests — JUnit + MockK + `kotlinx-coroutines-test`, no Robolectric, +alongside the existing `service/resourceusage/ResourceUsageLedgerTest.kt`. + +**Gate.** `LocationState` gains `locationSource: (Long, Float) -> Flow`, +defaulting to `LocationFlow(context.getSystemService(…) as LocationManager)::get` +(see *Registration pairing* for why `LocationFlow` now takes the manager rather +than the `Context`). That is the seam: +`Location.toGeoHash` is `GeoHash.encode(lat, lon, chars)` from quartz — pure +Kotlin — and `unitTests.isReturnDefaultValues = true` is already set, so a +`mockk` with stubbed `latitude`/`longitude` suffices. Against a +counting fake source: + +- backgrounded + permitted → source never subscribed +- foreground + permitted → subscribed exactly once +- foreground → background → subscription released after the R2 grace period, + last `Success` still readable via `.value` +- background edge shorter than the grace period → subscription **not** torn down +- return to foreground with a cached `Success` → **no** `Loading` emission (R1) +- return to foreground with no cached fix → `Loading` first +- permission revoked → `LackPermission` regardless of foreground state + +**Provider ladder.** Extracted as a pure function +`chooseProviders(sdkInt: Int, hasFine: Boolean, exists: (String) -> Boolean): +List` so §B is covered rather than sitting below the seam. Cases: rungs +returned in order; missing rungs filtered out; API < 31 coarse-only yields +`[network]`; API < 31 with fine yields the full ladder; no compatible provider +yields an empty list. + +`hasFine` is **always `false` in production** — the non-goals rule out ever +requesting `ACCESS_FINE_LOCATION`. It is a parameter rather than a constant so +that the function is total over the permission axis and the API < 31 branch can +be tested from both sides, not because fine access is anticipated. If that +changes, the ladder is already correct. + +R5 sits below the `locationSource` seam, so a fake source never fires it. It gets +its own tests against a mocked `LocationManager` — see *Registration pairing* +below. + +**Meter.** `RefCountedSession`: overlapping holders keep the session open; +balanced pairs close it; an unmatched release does not drive the count negative. + +Note what this class **cannot** do: it cannot distinguish an unpaired release +from a legitimate one, so `acquire → unpaired release` closes the session even +while another holder is listening. That is precisely the R5 bug, and +`coerceAtLeast(0)` is no defence against it. **The pairing guarantee belongs to +`LocationFlow`, not here** — which is why it needs its own test below. + +**Registration pairing (R5).** To make this testable rather than device-only, +`LocationFlow` takes a `LocationManager` instead of a `Context` +(`LocationFlow(context)` → `LocationFlow(locationManager)`; the caller in +`LocationState` does the `getSystemService` lookup). A `mockk` +then covers: + +- every rung throws `SecurityException` → the flow throws and `onListening` fires + **neither** edge +- `chooseProviders` returns an empty list → same: throws, neither edge +- an earlier rung throws and a later one succeeds → registration falls through, + exactly one `true` +- a successful registration → exactly one `true`, and exactly one `false` plus + `removeUpdates` on cancellation +- **cancellation mid-seed**, while the `getLastKnownLocation` sweep is in flight + → both edges still fire and `removeUpdates` is still called. This is the case + the `try`/`finally` exists for; without it the test fails by hanging the + refcount at 1 rather than by throwing, so assert on the edges, not on the + absence of an exception. +These cover the *semantics* only — the two-thread interleaving that motivates the +lock is made unobservable by the lock itself and is not reproduced by any test +here. + +## Acceptance criteria + +On device, re-running the measurement above: + +- **Backgrounded:** after the 5 s grace period, `adb shell dumpsys location` + shows no `com.vitorpamplona.amethyst` entry under any provider's `listeners:`, + and a `-registration` in the recent-events log. +- **Foregrounded:** **one registration per actively-collected flow — at most + two**, and one in the steady state where only the "Around Me" feed is live + (§C exists precisely because the two flows may overlap). Not four. The coarse + registration reads `@+60s0ms` / `minUpdateDistance=500.0` rather than + `@+10s0ms` / `100.0`. +- The historical aggregates are cumulative since boot; compare deltas across a + foreground/background cycle, not absolute totals. +- **Invariant:** a subsequent in-app Resource Usage Report shows + + ``` + location.ms ≤ app.fgms + 5 s × (background transitions) + ``` + + Both counters are driven by the same `foregroundTracker.isForeground` flow, so + without R2 this would hold exactly. R2 is deliberately the error term: the + registration really *is* live during the grace period, so counting it is the + honest reading, and a stated fudge factor beats an invariant quietly known to + be false. The term is not negligible — the source report shows 6 process + starts across two days, and app switches are far more frequent than that — so + writing `location.ms ≤ app.fgms` would guarantee that the first person to + check it files a bug against this change. + + Second caveat: Finding 4 of the source analysis suspects `app.fgms` of + under-reporting, so a violation beyond the grace term indicts that counter + rather than this one. +- If H1 holds: location works on the API 26 AVD after the change and did not + before. + +### Verified on device (2026-07-30, Pixel 9a / Android 17, API 37) + +Measured against the `benchmark` variant — `initWith(release)`, so R8-minified with +the shipping proguard rules, installed as `com.vitorpamplona.amethyst.benchmark` +beside the untouched Play install. The Play install was force-stopped for the +duration so its own (unfixed) registrations could not be mistaken for these. + +| criterion | before | after | +|---|---|---| +| registrations, foreground | **4** (passive, network, fused, gps) | **1** (fused) | +| request profile | `@+10s0ms HIGH_ACCURACY`, `minUpdateDistance=100.0` | `@+1m0s0ms BALANCED`, `minUpdateDistance=500.0` | +| registrations, backgrounded | **4**, held while `mWakefulness=Dozing` | **0** | + +Event trace for one full cycle, process alive throughout (pid 28682): + +``` +17:57:00.117 +registration fused …/40F5A6D7 @+1m0s0ms BALANCED, minUpdateDistance=500.0 +17:57:33.894 -registration fused …/40F5A6D7 ← HOME pressed, released after the grace +17:58:05.798 +registration fused …/091EDA96 @+1m0s0ms BALANCED, minUpdateDistance=500.0 + (HOME then reopen within 2 s — no -/+ pair; 091EDA96 survives) +``` + +- **§A gate** — zero registrations while backgrounded, with the process still + alive. That is the state the change exists to create; before, four + registrations survived screen-off and doze. +- **§B request shape** — one provider, top of the ladder (`fused`), at exactly + `COARSE_MIN_TIME` / `COARSE_MIN_DISTANCE`. The OS tags it `(COARSE)` and + coalesces the effective service request to `@+10m0s0ms LOW_POWER`. +- **R2 grace period** — a sub-grace app switch produced **no** teardown/rebuild + pair, so a brief switch no longer costs a re-registration and a fresh + `getLastKnownLocation` sweep. + +**The OS aggregate after four foreground/background cycles is the headline +result**, because it is the same counter shape `location.ms` measures: + +``` +com.vitorpamplona.amethyst.benchmark: + min/max interval = 60s/60s + total/active/foreground duration = +2m33s542ms / +2m33s456ms / +2m33s531ms + locations = 4 +``` + +Total ≈ active ≈ foreground, all three within 90 ms — against the Play install's +`9d14h20m / 8h26m / 8h14m`, where registration was held for 45 % of uptime while +only 1.7 % was active. The four foreground windows sum to 153.6 s, matching the +aggregate exactly, so nothing is held outside them. Registration-held time now +*equals* foreground time, which is precisely what makes `location.ms` honest: the +counter measures registration lifetime, and that quantity is no longer divorced +from reality. + +**A fix arrives within milliseconds of every re-registration**, which bounds the +staleness the coarser profile was feared to introduce: + +``` +17:57:00.117 +registration → 17:57:00.127 delivered location[1] (10 ms) +17:58:05.798 +registration → 17:58:05.802 delivered location[1] ( 4 ms) +17:59:09.965 +registration → 17:59:09.967 delivered location[1] ( 2 ms) +18:00:09.206 +registration → 18:00:09.213 delivered location[1] ( 7 ms) +``` + +The `fused` provider hands over its cached fix on registration, so the window in +which a returning user could act on a stale geohash is milliseconds, not the 60 s +poll interval. Caveat: that cache is warm on this device because Maps and GMS +keep it fresh; on a device with no other location consumer it could be colder, +which is what `freshestLastKnownLocation` exists to cover. + +**Side-by-side A/B, same device, same instant, both clients backgrounded and +running.** The unmodified release client (1.13.1, installed via Obtainium, pid +5552) and the benchmark build of this branch (pid 28682) were sampled together: + +``` +com.vitorpamplona.amethyst/B7B299BE {bg, na} (COARSE) Request[PASSIVE, minUpdateDistance=100.0] (inactive) +com.vitorpamplona.amethyst/B7B299BE {bg, na} (COARSE) Request[@+10m LOW_POWER, minUpdateDistance=100.0] (inactive) +com.vitorpamplona.amethyst/B7B299BE {bg, na} (COARSE) Request[@+10m LOW_POWER, minUpdateDistance=100.0] (inactive) +com.vitorpamplona.amethyst/B7B299BE {bg, na} (COARSE) Request[@+10m LOW_POWER, minUpdateDistance=100.0] (inactive) + ← com.vitorpamplona.amethyst.benchmark: no rows at all +``` + +Four held registrations versus zero. Note the release client's rows are all +`{bg, na} … (inactive)`: the OS has throttled the effective interval to 10 +minutes and suspended delivery, exactly as §"What the device reports" describes — +but the **registration is still held**, and registration-held time is precisely +what `location.ms` counts. That is the inflation, visible in one frame. + +Naming note for anyone re-reading the numbers above: both artifacts are `play` +**flavor** builds and differ only by buildType, so "the Play install" is an +ambiguous label. The unmodified client here is the *release* build, and on this +device it came from Obtainium rather than Google Play. + +### Ledger invariant confirmed (2026-07-31, benchmark client, in-app report) + +The acceptance criterion `location.ms ≤ app.fgms + 5 s × transitions` now checks +out against accumulated data: + +| | `location.ms` | `app.fgms` | ratio | +|---|---|---|---| +| release client 1.13.0, day 20663 (before) | 25,660,172 | 9,147 | **2,805×** | +| benchmark, day 20664 (permission granted mid-day) | 3m7.3s | 11m31.0s | 0.27× | +| benchmark, **day 20665** (granted all day) | **2m10.8s** | **1m56.9s** | **1.12×** | + +Day 20665 is the clean case: `location.ms` exceeds `app.fgms` by 13.9 s, which +requires ≥ 3 background transitions to fall inside the grace allowance — met by +the report navigation plus an `am start`. Day 20664 independently reconciles with +the `dumpsys` measurement: 2m33.5s of OS registration-held + 4 × 5 s grace = +~2m53.5s predicted, 3m7.3s actual, the residual being foreground use after the +measurement ended. **The ledger and the OS now agree**, where before they were +irreconcilable (10.7 h ledger vs 8h26m OS-active over three weeks). + +**Limitation:** this is not a within-package before/after. `location.ms` is +absent from days 20648–20663 because the benchmark client had location permission +*denied* until 2026-07-30; the "before" is no data, not inflated data. + +### The same data closes the battery question + +Over days 20659–20665 on this device: **5m18s** of location listening against +**596 pp** of background battery drain (~85 pp/day). Location cannot be a +meaningful contributor at that ratio — Finding 1 is settled, and not in the +direction the original analysis assumed. + +Three consumers visible in the same report, none of them location: + +- `service.alwayson.ms` ≈ **23.9 h/day** (148.9 h over 7 days) — an always-on + foreground service running essentially continuously. Largest structural + difference from a stock client; worth confirming it is deliberately enabled. +- **Finding 2, unchanged.** 3,732 relay-hours over 7 days. Day 20664 alone: + 9,831 successful dials against 25,133 failures = **71.9 %**, matching the + original report's 75 %. +- **Finding 4, now on cellular.** Day 20664 `net.other.mobile.bg.activems = + 52,392,613` — **14.6 h** of background mobile active time with **0 requests and + 0 bytes**. The three-moment `isForeground()` sampling, exactly as diagnosed, so + the fg/bg split in this report still cannot be trusted. + +Caveat: the benchmark client's round-the-clock always-on service makes its +battery figures non-comparable to a stock install. The relay and `net.other` +figures do match the release client's original report closely. + +Not verified by this run: **R1** (no `Loading` flash on return to foreground) is +a visual property and needs an observer at the screen; the unit test +`doesNotReemitLoadingWhenReturningToForegroundWithACachedFix` covers it and +mutation-fails correctly without the guard. The `location.ms ≤ app.fgms + 5 s × +transitions` invariant needs a day of accumulated ledger data. + +## Follow-ups (not in this change) + +- **`preciseGeohashStateFlow` is not actually building-level.** With only + `ACCESS_COARSE_LOCATION`, Android fuzzes every fix to roughly a 3 km grid, so + the 8-char geohash and the location chat channels built on it are far coarser + than they claim (`LocationState.kt:104-141`, `GeohashChatScreen.kt:165`, + `NewGeohashChatScreen.kt:309`). The profile is kept intact here so the intent + survives if the app ever requests `ACCESS_FINE_LOCATION`; whether to request + it, or to stop advertising building-level precision, is a separate decision. +- **`GeohashChatScreen.kt:161-163` is a one-way permission latch** — it calls + `setLocationPermission(true)` inside an `if (isGranted)` rather than passing + the boolean, as every other caller does (`LoggedInPage.kt:144`, + `LocationAsHash.kt:64`, `NewGeohashChatScreen.kt:285`, + `GeohashLocationPickerDialog.kt:270`). Once set, a revoked permission is never + reflected back into the shared `LocationState`. Small, in the blast radius, + and cheap. +- **An `Unavailable` `LocationResult`**, distinct from `LackPermission`, so a + device with no usable provider stops being told "No Location Permissions" when + it has them. Ten references across five files + (`DisplayLocationObserver.kt`, `FeedFilterSpinner.kt`, `HomeScreen.kt`, + `NewGeohashChatScreen.kt`, `LocationState.kt`). Belongs with the H1 fix + messaging — see the stated decision in §B. +- Recording the chosen provider as a ledger counter, so `location.ms` can be + read as a cost signal. +- Whether Products should still default to `TopFilter.AroundMe`. +- Finding 2 — the relay reconnect storm.