feat(device): opt-in auto-reconnect and streaming session resume after a drop (closes #379)

[tylerkron]

Why

When the cable blips at hour six of an overnight log, the session is over. The library notices the
drop and says so — and that is all it has ever done. Getting the run back means reconnecting the
transport, re-initializing the device, re-enabling every channel and restarting the stream, all by
hand, in an event handler, on a background thread. Nobody does that, so nobody recovers: the desktop
app just tears down and gives up.

This makes recovery something you turn on rather than something you write.

What

Set device.ReconnectOptions and a dropped connection rebuilds itself: the transport reconnects,
the device re-initializes, the channels you had enabled come back on, and a stream that was running
starts again — at the same rate, with no code from you.

It is off by default, and off means exactly what happens today: the drop is reported as
ConnectionStatus.Lost and nothing else changes.

While it works you can watch it. ReconnectAttempt says which attempt is coming and how long the
wait is, Reconnected says it worked and how long the outage lasted, ReconnectFailed says it
stopped. The connection status follows along too — Retrying between attempts, Failed if it gives
up — so an existing status-driven UI shows progress without subscribing to anything new.

Giving up is deliberately loud: as well as the event and the Failed status, it is logged as an
error and raised on the ErrorOccurred event added by #415.

CancelReconnect() stops it. So do Disconnect() and Dispose() — a caller always wins, including
one who disconnects from inside their own Lost handler.

How

A drop takes a snapshot of the session before anyone else sees it, because a consumer's own handler
is free to start tearing the device down, and by then there is nothing left to remember. A
background loop then works through the attempts, backing off a little further each time, tearing the
dead session down before each try so nothing is left bound to a stream that no longer exists.

Restoring the channels has to replay that snapshot rather than read the channel objects, because a
reconnected device reports its own enabled mask and overwrites what the library thought was on. That
is also what makes the test meaningful: the scripted device comes back reporting everything
disabled, exactly as a rebooted one does, so a restored channel set can only have been re-sent.

A resumed stream is a genuinely new session — timestamps re-anchor and the gap detector resets —
because the device's clock may well have restarted while it was away, and carrying the old anchor
across would invent a gap that never happened. Reconnected.Outage is the measure of the outage.

Design decisions

On the device, not a session wrapper. The issue offered either. A wrapper would have needed the
private status setter, the protected error-raising hook and the device's own text-exchange lock all
widened to public just to do its job — more API surface, for a thing that only ever wraps one class.
The mechanism sits on DaqifiDevice, and DaqifiStreamingDevice overrides two small hooks to
capture and restore its own session state, which is the pattern OnDeviceInitializingAsync already
uses.

No new ConnectionStatus value. Retrying ("retrying connection after a failure") and Failed
("failed after all retry attempts") already existed and were unused. Adding Reconnecting beside
Retrying would have been two names for one state.

Not on IDevice. #415 already made every direct implementer add a member; five more would be
worse, and the factory hands back a DaqifiDevice, so consumers reach all of this anyway.

Interaction with #341 (cancellable async connect/disconnect + IAsyncDisposable, being
implemented separately). Nothing here changes Connect(), Disconnect(), Dispose() or
IStreamTransport.ConnectAsync — the loop is built on the API as it stands. It already takes a
CancellationToken throughout and unwinds at documented checkpoints, so when #341 lands the loop's
internal ConnectCore()/DisconnectCore() calls become awaits of the async versions, and
DisposeAsync gets to await the loop rather than leaving it to notice on its own. The
CancelReconnect() / session-epoch handshake is what makes that a swap rather than a redesign.

Interaction with #342 (per-device operation serialization). Not solved here, and not attempted.
What the loop gets for free: re-initialization runs through ExecuteTextCommandAsync, which takes
the same device-wide text-exchange lock every user SCPI operation takes, so a reconnect cannot
interleave bytes on the wire with a text command — and an operation in flight when the drop happens
is what Disconnect()'s existing lock wait already covers. What it does not get is exclusion from
fire-and-forget Send-only calls (EnableChannel, SetDioValue) during the restore window, which
is precisely #342's job. Two things narrow that window meanwhile: the device is not Connected for
most of a reconnect, so those calls fail fast exactly as they do today after Lost; and a
caller-issued Connect() or Disconnect() supersedes the loop outright via a session epoch, so the
loop can never re-open a transport the caller just closed. When #342 lands, reconnect takes that
lock as a privileged holder and the window closes.

What is deliberately not restored. Only what the library itself owns comes back — the enabled
channel set, the streaming frequency, and an active stream. Everything else is the device's own
state, and after an outage of unknown length Core does not presume to know what it should be: DIO
directions and output levels, PWM, analog outputs, RAM-only calibration, and an SD logging session
are all left alone. Any operation in flight fails — an SD download interrupted by a drop is
neither resumed nor retried; run it again once Reconnected fires.

Same endpoint only. Reconnection re-opens the endpoint already in use. A serial device that
comes back on a different port path, or one whose IP moved after a reboot, is a new endpoint and
needs a fresh factory connect. Cross-transport failover (USB→WiFi) is explicitly out of scope.

Testing

• Full suite green on net9.0 and net10.0 (2242 passing each), plus the MCP tests; Release build with
  zero warnings.
• 46 new tests. A scripted transport that can be dropped on command and told to refuse the next N
  reconnects covers resume, retry-until-it-answers, both give-up paths (transport and
  initialization), cancellation, Disconnect() mid-reconnect, disposal mid-reconnect, a throwing
  subscriber, a second drop after a recovery, and — byte for byte — that a device with reconnect left
  at its default still produces exactly one status transition, to Lost, and nothing else. One test
  goes the long way round, driving real read failures through the production watchdog.
