fix(sdcard): bound the SD download path so a wedged card cannot hang or ignore cancellation (closes #399)

[tylerkron]

Summary

An SD download against a device whose SD subsystem is wedged hung indefinitely and ignored cancellation, and no consumer-side watchdog could rescue it. This bounds the host side of that path from the outside in, so the caller is released no matter which inner call is parked.

1. SerialStreamTransport — bound writes, not just reads. Only ReadTimeout was lowered after open; WriteTimeout kept the caller's ConnectionRetryOptions.ConnectionTimeout for the life of the port. Both are now set to operational values after open (read 500 ms unchanged, write 2000 ms) via ApplyOperationalTimeouts.

Correction to the issue's framing: the write was not unbounded before — it inherited the connect timeout, which is 3 s / 5 s / 10 s for the Fast / default / Resilient presets. So a parked write alone cannot explain an 11-minute hang unless a consumer configured a very large ConnectionTimeout. Treat this as hardening (decoupling the write bound from retry configuration), not as the fix.

2. DownloadSdCardFileAsync — a deadline it enforces itself. The transfer now runs on a LongRunning worker task raced against both the deadline and the caller's token, so neither depends on the parked code observing anything. This is the actual fix. Details:

• The worker matters as much as the race: ExecuteRawCaptureAsync's synchronous prefix (consumer stop-and-join, PrepareSdInterface's writes) otherwise runs on the calling thread before the first await — outside any deadline, and on a UI thread it freezes the window. There's a test for exactly that.
• On expiry the transfer is abandoned, not awaited (its token is cancelled first, so it unwinds at its next token check — which it only reaches if whatever it is blocked in returns). Pattern and its review-hardened details mirror WifiBridgeActivator.RunWithHardTimeoutAsync (fix: SerialDeviceFinder.DiscoverAsync blocks the calling thread before its first await (sync SerialPort.Open, no per-port timeout) #294/fix(discovery): make serial probing immune to hung ports (closes #294) #295/fix(firmware): isolate WifiBridgeActivator against uncancellable SerialPort.Open() hangs #326).
• Retries now draw from the remaining budget instead of getting a fresh 30 minutes each, so the total can't exceed the deadline the caller was promised.

3. SdCardFileReceiver — honor the token that is accepted. The loop now checks its token every iteration. SerialStream ignores the token once a read is in flight and, on a silent device, just returns 0 bytes when its ReadTimeout elapses — which the loop reported as "transport stream closed before receiving the EOF marker", i.e. a timeout for a transfer the caller had already cancelled. That is the case where a consumer's stall watchdog most wants its own cancellation back.

4. One SD download at a time (added during review). Because a timed-out transfer is abandoned rather than stopped, nothing stopped a caller from starting another one — and an abandoned transfer still owns the transport stream. A retry against a device that stays wedged (an "import all" loop over 30 files) would put a second concurrent reader on that stream — the framing corruption RestartMessageConsumerAfterSwap already refuses to risk — and stack another blocked thread each time. A per-device gate now admits one download at a time and is released only when the worker genuinely finishes, however long after the caller gave up; a retry before then fails fast with InvalidOperationException, and cancellation outranks that refusal.

What I fixed vs. what remains inferred

Fixed (verified): writes bounded independently of connect configuration; the download bounded by its own deadline including its synchronous prefix; caller cancellation able to end the call regardless of what the transfer is parked in; cancellation no longer misreported as a timeout on the zero-byte read path.

Still inferred: which call actually parked for 11 minutes in the original report. I could not confirm it, and two of the issue's own candidates got weaker on inspection:

• The write bound was already 3–10 s (above), and
• Send() on the SD path is queued — DaqifiDevice.Send hands string messages to MessageProducer, whose background thread does the blocking write. A wedged write therefore parks the producer thread, not the download; the download then waits for a reply to a SD:GET that never left the queue. That fits the reported evidence (0 bytes, no progress callbacks) better than a parked caller. Note MessageProducer logs and swallows write failures, so a timed-out SD:GET write is silent — the download is now bounded either way, but the command is quietly lost.

Because the park point is unconfirmed, the bounding is deliberately at the outside of the path rather than at any one suspect.

Bench verification (Nq1 on /dev/cu.usbmodem1101, non-destructive)

The board turned out to be in the wedged state, which was useful and also limiting:

• Connect + InitializeAsync: 32 channels populated. SD LIST: 31 files, run before and after the download tests — the device was still healthy afterwards. This exercises the new 2 s WriteTimeout on all normal command traffic.
• The wedge reproduced exactly: SD:GET on two different files produced a 0-byte destination and zero progress callbacks — matching the issue's report (device-side firmware SD:GET silently returns no data after any non-SD stream — SD circular buffer collapses 32 KB→512 B daqifi-nyquist-firmware#703/#567).
• Watchdog cancel: with a 10 s (and 8 s) CancellationTokenSource passed into DownloadSdCardFileAsync, the call returned OperationCanceledException at exactly 10.0 s / 8.0 s.
• A/B against origin/main: I rebuilt the same harness against pre-fix core and ran it on the same wedged file — it also cancelled at 10.0 s. So on macOS the token was already being observed; the inert-cancellation symptom is specific to the reporter's Windows serial stack (SerialStream.Windows differs from the Unix implementation here). The bench proves no regression, not a cure. The parked-path behavior this PR fixes is covered by unit tests only.
• Not validated on hardware: the "slow but healthy transfer is not falsely aborted" case — the board serves no SD:GET data at all right now, so there was no healthy transfer to run. Unit test covers it.

Tests

dotnet test green on net9.0 and net10.0 (1941 passed each), 0 warnings. New tests, each verified to fail with its corresponding fix disabled:

• deadline fires on a transfer that parks ignoring its token;
• deadline still fires when the park is synchronous, before the first await;
• caller cancellation ends the call while parked, well before the (distant) deadline;
• a slow-but-healthy transfer (240 B in 16 B chunks, 40 ms apart) completes untouched — the "must not falsely abort" case;
• a retry while a previous transfer is still abandoned is refused, and downloads resume once that worker unwinds;
• cancellation outranks the gate refusal (the test cancels from inside the pre-flight send, landing in the window between the entry guard and the gate check);
• plus a receiver test that a cancelled transfer on a token-ignoring silent stream reports cancellation rather than a timeout, and a transport test that both directions end up bounded.

Known consequences (documented on the method and ISdCardOperations)

• An abandoned transfer can still write to destinationStream after the method throws, so callers must not reuse that stream.
• The device is left mid-SD:GET with the protobuf consumer stopped; reconnect (or power-cycle a genuinely wedged card) before retrying.
• The pre-flight StopStreaming send stays on the caller's thread — it is a queued write, so it cannot block.
• A device left holding the gate by a transfer that never unwinds will refuse further downloads until it is reconnected. That is deliberate: the alternative is corrupting the stream.

Review

Qodo raised three findings across four rounds, all fixed and confirmed resolved: abandoned downloads could stack (→ the gate above), cancellation could be masked by that gate, and a wall-clock assertion in one test was flake-prone. Details in the thread replies.

Sibling PRs touching adjacent code

• api: retire four downstream workarounds — SD download failure typing, TimestampProcessor reset semantics, FirmwareUpdateException Verifying overload #398 owns exception typing inside SdCardFileReceiver (stall / empty-transfer classification). I stayed on hang/timeout/cancellation bounding and did not retype anything; textual conflicts in that file are expected.
• fix(transport): a physically unplugged serial device never reports the drop, so ConnectionStatus.Lost is unreachable over USB #382 also touches SerialStreamTransport (disconnect detection). My change there is limited to timeout bounding after open.

closes #399

Not merging — opened for review.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Bound SD card downloads with hard deadline, cancellation wins, and bounded serial writes

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Bound serial transport write timeouts after open to prevent stalled command writes
• Enforce SD download wall-clock deadline independent of cooperative cancellation (#399)
• Ensure SD receiver reports cancellation (not timeout) when reads ignore the token

Diagram

graph TD
  A(["Caller"]) --> B["DownloadSdCardFileAsync"] --> C{{"Hard deadline race"}} --> D[["LongRunning worker"]]
  D --> E["ExecuteRawCaptureAsync"] --> F["SerialStreamTransport"] --> G[("SerialPort")]
  E --> H["SdCardFileReceiver"] --> I[("Destination Stream")]

  subgraph Legend
    direction LR
    _act(["Caller"]) ~~~ _proc["Operation"] ~~~ _dec{{"Deadline/decision"}} ~~~ _task[["Worker thread"]] ~~~ _io[("I/O/Stream")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Close/Dispose serial port to break native I/O on timeout

• ➕ Actively interrupts blocked native reads/writes instead of abandoning a background worker
• ➕ Avoids continued writes to destination stream after the API has failed
• ➖ Can be more disruptive: affects other in-flight operations and device state
• ➖ Harder to make safe across transports and retry logic; increases coupling to connection lifecycle

2. Make the entire SD transfer path fully cooperative (token-aware, async-only)

• ➕ Simpler mental model: no abandoned tasks; cancellation propagates naturally
• ➕ Easier resource cleanup guarantees
• ➖ Not fully possible with System.IO.Ports: SerialPort.Write has no CancellationToken and synchronous prefixes can still block
• ➖ Requires broader refactors across capture/consumer stop-join paths

3. Rely only on shorter operational timeouts (read/write) without hard deadline race

• ➕ Less concurrency complexity; no abandoned worker
• ➕ Keeps single control flow
• ➖ Still vulnerable to hangs outside timeout-controlled calls (e.g., synchronous prefixes)
• ➖ Timeout tuning becomes fragile and device/driver-dependent; may not reliably honor caller cancellation

Recommendation: Keep the PR’s approach: a hard deadline enforced outside the parked transfer is the only reliable way to guarantee the caller is released when inner calls can’t observe cancellation. The added operational write timeout is good hardening, but the key value is moving the synchronous prefix and I/O onto a LongRunning worker and racing it against deadline/caller cancellation, with explicit documentation of the abandon semantics.

Files changed (7) +607 / -40

Bug fix (3) +237 / -40

SerialStreamTransport.csApply operational timeouts to both serial reads and writes after open +36/-5

Apply operational timeouts to both serial reads and writes after open

• Defines operational read/write timeout constants and replaces the previous read-only adjustment with ApplyOperationalTimeouts, invoked after SerialPort.Open. This ensures SerialPort.Write is bounded by an operational timeout rather than the connection/retry timeout configuration.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs

DaqifiStreamingDevice.csEnforce SD download hard deadline by racing a worker task vs timeout/cancellation +193/-35

Enforce SD download hard deadline by racing a worker task vs timeout/cancellation

• Wraps the SD download transfer in RunWithHardDeadlineAsync, executing the transfer on a LongRunning worker and racing it against a hard deadline and the caller token. Retries now use the remaining overall budget, and the method documents that timed-out/canceled parked transfers are abandoned rather than awaited (including implications for destinationStream reuse and device state).

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

SdCardFileReceiver.csCheck cancellation token each receive loop iteration +8/-0

Check cancellation token each receive loop iteration

• Adds token.ThrowIfCancellationRequested() on every iteration so caller cancellation wins even when the underlying serial read ignores the token and returns 0 bytes on timeout. Prevents canceled downloads from being misreported as timeouts/stream-closed errors.

src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs

Tests (3) +360 / -0

SerialStreamTransportTests.csAdd test asserting operational read/write timeouts are applied +20/-0

Add test asserting operational read/write timeouts are applied

• Introduces a unit test verifying ApplyOperationalTimeouts sets a short ReadTimeout (500ms) and WriteTimeout (2000ms) and avoids InfiniteTimeout. This guards against regressions where writes remain bounded by connect-time configuration.

src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs

SdCardFileReceiverTests.csAdd cancellation-vs-timeout regression test using token-ignoring silent stream +52/-0

Add cancellation-vs-timeout regression test using token-ignoring silent stream

• Adds a test ensuring ReceiveAsync throws OperationCanceledException when the caller token is canceled even if the underlying stream ignores the token and returns 0-byte reads (serial timeout behavior). Introduces a TokenIgnoringSilentStream test double to model SerialStream semantics.

src/Daqifi.Core.Tests/Device/SdCard/SdCardFileReceiverTests.cs

SdCardOperationsTests.csAdd SD download bounding tests and device/stream test doubles +288/-0

Add SD download bounding tests and device/stream test doubles

• Adds multiple regression tests for #399: downloads time out when the transfer parks ignoring cancellation, synchronous-prefix parks still time out, caller cancellation ends the await, and slow-but-healthy transfers complete successfully. Introduces ParkedDownloadDevice (sync/async park modes), SlowDownloadDevice, and SlowChunkStream to simulate wedged and slow transfers with controllable budgets.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs

Documentation (1) +10 / -0

ISdCardOperations.csDocument TimeoutException/abandon semantics on SD download APIs +10/-0

Document TimeoutException/abandon semantics on SD download APIs

• Extends XML docs for both DownloadSdCardFileAsync overloads to describe the enforced download deadline, TimeoutException behavior, and that an in-flight transfer may be abandoned when it cannot observe cancellation (#399).

src/Daqifi.Core/Device/SdCard/ISdCardOperations.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. Cancellation masked by gate ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

RunWithHardDeadlineAsync throws InvalidOperationException when the SD download gate is held without
first honoring cancellationToken, so caller cancellation can be reported as “previous download in
flight” instead of OperationCanceledException. This violates the method’s documented cancellation
behavior when cancellation races with gate acquisition.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R2335-2342]

+            // Fail fast rather than becoming a second reader on a stream an abandoned transfer
+            // still holds. Wait(0) never blocks: this either takes the gate or reports the state.
+            if (!_sdDownloadGate.Wait(0))
+            {
+                throw new InvalidOperationException(
+                    "A previous SD card download is still in flight, or was abandoned after timing out and " +
+                    "is still parked on the transport. Reconnect the device before retrying.");
+            }

Relevance

●●● Strong

Repo consistently enforces “cancellation wins” via extra ThrowIfCancellationRequested near
side-effects/races (PRs #329, #326).

PR-#329
PR-#326

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new gate admission path throws InvalidOperationException without consulting the caller token,
while the public method documents and generally expects OperationCanceledException on cancellation.
Because DownloadSdCardFileAsync only checks cancellation once before later work, a cancellation
that occurs after that check but before this gate check will be misreported.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2335-2342]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2112-2118]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2161-2163]
PR-#326

Agent prompt

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

### Issue description
`RunWithHardDeadlineAsync` checks `_sdDownloadGate.Wait(0)` and immediately throws `InvalidOperationException` if the gate is held. If the caller cancels around the same time (after the earlier guard in `DownloadSdCardFileAsync` but before/while entering the gate check), the method can surface `InvalidOperationException` instead of the documented `OperationCanceledException`.

### Issue Context
This is new behavior introduced by the gate logic intended to quarantine abandoned transfers. The API contract already states that `DownloadSdCardFileAsync` throws `OperationCanceledException` when `cancellationToken` is canceled.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2335-2342]

