fix(device): reject a stale blank line in a text exchange's capture-to-send window

[tylerkron]

Summary

• TextExchangeEngine.ExecuteAsync's stale-line boundary was captured before setupActionAsync ran, so a late reply to an earlier exchange landing in the sub-millisecond gap between the capture and the send completing was still counted as this exchange's answer.
• Content lines keep the existing (wider) boundary unchanged — narrowing it risks discarding a genuinely fast reply, and whether that race is even reachable on real hardware is a bench question, not one this PR settles.
• A blank line landing in that gap is now dropped: a device cannot emit a terminator for a command it hasn't been sent yet, so a blank there is necessarily a leftover from an earlier exchange. This matters most for SYSTem:LOG? (added in fix(diagnostics): tell an empty system log apart from a device that never answered #552), which reads "any line arrived" as "the device answered" — a stale blank there would otherwise report an empty log for a device that had gone silent.
• Adds a narrow test-only seam, ITextExchangeHost.OnStaleLineBoundaryCaptured (a no-op on the real device), fired at the boundary capture point, so a test double can release a line into the transport at that exact instant. The previous attempt at this fix (recorded on TextExchangeEngine's stale-line boundary is captured before the send, so a late reply can still be attributed to the wrong exchange #553) was reverted because the existing access-counted mock transport can't reach that window — both of its hookable Stream accesses land before the boundary is even captured.

Test plan

• New regression test (ExecuteTextCommand_DropsABlankThatArrivesWhileTheCommandIsStillBeingSent) fails without the fix (verified by reverting the engine change locally and re-running: exactly 1 failure) and passes with it.
• Complement test (ExecuteTextCommand_KeepsAContentLineThatArrivesWhileTheCommandIsStillBeingSent) documents that content lines are deliberately left alone.
• Full Daqifi.Core.Tests suite: 3726 passed, 0 failed.
• Bench-verified against a real DAQiFi Nyquist over serial: SYSTem:LOG? (x3) and SYSTem:LOG:CLEar still round-trip correctly through the changed engine.

Fixes #553.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Drop stale blank lines that arrive between boundary capture and command send

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Adds a test-only hook to target the sub-millisecond capture-to-send race window.
• Drops blank lines that arrive before the command has finished sending to avoid misattribution.
• Adds regression tests covering blank-line drop and content-line preservation in that window.

Diagram

graph TD
  A["DaqifiDevice (ITextExchangeHost)"] --> B["TextExchangeEngine.ExecuteAsync"] --> C["Capture stale boundary"] --> D["Host hook: OnStaleLineBoundaryCaptured"] --> E["Run setupActionAsync (send)"] --> F["Capture sent boundary"] --> G["Filter: drop pre-send blank lines"] --> H["Return response lines"]
  E --> I["IStreamTransport (wire)"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Move the stale boundary capture to after setupActionAsync

• ➕ Simpler mental model: one boundary instead of two
• ➕ Avoids needing an index-aware filter on collected lines
• ➖ Risks discarding legitimately fast replies that arrive during command send
• ➖ Changes semantics for content lines (the PR explicitly preserves the wider boundary for content)

2. Timestamp/sequence-tag incoming lines and attribute by send-time

• ➕ More robust attribution than count-based boundaries under concurrency
• ➕ Scales better if more race windows appear elsewhere
• ➖ Higher implementation complexity and pervasive changes to collection pipeline
• ➖ Adds runtime overhead and potential ordering/timing assumptions

3. Treat blank lines as non-semantic unless explicitly expected

• ➕ Prevents this class of blank-line misattribution broadly
• ➕ Could simplify callers like SYSTem:LOG? that interpret "any line" as "answered"
• ➖ Behavioral change: some callers intentionally want to see terminator blanks (keepBlankLines)
• ➖ May hide protocol/firmware quirks that tests expect to observe

Recommendation: Keep the PR’s targeted two-boundary approach: it fixes the demonstrated misattribution (blank terminator cannot precede a send) while intentionally not narrowing the boundary for content lines, preserving existing behavior for potentially fast replies. The added hook is narrowly scoped (no-op in production) and enables a deterministic regression test for a sub-millisecond window that prior seams could not reach.

Files changed (5) +128 / -1

Bug fix (3) +50 / -1

DaqifiDevice.csAdd overridable OnStaleLineBoundaryCaptured no-op seam for tests +11/-0

Add overridable OnStaleLineBoundaryCaptured no-op seam for tests

• Implements ITextExchangeHost.OnStaleLineBoundaryCaptured and forwards to a new internal virtual method. The default implementation is a no-op on real devices, enabling test subclasses to inject behavior at boundary capture time without affecting production behavior.

src/Daqifi.Core/Device/DaqifiDevice.cs

ITextExchangeHost.csIntroduce OnStaleLineBoundaryCaptured hook to target race window in tests +9/-0

Introduce OnStaleLineBoundaryCaptured hook to target race window in tests

• Adds a new host callback invoked immediately after stale-line boundary capture and before setupActionAsync runs. Documented as a test-only seam to deterministically exercise the sub-millisecond capture-to-send window described in issue #553.

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

TextExchangeEngine.csCapture post-send boundary and drop pre-send blank lines from results +30/-1

Capture post-send boundary and drop pre-send blank lines from results

• Adds a second boundary captured after setupActionAsync completes sending, and filters out blank lines that arrived at/before that boundary while leaving content lines governed by the original stale boundary. Invokes the new host hook at boundary capture time to support deterministic tests for the race window.

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

Tests (2) +78 / -0

DaqifiDeviceStaleTextLineTests.csAdd capture-to-send window regression tests and a hookable test device +77/-0

Add capture-to-send window regression tests and a hookable test device

• Adds two new tests to validate behavior when a line arrives after stale-boundary capture but before the command finishes sending: blanks are dropped while content lines are retained. Introduces a private test-only DaqifiDevice subclass overriding OnStaleLineBoundaryCaptured to release inbound data at the precise boundary capture instant.

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

ConnectionGuardTests.csUpdate test host stub for new ITextExchangeHost hook +1/-0

Update test host stub for new ITextExchangeHost hook

• Extends the GuardOnlyTextExchangeHost stub to implement the newly added OnStaleLineBoundaryCaptured method (throwing NotSupportedException), keeping the test suite compiling and consistent with the interface contract.

src/Daqifi.Core.Tests/Device/Internal/ConnectionGuardTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. Boundary captured before send 🐞 Bug ≡ Correctness

Description

TextExchangeEngine captures sentBoundaryLineCount immediately after setupActionAsync returns, but
setup actions typically only enqueue SCPI writes; the producer can still be writing afterward, so a
stale blank arriving after this capture but before the command hits the wire will still be kept and
misattributed to the current exchange.

Code

src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[R484-486]

+                    // reply, a real device having answered before setupActionAsync returns being a
+                    // bench question rather than one this fix can settle.
+                    sentBoundaryLineCount = CollectedLineCount();

Relevance

● Weak

Exact prior stale-line race finding was rejected; this PR intentionally limits filtering to blanks
while preserving content lines.

PR-#400
PR-#542

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The engine’s new boundary is captured right after setupActionAsync completes, but Send<T> and
MessageProducer<T>.Send both document/implement enqueueing that returns before the write.
Therefore, the command may still be unsent when sentBoundaryLineCount is captured, leaving a
remaining window where stale blanks can arrive and be incorrectly kept by the new filter.

src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[468-487]
src/Daqifi.Core/Device/DaqifiDevice.cs[1581-1589]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[184-207]
src/Daqifi.Core/Device/Internal/OperationSerializer.cs[612-647]

Agent prompt

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

### Issue description
`sentBoundaryLineCount` is intended to represent “lines collected by the time the setup action finished sending.” Today it’s captured immediately after `await setupActionAsync(...)`, but `setupActionAsync` commonly calls `DaqifiDevice.Send`, which enqueues to `MessageProducer` and returns before the stream write occurs. This leaves a remaining “setup-return-to-wire” gap where stale blank lines can still arrive and be incorrectly preserved.

### Issue Context
- `DaqifiDevice.Send<T>` explicitly documents that it returns before the write happens.
- `MessageProducer<T>.Send` enqueues to a background thread.
- The engine already has `_host.DrainOutboundQueueAsync(...)` (used before the swap) which waits on `producer.IsIdle` specifically to cover the “queue empty but still writing” gap.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[468-487]
- src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[570-590]

### Proposed fix
1. After `await setupActionAsync(cancellationToken)` and **before** capturing `sentBoundaryLineCount`, await an outbound-drain barrier (or a new narrower one if you don’t want to reuse the full `OutboundDrainWait`). For example:
  - `await _host.DrainOutboundQueueAsync(cancellationToken).ConfigureAwait(false);`
  - then `sentBoundaryLineCount = CollectedLineCount();`
2. Update the comment above `sentBoundaryLineCount` to say it’s captured after outbound drain / write completion, not merely after setup returns.
3. Consider adding (or adjusting) a regression test that models “setupAction returns immediately but producer write is delayed” to ensure the stale blank is dropped in that more realistic window (the current test only covers a slow setup action, not a slow send).

ⓘ 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 ↗

[tylerkron]

Re: "Boundary captured before send" (Qodo review round 1, finding 1, Low/Weak) — investigated, and deliberately not adopting the suggested fix. Explained in 5913940.

The point is correct: setupActionAsync returning only means Send() enqueued the command to MessageProducer, not that its background thread has written it. I implemented the suggested fix — reusing DrainOutboundQueueAsync (the same bounded IsIdle wait already used before the consumer swap) right after setupActionAsync, before capturing the boundary — and added a regression test for it using a gated-write transport double.

Bench-tested it against a real Nq1 before deciding, since this is the most order-sensitive code in the device. Isolated by toggling only that one call with debug logging on:

• With the drain: 9/9 runs slow, 2.6-3.1s round trip on SYSTem:LOG?.
• Without it: 6/6 runs fast, 0.6-1.1s.
• The drain call itself measured under 15ms every time (confirmed via sw.ElapsedMilliseconds either side of it) — the ~2s delay showed up entirely after it returned, in the device's actual reply.

So the extra wait itself isn't slow; something about issuing it causes the device to answer roughly two seconds later, consistently and reproducibly. I couldn't isolate the root cause within the scope of this fix. Trading a measured ~2.5x latency regression on every text exchange for a theoretical, Low-severity race that the finding itself flags as weak relevance is the wrong trade, so I reverted the drain and the test for it, keeping only a comment recording the investigation for whoever revisits this.

Not resolving this thread — it's a comment, not an inline review thread — but flagging that it's addressed via the commit above.

[tylerkron]

/agentic_review

[qodo-code-review]

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