• A self-review before opening this found two genuine bugs, both fixed in the second commit: a device
  restored with ResumeStreaming off reported IsStreaming from before the drop (which also made
  the caller's own StartStreaming() a silent no-op), and a consumer tearing down inside their own
  Lost handler got a reconnect started behind them anyway.
• Bench (Nyquist 1, FW 3.7.2), non-regression only: 20 s streams over USB and WiFi, each run twice —
  once with reconnect at its default, once with it enabled and no drop occurring. Identical sample
  counts in both modes on both transports (1586 USB, 1585 WiFi), zero reconnect events, zero errors,
  and Disconnect() still reporting Disconnected rather than Lost. Device still answering on the
  network afterwards.

Still to verify by hand, since it needs someone to actually touch the hardware: a physical mid-stream
USB unplug and replug, and a real TCP link drop.

closes #379

Not merging — for review. Stacked on #415.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Opt-in auto-reconnect with session restore + surfaced background errors

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

AI Description

• Add opt-in auto-reconnect that reinitializes, restores channels, and resumes streaming.
• Surface background read/parse/decode/reconnect failures via throttled ErrorOccurred diagnostics.
• Fix StreamMessageConsumer silent-spin paths and ensure loss escalation stays reliable.

Diagram

graph TD
A["App / SDK user"] --> B["DaqifiDevice"] --> C["IStreamTransport"] --> D["StreamMessageConsumer"]
D --> B --> E["ErrorOccurred"]
B --> F["Reconnect loop"] --> C --> G["Init + restore"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Transport-level reconnect only

• ➕ Keeps reconnect logic closer to the underlying connection mechanics
• ➕ Potentially reusable across multiple device types
• ➖ Cannot restore device-owned session state (channels/stream) without coupling back to device logic
• ➖ Harder to provide coherent device-level events/status semantics (Retrying/Failed) tied to restoration completion

2. Expose a reconnection hook and let consumers restore state

• ➕ Simpler core library; app can tailor restoration strategy
• ➕ Avoids library needing to interpret sent commands/session state
• ➖ Reintroduces the original problem (every consumer must reimplement complex recovery correctly)
• ➖ Risk of inconsistent behavior across apps and subtle races in user handlers

3. Formal state machine (explicit lifecycle states + transitions)

• ➕ Clearer invariants for concurrency, cancellation, and re-entrancy
• ➕ Potentially easier long-term maintenance as more lifecycle features accumulate
• ➖ Higher upfront refactor cost; larger diff and migration risk
• ➖ Not necessary if current locking/epoch strategy proves robust

Recommendation: Keep the PR’s device-level, opt-in reconnect with session snapshot/restore: it addresses the real usability gap (automatic recovery) while preserving existing default behavior. Transport-only reconnect or user-hook approaches either can’t restore session state or push complexity back to consumers; a full state machine could be a future refactor if lifecycle logic continues to grow.

Files changed (21) +6116 / -23

Enhancement (11) +2090 / -6

DaqifiDevice.csAdd throttled ErrorOccurred + opt-in reconnect loop with lifecycle safety +996/-1

Add throttled ErrorOccurred + opt-in reconnect loop with lifecycle safety

• Introduces a device-level ErrorOccurred diagnostics event with per-(source, exception type) throttling and safe isolation from throwing handlers. Adds ReconnectOptions-driven automatic reconnect after ConnectionStatus.Lost, including session-epoch supersession rules, cancellation support, reconnect progress events, and terminal give-up surfaced via ErrorOccurred + Failed status; also adds a lifecycle lock to prevent concurrent connect/disconnect races.

src/Daqifi.Core/Device/DaqifiDevice.cs

DaqifiStreamingDevice.csTrack session state and support stream/channel restoration after reconnect +397/-5

Track session state and support stream/channel restoration after reconnect

• Adds per-session decode failure counting and centralizes streaming-session initialization so timestamp/gap detection resets correctly. Tracks key SCPI commands sent via Send<T>() (start/stop streaming and ADC enable mask) so sessions driven by raw commands can be restored after reconnect, aligning streaming state with what the device is actually doing.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

DeviceErrorEventArgs.csIntroduce event args for background error reporting +84/-0

Introduce event args for background error reporting

• Adds DeviceErrorEventArgs to carry source classification, exception, suppressed-count metadata, optional raw bytes, and a timestamp for device background failures.

src/Daqifi.Core/Device/DeviceErrorEventArgs.cs

DeviceErrorSource.csAdd error source classification enum +47/-0

Add error source classification enum

• Adds DeviceErrorSource to distinguish message-consumer failures, stream decode failures, and terminal reconnect give-up for diagnostics and throttling buckets.

src/Daqifi.Core/Device/DeviceErrorSource.cs

DeviceErrorThrottle.csAdd throttling utility for repeated background failures +172/-0

Add throttling utility for repeated background failures

• Implements a bounded, per-bucket throttle to collapse repeated background failures and report suppressed counts, preventing kHz-rate error storms from overwhelming subscribers while keeping first occurrence immediate.

src/Daqifi.Core/Device/DeviceErrorThrottle.cs

DeviceReconnectFailedException.csAdd typed exception for terminal reconnect failure +35/-0

Add typed exception for terminal reconnect failure

• Introduces a non-thrown exception type used as the payload for ErrorOccurred when automatic reconnect exhausts all attempts, carrying attempts made and the final inner exception.

src/Daqifi.Core/Device/DeviceReconnectFailedException.cs

IDevice.csExtend IDevice with ErrorOccurred event +11/-0

Extend IDevice with ErrorOccurred event

• Adds ErrorOccurred to the device interface so consumers can observe background pipeline failures without changing device behavior or transport loss semantics.

src/Daqifi.Core/Device/IDevice.cs

ReconnectAttemptEventArgs.csAdd event args for reconnect-attempt progress +49/-0

Add event args for reconnect-attempt progress

• Adds ReconnectAttemptEventArgs carrying attempt number, max attempts, backoff delay, previous error, and timestamp for UI/progress reporting.

src/Daqifi.Core/Device/ReconnectAttemptEventArgs.cs

ReconnectFailedEventArgs.csAdd event args for reconnect stop (give up vs cancel) +48/-0

Add event args for reconnect stop (give up vs cancel)

• Adds ReconnectFailedEventArgs describing how many attempts were made, the last error (if any), whether reconnection was canceled, and a timestamp.

src/Daqifi.Core/Device/ReconnectFailedEventArgs.cs

ReconnectOptions.csDefine ReconnectOptions policy with backoff + presets +206/-0

Define ReconnectOptions policy with backoff + presets

• Adds a reconnect policy object (off by default) with max attempts, initial/max delays, exponential backoff multiplier, and an option to resume streaming. Includes Disabled/Default/Fast/Resilient presets and safe delay calculation (including overflow/NaN protections).

src/Daqifi.Core/Device/ReconnectOptions.cs

ReconnectedEventArgs.csAdd event args for successful reconnection +45/-0

Add event args for successful reconnection

• Adds ReconnectedEventArgs with the successful attempt number, outage duration, whether streaming was resumed, and timestamp—raised only once session restore is complete.

src/Daqifi.Core/Device/ReconnectedEventArgs.cs

Bug fix (1) +210 / -14

StreamMessageConsumer.csEliminate silent spins; unify fault reporting/backoff in reader loop +210/-14

Eliminate silent spins; unify fault reporting/backoff in reader loop

• Hardens the message-consumer loop to treat CanRead probe failures and unreadable streams as stream faults, backing off and reporting to the transport health sink. Ensures the top-level catch never crashes the host process, and adds consistent backoff to parse/dispatch failures to prevent hot spins while preserving I/O-loss escalation behavior.

src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs

Tests (7) +3622 / -0

StreamMessageConsumerBackoffTests.csAdd tests preventing reader-loop hot spins and preserving escalation +646/-0

Add tests preventing reader-loop hot spins and preserving escalation

• Introduces tests covering CanRead probe failures, parse/dispatch failures, and backoff cadence to ensure repeating failures don’t spin and that true stream faults are still reported to the transport health sink (while non-I/O parse failures are not).

src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs

ConnectionLossEscalationTests.csAdd end-to-end tests for drop escalation vs intentional disconnect +362/-0

Add end-to-end tests for drop escalation vs intentional disconnect

• Adds device-level tests validating that persistent mid-stream read failures transition to ConnectionStatus.Lost and also surface diagnostics, while intentional disconnects are never misclassified as Lost even if reads fail during teardown.

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

DeviceErrorSurfaceTests.csAdd tests for device-level ErrorOccurred propagation and isolation +574/-0

Add tests for device-level ErrorOccurred propagation and isolation

• Adds tests verifying message-consumer failures and other background faults reach DaqifiDevice.ErrorOccurred, that idle timeouts stay quiet, and that throwing subscribers do not disrupt the reader loop or leak subscriptions beyond teardown.

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

DeviceErrorThrottleTests.csPin throttling policy for ErrorOccurred storm suppression +179/-0

Pin throttling policy for ErrorOccurred storm suppression

• Adds unit tests for the per-bucket throttle: first occurrence always passes, repeats collapse within interval, suppressed counts are reported on next raise, different sources/types are independent, reset semantics, and bounded bucket growth behavior.

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

DeviceReconnectTests.csAdd comprehensive reconnection/session-restore test suite +1681/-0

Add comprehensive reconnection/session-restore test suite

• Adds a large scripted-transport test suite validating default no-reconnect behavior, successful reconnection with channel/stream restoration, retry/backoff semantics, cancellation, give-up behavior (including Failed status and error surfacing), and supersession rules when callers connect/disconnect mid-attempt.

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

ReconnectOptionsTests.csAdd unit tests for ReconnectOptions validation and delay math +166/-0

Add unit tests for ReconnectOptions validation and delay math

• Adds tests confirming defaults/presets, first-attempt delay behavior, backoff growth/capping, fixed delay at multiplier 1, and overflow/NaN edge cases plus rejected invalid values.

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

FirmwareUpdateServiceTests.csUpdate fake devices to satisfy new IDevice.ErrorOccurred member +14/-0

Update fake devices to satisfy new IDevice.ErrorOccurred member

• Adjusts test fakes/stubs implementing IDevice/IStreamingDevice to include the new ErrorOccurred event so firmware update tests compile and remain focused on firmware flows.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs

Documentation (2) +194 / -3

DEVICE_INTERFACES.mdDocument ErrorOccurred and automatic reconnect behavior +186/-1

Document ErrorOccurred and automatic reconnect behavior

• Adds IDevice.ErrorOccurred documentation and a full section describing ReconnectOptions, reconnect events, and the Retrying/Failed status semantics. Clarifies what is and is not restored after a drop and how reconnect failure is surfaced (including via ErrorOccurred).

docs/DEVICE_INTERFACES.md

ConnectionStatus.csClarify Retrying/Failed semantics for automatic reconnect +8/-2

Clarify Retrying/Failed semantics for automatic reconnect

• Updates enum documentation to explicitly tie Retrying/Failed to opt-in reconnect behavior and to mark Failed as terminal until an explicit Connect().

src/Daqifi.Core/Device/ConnectionStatus.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. DisconnectAsync cancels teardown ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

When DisconnectAsync is waiting for the new lifecycle semaphore and its cancellation token is
canceled, RunLifecycleExclusiveAsync treats this as "abandon" and DisconnectCoreAsync falls back to
MarkDisconnectedWithoutTeardown without stopping message pumps or closing the transport. This
violates DisconnectAsync’s documented guarantee that cancellation only shortens the text-exchange
wait and that the disconnect itself completes, and can leave active I/O while reporting
Disconnected.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R843-856]

+                acquired = await _lifecycleLock.WaitAsync(timeout, cancellationToken).ConfigureAwait(false);
+            }
+            catch (ObjectDisposedException)
+            {
+                await operation().ConfigureAwait(false);
+                return true;
+            }
+            catch (OperationCanceledException) when (onContention == LifecycleContention.Abandon)
+            {
+                // A teardown is never abandoned by its own token — the caller asked to stop
+                // waiting for the in-flight operation, not to stop disconnecting.
+                LogAbandonedTeardown(timeout);
+                return false;
+            }

