fix(discovery): wait out an abandoned port claim instead of reporting no devices

[tylerkron]

For review only — please do not merge.

The problem

A SerialDeviceFinder pass that ends by timeout or caller cancellation abandons its in-flight probe, but the PortClaims entry survives with Probe.IsCompleted == false. The stale-claim recovery in ProbeSafelyAsync only clears a completed claim, so every subsequent pass takes the return null path and silently reports the port as absent.

Bench-measured on a real Nq1 (fw 3.7.2, USB, macOS) against origin/main @ 4b8eed2 — full evidence:

preceding pass	next pass	latency
completed normally (control, 3/3)	FOUND	766-817 ms
timed out at 300 ms (3/3)	no devices	~1 ms
cancelled at 200 ms (2/2)	no devices	~1 ms

The ~1 ms proves the port was never opened — a real pass costs ~800 ms. The claim was held 488/490/491 ms across three runs before auto-releasing: nowhere near the 30 s QuarantineRetryTtlMs, so the abandoned probe was draining, not wedged. Because each skipped attempt costs ~1 ms, a naive immediate-retry loop burns ~18 retries inside the window and concludes no device exists.

The 2-3 s desktop sweep never lands inside that window (ContinuousDeviceFinder was confirmed unaffected). The gap is the one-shot caller that retries promptly — exactly the Daqifi.Mcp shape, where DaqifiAgent.DiscoverAsync builds a fresh SerialDeviceFinder per call.

Option chosen: 3 (bounded wait), with 1's documentation folded in

Option	Verdict
1 — documentation only	Rejected as the fix. Leaves the failure mode in place and asks every caller to work around it. Its substance is still worth having, so the contract note is included.
2 — surface the skip	Rejected. Makes the miss diagnosable but not fixed — Daqifi.Mcp and every future one-shot caller would still have to implement backoff, and it adds public API surface to do it.
3 — bounded wait	Chosen. Fixes it in one place for all callers, with no API change.

The decisive point for option 3: waiting on an existing claim's probe task starts no new probe and blocks no new thread. The wait awaits the Task that is already running, so the thread-leak bound from #294/#295 that motivated the claim is preserved for free — no tradeoff to make.

What changed

When a claim is abandoned-but-in-flight and less than AbandonedClaimDrainWaitMs (1 s, ~2x the measured drain) has elapsed since abandonment, ProbeSafelyAsync waits for it to clear before giving up, then re-attempts the claim. The wait is bounded by both the drain window and the caller's own token/timeout, via a linked CTS.

Behaviour for a genuinely wedged port is unchanged in the way that matters: the pass burns the remaining window once, then the claim ages out of the window and is skipped in ~1 ms exactly as before. Nothing is re-probed, and the QuarantineRetryTtlMs recovery path is untouched.

The claim loop goes from 2 attempts to 3 — the drain wait's worst path is wait → observe completed claim → clear → claim.

Also documented on IDeviceFinder.DiscoverAsync that an empty result means "nothing answered within the budget you gave", not "no device is attached" — so callers give a realistic timeout rather than retrying tightly.

Tests

Five new tests in SerialDeviceFinderTests, using the existing internal seams (probeOverride, portNameProvider, ResetPortQuarantineForTests, PortProbeHardTimeoutMs) plus a new internal AbandonedClaimDrainWaitMs knob. A gated fake probe (DrainProbe) stands in for the uncancellable native I/O, so the state machine is driven deterministically with no hardware:

• AfterTimedOutPass_WaitsForAbandonedClaimAndFindsDevice — the bench timeout row
• AfterCancelledPass_WaitsForAbandonedClaimAndFindsDevice — the bench cancellation row
• AfterTimedOutPass_WithoutDrainWait_SilentlyReportsNoDevices — pins the regression itself: port never reopened, bail-out under 500 ms
• WedgedPort_DrainWaitExpiresWithoutReProbing — the thread-leak bound holds; probe started exactly once
• DrainWait_IsBoundedByCallerTimeout — a 250 ms caller is not held for a 10 s window
• DrainWait_DoesNotDelayOtherPortsOnTheSameSweep — one draining port doesn't stall the rest of the pass
• MoreDrainingPortsThanProbeSlots_StillProbesHealthyPortPromptly (added in review round 1) — 5 draining ports against the cap of 4, healthy port last

Every discovery await is bounded by a 30 s SweepGuard, so a regression fails the test rather than hanging CI.

Each fix was confirmed against the unfixed code, not just asserted:

• the two "finds the device" tests fail with the drain-wait branch disabled;
• MoreDrainingPortsThanProbeSlots fails with the probe slot held across the whole call.

Timing-sensitive cases were run 5x for flakiness — stable.

Review round 1 (Qodo) — both findings accepted

