test(winc): make the abandoned-open tests deterministic instead of timing-based

[tylerkron]

Why

Two tests in WincFlasherTests fail on CI at random and are currently blocking every open PR in this repo — #427, #428 and #429 all went red on them, and none of those PRs touch WINC code at all. They pass when you run them on your own machine, which is the worst kind of failure: it looks like the PR broke something, and the natural next step is to start changing working code to appease it.

What

The two tests now pass reliably. Nothing about the behaviour they check has changed — this is a test-only fix, no production code is touched.

How

Both tests set up a fake serial port that fails 150 ms after you open it, against an inspector that gives up waiting after 50 ms, and then assert that you get the "gave up waiting" error. That only holds if the 50 ms clock actually fires first, and on a busy CI machine running 2,485 tests it often doesn't — .NET's WaitAsync only reports a timeout if its timer goes off before the task finishes, so a delayed timer means the port's failure gets reported instead, and the test fails on the wrong exception type.

Instead of a 150 ms delay, the fake port now waits for the test to explicitly tell it to fail, and the test only does that after it has already seen the timeout. So the order is guaranteed by construction rather than by hoping one clock beats another. The wait is capped at 30 s so a genuinely broken test reports its own failure instead of hanging.

Evidence

• Failing on CI across unrelated PRs, alternating between the two sibling tests and between net9.0/net10.0 — the signature of a load-dependent race, not a real break:
  • fix(sd): use the device's own timestamp clock when parsing SD logs (closes #426) #429 → Inspector_ObservesTheFaultOfAnAbandonedOpen (net9.0), then Inspector_DisposesTheAbandonedPort_EvenWhenObservingTheFaultThrows (net10.0) on re-run
  • fix(device): stop broadcasting the malformed first stream frame (closes #425) #428 → Inspector_DisposesTheAbandonedPort_EvenWhenObservingTheFaultThrows (net10.0), twice
  • feature/native-winc-flasher, the branch that introduced the tests, failed it on its own branch too
• Same assertion every time: Assert.Throws() Failure: Exception type was not an exact match
• Clean main @ 359bf74 passes locally 5/5 in isolation and 5/5 full-suite — it does not reproduce on an idle machine, which is why it slipped through
• This branch: full suite green 3/3 rounds with both frameworks running concurrently to load the machine — 2,469 passed, 0 failed each round
• Release build clean, 0 warnings

Introduced by #423 (359bf74).

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Make WINC abandoned-open tests deterministic (remove timing race)

🧪 Tests 🕐 10-20 Minutes

AI Description

• Replace timing-based port faults with an explicit, test-controlled fault release.
• Ensure timeout is observed before the abandoned open can fault, eliminating CI flakiness.
• Bound the blocked open to avoid hanging the test runner if the release is missed.

Diagram

graph TD
  T(["WincFlasherTests"]) --> I["WincModuleInspector"] --> W{"WaitAsync(openTimeout)"} --> X{"TimeoutException"} --> T
  T --> S[/"ReleaseFault()"/] --> P["FaultingOnReleasePort"] --> A["Abandoned-open handler"] --> L["Logger + Dispose observed"]
  I --> P
  A --> L

  subgraph Legend
    direction LR
    _t(["Test"]) ~~~ _p["Component"] ~~~ _d{"Decision/Outcome"} ~~~ _s[/"Signal"/]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Relax assertions to accept either timeout or port fault

• ➕ Minimal code changes; avoids synchronization primitives
• ➕ Less coupling to exact scheduling order
• ➖ Weakens the test’s intent (verifying timeout path specifically)
• ➖ Can hide real regressions where timeout handling breaks

2. Use ManualResetEventSlim instead of SemaphoreSlim

• ➕ Simple mental model for one-time gate
• ➕ No accidental multiple-release semantics
• ➖ Still blocks a thread in Open; similar behavior but different primitive
• ➖ SemaphoreSlim is already adequate and explicit about one release

3. Refactor production code to inject a testable clock/timeout strategy

• ➕ Makes timeout logic directly controllable without thread blocking
• ➕ Can reduce reliance on real timers in future tests
• ➖ Touches production code for a test-only flake
• ➖ Higher surface area and review risk than necessary

Recommendation: Keep the PR’s approach: gating the fake port fault behind an explicit release is the smallest change that guarantees ordering by construction, without weakening assertions or touching production code. The added 30s bound also prevents runaway hangs when a test fails before releasing.

Files changed (1) +35 / -7

Tests (1) +35 / -7

WincFlasherTests.csRemove timing race by gating fake port fault on explicit release +35/-7

Remove timing race by gating fake port fault on explicit release

• Updates two abandoned-open tests to use a new fake port that blocks until the test signals it to fault, ensuring the timeout is observed first. Replaces the delay-based fake with a SemaphoreSlim-gated implementation and adds a bounded wait to avoid hanging the suite.

src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.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. Overlong gated-open fallback ✓ Resolved 🐞 Bug ☼ Reliability

Description

FaultingOnReleasePort.Open blocks a thread-pool thread for up to 30 seconds even though the tests
only wait 10 seconds for the abandoned-open fault/disposal, so a failing test can finish while
leaving the abandoned Open task consuming a worker thread for ~20s longer. This degrades CI
reliability under load by increasing thread-pool contention in the exact code path that is
intentionally abandoning worker tasks.

Code

src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs[R653-656]

+            // Bounded so a test that fails before releasing surfaces its own assertion rather than
+            // parking a pool thread for the life of the run.
+            _release.Wait(TimeSpan.FromSeconds(30));
            throw new InvalidOperationException("abandoned-open-fault");

Relevance

●●● Strong

Team often accepts bounding test waits to avoid hangs/threadpool contention (accepted in PR #364;
similar 30s gate-wait flagged in PR #418).

PR-#364
PR-#418
PR-#411

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test helper’s Open() performs a synchronous 30s wait on a semaphore, while the tests only allow
10s for observing the fault and disposal; when OpenWithTimeout times out, it abandons the
Task.Run(Open) worker, so an unreleased gate can keep a worker thread blocked well past the point
the test has already failed.

src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs[651-657]
src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs[348-355]
src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs[376-378]
src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs[198-258]
PR-#364

Agent prompt

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

### Issue description
`FaultingOnReleasePort.Open()` uses `_release.Wait(TimeSpan.FromSeconds(30))` and ignores the return value. If the test fails before calling `ReleaseFault()`, the test’s own `WaitAsync(TimeSpan.FromSeconds(10))` will fail first, but the abandoned `Open()` task can still block a thread-pool thread for up to 30s.

### Issue Context
This helper is only used in tests, but it is specifically modeling the production scenario where `WincModuleInspector.OpenWithTimeout` abandons an `Open()` task running on the thread pool. Keeping that abandoned task blocked longer than the test’s timeout budget can add suite-level contention.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs[651-657]

### Proposed fix
- Change `Open()` to check the boolean result of `_release.Wait(...)`.
- Reduce the maximum wait to be at or below the test’s own 10s waits (e.g., 5–10s).
- If the wait times out, throw a distinct exception/message like `InvalidOperationException("Test bug: ReleaseFault was not called")` so failures are diagnosed immediately and the blocked thread is released sooner.

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[tylerkron]

/agentic_review