perf(discovery): serial discovery no longer spends half its time waiting on its own reader thread

[tylerkron]

What was wrong

Finding a DAQiFi device over USB took about 830 ms, and roughly 500 ms of that was the library waiting on itself.

The probe asks the device "who are you?", and the reply arrives on a background reader thread. Because of how the "reply arrived" signal was set up, everything that happened after the reply — including the cleanup — ran on that same reader thread. The first thing the cleanup does is wait for the reader thread to finish. So the reader thread sat there waiting for itself. It could never succeed, so it waited out its full 500 ms timeout and then gave up, every single time a device was found.

It also meant the probe handed the port back to the caller while its reader was still running over it — the port got closed out from under a live reader.

How it was fixed

Two things, and the second only became visible once the first was fixed.

The reply signal now resumes on a normal worker thread instead of the reader thread, so the cleanup is waiting for a different thread and the wait can actually complete. That alone exposed the real remaining cost: the reader only notices "please stop" when its in-progress read gives up, and the probe port's read timeout is 1 second — so the honest wait was up to a full second, worse than the 500 ms it replaced. So the moment the device answers, the probe drops that read timeout to 50 ms. It does this from inside the "status arrived" callback, which runs on the reader thread between two reads — the one moment the timeout can be changed with no read already in flight under the old value — and the next read is exactly the one the cleanup has to wait out.

The port still opens at the 1 second it always did. A port with nothing to say never reaches the callback, so it idles exactly as it did before.

What a reviewer may want to push back on:

