rx.parse_abort: count the abandoned-aggregate RX loss on every generation

[josephnef]

What

A machine event for a loss class that was structurally silent: when an RX descriptor walk hits a malformed/truncated descriptor mid-aggregate, it abandons every remaining frame in that bulk-IN buffer. Those frames were already admitted by the chip — and, with an ACK responder armed, already ACKed to the peer — so a hardware-ARQ transmitter counts them delivered and never retries. Jaguar2/Jaguar3/Kestrel broke out of the walk with no trace at all; Jaguar1 warned on stderr only.

One shared helper (src/RxParseAbort.h) now emits rx.parse_abort (off / buf_len / remaining / frame_len / drvinfo / shift / cumulative total) at all four walk sites. Normal end-of-aggregate zero padding (all-zero remainder) is excluded, so the event only fires on real aborts — no event in a session means the walk never abandoned anything. Schema in docs/logging.md.

Why now

A PixelPilot field investigation (8822E pair, hardware ARQ, FEC 1/1) surfaced monitor-correlated ACK-then-drop losses. The parse-walk abort was the one remaining silent path in devourer's RX chain that could produce that signature. Instrumenting it was the falsifier.

Measured

• The suspect is exonerated at field shape: 3-arm ARQ e2e campaign (tests/arq_e2e_delivery.sh, 8822EU DUT, ch161, retry 4, spsc-fat+backpressure, video-shaped 1400 B and aggregating 512 B loads) — ~1.14 M delivered frames, 685 k inside A-MPDU aggregates: 0 parse aborts, 0 ACKed-but-undelivered attributable to the RX chain. The adversarial pair: the A-MPDU arm's verdict line flagged au=2, but the witness ledger shows both frames aired 5× and 4× with rate fallback and were never BlockAcked — CCX false-oks under AGG_EN (the documented A-MPDU accounting collapse), not delivery loss.
• Non-flooding on every family's padding format: tests/parse_abort_smoke.sh (new; ambient-RX per generation, verdict = frames flow ∧ zero events) — 8814AU (J1), 8822BU (J2), 8812CU (J3, post-refactor walk), 35bc:0101 (Kestrel) all OK. The zero-abort result also means the event's cost on healthy links is zero emissions; the all-zero scan only runs on the walk's exit path.
• The smoke's J3 default DUT is the 8812CU, not the 8822EU: the bench 8822EU decodes no ambient 2.4 GHz (green init, DIG sees energy, zero frames — front-end behavior; its 5 GHz RX is proven at 684 k frames in the same campaign), which would fail the smoke for a reason unrelated to the walk under test.

🤖 Generated with Claude Code

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

PR Summary by Qodo

Emit rx.parse_abort for abandoned RX aggregates across generations

✨ Enhancement 📝 Documentation 🧪 Tests 🕐 20-40 Minutes

AI Description

• Add rx.parse_abort machine event when RX descriptor walks abort mid-aggregate.
• Share one helper across Jaguar1/2/3 and Kestrel, excluding benign zero padding.
• Document the schema and add a per-generation ambient-RX smoke test.

Diagram

