[tylerkron]

Summary

• Caps MaxMessageSizeBytes at 4 KB (down from 1 MB) so a garbage varint encoding tens of KB no longer parks the parser waiting for data that will never arrive.
• Adds gap + resync gates to the partial-frame wait path so a plausible-under-the-cap bogus prefix can't stall indefinitely and doesn't lead to re-entering the same garbage on every call.
• Regression test RecoversFromOversizedPrefix asserts a varint for 32768 is rejected and the parser recovers a valid frame sitting right behind it.

Context

v0.19.6 (#169) fixed the unparseable bytes case (advance 1 byte per parse failure, no retry cap). This addresses the other failure mode: bytes that varint-encode a plausible large length. Under the old 1 MB cap, the parser accepted the declared length and took the "wait for more data" branch. Buffer grew unbounded, zero MessageReceived events fired, Live Graph stayed empty despite the device LED blinking.

Diagnosed against a physical Nyquist1 on /dev/cu.usbmodem101 driven by daqifi-core-example-app --mimic-desktop. Core at HEAD streams cleanly at 1 Hz and 100 Hz with 16 analog + 16 digital channels both before and after this fix, so no regression on the happy path.

Test plan

• dotnet test src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj — 820 passed (both net8.0 and net9.0)
• New test ProtobufMessageParser_ParseMessages_RecoversFromOversizedPrefix fails on main, passes with this change
• Live streaming @ 100 Hz, 32 channels, 5s → 992 stream messages
• Live streaming @ 1 Hz, 32 channels + 300ms inter-command delays, 8s → 16 stream messages

🤖 Generated with Claude Code

[qodo-code-review]

Review Summary by Qodo

Fix parser stall on oversized-length garbage prefixes with resync gates

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Cap MaxMessageSizeBytes from 1 MB to 4 KB to reject oversized garbage prefixes immediately
• Add gap and resync gates to prevent indefinite stalls on plausible-but-bogus length prefixes
• Add regression test RecoversFromOversizedPrefix validating parser recovery from garbage

Diagram

flowchart LR
  A["Boot-time garbage<br/>on serial port"] -->|"varint encodes<br/>large length"| B["Parser receives<br/>plausible prefix"]
  B -->|"Old: 1 MB cap"| C["Wait indefinitely<br/>for data"]
  C -->|"Buffer grows<br/>unbounded"| D["Stall: 0 messages<br/>parsed"]
  B -->|"New: 4 KB cap +<br/>gap/resync gates"| E["Reject as garbage<br/>& resync"]
  E -->|"Advance 1 byte<br/>at a time"| F["Recover valid frame<br/>behind prefix"]

Loading

File Changes

1. src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs 🐞 Bug fix +56/-6

Add size caps and resync gates to prevent parser stalls

• Reduced MaxMessageSizeBytes from 1 MB to 4 KB to immediately reject oversized prefixes
• Added MaxPartialFrameGapBytes constant (1 KB) to detect implausible gaps between declared and
 available bytes
• Implemented gap gate: treat declared lengths exceeding available data by more than 1 KB as garbage
• Implemented resync gate: continue advancing one byte at a time if already resyncing without having
 parsed a message yet

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs

---

2. src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs 🧪 Tests +45/-0

Add regression test for oversized prefix recovery

• Added new regression test RecoversFromOversizedPrefix that reproduces the streaming stall
 scenario
• Test prepends a 3-byte varint encoding 32768 (0x80 0x80 0x02) before a valid DaqifiOutMessage
 frame
• Asserts parser correctly rejects the oversized prefix, resyncs, and recovers the valid frame
• Validates that all data is consumed and exactly one message is parsed

src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Resync gate drops partials ☑ 🐞 Bug ≡ Correctness

Description

In ParseMessages, the new wait-path “resync gate” advances currentIndex even when a valid length
prefix was decoded but the frame is merely incomplete, whenever any bytes were previously skipped in
the same call and no message has yet been produced. Because callers remove the first consumedBytes
from their buffers even when zero messages were parsed, this can permanently discard the first
byte(s) of a legitimate frame that spans reads (e.g., leading garbage + partial first real frame).

Code

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[R106-112]

+                if (messageLength - remainingData.Length > MaxPartialFrameGapBytes
+                    || (consumedBytes > 0 && messages.Count == 0))
+                {
+                    currentIndex++;
+                    consumedBytes = currentIndex;
+                    continue;
+                }

Evidence

ParseMessages treats an incomplete frame as garbage and advances by 1 byte when `(consumedBytes > 0
&& messages.Count == 0)` (even if the length prefix was successfully read), which can move the
consumption point into the start of a real frame. StreamMessageConsumer then unconditionally removes
the first consumedBytes from _messageBuffer regardless of whether any messages were produced,
and SdCardFileParser similarly discards the first consumed bytes when carrying forward
leftovers—so advancing into a real-but-incomplete frame irreversibly drops needed prefix/payload
bytes.

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[81-115]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[198-207]
src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[171-189]

Agent prompt

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

### Issue description
`ProtobufMessageParser.ParseMessages` currently advances into the start of a frame when the length prefix is valid but the frame is incomplete, solely because `consumedBytes > 0 && messages.Count == 0`. Downstream consumers treat `consumedBytes` as “safe to drop from the buffer”, so this can permanently discard prefix/payload bytes of a real frame that spans reads after leading garbage.

### Issue Context
This happens in the partial-frame wait branch (`remainingData.Length < prefixBytes + messageLength`). The parser should only consume bytes that are known to be garbage. If a plausible prefix is found and the missing gap is within tolerance, the parser should stop and let the caller retain those bytes for completion.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[81-115]

### Suggested direction
- Remove/relax the `(consumedBytes > 0 && messages.Count == 0)` skip behavior for incomplete frames.
- If you need to ensure progress while resyncing, consider consuming only the bytes *before* the candidate prefix (e.g., set `consumedBytes = currentIndex; break;`) rather than `currentIndex++`.
- Add a regression test: leading garbage + start of a valid delimited frame with truncated payload should not consume into the valid prefix/payload (i.e., consumedBytes should only reflect the garbage).

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

---

2. Gap gate miscomputes bytes ☑ 🐞 Bug ≡ Correctness

Description

The partial-frame “gap gate” compares messageLength - remainingData.Length, but remainingData
includes the length-prefix bytes; this makes the gap check inconsistent with the intent (“declared
length vs bytes available for the message”). While the magnitude is small (varint prefix is 1–5
bytes), it is still a correctness mismatch in the heuristic.

Code

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[R106-107]

+                if (messageLength - remainingData.Length > MaxPartialFrameGapBytes
+                    || (consumedBytes > 0 && messages.Count == 0))

Evidence

The code is already in the branch where remainingData.Length < prefixBytes + messageLength, so the
relevant ‘available payload bytes’ are remainingData.Length - prefixBytes. Using
remainingData.Length underestimates the missing payload by prefixBytes.

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[81-107]

Agent prompt

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

### Issue description
The gap calculation uses `messageLength - remainingData.Length`, but `remainingData.Length` includes the varint length prefix. The intended gap should be based on payload availability (`remainingData.Length - prefixBytes`).

### Issue Context
This is a heuristic, but keeping it mathematically consistent makes behavior easier to reason about and avoids edge-threshold surprises.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[81-112]

### Suggested change
Compute missing payload as:
- `var availablePayload = remainingData.Length - prefixBytes;`
- `var missingPayload = messageLength - availablePayload;`
and compare `missingPayload` to `MaxPartialFrameGapBytes`.

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

---

[tylerkron]

Addressed both Qodo findings in 31168cf:

1. Resync gate drops partials (Bug): Fixed. Removed the consumedBytes > 0 && messages.Count == 0 resync gate and replaced it with a protobuf field-tag plausibility check. When at least one body byte is available, the parser peeks at it — a real DaqifiOutMessage always starts with wire_type ∈ {0,1,2,5} and field_number ≥ 1; a byte that fails both is garbage, anything else is let through for the caller to complete. This keeps the oversized-prefix recovery working without eating real partial frames. Also added the regression test Qodo suggested (PreservesPartialFrameAfterGarbage): leading garbage + truncated real frame → consumedBytes stops exactly at the real frame boundary, zero messages parsed.

2. Gap gate miscomputes bytes (Bug): Fixed. Gap is now messageLength - (remainingData.Length - prefixBytes) — i.e., declared payload minus available payload. The magnitude was small (1–5 bytes) but this makes the heuristic consistent with its intent.

All 823 tests pass (10 parser tests).