### Implementation notes
- Add `cancellationToken.ThrowIfCancellationRequested()` at the start of `RunWithHardDeadlineAsync`.
- In the `if (!_sdDownloadGate.Wait(0))` branch, check `cancellationToken` again before throwing `InvalidOperationException` so cancellation wins when both are true (race window).
- Keep the existing `InvalidOperationException` for the non-canceled case (gate genuinely held).

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

---

2. Abandoned download leaks threads ✓ Resolved 🐞 Bug ☼ Reliability

Description

RunWithHardDeadlineAsync runs each SD download on a LongRunning worker and, on timeout/cancellation,
abandons that worker without awaiting it; if the underlying native I/O never returns, that dedicated
thread can remain blocked indefinitely. Because DownloadSdCardFileAsync does not prevent additional
downloads after such an abandonment, repeated retries against a persistently wedged device can
accumulate blocked threads and degrade long-lived processes.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R2319-2364]

+            // LongRunning (a dedicated thread, not a pooled one): the transfer's synchronous
+            // prefix — the consumer stop-and-join, PrepareSdInterface's blocking writes — otherwise
+            // runs on the CALLING thread up to the first await, which on a UI thread means a
+            // wedged device freezes the window, and which would put that prefix outside the very
+            // deadline it needs to be inside. A pooled Task.Run would also tie up a worker for the
+            // transfer's full blocking duration. Pass CancellationToken.None to StartNew itself:
+            // the worker's own token still cancels its waits, and "cancelled before start" must
+            // not surface as an operation fault. (Mirrors WifiBridgeActivator, #294/#295/#326.)
+            var workerTask = Task.Factory.StartNew(
+                () => operation(linkedCts.Token),
+                CancellationToken.None,
+                TaskCreationOptions.LongRunning,
+                TaskScheduler.Default).Unwrap();
+
+            try
+            {
+                var winner = await Task.WhenAny(
+                    workerTask,
+                    Task.Delay(hardDeadline, raceCts.Token)).ConfigureAwait(false);
+
+                // Only abandon when the worker is genuinely still running: WhenAny can hand back
+                // the delay even though the worker completed at that same boundary, and awaiting
+                // it below honors that result instead of discarding it.
+                if (winner != workerTask && !workerTask.IsCompleted)
+                {
+                    // Cancel explicitly instead of relying on the deadline timer having fired: the
+                    // delay above and hardDeadlineCts are two separate timers of the same duration,
+                    // so the delay can win by a hair and leave a late-returning worker running one
+                    // more state-changing step after the caller already threw. Idempotent.
+                    hardDeadlineCts.Cancel();
+
+                    // The worker may be parked in native serial I/O that no token can interrupt, so
+                    // it is ABANDONED, not awaited — waiting for it is the hang being bounded here.
+                    // Observe its eventual fault so it cannot resurface as an UnobservedTaskException,
+                    // and dispose the sources only once it is done with them (disposing early would
+                    // turn its pending waits into ObjectDisposedException instead of cancellation).
+                    _ = workerTask.ContinueWith(
+                        t =>
+                        {
+                            _ = t.Exception;
+                            linkedCts.Dispose();
+                            hardDeadlineCts.Dispose();
+                        },
+                        CancellationToken.None,
+                        TaskContinuationOptions.ExecuteSynchronously,
+                        TaskScheduler.Default);

Relevance

●●● Strong

Team previously accepted fixes preventing blocked-thread pileup from abandoned timed-out tasks
(SerialDeviceFinder quarantine, PR295).

PR-#295
PR-#326

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new deadline wrapper starts the operation on a dedicated LongRunning task and explicitly
abandons it on deadline/cancellation, meaning the worker thread can remain blocked indefinitely if
the underlying serial call never returns. The method documentation and ExecuteRawCaptureAsync
implementation confirm that a wedged worker can also prevent cleanup (consumer restart) from
running; unlike SerialDeviceFinder’s quarantine mechanism for abandoned probes, the SD download path
has no analogous suppression of repeated retries.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2101-2112]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2319-2373]
src/Daqifi.Core/Device/DaqifiDevice.cs[642-678]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[44-83]