Relevance

●●● Strong

Team explicitly documents DisconnectAsync cancellation skips wait but must still teardown; similar
cancellation-commit-point fixes accepted.

PR-#416
PR-#391
PR-#249

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

DisconnectCoreAsync forwards the caller token into lifecycle-lock acquisition, and
RunLifecycleExclusiveAsync converts cancellation into an abandoned teardown (returning false). The
abandoned path only updates in-memory state (MarkDisconnectedWithoutTeardown) while DisconnectAsync
documentation promises cancellation does not abort disconnect itself, creating a contract-breaking
path where resources may remain active despite reporting Disconnected.

src/Daqifi.Core/Device/DaqifiDevice.cs[1235-1246]
src/Daqifi.Core/Device/DaqifiDevice.cs[827-856]
src/Daqifi.Core/Device/DaqifiDevice.cs[1248-1271]
src/Daqifi.Core/Device/DaqifiDevice.cs[1297-1308]

Agent prompt

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

## Issue description
`DisconnectAsync` passes the caller `CancellationToken` into lifecycle-lock acquisition. If the token is canceled while waiting for `_lifecycleLock`, `RunLifecycleExclusiveAsync` returns `false` (abandoned), and `DisconnectCoreAsync` settles state via `MarkDisconnectedWithoutTeardown` without actually tearing down pumps/transport.

This contradicts `DisconnectAsync` XML docs stating cancellation only shortens the text-exchange courtesy wait and never aborts the disconnect itself.

## Issue Context
- Cancellation should affect only the `_textExchangeLock` wait (courtesy wait) and any cancellable transport APIs, not the lifecycle serialization gate.
- The lifecycle lock already has a bounded wait (`TeardownLockTimeout`), so it does not need the caller token to avoid indefinite hangs.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1235-1246]
- src/Daqifi.Core/Device/DaqifiDevice.cs[827-856]

## Suggested change
- In `DisconnectCoreAsync`, call `RunLifecycleExclusiveAsync(..., LifecycleContention.Abandon, CancellationToken.None)` so cancellation does **not** abort the lifecycle-lock wait.
- Keep passing the caller `cancellationToken` into `DisconnectCoreUnsynchronizedAsync(...)` so it can still shorten the text-exchange lock wait as documented.
- Optionally remove/adjust the `catch (OperationCanceledException) when (onContention == LifecycleContention.Abandon)` path in `RunLifecycleExclusiveAsync` to prevent future callers from reintroducing the same behavior.

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

---

2. Raw start skips stream reset ✓ Resolved 🐞 Bug ≡ Correctness

Description

TrackStreamingStart() can set IsStreaming=true when a start-streaming SCPI command is sent via
Send(), but it does not perform the per-session reset work that StartStreaming() performs (timestamp
anchor, gap detector, warmup guard, decode failure count). This can cause frames to be decoded using
stale session state, yielding incorrect reconstructed timestamps and/or incorrect gap/warmup
behavior for streams started with raw commands.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R533-536]

+            // Frequency first: anything observing IsStreaming must never catch it true next to a
+            // rate belonging to a previous session.
+            StreamingFrequency = frequency;
+            IsStreaming = true;

Relevance

●●● Strong

Session-start reset correctness is enforced (accepted warmup/gap/timestamp start handling in
#353/#362); raw start should match.

PR-#353
PR-#362

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

OnStreamMessageReceived() decodes frames only when IsStreaming is true, and StartStreaming()
resets all per-session processing state before setting IsStreaming=true. The new raw-command path
calls TrackStreamingStart() from Send(), and TrackStreamingStart() sets IsStreaming=true
without performing those resets, so decoding can start with stale session baselines.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[360-389]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[775-792]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[450-484]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[519-537]

Agent prompt

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

## Issue description
`TrackStreamingStart()` enables streaming state for raw `Send()`-driven `SYSTem:StartStreamData ...` commands by setting `IsStreaming = true`, but it does not run the same per-session initialization/reset logic that `StartStreaming()` runs before enabling streaming. Because `OnStreamMessageReceived()` gates decoding on `IsStreaming`, this can cause decoding to begin with stale `_timestampProcessor`/`_gapDetector` baselines and stale warmup-frame suppression state.

## Issue Context
`StartStreaming()` performs multiple per-session resets and only then sets `IsStreaming = true`. The raw-command tracking path should either reuse that same initialization logic or replicate it when transitioning from not-streaming to streaming via a raw SCPI start.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[360-389]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[775-792]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[519-537]

### Implementation direction
- Factor the “begin streaming session” reset logic in `StartStreaming()` into a private helper (no I/O) and call it from both `StartStreaming()` and `TrackStreamingStart()` **only when transitioning** from `!IsStreaming` to streaming.
- Ensure the helper resets at least: `_timestampProcessor.Reset(...)`, `_timestampProcessor.SetTimestampFrequency(...)`, `_gapDetector.Reset()`, warmup suppression flags/counters, and `DecodeFailureCount` backing field reset, matching `StartStreaming()` behavior.

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

---

3. Disconnect can be overruled ✓ Resolved 🐞 Bug ≡ Correctness

Description

If DisconnectCore() times out acquiring the lifecycle lock, it abandons teardown and only updates
Status/State; an in-flight ConnectCoreUnsynchronized() can still complete afterward and set
Status=Connected/start background loops even though Disconnect() already returned. This can leave
the device quietly alive (Connected + reader running) after the caller requested teardown,
especially with slow/blocked SerialPort.Open-style connects.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R873-896]

+        private void DisconnectCore(ConnectionStatus finalStatus)
+        {
+            if (RunLifecycleExclusive(
+                    () => DisconnectCoreUnsynchronized(finalStatus),
+                    LifecycleContention.Abandon))
+            {
+                return;
+            }
+
+            // The wait was abandoned: a lifecycle operation is stuck, most likely a
+            // SerialPort.Open wedged in uncancellable native I/O. Racing it would be the
+            // stream corruption this lock exists to prevent, so the transport is left to the
+            // holder — which is guaranteed to release it, because _callerWantsDisconnected was
+            // set before this and AbandonIfSuperseded tears down whatever the stuck connect
+            // eventually builds.
+            //
+            // What can still be done safely is record the caller's intent at the device level.
+            // These are this class's own fields, not the transport, so setting them cannot
+            // corrupt anything the stuck operation is doing — and without them the device would
+            // keep reporting itself connected after the caller had asked it not to.
+            State = DeviceState.Disconnected;
+            _isInitialized = false;
+            Status = finalStatus;
+        }

