diff --git a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleConnection.kt b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleConnection.kt index a9f32bdf63..a3ac6e8f68 100644 --- a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleConnection.kt +++ b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleConnection.kt @@ -91,9 +91,16 @@ interface BleConnection { fun requestBalancedConnectionPriority(): Boolean = false /** - * Clears the platform's cached GATT service table for the connected peripheral. Necessary when a device reboots - * into a different GATT profile (e.g., ESP32 OTA loader) on the same BLE MAC. Returns `true` if the cache was - * invalidated. Default implementation returns `false` for platforms without a service cache. + * Clears the platform's cached GATT service table for the connected peripheral, so the next discovery re-reads the + * device instead of replaying the cache. Necessary when a device reboots into a different GATT profile (e.g., ESP32 + * OTA loader) on the same BLE MAC, and as recovery when a bonded device that was out of range for a long time + * reconnects against a cache the platform never refreshed. + * + * Requires a live connection: the implementation needs the platform's connection handle, so callers must invoke + * this while connected and then reconnect to pick up the fresh service table. + * + * Returns `true` if the cache was invalidated. Default implementation returns `false` for platforms without a + * service cache. */ fun invalidateServiceCache(): Boolean = false } diff --git a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt index 3f4baf12c9..4784b68609 100644 --- a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt +++ b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt @@ -133,8 +133,14 @@ private val PRIORITY_DOWNGRADE_DELAY = 30.seconds /** * Settle delay after disconnecting to let the BLE stack release GATT resources before reconnecting post cache * invalidation. + * + * Reuses [BleReconnectPolicy.DEFAULT_SETTLE_DELAY] (3 s) rather than a shorter bespoke value: this is the same + * disconnect → reconnect cycle the reconnect loop performs, and the same firmware-side GATT release has to complete + * first. The probe data recorded on that constant is explicit that 1.5 s fails 3–4 times out of 5 and that ≥ 5 s is + * needed for reliable reconnection, so a 500 ms pause here reliably turned a cache refresh into a failed attempt plus a + * fresh round of backoff — the opposite of the recovery it exists to provide. */ -private val POST_INVALIDATION_RECONNECT_DELAY = 500.milliseconds +private val POST_INVALIDATION_RECONNECT_DELAY = BleReconnectPolicy.DEFAULT_SETTLE_DELAY /** * A [RadioTransport] implementation for BLE devices using the common BLE abstractions (which are powered by Kable). @@ -234,6 +240,10 @@ class BleRadioTransport( // toggles the connection off; until then, retry forever with the policy's exponential-backoff cap (60 s). private val reconnectPolicy = BleReconnectPolicy(maxFailures = Int.MAX_VALUE) + // Because the loop above never gives up, a stale platform GATT cache would otherwise retry forever without + // recovering (issue #6685). This gate decides when a long failure streak has earned one cache refresh. + private val gattCacheInvalidationGate = GattCacheInvalidationGate() + private val heartbeatSender = HeartbeatSender( sendToRadio = { handleSendToRadio(it) }, @@ -380,27 +390,28 @@ class BleRadioTransport( throw RadioNotConnectedException("Failed to connect to device at address $address") } - // Post-OTA GATT cache invalidation: the device rebooted with potentially different - // BLE service table handles. Refresh Android's cache and reconnect to force fresh - // service discovery before proceeding with profile setup. - val radioServiceForCache = callback as? RadioInterfaceService - if (radioServiceForCache?.consumeGattCacheInvalidationRequest() == true) { - val invalidated = bleConnection.invalidateServiceCache() - Logger.d { "[${address.anonymize()}] Post-OTA GATT cache invalidation requested: $invalidated" } - if (invalidated) { - Logger.i { - "[${address.anonymize()}] Reconnecting after GATT cache refresh to force service rediscovery" - } - bleConnection.disconnect() - delay(POST_INVALIDATION_RECONNECT_DELAY) - val reconnectState = bleConnection.connectAndAwait(device, CONNECTION_TIMEOUT) - if (reconnectState !is BleConnectionState.Connected) { - throw RadioNotConnectedException( - "Failed to reconnect after post-OTA GATT cache refresh (state=$reconnectState)", - ) - } - Logger.i { "[${address.anonymize()}] Reconnected after GATT cache refresh" } + // GATT cache invalidation has two triggers, both repaired the same way — refresh the platform's cached + // service table, then reconnect so discovery re-reads the device: + // 1. Post-OTA: the device rebooted with potentially different BLE service table handles. + // 2. Stale cache after a long absence: a bonded radio that went out of range or powered off can come back + // with Android still serving the old cached service table, failing every attempt until the user unpairs + // at the OS level (issue #6685). + // consumeGattCacheInvalidationRequest() is read into a val first: `||` short-circuiting must never skip + // consuming the one-shot post-OTA flag. + val postOtaRequested = (callback as? RadioInterfaceService)?.consumeGattCacheInvalidationRequest() == true + val consecutiveFailures = reconnectPolicy.consecutiveFailures + val staleCacheSuspected = gattCacheInvalidationGate.shouldInvalidateOnAttempt(consecutiveFailures) + if (postOtaRequested || staleCacheSuspected) { + val triggers = buildList { + if (postOtaRequested) add("post-OTA reboot") + if (staleCacheSuspected) add("$consecutiveFailures consecutive reconnect failures") } + val reason = triggers.joinToString(" + ") + // Only the stale-cache trigger spends the streak's one refresh. A post-OTA refresh is scheduled by the + // firmware-update flow, not by any failure streak, so charging it to the streak would disable stale-cache + // recovery for a streak that had not even started — and the gate can only be re-armed by a stable or + // intentional disconnect, which by definition cannot happen while a streak is running. + refreshGattCacheAndReconnect(device, reason, consumeStreakAllowance = staleCacheSuspected) } val gattConnectedAt = nowMillis @@ -484,6 +495,12 @@ class BleRadioTransport( val connectionUptime = (nowMillis - gattConnectedAt).milliseconds val wasStable = connectionUptime >= reconnectPolicy.minStableConnection + // Mirror the outcomes that make BleReconnectPolicy reset its own failure counter: both end the streak, so the + // cache refresh must be re-armed here. It cannot be inferred from consecutiveFailures on the next attempt — + // that reset lands after this function returns, and a radio that is out of range again fails its next attempt + // without ever reaching the point where the counter is read. + if (wasStable || wasIntentional) gattCacheInvalidationGate.onFailureStreakEnded() + if (!wasStable && !wasIntentional) { Logger.w { "[$address] Connection lasted only $connectionUptime " + @@ -494,6 +511,44 @@ class BleRadioTransport( return BleReconnectPolicy.Outcome.Disconnected(wasStable = wasStable, wasIntentional = wasIntentional) } + /** + * Refreshes the platform's cached GATT service table for the connected peripheral and reconnects so that service + * discovery re-reads the device instead of replaying the cache. + * + * A no-op when the platform cannot invalidate (non-Android targets, or an Android reflection miss): nothing is + * disconnected and the current link is used as-is. + * + * @param reason short diagnostic describing which trigger asked for the refresh + * @param consumeStreakAllowance true only when the refresh was triggered by a suspected stale cache, so that a + * post-OTA refresh never spends the failure streak's single allowance + * @throws RadioNotConnectedException when the post-refresh reconnect does not reach Connected, so the caller + * returns a retryable failure to [BleReconnectPolicy] + */ + private suspend fun refreshGattCacheAndReconnect( + device: BleDevice, + reason: String, + consumeStreakAllowance: Boolean, + ) { + val invalidated = bleConnection.invalidateServiceCache() + Logger.d { "[${address.anonymize()}] GATT cache invalidation ($reason): $invalidated" } + if (!invalidated) return + + // Consume the streak's single allowance only for a stale-cache refresh, and only now that the platform + // reported a real refresh. + if (consumeStreakAllowance) gattCacheInvalidationGate.onCacheInvalidated() + + Logger.i { "[${address.anonymize()}] Reconnecting after GATT cache refresh to force service rediscovery" } + bleConnection.disconnect() + delay(POST_INVALIDATION_RECONNECT_DELAY) + val reconnectState = bleConnection.connectAndAwait(device, CONNECTION_TIMEOUT) + if (reconnectState !is BleConnectionState.Connected) { + throw RadioNotConnectedException( + "Failed to reconnect after GATT cache refresh ($reason, state=$reconnectState)", + ) + } + Logger.i { "[${address.anonymize()}] Reconnected after GATT cache refresh ($reason)" } + } + private suspend fun bondDeviceBeforeConnect(device: BleDevice) { if (bluetoothRepository.isBonded(address)) return diff --git a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/GattCacheInvalidationGate.kt b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/GattCacheInvalidationGate.kt new file mode 100644 index 0000000000..7319281978 --- /dev/null +++ b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/GattCacheInvalidationGate.kt @@ -0,0 +1,102 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.network.radio + +/** + * Decides when the ordinary BLE reconnect loop should refresh the platform's cached GATT service table. + * + * Background: after a bonded radio goes out of range or power-cycles for a long time, Android can keep serving a stale + * cached service table for it. Every reconnect then discovers the wrong (or no) Meshtastic characteristics and fails, + * so [BleReconnectPolicy] retries forever and the UI never leaves "Not connected" — historically only an OS-level + * unpair/re-pair cleared it. + * + * Refreshing is not free: it costs an extra disconnect → settle → reconnect round trip and throws away a valid cache, + * so it must not fire on the ordinary out-of-range blip. A radio switched off for a minute, or a phone carried out of + * range and back, is not evidence of a stale cache — tearing down a link that was about to succeed would turn an + * ordinary reconnect into a failure plus a fresh round of backoff, which is worse than doing nothing at all. + * + * The gate therefore arms only once the failure streak has run for *minutes* (see [DEFAULT_FAILURE_THRESHOLD]) — the + * prolonged absence issue #6685 actually describes — and fires **at most once per streak**: if a genuine refresh did + * not fix the connection, repeating it every attempt only adds latency. + * + * Not thread-safe by design: it is owned by [BleRadioTransport]'s single reconnect-loop coroutine. + * + * @param failureThreshold consecutive reconnect failures required before a refresh is considered + */ +internal class GattCacheInvalidationGate(private val failureThreshold: Int = DEFAULT_FAILURE_THRESHOLD) { + + init { + require(failureThreshold > 0) { "failureThreshold must be positive, was $failureThreshold" } + } + + /** True once a refresh actually took effect during the current failure streak. */ + private var invalidatedForCurrentStreak: Boolean = false + + /** + * Returns true when the connection attempt now in progress should refresh the GATT cache. + * + * A [consecutiveFailures] of zero can never reach [failureThreshold] (which is always positive), so a healthy + * connection never refreshes. + * + * @param consecutiveFailures failures observed *before* the attempt in progress, i.e. + * [BleReconnectPolicy.consecutiveFailures] read from inside the attempt + */ + fun shouldInvalidateOnAttempt(consecutiveFailures: Int): Boolean = + !invalidatedForCurrentStreak && consecutiveFailures >= failureThreshold + + /** + * Records that a refresh actually took effect, consuming this streak's single allowance. + * + * Only call this when the platform reported success. A refresh that could not be performed at all (on Android the + * reflection hop into the `BluetoothGatt` can miss after a Kable upgrade) is a no-op that costs nothing, so it must + * not burn the allowance — otherwise one silent miss would disable the recovery for the rest of the streak. + */ + fun onCacheInvalidated() { + invalidatedForCurrentStreak = true + } + + /** + * Re-arms the gate because the failure streak ended, so a later streak earns its own refresh. + * + * Must be driven by the same outcomes that reset [BleReconnectPolicy.consecutiveFailures]. The counter cannot be + * used to infer this: the caller only reads it on an attempt that reached a connected link, and the streak-ending + * reset lands *after* that attempt returns. A radio that is out of range again fails its next attempt long before + * any zero could be observed, which would leave the gate consumed forever and silently reintroduce issue #6685 the + * second time a device disappears. + */ + fun onFailureStreakEnded() { + invalidatedForCurrentStreak = false + } + + companion object { + /** + * Consecutive failures before a stale cache is suspected. + * + * Deliberately far above [BleReconnectPolicy.DEFAULT_FAILURE_THRESHOLD] (3), which only marks a disconnect as + * "more than a blip" for the UI. Three failures are reached about 47 s into an ordinary out-of-range or + * powered-off gap, so refreshing there would sabotage a normal reconnect rather than repair a stale cache. + * + * Six is the first count at which [computeReconnectBackoff] has been saturated at its 60 s cap for two + * consecutive cycles: the retry ladder is exhausted and every further attempt is identical. With a + * [BleReconnectPolicy.DEFAULT_SETTLE_DELAY] before each attempt, the refresh lands on the seventh attempt, 3 + * min 36 s into an unbroken streak (seven settle delays plus 5+10+20+40+60+60 s of backoff). That is minutes of + * continuous failure, which is what issue #6685's "out of range / power off" report describes, rather than the + * sub-minute blip the lower threshold catches. + */ + const val DEFAULT_FAILURE_THRESHOLD = 6 + } +} diff --git a/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt index b690ab9762..84b6e00076 100644 --- a/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt +++ b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt @@ -25,6 +25,7 @@ import dev.mokkery.verify import dev.mokkery.verify.VerifyMode import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.async import kotlinx.coroutines.test.TestScope @@ -32,6 +33,7 @@ import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import kotlinx.coroutines.withTimeout +import org.meshtastic.core.ble.DisconnectReason import org.meshtastic.core.ble.MeshtasticBleConstants.FROMNUM_CHARACTERISTIC import org.meshtastic.core.ble.MeshtasticBleConstants.FROMRADIO_CHARACTERISTIC import org.meshtastic.core.ble.MeshtasticBleConstants.SERVICE_UUID @@ -514,9 +516,21 @@ class BleRadioTransportTest { ) bleTransport.start() try { - // 3s settle + connectAndAwait + 500ms reconnect delay + reconnect connectAndAwait, then profile setup - // bounded by CONNECTED_GATE_TIMEOUT / SUBSCRIPTION_READY_TIMEOUT (5s each). - advanceTimeBy(20_000) + // 3s settle + connectAndAwait, then POST_INVALIDATION_RECONNECT_DELAY must fully elapse before the + // forced reconnect fires. Checking at 4.5s — safely past where a regressed 500 ms delay would have + // already fired (3s settle + 500ms = 3.5s) but safely short of the real 3s delay firing (3s settle + + // 3s = 6s) — pins the delay's actual value: a regression back to the old 500 ms would already show + // the second connectAndAwait call by this point, which connectAndAwaitCalls == 1 below would catch. + advanceTimeBy(4_500) + assertEquals( + 1, + connection.connectAndAwaitCalls, + "the forced reconnect must wait for the full post-invalidation settle delay, not fire early", + ) + + // Clears the remaining settle delay, the reconnect itself, and profile setup (CONNECTED_GATE_TIMEOUT / + // SUBSCRIPTION_READY_TIMEOUT, 5s each) — 15.5s more, matching the original 20s total budget. + advanceTimeBy(15_500) // The transport consumed the flag during its first connect cycle: a second consume must return false. assertFalse( @@ -535,4 +549,295 @@ class BleRadioTransportTest { bleTransport.close() } } + + private fun bleTransportOn(scope: CoroutineScope, callback: RadioInterfaceService): BleRadioTransport = + BleRadioTransport( + scope = scope, + scanner = scanner, + bluetoothRepository = bluetoothRepository, + connectionFactory = connectionFactory, + callback = callback, + address = address, + ) + + /** + * Simulates the platform replaying a stale GATT service table: `profile()` cannot find the Meshtastic service, and + * the service only reappears once the platform has reported a *successful* cache refresh. + * + * Keyed on [FakeBleConnection.invalidateServiceCacheResult] as well as the call count so that a platform which + * cannot refresh at all (Android reflection miss → `false`) never recovers, which is what makes the recovery test + * below non-vacuous. + */ + private fun FakeBleConnection.simulateStaleServiceTable() { + missingServices.add(SERVICE_UUID) + onDisconnect = { + if (invalidateServiceCacheResult && invalidateServiceCacheCalls > 0) missingServices.remove(SERVICE_UUID) + } + } + + /** + * Issue #6685: a bonded radio that was out of range or powered off for a long time can come back with Android still + * serving a stale cached GATT service table, so every reconnect fails and [BleReconnectPolicy] (maxFailures = + * [Int.MAX_VALUE]) retries forever. Once the failure streak reaches + * [GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD], the next attempt that reaches a connected link must + * refresh the cache and reconnect so discovery re-reads the device. + * + * Timeline in virtual time (3 s settle before every attempt, backoff 5/10/20/40/60/60 s after failures 1–6): + * attempts fail at t = 3, 11, 24, 47, 90 and 153 s; the seventh attempt starts at t = 216 s (3 min 36 s) and + * connects. That the refresh is *not* reachable any earlier is the whole point — see the sub-threshold test below. + */ + @Test + fun `stale GATT cache is refreshed after the reconnect failure streak reaches the threshold`() = runTest { + val device = FakeBleDevice(address = address, name = "Test Device") + bluetoothRepository.bond(device) + scanner.emitDevice(device) + connection.service.addCharacteristic(FROMNUM_CHARACTERISTIC) + connection.service.addCharacteristic(FROMRADIO_CHARACTERISTIC) + connection.invalidateServiceCacheResult = true + connection.failNextN = GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD + + val bleTransport = bleTransportOn(this, FakeRadioInterfaceService()) + bleTransport.start() + try { + advanceTimeBy(240_000) + + assertEquals( + 1, + connection.invalidateServiceCacheCalls, + "the cache must be refreshed exactly once for the streak, not on every retry", + ) + assertTrue( + connection.connectAndAwaitCalls >= GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD + 2, + "expected the failed attempts, the connected attempt, and the post-refresh reconnect " + + "(got ${connection.connectAndAwaitCalls})", + ) + } finally { + bleTransport.close() + } + } + + /** + * Discriminator for the test above, and the regression this guards: an ordinary out-of-range gap must cost no cache + * refresh and no extra disconnect/reconnect round trip. With the threshold at the policy's transient-disconnect + * value (3 failures ≈ 47 s) this test fails, because a completely ordinary absence would be treated as a stale + * cache and the reconnect that was about to succeed would be torn down instead. + * + * Timeline: attempts fail at t = 3, 11, 24, 47 and 90 s; the sixth attempt starts at t = 153 s and connects with + * one failure fewer than the threshold recorded. + */ + @Test + fun `a reconnect failure streak below the threshold never refreshes the GATT cache`() = runTest { + val device = FakeBleDevice(address = address, name = "Test Device") + bluetoothRepository.bond(device) + scanner.emitDevice(device) + connection.service.addCharacteristic(FROMNUM_CHARACTERISTIC) + connection.service.addCharacteristic(FROMRADIO_CHARACTERISTIC) + connection.invalidateServiceCacheResult = true + connection.failNextN = GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD - 1 + + val bleTransport = bleTransportOn(this, FakeRadioInterfaceService()) + bleTransport.start() + try { + advanceTimeBy(180_000) + + assertTrue( + connection.connectAndAwaitCalls >= GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD, + "the connection must have recovered after the sub-threshold failures " + + "(got ${connection.connectAndAwaitCalls} attempts)", + ) + assertEquals( + 0, + connection.invalidateServiceCacheCalls, + "an ordinary out-of-range gap must not throw away a valid GATT cache", + ) + } finally { + bleTransport.close() + } + } + + /** + * The one-refresh-per-streak allowance must be re-armed when a streak ends, or the second time a radio disappears + * the recovery is silently unavailable and issue #6685 returns. + * + * This is the case that cannot be inferred from `BleReconnectPolicy.consecutiveFailures`: the counter is reset only + * *after* the attempt that owned the stable session returns, and the first attempt of the next streak fails before + * it ever reaches the point where the transport reads it. Only an explicit end-of-streak signal from the disconnect + * path re-arms the gate. + * + * The streak is ended here with [DisconnectReason.LocalDisconnect] rather than by letting the session age past + * `minStableConnection`: connection uptime is measured with `nowMillis` (the wall clock), which `advanceTimeBy` + * does not move, so the `wasStable` branch is unreachable from a virtual-time test. Both branches feed the same + * re-arm call, so the wiring under test is the same. + */ + @Test + fun `a second failure streak after the previous one ended earns its own cache refresh`() = runTest { + val device = FakeBleDevice(address = address, name = "Test Device") + bluetoothRepository.bond(device) + scanner.emitDevice(device) + connection.service.addCharacteristic(FROMNUM_CHARACTERISTIC) + connection.service.addCharacteristic(FROMRADIO_CHARACTERISTIC) + connection.invalidateServiceCacheResult = true + connection.failNextN = GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD + + val bleTransport = bleTransportOn(this, FakeRadioInterfaceService()) + bleTransport.start() + try { + // First streak: threshold failures, then the next attempt connects and refreshes the cache at ~t = 216 s. + advanceTimeBy(240_000) + assertEquals(1, connection.invalidateServiceCacheCalls, "the first streak must refresh the cache once") + + // End the streak with a disconnect the policy treats as non-failing, then arm a second streak behind it. + connection.failNextN = GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD + connection.simulateRemoteDisconnect(DisconnectReason.LocalDisconnect) + + advanceTimeBy(240_000) + assertEquals( + 2, + connection.invalidateServiceCacheCalls, + "a streak that follows an ended streak must earn its own cache refresh", + ) + } finally { + bleTransport.close() + } + } + + /** + * A post-OTA refresh must not spend the failure streak's single allowance. + * + * The two triggers are independent: the post-OTA refresh is scheduled by the firmware-update flow, not by any + * failure streak. If it charged the streak, a radio that came back from an OTA into an unstable streak would have + * its stale-cache recovery disabled for that whole streak — the gate is only re-armed by a stable or intentional + * disconnect, which by definition cannot happen while a streak is running. + * + * Here the post-OTA refresh fires on the very first attempt (zero consecutive failures), then an unstable + * disconnect starts a streak that runs up to the threshold. The second refresh proves the allowance survived. + */ + @Test + fun `a post-OTA cache refresh does not consume the stale-cache allowance`() = runTest { + val device = FakeBleDevice(address = address, name = "Test Device") + bluetoothRepository.bond(device) + scanner.emitDevice(device) + connection.service.addCharacteristic(FROMNUM_CHARACTERISTIC) + connection.service.addCharacteristic(FROMRADIO_CHARACTERISTIC) + connection.invalidateServiceCacheResult = true + + val radioService = FakeRadioInterfaceService() + radioService.requestGattCacheInvalidationOnNextConnect() + + val bleTransport = bleTransportOn(this, radioService) + bleTransport.start() + try { + advanceTimeBy(20_000) + assertEquals(1, connection.invalidateServiceCacheCalls, "the post-OTA flag must refresh the cache once") + + // An unstable (non-intentional, sub-minStableConnection) disconnect starts a streak, so the gate is never + // re-armed between the post-OTA refresh and the stale-cache refresh under test. + connection.failNextN = GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD - 1 + connection.simulateRemoteDisconnect(DisconnectReason.Timeout) + + advanceTimeBy(300_000) + assertEquals( + 2, + connection.invalidateServiceCacheCalls, + "the streak that followed the OTA must still earn its own stale-cache refresh", + ) + } finally { + bleTransport.close() + } + } + + /** + * The recovery this whole mechanism exists for: connects succeed, but service discovery keeps replaying a cached + * table without the Meshtastic service, so every attempt fails at profile setup. Once the streak reaches the + * threshold the cache is refreshed, and *because of that refresh* the following attempt finds the service and the + * reconnect loop settles. + * + * Unlike a connect-time failure this exercises the actual issue #6685 shape — the device is reachable, the cached + * service table is not. The fake only makes the service reappear after a successful `invalidateServiceCache()`, so + * a passing run cannot be explained by anything other than the invalidation; the companion test below pins that + * down with the platform refusing to refresh. + * + * Timings are not asserted: attempts here fail after connecting, so each one also pays session-cleanup waits. The + * assertions are on production-observable behavior only — one refresh call, and a loop that stopped retrying + * afterward — not on the fake's own `missingServices` bookkeeping, since that store is mutated by the fake's own + * [FakeBleConnection.simulateStaleServiceTable] callback and would just be testing the fake against itself. The + * negative-control test below is what actually proves recovery is caused by the invalidation. + */ + @Test + fun `a stale service table recovers on the attempt that follows the cache refresh`() = runTest { + val device = FakeBleDevice(address = address, name = "Test Device") + bluetoothRepository.bond(device) + scanner.emitDevice(device) + connection.service.addCharacteristic(FROMNUM_CHARACTERISTIC) + connection.service.addCharacteristic(FROMRADIO_CHARACTERISTIC) + connection.invalidateServiceCacheResult = true + connection.simulateStaleServiceTable() + + val bleTransport = bleTransportOn(this, FakeRadioInterfaceService()) + bleTransport.start() + try { + advanceTimeBy(1_200_000) + + assertEquals( + 1, + connection.invalidateServiceCacheCalls, + "the streak must earn exactly one refresh, and that refresh must have been enough", + ) + + val callsAfterRecovery = connection.connectAndAwaitCalls + advanceTimeBy(300_000) + assertEquals( + callsAfterRecovery, + connection.connectAndAwaitCalls, + "the loop must have settled on a working connection instead of continuing to retry", + ) + } finally { + bleTransport.close() + } + } + + /** + * Negative control for the recovery test: with the platform unable to refresh (`invalidateServiceCache()` returning + * `false`, e.g. the Android reflection hop missing), the stale service table is never repaired and the loop keeps + * retrying. This is what proves the recovery above is caused by the invalidation rather than by the retry loop + * eventually getting lucky. + * + * A refresh that never happened must also not burn the streak's allowance, so the gate keeps asking on every + * subsequent attempt — hence the "more than once" assertion here versus "exactly once" above. + */ + @Test + fun `a stale service table never recovers when the platform cannot refresh the cache`() = runTest { + val device = FakeBleDevice(address = address, name = "Test Device") + bluetoothRepository.bond(device) + scanner.emitDevice(device) + connection.service.addCharacteristic(FROMNUM_CHARACTERISTIC) + connection.service.addCharacteristic(FROMRADIO_CHARACTERISTIC) + connection.invalidateServiceCacheResult = false + connection.simulateStaleServiceTable() + + val bleTransport = bleTransportOn(this, FakeRadioInterfaceService()) + bleTransport.start() + try { + advanceTimeBy(1_200_000) + + assertTrue( + connection.invalidateServiceCacheCalls > 1, + "a refresh that never happened must not burn the allowance, so the gate must keep asking " + + "(got ${connection.invalidateServiceCacheCalls} calls)", + ) + assertTrue( + SERVICE_UUID in connection.missingServices, + "without a successful refresh the stale service table must persist", + ) + + val callsSoFar = connection.connectAndAwaitCalls + advanceTimeBy(300_000) + assertTrue( + connection.connectAndAwaitCalls > callsSoFar, + "the reconnect loop must still be retrying (stuck at $callsSoFar attempts)", + ) + } finally { + bleTransport.close() + } + } } diff --git a/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/GattCacheInvalidationGateTest.kt b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/GattCacheInvalidationGateTest.kt new file mode 100644 index 0000000000..cb92b8dc2a --- /dev/null +++ b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/GattCacheInvalidationGateTest.kt @@ -0,0 +1,133 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.network.radio + +import kotlin.test.Test +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue +import kotlin.time.Duration +import kotlin.time.Duration.Companion.minutes + +class GattCacheInvalidationGateTest { + + @Test + fun `no invalidation while the failure streak is below the threshold`() { + val gate = GattCacheInvalidationGate(failureThreshold = 3) + + assertFalse(gate.shouldInvalidateOnAttempt(0), "a healthy connection must never refresh the cache") + assertFalse(gate.shouldInvalidateOnAttempt(-1), "a nonsensical count must never refresh the cache") + assertFalse(gate.shouldInvalidateOnAttempt(1), "one failure is an ordinary out-of-range blip") + assertFalse(gate.shouldInvalidateOnAttempt(2), "two failures are still below the threshold") + } + + @Test + fun `invalidation is requested once the threshold is reached`() { + val gate = GattCacheInvalidationGate(failureThreshold = 3) + + assertTrue(gate.shouldInvalidateOnAttempt(3), "the threshold-th consecutive failure must arm the refresh") + } + + @Test + fun `the refresh fires at most once per failure streak`() { + val gate = GattCacheInvalidationGate(failureThreshold = 3) + + assertTrue(gate.shouldInvalidateOnAttempt(3)) + gate.onCacheInvalidated() + + assertFalse(gate.shouldInvalidateOnAttempt(4), "a refresh that did not help must not repeat every attempt") + assertFalse(gate.shouldInvalidateOnAttempt(9), "the allowance stays consumed for the rest of the streak") + } + + /** + * Discriminator for the "consume the allowance only on a real refresh" rule: on Android the reflection hop into + * `BluetoothGatt` can miss, in which case nothing was refreshed and nothing may be consumed. Without the guard, one + * silent miss would disable the recovery for the whole streak. + */ + @Test + fun `an attempted refresh that never happened leaves the allowance intact`() { + val gate = GattCacheInvalidationGate(failureThreshold = 3) + + assertTrue(gate.shouldInvalidateOnAttempt(3)) + // No onCacheInvalidated(): the platform reported it could not refresh. + + assertTrue(gate.shouldInvalidateOnAttempt(4), "a no-op refresh must not burn the streak allowance") + } + + @Test + fun `the end of a failure streak re-arms the gate for the next one`() { + val gate = GattCacheInvalidationGate(failureThreshold = 3) + + assertTrue(gate.shouldInvalidateOnAttempt(3)) + gate.onCacheInvalidated() + assertFalse(gate.shouldInvalidateOnAttempt(5)) + + gate.onFailureStreakEnded() + + assertFalse(gate.shouldInvalidateOnAttempt(2), "the new streak restarts below the threshold") + assertTrue(gate.shouldInvalidateOnAttempt(3), "a fresh streak earns a fresh refresh") + } + + @Test + fun `a threshold of one refreshes on the first failure`() { + val gate = GattCacheInvalidationGate(failureThreshold = 1) + + assertTrue(gate.shouldInvalidateOnAttempt(1)) + } + + @Test + fun `a non-positive threshold is rejected`() { + assertFailsWith { GattCacheInvalidationGate(failureThreshold = 0) } + assertFailsWith { GattCacheInvalidationGate(failureThreshold = -1) } + } + + /** + * The refresh must sit far above the policy's transient-disconnect threshold. That threshold only decides when to + * tell higher layers a disconnect is more than a blip — it is reached roughly 47 s into an ordinary out-of-range or + * powered-off gap, which is no evidence at all of a stale service table. Refreshing there tears down a link that + * was about to succeed and costs the user a failed attempt plus a fresh round of backoff. + */ + @Test + fun `the default threshold is far above the reconnect policy transient-disconnect threshold`() { + assertTrue( + GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD > BleReconnectPolicy.DEFAULT_FAILURE_THRESHOLD, + "a refresh at the transient-disconnect threshold would sabotage ordinary reconnects instead of repairing " + + "a stale cache (cache=${GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD}, " + + "transient=${BleReconnectPolicy.DEFAULT_FAILURE_THRESHOLD})", + ) + } + + /** + * Pins the intent behind the default rather than the number: the gate may only fire after *minutes* of unbroken + * failure, because the stale cache it repairs follows a prolonged absence (issue #6685 — "go out of range / power + * off"). Derived from the production settle delay and backoff ladder, so retuning either one cannot silently drag + * the refresh back into blip territory. + */ + @Test + fun `the default threshold is only reachable after minutes of unbroken failure`() { + val threshold = GattCacheInvalidationGate.DEFAULT_FAILURE_THRESHOLD + val backoff = + (1..threshold).fold(Duration.ZERO) { total, failures -> total + computeReconnectBackoff(failures) } + // One settle delay precedes every attempt: the `threshold` attempts that fail, plus the one that refreshes. + val elapsedBeforeRefresh = BleReconnectPolicy.DEFAULT_SETTLE_DELAY * (threshold + 1) + backoff + + assertTrue( + elapsedBeforeRefresh >= 3.minutes, + "the refresh must not be reachable inside an ordinary out-of-range gap (reached at $elapsedBeforeRefresh)", + ) + } +}