Repository navigation
fix(bindings): a closed hello-mapped link gives the peer back its live address, and each link keeps its own hello - #518
Merged
Conversation
…e 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.
kivtxs
force-pushed
the
fix/android-hello-followups
branch
from
October 6, 2026 17:44
f85d105 to
4fb2ada
Compare
bahdotsh
approved these changes
Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #516, from reviewing
mainafter the merges. Both test phones are busy with another workflow, so this is a code review plus tests, not a device run.1. Outbound stalls after a peer's inbound link closes cleanly (medium)
setDeviceIdentifieroverwritesdeviceToAddress). When that link closes with status 0/19, the clean branch ofhandleCentralDisconnectedOnBleThreadkeeps the mapping.resolveTargetAddressreturns the dead address. The drain finds no gatt and no subscription there, so it queues every fragment. BecausehasServerConnectionis now false, it also dials that non-advertising address, and our own client link to the peer sits idle.hasOtherLiveLinkis true, drop the address.removeIdentifiersForAddressalready re-points the peer at its surviving address, and a new link from the peer brings a new hello.2. A stale hello watchdog can mark a newer link ready mid-handshake (low-medium)
helloInFlightwas keyed by address and never cleared on teardown.BluetoothGattthey belong to;handleDisconnected,closeGattClient,forgetLinkandclearAll;ConcurrentHashMap, because disconnects arrive on a binder thread.Tests
CentralGattClientHelloTest: a link reopened at the same address keeps its own hello. It fails onmain(1 of 5 in the class) and passes here.StaleAddressRegistryTest: a hello-mapped link that closes gives the peer back its live address.react_native_android_clean_drop_of_a_hello_mapped_link_repoints_the_peerpins the facade rule (the facade has no harness).react_native_android*guards pass, andcargo fmtis clean.Also from the review, not changed here
BleTransportFacade.ktaround thebound == peerIdbranch). That map can outliveblePeerLostwhenfinalizeGivenUpPeerdrops only one address, so a returning hello may skipblePeerDiscovered. An explicit announced-to-core set would be safer.native_event_gapreports. RN keeps order today, so this is theoretical.[Unreleased]above[0.28.0], andUPGRADING.md§26 doesn't yet mention the 15 sneighbor_lostgrace, the Hello characteristic,native_event_gapor the wider relay fan-out. If v0.28.0 is cut from currentmain, those should move under 0.28.0 first. I can do that PR.