perf(sdcard): parsing an SD log no longer loads the whole file into memory first

[tylerkron]

What was wrong

Opening an SD card log read the entire file into memory before handing back a single sample — all three parsers did it, .bin, .csv and .json alike, even though the API advertises "lazy access to sample data". On a 30 MB protobuf log that meant about 940 MB of live objects and a 1.1 second wait before the first sample appeared. Anyone recording for an hour at a few hundred hertz ends up with a file large enough that the parse runs the process out of memory before it emits anything, and there is no way for a caller to work around it.

How it was fixed

Each parser now reads a short prefix of the file to work out the device configuration, then streams the rest as the caller consumes samples, re-opening the file from the start on each enumeration. Peak memory is one read buffer, whatever the file size.

Two things a reviewer should push back on if they disagree:

• The configuration scan is now bounded to the first 512 protobuf messages (SdCardParseOptions.ConfigurationScanMessageLimit, set to 0 for the old whole-file behaviour). Firmware states these fields in the status message or the first few stream messages; without a bound, a log that never states one field forces a full read before the first sample, which is the thing being fixed.
• The stream you hand ParseAsync must stay open until you have finished enumerating Samples. Both in-tree callers (the MCP CSV export and the example CLI) already do this, and the ParseFileAsync overloads manage their own file handles. A forward-only stream — a pipe or socket — cannot be re-read, so it still falls back to the old decode-up-front behaviour rather than failing.

One incidental fix came out of this: when a log's file name carries no date, the timestamp anchor was DateTime.UtcNow read inside the iterator, so enumerating the same session twice produced different timestamps. It is now captured once, at parse time.

Verification