Relevance

●●● Strong

Team repeatedly accepts lifecycle race fixes; many disconnect/connect contention/race mitigations
accepted (e.g., PR #295, #384).

PR-#295
PR-#384

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new DisconnectCore() abandonment path returns without touching the transport/producer/consumer;
meanwhile ConnectCoreUnsynchronized() always starts producer/consumer and sets Status=Connected with
no check for a concurrent teardown request, so a long connect can complete after the disconnect
returns and re-mark the device Connected.

src/Daqifi.Core/Device/DaqifiDevice.cs[852-896]
src/Daqifi.Core/Device/DaqifiDevice.cs[781-813]

Agent prompt

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

## Issue description
`DisconnectCore(...)` may abandon teardown when it cannot acquire `_lifecycleLock` within `TeardownLockTimeout`, but the in-flight connect that holds the lock can later complete and unconditionally mark the device `Connected` and start producer/consumer loops. This can resurrect a device after `Disconnect()` has already returned.

## Issue Context
This is currently mitigated for the *automatic reconnect* path by `AbandonIfSuperseded(...)`, but the ordinary caller-issued `Connect()` path does not have an equivalent post-connect “caller still wants connected?” guard.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[873-896]
- src/Daqifi.Core/Device/DaqifiDevice.cs[781-819]

## Suggested fix
Add a post-connect check in the connect path (ideally inside `ConnectCoreUnsynchronized()` while still under `_lifecycleLock`) that detects a concurrent caller teardown intent (e.g., `_callerWantsDisconnected == true` and/or `_disposed == true`). If teardown was requested while the connect was in flight, immediately tear the newly-opened session back down (e.g., invoke `DisconnectCoreUnsynchronized(ConnectionStatus.Disconnected)` or an equivalent internal teardown that does not re-cancel reconnect).

Ensure the fix preserves:
- No concurrent transport lifecycle calls (still under `_lifecycleLock`).
- No transient “Connected” state after a successful disconnect when the caller wanted the device down.

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

---

View more (4)

4. Disconnect can hang indefinitely ✓ Resolved 🐞 Bug ☼ Reliability

Description

RunLifecycleExclusive uses an unbounded Monitor.Enter when contention policy is Wait, so
Disconnect()/Dispose() can block indefinitely behind an in-flight lifecycle operation. Since
transport connect can run synchronously without any timeout/cancellation (e.g., SerialPort.Open), a
stalled connect can hold the lifecycle lock indefinitely and prevent teardown from ever completing.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R654-659]

+                if (onContention == LifecycleContention.Wait)
+                {
+                    // The ref overloads are the documented-safe pattern: they set the flag as part
+                    // of taking the lock, so the finally below can never miss a release.
+                    Monitor.Enter(_lifecycleLock, ref acquired);
+                }

Relevance

●●● Strong

Team repeatedly accepted bounding uncancellable SerialPort.Open hangs (PR #295) and other indefinite
waits (PR #401).

PR-#295
PR-#401

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new lifecycle lock wait path uses an unbounded Monitor.Enter, and Disconnect explicitly states
it is not bounded on lifecycle contention. The serial transport connect path performs a synchronous
SerialPort.Open() inside the connect attempt without any timeout/cancellation wrapper, meaning the
lifecycle lock holder is not guaranteed to be bounded, so Disconnect/Dispose can hang indefinitely.

src/Daqifi.Core/Device/DaqifiDevice.cs[649-659]
src/Daqifi.Core/Device/DaqifiDevice.cs[802-809]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[239-266]

Agent prompt

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

## Issue description
`Disconnect()` (and therefore `Dispose()`) can block indefinitely waiting for `_lifecycleLock` because `RunLifecycleExclusive(..., LifecycleContention.Wait)` uses `Monitor.Enter` with no timeout. If a transport `Connect()` stalls while holding the lifecycle lock, teardown can never proceed.

## Issue Context
This PR intentionally serializes connect/disconnect to avoid concurrent transport lifecycle calls, but the new “wait forever” behavior relies on connect operations being bounded. At least one production transport connect path performs a synchronous open with no timeout/cancellation wrapper.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[649-675]
- src/Daqifi.Core/Device/DaqifiDevice.cs[802-833]
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[239-275]

## Suggested fix approach
Choose one (or combine):
1) **Make transport connects bounded/cancelable** (preferred): ensure `SerialStreamTransport.ConnectAsync` cannot block forever (e.g., run `SerialPort.Open()` with a timeout strategy and tear down/dispose on timeout) so the lifecycle lock holder is actually bounded.
2) **Add a bounded wait path for teardown**: if `Disconnect()` cannot acquire `_lifecycleLock` within a generous timeout, log and attempt a safe “force teardown” strategy that cannot leave the device quietly alive (this is tricky because it risks reintroducing concurrent lifecycle calls).
3) If interface changes are acceptable, extend `IStreamTransport.ConnectAsync` to accept a `CancellationToken` so `Disconnect()`/`Dispose()` can reliably cancel a stuck connect attempt.

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

---

5. Lifecycle lock timeout bypass ✓ Resolved 🐞 Bug ☼ Reliability

Description

RunLifecycleExclusive() executes ConnectCore/DisconnectCore even when Monitor.TryEnter times out, so
lifecycle operations can still overlap and concurrently open/close the same transport and rebuild
producer/consumer state. This contradicts the new “wait rather than run alongside” guarantee and can
reintroduce the double-reader / stream corruption race under sustained contention or slow/blocking
transport operations.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R605-615]

+                acquired = Monitor.TryEnter(_lifecycleLock, LifecycleLockTimeout);
+                if (!acquired)
+                {
+                    SafeLog(() => _logger.LogWarning(
+                        "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock within "
+                        + "{TimeoutSeconds}s; proceeding without it.",
+                        Name,
+                        LifecycleLockTimeout.TotalSeconds));
+                }
+
+                operation();

Relevance

●●● Strong

Team repeatedly accepted fixes preventing overlapping lifecycle/reader races (double-reader
invariant) and improving lock/timeout safety in PRs #384/#350/#196.

PR-#384
PR-#350
PR-#196

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

RunLifecycleExclusive calls Monitor.TryEnter and, even when it fails, still invokes the
lifecycle operation; both ConnectCore() and DisconnectCore() are routed through this helper, so
a timeout allows them to run concurrently with an in-flight lifecycle call.
DisconnectCoreUnsynchronized can hold the lifecycle lock while waiting up to 10 seconds on
_textExchangeLock, making the timeout path realistically reachable under contention and allowing
overlap right when serialization is needed most.

src/Daqifi.Core/Device/DaqifiDevice.cs[600-623]
src/Daqifi.Core/Device/DaqifiDevice.cs[647-651]
src/Daqifi.Core/Device/DaqifiDevice.cs[748-752]
src/Daqifi.Core/Device/DaqifiDevice.cs[761-766]

Agent prompt

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

## Issue description
`RunLifecycleExclusive()` proceeds to run `operation()` even when the lifecycle lock cannot be acquired within `LifecycleLockTimeout`. This re-allows concurrent `ConnectCoreUnsynchronized()`/`DisconnectCoreUnsynchronized()` (including reconnect attempts), defeating the new serialization guarantee and enabling concurrent transport open/close and producer/consumer rebuilds.

## Issue Context
The intent of `_lifecycleLock` is to ensure the device never drives its transport lifecycle from two threads at once. The current implementation logs a warning on timeout but still executes the operation, which can overlap with an in-flight lifecycle call that is merely slow (e.g., a disconnect waiting on `_textExchangeLock`, or a slow serial port open).

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[600-623]
- src/Daqifi.Core/Device/DaqifiDevice.cs[647-649]
- src/Daqifi.Core/Device/DaqifiDevice.cs[748-751]

