test(sdcard): direct tests for SdCardParseSource's rewind, lease latch and stream ownership

[tylerkron]

Produced by the coverage-gap maintenance routine. It adds test cases only — no production file is touched — so it cannot change device behaviour.

What was wrong

SdCardParseSource is the small class that lets an SD card log session stay lazy. When you parse a downloaded log, the parser reads a short prefix to work out the device configuration and then hands you a session whose samples are produced by re-opening the log from the start every time you enumerate them. That is what keeps a multi-megabyte download at one read buffer of resident memory instead of the whole file, and making "from the start" mean the right thing is entirely this class's job.

It had no tests of its own. The existing parser fixtures reach it only along the happy path — a MemoryStream sitting at position zero, enumerated exactly once, never re-opened while a read is already in flight. Everything that only matters when something is unusual was therefore unpinned, and each of those could break silently:

• it rewinds to where the caller's stream was when it was handed over, not to byte zero, so a log embedded partway through a larger stream still parses. Nothing checked that, so a "helpful" Position = 0 would have looked correct in every existing test;
• a caller-supplied stream has one read cursor, so a second overlapping read is refused rather than allowed to interleave and hand back quietly corrupt samples. Nothing checked that the refusal happens, that it lifts again when the read finishes, or that a stale second dispose of an already-finished read cannot unlatch somebody else's live one;
• the source borrows a caller's stream but owns a stream it opened from a file path, and the lease has to close exactly one of those. Getting it backwards either leaks a file handle per enumeration or closes a stream the caller is still using;
• a file source reports its size as unknown (-1) until it is first opened, so a progress bar does not read a not-yet-opened log as an empty one;
• both argument guards.

How it was fixed

A new SdCardParseSourceTests class with 13 direct cases, one per behaviour above plus the two null guards, the forward-only-stream rejection (which returns null rather than throwing, because the caller has a fallback), and the failed-rewind path — a stream that claims it can seek and then refuses must leave the latch off, or the source is bricked and every later read reports "already being read" for a read that never started.

Every case was verified to be able to fail. Eight mutation rounds were applied to SdCardParseSource.cs from a pristine copy, and the union of what they killed is all 13 cases:

round	mutation	killed
A	drop both ArgumentNullException.ThrowIfNull calls	2
B	rewind origin pinned to 0 instead of the stream's position	1
C	remove the Interlocked.CompareExchange latch and the _disposed idempotence guard	2
D	flip ownsStream on both lease constructions	5
E	drop the try/catch that unlatches on a failed rewind; accept a non-seekable stream; stop recording the file's length	3
F	report Length - Position as the total instead of Length	1
G	apply the single-cursor latch to file sources too	1
H	swallow a missing file and hand back Stream.Null	1

The production file was restored from the pristine copy afterwards and git status shows only the new test file.

Two things a reviewer may want to push back on:

• Open_WhileALeaseOverACallerStreamIsOutstanding_Throws asserts on a fragment of the exception message ("already being read"), not just the type. That matches existing convention in this suite, and here the type alone is genuinely weak — InvalidOperationException is what a misused stream throws for several unrelated reasons.
• DisposeAsync_CalledTwice_DoesNotReleaseALeaseTakenInBetween looks contrived until you notice that an async iterator can be disposed by both its own finally and an outer await using. Without the idempotence guard, that stale dispose silently unlatches the live read and the corruption the latch exists to prevent happens anyway.

Verification

dotnet build clean, 0 warnings. Full solution suite green on net9.0 and net10.0. Bench validation does not apply — this is a test-only change with no device interaction.

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add direct tests for SdCardParseSource rewind, latch, and stream ownership

🧪 Tests 🕐 20-40 Minutes

AI Description

• Add direct unit tests for SdCardParseSource rewind origin and open semantics.
• Verify single-cursor latch behavior, including stale double-dispose safety.
• Cover stream ownership rules and ForFile size/exception behaviors.

Diagram

graph TD
  T["SdCardParseSourceTests"] --> S["SdCardParseSource"] --> L["Lease"]
  L --> CS[("Caller stream")]
  S --> FP["File path"] --> FS[("File stream")]
  L --> FS

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Only cover via existing parser fixtures (integration-style)