• Full suite green on net9 and net10 (3060 passed / 2 skipped, plus 86 MCP tests), 0 warnings. The 408 existing SD card tests pass unchanged — no test edits were needed, which is the equivalence check. 17 new tests cover the streaming contract: first sample without reading the file, re-enumeration, the forward-only fallback, the scan bound, and ParseFileAsync on all three formats plus the factory.
• Measured, 30 MB .bin, this branch vs main: peak RSS 988 MB → 64 MB; first sample 1101 ms → 21 ms; full parse 1355 ms → 543 ms; identical 715,967 samples. A 300 MB log now parses in 65 MB RSS and 3.2 s — on main it would need roughly 9 GB.
• Same comparison on 10 MB .csv and 12 MB .json: RSS 108 MB → 61 MB and 112 MB → 64 MB, CSV export byte-identical in both cases.
• Bench (real Nyquist 1, fw 3.7.2, non-destructive): captured a live 8 s stream at 200 Hz over USB (25,379 bytes of real firmware protobuf, 1,269 samples), then exported it to CSV through the old and the new parser — byte-identical, 44,458 bytes. SD:GET on this unit currently returns empty transfers (known firmware issue #703), so the real-bytes check was done on a live capture instead of a downloaded log.

closes #489

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Stream SD log parsing to avoid full-file buffering

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Stream .bin/.csv/.json SD logs so first samples appear without full-file decode.
• Add rewindable parse sources and bounded config scanning to keep memory flat.
• Add contract tests for streaming, re-enumeration, and forward-only stream fallback.

Diagram

graph TD
  A(["Client code"]) --> B["SdCardFileParserFactory"] --> C["SdCard*FileParser"] --> D["SdCardParseSource"] --> E[("SD log file")]
  C --> F["SdCardLogSession.Samples"] --> D
  C --> G["SdCardTextLineReader"]
  subgraph Legend
    direction LR
    _a(["Caller"]) ~~~ _p["Component"] ~~~ _f[("File")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Single-pass only (disallow re-enumeration)

• ➕ Avoids re-opening/rewinding costs and complexity of “replay” semantics
• ➕ Simplifies lifetime requirements: consume once, then discard
• ➖ Breaking change vs current “lazy access” expectations and existing callers/tests
• ➖ Harder to use in tooling that naturally enumerates multiple times (export + preview)

2. Keep one open handle and rewind via Seek

• ➕ Avoids file re-open overhead while still allowing multiple enumerations
• ➕ Can preserve streaming behavior for seekable streams
• ➖ Requires the original stream/file handle to stay open for the session lifetime (harder ownership model)
• ➖ Increases risk of handle leaks and contention; ParseFileAsync would need explicit disposal semantics

3. Pipeline-based parsing (System.IO.Pipelines)

• ➕ Potentially higher throughput and lower allocations for protobuf framing and text scanning
• ➕ Clear separation of producer/consumer backpressure
• ➖ More invasive refactor and steeper maintenance/debug cost
• ➖ Harder to keep behavior identical across three parsers without expanding surface area

Recommendation: The chosen approach (bounded config prefix + streaming sample iterator that re-reads per enumeration) is the best fit for preserving the advertised lazy contract while fixing OOM behavior. The main tradeoff—re-reading header/prefix per enumeration—is acceptable for SD logs and keeps ownership simple (ParseFileAsync owns handles; ParseAsync requires caller to keep the stream open).

Files changed (8) +1189 / -334

Enhancement (7) +699 / -334

SdCardCsvFileParser.csRefactor CSV parser to stream lines and support re-enumeration +165/-125

Refactor CSV parser to stream lines and support re-enumeration

• Switches from buffering all lines to a rewindable source-based approach that reads only the header region up front and streams data rows during Samples enumeration. Adds a ParseFileAsync path-based overload that re-opens the file per enumeration and anchors timestamps at parse time to keep them stable across repeated enumerations.

src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs

SdCardFileParser.csStream protobuf parsing with bounded configuration scan +227/-142

Stream protobuf parsing with bounded configuration scan

• Reworks '.bin' parsing to avoid ReadAllMessages by introducing an async message stream and a short prefix scan to resolve device configuration before yielding samples. Adds ParseFileAsync that uses a rewindable file source, implements forward-only stream fallback to eager buffering, and anchors base time once to prevent timestamp drift across multiple enumerations.

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs

SdCardFileParserFactory.csRoute ParseFileAsync to format-specific streaming parsers +11/-13

Route ParseFileAsync to format-specific streaming parsers

• Removes factory-owned FileStream handling and dispatches directly to each parser’s ParseFileAsync overload. Documents the contract that the underlying file must remain present because samples are read lazily during enumeration.

src/Daqifi.Core/Device/SdCard/SdCardFileParserFactory.cs

SdCardJsonFileParser.csStream JSONL parsing with header inference from first line +92/-54

Stream JSONL parsing with header inference from first line

• Switches JSON parsing from full-file line buffering to a streaming approach: reads only the first line to infer layout/config and then streams remaining lines during Samples enumeration. Adds ParseFileAsync path-based overload and anchors base time once for stable timestamps across re-enumeration.

src/Daqifi.Core/Device/SdCard/SdCardJsonFileParser.cs

SdCardParseOptions.csAdd bounded protobuf configuration scan option +19/-0

Add bounded protobuf configuration scan option

• Adds ConfigurationScanMessageLimit (default 512, 0 = unbounded) to cap how much of a '.bin' log is read before the first sample. Clarifies that progress reporting occurs during Samples enumeration because that’s when streaming reads happen.

src/Daqifi.Core/Device/SdCard/SdCardParseOptions.cs

SdCardParseSource.csIntroduce rewindable parse source abstraction for streaming sessions +113/-0

Introduce rewindable parse source abstraction for streaming sessions

• Adds SdCardParseSource to support re-reading from the beginning either by rewinding a seekable caller stream or by re-opening a file path on each read. Provides a Lease abstraction to manage ownership and disposal when the source owns the underlying FileStream.

src/Daqifi.Core/Device/SdCard/SdCardParseSource.cs

SdCardTextLineReader.csAdd reusable async text line streaming helper +72/-0

Add reusable async text line streaming helper

• Introduces SdCardTextLineReader to stream non-blank lines from either a Stream or SdCardParseSource without loading the file into memory. Adds a small adapter to expose materialized sequences as IAsyncEnumerable so streaming and fallback paths share the same iterator code.

src/Daqifi.Core/Device/SdCard/SdCardTextLineReader.cs

Tests (1) +490 / -0

SdCardStreamingParseTests.csAdd streaming contract tests for SD log parsers +490/-0

Add streaming contract tests for SD log parsers

• Introduces new tests that assert first-sample latency without full-file reads, re-enumeration support, forward-only stream fallback behavior, and configuration scan bounds. Includes small stream helpers (counting/forward-only) and temp file utilities to validate ParseFileAsync behavior across formats and factory.

src/Daqifi.Core.Tests/Device/SdCard/SdCardStreamingParseTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. Undocumented stream lifetime ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The stream-based ParseAsync overloads can now return an SdCardLogSession whose Samples will
re-read/rewind the provided stream; disposing the stream after ParseAsync returns can break
enumeration, but the public XML docs don’t state this new requirement.

Code

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[R50-53]

+        var source = SdCardParseSource.TryCreate(fileStream);
+        if (source != null)
+        {
+            return await BuildSessionAsync(

Relevance

●●● Strong

They frequently accept XML-doc clarifications when behavior/contract changes, especially around
SdCard APIs.

PR-#321
PR-#388

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ParseAsync now wraps seekable streams in SdCardParseSource, which rewinds and reuses the
caller’s stream on subsequent reads; the session type only documents laziness, not that it keeps a
dependency on the input stream’s lifetime.

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[26-74]
src/Daqifi.Core/Device/SdCard/SdCardParseSource.cs[67-90]
src/Daqifi.Core/Device/SdCard/SdCardLogSession.cs[22-31]

Agent prompt

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

### Issue description
`ParseAsync(Stream, ...)` now returns a session that may lazily re-read the provided stream during `SdCardLogSession.Samples` enumeration. If the caller disposes the stream after `ParseAsync` returns (a common pattern when returning a session), enumeration can fail at runtime, but the public API docs do not communicate that the input stream must remain open until all sample enumeration(s) complete.

### Issue Context
This change is intentional for streaming performance, but it’s a behavioral contract change for a public API surface. The path-based overloads already document that the file must still exist when enumerating samples; the stream-based overloads should similarly document the required lifetime/ownership expectations.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[26-74]
- src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[27-71]
- src/Daqifi.Core/Device/SdCard/SdCardJsonFileParser.cs[19-63]
- src/Daqifi.Core/Device/SdCard/SdCardFileParserFactory.cs[49-68]
- src/Daqifi.Core/Device/SdCard/SdCardLogSession.cs[22-31]

### Notes
Update XML docs/remarks to explicitly say:
- The provided stream must remain open until enumeration of `Samples` is finished.
- For streaming behavior, the stream must be seekable; forward-only streams will be buffered (old behavior).

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

---

2. Concurrent enumeration corrupts stream ✓ Resolved 🐞 Bug ☼ Reliability

Description

When ParseAsync is given a seekable stream, SdCardParseSource.Open rewinds and reuses the same
underlying stream for each enumeration; overlapping enumerations can interfere via shared Position
and yield incorrect samples.

Code

src/Daqifi.Core/Device/SdCard/SdCardParseSource.cs[R88-89]

+        _stream!.Position = _origin;
+        return new Lease(_stream, ownsStream: false);

Relevance

●● Moderate

Concurrency/thread-safety concerns are valid but fix could be behavioral/architectural; team may
prefer documenting non-thread-safe contract.

PR-#435
PR-#384

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

SdCardParseSource stores a single _stream and Open() mutates its Position and returns it
without synchronization, so multiple enumerators share and mutate the same stream state.

src/Daqifi.Core/Device/SdCard/SdCardParseSource.cs[67-90]

Agent prompt

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

### Issue description
For sessions created from a caller-supplied seekable `Stream`, `SdCardParseSource.Open()` returns the same underlying stream instance and rewinds it to the origin each time. If a consumer enumerates `SdCardLogSession.Samples` concurrently (or starts a second enumeration before disposing the first), the enumerations will race on a shared `Stream.Position`, leading to corrupted/duplicated/skipped reads.

### Issue Context
Path-based parsing avoids this by opening a fresh `FileStream` per enumeration. The issue is limited to the `Stream`-backed source path.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardParseSource.cs[67-90]

### Suggested fix direction
Implement one of:
- Enforce single-active-lease semantics for stream-backed sources (e.g., guard with a `SemaphoreSlim`/flag; throw `InvalidOperationException` with a clear message if `Open()` is called while a previous lease is active).
- Or always materialize/clone the stream for stream-backed sources when re-enumeration is required (tradeoff: memory/disk).
- At minimum, explicitly document that concurrent enumeration is unsupported for stream-based sessions (but note: this still leaves silent data corruption possible if consumers do it accidentally).

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

---

3. Progress byte counts wrong ✓ Resolved 🐞 Bug ◔ Observability

Description

CSV/JSON parsing now often reports TotalBytes from the underlying stream/file length, but computes
BytesRead from line.Length + 1 (characters, not bytes) and CSV additionally skips the header
region without counting it, so progress can be inaccurate and may never reach TotalBytes.

Code

src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[R356-359]

        var linesProcessed = 0;
        var bytesRead = 0L;
+        var totalBytes = totalBytesProvider();
+        var skipped = 0;

Relevance

●●● Strong

Team often accepts SdCard byte/size correctness fixes; progress should be byte-accurate to avoid
never reaching 100%.

PR-#257

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Progress is defined in bytes, but CSV/JSON increment bytesRead via string lengths while getting
totalBytes from a byte-sized source; CSV also skips header lines before incrementing bytesRead,
guaranteeing undercount for seekable/file sources.

src/Daqifi.Core/Device/SdCard/SdCardParseProgress.cs[3-9]
src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[341-375]
src/Daqifi.Core/Device/SdCard/SdCardJsonFileParser.cs[157-180]
src/Daqifi.Core/Device/SdCard/SdCardParseSource.cs[74-89]

Agent prompt

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

### Issue description
`SdCardParseProgress` is defined in bytes, but the CSV/JSON parsers currently derive `BytesRead` from decoded string lengths and (CSV) omit the skipped preamble from byte counting. With the new streaming changes, `TotalBytes` is frequently a true byte length (`Stream.Length`), making progress percentages wrong.

### Issue Context
This mostly affects seekable/file-backed sources where `TotalBytes` is known. It’s not a parsing correctness issue, but it is user-visible progress correctness.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardParseProgress.cs[3-9]
- src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[341-375]
- src/Daqifi.Core/Device/SdCard/SdCardJsonFileParser.cs[157-180]
- src/Daqifi.Core/Device/SdCard/SdCardTextLineReader.cs[20-54]

### Suggested fix direction
- For seekable sources, base `BytesRead` on the underlying stream’s byte position (or a counting stream), not `line.Length`.
- Ensure CSV’s skipped preamble still contributes to `BytesRead` so the final report can reach `TotalBytes`.
- If true byte-accurate counting is not feasible for some sources, consider redefining progress to be line-based for CSV/JSON (and rename fields accordingly), but that would be an API change.

ⓘ 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 type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Round 1 — all three findings were valid and are fixed in b8114b6.

1. Undocumented stream lifetime. Right, and it was the one contract change that could bite silently. Every Stream-taking overload (all three parsers plus both factory entry points) and SdCardLogSession.Samples now say it: keep the stream open and don't read from it yourself until enumeration is finished, seekable streams are re-read from their starting position, forward-only streams fall back to decoding up front.

2. Concurrent enumeration corrupts the stream. Also right — one stream, one read position, and the old materialized Samples made overlapping enumeration harmless, so this was a new hazard. Went with the loud option: a stream-backed source now refuses a second overlapping lease with an InvalidOperationException that names the two ways out (finish the first enumeration, or parse from a path). Sequential re-enumeration still works, and path-backed sessions are unaffected because they open the file independently each time. Covered by ParseAsync_StreamBackedSession_RefusesOverlappingEnumerations and ParseFileAsync_FileBackedSession_AllowsOverlappingEnumerations.

3. Progress byte counts. Right that mixing a real Stream.Length total with a char-derived BytesRead made the percentage wrong, and that CSV omitting the preamble meant it never reached the total. BytesRead for CSV/JSON is now the stream's real byte position, preamble included, so a completed read lands exactly on the file length; it advances a read buffer at a time. A source that was already read into memory keeps the old line-length figure, which is self-consistent there. Pinned by ParseAsync_TextProgress_ReachesTheFileLength for both formats.

Re-verified after the fixes: full suite green on net9 + net10 (3064 passed / 2 skipped, plus 86 MCP), 0 warnings; 30 MB .bin still 65 MB peak RSS and 24 ms to first sample; and the CSV exports of the real 8 s Nyquist capture and the 10 MB/12 MB CSV and JSON logs are all still byte-identical to the pre-change parser.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit b8114b6

[tylerkron]

Qodo-clean, CI green — ready for review. (2 rounds on head b8114b6: round 1 raised 3 findings — undocumented stream lifetime, overlapping enumerations sharing a stream cursor, and CSV/JSON progress that could never reach 100% — all valid, all fixed, all three threads resolved. Round 2 came back Bugs (0) / Rule violations (0) / Skill insights (0) with 0 unresolved threads, and a settle re-check 4 minutes later found the review comment byte-identical and still 0 unresolved. build SUCCESS, mergeStateStatus: CLEAN. Bench re-run after the fixes: real 8 s Nyquist capture plus 10 MB CSV / 12 MB JSON logs all export byte-identical to the pre-change parser, and the 30 MB .bin still parses in 65 MB peak RSS with the first sample in 24 ms.)

[tylerkron]

Bench follow-up: the real-firmware equivalence check this PR was missing

The earlier bench run on this PR only got 25 KB of real device data through the new parser, because SD:GET on the bench Nq1 returns empty transfers (firmware #703). Redone here at a size where the change actually matters, on 1.2 MB of genuine firmware protobuf captured off the wire (45 s @ 1 kHz, 3 channels, fw 3.7.2 on /dev/cu.usbmodem1101) — 35,643 samples, 48x the previous sample.

Equivalence, this branch vs. main (655c645), same input file:

input	rows	data columns	relative timestamps
1.2 MB real capture	35,643	byte-identical	identical to the microsecond
36 MB (30x the real capture)	1,069,290	byte-identical	identical to the microsecond

Memory and time on the 36 MB file: peak RSS 1.49 GB → 63.6 MB (24x less), wall clock 4.51 s → 3.47 s. On the 1.2 MB file: 106 MB → 61 MB, 0.68 s → 0.37 s.

One thing worth knowing, because it looks alarming at first. A naive cmp of the two exported CSVs fails. That is not this PR. A captured/parsed file carries no absolute time, so the export anchors on wall clock at parse time — main compared against itself, run twice two seconds apart, differs in exactly the same way. Strip the timestamp column and the bytes are identical; measure each row's offset from the first row and the two sequences match exactly, across all 1,069,290 rows. So the data and the timing are equivalent; only the anchor moves, and it moves on main too.

Still blocked, and not by Core: a live SD:GET end-to-end could not be validated. Four attempts across three files — including a brand-new recording made moments earlier — all returned 0 bytes and stalled. Note for the record that this falsifies the standing workaround: an SD recording with no live stream did not re-arm the transfer. It looks like this unit needs a power cycle, which is outside the non-destructive rules here.

Non-destructive throughout: no format, no delete, no reboot, no firmware ops. Board left streaming-stopped and disconnected cleanly.