From 4fb2adabc36fcc10f8879fe99114951b0828f04e Mon Sep 17 00:00:00 2001 From: kivtxs <131401183+kivtxs@users.noreply.github.com> Date: Tue, 6 Oct 2026 13:27:52 -0400 Subject: [PATCH] fix(bindings): a closed hello-mapped link gives the peer back its live address, and each link keeps its own hello Found reviewing #516 on main, not on a device (both phones are in use). - Routing. A hello makes the writer's central-role address the peer's send address (setDeviceIdentifier). When that link closed cleanly, the clean branch of handleCentralDisconnectedOnBleThread kept the mapping, so with no client gatt and no subscription at that address the drain queued every fragment and dialled an address that does not advertise, while our own client link to the peer sat idle. It healed only when the peer dialled in again with a new hello, or the dial timed out (~30 s). Now, on a clean disconnect where we hold no client link at the address and the peer is up on another link, the address is dropped; removeIdentifiersForAddress already re-points the peer at its surviving address. - Hello watchdog. helloInFlight was keyed by address only and never cleared on teardown. A link that closed mid-hello and a new one at the same address within 3 s shared the entry: the old watchdog, or a late callback from the old link, marked the new link ready mid-handshake, letting Message writes run beside it. Entries now carry the BluetoothGatt they belong to, are removed in handleDisconnected, closeGattClient, forgetLink and clearAll, and the map is concurrent because a disconnect arrives on a binder thread. Tests: CentralGattClientHelloTest gains the reconnect case (fails on main, passes here); StaleAddressRegistryTest pins the re-pointing; a source guard pins the facade rule. Android JVM suite 630/630. --- .../offlineprotocol/ble/BleTransportFacade.kt | 16 ++++++++ .../offlineprotocol/ble/CentralGattClient.kt | 37 +++++++++++++++---- .../ble/CentralGattClientHelloTest.kt | 31 ++++++++++++++++ .../ble/StaleAddressRegistryTest.kt | 24 ++++++++++++ crates/offline-protocol-uniffi/src/lib.rs | 33 +++++++++++++++++ 5 files changed, 134 insertions(+), 7 deletions(-) diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/BleTransportFacade.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/BleTransportFacade.kt index 9b0aca36..a58cdc3a 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/BleTransportFacade.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/BleTransportFacade.kt @@ -4546,6 +4546,22 @@ class BleTransportFacade( flushPeerMtu(address, peerId) } } + if (isCleanDisconnect && connections.getGatt(address) == null) { + connections.deviceIdForAddress(address)?.let { peerId -> + if (connections.hasOtherLiveLink(peerId, address)) { + // A hello mapped this peer at the central-role address its + // link to us came from, and that became the address we send + // to. With that link gone and no client link of ours there, + // every fragment would queue and the drain would dial an + // address that does not advertise, while our own link to the + // peer sat idle. Drop the address: the registry points the + // peer back at its live link, and a new link from the peer + // brings a new hello. + connections.removeIdentifiersForAddress(address) + centralClient.clearResolutionAttempt(address) + } + } + } if (!isCleanDisconnect) { lastSeenRssi.remove(address) connections.deviceIdForAddress(address)?.let { peerId -> diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/CentralGattClient.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/CentralGattClient.kt index 6f49796b..e4f68c30 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/CentralGattClient.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/CentralGattClient.kt @@ -261,8 +261,13 @@ internal class CentralGattClient( private val negotiatedPayload = ConcurrentHashMap() // Hello writes awaiting their callback, keyed by address, holding the - // watchdog that goes ready without it. BLE-thread only. - private val helloInFlight = HashMap() + // link the write went out on and the watchdog that goes ready without the + // callback. Keyed to the link as well as the address: a link that closes + // mid-hello and a new one at the same address within the watchdog window + // must not share an entry, or the old watchdog marks the new link ready + // mid-handshake and a late callback from the old link does the same. + // Concurrent because a disconnect arrives on a binder thread. + private val helloInFlight = ConcurrentHashMap>() private val deviceIdResolutionAttempts = ConcurrentHashMap() @@ -346,6 +351,7 @@ internal class CentralGattClient( deviceIdResolutionAttempts.remove(address) advertisedDeviceIds.remove(address) cancelMtuWatchdog(address) + cancelHelloWatchdog(address, null) clearServiceInstanceSelection(address) } @@ -361,9 +367,21 @@ internal class CentralGattClient( serviceInstanceProbes.clear() appTagReadWatchdogs.values.forEach { bleHandler.removeCallbacks(it) } appTagReadWatchdogs.clear() + helloInFlight.values.forEach { bleHandler.removeCallbacks(it.second) } + helloInFlight.clear() boundServiceInstanceIds.clear() } + /** + * Drops the hello pending at [address], only if it belongs to [gatt] when + * one is given, and cancels its watchdog. + */ + private fun cancelHelloWatchdog(address: String, gatt: BluetoothGatt?) { + val entry = helloInFlight[address] ?: return + if (gatt != null && entry.first !== gatt) return + if (helloInFlight.remove(address, entry)) bleHandler.removeCallbacks(entry.second) + } + /** * Cancels any pending MTU watchdog for [address] and drops the * associated `mtuInFlight` slot. Called from every handshake @@ -866,8 +884,11 @@ internal class CentralGattClient( val address = gatt.device.address bleHandler.post { if (host.isShuttingDown()) return@post - val watchdog = helloInFlight.remove(address) ?: return@post - bleHandler.removeCallbacks(watchdog) + // Only for the hello this link wrote: a link that closed + // and reopened at the same address holds its own entry. + val entry = helloInFlight[address] ?: return@post + if (entry.first !== gatt || !helloInFlight.remove(address, entry)) return@post + bleHandler.removeCallbacks(entry.second) if (status != BluetoothGatt.GATT_SUCCESS) { diagnosticEmitter("info", "BLE hello refused", mapOf("address" to address, "status" to status)) } @@ -976,6 +997,7 @@ internal class CentralGattClient( val address = gatt.device.address val wasConnected = host.connections.getGatt(address) != null host.connections.removeGatt(address) + cancelHelloWatchdog(address, gatt) linkReady.remove(address) // Don't remove from discovered list - keep trying to reconnect. @@ -1438,12 +1460,12 @@ internal class CentralGattClient( override fun run() { // Only for this write: a later link at the same address // holds its own entry. - if (helloInFlight[address] !== this) return - helloInFlight.remove(address) + val entry = helloInFlight[address] ?: return + if (entry.second !== this || !helloInFlight.remove(address, entry)) return if (!host.isShuttingDown()) markLinkReady(address) } } - helloInFlight[address] = watchdog + helloInFlight[address] = gatt to watchdog bleHandler.postDelayed(watchdog, HELLO_WATCHDOG_MS) } @@ -1467,6 +1489,7 @@ internal class CentralGattClient( Log.w(TAG, "Error closing gatt for $address ($reason)", e) } host.connections.removeGatt(address) + cancelHelloWatchdog(address, gatt) linkReady.remove(address) // Drop the unproven claim with the link, so a reconnect cannot pair // this connection's device id with the next one's identity. diff --git a/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/CentralGattClientHelloTest.kt b/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/CentralGattClientHelloTest.kt index 3a1a9d32..9b69b80f 100644 --- a/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/CentralGattClientHelloTest.kt +++ b/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/CentralGattClientHelloTest.kt @@ -163,6 +163,37 @@ class CentralGattClientHelloTest { assertEquals(1, host.drains) } + @Test + fun `a link reopened at the same address keeps its own hello`() { + // Link one sends its hello and closes before the callback. Link two + // opens at the same address within the watchdog window and sends its + // own. Neither link one's watchdog nor a late callback from link one + // may mark link two ready while its hello is still in flight: the + // drain would then write Message fragments beside the handshake. + val first = link(withHello = true) + cccdAcked(first) + shadowOf(Looper.getMainLooper()).idleFor(Duration.ofSeconds(2)) + client.callback.onConnectionStateChange(first.gatt, 19, android.bluetooth.BluetoothProfile.STATE_DISCONNECTED) + idle() + host.drains = 0 + + val second = link(withHello = true) + cccdAcked(second) + assertEquals("link two sent its own hello", listOf(helloUuid), second.shadow.writes.map { it.first }) + + client.callback.onCharacteristicWrite(first.gatt, first.hello!!, BluetoothGatt.GATT_SUCCESS) + idle() + assertEquals("a late callback from link one", 0, host.drains) + + // Past link one's watchdog (3 s after its hello), short of link two's. + shadowOf(Looper.getMainLooper()).idleFor(Duration.ofMillis(1500)) + assertEquals("link one's watchdog", 0, host.drains) + + client.callback.onCharacteristicWrite(second.gatt, second.hello!!, BluetoothGatt.GATT_SUCCESS) + idle() + assertEquals("link two's own callback", 1, host.drains) + } + private class FakeHost(private val hello: ByteArray?) : CentralGattClient.Host { var drains = 0 override val protocol: OfflineProtocol diff --git a/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/StaleAddressRegistryTest.kt b/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/StaleAddressRegistryTest.kt index 41469b10..760af456 100644 --- a/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/StaleAddressRegistryTest.kt +++ b/bindings/react-native/android/src/test/java/com/offlineprotocol/ble/StaleAddressRegistryTest.kt @@ -106,4 +106,28 @@ class StaleAddressRegistryTest { assertFalse(registry.hasEstablishedLink("peerA")) assertEquals(setOf("server-side", "client-side"), registry.addressesForDevice("peerA").toSet()) } + + @Test + fun `a hello-mapped link that closes gives the peer back its live address`() { + // We hold a client link to peerA at its advertising address. Its + // central then opened a link to our server from its central-role + // address and wrote a hello, which made that address peerA's + // send address. When that link closes, sending must go back to the + // client link rather than to an address that does not advertise. + val registry = MeshConnectionRegistry() + registry.setDeviceIdentifier("advertising", "peerA") + registry.trackServerConnection("central-role") + registry.setDeviceIdentifier("central-role", "peerA") + assertEquals("central-role", registry.addressForDevice("peerA")) + + registry.untrackServerConnection("central-role") + // A live link at the advertising address. The registry's client map + // needs a BluetoothGatt; a server link counts the same for liveness. + registry.trackServerConnection("advertising") + assertTrue(registry.hasOtherLiveLink("peerA", excluding = "central-role")) + registry.removeIdentifiersForAddress("central-role") + + assertEquals("advertising", registry.addressForDevice("peerA")) + assertEquals(null, registry.deviceIdForAddress("central-role")) + } } diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs index ed657725..d4677c2d 100644 --- a/crates/offline-protocol-uniffi/src/lib.rs +++ b/crates/offline-protocol-uniffi/src/lib.rs @@ -14450,6 +14450,39 @@ mod tests { ); } + /// A hello makes the central-role address a peer's send address. When the + /// link from that address closes cleanly while the peer is still up on + /// another link, the address must be dropped so sending goes back to the + /// live link. Kept, every fragment queued and the drain dialled an address + /// that does not advertise, while our own link to the peer sat idle. + #[test] + fn react_native_android_clean_drop_of_a_hello_mapped_link_repoints_the_peer() { + let facade = rn_source_code_only( + "android/src/main/java/com/offlineprotocol/ble/BleTransportFacade.kt", + ); + let decl = "private fun handleCentralDisconnectedOnBleThread("; + let start = facade + .find(decl) + .expect("the facade handles a server-side disconnect") + + decl.len(); + let body = &facade[start..]; + let body = &body[..rn_kotlin_body_end(body)]; + let rule = body + .find("if (isCleanDisconnect && connections.getGatt(address) == null) {") + .expect("a clean server-side disconnect must check for a client link at the address"); + let rest = &body[rule..]; + let live = rest + .find("connections.hasOtherLiveLink(peerId, address)") + .expect("the peer must still be up elsewhere before its address is dropped"); + let drop = rest + .find("connections.removeIdentifiersForAddress(address)") + .expect("the dead central-role address must be dropped"); + assert!( + live < drop, + "check the live link before dropping the address" + ); + } + /// Android hears Bluetooth go off and come back from the adapter's state /// broadcast, not only from a failed scan start. Polled once a minute, the /// transport missed a Bluetooth stack crash entirely (the stack restarts