fix(ble): Restore bounded bonded reconnect fallback#5960
Conversation
jamesarich
left a comment
There was a problem hiding this comment.
Verdict: clean PR, no correctness defects found. I traced findDevice → attemptConnection → connectAndAwait → KableBleConnection.connect: the bonded handle's advertisement == null sets autoConnect = true, and the attempt is wrapped in withTimeout(CONNECTION_TIMEOUT = 15s) inside connectAndAwait, so the "bounded by CONNECTION_TIMEOUT" claim holds. Tests are internally consistent (bondedReconnectWindowMillis = settle + 2s + 5s + 1s buffer; the 11_000ms in BleRadioTransportTest matches 3+2+5+1).
Two non-blocking points:
-
This deliberately reverses #5912's premise — confirm that's intended. #5912 treated this exact bonded-handle fallback as a defect ("forces autoConnect=true and hangs for CONNECTION_TIMEOUT (15s)"); this PR restores it. The cost: a device that is genuinely gone now runs ~10s scan + 15s autoConnect timeout ≈ 25s per reconnect iteration before
BleReconnectPolicybackoff re-iterates, versus ~10s before. That's the stated goal (let Android wait patiently via autoConnect), not a bug, but it re-trades the reconnect cadence #5912 optimized. Worth a sentence in the PR body confirming the trade is accepted. -
The central guarantee is untested. Both updated tests use the
RadioInterfaceServicefake whoseconnectAndAwaitreturns synchronously. They provefindDevicereturns the bonded handle and thatconnectAndAwaitis called once, but the headline safety property — "remains bounded byCONNECTION_TIMEOUT" — is asserted only in a comment, never exercised. Fine for unit scope; just flagging that the PR's central claim has no test behind it.
Bottom line: well-documented, no correctness issues. The real question is the design trade — is re-accepting ~25s/iteration autoConnect park the intended behavior over #5912?
Yes, that's intentional. After additional testing, particularly on older Android phones, it became clear that removing the bonded-device fallback was too aggressive. The bounded scan window introduced in #5912 works well when advertisements are seen reliably, but in practice there are legitimate cases where the application simply misses advertisements during that window. On older Android Bluetooth stacks especially, scan results can be delayed or missed even though the device is still nearby and advertising. Restoring the bonded-device fallback lets Android's native The important difference from the behavior before #5912 is that this is still bounded. The fallback is limited by
That's a fair point. The current tests verify that we fall back to the bonded handle after the bounded scan window and exercise that path, but they don't directly prove the 15-second timeout because the fake transport completes synchronously. Exercising that guarantee would require a different style of test that can control the asynchronous connection lifecycle rather than the current fake implementation. Edit: fixing that lint-check |
Overview
This pull request restores the bonded-device BLE reconnect fallback used when a selected bonded device misses the short fresh-advertisement scan windows.
Before this change, the reconnect path preferred a fresh advertisement before connecting, but treated a bonded device as unreachable if both bounded scans missed. That could push devices into repeated reconnect backoff even though Android still had a viable bonded reconnect path.
This change keeps the fresh-advertisement preference from #5912, but restores the bonded handle as a final bounded fallback. When both fresh scans miss, the transport may connect through the bonded handle so Android's
autoConnectpath can patiently wait for the device to advertise again.The trade-off is deliberate: a genuinely unavailable device may spend longer in an individual reconnect attempt, but unlike the pre-#5912 behavior, this fallback is still limited by
CONNECTION_TIMEOUTbefore reconnect policy resumes. In testing, that bounded wait was preferable for real-world reliability on slower devices and older Android Bluetooth stacks, while still avoiding the unbounded hangs that motivated #5912.Key Changes
CONNECTION_TIMEOUTused byconnectAndAwait.Testing
BleRadioTransportTestcoverage for bonded-device scan misses.BleRadioTransportReconnectCrashTestcoverage for targeted-scan miss, escalated-scan miss, and bonded fallback.Migration Notes