## Implementation notes
- Do not execute lifecycle operations when the lock isn’t acquired.
- Preferred options:
 - Wait without bypassing (no timeout), OR
 - If you must keep a timeout, fail fast for `Connect()`/reconnect attempts (throw/return) rather than running unlocked, and consider a separate policy for `Disconnect()` if teardown must not block forever.
- If you keep a timeout path, update the `Connect()` remarks that currently claim it “waits for it to finish rather than running alongside it,” since that’s not true today.

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

---

6. Reconnect races caller teardown ✓ Resolved 🐞 Bug ☼ Reliability

Description

RunReconnectLoopAsync calls ConnectCore() (which sets Status=Connected and starts producer/consumer)
before re-checking whether the session epoch was superseded by a caller
Connect()/Disconnect()/Dispose(), so a caller teardown during an in-flight reconnect can be
overwritten and leave the transport reopened/Connected.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1804-1813]

+                    ConnectCore();
+
+                    // The caller took the session over while the connect was in flight. Whatever
+                    // they did next owns the device now, so stop here without touching status or
+                    // tearing anything down — either could undo the session they just established.
+                    if (IsSessionStale(epoch))
+                    {
+                        ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: true);
+                        return;
+                    }

Relevance

●●● Strong

Team often accepts race/cancellation re-checks near side-effects (e.g., PR #381 ConnectAsync/dispose
re-check).

PR-#381
PR-#162
PR-#384

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ConnectCore() performs observable side-effects (opens transport, starts producer/consumer, sets
Status=Connected). The reconnect loop invokes ConnectCore() and only then checks whether the session
became stale; when stale, ReportReconnectStopped() intentionally skips teardown, so ConnectCore()
effects can persist and override a caller-issued Disconnect()/Dispose() that happened during the
in-flight connect.

src/Daqifi.Core/Device/DaqifiDevice.cs[554-628]
src/Daqifi.Core/Device/DaqifiDevice.cs[649-736]
src/Daqifi.Core/Device/DaqifiDevice.cs[1755-1813]
src/Daqifi.Core/Device/DaqifiDevice.cs[1847-1859]
src/Daqifi.Core/Device/DaqifiDevice.cs[1872-1881]

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 reconnect loop can still complete `ConnectCore()` after a caller has issued `Disconnect()`/`Dispose()` (or even a new `Connect()`), because epoch/cancellation is only checked *after* `ConnectCore()` returns. Since `ReportReconnectStopped()` intentionally does not tear anything down for a stale epoch, the side-effects of `ConnectCore()` (transport open, producer/consumer started, `Status=Connected`) can remain and override the caller’s desired final state.

### Issue Context
- Caller operations attempt to “win” by bumping `_sessionEpoch` and canceling via `SupersedeReconnect()`, but that does not prevent `ConnectCore()` side-effects during an in-flight connect.
- The reconnect loop explicitly states it does not interrupt a connect attempt in flight, so this race is expected to occur under real timing.

### Fix Focus Areas
- Add a shared synchronization gate (e.g., `lock`/`SemaphoreSlim`) that serializes *all* session-mutating operations: caller `Connect()`, caller `Disconnect()`, `Dispose()`, and the reconnect loop’s `DisconnectCore(...)`/`ConnectCore()`/`InitializeAsync(...)`/restore steps.
- In `RunReconnectLoopAsync`, acquire the gate **before** calling `DisconnectCore(...)` and **before** calling `ConnectCore()`, and re-check `IsSessionStale(epoch)` and `cancellationToken` immediately after acquiring the gate (so a caller that already superseded will prevent further side-effects).
- Ensure caller `Disconnect()`/`Dispose()` also acquires the same gate so that if a reconnect `ConnectCore()` is in flight, the caller blocks until it completes and then reliably tears down (caller truly “wins” without the reconnect reopening behind it).

- src/Daqifi.Core/Device/DaqifiDevice.cs[554-628]
- src/Daqifi.Core/Device/DaqifiDevice.cs[649-736]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1680-1742]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1755-1833]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1872-1881]
- src/Daqifi.Core/Device/DaqifiDevice.cs[2046-2057]

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

---

7. Lifecycle connect race ✓ Resolved 🐞 Bug ☼ Reliability

Description

Caller Connect()/Disconnect() can overlap with an in-flight automatic reconnect attempt because
SupersedeReconnect() only cancels and does not serialize lifecycle operations, so
ConnectCore()/DisconnectCore() may run concurrently and call the underlying transport
simultaneously. This can corrupt transport/device state (double-open/close, producer/consumer swaps)
and violates the promise that caller lifecycle actions “win” cleanly.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R563-566]

+            _callerWantsDisconnected = false;
+            SupersedeReconnect();
+            ConnectCore();
+        }

Relevance

●● Moderate

Team accepts many concurrency/race fixes, but no clear precedent for adding a lifecycle lock around
Connect/Disconnect/reconnect (PR #384, #411).

PR-#384
PR-#411

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Connect() immediately calls ConnectCore() after SupersedeReconnect(), while the reconnect loop also
calls DisconnectCore(...)/ConnectCore() and cannot be interrupted mid-connect; without a shared
lifecycle lock these operations can run concurrently against the same transport and
producer/consumer fields. SerialStreamTransport’s Connect path is not synchronized, so concurrent
opens are not safe to assume.

src/Daqifi.Core/Device/DaqifiDevice.cs[554-566]
src/Daqifi.Core/Device/DaqifiDevice.cs[1647-1657]
src/Daqifi.Core/Device/DaqifiDevice.cs[1799-1828]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[239-260]

Agent prompt

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

## Issue description
`Connect()`/`Disconnect()` can execute `ConnectCore()`/`DisconnectCore()` while the reconnect loop is still inside its own `ConnectCore()`/`DisconnectCore()`/initialization path, because `SupersedeReconnect()` only increments `_sessionEpoch` and cancels the token source. The epoch checks prevent a stale loop from *winning*, but do not prevent the two threads from mutating the transport and device internals concurrently.

## Issue Context
This becomes reachable specifically because the PR introduced a background reconnect loop that calls `DisconnectCore(...)` and `ConnectCore()` while also explicitly documenting that cancellation does not interrupt an already in-flight connect attempt.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[561-566]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1799-1828]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1870-1928]

## Implementation guidance
- Introduce a dedicated lifecycle gate (e.g., `private readonly SemaphoreSlim _lifecycleGate = new(1,1);` or a `lock`-based monitor) that serializes **all** connect/disconnect/teardown that touches `_transport`, `_messageProducer`, `_messageConsumer`, and status/state.
- Acquire the gate in:
 - `Connect()` around `ConnectCore()`
 - `Disconnect()` around `DisconnectCore(...)`
 - the reconnect loop around `DisconnectCore(...)`, `ConnectCore()`, and the critical “session established” window where producer/consumer are (re)created and started.
- For the reconnect loop (async), use `await _lifecycleGate.WaitAsync(cancellationToken)` and `Release()` in `finally`.
- Ensure the gate is not held while doing unrelated long waits (e.g., backoff `Task.Delay`)—but it *should* be held across the actual transport open/close and producer/consumer wiring to prevent concurrent mutation.
- Add/extend a regression test for **caller Connect while reconnect ConnectCore is in flight** (analogous to the existing Disconnect-in-flight test).

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

---

8. Tracking can throw after send ✓ Resolved 🐞 Bug ☼ Reliability

Description

TrackStreamingStart() validates a raw start-streaming rate against
Metadata.Capabilities.MaxSamplingRate and then assigns StreamingFrequency via its throwing public
setter, so if MaxSamplingRate changes concurrently, Send() can throw after the command was already
queued/written. This makes session tracking an unexpected exception source and can leave host-side
session state inconsistent with what was actually sent.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R523-536]