#	Finding	Resolution
1	Drain wait consumes probe slots	Real. The gate was held across the whole ProbeSafelyAsync call, so a drain wait (which opens no port) occupied one of 4 slots. A pass that times out with every slot busy abandons all its probes at once, so the next pass can park every slot on drain waits. Gate now wraps only the port open. My existing 2-port test never contended and passed either way — replaced that false confidence with the 5-port test above.
2	Drain tests can hang	Real. All new discovery awaits now bounded by SweepGuard.

Verification

• Discovery tests: 180/180 green on net9.0 and net10.0.
• Full solution: 2710 passed, 2 skipped, 0 failed on both net9.0 and net10.0.

(An earlier revision of this PR noted 5 failing FirmwareUpdateServiceTests; those were pre-existing on main and have since been fixed by #456, now merged in here.)

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix Serial discovery by waiting for abandoned port claims to drain

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Wait briefly for recently-abandoned in-flight port probes to finish draining before skipping.
• Bound the wait by both a drain window and the caller’s timeout/cancellation budget.
• Add regression tests and clarify IDeviceFinder empty-result semantics for tight-retry callers.

Diagram

graph TD
  A([Caller]) --> B["SerialDeviceFinder.DiscoverAsync"] --> C["ProbeSafelyAsync"] --> D[("PortClaims (static)")]
  D --> E{"Claim state?"}
  E -->|"Owned"| F["Start probe Task.Run"] --> G["Device probe"]
  E -->|"Abandoned & in-flight"| H["Bounded drain wait"] --> C
  E -->|"Wedged/in-flight"| I["Skip port (null)"]

  subgraph Legend
    direction LR
    _a([Caller]) ~~~ _b["Code"] ~~~ _c[("Shared state")] ~~~ _d{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Documentation-only contract (callers must back off)

• ➕ No runtime behavior change
• ➕ Zero risk to discovery timing paths
• ➖ Leaves silent false-negative behavior in place
• ➖ Forces every prompt-retry caller to reimplement backoff heuristics

2. Surface a distinct “skipped due to claim” outcome

• ➕ Makes misses diagnosable
• ➕ Lets callers decide backoff vs fail-fast
• ➖ Expands public API surface area
• ➖ Still requires caller-side retry policy to fix the issue

3. Immediate claim takeover / force-clear abandoned in-flight claims

• ➕ Potentially fastest recovery in some cases
• ➖ Risks violating the one-thread-per-port bound by spawning new probes while old ones unwind
• ➖ Higher chance of races/handle contention with native serial I/O

Recommendation: The chosen bounded-wait approach is the best tradeoff: it fixes the false-negative for prompt-retry callers without adding API surface and without spawning additional probe work. Because it awaits the already-running probe task and caps the wait by both drain-window and caller budget, it preserves the original thread-leak bound while improving correctness.

Files changed (3) +344 / -18

Bug fix (1) +108 / -17

SerialDeviceFinder.csWait out recently-abandoned in-flight port claims before skipping +108/-17

Wait out recently-abandoned in-flight port claims before skipping

• Introduces an AbandonedClaimDrainWaitMs window (default 1s) and extends ProbeSafelyAsync to await an existing abandoned-but-in-flight probe task once, bounded by both the drain window and the caller’s cancellation/timeout budget. Expands the claim loop to allow a post-wait re-check/re-claim attempt while preserving the existing quarantine TTL behavior and the one-thread-per-port safety bound.

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

Tests (1) +223 / -1

SerialDeviceFinderTests.csAdd tests for abandoned-claim drain waiting and bounded behavior +223/-1

Add tests for abandoned-claim drain waiting and bounded behavior

• Adds a gated probe harness to simulate an abandoned in-flight probe that later drains, plus tests covering timeout and cancellation scenarios. Verifies the wait is bounded (by drain window and caller timeout), does not re-probe wedged ports, and does not delay discovery/events for other healthy ports in the same sweep.

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

Documentation (1) +13 / -0

IDeviceFinder.csClarify empty discovery results as budget-limited, not proof of absence +13/-0

Clarify empty discovery results as budget-limited, not proof of absence

• Adds remarks documenting that an empty result can mean “no response within the provided budget,” especially after a prior timed-out/cancelled pass that is still releasing resources. Points callers to provide realistic timeouts rather than tight retry loops.

src/Daqifi.Core/Device/Discovery/IDeviceFinder.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. Drain tests can hang ✓ Resolved 🐞 Bug ☼ Reliability

Description

The newly added drain-wait tests start discovery sweeps and await them without a timeout, so a
regression that prevents DiscoverAsync from completing will hang the test run instead of failing
fast.

Code

src/Daqifi.Core.Tests/Device/Discovery/SerialDeviceFinderTests.cs[R620-624]

+        var sweep = second.DiscoverAsync(CancellationToken.None);
+        await Task.Delay(150);
+        drainProbe.ReleaseFirstProbe();
+
+        var devices = (await sweep).ToList();

Relevance

●●● Strong

Team recently accepted adding timeouts/bounded awaits to prevent CI hangs in discovery tests.

PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new tests create sweep tasks and later await them directly with no bounding timeout; this is
the same hang-risk pattern previously fixed elsewhere in the test suite.

src/Daqifi.Core.Tests/Device/Discovery/SerialDeviceFinderTests.cs[608-656]
src/Daqifi.Core.Tests/Device/Discovery/SerialDeviceFinderTests.cs[682-705]
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
Several newly added tests await `SerialDeviceFinder.DiscoverAsync(...)` tasks (`sweep`) without bounding the await. If a regression causes discovery to never complete, CI can hang indefinitely (or until an external runner timeout), making failures slow and expensive to diagnose.

### Issue Context
This repo previously accepted changes to bound test awaits to prevent hangs (see prior work around discovery timeouts).

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/SerialDeviceFinderTests.cs[593-761]

### Implementation notes
- Wrap discovery awaits in a bounded wait, e.g.:
 - `await sweep.WaitAsync(TimeSpan.FromSeconds(5));`
 - or apply `[Fact(Timeout = 10000)]` (or equivalent) to the new tests.
- Apply consistently to:
 - sweeps started as `var sweep = second.DiscoverAsync(...)`
 - any direct `await second.DiscoverAsync(...)` that could hang under regression.

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

---

2. Drain wait consumes probe slots ✓ Resolved 🐞 Bug ➹ Performance

Description

DiscoverAsync acquires probeGate before calling ProbeSafelyAsync, and the new abandoned-claim
drain wait can await up to AbandonedClaimDrainWaitMs while holding that slot. When
availablePorts.Count > MaxParallelProbes, a few draining ports can occupy all slots and delay
probing/DeviceDiscovered for healthy ports in the same sweep.

Code

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[R423-426]

+                            drainCts.CancelAfter((int)drainBudgetMs);
+                            try
+                            {
+                                await pending.WaitAsync(drainCts.Token).ConfigureAwait(false);

Relevance

●● Moderate

Repo prioritizes not letting bad ports delay healthy probes, but fixing semaphore-slot hold may
require risky concurrency refactor.

PR-#295

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

DiscoverAsync limits concurrency via probeGate and calls ProbeSafelyAsync only after acquiring
a slot; the new drain-wait awaits an existing task inside ProbeSafelyAsync, so that wait time
holds the semaphore slot and can block other ports from starting when MaxParallelProbes is
saturated.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[270-293]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[385-441]
PR-#295

Agent prompt

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

### Issue description
`SerialDeviceFinder.DiscoverAsync` takes a `probeGate` slot before calling `ProbeSafelyAsync`. The new drain-wait path inside `ProbeSafelyAsync` can then await an existing abandoned probe task for up to `AbandonedClaimDrainWaitMs`, which keeps the `probeGate` slot occupied even though no new SerialPort open is happening.

This reduces effective probe concurrency and can delay probing healthy ports (and therefore `DeviceDiscovered`) when the port list is larger than `MaxParallelProbes` and several ports are simultaneously in the drain window.

### Issue Context
The `probeGate` is intended to cap concurrent *opens/probes* (blocking I/O). The new drain-wait is purely an `await` on an already-running task and should not consume one of the limited probe slots.

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[270-294]
- src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[349-447]

### Implementation notes
- Restructure so `probeGate.WaitAsync` is acquired **only immediately before starting a new probe** (i.e., just before `Task.Run(() => probe(...))`).
 - Option A: Move the claim/drain-check logic out of `ProbeSafelyAsync` into the per-port lambda, and only enter the gated section once the claim is won.
 - Option B: Pass `probeGate` into `ProbeSafelyAsync` and acquire/release it only around the `Task.Run` + hard-timeout section, not around the claim/drain-wait section.
- Add/adjust a test that uses > `MaxParallelProbes` ports (e.g., 5+), where the first `MaxParallelProbes` ports are draining and another port is healthy, and assert the healthy device is discovered promptly (before the drain window elapses).

ⓘ 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]

Round 1 addressed in 55fdd6b — both findings were real and are fixed; replies on each inline thread.

• Drain wait consumed a probe slot — the gate now wraps only the port open, not claim acquisition or the drain wait. Notably my own 2-port test passed either way, so I added a 5-port test that contends against the cap of 4 and confirmed it fails without the fix.
• Unbounded test awaits — every discovery await in the new tests is now bounded by a 30s SweepGuard.

Full suite green: 2710 passed / 2 skipped / 0 failed on net9.0 and net10.0.

/agentic_review

[tylerkron]

Round 2 came back clean (Bugs 0, no inline findings) — thanks.

One commit landed after that review ran: 07b162c, a test-only robustness fix. MoreDrainingPortsThanProbeSlots hard-coded five draining ports against MaxParallelProbes = 4; raising that cap would have left the test passing while no longer contending for a slot, which is the whole point of it. The const is now internal and the port list is derived from it.

Requesting one more pass to cover that commit.

/agentic_review