• ➕ Less test-only scaffolding (custom streams) to maintain
• ➕ Validates behavior in realistic end-to-end parse scenarios
• ➖ Hard to force edge cases (overlapping reads, seek failures, stale double-dispose)
• ➖ Failures become harder to localize to SdCardParseSource vs parser logic

2. Property-based tests over stream behaviors

• ➕ Can explore many seek/position/dispose interleavings automatically
• ➕ May catch additional corner cases beyond enumerated scenarios
• ➖ Higher complexity and runtime; harder to interpret failures
• ➖ Requires careful modeling of stream contracts and fakes

Recommendation: The PR’s approach—direct, contract-level unit tests focused on rewind origin, latch correctness, and ownership—is the best fit for this component. These behaviors are difficult to validate reliably through higher-level parser fixtures, and the targeted custom stream fakes make the edge conditions explicit and reviewable.

Files changed (1) +347 / -0

Tests (1) +347 / -0

SdCardParseSourceTests.csAdd direct contract tests for SdCardParseSource and Lease +347/-0

Add direct contract tests for SdCardParseSource and Lease

• Introduces 13 focused tests covering: null guards, forward-only stream rejection, rewind-to-original-position semantics, single-cursor latch behavior (including release and stale double-dispose safety), rewind-failure recovery, stream ownership on disposal, and ForFile total-size/exception/overlapping-read behaviors. Adds small helper utilities (temp file writer, stream readers, and stream fakes) to deterministically exercise edge cases.

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

[qodo-code-review]

Code Review by Qodo

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

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Qodo-clean, CI green — ready for review.

Independently re-verified on head cfcfdf8 (nothing needed pushing, so the review round still matches current head):

• Qodo, both surfaces at cfcfdf8: Bugs (0) / Rule violations (0) / Requirement gaps (0), the review's embedded commit marker equals headRefOid, and 0 unresolved qodo-code-review inline threads (0 inline review comments via REST as well). Re-checked twice more after a settle window — the review body came back byte-identical and thread count still 0. The zero-thread result was sanity-checked against fix(sdcard): a blank file-derived string is a gap, not an answer, in the SD card config merge #627, which returns 2 for the same query.

• CI: build SUCCESS, on a run whose head_sha is cfcfdf8 — the run for current head, not an earlier push.

• Re-validated against current main, which matters here: chore(build): centralize the shared build properties in Directory.Build.props #647 (root Directory.Build.props, TreatWarningsAsErrors now on repo-wide) merged after this branch was cut, and branch CI builds the head unmerged so it could not have covered the interaction. Test-merged origin/main locally: clean merge, dotnet build --no-incremental succeeded with 0 warnings / 0 errors under warnings-as-errors, and the full suite is green on the merged tree — Daqifi.Core.Tests 3846 passed / 2 skipped on net9.0 and on net10.0, Daqifi.Mcp.Tests 217 passed.

• Mutation claims re-checked by hand, not taken from the PR body. Five rounds against a pristine copy of SdCardParseSource.cs reproduce the table exactly, and their union kills all 13 cases:

  • _origin = 0 and TotalBytes = Length - Position → kills Open_RewindsToWhereTheStreamWasWhenItWasWrapped_NotToByteZero
  • flip ownsStream on both leases → kills 5
  • apply the single-cursor latch to file sources too → kills ForFile_SupportsTwoOverlappingReads
  • drop both null guards + accept a non-seekable stream + drop the latch + stop recording the file length + swallow a missing file → kills 7
  • drop only the failed-rewind try/catch, keeping the latch → kills Open_WhenRewindingFails_LeavesTheSourceUsable (this one survives the round above precisely because that round also removes the latch, so there is nothing left to fail to unlatch)

  Production file restored from the pristine copy afterwards; working tree clean. Every one of the 13 cases is confirmed able to fail.

• No vacuous-assertion shapes: no assertion on a constant, no mocks to re-assert (the two fakes are MemoryStream subclasses whose overridden behaviour is the thing under test), and no assertion made redundant by an earlier one in the same test. The two plainest-looking lines — the Stream.Position checks in Open_AfterTheOutstandingLeaseIsDisposed_Succeeds and Open_WhenRewindingFails_LeavesTheSourceUsable — each die under a mutation above, so neither is decoration.

Tests only; no production file is touched, so there is no device behaviour to bench. Not merged.