+            if (!int.TryParse(rate, NumberStyles.Integer, CultureInfo.InvariantCulture, out var frequency)
+                || frequency < 1
+                || frequency > Math.Max(1, Metadata.Capabilities.MaxSamplingRate))
+            {
+                Trace.WriteLine(
+                    $"[{nameof(TrackStreamingStart)}] Ignoring a start-streaming command with an unusable rate "
+                    + $"('{rate.ToString()}'); the session state is unchanged.");
+                return;
+            }
+
+            // Frequency first: anything observing IsStreaming must never catch it true next to a
+            // rate belonging to a previous session.
+            StreamingFrequency = frequency;
+            IsStreaming = true;

Relevance

●●● Strong

Team routinely accepts race/atomicity fixes around streaming state & capabilities (accepted #314,
#411, #404).

PR-#314
PR-#411
PR-#404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The StreamingFrequency setter validates against the mutable
Metadata.Capabilities.MaxSamplingRate and throws when out of range; TrackStreamingStart()
assigns through that setter after its own validation. MaxSamplingRate is updated at runtime
(capability document merge), so a concurrent update can invalidate the value between the check and
the setter assignment.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[191-206]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[519-537]
src/Daqifi.Core/Device/Capabilities/CapabilityDocument.cs[158-167]

Agent prompt

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

## Issue description
`TrackStreamingStart()` performs a range check using `Metadata.Capabilities.MaxSamplingRate`, then assigns `StreamingFrequency = frequency`. Since `StreamingFrequency`’s setter also validates against `Metadata.Capabilities.MaxSamplingRate` and throws, a concurrent change to `MaxSamplingRate` can cause an `ArgumentOutOfRangeException` from this bookkeeping path *after* the outbound command has already been sent/queued.

## Issue Context
`MaxSamplingRate` is mutable and can be updated at runtime (e.g., capability document merge), so session tracking should not introduce a post-send exception path.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[191-206]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[519-537]
- src/Daqifi.Core/Device/Capabilities/CapabilityDocument.cs[158-167]

### Implementation direction
- In `TrackStreamingStart()`, snapshot `maxSamplingRate` into a local once and use it consistently.
- Avoid calling the throwing public setter from this tracking path; instead set the backing field (`_streamingFrequency`) directly after validation, or wrap the assignment in a `try/catch (ArgumentOutOfRangeException)` and treat it as “ignore tracking update” (optionally Trace/Log).
- Goal: `Send()` should not start throwing due to session-tracking side effects.

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

---

9. Invalid start marks streaming ✓ Resolved 🐞 Bug ≡ Correctness

Description

DaqifiStreamingDevice.TrackSessionCommand sets IsStreaming = true as soon as a command starts
with SYSTem:StartStreamData, even if the frequency argument can’t be parsed/validated, leaving
IsStreaming=true with a stale StreamingFrequency. This can make subsequent StartStreaming()
calls silently no-op and can cause reconnect restore to resume streaming at the wrong rate.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R480-484]

