test(device): stop the stale-blank test racing the reader thread on CI

[tylerkron]

What was wrong. Pull requests kept getting kicked out of the merge queue for a failure that had nothing to do with them. A single test — ExecuteTextCommand_DoesNotExitEarlyWhenOnlyAStaleBlankPrecedesTheRealResponse — fails intermittently on the macOS CI job, and because the merge queue re-runs CI on the queued merge commit, whichever PR is merging at that moment gets dequeued. #666, #671, #677 and #678 have all been bounced by it; none of them touch this code. Nothing was ever wrong with the library — the test was racing the machine it ran on.

How it was fixed. The test needs the stale blank to be collected before the exchange marks the send boundary, and it arranged that by sleeping 50 ms in the setup action and hoping the reader thread got there first. On a loaded runner it sometimes didn't: the blank then landed at or past the boundary, stopped counting as a pre-send leftover, and legitimately seeded hasReceivedAny — so the exchange finished on the short 100 ms completion timeout, 50 ms before the test released the real response. The empty result the assertion tripped on was the correct answer to the question the test accidentally asked. The 698 ms duration against a 1000 ms responseTimeoutMs in the #678 run is the tell — it ended early rather than timing out.

Both halves of the setup are now anchored to the exchange instead of the clock:

• The stream double raises a signal when the reader re-enters Read having fully handed over the stale line. The consumer runs one thread through read → parse → raise MessageParsed → read, so coming back for more bytes means the blank is already in collectedLines. The setup action waits on that signal instead of sleeping.
• The response is released from the OnSendBoundaryCaptured seam — provably after the boundary — on a dedicated thread rather than the shared thread pool, which is exactly what gets starved when this suite runs two target frameworks at once. responseTimeoutMs goes to 5000 so a slow runner has room, while the 100 ms completion timeout the test actually turns on is unchanged.

StaleBoundaryHookTestableDevice gains an optional onSendBoundaryCaptured hook to reach that seam; the other four tests using it are untouched.

---

Verified: the starvation was reproduced directly rather than waited for. With the stream double stalled 80 ms on the first read after the blank is released — standing in for a reader thread that loses its slice — the old test fails with exactly the CI signature (Collection: []) and the new test passes. The new test also still fails when the production seed is put back to a raw line-count comparison against staleLineCount, so it has not stopped guarding what it was written to guard. Full dotnet test Daqifi.Core.sln -warnaserror is green on both frameworks: net9.0 4060 passed / 3 skipped, net10.0 4060 passed / 3 skipped, Daqifi.Mcp.Tests 217 passed on each. The test itself ran 25/25 clean, and 20/20 with the machine oversubscribed 2:1. No runtime code changed, so no bench run applies.

closes #687

Not merging — for review.

[qodo-code-review]

PR Summary by Qodo

Stabilize stale-blank exchange regression test

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Replace timing guesses with deterministic stale-consumption and send-boundary synchronization.
• Release the real response on a dedicated thread, avoiding CI thread-pool starvation.
• Widen only the deadlock guard; production exchange behavior remains unchanged.

Diagram

sequenceDiagram
    participant T as Regression Test
    participant E as Text Exchange
    participant D as Device Hooks
    participant S as Two-Stage Stream
    participant R as Reader Thread
    participant C as Release Thread
    E->>D: Capture stale boundary
    D->>S: Release stale blank
    R->>S: Read stale blank
    R->>R: Parse and collect
    R->>S: Re-enter Read
    S-->>T: Signal stale consumed
    T-->>E: Complete setup
    E->>D: Capture send boundary
    D->>C: Start delayed release
    C->>S: Release real response
    R->>S: Read real response
    R-->>E: Collect response
    E-->>T: Return content line

Loading

High-Level Assessment

The event-driven synchronization is the appropriate approach because it anchors both test phases to observable exchange state. Longer sleeps, larger pacing delays, or Task.Delay continuations would retain scheduler-dependent races, while production changes are unnecessary because the failure is isolated to test orchestration.

Files changed (1) +79 / -10

Tests (1) +79 / -10

DaqifiDeviceStaleTextLineTests.csSynchronize stale-blank regression test with exchange boundaries +79/-10

Synchronize stale-blank regression test with exchange boundaries

• Replaces the pre-boundary sleep with a stream signal proving the stale blank has been parsed and collected. Adds an optional send-boundary hook and dedicated response-release thread, with bounded waits and a larger first-response guard to remain reliable under CI starvation.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.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 ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