spsc-fat: backpressure-preserving pool-exhaustion policy (default) — keep the hardware-ARQ contract under consumer stalls

[josephnef]

Closes #361.

Problem

The spsc-fat ring keeps the USB ring armed by discarding the payload at pool exhaustion — after the chip already admitted and ACKed the frame. Under a hardware-ARQ peer that converts receiver congestion into silent, unrecoverable loss the TX side logs as delivered: the exact field failure from the PixelPilot ACK/retry deployment, reproduced on the bench in #359 (5,468–5,739 ACKed-but-undelivered per escalation run).

Change

PoolExhaust policy (DeviceConfig::Rx::pool_exhaust, DEVOURER_RX_POOL_EXHAUST=backpressure|drop, default backpressure):

• backpressure: at exhaustion the completed URB parks bufferless — its payload still reaches the consumer queue (nothing is lost host-side), the ring shrinks, the chip FIFO fills and the chip declines further ACKs, so overload loss happens chip-side where the ARQ loop sees and retries it. The consumer's buffer-return path re-arms parked URBs; the submit is held under pool_mu so teardown's parking_closed flip strictly excludes a late launch of an uncancellable infinite-timeout URB, and teardown retires the parked set before its cancel pass (parked URBs are not in flight — the active-drain would otherwise hang). Parked URBs stay counted in active: the pump loop must keep running while the whole ring is parked.
• drop: previous behaviour, kept for latency-over-integrity consumers, still counted (pool_dropped).

Telemetry: rx.ring gains pool_stalls (cumulative park events). Docs: docs/logging.md schema row + the CLAUDE.md ACK-horizon paragraph. Harness: DUT_POOL_EXHAUST passthrough in tests/arq_e2e_delivery.sh.

Validation (on-air, 8812EU DUT / 8812CU drone-sim / 8814AU witness, per-seq three-ledger accounting)

Escalation conditions throughout: 40 µs/frame consumer cost + 150 ms consumer freeze every 3000 frames vs a ~2.3 k fps retrying unicast flood — the conditions that reliably produce ~5.5 k contract violations on the drop policy.