graph TD
  A["RX bulk-IN completion"] --> B["Descriptor walk"] --> C{"Parse fails?"}
  C -->|"yes"| D["emit_rx_parse_abort()"] --> E["EventSink"] --> F[("JSONL events")]
  C -->|"no"| G["Packet processor"]
  D -. "schema" .-> H["docs/logging.md"]
  D -. "validated by" .-> I["parse_abort_smoke.sh"]
  subgraph Legend
    direction LR
    _proc["Process"] ~~~ _dec{"Decision"} ~~~ _store[("Log store")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Emit a per-loop counter only (no per-abort event)

• ➕ Lower log volume in pathological failure scenarios
• ➕ Simpler downstream processing (single metric)
• ➖ Loses critical context (offset/remaining/frame_len) needed to debug descriptor corruption
• ➖ Harder to correlate with specific URB completions and walk behavior

2. Move abort emission inside each parse_rx_* implementation

• ➕ Parse routines know exact failure reason and field validity
• ➕ Could emit richer, parser-specific diagnostics
• ➖ Duplicates logic across generations and increases maintenance burden
• ➖ Harder to keep padding-exclusion semantics consistent everywhere

3. Treat abort as a hard error (drop/abort RX loop or reset device)

• ➕ Avoids silently continuing after a malformed aggregate
• ➕ May recover from persistent corruption sooner
• ➖ Overreacts to transient descriptor corruption; risks destabilizing RX in the field
• ➖ Operational behavior change beyond instrumentation scope

Recommendation: Keep the PR’s shared-helper approach: it centralizes the padding-exclusion heuristic and ensures uniform visibility across all generations with minimal behavior change. The per-abort event (with offset/remaining and a cumulative counter) is the right granularity for diagnosing post-admission loss without duplicating logic in each parser.

Files changed (8) +150 / -5

Enhancement (6) +85 / -5

RxParseAbort.hAdd shared helper to emit rx.parse_abort with zero-padding suppression +49/-0

Add shared helper to emit rx.parse_abort with zero-padding suppression

• Introduces emit_rx_parse_abort(), which scans the remaining buffer for non-zero bytes to distinguish real aborts from benign aggregate padding. On a real abort it emits rx.parse_abort with offset, buffer lengths, descriptor metadata, and increments a caller-provided cumulative total.

src/RxParseAbort.h

FrameParser.cppEmit rx.parse_abort when Jaguar1 RX walk breaks on invalid descriptor +7/-0

Emit rx.parse_abort when Jaguar1 RX walk breaks on invalid descriptor

• Includes the shared RxParseAbort helper and calls it on the existing Jaguar1 warning path where pkt_len/pkt_offset indicates a malformed/truncated frame. The emitted event reports the remainder slice and cumulative abort count before breaking out of the walk.

src/jaguar1/FrameParser.cpp

FrameParser.hTrack cumulative parse-abort count for Jaguar1 parser +1/-0

Track cumulative parse-abort count for Jaguar1 parser

• Adds a FrameParser member to accumulate the total number of rx.parse_abort emissions across the RX loop lifetime, enabling monotonically increasing totals in logs.

src/jaguar1/FrameParser.h

RtlJaguar2Device.cppEmit rx.parse_abort on 8822B RX descriptor parse failure +10/-2

Emit rx.parse_abort on 8822B RX descriptor parse failure

• Adds a per-RX-loop parse_aborts counter and invokes emit_rx_parse_abort when parse_rx_8822b fails mid-walk. Ensures aborts are visible on Jaguar2 where the walk previously broke silently.

src/jaguar2/RtlJaguar2Device.cpp

RtlJaguar3Device.cppEmit rx.parse_abort on 8822C RX descriptor parse failure +10/-2

Emit rx.parse_abort on 8822C RX descriptor parse failure

• Adds a per-RX-loop parse_aborts counter and calls emit_rx_parse_abort when parse_rx_8822c fails. Aligns Jaguar3 behavior with other generations and prevents silent aggregate abandonment.

src/jaguar3/RtlJaguar3Device.cpp

RtlKestrelDevice.cppEmit rx.parse_abort on 8852B RX descriptor parse failure +8/-1

Emit rx.parse_abort on 8852B RX descriptor parse failure

• Adds a per-RX-loop parse_aborts counter and emits rx.parse_abort when parse_rx_8852b fails while walking an aggregate. Preserves existing break behavior while making the abandoned remainder observable.

src/kestrel/RtlKestrelDevice.cpp

Tests (1) +64 / -0

parse_abort_smoke.shAdd per-generation ambient-RX smoke test for rx.parse_abort non-flooding +64/-0

Add per-generation ambient-RX smoke test for rx.parse_abort non-flooding

• Adds a root-run hardware smoke that runs rxdemo on ambient 2.4GHz traffic for a set of known DUT PIDs, asserting frames flow and rx.parse_abort emits zero times. Captures per-DUT logs and temporarily blacklists relevant kernel modules to avoid driver conflicts.

tests/parse_abort_smoke.sh

Documentation (1) +1 / -0

logging.mdDocument new rx.parse_abort event and field schema +1/-0

Document new rx.parse_abort event and field schema

• Adds an entry for rx.parse_abort describing when it fires (descriptor-walk abort mid-aggregate), why it matters (post-admission/ACKed loss), and the emitted fields including a cumulative counter. Clarifies that all-zero end padding is excluded so absence of the event implies no aborts occurred.

docs/logging.md

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

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📜 Skill insights (0)

1. rx.parse_abort docs duplicated 📘 Rule violation ⚙ Maintainability

Description

The new rx.parse_abort entry in docs/logging.md repeats the same explanatory narrative that is
already present in the new header doc-comment for src/RxParseAbort.h. Duplicating the same
description in two places risks drift and violates the non-redundant documentation rule.

Code

docs/logging.md[94]

+| `rx.parse_abort` | RX (every generation, always on) | t, off, buf_len, remaining, frame_len, drvinfo, shift, total (cumulative) — the RX descriptor walk hit a malformed/truncated descriptor mid-aggregate and abandoned the rest of the bulk-IN buffer: every abandoned frame was already admitted by the chip (and, with an ACK responder armed, already ACKed to the peer), so this is post-admission loss a hardware-ARQ peer counts as delivered. Normal end-of-aggregate zero padding (all-zero remainder) is excluded. No event in a session = the walk never aborted |

Evidence

PR Compliance ID 1 requires avoiding duplicated documentation when the information is already
documented in header doc-comments. The rx.parse_abort description added to docs/logging.md
restates the same narrative already added as the header doc-comment in src/RxParseAbort.h,
creating redundant sources of truth.

CLAUDE.md: Do Not Duplicate Information Already Documented in Header Doc-Comments
docs/logging.md[94-94]
src/RxParseAbort.h[4-10]

Agent prompt

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

## Issue description
`rx.parse_abort` is described in both `docs/logging.md` and the header doc-comment in `src/RxParseAbort.h`, creating duplicated documentation that can drift.

## Issue Context
Compliance requires that documentation not restate content already documented in header doc-comments; secondary docs should point to the authoritative header comment instead.

## Fix Focus Areas
- docs/logging.md[94-94]
- src/RxParseAbort.h[4-10]

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

---

2. Abort total increments silently 🐞 Bug ≡ Correctness

Description

emit_rx_parse_abort() increments total via ++total inside the .f() argument list, so total
is incremented even if the EventSink is disabled and no event is emitted; it also returns true in
that case, contradicting its own “event emitted” return contract. This can make later rx.parse_abort
totals disagree with the number of emitted events when events are disabled and later re-enabled.

Code

src/RxParseAbort.h[R41-44]

+      .f("drvinfo", drvinfo)
+      .f("shift", shift)
+      .f("total", ++total);
+  return true;

Evidence

The helper claims total is “incremented on emit”, but increments it as an argument expression;
argument evaluation happens even if the sink is disabled and Ev becomes a no-op. Event.h shows
Ev bails out immediately when sink.enabled() is false, so in that case the increment/true return
can occur without any emission.

src/RxParseAbort.h[19-45]
src/Event.h[86-93]

Agent prompt

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

### Issue description
`emit_rx_parse_abort()` is documented to increment `total` only when an event is emitted and to return `true` only when an event is emitted. Today it does `++total` inside `.f("total", ++total)`, so the increment happens even if the sink is disabled (no emission). It also returns `true` even when no emission occurs.

### Issue Context
`Ev` short-circuits when `EventSink::enabled()` is false, so no output happens, but C++ still evaluates `++total` before calling `.f()`.

### Fix Focus Areas
- src/RxParseAbort.h[19-45]

### Suggested fix
1. Early-exit when `!sink.enabled()` to avoid both the scan and any counter changes.
2. Move the increment to a separate statement executed only on the emission path:

```cpp
if (!sink.enabled())
 return false;
...
if (all_zero)
 return false;
++total;
Ev(sink, "rx.parse_abort")... .f("total", total);
return true;
```

This restores the stated contract for both `total` and the return value.

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

---

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

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

1. rx.parse_abort docs duplicated 📘 Rule violation ⚙ Maintainability

The new rx.parse_abort entry in docs/logging.md repeats the same explanatory narrative that is
already present in the new header doc-comment for src/RxParseAbort.h. Duplicating the same
description in two places risks drift and violates the non-redundant documentation rule.

Agent Prompt

## Issue description
`rx.parse_abort` is described in both `docs/logging.md` and the header doc-comment in `src/RxParseAbort.h`, creating duplicated documentation that can drift.

## Issue Context
Compliance requires that documentation not restate content already documented in header doc-comments; secondary docs should point to the authoritative header comment instead.

## Fix Focus Areas
- docs/logging.md[94-94]
- src/RxParseAbort.h[4-10]

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

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

2. Abort total increments silently 🐞 Bug ≡ Correctness

emit_rx_parse_abort() increments total via ++total inside the .f() argument list, so total
is incremented even if the EventSink is disabled and no event is emitted; it also returns true in
that case, contradicting its own “event emitted” return contract. This can make later rx.parse_abort
totals disagree with the number of emitted events when events are disabled and later re-enabled.

Agent Prompt

### Issue description
`emit_rx_parse_abort()` is documented to increment `total` only when an event is emitted and to return `true` only when an event is emitted. Today it does `++total` inside `.f("total", ++total)`, so the increment happens even if the sink is disabled (no emission). It also returns `true` even when no emission occurs.

### Issue Context
`Ev` short-circuits when `EventSink::enabled()` is false, so no output happens, but C++ still evaluates `++total` before calling `.f()`.

### Fix Focus Areas
- src/RxParseAbort.h[19-45]

### Suggested fix
1. Early-exit when `!sink.enabled()` to avoid both the scan and any counter changes.
2. Move the increment to a separate statement executed only on the emission path:

```cpp
if (!sink.enabled())
  return false;
...
if (all_zero)
  return false;
++total;
Ev(sink, "rx.parse_abort")... .f("total", total);
return true;
```

This restores the stated contract for both `total` and the return value.

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