fix: Group Tracker: host allowlist, self-echo, host picker stability, background location#593
Conversation
…address migration - Add continuous location tracking for reliable background telemetry sends - Request ACCESS_BACKGROUND_LOCATION permission for telemetry when needed - Seed identity fields from DB on cold start for stable host picker - Migrate legacy truncated collector addresses to full 32-char hashes - Show raw collector address when no contact match found - Preserve identity hashes on settings flow error recovery - Handle CancellationException properly in settings flow
…lexity - Extract location tracking logic from TelemetryCollectorManager into TelemetryLocationTracker to reduce LargeClass violation (1013→820 lines) - Extract executePeriodicSendIteration/executePeriodicRequestIteration helpers to reduce CyclomaticComplexMethod in restartPeriodicSend and restartPeriodicRequest - Extract applySettingsUpdate from SettingsViewModel.loadSettings to reduce CyclomaticComplexMethod - No detekt-baseline.xml changes needed
The previous detektBaseline regeneration had silently added/removed entries unrelated to our changes. Restore the exact file from main now that our refactoring resolved all new violations.
Greptile SummaryThis PR fixes eight interconnected bugs in the Group Tracker telemetry feature. The majority of the fixes are sound and well-tested. However, three issues need attention before merging:
Confidence Score: 3/5
Sequence DiagramsequenceDiagram
participant UI as SettingsScreen / MapViewModel
participant TCM as TelemetryCollectorManager
participant TLT as TelemetryLocationTracker
participant RPC as ReticulumProtocol
participant PY as reticulum_wrapper.py
Note over TCM,PY: Startup — host mode + allowlist sync
UI->>TCM: start()
TCM->>TLT: update(shouldTrack)
TLT-->>TCM: continuous location cached
TCM->>RPC: networkStatus.first { READY }
TCM->>PY: setTelemetryCollectorMode(enabled)
TCM->>PY: setTelemetryAllowedRequesters(hashes) [normalized lowercase]
Note over TCM,PY: Periodic send loop
loop every sendInterval
TCM->>TLT: getTelemetryLocation()
TLT-->>TCM: cached Location (or one-shot fallback)
alt collector == local device
TCM->>PY: storeOwnTelemetry(locationJson)
Note right of PY: ⚠ fails if telemetry_collector_enabled=False
else collector == remote
TCM->>RPC: sendLocationTelemetry(destHash, locationJson)
RPC->>PY: LXMF delivery to peer
end
end
Note over PY: Incoming telemetry request
PY->>PY: requester_hash = source.hex().lower()
PY->>PY: check requester_hash in telemetry_allowed_requesters (set)
alt allowed
PY-->>RPC: FIELD_TELEMETRY_STREAM response
else blocked
PY-->>RPC: silently ignore
end
Note over UI,TCM: Self-echo filtering on map
UI->>TCM: getLocalIdentityHashes()
TCM-->>UI: [destHash, identityHash] (lowercase)
UI->>UI: filter markers where senderHash == localHash
Last reviewed commit: 496544b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
app/src/main/java/com/lxmf/messenger/service/TelemetryLocationTracker.kt
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Location callback fields are written from the main looper thread and read from background coroutines. Without @volatile the JVM memory model does not guarantee cross-thread visibility.
The inline comment claimed freshness was based on receivedAt, but calculateMarkerState actually uses the sender emission timestamp.
|
@greptile |
| private suspend fun delayUntil(targetTimeMs: Long) { | ||
| val timeUntil = maxOf(0L, targetTimeMs - System.currentTimeMillis()) | ||
| val capped = minOf(timeUntil, 30_000L) | ||
| delay(if (capped > 0) capped else 30_000L) | ||
| } |
There was a problem hiding this comment.
delayUntil() uses unnecessary 30s fallback when target is in the past
When targetTimeMs is already in the past (which occurs on the very first periodic send when lastSendTime and lastSendAttemptAt are both zero/null), timeUntil resolves to 0, capped becomes 0, and the condition if (capped > 0) capped else 30_000L causes an unnecessary 30-second delay instead of returning promptly.
A simpler implementation that caps at 30s only when timeUntil > 0:
| private suspend fun delayUntil(targetTimeMs: Long) { | |
| val timeUntil = maxOf(0L, targetTimeMs - System.currentTimeMillis()) | |
| val capped = minOf(timeUntil, 30_000L) | |
| delay(if (capped > 0) capped else 30_000L) | |
| } | |
| private suspend fun delayUntil(targetTimeMs: Long) { | |
| val timeUntil = maxOf(0L, targetTimeMs - System.currentTimeMillis()) | |
| delay(minOf(timeUntil, 30_000L)) | |
| } |
This is safe because delay(0L) is a no-op coroutine suspension in Kotlin that immediately resumes, allowing the loop to quickly re-run executePeriodicSendIteration() to check whether it is time to send again.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/service/TelemetryCollectorManager.kt
Line: 636-640
Comment:
**`delayUntil()` uses unnecessary 30s fallback when target is in the past**
When `targetTimeMs` is already in the past (which occurs on the very first periodic send when `lastSendTime` and `lastSendAttemptAt` are both zero/null), `timeUntil` resolves to 0, `capped` becomes 0, and the condition `if (capped > 0) capped else 30_000L` causes an unnecessary 30-second delay instead of returning promptly.
A simpler implementation that caps at 30s only when `timeUntil > 0`:
```suggestion
private suspend fun delayUntil(targetTimeMs: Long) {
val timeUntil = maxOf(0L, targetTimeMs - System.currentTimeMillis())
delay(minOf(timeUntil, 30_000L))
}
```
This is safe because `delay(0L)` is a no-op coroutine suspension in Kotlin that immediately resumes, allowing the loop to quickly re-run `executePeriodicSendIteration()` to check whether it is time to send again.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
When a new user with no existing permissions toggles telemetry on, the code shows the permission sheet and sets This leaves the user with telemetry enabled but no background location access on Android 10+, causing silent To fix this, after foreground permission is granted, the code should check whether background permission is also needed and request it before enabling telemetry: Prompt To Fix With AIThis is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/ui/screens/SettingsScreen.kt
Line: 297-300
Comment:
**Background permission skipped after foreground grant flow**
When a new user with no existing permissions toggles telemetry on, the code shows the permission sheet and sets `pendingTelemetryAction` to enable telemetry. However, when `telemetryPermissionLauncher` callback fires (line 124) after foreground permission is granted, it directly invokes `pendingTelemetryAction` — calling `setTelemetryCollectorEnabled(true)` **without ever requesting `ACCESS_BACKGROUND_LOCATION`**.
This leaves the user with telemetry enabled but no background location access on Android 10+, causing silent `NoLocationAvailable` failures for all background sends. The two-step permission flow only works correctly for users who already had foreground permission before entering this branch.
To fix this, after foreground permission is granted, the code should check whether background permission is also needed and request it before enabling telemetry:
```suggestion
val telemetryPermissionLauncher =
rememberLauncherForActivityResult(
contract = ActivityResultContracts.RequestMultiplePermissions(),
) { permissions ->
val granted = permissions.values.any { it }
if (granted) {
// Foreground granted; check if background is also needed
if (LocationPermissionManager.hasTelemetryBackgroundPermission(context)) {
pendingTelemetryAction?.invoke()
pendingTelemetryAction = null
} else {
// Now request background before enabling telemetry
telemetryBackgroundPermissionLauncher.launch(Manifest.permission.ACCESS_BACKGROUND_LOCATION)
// pendingTelemetryAction is preserved for the background launcher
}
} else {
pendingTelemetryAction = null
}
}
```
How can I resolve this? If you propose a fix, please make it concise. |
- MapViewModel: replace bidirectional prefix-match with equality check for self-echo detection (hashes are now full 32-char lowercase) - TelemetryCollectorManager: remove unnecessary 30s fallback in delayUntil() when target is in the past; delay(0) resumes immediately - SettingsScreen: after foreground location grant, check and request background permission before enabling telemetry (fixes silent NoLocationAvailable on Android 10+)
|
@greptile |
- Initialize lastSendAttemptAt/lastRequestAttemptAt from persisted success times to avoid immediate retry after process death - Only treat short collector addresses as legacy when they are a prefix of the local identity hash (non-Columba peers may use valid shorter representations) - Add DEST_HASH_LENGTH constant for clarity
MatthieuTexier
changed the title
Additional Comments (2)
if not self.telemetry_collector_enabled:
return {'success': False, 'error': 'Host mode not enabled'}This means that if a user selects "Myself" as the collector but has not explicitly enabled host mode separately, every self-telemetry send silently fails with The fix should ensure host mode is activated regardless of the user-visible Prompt To Fix With AIThis is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/service/TelemetryCollectorManager.kt
Line: 649-658
Comment:
**Self-telemetry always fails when host mode is not explicitly enabled**
`syncHostModeIfNeededForLocalStore()` returns `null` (no error) when `_isHostModeEnabled.value` is `false` (line 650), allowing the code to continue and call `storeOwnTelemetry()`. However, the Python-side `store_own_telemetry()` unconditionally checks `if not self.telemetry_collector_enabled` and returns an error:
```python
if not self.telemetry_collector_enabled:
return {'success': False, 'error': 'Host mode not enabled'}
```
This means that if a user selects "Myself" as the collector but has not explicitly enabled host mode separately, every self-telemetry send silently fails with `TelemetrySendResult.Error("Host mode not enabled")`.
The fix should ensure host mode is activated regardless of the user-visible `_isHostModeEnabled` flag, or the Python layer should permit storing own telemetry independently of whether the device is acting as a collector for peers:
```suggestion
private suspend fun syncHostModeIfNeededForLocalStore(): TelemetrySendResult.Error? {
val hostModeSyncResult = reticulumProtocol.setTelemetryCollectorMode(true)
if (hostModeSyncResult.isSuccess) return null
val syncError = hostModeSyncResult.exceptionOrNull()?.message ?: "Unknown host mode sync error"
Log.e(TAG, "❌ Failed to enable host mode before self telemetry store: $syncError")
return TelemetrySendResult.Error("Failed to enable host mode: $syncError")
}
```
How can I resolve this? If you propose a fix, please make it concise.
The sealed class declares Callers that branch on Either add an Prompt To Fix With AIThis is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/util/LocationPermissionManager.kt
Line: 122-140
Comment:
**`checkPermissionStatus()` never returns `PermanentlyDenied` variant**
The sealed class declares `PermanentlyDenied` (with documentation indicating users should be directed to app Settings), but `checkPermissionStatus()` only ever returns `Granted` or `Denied()`. Detecting permanent denial requires calling `Activity.shouldShowRequestPermissionRationale()`, which is unavailable here since only a `Context` is passed.
Callers that branch on `PermanentlyDenied` to show an "Open Settings" UI will never take that code path.
Either add an `activity: Activity` parameter and detect permanent denial correctly, or remove the `PermanentlyDenied` variant and document that permanent denial detection must occur at the call site (e.g., inside an `ActivityResultCallback` after a `RequestPermission` launch).
How can I resolve this? If you propose a fix, please make it concise. |
telemetryBackgroundPermissionLauncher must be declared before telemetryPermissionLauncher which references it in its callback.
telemetry_allowed_requesters is already a set of lowercase strings; remove redundant list comprehension that converted to O(n) scan.
Detecting permanent denial requires Activity.shouldShowRequestPermissionRationale() which is unavailable with only a Context. Document this in Denied's KDoc instead.
This is by design. The "Myself" host picker option means "send my position to the group I'm hosting." If host mode is not enabled, there is no group to send to — self-storing telemetry without a group has no practical use since the position isn't displayed on the map anyway. The current behavior (send fails with "Host mode not enabled") is the correct outcome for this configuration. A future improvement could warn the user in the UI that selecting "Myself" without enabling Group Host is a no-op, but that's out of scope for this PR. No code change needed here. |
2 participants
Summary
Fixes several interconnected bugs in the Group Tracker (telemetry collector) feature that caused unreliable host selection, self-echo markers on the map, and missed background telemetry sends.
Bugs Fixed
1. Host allowlist canonicalization mismatch
Problem:
setAllowedRequesters()stored identity hashes as-is (mixed case), but the Python layer compared them case-sensitively. Requesters whose hashes had uppercase hex chars were silently blocked.Fix: Normalize all hashes to lowercase before persisting and before syncing with Python (
TelemetryCollectorManager.kt,reticulum_wrapper.py).2. Self-echo marker displayed on map
Problem: When the host device sends its own telemetry to itself (collector = self), its position appeared as a separate peer marker on the map instead of being recognized as "self".
Fix:
MapViewModelnow queriesTelemetryCollectorManager.getLocalIdentityHashes()to identify and exclude the device's own markers from peer display.3. Host picker loses "Myself" selection after sleep/wake
Problem: On cold start or after the device wakes from deep sleep,
SettingsViewModelbriefly emitsidentityHash = null/destinationHash = nullbefore Reticulum finishes initializing. The host picker UI compared the persistedcollectorAddressagainst a null hash, lost the "Myself" match, and showed no selection.Fix: Seed identity fields from the Room database immediately on load (before Reticulum APIs are ready), and preserve previous non-null values across transient nulls (
SettingsViewModel.kt).4. Legacy truncated collector addresses
Problem: Earlier versions could persist a truncated destination hash prefix (< 32 chars). These stale values caused silent send failures and confused the host picker.
Fix: On startup, detect truncated addresses and attempt to migrate them to the full 32-char hash by matching against the local identity. If no match, clear the invalid address (
TelemetryCollectorManager.kt,SettingsRepository.kt).5. Background location tracking gaps
Problem: Telemetry sends in the background relied on one-shot location requests, which often timed out when no other app was requesting location. This caused
NoLocationAvailablefailures.Fix: Start continuous location tracking (via
FusedLocationProviderClientor platformLocationManager) whenever telemetry sending is enabled. The tracked location is cached and reused by periodic sends. Extracted intoTelemetryLocationTrackerclass.6. Periodic send/request retry flooding
Problem: When a periodic send or request failed, the scheduler immediately retried on the next loop iteration (every 30s) regardless of the configured interval, causing unnecessary network traffic.
Fix: Track
lastSendAttemptAt/lastRequestAttemptAttimestamps for both successes and failures, and usemax(lastSuccess, lastAttempt)for scheduling.7. Self-store when collector is local device
Problem: When the user selects "Myself" as collector host, telemetry was sent over the network to the device's own destination hash, which failed or was wasteful.
Fix: Detect local destination and call
reticulumProtocol.storeOwnTelemetry()directly, bypassing network send. Re-sync host mode before storing to ensure the Python collector is ready.8. Background location permission prompt
Problem: The app requested
ACCESS_FINE_LOCATIONbut notACCESS_BACKGROUND_LOCATION, so background telemetry sends had no location access on Android 10+.Fix: Added
ACCESS_BACKGROUND_LOCATIONto manifest andLocationPermissionManagerutility to handle the two-step permission flow.Files Changed
TelemetryCollectorManager.ktTelemetryLocationTracker.ktSettingsViewModel.ktapplySettingsUpdateMapViewModel.ktSettingsRepository.ktSettingsScreen.ktLocationSharingCard.ktLocationPermissionManager.ktAndroidManifest.xmlACCESS_BACKGROUND_LOCATIONpermissionreticulum_wrapper.pyColumbaApplication.ktMapViewModelTest.ktTesting
detekt✅,ktlintCheck✅,cpdCheck✅,:app:testNoSentryDebugUnitTest✅,:reticulum:testDebugUnitTest✅