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