• The short read timeout is applied late, not for the whole probe. Qodo's first review round flagged the obvious version of this change — a 50 ms timeout from the start — because a silent port would then wake twenty times a second and each wake-up ends in a thrown TimeoutException. That was measured, not waved away: ~180 ms of CPU per 2 s of probing, against ~5-13 ms at the one-second timeout, and a silent port is probed for the full response window. Applying it only after a device has identified keeps all of the teardown benefit and leaves the silent path byte-for-byte as it was. A test pins both halves.
• The probe puts the read timeout back before returning. The stream belongs to the caller and stays open, so leaving it permanently twitchier would be a side effect on someone else's stream — caught in review, and my own two-exchanges-over-one-stream test was already exercising the leak. The restore happens strictly after the consumer join, since the short timeout is what makes that join quick.
• The issue suggested returning the device as soon as it identifies and finishing teardown in the background. I deliberately did not do that. It would be faster still, but the normal thing a caller does next is connect to the port that was just discovered — and if the probe has not finished closing it, that open fails with a busy port. Discovery still returns only once the port is genuinely closed. Verified on the bench: opening the port the instant DiscoverAsync returns succeeded 10/10, in 2-4 ms.
• The MCP discovery floor (MinDiscoveryTimeoutMs) stays at 1000 ms. The issue lists lowering it as a success criterion, and the ~830 ms measurement it was derived from is now obsolete — but the new measurement is one macOS host, one candidate port, warm descriptor cache. The floor has to cover the slowest supported path, including platforms that still run an uncached USB descriptor query per port (perf(discovery): Windows runs one uncached WMI query per COM port per sweep — batch and cache like the macOS provider #487). Lowering it on the strength of the fast case is how you land back in mcp: discover_devices clamps the timeout to a 250 ms floor, but serial identify takes ~830 ms — any short timeout silently returns no devices #448, where an empty result is indistinguishable from "nothing attached". The comment now says all of that instead of quoting a stale number.
• The request/reply exchange moved into its own method (RequestDeviceStatusAsync), leaving TryGetDeviceInfoAsync owning just the port lifecycle. That is what makes the bug testable at all — the exchange can now be driven by a scripted stream with no serial hardware attached.

Verification

Tests — 11 new in SerialProbeTeardownTests. Two are regression catchers and were proven to be: with only the one-line signal change reverted, DoesNotCompleteOnTheReaderThread and WhenItReturns_ReaderThreadHasExited both fail, twice out of two runs. Neither uses a wall clock — they assert that the thread reading the stream has exited by the time the exchange completes, and that the completion did not run on it. Two more catchers cover the review-round fixes and were proven the same way: removing the timeout-shortening call fails AfterIdentifying_ShortensTheReadTimeoutForTeardown, and removing the restore fails both AfterIdentifying_RestoresTheReadTimeoutItFound and CanBeRunTwiceOverTheSameStream. Counterparts assert a silent port's timeout is left untouched and that a stream refusing timeout access still probes fine. The rest cover the parsed status, the silent-device retry count, cancellation, stream ownership, and running two exchanges back-to-back over one stream.

Full suite green on net9.0 (2932 Core + 43 Mcp) and net10.0 (2932), 0 failures, 0 warnings, on every push.

Bench (non-destructive), Nq1 fw 3.7.2 on /dev/cu.usbmodem1101:

	before	after
SerialDeviceFinder.DiscoverAsync	834 / 836 / 839 ms	397 / 380 / 372 / 373 / 373 / 382 ms
same, warm USB-descriptor cache	—	312-318 ms
abandoned-claim drain	488 / 490 / 491 ms	58 / 56 / 57 ms

Both rows measured on the same host and device with the same harness; the "after" run uses 2.5 s spacing so every run pays the macOS ioreg descriptor refresh exactly like the baseline runs did. That refresh (~85 ms, 2 s TTL) is the whole difference between the two "after" rows and is outside the probe.

Also: back-to-back sweeps with zero delay found the device 16/16 across both pushes; discover-then-immediately-open succeeded 10/10, in 2-4 ms; example CLI --discover-serial, --show-status, and a 3 s / 500 Hz stream (1186-1187 samples, this unit's known ~79% clock ratio) and --sd-list (45 files) all clean, sn=9090539562006014104, firmware unchanged, clean Disconnected. No reboot, no format, no delete, no SD:GET, no firmware, no LAN writes; serial only.

closes #486

Not merging — this is for your review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix serial discovery self-join and reduce probe teardown latency

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Prevent serial probe continuations from running on the reader thread (fixes self-join teardown
 stall).
• Reduce probe read timeout to cut stop latency while keeping response deadline logic unchanged.
• Add deterministic teardown/threading tests and update discovery timeout documentation.

Diagram

graph TD
  A["DiscoverAsync"] --> B["TryGetDeviceInfoAsync"] --> C[("SerialPort/BaseStream")]
  B --> D["RequestDeviceStatusAsync"] --> E["MessageProducer"] --> C
  D --> F["StreamMessageConsumer"] --> C
  F --> G["TaskCompletionSource (async cont.)"] --> D
  H["SerialProbeTeardownTests"] --> D

  subgraph Legend
    direction LR
    _api(["API/Method"]) ~~~ _io[("Stream/Port")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Async teardown (return device before port fully closes)

• ➕ Could reduce perceived discovery latency further by overlapping cleanup with caller work
• ➖ High risk of immediate follow-up open failing due to still-running reader/handle not released
• ➖ More complex lifecycle coordination and harder-to-reproduce race conditions

2. Replace reader thread with cancellable async read loop

• ➕ Can stop promptly via CancellationToken without relying on short read timeouts
• ➕ Potentially fewer threads per probe and simpler teardown semantics
• ➖ Larger refactor surface area; Stream/SerialPort cancellation semantics vary by platform
• ➖ Higher regression risk vs. targeted fix

3. Use a dedicated scheduler/queue for consumer event dispatch

• ➕ Avoids inline event handlers entirely; centralizes threading policy for message delivery
• ➖ Adds infrastructure complexity and latency; still needs careful teardown ordering

Recommendation: The PR’s targeted fix (RunContinuationsAsynchronously on the status TaskCompletionSource + shorter probe ReadTimeout) is the best tradeoff: it directly addresses the self-join dead-wait and bounds stop latency without changing discovery semantics (still returns only after the port is safe to reuse). Consider a cancellable async-read refactor only if future work needs deeper reductions in thread usage or more uniform cross-platform stop behavior.

Files changed (4) +501 / -97

Bug fix (1) +171 / -84

SerialDeviceFinder.csFix probe continuation thread and reduce teardown wait via shorter read timeout +171/-84

Fix probe continuation thread and reduce teardown wait via shorter read timeout

• Refactors the request/reply identify handshake into RequestDeviceStatusAsync(Stream, CancellationToken) so it can be tested against a scripted stream. Uses a TaskCompletionSource<DaqifiOutMessage> with RunContinuationsAsynchronously to prevent teardown from running on the consumer reader thread (avoiding self-join) and reduces SerialPort probe ReadTimeout to 50ms to bound stop latency while keeping write timeout at 1s.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs

Tests (2) +311 / -3

SerialProbeTeardownTests.csAdd scripted-stream tests for probe exchange and teardown threading +307/-0

Add scripted-stream tests for probe exchange and teardown threading

• Introduces a ScriptedProbeStream that mimics SerialPort.BaseStream timeout behavior, captures reader thread IDs, and can emit a real delimited protobuf status frame. Adds tests that assert the exchange returns parsed status, does not complete on the reader thread, and fully stops the reader before returning (plus retry/cancellation/stream-ownership cases).

src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.cs

DaqifiMcpTests.csUpdate discovery timeout floor comment with post-fix timing context +4/-3

Update discovery timeout floor comment with post-fix timing context

• Adjusts test commentary to reflect that the identify handshake timing improved after #486 while keeping the floor rationale focused on preventing empty-results ambiguity. No behavioral changes to the tests themselves.

src/Daqifi.Mcp.Tests/DaqifiMcpTests.cs

Documentation (1) +19 / -10

DaqifiAgent.csClarify why MinDiscoveryTimeoutMs remains 1000ms despite faster serial identify +19/-10

Clarify why MinDiscoveryTimeoutMs remains 1000ms despite faster serial identify

• Expands documentation for MinDiscoveryTimeoutMs and ClampDiscoveryTimeout to explain why the floor should cover worst-case platforms/descriptor caching behavior rather than a fast-path bench measurement. No functional changes.

src/Daqifi.Mcp/DaqifiAgent.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. ReadTimeout not restored ✓ Resolved 🐞 Bug ≡ Correctness

Description

RequestDeviceStatusAsync shortens the caller-owned stream’s ReadTimeout to 50ms after a successful
status response but never restores the original value before returning. If the still-open stream is
reused, subsequent reads can start timing out unexpectedly fast and potentially increase
timeout-related exception handling.

Code

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[R847-850]

+            var current = stream.ReadTimeout;
+            if (current == Timeout.Infinite || current > PostIdentifyReadTimeoutMs)
            {
-                // Ignore cleanup errors
+                stream.ReadTimeout = PostIdentifyReadTimeoutMs;

Relevance

●●● Strong

Team often fixes caller-visible behavioral changes around timeouts; restoring ReadTimeout avoids
leaked side effects.

PR-#420

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation explicitly leaves the stream open for the caller but shortens ReadTimeout
without restoring it, which makes this a caller-observable side effect. Elsewhere in the codebase,
temporary ReadTimeout adjustments are restored in a finally, indicating the expected pattern is
to avoid leaking timeout configuration changes.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[704-712]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[826-856]
src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.cs[140-151]
src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[235-255]
src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[367-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
`RequestDeviceStatusAsync` changes the passed-in `Stream.ReadTimeout` to `PostIdentifyReadTimeoutMs` (50ms) to speed teardown, but it never restores the original timeout. The method explicitly leaves the stream open for its owner, so this configuration mutation can leak into later operations if the stream is reused.

### Issue Context
- Timeout is shortened inside the consumer callback (`MessageReceived`).
- The method contract says the stream is caller-owned and remains open.
- Other code in the repo (e.g., `TextExchangeEngine`) temporarily adjusts `ReadTimeout` but restores it in a `finally`.

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[719-824]
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[826-857]

### Suggested fix
1. Capture the original `ReadTimeout` at the start of `RequestDeviceStatusAsync` (best-effort; only if `stream.CanTimeout`, and wrap in try/catch).
2. Allow `ShortenReadTimeoutForTeardown(stream)` to temporarily shorten the timeout as it does today.
3. In `RequestDeviceStatusAsync`’s `finally`, after `consumer.StopSafely(...)` has completed (i.e., after the join benefit is realized), restore the original `ReadTimeout` (best-effort; try/catch).
4. Add/extend a unit test asserting that after a successful call, the stream’s `ReadTimeout` equals its original value (cover both a finite original timeout and `Timeout.Infinite` where applicable).

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

---

2. Exception-driven polling overhead ✓ Resolved 🐞 Bug ➹ Performance

Description

Setting the probe SerialPort.ReadTimeout to 50ms causes StreamMessageConsumer’s no-data path to
throw/catch TimeoutException roughly every 50ms while probing a silent/non-DAQiFi port, increasing
exception churn compared to the prior 1s timeout. This is a potential CPU/GC cost when many ports
are probed or discovery runs frequently, even though it improves teardown latency.

Code

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[65]

+    private const int ProbeReadTimeoutMs = 50;

Relevance

●● Moderate

Tradeoff vs teardown latency; perf micro-optimizations have mixed history and no close precedent on
timeout-exception churn.

PR-#420
PR-#503

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

SerialDeviceFinder reduces the probe port ReadTimeout to 50ms, and StreamMessageConsumer explicitly
handles read timeouts by catching TimeoutException and looping; therefore a silent probe can
repeatedly throw/catch exceptions at a cadence set by that ReadTimeout.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[43-66]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[618-623]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[370-388]

Agent prompt

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

## Issue description
The probe read timeout was reduced to 50ms to improve teardown latency, but `StreamMessageConsumer` treats read timeouts by catching `TimeoutException` and immediately looping. With a 50ms timeout, silent ports can generate frequent exceptions during probing, which can add CPU/GC overhead in port-heavy or frequent-sweep scenarios.

## Issue Context
- The change is intentional for stop-latency (#486), so the goal is to keep the teardown benefit while limiting exception churn.

## Fix Focus Areas
- Evaluate whether the 50ms value can be increased (e.g., 100–200ms) without reintroducing unacceptable teardown latency, based on benchmarks.
- If feasible for SerialPort/BaseStream, consider a non-exception-based polling strategy (e.g., ReadAsync with cancellation/timeout) or adjust consumer logic to avoid using exceptions as the steady-state idle signal.

### Code pointers
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[43-66]
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[618-623]
- src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[370-388]

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

---

3. Timeout assertion too weak ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

RequestDeviceStatusAsync_CanBeRunTwiceOverTheSameStream claims to verify each exchange starts with
the restored read timeout, but it only asserts the first entry of ReadsIssuedWithTimeout. This can
let a regression where the second exchange begins with the shortened timeout (50ms) pass undetected.

Code

src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.cs[R167-168]

+        // Each exchange started from the timeout it found, because the previous one put it back.
+        Assert.All(stream.ReadsIssuedWithTimeout.Take(1), issued => Assert.Equal(ProbeStartReadTimeoutMs, issued));

Relevance

●●● Strong

Team often accepts strengthening tests to catch regressions; this assertion currently under-verifies
intended timeout restoration.

PR-#478
PR-#416

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test asserts only the first recorded read timeout, but the stream records an entry for every
Read call; therefore the check does not validate the second exchange’s initial read timeout at all.

src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.cs[153-169]
src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.cs[340-346]

Agent prompt

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

### Issue description
The test `RequestDeviceStatusAsync_CanBeRunTwiceOverTheSameStream` intends to validate that **each** call to `RequestDeviceStatusAsync` starts reading with the original timeout (because the previous call restored it). However, it currently checks only `ReadsIssuedWithTimeout.Take(1)`, which validates only the first read of the *first* exchange.

### Issue Context
`ReadsIssuedWithTimeout` is appended on *every* stream `Read(...)` call, so verifying only the first element does not prove anything about the second exchange’s initial timeout.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.cs[167-168]

### Suggested fix
Update the assertion to validate the first read timeout for **both** exchanges. For example:
- capture `var beforeSecond = stream.ReadsIssuedWithTimeout.Count;` after the first exchange, then after the second exchange assert `stream.ReadsIssuedWithTimeout[0] == ProbeStartReadTimeoutMs` and `stream.ReadsIssuedWithTimeout[beforeSecond] == ProbeStartReadTimeoutMs`; or
- group reads by `ReaderThreadIds` (since each exchange uses a different reader thread) and assert the first timeout per group is `ProbeStartReadTimeoutMs`.

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

---

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 1705eb8

Results up to commit 5ee0427 ⚖️ Balanced

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

1. Exception-driven polling overhead ✓ Resolved 🐞 Bug ➹ Performance

Description

Setting the probe SerialPort.ReadTimeout to 50ms causes StreamMessageConsumer’s no-data path to
throw/catch TimeoutException roughly every 50ms while probing a silent/non-DAQiFi port, increasing
exception churn compared to the prior 1s timeout. This is a potential CPU/GC cost when many ports
are probed or discovery runs frequently, even though it improves teardown latency.

Code

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[65]

+    private const int ProbeReadTimeoutMs = 50;

Relevance

●● Moderate

Tradeoff vs teardown latency; perf micro-optimizations have mixed history and no close precedent on
timeout-exception churn.

PR-#420
PR-#503

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

SerialDeviceFinder reduces the probe port ReadTimeout to 50ms, and StreamMessageConsumer explicitly
handles read timeouts by catching TimeoutException and looping; therefore a silent probe can
repeatedly throw/catch exceptions at a cadence set by that ReadTimeout.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[43-66]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[618-623]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[370-388]

Agent prompt

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

## Issue description
The probe read timeout was reduced to 50ms to improve teardown latency, but `StreamMessageConsumer` treats read timeouts by catching `TimeoutException` and immediately looping. With a 50ms timeout, silent ports can generate frequent exceptions during probing, which can add CPU/GC overhead in port-heavy or frequent-sweep scenarios.

## Issue Context
- The change is intentional for stop-latency (#486), so the goal is to keep the teardown benefit while limiting exception churn.

## Fix Focus Areas
- Evaluate whether the 50ms value can be increased (e.g., 100–200ms) without reintroducing unacceptable teardown latency, based on benchmarks.
- If feasible for SerialPort/BaseStream, consider a non-exception-based polling strategy (e.g., ReadAsync with cancellation/timeout) or adjust consumer logic to avoid using exceptions as the steady-state idle signal.

### Code pointers
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[43-66]
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[618-623]
- src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[370-388]

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

---

Results up to commit 99531c8 ⚖️ Balanced

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

1. ReadTimeout not restored ✓ Resolved 🐞 Bug ≡ Correctness

Description

RequestDeviceStatusAsync shortens the caller-owned stream’s ReadTimeout to 50ms after a successful
status response but never restores the original value before returning. If the still-open stream is
reused, subsequent reads can start timing out unexpectedly fast and potentially increase
timeout-related exception handling.

Code

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[R847-850]

+            var current = stream.ReadTimeout;
+            if (current == Timeout.Infinite || current > PostIdentifyReadTimeoutMs)
            {
-                // Ignore cleanup errors
+                stream.ReadTimeout = PostIdentifyReadTimeoutMs;

Relevance

●●● Strong

Team often fixes caller-visible behavioral changes around timeouts; restoring ReadTimeout avoids
leaked side effects.

PR-#420

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation explicitly leaves the stream open for the caller but shortens ReadTimeout
without restoring it, which makes this a caller-observable side effect. Elsewhere in the codebase,
temporary ReadTimeout adjustments are restored in a finally, indicating the expected pattern is
to avoid leaking timeout configuration changes.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[704-712]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[826-856]
src/Daqifi.Core.Tests/Device/Discovery/SerialProbeTeardownTests.cs[140-151]
src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[235-255]
src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[367-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
`RequestDeviceStatusAsync` changes the passed-in `Stream.ReadTimeout` to `PostIdentifyReadTimeoutMs` (50ms) to speed teardown, but it never restores the original timeout. The method explicitly leaves the stream open for its owner, so this configuration mutation can leak into later operations if the stream is reused.

### Issue Context
- Timeout is shortened inside the consumer callback (`MessageReceived`).
- The method contract says the stream is caller-owned and remains open.
- Other code in the repo (e.g., `TextExchangeEngine`) temporarily adjusts `ReadTimeout` but restores it in a `finally`.

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[719-824]
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[826-857]

### Suggested fix
1. Capture the original `ReadTimeout` at the start of `RequestDeviceStatusAsync` (best-effort; only if `stream.CanTimeout`, and wrap in try/catch).
2. Allow `ShortenReadTimeoutForTeardown(stream)` to temporarily shorten the timeout as it does today.
3. In `RequestDeviceStatusAsync`’s `finally`, after `consumer.StopSafely(...)` has completed (i.e., after the join benefit is realized), restore the original `ReadTimeout` (best-effort; try/catch).
4. Add/extend a unit test asserting that after a successful call, the stream’s `ReadTimeout` equals its original value (cover both a finite original timeout and `Timeout.Infinite` where applicable).

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

---

[tylerkron]

Round 1 — Exception-driven polling overhead: valid, taken (99531c8).

I measured it before deciding, and it was bigger than I would have guessed. Running the real consumer loop over an idle bench port for a fixed 2 s window, marginal reader CPU (idle baseline subtracted, two runs each):

probe ReadTimeout	read timeouts in the window	reader CPU
1000 ms	~2	5 / 13 ms
50 ms	~40	178 / 184 ms

A silent port is read for the whole ResponseTimeoutMs window, so the blanket 50 ms would have cost roughly 90 ms of CPU per silent-port probe against ~5 ms before — and with the desktop apps sweeping every 2-3 s, that is not noise on a host in the legacy fallback path where several unclassifiable ports get probed.

Rather than trade latency against churn (100 ms was the obvious compromise, and it put DiscoverAsync back over the ticket's 400 ms target on cold-cache runs), the timeout is now shortened only once the device has identified. The port opens at the same 1 s it always did; ShortenReadTimeoutForTeardown runs from inside the status dispatch, which is on the reader thread between two reads — the one moment the timeout can change with no read in flight under the old value — so the next read, the one teardown waits out, gets the 50 ms. A port that never answers never reaches that callback and idles exactly as it did before this PR.

So the finding is closed rather than accepted-with-a-tradeoff: the fast path keeps the full teardown win and the silent path is unchanged.

Two tests pin both halves, and the first was proven to catch its own bug — deleting the ShortenReadTimeoutForTeardown call fails AfterIdentifying_ShortensTheReadTimeoutForTeardown:

• AfterIdentifying_ShortensTheReadTimeoutForTeardown — every read issued after the status carries the 50 ms.
• SilentDevice_LeavesTheReadTimeoutAlone — the timeout is never written, and every read is issued at the starting value.

Re-verified on the bench after the change: DiscoverAsync 390 / 384 / 373 / 388 / 377 ms under the same conditions that measured 834 / 836 / 839 ms before the fix (so the round-1 fix cost nothing), abandoned-claim drain 56 / 55 / 53 ms, 6/6 back-to-back sweeps with zero delay found the device, 4/4 discover-then-immediately-open succeeded in 2-3 ms, and the example CLI ran --discover-serial, --show-status and a 3 s / 500 Hz stream (1187 samples) clean with sn=9090539562006014104 and firmware unchanged.

Full suite green on net9.0 (2930 Core + 43 Mcp) and net10.0 (2930), 0 failures, 0 warnings.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 99531c8

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 407f83a

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Qodo-clean, CI green — ready for review.

Round 4 on head c3cc25c: no new findings (Bugs 0 / Rule violations 0 / Skill insights 0), 0 unresolved review threads, and the round-2 finding struck ✓ Resolved. Freshness confirmed two ways — the bot's "updated up to the latest commit c3cc25c" note, and 22 SHA references to c3cc25c against 1 each to the three older commits. Settle re-check at +5 min: the summary comment was byte-identical (diff -q), threads still 0, nothing posted in between. CI build SUCCESS on c3cc25c; MERGEABLE / CLEAN.

Three rounds of findings, all taken:

1. Exception-driven polling overhead — the first cut shortened the probe port's read timeout for the whole probe, which made a silent port wake twenty times a second, each wake-up ending in a thrown TimeoutException. Measured at ~180 ms of CPU per 2 s of probing against ~5-13 ms at the one-second timeout. Fixed by shortening the timeout only after a device has identified, which keeps the entire teardown win and leaves the silent path exactly as it was — the finding is closed rather than traded against.
2. ReadTimeout not restored — that shortened timeout leaked onto a stream the method documents as caller-owned. Now restored in the finally, strictly after the consumer join.
3. Timeout assertion too weak — my reuse test only asserted the first exchange's opening read, so it could not have caught the regression it exists for. Now indexes the second exchange's opening read explicitly.

The same push as (3) also fixed a flake of my own that turned CI red on 407f83a: the cancellation test asserted no retry had gone out yet, racing a 300 ms retry timer against wherever the scheduler put the cancellation. It now asserts the bound that always holds. Not a defect in the change — a bad assertion I wrote — and worth flagging since it is the one red run on this PR.

Full suite green on net9.0 (2932 Core + 43 Mcp) and net10.0 (2932), 0 failures, 0 warnings, on all four pushes; the new test class also ran five times back-to-back locally with no failures.

Bench was re-run on real hardware after the last production change (407f83a); the final push is test-only, so those results stand. DiscoverAsync 397/380/372/373/373/382 ms against the 834/836/839 ms baseline measured before any edit, under matched conditions.

Not merging — this is for your review.

[qodo-code-review]

Qodo-clean, CI green — ready for review.

Round 4 on head c3cc25c: no new findings (Bugs 0 / Rule violations 0 / Skill insights 0), 0 unresolved review threads, and the round-2 finding struck ✓ Resolved. Freshness confirmed two ways — the bot's "updated up to the latest commit c3cc25c" note, and 22 SHA references to c3cc25c against 1 each to the three older commits. Settle re-check at +5 min: the summary comment was byte-identical (diff -q), threads still 0, nothing posted in between. CI build SUCCESS on c3cc25c; MERGEABLE / CLEAN.

Three rounds of findings, all taken:

1. Exception-driven polling overhead — the first cut shortened the probe port's read timeout for the whole probe, which made a silent port wake twenty times a second, each wake-up ending in a thrown TimeoutException. Measured at ~180 ms of CPU per 2 s of probing against ~5-13 ms at the one-second timeout. Fixed by shortening the timeout only after a device has identified, which keeps the entire teardown win and leaves the silent path exactly as it was — the finding is closed rather than traded against.
2. ReadTimeout not restored — that shortened timeout leaked onto a stream the method documents as caller-owned. Now restored in the finally, strictly after the consumer join.
3. Timeout assertion too weak — my reuse test only asserted the first exchange's opening read, so it could not have caught the regression it exists for. Now indexes the second exchange's opening read explicitly.

The same push as (3) also fixed a flake of my own that turned CI red on 407f83a: the cancellation test asserted no retry had gone out yet, racing a 300 ms retry timer against wherever the scheduler put the cancellation. It now asserts the bound that always holds. Not a defect in the change — a bad assertion I wrote — and worth flagging since it is the one red run on this PR.

Full suite green on net9.0 (2932 Core + 43 Mcp) and net10.0 (2932), 0 failures, 0 warnings, on all four pushes; the new test class also ran five times back-to-back locally with no failures.

Bench was re-run on real hardware after the last production change (407f83a); the final push is test-only, so those results stand. DiscoverAsync 397/380/372/373/373/382 ms against the 834/836/839 ms baseline measured before any edit, under matched conditions.

Not merging — this is for your review.

Review status looks clean based on the supplied Round 4 results: there are no active Qodo findings and no unresolved review threads. The previously reported issues are addressed: finding 1 restores the caller-owned ReadTimeout, finding 2 limits the shortened timeout to identified devices, and finding 3 verifies the second exchange’s opening read. The cancellation assertion change also removes a scheduler-dependent test race. I have no further findings from this diff; the reported green CI and stable re-check support marking the PR ready for merge when you are.

[tylerkron]

Merged main in — #506/#507/#508 landed while this was open and #508's ConfigureAwait(false) sweep collided with this branch inside SerialDeviceFinder.TryGetDeviceInfoAsync.

Two conflicts, both resolved the obvious way: the probe body keeps this PR's extracted RequestDeviceStatusAsync call (main's side was just the old inline producer/consumer block this PR replaced), and the DTR settle wait takes both changes at once — await Task.Delay(DtrSettleMs).ConfigureAwait(false). Nothing else in the merge needed a decision. CA2007 is a build error since #508, and the merged branch builds warning-free, so no naked awaits came across.

Re-verified after the merge rather than assumed. Full suite green net9.0 (2972 Core + 43 Mcp) and net10.0 (2972 Core), 0 failures, 0 warnings. Bench re-run non-destructively on fw 3.7.2 / /dev/cu.usbmodem1101 because the merge changed production code on this branch: --discover-serial found the unit on 3/3 runs (sn=9090539562006014104), 3 s @ 500 Hz on channels 0-2 gave 1186 samples (this unit's known ~79% clock ratio, same as the pre-merge run), --sd-list returned 45 files, clean Disconnected. Serial only — no reboot/format/delete/SD:GET/firmware/LAN writes.

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1705eb8

[tylerkron]

Qodo-clean, CI green, conflict gone — ready for review.

Round 5 on head 1705eb8 (the merge commit): Bugs (0) / Rule violations (0) / Skill insights (0), 0 unresolved threads, round-2 finding still struck. Freshness: the bot's latest-commit note plus 22 SHA refs to 1705eb8 against 1 to the old head. Settle re-check five minutes later — summary byte-identical, comment count unchanged, threads still 0. CI build SUCCESS; MERGEABLE / CLEAN.

Not merging — yours to merge.

[qodo-code-review]

Qodo-clean, CI green, conflict gone — ready for review.

Round 5 on head 1705eb8 (the merge commit): Bugs (0) / Rule violations (0) / Skill insights (0), 0 unresolved threads, round-2 finding still struck. Freshness: the bot's latest-commit note plus 22 SHA refs to 1705eb8 against 1 to the old head. Settle re-check five minutes later — summary byte-identical, comment count unchanged, threads still 0. CI build SUCCESS; MERGEABLE / CLEAN.

Not merging — yours to merge.

I can’t merge the PR here. Based on the information provided, the head is clean, CI is green, and there are no active Qodo findings; I can still explain the implemented findings, review a specific change, or help assess merge readiness.