feat(device): cancellable async connect/disconnect and IAsyncDisposable (closes #341)

[tylerkron]

Not merging — for review.

closes #341

Why

Connecting to a device and closing it were the last two things in daqifi-core you could not do
without blocking. Disconnect() waits up to ten seconds for any command still in flight, and
Dispose() runs that wait on whatever thread happens to be disposing — in the WPF desktop app,
that is the UI thread, and the app visibly freezes. There was also no way to give up on a connection
attempt once it started: if a user hit Cancel while the app was dialling an unresponsive device,
nothing happened until the connect timed out on its own.

What

The device now has async twins of the three lifecycle methods, and cancellation reaches all the way
down to the transport:

• ConnectAsync(CancellationToken) and DisconnectAsync(CancellationToken) on the device.
• await using var device = ... now works, and never blocks the caller.
• A connect attempt can be cancelled mid-dial. On the bench, cancelling a connect to an unreachable
  address configured with a 60-second timeout and five retries returned in 513 ms instead of
  waiting the timeout out.
• A cancelled attempt cleans up after itself: if the transport had already opened when the cancel
  landed, it gets closed again rather than left dangling.

DisconnectAsync treats its token slightly differently, on purpose: cancelling it skips the wait
for an in-flight command and goes straight to teardown. It never aborts the disconnect half-way,
because a half-torn-down device is worse than a slow one. This is documented on the method.

Nothing is removed. Connect(), Disconnect() and Dispose() all still exist and do exactly what
they did before.

How

Both the sync and async paths now share one set of internal steps, so they cannot drift apart — the
only difference is whether the transport is opened by a blocking call or an awaited one.

For the two public interfaces, the new cancellable methods are added as default interface
implementations that fall back to the existing uncancellable ones. That means anyone who has
written their own IStreamTransport or IDevice keeps compiling and working with no changes; they
simply do not gain cancellation until they override the new member. The transports shipped here do
override them.

Is anything breaking?

No. Not source-breaking and not binary-breaking:

• Nothing was removed or renamed, and no existing signature changed.
• The new interface members carry default implementations, so existing implementors of
  IStreamTransport and IDevice — including ones already compiled against an earlier version —
  remain valid.
• DaqifiDevice gained IAsyncDisposable alongside IDisposable. Existing using var device = ...
  code is unaffected.
• The one internal helper whose signature did change (ConnectRetryExecutor) is not public.

The only behavioural change on an existing path is inside the connect factory, which now passes the
caller's cancellation token down to the transport — previously it checked the token only between
steps. That is the fix, not a side effect.

Testing

• Full suite green on net9.0 and net10.0: 2215 passed, 2 skipped, 0 failed on each. MCP suite
  green (23/23). Release build with zero warnings.
• New tests cover cancelling mid-connect through the transport layer, cancelling during retry
  backoff, async disposal, the "cancel arrived after the transport opened" cleanup, a transport
  written against the old interface still working through the new overload, and that sync
  Connect/Disconnect/Dispose behave exactly as before.

Bench (Nyquist 1, HW 2.0.0, FW 3.7.2)

Check	Result
USB await using — connect, stream 5 s @ 100 Hz, async dispose	396 analog messages; disposal 199 ms; process exited promptly
WiFi await using (single connect)	396 analog messages; disposal 220 ms; exited promptly
Cancelled connect to an unreachable IP (60 s timeout, 5 attempts)	OperationCanceledException after 513 ms
Sync path — manual Connect() / Disconnect() / Dispose()	32 channels, 397 messages, teardown 202 ms — unchanged
Stock example CLI (unmodified consumer code) over USB	exit 0, sample threshold met

Message counts are ~79 % of the requested rate, which is the known firmware clock issue on this
unit, not a regression.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add cancellable async connect/disconnect and IAsyncDisposable for devices

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add cancellable ConnectAsync/DisconnectAsync and async disposal to device lifecycle.
• Thread CancellationToken through transports and retry/backoff so mid-dial cancels promptly.
• Add tests and docs to pin cancellation/timeout semantics and async-using guidance.

Diagram

graph TD
  A["App / caller"] --> B["DaqifiDeviceFactory"] --> C["DaqifiDevice"] --> D["IStreamTransport"]
  D --> E["TcpStreamTransport"] --> F["ConnectRetryExecutor"]
  D --> G["SerialStreamTransport"] --> F

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce parallel async interfaces (e.g., IAsyncDevice, ICancelableTransport)

• ➕ Avoids changing existing interfaces; no reliance on default interface implementations
• ➕ Clear opt-in surface for async/cancellation support
• ➖ More types and casting/adapter code for consumers
• ➖ Harder to keep sync/async parity; callers must choose the right interface

2. Extension-method async wrappers over existing sync APIs

• ➕ No interface changes; simplest for binary compatibility
• ➕ Low implementation risk
• ➖ Cannot truly cancel in-flight transport dials; cancellation becomes best-effort only
• ➖ Does not solve UI-thread blocking for Disconnect/Dispose without pushing work to Task.Run

3. Provide a separate cancellable connect entry via factory only

• ➕ Minimizes changes to device/transport APIs
• ➕ Keeps cancellation policy centralized
• ➖ Doesn’t help custom device/transport users calling Connect directly
• ➖ Disconnect/dispose blocking issues still need new lifecycle surface

Recommendation: Keep the PR’s approach: adding cancellable async members as default interface implementations provides real cancellation end-to-end while preserving source/binary compatibility for third-party transports/devices. The explicit semantic choice for DisconnectAsync token handling (shorten courtesy wait but never abort teardown) is a pragmatic safety trade-off, and the added tests meaningfully pin the tricky behaviors (caller cancel vs timeout, cancellation with zero backoff, and cleanup after partial connect).

Files changed (12) +1294 / -132

Enhancement (7) +531 / -106

ConnectRetryExecutor.csMake retry executor cancellation-aware (attempts + backoff) +31/-5

Make retry executor cancellation-aware (attempts + backoff)

• Extends the executor to accept a CancellationToken and pass it into connect attempts. Ensures cancellation is checked each loop iteration and during Task.Delay backoff, and treats cancellation as terminal (no retry) while still running failure cleanup and reporting disconnected status.

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

IStreamTransport.csAdd cancellable ConnectAsync overloads via default implementations +28/-0

Add cancellable ConnectAsync overloads via default implementations

• Adds 'ConnectAsync(CancellationToken)' and 'ConnectAsync(ConnectionRetryOptions?, CancellationToken)' with default implementations that forward to the preexisting uncancellable overload. This preserves compatibility for existing transport implementations while enabling true cancellation for updated transports.

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

SerialStreamTransport.csImplement cancellable serial connect with retry/backoff awareness +27/-2

Implement cancellable serial connect with retry/backoff awareness

• Routes existing ConnectAsync through a new token-aware overload and passes cancellation to the retry executor. Adds a pre-Open cancellation check to avoid starting uninterruptible SerialPort.Open when cancellation has already been requested.

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

TcpStreamTransport.csImplement cancellable TCP connect and distinguish timeout vs caller cancel +33/-8

Implement cancellable TCP connect and distinguish timeout vs caller cancel

• Adds token-aware ConnectAsync overloads and links the caller token with the per-attempt timeout so an in-flight dial can be abandoned promptly. Preserves existing behavior by surfacing timeouts as TimeoutException while propagating caller-driven cancellation as OperationCanceledException.

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

DaqifiDevice.csAdd ConnectAsync/DisconnectAsync and IAsyncDisposable to device lifecycle +351/-85

Add ConnectAsync/DisconnectAsync and IAsyncDisposable to device lifecycle

• Implements IAsyncDisposable and adds cancellable async connect/disconnect methods while keeping existing sync APIs unchanged. Refactors shared connect/disconnect steps (begin/complete/fail connect; coordinated teardown with text-exchange lock; shared pump stopping and resource release) and ensures canceled connect attempts close any partially opened transport.

src/Daqifi.Core/Device/DaqifiDevice.cs

DaqifiDeviceFactory.csThread cancellation through transport/device connect and async dispose on failure +5/-4

Thread cancellation through transport/device connect and async dispose on failure

• Updates factory connection pipeline to call the new token-aware transport connect and device ConnectAsync. Ensures failure cleanup uses DisposeAsync so cancellation/teardown does not block the calling thread.

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs

IDevice.csAdd default async lifecycle members to IDevice +56/-2

Add default async lifecycle members to IDevice

• Introduces 'ConnectAsync' and 'DisconnectAsync' as default interface implementations that fall back to the existing sync methods. Documents intended usage (prefer async on UI threads) and clarifies that cancellation for DisconnectAsync is not meant to abort teardown.

src/Daqifi.Core/Device/IDevice.cs

Tests (3) +722 / -7

ConnectRetryExecutorTests.csExtend retry executor tests for cancellation behavior +118/-7

Extend retry executor tests for cancellation behavior

• Updates connectAttempt delegate signature to accept a CancellationToken. Adds new tests ensuring canceled operations never retry, cancel is observed between attempts (including zero-backoff policies), and backoff delays are cancellable.

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

StreamTransportCancellationTests.csAdd transport-level cancellation contract tests +149/-0

Add transport-level cancellation contract tests

• Introduces new tests for TCP and serial transport ConnectAsync cancellation, including prompt cancellation mid-dial, token-already-canceled short-circuiting, and timeout vs cancellation disambiguation. Also verifies default interface implementation fallback for legacy transports.

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

DaqifiDeviceAsyncLifecycleTests.csAdd device async lifecycle + IAsyncDisposable tests +455/-0

Add device async lifecycle + IAsyncDisposable tests

• Adds comprehensive coverage for DaqifiDevice ConnectAsync/DisconnectAsync/DisposeAsync, including cleanup on canceled connect, cancellation-shortened disconnect wait behavior, and idempotent disposal. Includes parity checks to ensure existing sync Connect/Disconnect/Dispose semantics remain intact.

src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs

Documentation (2) +41 / -19

README.mdUpdate examples to use await using for async disposal +5/-5

Update examples to use await using for async disposal

• Switches documentation samples from 'using var' to 'await using var' for devices returned by async factory methods. This reflects the new IAsyncDisposable support and avoids blocking disposal on UI threads.

README.md

DEVICE_INTERFACES.mdDocument cancellable async lifecycle and non-blocking disposal +36/-14

Document cancellable async lifecycle and non-blocking disposal

• Updates snippets to use 'await using' and threads CancellationToken through manual transport/device setup examples. Adds a new section clarifying cancellation semantics for ConnectAsync vs DisconnectAsync and the guarantee that sync APIs remain unchanged.

docs/DEVICE_INTERFACES.md

[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. Dispose overlap race ✓ Resolved 🐞 Bug ☼ Reliability

Description

DaqifiDevice.Dispose() and DisposeAsync() both gate on a non-atomic _disposed flag that is only
set at the end of ReleaseResources(), so the two methods can overlap and both proceed with teardown.
This violates the new documented guarantee that mixing Dispose and DisposeAsync makes the second
call a no-op, and can result in double-disconnect/double-dispose races and inconsistent state
transitions.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1667-1713]

        public void Dispose()
        {
-            if (!_disposed)
+            if (_disposed)
            {
-                Disconnect();
-                _messageConsumer?.Dispose();
-                _messageProducer?.Dispose();
-                _transport?.Dispose();
-                _textExchangeLock.Dispose();
-                _disposed = true;
+                return;
            }
+
+            Disconnect();
+            ReleaseResources();
+        }
+
+        /// <summary>
+        /// Disposes the device and releases resources without blocking the calling thread, so
+        /// <c>await using var device = ...</c> is safe on a UI thread.
+        /// </summary>
+        /// <remarks>
+        /// Equivalent to <see cref="Dispose"/> except that the disconnect it performs first runs
+        /// through <see cref="DisconnectAsync"/>. Safe to call more than once, and safe to mix with
+        /// <see cref="Dispose"/> — whichever runs first wins and the other becomes a no-op.
+        /// </remarks>
+        /// <returns>A task representing the asynchronous dispose operation.</returns>
+        public async ValueTask DisposeAsync()
+        {
+            if (_disposed)
+            {
+                return;
+            }
+
+            await DisconnectAsync().ConfigureAwait(false);
+            ReleaseResources();
+        }
+
+        /// <summary>
+        /// Releases everything the device owns once it is already disconnected. Shared tail of
+        /// <see cref="Dispose"/> and <see cref="DisposeAsync"/>.
+        /// </summary>
+        /// <remarks>
+        /// The transport is already closed by the preceding disconnect, so
+        /// <see cref="IDisposable.Dispose"/> on it does not block here.
+        /// </remarks>
+        private void ReleaseResources()
+        {
+            _messageConsumer?.Dispose();
+            _messageProducer?.Dispose();
+            _transport?.Dispose();
+            _textExchangeLock.Dispose();
+            _disposed = true;

Relevance

●●● Strong

Matches accepted precedent: dispose flags should be atomic/published early to avoid teardown races.

PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new DisposeAsync/ReleaseResources logic checks _disposed before teardown, then awaits and
disposes resources, and only then sets _disposed = true, which permits concurrent entry into both
Dispose and DisposeAsync. A similar previously-accepted bug pattern in the codebase shows this
should be gated atomically and published early.

src/Daqifi.Core/Device/DaqifiDevice.cs[332-341]
src/Daqifi.Core/Device/DaqifiDevice.cs[1667-1713]
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
`Dispose()` and `DisposeAsync()` are documented as interchangeable (“whichever runs first wins and the other becomes a no-op”), but `_disposed` is a plain `bool` checked before teardown and only set after resource disposal. If `DisposeAsync()` starts and awaits `DisconnectAsync()`, a concurrent `Dispose()` can still enter and perform a second teardown.

### Issue Context
This PR adds `IAsyncDisposable` and encourages `await using`, increasing the likelihood of overlapping disposal paths (e.g., user code calls `Dispose()` while an `await using` scope is unwinding, or two shutdown paths race).

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1667-1713]
- src/Daqifi.Core/Device/DaqifiDevice.cs[332-341]

### Suggested fix
Implement an atomic, early-published disposal gate, and make both disposal entry points share the same underlying operation:
- Replace `private bool _disposed;` with an atomic state (e.g., `private int _disposeState;`) and use `Interlocked.Exchange`/`CompareExchange` at the *start* of `Dispose()` and `DisposeAsync()`.
- Prefer a shared `Task _disposeTask` pattern:
 - `DisposeAsync()` returns/awaits the shared task.
 - `Dispose()` synchronously waits (`GetAwaiter().GetResult()`) on the same shared task.
This preserves the “whichever starts first wins” behavior while preventing double-teardown.

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

---

2. Flaky timing-based test gate ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect hard-fails when the reconnect gap exceeds
DeviceErrorThrottle.DefaultInterval, so a slow/contended run can fail even if ConnectAsync correctly
resets the throttle. This introduces CI flakiness unrelated to the implementation under test.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[R412-421]

+        // Only reachable with the count clean. If the two errors still fell outside one throttle
+        // window, the machine was slow enough that the second raise was due anyway — the run
+        // happens to agree with the guard without having exercised it. Fail loudly rather than
+        // bank a pass that proves nothing.
+        Assert.True(
+            gap < DeviceErrorThrottle.DefaultInterval,
+            $"Inconclusive run: {gap.TotalSeconds:0.##}s separated the two errors, but the throttle "
+            + $"window is {DeviceErrorThrottle.DefaultInterval.TotalSeconds:0.##}s. The second raise "
+            + "would have been due even without the reset, so this run cannot distinguish the two. "
+            + "Failing rather than reporting a pass that guards nothing.");

Relevance

●●● Strong

Team often fixes CI-flaky timing assertions in tests; prefers deterministic synchronization or
relaxed timing guards.

PR-#226
PR-#104
PR-#237

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test unconditionally fails when the measured reconnect gap exceeds the throttle window, which is
5 seconds by default; this can happen due to host scheduling/contended CI rather than a regression
in throttle reset behavior.

src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[412-421]
src/Daqifi.Core/Device/DeviceErrorThrottle.cs[38-49]

Agent prompt

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

### Issue description
`ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect` currently treats a slow run (gap >= throttle interval) as a test failure, even though it may simply be an “inconclusive” timing outcome on a busy machine. This can create intermittent CI failures unrelated to production correctness.

### Issue Context
The test’s main proof is `SuppressedCount == 0` on the *first* error in the second session, but the test doesn’t deterministically ensure the first session actually accumulated any suppressed occurrences in the bucket (it flips `FailReads` off immediately after the first raise). The added wall-clock `gap < DefaultInterval` assertion is compensating for that, at the cost of flakiness.

### Fix approach (deterministic)
- Keep `FailReads = true` long enough in session one to guarantee at least one additional failure occurred after the first raise (so the throttle bucket’s suppressed counter becomes non-zero if it isn’t reset).
- A deterministic way is to use the scripted stream’s `ReadCount` (already available) to wait until at least N reads occurred while failing.
- Once you can guarantee suppression was possible, remove (or relax/remove-fail) the `gap < DefaultInterval` assertion.

Example sketch:
- Record `var startReads = transport.ScriptedStream.ReadCount;`
- After `firstSessionRaised` is set, wait until `transport.ScriptedStream.ReadCount >= startReads + 3` (with a reasonable timeout like 1s).
- Then proceed to quiesce/disconnect.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[375-386]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[412-421]

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

---

3. Throttle reset test weak ✓ Resolved 🐞 Bug ☼ Reliability

Description

ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect can still pass even if the throttle reset
regresses, because it doesn’t bound the elapsed time between the first and second error below the 5s
throttle interval. This weakens the regression guard and can let a future ConnectAsync refactor drop
the reset without failing CI on slower runs.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[R324-357]

+    [Fact]
+    public async Task ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect()
+    {
+        // The throttle collapses repeats of the same (source, exception type) for five seconds.
+        // A reconnect is a new session and must report its first failure at once — so the second
+        // session's error has to arrive well inside that five-second window to prove the reset ran.
+        using var transport = new ScriptedErrorTransport();
+        using var device = new DaqifiDevice("Erroring Device", transport);
+
+        var raises = 0;
+        var raised = new ManualResetEventSlim(false);
+        device.ErrorOccurred += (_, _) =>
+        {
+            Interlocked.Increment(ref raises);
+            raised.Set();
+        };
+
+        await device.ConnectAsync();
+        transport.ScriptedStream.FailReads = true;
+        Assert.True(raised.Wait(TimeSpan.FromSeconds(10)), "the first session never reported an error.");
+
+        transport.ScriptedStream.FailReads = false;
+        await device.DisconnectAsync();
+
+        raised.Reset();
+        var raisesAfterFirstSession = Volatile.Read(ref raises);
+
+        await device.ConnectAsync();
+        transport.ScriptedStream.FailReads = true;
+
+        Assert.True(raised.Wait(TimeSpan.FromSeconds(3)),
+            "the reconnected session's first failure was collapsed into the previous session's "
+            + "throttle window — ConnectAsync did not reset the error throttle.");
+        Assert.True(Volatile.Read(ref raises) > raisesAfterFirstSession);

Relevance

●●● Strong

Team often strengthens regression tests with explicit bounds/preconditions to prevent silent
coverage loss/flaky hangs.

PR-#197
PR-#198

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test gives itself up to 10 seconds to observe the first error and does not measure how long
disconnect+reconnect takes before waiting for the second error, so the second error may occur after
the throttle’s 5-second interval has already elapsed and would therefore be raised even without a
reset.

src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[324-357]
src/Daqifi.Core/Device/DeviceErrorThrottle.cs[40-48]

Agent prompt

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

### Issue description
`ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect` is intended to prove that reconnect resets the error throttle (interval is 5s), but the test never asserts that the second error happens within that 5s window relative to the first error.

### Issue Context
The test currently waits up to 10s for the first error, then performs disconnect/reconnect work, then waits up to 3s for the second error. If the time between the two error events exceeds the throttle interval (5s), the second error would be raised even without calling `Reset()`, so the test may pass without actually validating the behavior.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[324-357]

### Suggested fix
- Record a timestamp for the *first* `ErrorOccurred` (e.g., `Stopwatch.GetTimestamp()` or a `Stopwatch` started at first raise).
- When the second error arrives, assert the elapsed time since the first raise is **< 5 seconds** (preferably with some margin, e.g. < 2 seconds).
- Optionally reduce the first wait budget and/or explicitly assert reconnect completes quickly enough that the second raise is forced to occur inside the throttle interval.

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

---

4. Transport cancel precheck missing ✓ Resolved 🐞 Bug ≡ Correctness

Description

The default interface implementation of IStreamTransport.ConnectAsync(ConnectionRetryOptions?,
CancellationToken) ignores the CancellationToken entirely, so even an already-canceled token can
still start a connect attempt for legacy/custom transports. This contradicts the new cancellation
contract and can cause unwanted side effects (e.g., opening a socket/port) when callers expect
immediate cancellation.

Code

src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[R54-73]

+    Task ConnectAsync(CancellationToken cancellationToken) => ConnectAsync(null, cancellationToken);
+
+    /// <summary>
+    /// Establishes the transport connection with retry support, abandoning the attempt if
+    /// <paramref name="cancellationToken"/> is signalled — including while waiting out the
+    /// backoff delay between retries.
+    /// </summary>
+    /// <remarks>
+    /// This is the cancellable form of <see cref="ConnectAsync(ConnectionRetryOptions?)"/>. It has a
+    /// default implementation that simply forwards to the uncancellable overload, so an existing
+    /// <see cref="IStreamTransport"/> implementation keeps compiling and working unchanged — it just
+    /// cannot honor the token. Implementations that can abandon an in-flight attempt should override
+    /// this member; the transports shipped in daqifi-core do.
+    /// </remarks>
+    /// <param name="retryOptions">Configuration for retry behavior. If null, uses default single attempt.</param>
+    /// <param name="cancellationToken">A cancellation token to observe while connecting.</param>
+    /// <returns>A task representing the asynchronous connect operation.</returns>
+    /// <exception cref="OperationCanceledException">Thrown when the attempt is canceled.</exception>
+    Task ConnectAsync(ConnectionRetryOptions? retryOptions, CancellationToken cancellationToken) =>
+        ConnectAsync(retryOptions);

Relevance

●●● Strong

Team often adds ThrowIfCancellationRequested prechecks to avoid side effects when token already
canceled.

PR-#381
PR-#320

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The interface XML docs state the connect attempt is abandoned if the token is signalled and
documents OperationCanceledException, but the default bodies simply forward to the uncancellable
overload and never read the token, so a canceled token won’t stop anything by default.

src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[47-73]

Agent prompt

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

### Issue description
`IStreamTransport.ConnectAsync(ConnectionRetryOptions?, CancellationToken)` has a default implementation for backward compatibility, but it currently does **not** check `cancellationToken` at all. As a result, `ConnectAsync(token)` / `ConnectAsync(retry, token)` can begin connecting even when the token is already canceled.

### Issue Context
This PR introduces cancellable connect semantics across the stack. Even if legacy transports cannot abandon an in-flight dial, the default implementations should at least honor *pre-cancellation* (consistent with `IDevice.ConnectAsync`’s default implementation).

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[47-73]

### Suggested fix
- Update the default implementation of `ConnectAsync(ConnectionRetryOptions? retryOptions, CancellationToken cancellationToken)` to:
 - call `cancellationToken.ThrowIfCancellationRequested();` before forwarding to the uncancellable overload.
- Optionally add a unit test similar to `LegacyTransport_...` that asserts a pre-canceled token throws and does not connect.

ⓘ 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 1895c1e

Results up to commit 52c99e5 ⚖️ Balanced

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

1. Dispose overlap race ✓ Resolved 🐞 Bug ☼ Reliability

Description

DaqifiDevice.Dispose() and DisposeAsync() both gate on a non-atomic _disposed flag that is only
set at the end of ReleaseResources(), so the two methods can overlap and both proceed with teardown.
This violates the new documented guarantee that mixing Dispose and DisposeAsync makes the second
call a no-op, and can result in double-disconnect/double-dispose races and inconsistent state
transitions.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1667-1713]

        public void Dispose()
        {
-            if (!_disposed)
+            if (_disposed)
            {
-                Disconnect();
-                _messageConsumer?.Dispose();
-                _messageProducer?.Dispose();
-                _transport?.Dispose();
-                _textExchangeLock.Dispose();
-                _disposed = true;
+                return;
            }
+
+            Disconnect();
+            ReleaseResources();
+        }
+
+        /// <summary>
+        /// Disposes the device and releases resources without blocking the calling thread, so
+        /// <c>await using var device = ...</c> is safe on a UI thread.
+        /// </summary>
+        /// <remarks>
+        /// Equivalent to <see cref="Dispose"/> except that the disconnect it performs first runs
+        /// through <see cref="DisconnectAsync"/>. Safe to call more than once, and safe to mix with
+        /// <see cref="Dispose"/> — whichever runs first wins and the other becomes a no-op.
+        /// </remarks>
+        /// <returns>A task representing the asynchronous dispose operation.</returns>
+        public async ValueTask DisposeAsync()
+        {
+            if (_disposed)
+            {
+                return;
+            }
+
+            await DisconnectAsync().ConfigureAwait(false);
+            ReleaseResources();
+        }
+
+        /// <summary>
+        /// Releases everything the device owns once it is already disconnected. Shared tail of
+        /// <see cref="Dispose"/> and <see cref="DisposeAsync"/>.
+        /// </summary>
+        /// <remarks>
+        /// The transport is already closed by the preceding disconnect, so
+        /// <see cref="IDisposable.Dispose"/> on it does not block here.
+        /// </remarks>
+        private void ReleaseResources()
+        {
+            _messageConsumer?.Dispose();
+            _messageProducer?.Dispose();
+            _transport?.Dispose();
+            _textExchangeLock.Dispose();
+            _disposed = true;

Relevance

●●● Strong

Matches accepted precedent: dispose flags should be atomic/published early to avoid teardown races.

PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new DisposeAsync/ReleaseResources logic checks _disposed before teardown, then awaits and
disposes resources, and only then sets _disposed = true, which permits concurrent entry into both
Dispose and DisposeAsync. A similar previously-accepted bug pattern in the codebase shows this
should be gated atomically and published early.

src/Daqifi.Core/Device/DaqifiDevice.cs[332-341]
src/Daqifi.Core/Device/DaqifiDevice.cs[1667-1713]
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
`Dispose()` and `DisposeAsync()` are documented as interchangeable (“whichever runs first wins and the other becomes a no-op”), but `_disposed` is a plain `bool` checked before teardown and only set after resource disposal. If `DisposeAsync()` starts and awaits `DisconnectAsync()`, a concurrent `Dispose()` can still enter and perform a second teardown.

### Issue Context
This PR adds `IAsyncDisposable` and encourages `await using`, increasing the likelihood of overlapping disposal paths (e.g., user code calls `Dispose()` while an `await using` scope is unwinding, or two shutdown paths race).

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1667-1713]
- src/Daqifi.Core/Device/DaqifiDevice.cs[332-341]

### Suggested fix
Implement an atomic, early-published disposal gate, and make both disposal entry points share the same underlying operation:
- Replace `private bool _disposed;` with an atomic state (e.g., `private int _disposeState;`) and use `Interlocked.Exchange`/`CompareExchange` at the *start* of `Dispose()` and `DisposeAsync()`.
- Prefer a shared `Task _disposeTask` pattern:
 - `DisposeAsync()` returns/awaits the shared task.
 - `Dispose()` synchronously waits (`GetAwaiter().GetResult()`) on the same shared task.
This preserves the “whichever starts first wins” behavior while preventing double-teardown.

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

---

2. Transport cancel precheck missing ✓ Resolved 🐞 Bug ≡ Correctness

Description

The default interface implementation of IStreamTransport.ConnectAsync(ConnectionRetryOptions?,
CancellationToken) ignores the CancellationToken entirely, so even an already-canceled token can
still start a connect attempt for legacy/custom transports. This contradicts the new cancellation
contract and can cause unwanted side effects (e.g., opening a socket/port) when callers expect
immediate cancellation.

Code

src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[R54-73]

+    Task ConnectAsync(CancellationToken cancellationToken) => ConnectAsync(null, cancellationToken);
+
+    /// <summary>
+    /// Establishes the transport connection with retry support, abandoning the attempt if
+    /// <paramref name="cancellationToken"/> is signalled — including while waiting out the
+    /// backoff delay between retries.
+    /// </summary>
+    /// <remarks>
+    /// This is the cancellable form of <see cref="ConnectAsync(ConnectionRetryOptions?)"/>. It has a
+    /// default implementation that simply forwards to the uncancellable overload, so an existing
+    /// <see cref="IStreamTransport"/> implementation keeps compiling and working unchanged — it just
+    /// cannot honor the token. Implementations that can abandon an in-flight attempt should override
+    /// this member; the transports shipped in daqifi-core do.
+    /// </remarks>
+    /// <param name="retryOptions">Configuration for retry behavior. If null, uses default single attempt.</param>
+    /// <param name="cancellationToken">A cancellation token to observe while connecting.</param>
+    /// <returns>A task representing the asynchronous connect operation.</returns>
+    /// <exception cref="OperationCanceledException">Thrown when the attempt is canceled.</exception>
+    Task ConnectAsync(ConnectionRetryOptions? retryOptions, CancellationToken cancellationToken) =>
+        ConnectAsync(retryOptions);

Relevance

●●● Strong

Team often adds ThrowIfCancellationRequested prechecks to avoid side effects when token already
canceled.

PR-#381
PR-#320

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The interface XML docs state the connect attempt is abandoned if the token is signalled and
documents OperationCanceledException, but the default bodies simply forward to the uncancellable
overload and never read the token, so a canceled token won’t stop anything by default.

src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[47-73]

Agent prompt

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

### Issue description
`IStreamTransport.ConnectAsync(ConnectionRetryOptions?, CancellationToken)` has a default implementation for backward compatibility, but it currently does **not** check `cancellationToken` at all. As a result, `ConnectAsync(token)` / `ConnectAsync(retry, token)` can begin connecting even when the token is already canceled.

### Issue Context
This PR introduces cancellable connect semantics across the stack. Even if legacy transports cannot abandon an in-flight dial, the default implementations should at least honor *pre-cancellation* (consistent with `IDevice.ConnectAsync`’s default implementation).

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[47-73]

### Suggested fix
- Update the default implementation of `ConnectAsync(ConnectionRetryOptions? retryOptions, CancellationToken cancellationToken)` to:
 - call `cancellationToken.ThrowIfCancellationRequested();` before forwarding to the uncancellable overload.
- Optionally add a unit test similar to `LegacyTransport_...` that asserts a pre-canceled token throws and does not connect.

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

---

Results up to commit 91c6e03 ⚖️ Balanced

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

1. Throttle reset test weak ✓ Resolved 🐞 Bug ☼ Reliability

Description

ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect can still pass even if the throttle reset
regresses, because it doesn’t bound the elapsed time between the first and second error below the 5s
throttle interval. This weakens the regression guard and can let a future ConnectAsync refactor drop
the reset without failing CI on slower runs.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[R324-357]

+    [Fact]
+    public async Task ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect()
+    {
+        // The throttle collapses repeats of the same (source, exception type) for five seconds.
+        // A reconnect is a new session and must report its first failure at once — so the second
+        // session's error has to arrive well inside that five-second window to prove the reset ran.
+        using var transport = new ScriptedErrorTransport();
+        using var device = new DaqifiDevice("Erroring Device", transport);
+
+        var raises = 0;
+        var raised = new ManualResetEventSlim(false);
+        device.ErrorOccurred += (_, _) =>
+        {
+            Interlocked.Increment(ref raises);
+            raised.Set();
+        };
+
+        await device.ConnectAsync();
+        transport.ScriptedStream.FailReads = true;
+        Assert.True(raised.Wait(TimeSpan.FromSeconds(10)), "the first session never reported an error.");
+
+        transport.ScriptedStream.FailReads = false;
+        await device.DisconnectAsync();
+
+        raised.Reset();
+        var raisesAfterFirstSession = Volatile.Read(ref raises);
+
+        await device.ConnectAsync();
+        transport.ScriptedStream.FailReads = true;
+
+        Assert.True(raised.Wait(TimeSpan.FromSeconds(3)),
+            "the reconnected session's first failure was collapsed into the previous session's "
+            + "throttle window — ConnectAsync did not reset the error throttle.");
+        Assert.True(Volatile.Read(ref raises) > raisesAfterFirstSession);

Relevance

●●● Strong

Team often strengthens regression tests with explicit bounds/preconditions to prevent silent
coverage loss/flaky hangs.

PR-#197
PR-#198

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test gives itself up to 10 seconds to observe the first error and does not measure how long
disconnect+reconnect takes before waiting for the second error, so the second error may occur after
the throttle’s 5-second interval has already elapsed and would therefore be raised even without a
reset.

src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[324-357]
src/Daqifi.Core/Device/DeviceErrorThrottle.cs[40-48]

Agent prompt

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

### Issue description
`ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect` is intended to prove that reconnect resets the error throttle (interval is 5s), but the test never asserts that the second error happens within that 5s window relative to the first error.

### Issue Context
The test currently waits up to 10s for the first error, then performs disconnect/reconnect work, then waits up to 3s for the second error. If the time between the two error events exceeds the throttle interval (5s), the second error would be raised even without calling `Reset()`, so the test may pass without actually validating the behavior.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[324-357]

### Suggested fix
- Record a timestamp for the *first* `ErrorOccurred` (e.g., `Stopwatch.GetTimestamp()` or a `Stopwatch` started at first raise).
- When the second error arrives, assert the elapsed time since the first raise is **< 5 seconds** (preferably with some margin, e.g. < 2 seconds).
- Optionally reduce the first wait budget and/or explicitly assert reconnect completes quickly enough that the second raise is forced to occur inside the throttle interval.

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

---

[tylerkron]

Both Qodo findings addressed in 66d2ff5 — see the inline replies for the reasoning on each.

1. Transport cancel precheck missing — real defect, fixed. The default IStreamTransport.ConnectAsync(retryOptions, token) now calls ThrowIfCancellationRequested() before delegating, matching IDevice's default shim. On this hardware a serial open pulses DTR and resets the MCU, so declining to start is worth it even though a legacy transport still cannot abandon a dial already in flight.

2. Dispose overlap race — real defect, fixed. An interlocked claim taken at the start of disposal replaces the flag published at the end. I did not adopt the suggested shared-Task variant: having Dispose() block on an in-flight DisposeAsync() would reintroduce on the calling thread exactly the stall this PR removes. The redundant caller returns immediately, which is the normal .NET contract. Teardown also moved into a finally, so a throwing disconnect no longer leaks handles on a device that can never be disposed again.

Three regression tests added, each verified to fail against the pre-fix code.

Tests: Release build 0 warnings. Full suite green on net9.0 and net10.0 — 2219 passed, 2 skipped, 0 failed on each. MCP suite 23/23.

Bench re-run (Nyquist 1, FW 3.7.2, USB only — WiFi skipped to avoid connect churn on a shared unit). The disposal path is what the bench measures, so it was worth re-confirming even though both fixes are pure guard/synchronization changes with no wire-level effect:

Check	Result
USB await using — stream 5 s @ 100 Hz, async dispose	396 analog messages; disposal 196 ms (was 199 ms)
USB sync Connect/Disconnect/Dispose	32 channels, 317 messages in 4 s; teardown 194 ms (was 202 ms)
Cancelled connect to unreachable IP (60 s timeout, 5 attempts)	OperationCanceledException after 513 ms

Unchanged from the pre-fix bench within noise.

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 66d2ff5

[tylerkron]

Merged main in twice — #415/#417 first, then #414 which landed mid-resolution. Now MERGEABLE. Hand-resolved, so flagging what needed judgement:

Conflicts (4)

Where	Resolution
IDevice.cs	#415's ErrorOccurred event landed immediately before Connect(), whose doc comment this branch rewrote. Kept both.
DaqifiDevice.Connect()	#415's consumer ErrorOccurred subscribe moved into the shared CompleteConnect(), so the async path wires it too.
DaqifiDevice.Disconnect()	#415's ErrorOccurred unsubscribe moved into the shared StopMessagePumps(), reached by both Disconnect() and DisconnectAsync().
docs/DEVICE_INTERFACES.md	#414 added a PreserveActiveStream note to the same Advanced snippet this branch had switched to the cancellable calls. Kept both.

The one that would have slipped through. _errorThrottle.Reset() — #415's per-session reset — sat inside a conflicted hunk, and git's auto-merge left it in Connect() only. The factory now connects via ConnectAsync, so that would have silently dropped it from the primary connect path. I moved it into the shared BeginConnect().

Worth stating plainly: nothing in #415's suite covers that reset. I verified by deleting it outright — all 23 of #415's error-surface/throttle/loss-escalation tests still passed. It would have shipped green.

Two regression tests added for that seam, since every #415 test drives Connect() (ConnectAsync() didn't exist when they were written):

• ConnectAsync_WiresUpTheBackgroundErrorSurface
• ConnectAsync_OpensAFreshErrorThrottleSessionOnReconnect

Both verified to fail when the connect-side wiring is dropped, and to pass with it.

Verified unchanged from main: OnTransportStatusChanged (byte-identical, so #415's Lost escalation and the _isDisconnecting guard are untouched), #417's finalizeAsync plumbing, and #414's PreserveActiveStream — which in the factory still runs before the connect call, preserving its "never observed half-applied" invariant now that the call is ConnectAsync. I also audited every line this branch removes relative to main; all of them are lines relocated into the shared step helpers, none dropped.

Tests: Release 0 warnings. 2276 passed / 2 skipped / 0 failed on both net9.0 and net10.0. MCP 23/23. No bench re-run — the merge added no wire-level change beyond what #414/#415/#417 already benched.

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 91c6e03

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 17f39a6

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1895c1e