Connection-freeze hardening: Phases 2-4 (owner token, idle drop, auth-abuse disconnect)

CLAUDE.md

@@ -12,7 +12,7 @@ pio run -e <env> -t upload       # build + flash
 pio run                          # build every environment
 ```
 
-Common envs: `nrf52840custom`, `esp32-s3-N16R8`, `esp32-s3-N8R8`, `esp32-c3-N16`, `esp32-c6-N4`. CI (`.github/workflows/main.yaml`) builds **all eleven** environments on every push — keep them all green.
+Common envs: `nrf52840custom`, `esp32-s3-N16R8`, `esp32-s3-N8R8`, `esp32-c3-N16`, `esp32-c6-N4`. CI (`.github/workflows/main.yaml`) builds every environment in `.github/firmware-targets.json` on every push — **12** of them — keep them all green. Note `platformio.ini`'s `default_envs` lists only 11: `esp32-wrover-e-N4R8` ships but is NOT in it, so a bare `pio run` silently skips the target most likely to catch a broken `#ifndef OPENDISPLAY_HAS_WIFI` path. Build it explicitly (`pio run -e esp32-wrover-e-N4R8`) before claiming a clean sweep.
 
 Factory provisioning: `OPENDISPLAY_FACTORY_CONFIG_HEX="..." pio run -e <env>` (or `tools/provision_firmware.py`). `scripts/factory_config_gen.py` runs as a pre-build step.
 

docs/PLAN_PHASE0_LINK_DROP_SEAM_2026-07-31.md