Agent prompt

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

### Issue description
`DownloadSdCardFileAsync` can time out and intentionally *abandon* a `LongRunning` worker that may remain blocked forever in native serial I/O. Because the API does not mark the device/download subsystem as “quarantined” after an abandonment, callers can retry repeatedly and create additional permanently-blocked dedicated threads.

### Issue Context
This PR intentionally uses the “abandon on timeout” pattern (similar to other areas), but unlike the serial discovery code it does not add any guardrail to prevent unbounded accumulation of abandoned workers under repeated retries.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2113-2219]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2292-2391]

### Suggested approach
- Add a per-device SD-download gate (e.g., `SemaphoreSlim` or an atomic “in flight / abandoned” state) so only one SD download can be in-flight or abandoned at a time.
- On the hard-timeout/caller-cancel abandonment path, mark SD-download as quarantined until reconnect/disconnect (or until an explicit recovery method is called), and fail fast on subsequent download attempts with a clear exception message.
- Optionally, consider triggering a best-effort disconnect on abandonment (if safe) to force cleanup and prevent further use in a degraded state.

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

---

3. Flaky fast-fail timing ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The new test asserts the second download attempt completes in under 250ms, which can intermittently
fail on slow/loaded CI runners even when the implementation is correct. This can produce spurious
failures unrelated to the gate logic being tested.

