fix(tests): stop the serial-probe teardown tests racing a 1s response window

[tylerkron]

Produced by the FLAKY-TEST-FIXER maintenance routine. Test-only change: no production code is touched and no assertion was weakened, so the only thing it can affect is how reliably the existing assertions get a chance to run.

What was wrong. SerialProbeTeardownTests.RequestDeviceStatusAsync_WhenItReturns_ReaderThreadHasExited failed on CI on net10.0 while the same commit passed on net9.0 (run 31724429734, merge-queue run for #514) with Assert.NotNull() Failure: Value is null — the probe returned "no device" instead of the status it was scripted to receive. The cause is a wall-clock race, not a bug in the probe. SerialDeviceFinder.RequestDeviceStatusAsync gives a device a fixed 1000 ms (ResponseTimeoutMs) to answer, and that clock starts the moment the test calls it. The test then let the device answer only after await stream.WaitForFirstReadAsync(), whose body was await Task.Run(() => _firstRead.Wait(token)) — two thread-pool queue hops (one to dispatch the Task.Run body, one to resume the awaiting test method) spent inside that 1000 ms budget. CI runs both target frameworks' test processes at once, each running xunit collections in parallel; when the pool is saturated it injects new workers only about twice a second, so those two hops can easily outlast the window. The probe gave up and returned null before the test ever got to call RespondWithStatus(). Two sibling tests in the same file used the same helper and carried the same latent race.

How it was fixed. WaitForFirstReadAsync becomes a plain synchronous WaitForFirstRead() that waits the ManualResetEventSlim directly on the calling thread. That is what removes the race rather than hiding it: the thing being waited for is the consumer's reader, which StreamMessageConsumer runs on a dedicated Thread rather than a pool work item, so it reaches its first read regardless of pool pressure — while the old helper needed the pool to schedule two continuations before the test was allowed to answer. Nothing about the scenario changes: the device still answers while the reader is genuinely parked in Read(), the ContinueWith is still attached before any answer can arrive (so it still samples reader-liveness at completion rather than running vacuously inline), and every assertion is byte-for-byte the same. Widening a timeout or relaxing Assert.NotNull would have been the hiding fix; this deletes the pool dependency instead. The Wait is still bounded at 5 s and now asserts on expiry, so a genuine regression that stops the reader from reading fails loudly instead of hanging.

Verified: full suite green on both target frameworks locally (3730 passed / 0 failed on net9.0 and net10.0, plus Daqifi.Mcp.Tests 217/0), build clean with 0 warnings, and the touched class passes 11/11 on both.

Not merging — opened for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Deflake SerialProbeTeardownTests by making first-read wait synchronous

🧪 Tests 🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Remove thread-pool dependent async gating before scripted probe responses
• Block synchronously until the consumer enters its first Read() to avoid 1s timeout races
• Fail fast with an assertion if the consumer never reaches its first read

Diagram

graph TD
  A["SerialProbeTeardownTests"] --> B["SerialDeviceFinder.RequestDeviceStatusAsync"] --> C["Scripted probe stream"] --> D["StreamMessageConsumer reader thread"] --> E["WaitForFirstRead()"] --> F["RespondWithStatus()"]
  F --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Increase the probe response timeout in tests

• ➕ Minimal code change
• ➕ Avoids needing to reason about thread scheduling details
• ➖ Hides the underlying scheduling race instead of removing it
• ➖ Slows test feedback and can still be flaky under heavier load

2. Keep async API but use a wait primitive without Task.Run hops

• ➕ Preserves an async-shaped helper method
• ➕ Can avoid blocking the test thread if implemented carefully
• ➖ More complexity than needed here; still risks introducing scheduling/continuation timing issues
• ➖ Harder to guarantee the wait doesn't consume the 1s response budget

Recommendation: The chosen approach (make the gate synchronous and assert-bounded) is the best fit: it removes thread-pool scheduling from the fixed 1s response window rather than stretching the window, while keeping the test semantics intact (response is still sent only after the consumer is genuinely in its first Read()).

Files changed (1) +24 / -6

Tests (1) +24 / -6

SerialProbeTeardownTests.csMake first-read gate synchronous to avoid 1s response-timeout races +24/-6

Make first-read gate synchronous to avoid 1s response-timeout races

• Replaces Await/Task.Run-based WaitForFirstReadAsync with a synchronous WaitForFirstRead that directly waits on the ManualResetEventSlim. Updates three tests to use the synchronous gate and adds a bounded wait with an assertion to fail loudly if the consumer never enters its first read, along with detailed rationale in XML docs.

src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.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 turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