test: stop the wall-clock-tight unit tests racing a stalled CI runner

[tylerkron]

Two unrelated tests were failing PRs that had nothing to do with either of them. Both are pre-existing flakes on main — both fail on net9.0 while net10.0 passes the same test in the same run, which is the signature of a starved runner rather than a regression.

1. Post-reconnect readiness probe (hit #452)

System.TimeoutException : Device did not become application-ready within 00:00:01 (probes executed: 2).

UpdateFirmwareAsync_PostReconnectReadinessProbe_AwaitedBeforeComplete and UpdateFirmwareAsync_PostReconnectProbeThrowsOwnOCE_RetriesAndCompletes configure a probe that reports not-ready twice then ready, and assert probeCallCount == 3 with CurrentState == Complete. Neither assertion is about how long readiness takes — the budget only exists so a hung probe can't hang the suite. A 1s budget for a ~30ms need was not enough margin: the runner got 2 of the 3 probes away inside the second and the timeout fired one probe short.

Budgets raised to 10s readiness / 15s JumpingToApplicationTimeout (both must move together — Validate requires readiness to stay strictly below the outer state timeout whenever a probe is set).

2. Hung-port discovery sweeps (hit #453)

DiscoverAsync_HungPort_TimesOutAndStillReturnsHealthyDevices
Assert.Single() Failure: The collection was empty

#453 touches the live-stream device tests and nothing in discovery.

Every probe — healthy ones included — is dispatched through Task.Run, so it must be handed a thread-pool thread before it can complete. That handoff races Task.Delay(PortProbeHardTimeoutMs), and SerialDeviceFinder abandons a probe still waiting for a thread when the ceiling expires. That is precisely an empty result for COM_OK. Under a saturated pool, thread injection is throttled to roughly one new thread per second, so a 300ms ceiling can expire before the delegate ever starts.

The file already half-knew this. The cross-sweep tests were relaxed to hungProbeCalls <= 1 with the comment "under thread-pool contention the hung probe may not have STARTED before the first sweep's hard timeout fires" — but the healthy-device assertions were left racing the same clock.

Ceiling raised to 2000ms on the four sites that assert a healthy device is reported, matching what DiscoverAsync_HungPort_HealthyDeviceEventFiresBeforeSweepSettles already used. DiscoverAsync_QuarantineTtl_AllowsPeriodicRetry keeps its 100ms — it only counts probes of a wedged port and depends on the short window.

CreateFinderWithProbes now documents the constraint so the next test picks a value deliberately rather than picking the smallest number that passed locally.

Scope

Tests only — no production code changed. The abandon-on-timeout behaviour in SerialDeviceFinder is correct and deliberate (#294); the tests were just holding it to a stopwatch.

Verification

• Full solution green on net9.0 and net10.0 (2703 passed each, 0 failed).
• Suite cost: about +7s, from the raised discovery ceilings. The readiness changes cost nothing — that probe still succeeds after two 10ms delays.

Not reproduced locally. A 12-core dev box does not starve the .NET thread pool the way a 2-core runner running 2700 parallel tests does; the unfixed discovery test still passes here under 6x CPU oversubscription. The diagnosis rests on the failure signature (an empty collection means the probe never completed), the abandon-on-timeout code path, and the net9.0/net10.0 split — not on a local repro.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Deflake post-reconnect readiness-probe tests by increasing timeout headroom

🧪 Tests 🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Increase readiness-probe time budgets to avoid CI wall-clock starvation flakes.
• Keep behavioural assertions unchanged (probe polled until ready; completion awaits readiness).
• Preserve options validation invariant by raising the outer state timeout alongside readiness
 timeout.

Diagram

graph TD
  T["FirmwareUpdateServiceTests.cs"] --> S(["FirmwareUpdateService"]) --> O[["FirmwareUpdateServiceOptions"]] --> P(["Post-reconnect readiness probe"]) --> C["State: Complete"]
  O --> R["Readiness timeout: 30s"]
  O --> J["Jumping-to-app timeout: 60s"]
  subgraph Legend
    direction LR
    _file["Test file"] ~~~ _svc(["Service"]) ~~~ _cfg[["Options/config"]]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use a fake/virtual clock for probe polling

• ➕ Eliminates wall-clock dependence entirely; deterministic under runner starvation
• ➕ Keeps time-based assertions fast without large real timeouts
• ➖ Likely requires production-code seams (injectable clock/scheduler) and broader refactor
• ➖ Higher implementation and maintenance cost than a test-only change

2. Mock the readiness polling loop (assert call counts without real delays)

• ➕ Avoids real timing, reduces flake risk further
• ➕ Very fast unit tests
• ➖ Can drift from actual integration behaviour of retries/timeouts
• ➖ May require additional abstractions around polling/retry mechanism

Recommendation: The PR’s approach is appropriate: it fixes a CI-only wall-clock race with minimal, test-only changes while preserving the behavioural assertions. Alternatives like injecting a virtual clock would be more robust but require wider design changes that are not warranted for this failure mode.

Files changed (1) +19 / -4

Tests (1) +19 / -4

FirmwareUpdateServiceTests.csIncrease readiness and outer state timeouts for success-path probe tests +19/-4

Increase readiness and outer state timeouts for success-path probe tests

• Raises PostReconnectReadinessTimeout from 1s to 30s in two success-path tests and also increases JumpingToApplicationTimeout to 60s to satisfy options validation. Expands comments to document that the assertions are behavioural and the budgets exist only to prevent hangs and CI starvation flakes.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.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. Slow test failure latency ✓ Resolved 🐞 Bug ☼ Reliability

Description

The two modified readiness-probe tests raise JumpingToApplicationTimeout from CreateFastOptions()’s
2s to 60s, so a regression in the JumpingToApp state will now take up to a minute to fail and
report. This doesn’t create an unbounded hang, but it materially increases worst-case CI time and
delays diagnosis compared to the prior fast-fail configuration.

Code

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[R2915-2916]

+        options.JumpingToApplicationTimeout = TimeSpan.FromSeconds(60);
+        options.PostReconnectReadinessTimeout = TimeSpan.FromSeconds(30);

Relevance

●● Moderate

Team often increases/relaxes test time bounds to reduce CI flakiness; may accept slower worst-case
failures as tradeoff.

PR-#226
PR-#430

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR changes set JumpingToApplicationTimeout=60s in both tests; CreateFastOptions configures
JumpingToApplicationTimeout=2s for unit test speed, and JumpingToApplicationTimeout is the
configured timeout for the JumpingToApp state. Therefore regressions in JumpingToApp that previously
failed quickly under fast options can now take up to 60s to surface in these tests.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[2871-2940]
src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[2995-3056]
src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[4816-4852]
src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs[364-379]

Agent prompt

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

### Issue description
Two tests override `CreateFastOptions()` by setting `options.JumpingToApplicationTimeout = 60s`. Since `FirmwareUpdateServiceOptions.GetStateTimeout(FirmwareUpdateState.JumpingToApp)` uses this value, any regression/hang in that state now takes up to 60s to fail, significantly slowing CI feedback.

### Issue Context
The increased timeouts were intentionally added to deflake wall-clock races. The goal is to keep generous *readiness* headroom while avoiding a much larger-than-necessary outer state timeout.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[2915-2916]
- src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[3031-3033]

### Suggested change
- Keep `PostReconnectReadinessTimeout` at 30s, but reduce `JumpingToApplicationTimeout` to a smaller margin above it (e.g., `PostReconnectReadinessTimeout + TimeSpan.FromSeconds(5)`), so the test remains robust to runner stalls but fails faster on genuine regressions.
- (Optional) Add an explicit per-test timeout if the project wants a hard cap on overall test duration.

ⓘ 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.}