Code

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs[R1904-1914]

+                var elapsed = Stopwatch.StartNew();
+                var ex = await Assert.ThrowsAsync<InvalidOperationException>(
+                    () => device.DownloadSdCardFileAsync("other.bin", secondDestination));
+                elapsed.Stop();
+
+                Assert.Contains("abandoned", ex.Message, StringComparison.OrdinalIgnoreCase);
+
+                // Fail fast, not another full deadline.
+                Assert.True(
+                    elapsed.Elapsed < TimeSpan.FromMilliseconds(250),
+                    $"Second download took {elapsed.ElapsedMilliseconds}ms — it should be refused immediately.");

Relevance

●●● Strong

Team previously accepted removing strict elapsed-ms bounds from SD tests as CI-flaky (PR #226).

PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test enforces a tight absolute time bound (250ms) despite the first download budget being 300ms;
this small margin can be exceeded by runtime/CI variability, causing flaky failures unrelated to the
intended gating behavior.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs[1887-1914]

Agent prompt

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

### Issue description
`DownloadSdCardFileAsync_WhileAPreviousTransferIsStillAbandoned_FailsFast` uses a strict wall-clock threshold (`< 250ms`) to prove the retry fails fast. On contended CI hardware, GC pauses/scheduling delays can exceed this even if the second call is still “fast” and correctly refused.

### Issue Context
The goal is to ensure the retry is refused due to the gate (InvalidOperationException) rather than waiting out another full deadline.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs[1904-1914]

### Safer alternatives
- Increase the margin while still staying clearly below the 300ms budget (e.g., `< 290ms`), or
- Replace the stopwatch-based assertion with a more deterministic completion check (e.g., assert the call completes immediately via `Record.ExceptionAsync` + `Task.WhenAny(secondCall, Task.Delay(1s))`, and rely on the exception type/message to prove it was refused by the gate rather than timing out).

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

Previous review results

Review updated until commit a0ee3cd

Results up to commit f880a49 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Abandoned download leaks threads ✓ Resolved 🐞 Bug ☼ Reliability

Description

RunWithHardDeadlineAsync runs each SD download on a LongRunning worker and, on timeout/cancellation,
abandons that worker without awaiting it; if the underlying native I/O never returns, that dedicated
thread can remain blocked indefinitely. Because DownloadSdCardFileAsync does not prevent additional
downloads after such an abandonment, repeated retries against a persistently wedged device can
accumulate blocked threads and degrade long-lived processes.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R2319-2364]