+            if (trimmed.StartsWith(StartStreamingCommand, StringComparison.OrdinalIgnoreCase))
+            {
+                IsStreaming = true;
+                TrackStreamingFrequency(trimmed.AsSpan(StartStreamingCommand.Length));
+                return;

Relevance

●●● Strong

Team previously fixed stale IsStreaming state causing silent no-ops (accepted PRs #125, #214, #159).

PR-#125
PR-#214
PR-#159

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new tracking path sets IsStreaming=true before parsing the frequency, while the parser
intentionally returns without updating StreamingFrequency on invalid input; meanwhile,
StartStreaming() returns immediately when IsStreaming is already true. The SCPI producer
documents that StartStreamData is a frequency-bearing command and that invalid/out-of-range values
can be rejected by firmware, so preemptively setting IsStreaming creates an inconsistent state
when parsing/validation fails.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[361-389]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[465-491]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[493-509]
src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[393-411]

Agent prompt

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

## Issue description
`TrackSessionCommand` marks the device as streaming before it has confirmed that the `SYSTem:StartStreamData` command includes a valid frequency argument (parseable and within allowed bounds). If parsing/validation fails, `IsStreaming` remains `true` while `StreamingFrequency` remains unchanged.

## Issue Context
- `StartStreaming()` uses `IsStreaming` as an early-return guard, so a bad tracked state can cause silent no-ops.
- `TrackStreamingFrequency(...)` currently fails silently (returns) on malformed/out-of-range input.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[465-509]

## Suggested fix
- Change `TrackStreamingFrequency(...)` to return a `bool` (or `int?`) indicating whether it successfully parsed/accepted a frequency.
- In `TrackSessionCommand`, only set `IsStreaming = true` (and update `StreamingFrequency`) when parsing/validation succeeds; otherwise, leave streaming state unchanged.
- (Optional hardening) Ensure the command match has a boundary (e.g., end-of-string or whitespace) to avoid treating `SYSTem:StartStreamData?` / suffixed tokens as a start command.

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

---

10. Overlong test gate waits ✓ Resolved 🐞 Bug ☼ Reliability

Description

The scripted test transport/device block up to 30 seconds on gate waits while the suite timeout is
15 seconds, so a failed test path can leave background work blocked long after the test has already
timed out. This can prolong failures and increase flakiness due to lingering background operations.

Code

src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs[R945-947]

+            ConnectEntered.Set();
+            _connectGate?.Wait(TimeSpan.FromSeconds(30));
+

Relevance

●●● Strong

Team repeatedly accepts tightening/bounding test waits to avoid hangs/flakiness (PR #198, #364,
#411).

PR-#198
PR-#364
PR-#411

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test suite defines EventTimeout as 15 seconds, but both the connect gate and initialization gate
waits are 30 seconds and ignore whether the wait actually succeeded.

src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs[24-34]
src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs[820-825]
src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs[941-947]

Agent prompt

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

## Issue description
Test seams use hard-coded 30s `ManualResetEventSlim.Wait(...)` calls but the suite’s `EventTimeout` is 15s. If a test fails before setting a gate, the test can time out at 15s while the background reconnect/connect/initialize path stays blocked for up to 30s, slowing failure runs and potentially interfering with subsequent tests.

## Issue Context
This is limited to the test harness (ScriptedReconnectTransport / ScriptedBaseDevice) but impacts CI reliability and debuggability.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs[24-34]
- src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs[820-825]
- src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs[941-947]

## Implementation guidance
- Replace `TimeSpan.FromSeconds(30)` with `EventTimeout` (or a smaller, explicit gate timeout).
- Check the boolean return value from `Wait(...)` and fail fast (e.g., throw/assert with a clear message) instead of silently continuing.
- Optionally: pass a `CancellationToken` into these waits (where feasible) so a failing test can cancel the blocked background operation deterministically.

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

---

View more (1)

11. Zero delay backoff bug ✓ Resolved 🐞 Bug ≡ Correctness

Description

ReconnectOptions.CalculateDelay() can return MaxDelay when InitialDelay is TimeSpan.Zero and the
exponential term overflows (0 × Infinity becomes NaN), so a policy configured for immediate retries
can unexpectedly jump to the maximum delay at very high attempt numbers.

Code

src/Daqifi.Core/Device/ReconnectOptions.cs[R173-189]

+    public TimeSpan CalculateDelay(int attemptNumber)
+    {
+        if (attemptNumber <= 1)
+        {
+            return InitialDelay;
+        }
+
+        var delayMs = InitialDelay.TotalMilliseconds * Math.Pow(BackoffMultiplier, attemptNumber - 1);
+
+        // Math.Pow overflows to +Infinity for a large enough attempt count; Math.Min then yields
+        // MaxDelay, which is the intended cap, but guard NaN (0 * Infinity) explicitly.
+        if (double.IsNaN(delayMs))
+        {
+            return MaxDelay;
+        }
+
+        return TimeSpan.FromMilliseconds(Math.Min(delayMs, MaxDelay.TotalMilliseconds));

Relevance

●●● Strong

Repo has accepted numeric edge/overflow hardening fixes (e.g., PR #257 overflow-safe math); aligns
with test intent.

PR-#257

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation explicitly returns MaxDelay when delayMs is NaN; with InitialDelay==0 and a
sufficiently large attemptNumber, Math.Pow overflows to +Infinity and the product becomes NaN,
contradicting the test’s stated expectation that a zero initial delay remains zero.

src/Daqifi.Core/Device/ReconnectOptions.cs[165-190]
src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs[78-85]

Agent prompt

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

### Issue description
`ReconnectOptions.CalculateDelay()` uses `InitialDelay.TotalMilliseconds * Math.Pow(...)`. For `InitialDelay == 0` and sufficiently large `attemptNumber` (with multiplier > 1), `Math.Pow` overflows to `+Infinity`, making `0 * Infinity` => `NaN`; the current `NaN` guard returns `MaxDelay`, violating the intended invariant that a zero initial delay stays zero.

### Issue Context
There is already a unit test asserting “AZeroInitialDelayStaysZero”, but it only covers small attempt counts. The current implementation can still diverge for large attempt numbers.

### Fix Focus Areas
- Add an early return: if `InitialDelay == TimeSpan.Zero`, return `TimeSpan.Zero` for all attempt numbers.
- Alternatively/also: change the `NaN` guard to return `TimeSpan.Zero` when `InitialDelay == 0` (and keep `MaxDelay` for other NaN cases if desired).
- Add a regression test that calls `CalculateDelay()` with a very large `attemptNumber` (large enough to overflow `Math.Pow` for a multiplier > 1) and asserts it remains `TimeSpan.Zero`.

- src/Daqifi.Core/Device/ReconnectOptions.cs[165-190]
- src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs[78-85]

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

---

12. Teardown log severity mismatch ✓ Resolved 🐞 Bug ◔ Observability

Description

Disconnect() remarks say an abandoned teardown is logged at warning level, but RunLifecycleExclusive
logs the teardown-contention case as an error. This inconsistency can mislead consumers/ops about
the severity of an expected safety fallback under contention.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R690-700]

+                    if (isTeardown)
+                    {
+                        SafeLog(() => _logger.LogError(
+                            "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock "
+                            + "within {TimeoutSeconds}s, so nothing was torn down. A connect is most likely "
+                            + "wedged in uncancellable native I/O; it will release its own session when it "
+                            + "returns.",
+                            Name,
+                            timeout.TotalSeconds));
+
+                        return false;

Relevance

●●● Strong

Team often fixes doc/logging drift; similar “misleading logging/docs mismatch” accepted in PRs #354
and #321.

PR-#354
PR-#321

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Disconnect() remarks explicitly describe a warning-level log on abandoned teardown, but the
actual contention branch in RunLifecycleExclusive uses LogError for the teardown-abandonment path.

src/Daqifi.Core/Device/DaqifiDevice.cs[840-849]
src/Daqifi.Core/Device/DaqifiDevice.cs[688-700]

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 XML remarks for `Disconnect()` state that when teardown abandons waiting for `_lifecycleLock`, it is recorded as a *warning-level* log, but the implementation logs it with `LogError`.

## Issue Context
This is a documentation/observability mismatch. Either the implementation should log a warning (matching the comment/contract) or the documentation should be updated to explicitly state it is an error-level log and why.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[688-700]
- src/Daqifi.Core/Device/DaqifiDevice.cs[840-849]

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

Note for reviewers: this PR currently has no CI checks, and that is not a pass. The build workflow triggers on PRs targeting main, and this one is stacked on fix/connection-loss-detection-and-error-surface (#415). GitHub will retarget it to main once #415 merges, and CI will run then.

Until that happens the only build signal is local, so for the record, on b130df6:

• Release build of the full solution: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2251 passed, 2 skipped on net9.0 and again on net10.0
• Daqifi.Mcp.Tests: 23 passed (net9.0 only — that project is single-target)
• Bench (Nyquist 1, FW 3.7.2), USB non-regression after the teardown fix: 1586 samples with reconnect at its default and 1586 with it enabled and no drop, identical to the pre-fix baseline, no spurious reconnect events, Disconnect() still reporting Disconnected rather than Lost.

Both review findings are addressed in b130df6, each with regression tests confirmed to fail against the pre-fix code.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Round 2 addressed in 5e157b4. Reminder that this PR still has no CI — the build workflow triggers on PRs targeting main and this one is stacked on #415, so GitHub will only run checks once it retargets. The absence of checks is not a pass.

Local signal on 5e157b4:

• Release build of the full solution: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2253 passed, 2 skipped on net9.0 and again on net10.0
• Daqifi.Mcp.Tests: 23 passed (net9.0 only — single-target project)
• Bench (Nyquist 1, FW 3.7.2), USB non-regression after wrapping the real connect/disconnect path in the lifecycle lock: 1586 samples with reconnect at its default and 1586 with it enabled and no drop — identical to both previous baselines, no spurious reconnect events, Disconnect() still reporting Disconnected.

Both findings had regression tests confirmed to fail against the pre-fix code.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5e157b4

[tylerkron]

Round 3 addressed in c2517f3, and the stabilized base is merged in 5463748.

Base merge: merged origin/fix/connection-loss-detection-and-error-surface at 048d29e as a merge commit (not a rebase — this branch is public). No conflicts, nothing resolved by hand. DaqifiDevice.cs was the one file both branches touched and it auto-merged cleanly, since #415 changes there are in the error-surface region and mine are in the lifecycle/reconnect region. I also checked that #415 round-4 partial-read fix to its test doubles does not apply here: my IdleStream only ever idles or throws, it never delivers a payload, so it has no partial-read contract to violate.

Reminder that this PR still has no CI — the build workflow triggers on PRs targeting main and this is stacked on #415, so checks will only run once GitHub retargets it. The absence of checks is not a pass.

Local signal on 5463748 (post-merge):

• Release build of the full solution: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2266 passed, 2 skipped on net9.0 and again on net10.0 (up from 2255 — the merge brings in fix(device): make background failures visible and stop the last silent read-loop spin (closes #377, #394, #378) #415 new backoff tests)
• Daqifi.Mcp.Tests: 23 passed (net9.0 only — single-target project)
• Reconnect subset run three times before the merge, green each time
• Bench (Nyquist 1, FW 3.7.2), USB non-regression after changing the connect/disconnect contention semantics: 1586 samples with reconnect at its default and 1586 with it enabled — identical to all three previous baselines, no spurious reconnect events, Disconnect() still reporting Disconnected.

The round-3 finding had regression tests confirmed to fail against the pre-fix behaviour.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Round 4 addressed in 9882740. The finding was correct and my round-3 justification was factually wrong: SerialStreamTransport calls _serialPort.Open() synchronously with no timeout while holding the lifecycle lock, and this repo already documents that call wedging in uncancellable native I/O (the SerialDeviceFinder port quarantine, which cites PR #295). Teardown now bounds its wait and abandons rather than hanging — details on the inline thread.

Reminder that this PR still has no CI — the build workflow triggers on PRs targeting main, and this is stacked on #415. Checks will run once GitHub retargets it. The absence of checks is not a pass.

Local signal on 9882740:

• Release build of the full solution: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2267 passed, 2 skipped on net9.0 and again on net10.0
• Daqifi.Mcp.Tests: 23 passed (net9.0 only — single-target project)
• Reconnect subset (59 tests) run three times, green each time
• Bench (Nyquist 1, FW 3.7.2), USB non-regression after changing teardown contention semantics: 1586 samples with reconnect at its default and 1586 with it enabled — identical to all four previous baselines, no spurious reconnect events, Disconnect() still reporting Disconnected.

Base is still #415 stable head (048d29e); no re-merge needed this round.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Round 7 addressed in 5bc5cc1 — the finding was in the tracking code added for the bench fix, and it is the same silent-wrong-data family. A start-streaming command whose rate is missing, unparseable or out of range no longer marks the device as streaming, so IsStreaming is never true beside an unvalidated rate and reconnect can never resume at a guessed one. Reasoning and the precedent check are on the inline thread.

Bench status: the physical unplug/replug re-verification passed on the previous commit — 1117 CSV rows, cable pulled, 14.8 s outage across three attempts, then 7068 rows flowing again with streaming resumed: True. #379 headline promise is hardware-proven. This round is not re-benched, deliberately: it is an error path a valid rate never reaches, so the board has nothing to add and taking the lock would be pure risk.

Reminder that this PR still has no CI (stacked on #415), so the local run is the only build signal. On 5bc5cc1:

• Release build: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2279 passed, 2 skipped on net9.0 and again on net10.0
• Daqifi.Mcp.Tests: 23 passed
• Reconnect subset (71 tests) run three times, green each time

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5bc5cc1

[tylerkron]

Round 8 addressed in c6ddd0b — both findings, plus a deliberate pass over the whole Send-tracking method rather than only what was listed, since three findings in one method meant its requirements were being discovered a round at a time.

The important one: a raw-started stream was decoding against the previous session timestamp anchor. A new unit test pins that at 60 seconds of error — samples stamped with times that never happened. StartStreaming() turned out to do six things beyond setting a flag, and the raw path skipped all six; they now live in a shared BeginStreamingSession() so the two paths cannot drift again. Full audit of StartStreaming / StopStreaming / EnableChannels, and the three remaining divergences, are written up on the inline thread and in the XML docs so they read as decisions rather than oversights. Nothing further turned up beyond those.

Reminder that this PR still has no CI (stacked on #415), so the local run is the only build signal. On c6ddd0b:

• Release build: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2284 passed, 2 skipped on net9.0 and again on net10.0
• Daqifi.Mcp.Tests: 23 passed
• Reconnect subset (76 tests) run three times, green each time
• Of five new tests, four fail against pre-fix code; the fifth documents the restart-in-place decision, which pre-fix also satisfied

No re-bench this round, deliberately. The unit test pins the timestamp defect at a precise 60-second error, which is sharper evidence than eyeballing reconstructed timestamps on a live capture, and nothing in the fix changes link behaviour the board could speak to. The hardware has already answered the question it is best at: 1117 rows, cable pulled, 14.8 s outage, 7068 rows flowing again with streaming resumed: True.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Merged origin/main (257564a) in f5531b7. All four sibling PRs are now underneath this one, and the base has retargeted to main, so this PR finally has real CI.

What conflicted, and how each was resolved

Five files, ten hunks.

DeviceErrorSource.cs (add/add) — main carries the squashed #415 copy without my Reconnect = 3. Kept my addition on top of it.

IDevice.cs — the pure squash-duplication shape: my branch's ErrorOccurred block and main's were the same declaration, and main's side additionally carried #416's ConnectAsync/DisconnectAsync members. Dropped my duplicate copy, kept main's.

DaqifiStreamingDevice.cs — my BeginStreamingSession() extraction against main's still-inline StartStreaming body. Taking both sides would have put IsStreaming = true; Send(StartStreaming(...)) inside the reset helper, so the helper would have sent a start command every time it ran. Kept my side; verified those two lines still live in StartStreaming() itself (they show up in the deletion audit below, which is how I checked).

docs/DEVICE_INTERFACES.md (2) — both were additions of mine main simply lacks (the reconnect section, and the feature-list bullet). Kept.

DaqifiDevice.cs (6) — the substantive one. My lifecycle work and #416's restructure land on the same methods. Rather than patch six interleaved hunks, I reset the file to origin/main and re-applied my additions onto it, so nothing from #414/#415/#416/#417 could be silently lost in a hand-merge.

The one real design decision

My lifecycle lock was a Monitor, chosen because it is reentrant — both connect and disconnect raise StatusChanged from inside their critical section, and a handler calling Disconnect() from there must not deadlock. #416 added ConnectAsync/DisconnectAsync, and a monitor cannot be held across await (the continuation may resume on another thread, and Monitor.Exit from a different thread throws).

Leaving the async paths outside the lock would have silently reopened the exact race Qodo spent rounds 2–5 on: the reconnect loop connects synchronously, so a caller's ConnectAsync would have been free to drive the transport alongside it.

So the lock is now a SemaphoreSlim plus an AsyncLocal<bool> re-entry flag — the same technique _textExchangeLock / _isInsideTextExchange already uses in this class. RunLifecycleExclusive and RunLifecycleExclusiveAsync share the contention policy, timeouts and logging; re-entry proceeds without acquiring, exactly as the monitor did. Both Connect/ConnectAsync and Disconnect/DisconnectAsync now wrap main's BeginConnect/CompleteConnect/FailConnect/StopMessagePumps/FinishDisconnect step helpers rather than replacing them, and FinishDisconnect gained the finalStatus parameter the reconnect loop needs to report Retrying between attempts.

Deletion audit

Diffed the merged tree against origin/main and reviewed every removed line. Nine, all intentional:

• FinishDisconnect(bool) → FinishDisconnect(bool, ConnectionStatus), its two call sites, and its hardcoded Status = ConnectionStatus.Disconnected → Status = finalStatus (5 lines)
• two ConnectionStatus doc lines, expanded to describe the reconnect states
• one docs example line, replaced by a fuller status switch
• IsStreaming = true; Send(ScpiMessageProducer.StartStreaming(StreamingFrequency)); — moved, not dropped: out of the inlined body and into StartStreaming() when BeginStreamingSession() was extracted. Verified present.

Nothing auto-merged was quietly lost. I specifically re-checked the failure mode found on #416 — _errorThrottle.Reset() is in the shared BeginConnect(), so both connect paths reset it.

All five PRs' behaviour confirmed present

• fix(device): make background failures visible and stop the last silent read-loop spin (closes #377, #394, #378) #415 — ErrorOccurred, DeviceErrorSource (+ my Reconnect = 3)
• fix(sdcard): run the SD→LAN restore inside the exchange lock, not after it (closes #407) #417 — finalizeAsync phase, 8 references
• fix(device): an opt-in connect that leaves a running stream alone (closes #385) #414 — OnDeviceInitializingAsync(bool preserveActiveStream, CancellationToken) on both the base and the streaming override; ThrowIfCancellationRequested() immediately before _isInitialized = true
• feat(device): cancellable async connect/disconnect and IAsyncDisposable (closes #341) #416 — ConnectAsync/DisconnectAsync/IAsyncDisposable, the BeginConnect/CompleteConnect/StopMessagePumps decomposition, SetErrorThrottleForTesting
• feat: opt-in auto-reconnect and streaming session resume after ConnectionStatus.Lost #379 (this PR) — reconnect loop, lifecycle serialization on both sync and async paths, session capture/restore, raw-command session tracking

Tests

• Release build of the full solution: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2336 passed, 2 skipped on net9.0 and again on net10.0 — every sibling PR's tests green alongside mine
• Daqifi.Mcp.Tests: 23 passed
• Reconnect subset (78 tests) run three times, green each time — re-checked deliberately because the lock primitive changed

No re-bench: the resolution preserves connect/disconnect behaviour rather than changing it, and the reconnect path is already hardware-validated (1117 rows, cable pulled, 14.8 s outage, 7068 rows flowing again).

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Round 9 addressed in f65a2ce — one finding, a genuine cross-PR interaction that existed only in the merged result. A cancelled DisconnectAsync was skipping teardown entirely and reporting Disconnected with the transport still open and the pumps still running. Because SemaphoreSlim throws for an already-cancelled token even when free, it hit every cancelled disconnect, not just a contended one. Reasoning and the audit of the other exit paths are on the inline thread; this was the only site where "token cancelled" and "lock unavailable" were conflated.

Local signal on f65a2ce:

• Release build: 0 warnings, 0 errors
• Daqifi.Core.Tests: 2338 passed, 2 skipped on net9.0 and again on net10.0
• Daqifi.Mcp.Tests: 23 passed
• Reconnect subset (80 tests) run three times, green each time
• Of two new tests, one fails against pre-fix code (transport left open after a cancelled disconnect); the second documents the contended case and passes pre-fix, because round 5 caller-side guard cleans up behind the skipped teardown

No re-bench. The change is which CancellationToken is passed to an in-process semaphore — the bytes on the wire and the order of the teardown steps are identical either way. The reconnect path the board can speak to is already hardware-validated (1117 rows, cable pulled, 14.8 s outage, 7068 rows flowing again).

[tylerkron]

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

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