arm	acked_undelivered	pool_stalls	pool_dropped	note
backpressure, escalation ×3	0 / 0 / 0	1,428 / 1,433 / 1,454	0	contract holds; min_armed→0 now visible in telemetry (the pump isn't the stalled thread)
drop control (same conditions)	5,465	0	3,484	knob genuinely selects; matches the pre-change 5,468–5,739 series
backpressure, no-stall parity	0	0	0	parked path dormant; ok 99.68% ≈ baseline

Bonus measured property: burst-phase ok% under backpressure is ~98.6 vs ~93 for plain async under identical stalls — the pool still absorbs most of each stall and only the tail converts to chip backpressure, so the mode keeps spsc-fat's smoothness while restoring async's honesty. Visible (retryable) drops per burst phase: ~150 vs async's ~700–900.

All five runs exited cleanly through the parked-retire teardown path; ctest 48/48. Residual noted for the record: an ASan-instrumented run of the parked-URB lifecycle (per #361's teardown checklist) has not been done — the path was exercised by five hardware teardowns, four of them with parked URBs plausible at kill time.

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Add backpressure-preserving pool-exhaust policy for spsc-fat RX ring

🐞 Bug fix ✨ Enhancement ⚙️ Configuration changes 📝 Documentation 🧪 Tests 🕐 40+ Minutes

AI Description

• Add configurable spsc-fat pool-exhaust policy; default to backpressure to preserve ARQ semantics.
• Implement URB “parking” and teardown-safe re-arming to avoid ACKed-but-undelivered loss.
• Extend rx.ring telemetry and harness/docs to measure stalls vs drops.

Diagram

graph TD
  Chip["WiFi chip"] --> URB["libusb RX callback"] --> Policy{"Pool exhausted?"}
  Policy -->|"no"| Rearm["Submit URB"] --> Chip
  Policy -->|"bp"| Park["Park URB"] --> Queue["SPSC queue"] --> Cons["Consumer thread"] --> Rearm
  Policy -->|"drop"| Drop["Drop payload"] --> Rearm
  Park -."chip backpressure".-> Chip

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Auto-grow the host pool under sustained overload

• ➕ Avoids chip-side backpressure and reduces stalls under long consumer freezes
• ➕ Keeps drop-free behavior without shrinking the armed URB set
• ➖ Unbounded memory growth or complex caps/eviction policy
• ➖ Does not enforce ARQ honesty if a capped pool still drops post-ACK under pressure
• ➖ Harder to reason about worst-case latency/GC interactions

2. Degrade to inline consume (block pump thread) on exhaustion

• ➕ Simpler than maintaining a parked-URB set
• ➕ Naturally backpressures the chip without introducing a new URB lifecycle state
• ➖ Violates the spsc-fat design goal (callback must stay microsecond-fast)
• ➖ Higher risk of cascading starvation (ring collapses because resubmit is delayed)
• ➖ More sensitive to libusb event-loop scheduling and stalls

3. Always use drop, but surface it as an explicit contract signal

• ➕ Lowest latency under overload; simplest operational behavior
• ➕ Maintains historical behavior for consumers that accept loss
• ➖ Still breaks hardware-ARQ delivery accounting (TX sees ACKed-but-undelivered)
• ➖ Requires consumers/peer to interpret a new signal; doesn’t help existing ARQ deployments

Recommendation: Keep the PR’s approach: making backpressure the default aligns host behavior with the hardware-ARQ ACK horizon, eliminating silent post-ACK loss while preserving an opt-in drop mode for latency-first consumers. The parked-URB lifecycle and teardown gating are the right tradeoff versus inline blocking or unbounded pool growth, but reviewers should focus on lock ordering and teardown correctness (parking_closed sequencing, active accounting, and submit failure paths).

Files changed (8) +146 / -25

Enhancement (3) +26 / -2

DeviceConfig.hIntroduce PoolExhaust enum and Rx.pool_exhaust config knob +21/-0

Introduce PoolExhaust enum and Rx.pool_exhaust config knob

• Defines PoolExhaust::{Backpressure,Drop} with contract-oriented documentation describing ARQ implications. Adds DeviceConfig::Rx::pool_exhaust (default Backpressure) and documents the DEVOURER_RX_POOL_EXHAUST mapping.

src/DeviceConfig.h

RtlAdapter.cppPlumb pool_exhaust into UsbTransport construction +2/-1

Plumb pool_exhaust into UsbTransport construction

• Extends the UsbTransport constructor call to pass cfg.rx.pool_exhaust. This wires the new policy into the transport layer at adapter initialization.

src/RtlAdapter.cpp

UsbTransport.hAdd PoolExhaust parameter/state to UsbTransport +3/-1

Add PoolExhaust parameter/state to UsbTransport

• Extends the UsbTransport constructor signature with an optional PoolExhaust argument (default Backpressure). Stores the selected policy in a new _pool_exhaust member used by rx_loop.

src/UsbTransport.h

Bug fix (1) +103 / -17

UsbTransport.cppImplement URB parking + teardown-safe re-arming for backpressure policy +103/-17

Implement URB parking + teardown-safe re-arming for backpressure policy

• Adds backpressure-policy state (parked transfers, parking_closed gate, pool_stalls counter) to spsc-fat mode. On pool exhaustion, parks bufferless URBs while still queueing payloads, and re-arms parked URBs from the consumer buffer-return path under pool_mu; adds teardown logic to retire parked URBs before cancel/drain and emits pool_stalls in rx.ring telemetry.

src/UsbTransport.cpp

Tests (1) +2 / -0

arq_e2e_delivery.shExpose DUT_POOL_EXHAUST and pass through to DEVOURER_RX_POOL_EXHAUST +2/-0

Expose DUT_POOL_EXHAUST and pass through to DEVOURER_RX_POOL_EXHAUST

• Adds a DUT_POOL_EXHAUST harness variable and forwards it into the DUT environment. Enables matrix testing between backpressure (default) and drop behavior in the ARQ end-to-end delivery bench.

tests/arq_e2e_delivery.sh

Documentation (2) +9 / -6

CLAUDE.mdDocument spsc-fat pool-exhaust choice and ARQ-visible loss behavior +8/-5

Document spsc-fat pool-exhaust choice and ARQ-visible loss behavior

• Updates the hardware-ARQ “ACK horizon” guidance to reference the new DEVOURER_RX_POOL_EXHAUST policy. Clarifies that backpressure parking keeps overload loss ARQ-visible and points to rx.ring pool_stalls/pool_dropped counters.

CLAUDE.md

logging.mdExtend rx.ring schema with pool_stalls for backpressure mode +1/-1

Extend rx.ring schema with pool_stalls for backpressure mode

• Adds pool_stalls to the rx.ring event schema, defining it as cumulative URB park events under the backpressure policy. Refines the description of pool_dropped to explicitly tie it to the drop policy.

docs/logging.md

Other (1) +6 / -0

env_config.cppParse DEVOURER_RX_POOL_EXHAUST env var into DeviceConfig +6/-0

Parse DEVOURER_RX_POOL_EXHAUST env var into DeviceConfig

• Adds environment parsing for DEVOURER_RX_POOL_EXHAUST with values backpressure/bp or drop. Plumbs the value into cfg.rx.pool_exhaust.

examples/common/env_config.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (1) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Teardown frees parked callback 🐞 Bug ⛨ Security

Description

In backpressure mode, devourer_rx_cb can push a transfer into sh.parked and then continue executing
after releasing pool_mu, while rx_loop teardown can clear parked and force-decrement sh.active to 0,
allowing libusb_free_transfer() to run before that callback returns. Because callbacks may run on
the TX event loop, this creates a shutdown-time use-after-free/UB risk for libusb_transfer objects.

Code

src/UsbTransport.cpp[R540-543]

+    std::lock_guard<std::mutex> lk(sh.pool_mu);
+    sh.parking_closed = true;
+    sh.active -= static_cast<int>(sh.parked.size());
+    sh.parked.clear();

Evidence

UsbTransport explicitly documents that the RX callback may run on both the RX loop event pump and
the TX event loop, meaning teardown (in the RX loop thread) can overlap a callback (in the TX
thread). The backpressure park path pushes t into parked and returns without decrementing
active, while teardown force-decrements active by parked.size() and then frees all transfers
when active is no longer >0, which can occur before the still-running callback has returned
through libusb.

src/UsbTransport.cpp[40-43]
src/UsbTransport.cpp[247-267]
src/UsbTransport.cpp[539-553]
src/UsbTransport.cpp[838-863]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In backpressure mode, teardown subtracts `parked.size()` from `active` and clears `parked`, which can make `active` reach 0 while a callback that already parked a transfer is still executing (after it released `pool_mu`). Teardown then frees `libusb_transfer*` objects based on `active`, risking freeing a transfer while libusb is still unwinding its callback.
### Issue Context
- Callbacks can run concurrently from multiple event pumps (RX loop pump and TX event loop).
- The backpressure park path adds a new state where transfers are “pending” but not in-flight, and `active` is manually adjusted at teardown.
### Fix Focus Areas
- Add a separate atomic `cb_inflight` (or similar) in `AsyncRxShared`:
- `++cb_inflight` at the top of `devourer_rx_cb`
- `--cb_inflight` immediately before every return path.
- In teardown, do not call `libusb_free_transfer()` until `cb_inflight == 0` (with a bounded wait).
- Keep `parking_closed`/`parked` retirement, but ensure the "active reaches 0" condition cannot be satisfied while callbacks are still on-stack.
- src/UsbTransport.cpp[40-43]
- src/UsbTransport.cpp[247-267]
- src/UsbTransport.cpp[539-553]
- src/UsbTransport.cpp[838-863]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. No ASan test for parking 📎 Requirement gap ☼ Reliability

Description

The PR introduces parked (bufferless) URBs in spsc-fat backpressure mode, but the existing teardown
ASan scripts do not configure spsc-fat + pool exhaustion to exercise this new shutdown path. This
falls short of the checklist requirement to have teardown tests cover parked-transfer lifecycle
safety (hang/leak/UAF risk).

Code

src/UsbTransport.cpp[R539-543]

+  {
+    std::lock_guard<std::mutex> lk(sh.pool_mu);
+    sh.parking_closed = true;
+    sh.active -= static_cast<int>(sh.parked.size());
+    sh.parked.clear();

Evidence

Rule 5 requires teardown logic for parked transfers and explicit test coverage via teardown ASan
scripts. The PR adds a parked-URB teardown path in UsbTransport::rx_loop() (closing parking and
retiring parked URBs), but the referenced ASan scripts do not configure RX to run in
spsc-fat/backpressure or induce pool exhaustion, so they do not exercise the parked-transfer
lifecycle during teardown.

Ensure teardown cancels and reaps parked transfers like armed transfers (with test coverage)
src/UsbTransport.cpp[534-552]
tests/teardown_gen_sanity.sh[53-91]
tests/tx_teardown_asan.sh[120-126]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New spsc-fat backpressure logic can park completed URBs (`sh.parked`) and has custom teardown steps (`parking_closed`, retiring parked URBs). The compliance checklist requires teardown/ASan tests that actually exercise this parked-transfer path; current scripts run demos without forcing spsc-fat + pool exhaustion, so they likely never cover the new logic.
## Issue Context
- Parked URBs are introduced and explicitly handled during shutdown.
- Existing teardown ASan scripts run `rxdemo`/`txdemo` but do not set `DEVOURER_RX_MODE=spsc-fat` nor force pool exhaustion/parking conditions.
## Fix Focus Areas
- tests/teardown_gen_sanity.sh[53-91]
- tests/tx_teardown_asan.sh[1-126]
- src/UsbTransport.cpp[534-552]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Submit-failure can hang RX 🐞 Bug ☼ Reliability

Description

When re-arming a parked URB from the consumer thread, a libusb_submit_transfer() failure re-parks
the URB but does not retire it (active is unchanged). If submits keep failing, rx_loop can remain
stuck with active>0 and no in-flight transfers, preventing clean shutdown unless the external stop
condition fires.

Code

src/UsbTransport.cpp[R470-473]

+          /* Rare submit failure: re-park the URB, pool the buffer. */
+          t->buffer = nullptr;
+          sh.parked.push_back(t);
+          if (sh.telemetry)

Evidence

The consumer thread re-arms parked transfers and on submit failure re-parks them without adjusting
active. Since rx_loop’s main loop condition is driven by active>0, persistent failures can leave
active nonzero indefinitely even though there are no transfers in flight to generate callbacks
that would change active.

src/UsbTransport.cpp[459-476]
src/UsbTransport.cpp[491-493]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In the consumer-thread buffer-return path (backpressure mode), `libusb_submit_transfer()` failure causes the transfer to be pushed back into `parked` without decrementing `active`. With persistent submit failures, there may be zero in-flight transfers yet `active` stays positive, so `rx_loop` may not make progress/exit.
### Issue Context
- `rx_loop` runs `while (!should_stop() && sh.active > 0)` and pumps libusb events.
- If nothing is in flight, `libusb_handle_events_timeout_completed()` will only return by timeout, and `active` will not change.
### Fix Focus Areas
- Decide a policy for re-arm failure:
- Option A (simple): treat submit failure as retiring that URB: do **not** re-park it; decrement `sh.active` and keep the returned buffer in `free_bufs`.
- Option B: add a bounded retry counter per transfer; after N failures, retire the transfer and decrement `active`.
- Option C: on certain fatal error codes, trigger internal shutdown (set a shared stop flag) so the loop exits.
- src/UsbTransport.cpp[459-476]
- src/UsbTransport.cpp[491-493]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}