Skip to content

Complete only the oldest matching pending write on onCharacteristicWrite - #273

Open
tanguy-penfen wants to merge 1 commit into
Navideck:mainfrom
tanguy-penfen:write-completion-fifo
Open

Complete only the oldest matching pending write on onCharacteristicWrite#273
tanguy-penfen wants to merge 1 commit into
Navideck:mainfrom
tanguy-penfen:write-completion-fifo

Conversation

@tanguy-penfen

@tanguy-penfen tanguy-penfen commented Jul 28, 2026

Copy link
Copy Markdown

Problem

On Android, onCharacteristicWrite completes every pending write future matching
(device, service, characteristic) with the single callback status, and writeValue adds
the future to writeResultFutureList before the native gatt.writeCharacteristic()
call returns.

Android serializes GATT operations per device, so a write callback always belongs to the
oldest accepted write. When a callback for a previous operation lands between "future
listed" and "native call returned" (a window the size of a binder round-trip), the newer
write is completed with the previous operation's status:

  1. False ACKwriteValue() resolves successfully although the write's bytes were
    never transmitted. With withoutResponse and withResponse writes sharing a
    characteristic, a local no-response completion is indistinguishable from a peer ATT
    response, so the caller's protocol state silently diverges from the peripheral's.
  2. Double completion — if the racing callback completes the new future and the native
    call then returns non-success, the failure branch invokes the already-invoked
    platform-channel reply.
  3. Sustained off-by-one — the falsely-acked write still goes on air; its real response
    later completes the next pending future. Under continuous traffic every write is
    acknowledged by its predecessor's outcome; a quiet gap heals it.

When can two pending futures for one characteristic coexist? Three verified paths:

  • under the default global command queue: a command timeout abandons the platform call
    on the Dart side while the Kotlin future stays in writeResultFutureList, and the queue
    then releases the next command — a late peer response completes both the stale and the
    fresh future;
  • QueueType.none is a documented API — concurrent commands are supported usage;
  • a late or duplicate stack callback while any future is pending: with per-characteristic
    matching, one stray callback acknowledges a write that was never transmitted.

We traced a field incident in our app to exactly this signature: a stop command reported as acknowledged that provably never executed on the peripheral, followed by a stretch of misattributed acknowledgements that ended after a connection gap.

Fix

Complete only the oldest matching future. This is exact for the Android backend —
per-device GATT serialization guarantees the callback belongs to the oldest accepted
operation — and it defuses all three failure modes above: a future that is listed but not
yet accepted natively can no longer be completed by an earlier operation's callback.

cleanUpConnection intentionally keeps complete-all semantics: on disconnect, every
pending write has genuinely failed. The #268 delivery properties are preserved: removal
under the lock, completion posted to the main looper, each completion individually
contained.

One deliberate trade worth naming: if an accepted operation's callback were permanently
lost while the link stayed up, the stale queue head would shift subsequent attributions
(the previous behavior "healed" this by flushing everything on the next callback — as
false ACKs). In practice the ATT transaction timeout tears the link down within ~30 s and
cleanUpConnection fails every pending write, which bounds that case.

Tests

The Android unit tests do not compile on main — the template
UniversalBlePluginTest.kt still references onMethodCall, gone since the pigeon
migration — so no Android unit test can currently run at all, including the ones this
PR adds. The PR therefore replaces that dead template with WriteCompletionTest.kt
(happy to restore a minimal compiling template test instead, if you'd rather keep the
file). The new test drives the real onCharacteristicWrite against seeded pending
writes:

  • one successful completion must ack only the oldest write, and the second write's own
    later response must complete it exactly once — fails on current main:

    second write is still in flight: acknowledging it with the first write's
    status is a false ACK ==> expected: <0> but was: <1>
    
  • one failed completion must fail only the oldest write, not propagate to the second,
    still-in-flight one — also fails on current main.

With the fix both pass and the existing UniversalBleHelperTest suite stays green
(gradlew :universal_ble:testDebugUnitTest, JDK 17, Gradle 8.14.3).

We also validated this commit in our production app on Android hardware (pinned via
dependency_overrides) with an HCI snoop running: several rapid series of BLE
request/response cycles, deliberate peer-rejected writes, and a full OTA transfer
(~1500 write-without-response packets with write-requests interleaved — the exact mixed
pattern above). Every write request was answered on the air (126/126, p50 36 ms), the
rejections surfaced as genuine ATT error responses, and no response ever arrived without
a pending request.

Scope: Android backend only — the Darwin queue has its own bookkeeping (recently reworked
in #272) and is not touched here.

Notes for review: the test reaches the private writeResultFutureList and
mainThreadHandler via reflection to avoid widening any API — happy to switch to
@VisibleForTesting internals or a small extracted registry if you prefer. Also happy to
add a CHANGELOG entry to this PR.

Related: #268 fixed the thread-safety of this same list; this PR addresses the
completion-matching granularity, which #268 deliberately left unchanged ("write-await
semantics are unchanged").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant