fix: replace continuous GPS with on-demand one-shot in Group Tracker

[MatthieuTexier]

Summary

• TelemetryLocationTracker maintained a continuous FusedLocationProvider subscription (30s/15s) to cache positions, but TelemetryCollectorManager only reads it every 5+ minutes. This kept GPS hardware active ~100% of the time for no benefit.
• Replace with on-demand one-shot fixes that activate GPS only when a send needs a position (~10-20s per fix). Reduce cache TTL from 5 min to 30s to ensure fresh positions.

Measured impact (5-min benchmark, Android 16)

Metric	Before	After
GNSS energy	2.43 mAh	0.009 mAh (÷270)
GPS active time	5m01s (100%)	1.1s (0.4%)
Hourly drain (GNSS)	~29 mAh/h (~5.9%/h)	~0.11 mAh/h (~0.02%/h)

Feature behavior unchanged — Group Tracker sends positions at the same interval with the same accuracy.

Test plan

• Enable Group Tracker ("Share with group"), configure 5-min interval
• Verify positions are received by group host at expected interval
• Verify position accuracy is unchanged (HIGH_ACCURACY one-shot)
• Run adb shell dumpsys batterystats — confirm no continuous GPS wakelock from Columba
• Test on device without GMS (LocationCompat fallback path)

[greptile-apps]

Greptile Summary

After several fixup commits, the implementation lands on a hybrid model: continuous BALANCED_POWER_ACCURACY tracking at a derived interval keeps the cache warm, and a HIGH_ACCURACY one-shot fills in when the cache is stale. Previous review concerns (the locationTrackingActive guard, stale OS-cached fix rejection, and AtomicBoolean for the pre-R cancellation race) are all addressed. Two minor P2 points remain: the PR description still describes the abandoned pure one-shot approach, and getTrackedLocationAgeMs reads an instance-level timestamp that isn't coupled to the specific location being tested, which can cause a stale hit in a narrow concurrent-update window.

Confidence Score: 5/5

Safe to merge — all previously-raised P1 concerns are resolved; remaining findings are minor P2 items that don't affect correctness in practice.

The three P1/P0 issues from prior review rounds (stop()-race guard, stale OS-fix rejection, AtomicBoolean for pre-R cancellation) are all addressed. The two remaining comments are P2: a documentation mismatch in the PR description, and a narrow concurrent-update race in getTrackedLocationAgeMs whose practical impact is negligible. Neither blocks a merge.

TelemetryLocationTracker.kt — the uncoupled latestTrackedLocationRecordedAtMs field warrants a follow-up refactor if location accuracy SLAs tighten.

Important Files Changed

Filename	Overview
app/src/main/java/network/columba/app/service/TelemetryLocationTracker.kt	Implements hybrid continuous BALANCED_POWER tracking with HIGH_ACCURACY one-shot fallback; locationTrackingActive guard, stale-fix rejection, and cache TTL all look correct; minor race in getTrackedLocationAgeMs reading an instance-level timestamp not coupled to the passed location object.
app/src/main/java/network/columba/app/service/TelemetryCollectorManager.kt	Wires locationTracker.update() correctly in all settings-change paths; sendTelemetryToCollector delegates to the tracker and handles NoLocationAvailable cleanly; no new issues.
app/src/main/java/network/columba/app/util/LocationCompat.kt	AtomicBoolean fix correctly guards against cancellation-from-background-thread racing the main-looper location callback; pre-R timeout and cancel-listener logic look sound.
app/src/test/java/network/columba/app/service/TelemetryCollectorManagerTest.kt	Tests activate the tracker via ensureLocationTrackerActive() and mock LocationCompat.getCurrentLocation to exercise the one-shot path; coverage is adequate for the changed logic.

Sequence Diagram

sequenceDiagram
    participant CM as TelemetryCollectorManager
    participant LT as TelemetryLocationTracker
    participant Cache as latestTrackedLocation
    participant FLP as FusedLocationClient (BALANCED)
    participant OSS as One-shot (HIGH_ACCURACY)

    Note over FLP,Cache: Continuous background subscription
    FLP-->>Cache: cacheTrackedLocation(fix)

    CM->>LT: sendTelemetryToCollector()
    LT->>LT: getTelemetryLocation()
    LT->>Cache: read latestTrackedLocation
    alt Cache hit (isLocationRecent)
        Cache-->>LT: fresh location
    else Cache miss or stale
        LT->>OSS: getCurrentLocation() with 20s timeout
        alt Fix received and isFixFresh
            OSS-->>LT: location
            LT->>Cache: cacheTrackedLocation(fix)
        else Timed out or stale OS fix
            OSS-->>LT: null
        end
    end
    LT-->>CM: Location? (null → NoLocationAvailable)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: app/src/main/java/network/columba/app/service/TelemetryLocationTracker.kt
Line: 187-202

Comment:
**Instance-level timestamp not coupled to the passed location**

`getTrackedLocationAgeMs` reads `latestTrackedLocationRecordedAtMs` from instance state rather than a per-location value. If a background tracking callback fires between the two lines in `getTelemetryLocation`:

```kotlin
val tracked = latestTrackedLocation          // reads old location
if (tracked != null && isLocationRecent(tracked)) {   // now latestTrackedLocationRecordedAtMs == "just now"
```

…then `maxOf(old_fix_time, just_now)` yields an age of ~0 ms, making a stale cached location appear fresh. The window is narrow but the root cause is that the "recorded at" timestamp is instance-global rather than stored alongside the location it belongs to.

Pairing them in a small data class would eliminate the race entirely:

```kotlin
private data class CachedLocation(val location: Location, val recordedAtMs: Long)
@Volatile private var cachedLocation: CachedLocation? = null
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: app/src/main/java/network/columba/app/service/TelemetryLocationTracker.kt
Line: 18-27

Comment:
**KDoc accurately describes implementation; PR description does not**

The class KDoc correctly describes the final approach — continuous `BALANCED_POWER_ACCURACY` tracking with a `HIGH_ACCURACY` one-shot fallback — but the PR title and summary still say "replace continuous GPS with on-demand one-shot." The git history shows an intermediate commit (`bdcb0d5b`) reverted the pure one-shot approach to this hybrid, which is the right call for Doze reliability, but the PR description was never updated. Worth amending the PR description so the reviewed intent matches what was actually shipped.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (11): Last reviewed commit: "fix: replace resultDelivered boolean wit..." | Re-trigger Greptile}

[sentry]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!