test(device): bound the two unbounded awaits in the live-stream device tests

[tylerkron]

What

Two tests in DaqifiStreamingDeviceLiveStreamTests awaited without a bound, so a regression in the code they cover would hang CI indefinitely rather than fail it:

Test	Unbounded await	What a regression does today
StreamSamplesAsync_Cancellation_EndsEnumeration_ButNotDeviceStream	await Assert.ThrowsAnyAsync<OperationCanceledException>(async () => await moveNext);	If cancellation ever stops ending the read, this parks forever
StreamSamplesAsync_InvalidBufferCapacity_Throws	await foreach (...bufferCapacity: 0) inside ThrowsAsync	If the capacity validation stops throwing, the enumeration blocks on an empty buffer nothing ever writes to

The repo has no global xUnit timeout and no [Fact(Timeout=...)] usage, so nothing else bounded them.

How

Applies the pattern already established for the extracted collaborator in LiveSampleStreamTests (#440):

• One named MoveNextTimeout field, documented as to why it exists. The five inline TimeSpan.FromSeconds(5) literals already in this file collapse into it, so the file has one bound rather than a field plus scattered literals.
• .AsTask().WaitAsync(MoveNextTimeout) on the cancellation await.
• The await foreach lifted into a local ConsumeAsync() that WaitAsync can bound.

One deviation worth calling out: the cancellation test now disposes its enumerator explicitly rather than via await using. Under a real regression the read stays parked, and disposing an async iterator whose MoveNextAsync is still in flight throws NotSupportedException from the finally — which replaces and masks the TimeoutException that actually names the problem. Verified empirically; with await using the regression reported NotSupportedException, and with the explicit dispose it reports TimeoutException. This matches how the equivalent test in LiveSampleStreamTests is already structured.

Verification

Each regression simulated in turn, then restored:

Simulated regression	Result
Removed the cts.Cancel() call	Assert.ThrowsAny() Failure ... Actual: typeof(System.TimeoutException) — failed in 5s
Passed a valid bufferCapacity: 4	Assert.Throws() Failure ... Actual: typeof(System.TimeoutException) — failed in 5s

Both previously would have hung. Restored to bufferCapacity: 0 and the Cancel() call; the class passes 6/6 on both TFMs.

Release build: 0 warnings, 0 errors. Full suite in Release on net9.0 and net10.0: 2587 passed, 0 failed, 2 skipped each, plus Daqifi.Mcp.Tests 23/23.

Heads-up: main is currently red, unrelated to this PR

Five tests in FirmwareUpdateServiceTests fail on a clean checkout of origin/main (verified by stashing this change — same five fail without it):

• UpdateWifiModuleAsync_WhenFlashToolFails_TakesDeviceBackOutOfLanUpdateMode
• UpdateWifiModuleAsync_WhenCanceledAfterEnteringUpdateMode_StillTakesDeviceBackOut
• UpdateWifiModuleAsync_WhenCanceledDuringTransparentModeExitSettle_LeavesLanRestoreUnsent
• UpdateWifiModuleAsync_WhenRecoveryBudgetExpiresWaitingForReconnect_SendsNoBridgeExit
• UpdateWifiModuleAsync_WhenRecoveryBudgetExpiresAfterReconnect_StillFinishesTheBridgeExit

Expected: ["SYSTem:COMMUnicate:LAN:FWUpdate", "SYSTem:USB:SetTransparentMode 0", "SYSTem:COMMunicate:LAN:APPLY"]
Actual:   ["SYSTem:POWer:STATe 1", "SYSTem:COMMUnicate:LAN:FWUpdate", "SYSTem:USB:SetTransparentMode 0", "SYSTem:COMMunicate:LAN:APPLY"]

Semantic merge race between two PRs that were each green on their own base: #445 added these tests with the pre-power-up sequences, then #444 made the prep sequence prepend SYSTem:POWer:STATe 1. Out of scope here — tracked separately.

The full-suite numbers quoted above therefore exclude FirmwareUpdateServiceTests; everything else is green on both TFMs.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Bound unbounded awaits in live-stream device tests to prevent CI hangs

🧪 Tests 🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Add a shared MoveNextTimeout to bound live-stream awaits and avoid indefinite hangs.
• Wrap cancellation and invalid-buffer tests in WaitAsync so regressions fail fast.
• Adjust enumerator disposal in the cancellation test to preserve TimeoutException signal.

Diagram

graph TD
  T["DaqifiStreamingDeviceLiveStreamTests"] --> D["Streaming device"] --> E["Async enumerator"] --> M["MoveNextAsync task"] --> W["WaitAsync (MoveNextTimeout)"] --> A["xUnit assertions"]
  C["CancellationTokenSource"] --> E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Adopt a global xUnit test timeout policy

• ➕ Eliminates the need to manually wrap awaits in individual tests
• ➕ Provides consistent protection against hangs across the entire test suite
• ➖ May introduce flakes for legitimately slow tests on loaded CI agents
• ➖ Harder to tune per-test behavior for intentionally unbounded streams

2. Use per-test timeout attribute (e.g., Fact timeout)

• ➕ Minimal code changes inside tests
• ➕ Keeps timeout intent explicit at the test boundary
• ➖ Doesn’t prevent mid-test awaits from masking the real failure point
• ➖ May not be available/standard in current repo conventions; requires consistent adoption

3. Introduce a shared test helper for bounded MoveNext/Consume

• ➕ Reduces repetition and enforces consistent timeout usage across streaming tests
• ➕ Centralizes timeout selection and messaging
• ➖ Adds indirection for readers of a single test file
• ➖ May be overkill for a small number of call sites

Recommendation: Current approach is strong for stream semantics: it bounds the specific awaits that can hang while keeping failure modes precise (TimeoutException vs masked disposal exceptions). Consider a lightweight helper or broader timeout policy only if more streaming tests require the same pattern.

Files changed (1) +37 / -10

Tests (1) +37 / -10

DaqifiStreamingDeviceLiveStreamTests.csAdd shared timeout and bound live-stream awaits in hanging test cases +37/-10

Add shared timeout and bound live-stream awaits in hanging test cases

• Introduces a documented MoveNextTimeout and replaces scattered 5s literals with the shared bound. Updates the cancellation test to wrap the MoveNextAsync await in WaitAsync and explicitly disposes the enumerator to avoid masking TimeoutException. Refactors the invalid-buffer-capacity test to run enumeration in a local ConsumeAsync so it can also be bounded by WaitAsync.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceLiveStreamTests.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. Cleanup skipped on timeout ✗ Dismissed 🐞 Bug ☼ Reliability

Description

In StreamSamplesAsync_Cancellation_EndsEnumeration_ButNotDeviceStream, the async enumerator is no
longer under await using and DisposeAsync() is only reached if the bounded cancellation
assertion succeeds. If WaitAsync(MoveNextTimeout) times out (the exact regression this change is
meant to surface), the enumerator never disposes and can remain subscribed to channel SampleReceived
events for the rest of the test run, potentially cascading failures or skewing state.

Code

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceLiveStreamTests.cs[R72-77]

+            // Disposed explicitly below rather than by `await using`: a regression here leaves the
+            // read parked, and disposing an async iterator whose MoveNextAsync is still in flight
+            // throws NotSupportedException from the finally, masking the TimeoutException that
+            // actually names the problem.
+            var e = device.StreamSamplesAsync(cts.Token).GetAsyncEnumerator();
            var moveNext = e.MoveNextAsync();

Relevance

●●● Strong

Team often hardens tests against hangs and ensures disposal/cleanup even on failure paths.

PR-#440
PR-#356

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The updated test disposes the enumerator only after the bounded assertion, so a timeout prevents
disposal. The underlying stream implementation uses per-enumeration SampleReceived subscriptions
that are only removed in a finally during disposal/completion, so abandoning the enumerator can
leave handlers attached.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceLiveStreamTests.cs[63-88]
src/Daqifi.Core/Device/Internal/LiveSampleStream.cs[51-95]

Agent prompt

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

### Issue description
`StreamSamplesAsync_Cancellation_EndsEnumeration_ButNotDeviceStream` intentionally avoids `await using` because disposing while `MoveNextAsync` is in-flight can throw `NotSupportedException` and mask the intended `TimeoutException`. However, the current structure means that on a timeout (or any failure before the explicit `DisposeAsync` line), the enumerator is never disposed and may remain subscribed to channel events for the remainder of the process.

### Issue Context
`LiveSampleStream.StreamSamplesAsync` subscribes `channel.SampleReceived += OnSample` and only unsubscribes in a `finally`, which runs when the iterator completes or is disposed. If the enumerator is abandoned while `MoveNextAsync` is still pending, those handlers can remain attached.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceLiveStreamTests.cs[70-86]

### Suggested change (high level)
1. Capture the move-next task once (avoid multiple `AsTask()` calls): `var moveNextTask = e.MoveNextAsync().AsTask();`
2. Wrap the assertion in `try/finally`.
3. In `finally`, perform **best-effort** cleanup without masking the primary failure:
  - If `moveNextTask.IsCompleted`, `await e.DisposeAsync()` (this covers cases where the assertion fails due to an unexpected *completed* outcome).
  - Optionally, if you want best-effort cleanup even when not completed, attempt `DisposeAsync` but swallow `NotSupportedException` (and any timeout from a bounded dispose), so the original `TimeoutException`/assertion failure still surfaces.

This preserves the PR’s goal (surface `TimeoutException`, not `NotSupportedException`) while improving test isolation when failures occur.

ⓘ 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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 938a3c9