+            // LongRunning (a dedicated thread, not a pooled one): the transfer's synchronous
+            // prefix — the consumer stop-and-join, PrepareSdInterface's blocking writes — otherwise
+            // runs on the CALLING thread up to the first await, which on a UI thread means a
+            // wedged device freezes the window, and which would put that prefix outside the very
+            // deadline it needs to be inside. A pooled Task.Run would also tie up a worker for the
+            // transfer's full blocking duration. Pass CancellationToken.None to StartNew itself:
+            // the worker's own token still cancels its waits, and "cancelled before start" must
+            // not surface as an operation fault. (Mirrors WifiBridgeActivator, #294/#295/#326.)
+            var workerTask = Task.Factory.StartNew(
+                () => operation(linkedCts.Token),
+                CancellationToken.None,
+                TaskCreationOptions.LongRunning,
+                TaskScheduler.Default).Unwrap();
+
+            try
+            {
+                var winner = await Task.WhenAny(
+                    workerTask,
+                    Task.Delay(hardDeadline, raceCts.Token)).ConfigureAwait(false);
+
+                // Only abandon when the worker is genuinely still running: WhenAny can hand back
+                // the delay even though the worker completed at that same boundary, and awaiting
+                // it below honors that result instead of discarding it.
+                if (winner != workerTask && !workerTask.IsCompleted)
+                {
+                    // Cancel explicitly instead of relying on the deadline timer having fired: the
+                    // delay above and hardDeadlineCts are two separate timers of the same duration,
+                    // so the delay can win by a hair and leave a late-returning worker running one
+                    // more state-changing step after the caller already threw. Idempotent.
+                    hardDeadlineCts.Cancel();
+
+                    // The worker may be parked in native serial I/O that no token can interrupt, so
+                    // it is ABANDONED, not awaited — waiting for it is the hang being bounded here.
+                    // Observe its eventual fault so it cannot resurface as an UnobservedTaskException,
+                    // and dispose the sources only once it is done with them (disposing early would
+                    // turn its pending waits into ObjectDisposedException instead of cancellation).
+                    _ = workerTask.ContinueWith(
+                        t =>
+                        {
+                            _ = t.Exception;
+                            linkedCts.Dispose();
+                            hardDeadlineCts.Dispose();
+                        },
+                        CancellationToken.None,
+                        TaskContinuationOptions.ExecuteSynchronously,
+                        TaskScheduler.Default);

Relevance

●●● Strong

Team previously accepted fixes preventing blocked-thread pileup from abandoned timed-out tasks
(SerialDeviceFinder quarantine, PR295).

PR-#295
PR-#326

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new deadline wrapper starts the operation on a dedicated LongRunning task and explicitly
abandons it on deadline/cancellation, meaning the worker thread can remain blocked indefinitely if
the underlying serial call never returns. The method documentation and ExecuteRawCaptureAsync
implementation confirm that a wedged worker can also prevent cleanup (consumer restart) from
running; unlike SerialDeviceFinder’s quarantine mechanism for abandoned probes, the SD download path
has no analogous suppression of repeated retries.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2101-2112]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2319-2373]
src/Daqifi.Core/Device/DaqifiDevice.cs[642-678]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[44-83]

Agent prompt

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

### Issue description
`DownloadSdCardFileAsync` can time out and intentionally *abandon* a `LongRunning` worker that may remain blocked forever in native serial I/O. Because the API does not mark the device/download subsystem as “quarantined” after an abandonment, callers can retry repeatedly and create additional permanently-blocked dedicated threads.

### Issue Context
This PR intentionally uses the “abandon on timeout” pattern (similar to other areas), but unlike the serial discovery code it does not add any guardrail to prevent unbounded accumulation of abandoned workers under repeated retries.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2113-2219]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[2292-2391]

### Suggested approach
- Add a per-device SD-download gate (e.g., `SemaphoreSlim` or an atomic “in flight / abandoned” state) so only one SD download can be in-flight or abandoned at a time.
- On the hard-timeout/caller-cancel abandonment path, mark SD-download as quarantined until reconnect/disconnect (or until an explicit recovery method is called), and fail fast on subsequent download attempts with a clear exception message.
- Optionally, consider triggering a best-effort disconnect on abandonment (if safe) to force cleanup and prevent further use in a degraded state.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit f880a49

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1acbf57

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit b2a8da5