[tylerkron]

Summary

• Removes the MaxRetryAttempts = 3 cap in ProtobufMessageParser. After 3 failed parse attempts the loop exited without advancing past unrecognizable bytes — the consumer buffer stayed clogged forever and downstream MessageReceived events stopped firing.
• Replaces the cap with single-byte resync on every failure (malformed varint, oversized length, parse exception). The parser now drains until either a real frame is recovered or the buffer is empty.
• Adds regression test ProtobufMessageParser_ParseMessages_RecoversFromLeadingGarbage that prepends 20 zero bytes to a valid WriteDelimitedTo frame and asserts the parser recovers MsgTimeStamp = 42 and consumes the full input.

Why this matters

This regression manifests as the desktop Live Graph receiving zero stream messages after StartStreaming. The DTR pulse on Microchip USB-to-Serial chips emits a few boot-time bytes when the port opens; those land at the head of the consumer buffer. Pre-fix, QueuedMessageCount (which is byte count, not message count) climbed from 66 → 8202 in 2 seconds while no frames were dispatched. Post-fix, the parser advances past the boot garbage and the first valid streaming frame is recovered.

Test plan

• dotnet test --filter "FullyQualifiedName~ProtobufMessageParser" -f net9.0 — 8/8 pass (new test green)
• dotnet test --filter "FullyQualifiedName~Consumers" -f net9.0 — 41/41 pass (no regressions)
• Smoke test with desktop streaming flow against /dev/cu.usbmodem101 after the new Core release lands

🤖 Generated with Claude Code

[qodo-code-review]

Review Summary by Qodo

Remove retry cap and implement single-byte resync in protobuf parser

🐞 Bug fix

Walkthroughs

Description

• Removes MaxRetryAttempts cap that caused parser to stall on garbage bytes
• Implements single-byte resync on every parse failure to drain buffer
• Adds regression test for leading garbage recovery scenario
• Fixes desktop Live Graph receiving zero messages after stream start

Diagram

flowchart LR
  A["Malformed/Invalid Bytes"] -->|Old: 3 retries then exit| B["Buffer Stalls"]
  A -->|New: Advance 1 byte| C["Continue Parsing"]
  C -->|Valid Frame Found| D["Message Recovered"]
  C -->|Buffer Empty| E["Wait for More Data"]

Loading

File Changes

1. src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs 🐞 Bug fix +15/-21

Remove retry cap and implement continuous resync

• Removes MaxRetryAttempts = 3 constant that limited resync attempts
• Replaces retry count logic with unconditional single-byte advancement on parse failures
• Consolidates invalid message length checks (zero or oversized) into single condition
• Advances by 1 byte instead of Math.Max(prefixBytes, 1) for better garbage recovery
• Removes retryCount variable and related tracking throughout parse loop

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

---

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

Add regression test for garbage recovery

• Adds new regression test ProtobufMessageParser_ParseMessages_RecoversFromLeadingGarbage
• Test prepends 20 zero bytes (junk) to valid protobuf frame
• Asserts parser recovers the valid message with MsgTimeStamp = 42
• Verifies full input buffer is consumed despite leading garbage

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 allocates per attempt ☑ 🐞 Bug ➹ Performance

Description

ProtobufMessageParser.ParseMessages copies each candidate payload with Slice(...).ToArray() before
parsing; with the retry cap removed and byte-by-byte resync, garbage buffers can trigger many
allocations and exceptions, increasing CPU/GC pressure and delaying downstream MessageReceived
processing.

Code

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[R67-79]

            try

Evidence

The parser now loops over the entire buffer (no retry cap) and on each candidate frame it allocates
a new byte[] payload, then relies on exceptions for control flow to resync; this can repeat for many
indices in a garbage-filled buffer. StreamMessageConsumer copies the buffer and calls ParseMessages
each read, so any extra CPU/GC in ParseMessages directly affects streaming responsiveness.

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[31-79]
src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs[12-14]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[198-207]

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` allocates a new `byte[]` for every parse attempt (`Slice(...).ToArray()`), and the PR removes the retry cap, so resync can now cause many allocations/exceptions per buffer under garbage input.

### Issue Context
This parser runs on the streaming hot path (called from `StreamMessageConsumer.ProcessMessageBuffer` for every read). Garbage-at-head scenarios are expected in real usage, and the new behavior may amplify CPU/GC overhead.

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

### What to change
- Avoid copying the payload:
 - Prefer `DaqifiOutMessage.Parser.ParseFrom(byte[] data, int offset, int length)` if available.
 - Otherwise, parse from the original buffer using `CodedInputStream` over the existing `byte[]` segment (offset/length) to avoid `ToArray()`.
- Narrow the exception handling to protobuf parse exceptions if feasible, so unexpected exceptions aren’t silently used for resync.

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

---

ⓘ The new review experience is currently in Beta. Learn more

[tylerkron]

Re: Qodo "Resync allocates per attempt" — addressed in d9aa65b

Fixed the allocation by switching to MessageParser<T>.ParseFrom(ReadOnlySpan<byte>) (available in Google.Protobuf 3.18+, we're on 3.34.1). Eliminates the per-attempt byte[] copy entirely while keeping the slice/length math the same.

Not addressing the second part of Qodo's note (exception-as-control-flow) in this PR — short of reimplementing protobuf parsing, the only way to know whether a candidate length-prefixed slice is valid is to attempt to parse it. The garbage-recovery path is already a cold/exceptional case (real streaming traffic parses the first try and never hits the catch); the allocation fix removes the dominant cost.