@@ -0,0 +1,159 @@
+# Phase 0 — BLE Link-Drop Seam (2026-07-31)
+
+> **SUPERSEDED 2026-07-31 — do not implement from this document.**
+>
+> Both deliverables were folded into **Phase 2 (BLE-HAL foundation)** of
+> [`PLAN_FREEZE_HARDENING_2026-07-31.md`](PLAN_FREEZE_HARDENING_2026-07-31.md), which
+> is the live plan. Read that instead; this file is kept only for the reasoning trail.
+>
+> Two things here are **out of date** and were corrected in the fold:
+>
+> - **The seam signature.** This document specifies `disconnect(uint8_t reason)`. The
+>   live plan specifies `disconnect(uint16_t handle)` with 0x13 hard-coded and *no*
+>   reason parameter. Both stacks were read to settle it: Bluefruit's
+>   `disconnect(uint16_t conn_hdl)` (`bluefruit.h:171`) has no reason parameter at all
+>   — it delegates to `sd_ble_gap_disconnect(_conn_hdl, BLE_HCI_REMOTE_USER_TERMINATED_CONNECTION)`
+>   (`BLEConnection.cpp:206`) — and NimBLE already *defaults* its reason to 0x13
+>   (`NimBLEServer.h:66`). A handle is what both stacks genuinely take, and Phase 3's
+>   admission policy needs to drop a *specific* link, not "the current one".
+> - **The phase numbering.** References below to "Phases 2, 4 and 5" are the earlier
+>   five-phase draft. The live plan has four phases; the seam is in Phase 2.
+>
+> Deliverable 2 (the ESP32 disconnect-reason truncation fix) and the deferred
+> `OdDiscReason` classifier carried over unchanged, and now live in the live plan's
+> Phase 2 seam section.
+
+The foundational seam for [`PLAN_FREEZE_HARDENING_2026-07-31.md`](PLAN_FREEZE_HARDENING_2026-07-31.md).
+Phases 2, 4 and 5 all need to **drop a BLE link from the loop task**, and none can
+today. This phase adds that one capability, plus the minimal fix to stop the
+disconnect-reason log from lying.
+
+No wire change (a disconnect reason is an HCI byte, not an app-protocol field).
+
+## Scope decision — why this is small
+
+An earlier draft of this phase also normalized the *inbound* disconnect reason into
+a six-value enum. That was cut after checking what actually consumes it: **nothing
+in Phases 2–5 branches on why a link dropped.**
+
+- Phase 2 (owner token) releases and tears down on *any* disconnect.
+- Phase 3 (abort) runs the same teardown regardless of reason.
+- Phases 4 and 5 *initiate* the drop, and their authoritative "did I cause this" is
+  a `*DropPending` flag (see [Deliverable 1](#deliverable-1)), not the reason byte.
+
+So the normalized reason would feed a log line and nothing else. A classification
+layer nothing consumes is not worth its surface. It is deferred to
+[Deferred](#deferred-until-something-consumes-it) — a small header and a `switch`,
+cheap to add the day a phase branches on a reason (repeated-MIC-failure handling is
+the likely first customer).
+
+What is **not** deferred is the outbound drop, and the one honest bug in the
+current reason handling.
+
+## Deliverable 1 — `BleTransport::disconnect(uint8_t reason)`
+
+Add to the abstraction ([ble_transport.h](../src/ble_transport.h)); implement per
+target; call **only from the loop task**.
+
+- **ESP32:** `s_server->disconnect(s_connHandle, reason)` when `s_connHandle !=
+  BLE_HS_CONN_HANDLE_NONE`. Return the call's bool; log WARN on failure.
+- **nRF:** `Bluefruit.disconnect(s_connHandle)` when `s_connHandle !=
+  BLE_CONN_HANDLE_INVALID`; keep `restartOnDisconnect(true)` (unlike the DFU path at
+  [device_control.cpp:857](../src/device_control.cpp), which disables it).
+
+**Reason to send: `0x13`** (`BLE_ERR_REM_USER_CONN_TERM` /
+`BLE_HCI_REMOTE_USER_TERMINATED_CONNECTION`, identical in both stacks and in the
+Core Spec's legal `HCI_Disconnect` allowlist). **Do NOT send `0x09`** (`CONN_LIMIT`):
+it is not a legal host-disconnect reason, so the controller rejects it (0x12) while
+the code looks like it worked and the gatecrasher stays connected. Neither constant
+exists in `src/` today; add one named constant with a comment carrying the 0x09
+trap.
+
+**Loop-task only.** The call is made from a loop-serviced helper (or inline in the
+existing `serviceBle*` helpers), never a stack callback — a callback that severs its
+own link mid-dispatch is exactly the class of bug `#132` removed. The phases that
+request a drop do so by raising a `*DropPending` flag; the loop services it. That
+flag — not any reason byte read back afterward — is the authoritative record of a
+self-initiated drop.
+
+## Deliverable 2 — stop the disconnect-reason log from lying (ESP32)
+
+Not a new feature; a correctness fix to what is already logged at
+[main.cpp:472](../src/main.cpp). Today ESP32 stores the reason wrong:
+
+```cpp
+// ble_transport_esp32.cpp:35,99
+static volatile uint8_t s_disconnectReason = 0;
+...
+s_disconnectReason = (uint8_t)reason;   // int -> uint8_t: truncates
+```
+
+NimBLE's `onDisconnect(int reason)` uses two ranges: HCI reasons wrapped as
+`BLE_HS_ERR_HCI_BASE + code` (`0x200 + code`), and host-layer `BLE_HS_E*` codes in
+`1..31`. The `uint8_t` cast keeps only the low byte, so:
+
+- an HCI reason survives by luck (`0x213 & 0xFF == 0x13`), but
+- a host code like `BLE_HS_ENOTCONN = 7` truncates to `0x07`, which reads back as
+  the unrelated HCI code "memory capacity exceeded". The stored byte is ambiguous
+  and the log can name the wrong reason.
+
+nRF is unaffected — it stores a raw HCI `uint8_t` from the SoftDevice with no
+wrapping.
+
+**Fix (ESP32 only, ~3 lines):** widen `s_disconnectReason` to `uint16_t` so the
+`0x200` offset survives capture, and log the raw value as-is:
+
+```cpp
+static volatile uint16_t s_disconnectReason = 0;
+...
+s_disconnectReason = (uint16_t)reason;   // keep the full value, no truncation
+```
+
+`takeDisconnectedEvent`'s out-param widens to `uint16_t*`
+([ble_transport.h:81](../src/ble_transport.h), one caller at
+[main.cpp:471](../src/main.cpp)), and the log line becomes
+`"Disconnect reason: 0x%03X"` so a wrapped HCI reason (`0x213`) and a host reason
+(`0x007`) are visibly distinct rather than colliding on `0x13`/`0x07`. No enum, no
+classifier, no interpretation — just stop discarding half the value.
+
+## Files touched
+
+| File | Change |
+|---|---|
+| `src/ble_transport.h` | add `disconnect(uint8_t)` + the `0x13`/`0x09` reason constant & comment; widen `takeDisconnectedEvent`'s reason out-param to `uint16_t*` |
+| `src/ble_transport_nrf.cpp` | implement `disconnect()`; reason storage unchanged (already a raw HCI byte, widened only to match the signature) |
+| `src/ble_transport_esp32.cpp` | implement `disconnect()`; widen `s_disconnectReason` to `uint16_t`, drop the truncating cast |
+| `src/main.cpp` | update the one `takeDisconnectedEvent` caller + its log line |
+
+No new file, no host test (nothing here is pure logic worth a standalone test — the
+drop needs a board; the widening is a type change verified by build + bench log).
+
+## Verification
+
+- **Build** all envs.
+- **Bench (closes the phase):** on nRF and ESP32, call `disconnect(0x13)` from the
+  loop task and confirm the link actually drops (the `0x09` trap means "it
+  compiled" is not enough — watch for the disconnect on a scanner or the client
+  side). Confirm a real client disconnect logs a sensible reason, and that a
+  NimBLE host-layer reason now logs as `0x0xx` rather than masquerading as an HCI
+  code.
+
+## Deferred until something consumes it
+
+The normalized inbound reason (a `src/ble_disc_reason.h` with an `OdDiscReason`
+enum — `SUCCESS / REMOTE / LOCAL / TIMEOUT / MIC_FAILURE / OTHER` — a pure
+`od_disc_classify(uint8_t hci)` switch, the ESP32 `0x200`-offset normalization, and
+a host test) is **not built here**. It is deferred until a phase branches on a
+reason rather than just logging it. The most likely trigger is MIC-failure-driven
+behaviour (0x3D signals encryption desync — the failure class this whole effort
+targets), e.g. forcing re-auth after repeated MIC failures. When that lands, the
+enum is a small header and a `switch`; the `uint16_t` raw value this phase already
+preserves is exactly the input the classifier needs, so nothing here has to be
+redone.
+
+## Out of scope
+
+- Any behaviour that *acts* on a link drop — that is Phases 2/4/5. Phase 0 only
+  makes the drop possible and the reason log honest.
+- LAN disconnects; the owner token (Phase 2) handles LAN, and TCP has no HCI reason
+  to preserve.

src/ble_transport.h

@@ -11,12 +11,18 @@
 // base: virtual dispatch would cost a vtable and indirect calls for zero
 // benefit, and application code would still only ever see one type.
 //
-// Threading, as of Phase 1: the callback contract is NOT yet symmetric. ESP32
-// stack callbacks are flag-only (they copy into the RX ring and set a flag);
-// nRF still dispatches commands inline on the SoftDevice callback task and runs
-// the app connect/disconnect hooks there. Phase 3 makes nRF match ESP32. Until
-// then the asymmetry lives entirely inside the two implementation files -- see
-// docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md.
+// Threading: the callback contract is now SYMMETRIC. On both targets a stack
+// callback may copy bytes into the RX ring, publish its own connection-instance
+// metadata, attempt the one ownership claim CAS, and set an event flag -- nothing
+// else. Command dispatch, decrypt, EPD streaming, notify() and the connect/
+// disconnect application work all run on the loop() task. (An earlier revision of
+// this note said nRF still dispatched inline and that a later phase would fix it;
+// that landed with the loop/BLE unification.)
+//
+// The claim CAS is the one addition to the historical "copy and flag" rule, and it
+// is deliberate: ownership must be decided at the earliest transport hook, because
+// the write filter below has to be able to answer "is this the owner?" long before
+// any loop pass runs -- during a refresh, up to ~16 s before one.
 class BleTransport {
 public:
     // --- lifecycle ---
@@ -33,9 +39,80 @@ class BleTransport {
 
     // --- state ---
     bool    isReady() const;       // stack initialised and usable
+    // The stack's TOTAL peer count. Note what this is NOT: a test for whether one
+    // particular link is up. CONNECTION_POLICY R1 permits a refused contender to be
+    // transiently attached, so dropping the owner takes this 2->1, never to 0 --
+    // which is why the R3a wait polls instanceLive() per handle instead. Keep using
+    // this only for "is anything connected at all".
     uint8_t connectedCount() const;
     bool    isConnected() const { return connectedCount() > 0; }
-    bool    notifyReady() const;   // connected AND the client has subscribed (CCCD)
+    bool    notifyReady() const;   // owner connected AND subscribed (CCCD), per-instance
+
+    // --- connection instances (CONNECTION_POLICY R2/R3 requirement 5) ---
+    // A fixed per-handle table, sized by the connection cap (3 on ESP32, 1 on nRF),
+    // holding metadata only -- never frames. Callbacks write their own handle's
+    // entry; the loop scans it. That inversion is what makes lost edges stop
+    // mattering: state is bounded by the connection cap rather than by event rate,
+    // so there is nothing to overflow and no eviction policy to specify.
+    //
+    // Liveness IS the packed (handle, epoch) identity word -- all-zero means empty --
+    // so an entry can never present a live identity with a stale state, and the R3a
+    // wait can read identity and liveness in one atomic load.
+    bool     instanceLive(uint16_t handle, uint16_t epoch) const;
+    uint8_t  liveInstanceCount() const;
+    // Packed identity word of the i'th live instance, 0 when the slot is empty.
+    // Phase 3's admission scan walks these to find contenders (any live instance
+    // that is not the owner); Phase 2 only needs it for diagnostics and the wait.
+    uint32_t instanceWordAt(uint8_t index) const;
+    // 0 while this entry's ownership claim is still in flight; otherwise the
+    // IDENTITY WORD the claim was decided for. Published with release ordering after
+    // the claim CAS.
+    //
+    // Why the identity and not a bool: the loop-side scan reads the entry word, the
+    // disposition and the owner word separately and cannot get them atomically. A
+    // bare flag lets it pair one entry's identity with another's disposition after a
+    // slot is retired and reused (ABA). Requiring decidedWord == the entry word
+    // proves the disposition belongs to THIS instance.
+    //
+    // The distinction is load-bearing either way: refusing an in-flight instance can
+    // disconnect the connection that is winning the slot, while never refusing one
+    // leaves a decided loser attached forever -- on nRF, holding the only link.
+    uint32_t instanceClaimDecidedWordAt(uint8_t index) const;
+    static uint8_t instanceCapacity();
+
+    // --- link drop (CONNECTION_POLICY R3a) ---
+    // Requests termination of ONE link. Returns false only on a genuine failure to
+    // ask; a true return means "requested", NOT "down" -- BLE disconnect is
+    // asynchronous, so callers that need the link actually gone must wait on
+    // instanceLive() (bleDropAndWait() in session_guard.cpp does exactly that).
+    //
+    // No `reason` parameter, deliberately. A host-initiated disconnect must send a
+    // Core-Spec-legal HCI reason; 0x13 (REMOTE_USER_TERMINATED) is the only value
+    // this firmware wants, it is what NimBLE defaults to, and it is the only value
+    // Bluefruit can send at all (BLEConnection::disconnect() hardcodes it, with no
+    // reason argument to pass). Note 0x09 (CONN_LIMIT) is NOT legal here: the
+    // controller silently rejects it and the peer stays connected while the code
+    // looks like it worked.
+    //
+    // Loop task only. A callback that severs its own link mid-dispatch is exactly
+    // the class of bug the unified-loop work removed.
+    // Takes the full instance identity, not just a handle: the transport
+    // re-validates that (handle, epoch) is still the live instance immediately
+    // before asking the stack, so a caller acting on a slightly stale scan cannot
+    // disconnect whoever inherited the numeric handle in the meantime.
+    //
+    // RESIDUAL, stated rather than implied: this narrows that window to a few
+    // instructions but cannot close it, because the stack API is handle-addressed
+    // and the host task can retire and reassign a handle at any point. Closing it
+    // fully would need the validate-and-disconnect pair to run on the host task
+    // itself. The exposure is a spuriously dropped client that reconnects -- not
+    // stranded ownership.
+    bool disconnect(uint16_t handle, uint16_t epoch);
+
+    // Negotiated connection interval in ms for `handle`, 0 when unknown. The
+    // central chooses it; this firmware requests none. Phase 4's TX-flush dwell
+    // sizes itself on this rather than on a constant.
+    uint16_t connIntervalMs(uint16_t handle) const;
 
     // --- data out ---
     // false means backpressure ("retry next pass"), not a hard failure: the
@@ -70,15 +147,25 @@ class BleTransport {
     // arriving inside the check-then-clear window is lost. This peek neither
     // introduces nor worsens that; fixing it is a separate change.
     bool eventPending() const;
-    bool takeConnectedEvent();
-    // Optionally reports the stack's disconnect reason code, which is otherwise
-    // lost now that the callback no longer runs application code inline.
-    // rxBoundary, when requested, is the RX ring head at the instant the link went
-    // down -- the dividing line between the departed client's queued frames and any
-    // pushed by whoever connected afterwards. Pass it to bleRxQueueDiscardTo(); a
-    // flush without it drops the next client's frames whenever loop() was blocked
-    // long enough for a reconnect to land before this event was serviced.
-    bool takeDisconnectedEvent(uint8_t* reason = nullptr, uint8_t* rxBoundary = nullptr);
+    // Reports the connecting instance's identity (packed word, link_owner.h) so the
+    // loop can act on a specific newcomer. Under requirement 5 this is a hint: the
+    // instance table, not the event, is the mechanism -- a connect that coalesces
+    // away still leaves a live table entry.
+    bool takeConnectedEvent(uint32_t* instanceWord = nullptr);
+    // Optionally reports the departing instance's identity and the stack's
+    // disconnect reason, which is otherwise lost now that the callback no longer
+    // runs application code inline.
+    //
+    // `reason` is uint16_t because NimBLE's is not a byte: it uses two ranges, HCI
+    // reasons wrapped as BLE_HS_ERR_HCI_BASE + code (0x200 + code) and host-layer
+    // BLE_HS_E* codes in 1..31. The old uint8_t truncation kept only the low byte,
+    // so an HCI reason survived by luck (0x213 & 0xFF == 0x13) while BLE_HS_ENOTCONN
+    // (7) read back as the unrelated HCI "memory capacity exceeded". nRF stores a
+    // raw HCI byte with no wrapping and is unaffected.
+    //
+    // The rxBoundary out-param is GONE: the RX-boundary mechanism it fed is retired
+    // in favour of per-frame identity tags (CommandQueueItem::tag).
+    bool takeDisconnectedEvent(uint16_t* reason = nullptr, uint32_t* instanceWord = nullptr);
 
     // --- identity ---
     const char* addressString();   // advertised BLE address, lowercase "aa:bb:.."