Skip to content

fix(bindings): a closed hello-mapped link gives the peer back its live address, and each link keeps its own hello - #518

Merged
bahdotsh merged 1 commit into
mainfrom
fix/android-hello-followups
Oct 6, 2026
Merged

bahdotsh merged 1 commit into
mainfrom
fix/android-hello-followups

Conversation

@kivtxs

@kivtxs kivtxs commented Oct 6, 2026

Copy link
Copy Markdown
Member

Follow-up to #516, from reviewing main after 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)

  • Cause: a hello makes the writer's central-role address the peer's send address (setDeviceIdentifier overwrites deviceToAddress). When that link closes with status 0/19, the clean branch of handleCentralDisconnectedOnBleThread keeps the mapping.
  • What happens next: resolveTargetAddress returns the dead address. The drain finds no gatt and no subscription there, so it queues every fragment. Because hasServerConnection is now false, it also dials that non-advertising address, and our own client link to the peer sits idle.
  • How long: it heals only when the peer dials in again with a new hello, or the dial times out (~30 s).
  • Fix: on a clean disconnect where we hold no client link at the address and hasOtherLiveLink is true, drop the address. removeIdentifiersForAddress already 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)

  • Cause: helloInFlight was keyed by address and never cleared on teardown.
  • Failure: if a link closes mid-hello and a new link opens at the same address within the 3 s watchdog, the old watchdog, or a late callback from the old link, marks the new link ready while its own hello is in flight. The drain then runs Message writes beside the handshake.
  • Fix:
    • entries carry the BluetoothGatt they belong to;
    • they're removed in handleDisconnected, closeGattClient, forgetLink and clearAll;
    • the map is a ConcurrentHashMap, because disconnects arrive on a binder thread.

Tests

  • CentralGattClientHelloTest: a link reopened at the same address keeps its own hello. It fails on main (1 of 5 in the class) and passes here.
  • StaleAddressRegistryTest: a hello-mapped link that closes gives the peer back its live address.
  • Source guard react_native_android_clean_drop_of_a_hello_mapped_link_repoints_the_peer pins the facade rule (the facade has no harness).
  • Android JVM suite: 630/630. react_native_android* guards pass, and cargo fmt is clean.

Also from the review, not changed here

  • Hello "already announced" uses the address map (BleTransportFacade.kt around the bound == peerId branch). That map can outlive blePeerLost when finalizeGivenUpPeer drops only one address, so a returning hello may skip blePeerDiscovered. An explicit announced-to-core set would be safer.
  • iOS peers reconnecting from a rotated address send no hello. If dial-back resolution takes more than the 15 s grace, the peer is reported lost while it is writing to us.
  • With a hello-mapped server link up, any drop of our client link now takes the stale path (no backoff redial).
  • TS seq gap check: an out-of-order pair would produce two false native_event_gap reports. RN keeps order today, so this is theoretical.
  • Release: chore(release): 0.28.0 #509 renamed Unreleased to 0.28.0 before fix(protocol): a new confirmation probe supersedes the last #515/fix: close the four open issues (#510, #512, #513, #514) #516/fix(bindings): UIScene lifecycle for RN example apps on iOS 26+ #517 merged. Their entries now sit under a new [Unreleased] above [0.28.0], and UPGRADING.md §26 doesn't yet mention the 15 s neighbor_lost grace, the Hello characteristic, native_event_gap or the wider relay fan-out. If v0.28.0 is cut from current main, those should move under 0.28.0 first. I can do that PR.

…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
kivtxs force-pushed the fix/android-hello-followups branch from f85d105 to 4fb2ada Compare October 6, 2026 17:44
@bahdotsh
bahdotsh merged commit 0e47f26 into main Oct 6, 2026
24 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants