[tylerkron]

What was wrong

Ask a DAQiFi device for its system log and, very occasionally, you would get an answer that was not yours. SYSTem:LOG? ends its dump with a blank line, and that blank is the whole signal the library uses to tell "the device answered and its log is empty" apart from "the device never answered at all". A blank left over from an earlier command that gave up could still arrive in the moment between the library queueing your command and actually writing it to the wire — and it would be counted as your answer. The visible result is an empty log reported for a device that had not yet been asked anything, and, worse, a diagnostics read that stops waiting early because it thinks the reply has already started coming in.

PR #591 closed most of this window. What remained is the gap between "the command has been handed to the writer" and "the writer has put it on the wire", which is where this fix lands.

How it was fixed

Every line the exchange collects now carries a snapshot of the outbound writer taken at the moment the line arrived: how many writes the writer had begun, and whether it still had something waiting. A blank that arrived while a command sat queued behind a writer that had not begun writing anything for this exchange cannot be answering that exchange, so it is dropped. Anything from the instant the write begins onward is kept.

The obvious alternative — waiting for the outbound queue to drain before deciding — is what issue #593 exists to rule out: on a real Nq1 that breaks SYSTem:LOG? outright, 10/10 runs, because the device answers faster than the drain's own poll tick and its genuine terminator lands on the wrong side of the boundary. Nothing here waits for anything; the writer is only ever sampled.

Two things a reviewer may want to push back on, both deliberate:

• The write count rises before the write, not after. A marker recorded after the write can, on a preempted thread, land later than a fast device's reply — which would discard a genuine terminator, the exact failure the drain caused. Counting the write as started is the safe direction, and it still closes nearly all of the window (the queue wait, which is what the window is made of).
• Both halves of the snapshot are load-bearing. A write count that has not moved is not by itself evidence that nothing was asked: a setup action that sends by another route, or sends nothing and expects the device to be mid-dump, leaves it just as still. Requiring work to be outstanding as well narrows the rule to the case in question. The full suite caught this: without it, the SYSTem:LOG? framing test regressed exactly the way the drain does.

Verification

• Full suite green on net9.0 and net10.0 (3733 passed, 2 skipped, each framework).
• New tests fail without the fix and pass with it; the "a blank that arrives once the write has begun is kept" test passes either way — it is the guard against becoming the drain.
• Bench, Nq1 firmware 3.7.2 over serial (/dev/cu.usbmodem1101), example CLI and a read-only probe built against this branch: SYSTem:LOG? 10/10 clean at ~1056 ms per call — the same figure Residual capture-to-write window in TextExchangeEngine's blank-line boundary, and why draining the outbound queue must not be used to close it #593 records for the healthy baseline — with a populated log on the first read and the blank-terminated empty log on the rest. Connect/initialize, --show-status, --lan-chip-info and a short 10 Hz stream all behaved normally. Nothing destructive was run.

closes #593

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix stale blank lines arriving before queued command is written

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Sample outbound writer state per inbound line to detect pre-send stale blanks
• Drop blank lines that arrive while a command is queued but not yet writing
• Add concurrency-focused tests to lock in the queue-to-write boundary behavior

Diagram

graph TD
  A["Caller"] --> B["DaqifiDevice"] --> C["TextExchangeEngine"]
  C --> D["TextConsumer (reader)"] --> C
  B --> E["MessageProducer"] --> F["Transport Stream"]
  D --> G[("SampleOutboundWriter") ] --> B
  C --> H["CollectedLine (text+sample)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Drain outbound queue before capturing boundary

• ➕ Simple mental model (everything before drain is stale)
• ➖ Empirically breaks fast devices: genuine SYSTem:LOG? terminator can land after the drain boundary (Residual capture-to-write window in TextExchangeEngine's blank-line boundary, and why draining the outbound queue must not be used to close it #593)
• ➖ Introduces extra waiting/polling and can slow exchanges

2. Use write-completion count instead of write-start count

• ➕ Completion is an intuitively strong signal of "command definitely sent"
• ➖ Can be observed as 0 while bytes are already on the wire (thread preemption), misclassifying fast replies as stale
• ➖ Fails the exact scenario this PR targets (reply can beat completion marker)

3. Rely only on line-count boundary (#591/#553)

• ➕ No producer coupling; simplest implementation
• ➖ Does not cover the remaining queue-to-write gap; a leftover blank can be misattributed to the new exchange

Recommendation: Keep the PR’s sampling-based approach. It closes the residual queue-to-write window without waiting, and it chooses the safe ordering (count write as started before bytes are handed off) so fast device replies are never discarded. The added tests explicitly lock in the failure modes that the drain and completion-based markers would reintroduce.

Files changed (9) +613 / -22

Bug fix (6) +226 / -20

IMessageProducer.csExpose optional StartedWriteCount on message producers +28/-0

Expose optional StartedWriteCount on message producers

• Adds a default interface property StartedWriteCount (nullable) documenting that the count is incremented at write start and even on failed writes. This provides a non-blocking marker for consumers that need to reason about whether an exchange’s command could have been sent yet.

src/Daqifi.Core/Communication/Producers/IMessageProducer.cs

MessageProducer.csImplement StartedWriteCount and increment before writes +23/-0

Implement StartedWriteCount and increment before writes

• Implements IMessageProducer.StartedWriteCount using an interlocked counter. Increments the counter immediately before writing to the stream (and before potential exceptions) to provide a reliable happens-before marker across threads.

src/Daqifi.Core/Communication/Producers/MessageProducer.cs

DaqifiDevice.csImplement outbound writer sampling for text exchanges +21/-0

Implement outbound writer sampling for text exchanges

• Adds ITextExchangeHost.SampleOutboundWriter() implementation that snapshots the producer’s StartedWriteCount and whether work is outstanding, returning null when unavailable. Carefully snapshots the producer reference and reads state without double-dereferencing to avoid races during teardown.

src/Daqifi.Core/Device/DaqifiDevice.cs

ITextExchangeHost.csAdd SampleOutboundWriter contract to exchange host +21/-0

Add SampleOutboundWriter contract to exchange host

• Extends the text exchange host interface with a non-blocking SampleOutboundWriter() API, explicitly documenting that it must be cheap and never wait (to avoid the drain-based regression).

src/Daqifi.Core/Device/Internal/ITextExchangeHost.cs

OutboundWriterSample.csIntroduce OutboundWriterSample value type +32/-0

Introduce OutboundWriterSample value type

• Adds a small record struct carrying StartedWrites and HasWorkOutstanding, with rationale for using both fields together to avoid false conclusions when no command was actually queued to the producer.

src/Daqifi.Core/Device/Internal/OutboundWriterSample.cs

TextExchangeEngine.csStamp inbound lines with writer samples and drop pre-send blank terminators +101/-20

Stamp inbound lines with writer samples and drop pre-send blank terminators

• Changes line collection to store CollectedLine(text + writer sample at arrival). Adds logic to treat certain blank lines as provably stale when they arrived before the exchange’s command began writing (or before the setup action finished), and ensures the wait loop and final projection use the same predicate to avoid early-exit races.

src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs

Tests (3) +387 / -2

MessageProducerTests.csAdd tests for StartedWriteCount semantics +91/-0

Add tests for StartedWriteCount semantics

• Adds unit tests asserting StartedWriteCount increments when a write begins (not when it completes) and that failed writes still increment. Introduces a BlockingWriteStream test double to hold a write open and observe in-flight producer state.

src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs

DaqifiDeviceStaleTextLineTests.csAdd regression coverage for queue-to-write stale blank window +295/-2

Add regression coverage for queue-to-write stale blank window

• Adds targeted tests for issue #593: dropping blanks that arrive after queueing but before writing, keeping blanks once writing has begun, and ensuring the wait loop doesn’t exit early when such a blank precedes a real response. Extends the test helper to pass keepBlankLines and timeout parameters, and adds a QueuedWriteTransport that can hold writes and inject inbound lines deterministically.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs

ConnectionGuardTests.csUpdate exchange host test stub for new sampling API +1/-0

Update exchange host test stub for new sampling API

• Extends GuardOnlyTextExchangeHost with SampleOutboundWriter() to satisfy the updated ITextExchangeHost interface in tests.

src/Daqifi.Core.Tests/Device/Internal/ConnectionGuardTests.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. Silent queue-hold timeout ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

HoldableStream/BlockingWriteStream.Write() is meant to park the producer thread until the test calls
ReleaseWrite(), but when the hold limit (10s/30s) elapses it returns silently and allows the
producer to continue even though the write was never intentionally released. This can mask the exact
condition the tests are asserting (a command remaining queued behind a held write), leading to false
passes, non-determinism, and harder-to-diagnose failures.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1201-1207]

+                    while (_holdWrites)
+                    {
+                        var remaining = limit - clock.Elapsed;
+                        if (remaining <= TimeSpan.Zero)
+                        {
+                            return;
+                        }

Relevance

●●● Strong

Recent accepted precedents favor explicit failure handling and bounded test doubles to prevent
silent continuation or hangs.

PR-#430
PR-#547
PR-#578

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In the new implementations, Write() increments the relevant counters and then blocks only while the
internal “hold writes” flag remains set, using a bounded wait (monitor wait) to avoid hanging
indefinitely; however, when that wait reaches its configured timeout, the code exits Write() by
returning rather than signaling failure. Because returning from Write() lets the producer thread
resume even if ReleaseWrite() was never called, the test can proceed without ever proving the write
remained in-flight/blocked, undermining the determinism and assertions these timing-sensitive test
doubles are intended to provide.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1178-1211]
src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs[542-565]

Agent prompt

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

## Issue description
`HoldableStream.Write()` / `BlockingWriteStream.Write()` is intended to park the producer thread inside `Stream.Write()` until the test explicitly calls `ReleaseWrite()`, keeping a narrow “queued behind a held write” timing window deterministic. Today, when the configured hold limit elapses (e.g., 10s/30s), `Write()` returns silently rather than failing, which can unblock the producer without proving the write stayed in-flight and can turn real regressions (missed `ReleaseWrite()`, unexpected scheduling) into false passes or non-deterministic behavior.

## Issue Context
These transports/streams are timing-sensitive test doubles used to make specific concurrency windows deterministic and to validate semantics like a command remaining queued behind a parked write and `StartedWriteCount` behavior while a write is genuinely in-flight.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1178-1211]
- src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs[542-565]

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

---

2. Non-monotonic wait timeouts ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new BlockingWriteStream/HoldableStream wait loops compute deadlines using DateTime.UtcNow, which
is not monotonic and can jump (NTP/VM clock adjustments), causing premature timeouts or unexpectedly
long waits and flaky tests.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1105-1112]

+                var deadline = DateTime.UtcNow + timeout;
+
+                lock (_writeGate)
+                {
+                    while (!_writeStarted)
+                    {
+                        var remaining = deadline - DateTime.UtcNow;
+                        if (remaining <= TimeSpan.Zero)

Relevance

●●● Strong

Recent test-helper precedents accept hardening flaky waits and synchronization; monotonic deadlines
are a straightforward reliability fix.

PR-#403
PR-#565
PR-#547

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code computes timeouts based on wall-clock time (DateTime.UtcNow) in multiple Monitor.Wait
loops; wall-clock changes during execution can invalidate the computed remaining duration and thus
make the waits misbehave.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1103-1118]
src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs[495-510]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1184-1206]

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 test doubles use `DateTime.UtcNow` to compute a deadline and then derive `remaining` time inside `Monitor.Wait(...)` loops. Because `DateTime.UtcNow` can move backwards/forwards (NTP sync, VM clock drift corrections), these loops can time out too early or wait longer than intended, making CI flaky.

### Issue Context
This affects the new monitoring-based synchronization used by:
- `BlockingWriteStream.WaitForWriteStarted(...)` and its blocking `Write(...)` loop
- `QueuedWriteTransport.HoldableStream.WaitForWriteStarted(...)` and its blocking `Write(...)` loop

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs[495-510]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1103-1118]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1184-1206]

### Suggested fix
Replace `DateTime.UtcNow` deadline arithmetic with a monotonic clock source, e.g.:
- `var deadlineMs = Environment.TickCount64 + (long)timeout.TotalMilliseconds;`
- `var remainingMs = deadlineMs - Environment.TickCount64;`
- `Monitor.Wait(gate, TimeSpan.FromMilliseconds(Math.Max(0, remainingMs)))`

Alternatively use `Stopwatch`:
- `var sw = Stopwatch.StartNew();`
- `var remaining = timeout - sw.Elapsed;`

Keep the existing `while(...)` loops to handle spurious wakeups.

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

---

3. Dispose/wait race in stream ✓ Resolved 🐞 Bug ☼ Reliability

Description

HoldableStream.Dispose() disposes ManualResetEventSlim instances while the producer thread may still
be blocked inside _writeRelease.Wait(...), which can throw ObjectDisposedException on the background
writer thread and make tests flaky during teardown.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1169-1172]

+                    // Released before the handles go away, so a writer parked in Write above is
+                    // woken rather than left to fault on a disposed event.
+                    _writeRelease.Set();
+                    _writeStarted.Dispose();

Relevance

●●● Strong

Recent accepted teardown findings favor unblocking background threads and preventing flaky test
cleanup races.

PR-#565
PR-#547
PR-#421

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The producer thread can be blocked in _writeRelease.Wait(...) in Write(). Dispose()
immediately disposes _writeRelease/_writeStarted after calling _writeRelease.Set(), which does
not guarantee the waiting thread has fully returned from Wait(), creating a race that can throw on
the background thread during teardown.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1146-1163]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1165-1173]
PR-#578

Agent prompt

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

### Issue description
`HoldableStream.Write(...)` can block on `_writeRelease.Wait(...)` while writes are held. `HoldableStream.Dispose(...)` calls `_writeRelease.Set()` and then immediately disposes `_writeStarted`/`_writeRelease`, but unblocking is not the same as “the waiting thread has definitely returned from Wait()”. Disposing a `ManualResetEventSlim` while another thread is still inside `Wait()` can surface as `ObjectDisposedException` on the producer/background thread, causing flaky/teardown-dependent test failures.

### Issue Context
This helper is used to deterministically hold the producer thread in `Stream.Write` so tests can target the queue-to-write window. It’s specifically designed to be used in scenarios where a background thread is in-flight during teardown, so dispose must be robust against an in-progress `Wait()`.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1146-1163]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1165-1173]

### Suggested fix approach
- Track whether a writer is currently inside the held wait (e.g., `int _writersWaiting`, increment immediately before `_writeRelease.Wait(...)`, decrement in a `finally`).
- In `Dispose(bool disposing)`, call `_writeRelease.Set()` first, then wait (bounded) for `_writersWaiting` to reach 0 before disposing the events.
 - Example: `SpinWait.SpinUntil(() => Volatile.Read(ref _writersWaiting) == 0, TimeSpan.FromSeconds(1))`.
- Keep the wait bounded so a broken test still fails quickly rather than hanging CI.

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

---

View medium (1)

4. Released writes silently dropped ✓ Resolved 🐞 Bug ≡ Correctness

Description

QueuedWriteTransport.HoldableStream.Write returns immediately when writes aren’t held, discarding
all outbound bytes; this contradicts the remarks that “Writes pass straight through until HoldWrites
is called” and can make the #593 tests pass without ever simulating the command reaching the wire
after ReleaseWrite().

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1123-1127]

+            public override void Write(byte[] buffer, int offset, int count)
+            {
+                if (!_holdWrites)
+                {
+                    return;

Relevance

●●● Strong

Contradicts documented pass-through behavior and undermines the PR's own regression tests for #593.

PR-#591

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new HoldableStream.Write implementation returns without writing any bytes when _holdWrites is
false, while the class-level remarks state that writes should pass through until HoldWrites is
called—meaning the test can’t actually model ‘write eventually hits the wire’ after ReleaseWrite().

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[984-991]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1123-1128]

Agent prompt

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

### Issue description
`QueuedWriteTransport.HoldableStream.Write(...)` currently discards writes when `_holdWrites` is false. The class remarks explicitly claim writes “pass straight through until HoldWrites is called”, but the implementation does not pass them anywhere.

This makes the test double less faithful: after `ReleaseWrite()`, queued commands are no longer blocked (good) but are also never written (bad), so tests that conceptually depend on “command eventually reaches the wire” are not actually exercising that behavior.

### Issue Context
This is test-only code, but it directly affects the meaning/faithfulness of the new stale-blank tests for issue #593.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1123-1135]

### Suggested fix
Implement a minimal “wire sink” for non-held writes instead of returning:
- Option A: back the stream with a `MemoryStream` (or `NullStream`) and write into it when `_holdWrites == false`.
- Option B: at least count/record bytes written so the transport can later assert that a write actually happened.

Also update the remarks if the intended behavior is “drop writes unless held” (but that would be an odd model for a transport).

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

---

5. Wait handles never disposed ✓ Resolved 🐞 Bug ☼ Reliability

Description

QueuedWriteTransport.HoldableStream allocates ManualResetEventSlim instances but never disposes
them, which can leak OS handles across the test run.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1060-1062]

+            private readonly ManualResetEventSlim _writeStarted = new(false);
+            private readonly ManualResetEventSlim _writeRelease = new(true);
+            private byte[]? _current;

Relevance

●●● Strong

Team consistently accepts adding disposal/cleanup for test-fixture resources (events, threads).

PR-#565
PR-#528

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new HoldableStream stores two ManualResetEventSlim instances and only signals _writeRelease on
dispose; there is no corresponding Dispose() call on either event object.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1058-1065]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1137-1145]

Agent prompt

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

### Issue description
`HoldableStream` creates `ManualResetEventSlim` fields (`_writeStarted`, `_writeRelease`) but does not dispose them. In long-running test suites, undisposed wait handles can accumulate and cause resource pressure.

### Issue Context
This is a private test stream. It already has a `Dispose(bool)` override but only sets `_writeRelease`.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1056-1145]

### Suggested fix
- In `HoldableStream.Dispose(bool disposing)`, when `disposing == true`:
 - call `_writeStarted.Dispose()` and `_writeRelease.Dispose()`.
- Optionally: make `QueuedWriteTransport.Dispose()` call `_stream.Dispose()` after releasing the write, so the nested stream’s dispose path runs consistently.

ⓘ 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 turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit a28b81e

Results up to commit 2671d03 ⚖️ Balanced

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

1. Released writes silently dropped ✓ Resolved 🐞 Bug ≡ Correctness

Description

QueuedWriteTransport.HoldableStream.Write returns immediately when writes aren’t held, discarding
all outbound bytes; this contradicts the remarks that “Writes pass straight through until HoldWrites
is called” and can make the #593 tests pass without ever simulating the command reaching the wire
after ReleaseWrite().

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1123-1127]

+            public override void Write(byte[] buffer, int offset, int count)
+            {
+                if (!_holdWrites)
+                {
+                    return;

Relevance

●●● Strong

Contradicts documented pass-through behavior and undermines the PR's own regression tests for #593.

PR-#591

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new HoldableStream.Write implementation returns without writing any bytes when _holdWrites is
false, while the class-level remarks state that writes should pass through until HoldWrites is
called—meaning the test can’t actually model ‘write eventually hits the wire’ after ReleaseWrite().

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[984-991]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1123-1128]

Agent prompt

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

### Issue description
`QueuedWriteTransport.HoldableStream.Write(...)` currently discards writes when `_holdWrites` is false. The class remarks explicitly claim writes “pass straight through until HoldWrites is called”, but the implementation does not pass them anywhere.

This makes the test double less faithful: after `ReleaseWrite()`, queued commands are no longer blocked (good) but are also never written (bad), so tests that conceptually depend on “command eventually reaches the wire” are not actually exercising that behavior.

### Issue Context
This is test-only code, but it directly affects the meaning/faithfulness of the new stale-blank tests for issue #593.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1123-1135]

### Suggested fix
Implement a minimal “wire sink” for non-held writes instead of returning:
- Option A: back the stream with a `MemoryStream` (or `NullStream`) and write into it when `_holdWrites == false`.
- Option B: at least count/record bytes written so the transport can later assert that a write actually happened.

Also update the remarks if the intended behavior is “drop writes unless held” (but that would be an odd model for a transport).

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

---

2. Wait handles never disposed ✓ Resolved 🐞 Bug ☼ Reliability

Description

QueuedWriteTransport.HoldableStream allocates ManualResetEventSlim instances but never disposes
them, which can leak OS handles across the test run.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1060-1062]

+            private readonly ManualResetEventSlim _writeStarted = new(false);
+            private readonly ManualResetEventSlim _writeRelease = new(true);
+            private byte[]? _current;

Relevance

●●● Strong

Team consistently accepts adding disposal/cleanup for test-fixture resources (events, threads).

PR-#565
PR-#528

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new HoldableStream stores two ManualResetEventSlim instances and only signals _writeRelease on
dispose; there is no corresponding Dispose() call on either event object.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1058-1065]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1137-1145]

Agent prompt

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

### Issue description
`HoldableStream` creates `ManualResetEventSlim` fields (`_writeStarted`, `_writeRelease`) but does not dispose them. In long-running test suites, undisposed wait handles can accumulate and cause resource pressure.

### Issue Context
This is a private test stream. It already has a `Dispose(bool)` override but only sets `_writeRelease`.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1056-1145]

### Suggested fix
- In `HoldableStream.Dispose(bool disposing)`, when `disposing == true`:
 - call `_writeStarted.Dispose()` and `_writeRelease.Dispose()`.
- Optionally: make `QueuedWriteTransport.Dispose()` call `_stream.Dispose()` after releasing the write, so the nested stream’s dispose path runs consistently.

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

---

Results up to commit a9f1822 ⚖️ Balanced

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

1. Dispose/wait race in stream ✓ Resolved 🐞 Bug ☼ Reliability

Description

HoldableStream.Dispose() disposes ManualResetEventSlim instances while the producer thread may still
be blocked inside _writeRelease.Wait(...), which can throw ObjectDisposedException on the background
writer thread and make tests flaky during teardown.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1169-1172]

+                    // Released before the handles go away, so a writer parked in Write above is
+                    // woken rather than left to fault on a disposed event.
+                    _writeRelease.Set();
+                    _writeStarted.Dispose();

Relevance

●●● Strong

Recent accepted teardown findings favor unblocking background threads and preventing flaky test
cleanup races.

PR-#565
PR-#547
PR-#421

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The producer thread can be blocked in _writeRelease.Wait(...) in Write(). Dispose()
immediately disposes _writeRelease/_writeStarted after calling _writeRelease.Set(), which does
not guarantee the waiting thread has fully returned from Wait(), creating a race that can throw on
the background thread during teardown.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1146-1163]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1165-1173]
PR-#578

Agent prompt

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

### Issue description
`HoldableStream.Write(...)` can block on `_writeRelease.Wait(...)` while writes are held. `HoldableStream.Dispose(...)` calls `_writeRelease.Set()` and then immediately disposes `_writeStarted`/`_writeRelease`, but unblocking is not the same as “the waiting thread has definitely returned from Wait()”. Disposing a `ManualResetEventSlim` while another thread is still inside `Wait()` can surface as `ObjectDisposedException` on the producer/background thread, causing flaky/teardown-dependent test failures.

### Issue Context
This helper is used to deterministically hold the producer thread in `Stream.Write` so tests can target the queue-to-write window. It’s specifically designed to be used in scenarios where a background thread is in-flight during teardown, so dispose must be robust against an in-progress `Wait()`.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1146-1163]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1165-1173]

### Suggested fix approach
- Track whether a writer is currently inside the held wait (e.g., `int _writersWaiting`, increment immediately before `_writeRelease.Wait(...)`, decrement in a `finally`).
- In `Dispose(bool disposing)`, call `_writeRelease.Set()` first, then wait (bounded) for `_writersWaiting` to reach 0 before disposing the events.
 - Example: `SpinWait.SpinUntil(() => Volatile.Read(ref _writersWaiting) == 0, TimeSpan.FromSeconds(1))`.
- Keep the wait bounded so a broken test still fails quickly rather than hanging CI.

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

---

Results up to commit 013475c ⚖️ Balanced

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

1. Non-monotonic wait timeouts ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new BlockingWriteStream/HoldableStream wait loops compute deadlines using DateTime.UtcNow, which
is not monotonic and can jump (NTP/VM clock adjustments), causing premature timeouts or unexpectedly
long waits and flaky tests.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[R1105-1112]

+                var deadline = DateTime.UtcNow + timeout;
+
+                lock (_writeGate)
+                {
+                    while (!_writeStarted)
+                    {
+                        var remaining = deadline - DateTime.UtcNow;
+                        if (remaining <= TimeSpan.Zero)

Relevance

●●● Strong

Recent test-helper precedents accept hardening flaky waits and synchronization; monotonic deadlines
are a straightforward reliability fix.

PR-#403
PR-#565
PR-#547

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code computes timeouts based on wall-clock time (DateTime.UtcNow) in multiple Monitor.Wait
loops; wall-clock changes during execution can invalidate the computed remaining duration and thus
make the waits misbehave.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1103-1118]
src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs[495-510]
src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1184-1206]

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 test doubles use `DateTime.UtcNow` to compute a deadline and then derive `remaining` time inside `Monitor.Wait(...)` loops. Because `DateTime.UtcNow` can move backwards/forwards (NTP sync, VM clock drift corrections), these loops can time out too early or wait longer than intended, making CI flaky.

### Issue Context
This affects the new monitoring-based synchronization used by:
- `BlockingWriteStream.WaitForWriteStarted(...)` and its blocking `Write(...)` loop
- `QueuedWriteTransport.HoldableStream.WaitForWriteStarted(...)` and its blocking `Write(...)` loop

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs[495-510]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1103-1118]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs[1184-1206]

### Suggested fix
Replace `DateTime.UtcNow` deadline arithmetic with a monotonic clock source, e.g.:
- `var deadlineMs = Environment.TickCount64 + (long)timeout.TotalMilliseconds;`
- `var remainingMs = deadlineMs - Environment.TickCount64;`
- `Monitor.Wait(gate, TimeSpan.FromMilliseconds(Math.Max(0, remainingMs)))`

Alternatively use `Stopwatch`:
- `var sw = Stopwatch.StartNew();`
- `var remaining = timeout - sw.Elapsed;`

Keep the existing `while(...)` loops to handle spurious wakeups.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 013475c

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Qodo-clean, CI green — ready for review.