fix: surface typed exception when serial port closes mid-operation (#238)

[tylerkron]

Summary

Fixes #238 — the serial analog of #237.

SerialStreamTransport.Stream was:

return _serialPort?.BaseStream ?? throw new InvalidOperationException("Transport is not connected.");

When the underlying SerialPort is non-null but closed (device unplugged, or a DTR-triggered MCU reset re-enumerated the COM port mid-connect), the SerialPort.BaseStream getter itself throws before the ?? throw guard can run:

System.InvalidOperationException: The BaseStream is only available when the port is open.

So the intended typed signal never surfaced; consumers got a raw framework exception that reads like an app bug and is auto-filed as an error by crash reporting (daqifi/daqifi-desktop#588).

Changes

• New TransportNotConnectedException : InvalidOperationException (Communication/Transport/). Deriving from InvalidOperationException keeps backward compatibility — existing catch (InvalidOperationException) blocks and the IStreamTransport.Stream contract still hold — while giving consumers a precise type to classify a dropped/closed transport as a transient, environmental condition (the same tier as FileNotFoundException / UnauthorizedAccessException and the TCP TimeoutException from fix: surface TCP connect timeout as TimeoutException #237).
• SerialStreamTransport.Stream now guards on _serialPort?.IsOpen != true before touching BaseStream, throwing the typed exception with a message that names the transport state instead of leaking the raw framework message. (IsConnected already returns false once the port closes, since it is => _serialPort?.IsOpen == true — issue point 2 was already satisfied.)
• TcpStreamTransport.Stream throws the same typed exception for cross-transport consistency (the WiFi twin, daqifi-desktop#595).
• DaqifiDevice.ExecuteTextCommandCoreAsync detects a dropped transport (_transport.IsConnected == false) and fails with the typed exception rather than dereferencing Stream and leaking the framework message (issue point 3). The device-level IsConnected is status-based and can still report Connected when the transport has silently dropped (a serial unplug/DTR-reset raises no status event).

Tests

• SerialStreamTransport_Stream_WhenPortClosedMidOperation_ThrowsTypedException_NotRawBaseStreamMessage — injects a constructed-but-unopened SerialPort (via an internal test seam), asserts the typed exception and that the message is not the raw "BaseStream is only available..." text, and that IsConnected is false. No hardware needed.
• Typed-exception assertions for the never-connected case (serial + TCP), plus the existing not-connected tests updated to Assert.ThrowsAny<InvalidOperationException> to lock in the preserved assignability contract.
• DaqifiDevice_ExecuteTextCommand_WhenTransportDroppedSilently_ThrowsTransportNotConnectedException — drives InitializeAsync → ExecuteTextCommandCoreAsync against a transport that silently drops, asserting the typed exception and DeviceState.Error.

All 1011 tests pass on both net9.0 and net10.0; Release solution build is clean with TreatWarningsAsErrors. Also verified end-to-end against a real device on COM3: the happy path returns the live SerialStream unchanged, and the closed-port path surfaces the typed exception without leaking the raw message.

Consumer impact / release notes

Behavioral API change worth a release-notes mention (as with #237): consumers catching the raw InvalidOperationException around stream access / connect to detect a dropped serial transport should catch TransportNotConnectedException. Because it derives from InvalidOperationException, existing broad catches keep working unchanged; consumers opt in to the warning-tier classification by adding the specific type.

References

• Serial analog of fix: surface TCP connect timeout as TimeoutException #237 (TCP connect timeout → TimeoutException)
• Consumer-side issue: System.InvalidOperationException: The BaseStream is only available when the port is open. daqifi-desktop#588
• WiFi twin precedent: fix(device): classify WiFi connect timeout as warning, surface inline dialog error daqifi-desktop#595

🤖 Generated with Claude Code

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 bug: SerialStreamTransport.Stream leaks raw InvalidOperationException when serial port closes mid-operation

1. Serial Stream race NRE ✓ Resolved 🐞 Bug ☼ Reliability

Description

SerialStreamTransport.Stream checks _serialPort?.IsOpen and then dereferences
_serialPort.BaseStream without capturing a stable local reference, so a concurrent
DisconnectAsync/Dispose can null _serialPort between those two reads and throw
NullReferenceException instead of TransportNotConnectedException. This regresses the intended
typed-disconnect signal and can crash callers under concurrent disconnect.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R77-83]

+            if (_serialPort?.IsOpen != true)
+            {
+                throw new TransportNotConnectedException(
+                    $"Serial transport is not connected ({_portName}).");
+            }
+
+            return _serialPort.BaseStream;

Evidence

The Stream getter performs a check-then-use on _serialPort (nullable access for IsOpen, then
non-null dereference for BaseStream) while DisconnectAsync can concurrently dispose and set
_serialPort = null in a finally block, making a NullReferenceException possible.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[64-85]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[194-217]

Agent prompt

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

### Issue description
`SerialStreamTransport.Stream` reads `_serialPort` twice (IsOpen check, then BaseStream access). If another thread calls `DisconnectAsync()` and sets `_serialPort = null` between those reads, the getter can throw `NullReferenceException`, bypassing the intended `TransportNotConnectedException`.

### Issue Context
This transport type is used concurrently (device code stops/starts consumers and can disconnect), and `DisconnectAsync()` unconditionally nulls `_serialPort` in `finally` with no synchronization.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[64-85]
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[194-217]

### Suggested fix
- Capture the field into a local variable and use that consistently:
 - `var port = _serialPort;`
 - `if (port?.IsOpen != true) throw ...;`
 - `return port.BaseStream;`
- (Optional hardening) wrap `port.BaseStream` access to translate an `InvalidOperationException` into `TransportNotConnectedException` if the port closes between the check and getter.

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

---

2. Test seam leaks SerialPort ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

SetSerialPortForTesting documents that the transport “takes ownership and disposes” the injected
SerialPort, but it only assigns _serialPort and does not dispose any previously held instance when
replacing or clearing it. If tests call it multiple times or inject then later call ConnectAsync
(which overwrites _serialPort), prior SerialPort instances can remain undisposed and keep OS
handles/ports open.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R44-54]

+    /// <summary>
+    /// Test seam: injects the underlying <see cref="SerialPort"/> so the closed-port
+    /// stream-access path (issue #238) can be exercised without real hardware — e.g. by
+    /// passing a constructed-but-unopened port whose <see cref="SerialPort.IsOpen"/> is
+    /// <c>false</c>. The transport takes ownership and disposes it. Never used in production.
+    /// </summary>
+    /// <param name="serialPort">The serial port to use, or <c>null</c> to clear it.</param>
+    internal void SetSerialPortForTesting(SerialPort? serialPort)
+    {
+        _serialPort = serialPort;
+    }

Evidence

The seam explicitly states the transport takes ownership/disposes, but the implementation only
assigns _serialPort. ConnectAsync later assigns a new SerialPort into _serialPort, which will
overwrite any injected instance without disposal unless tests do it manually.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[44-54]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[150-158]

Agent prompt

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

### Issue description
`SetSerialPortForTesting()` claims ownership/disposal in its XML docs, but it does not dispose the existing `_serialPort` when swapping or clearing it. This can leak `SerialPort` instances in unit tests and contradicts the method contract.

### Issue Context
`ConnectAsync()` assigns a new `SerialPort` into `_serialPort`; if a test has already injected a port via the seam, the injected instance will be overwritten without disposal.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[44-54]
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[150-158]

### Suggested fix
- Implement the documented ownership semantics:
 - Store `var old = _serialPort; _serialPort = serialPort; old?.Dispose();`
 - If clearing (`serialPort == null`), also dispose the old one.
- Consider guarding with `ThrowIfDisposed()` to prevent injecting into a disposed transport.
- If you prefer not to dispose on swap, update the XML doc to match actual behavior.

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

---

[qodo-code-review]

PR Summary by Qodo

Surface TransportNotConnectedException when serial/TCP transports drop
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Walkthroughs

Description

• Add TransportNotConnectedException for classifying dropped/not-connected transports by type.
• Guard SerialStreamTransport.Stream against closed ports to avoid leaking BaseStream exceptions.
• Fail device text commands when transport silently drops; add targeted regression tests.

Diagram

graph TD
  CON{{"Consumer"}} --> DEV["DaqifiDevice"] --> API["IStreamTransport"]
  API --> SER["SerialStreamTransport"]
  API --> TCP["TcpStreamTransport"]
  SER --> EX(("TransportNotConnectedException"))
  TCP --> EX
  DEV --> EX

  subgraph Legend
    direction LR
    _ext{{"External"}} ~~~ _mod["Module/Class"] ~~~ _exc(("Exception"))
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Translate BaseStream failure by catching InvalidOperationException

• ➕ Covers TOCTOU race where port closes after IsOpen check but before BaseStream access
• ➕ Can preserve original exception as InnerException for diagnostics
• ➖ Slightly more code/branches in Stream getter
• ➖ Must ensure only expected framework exceptions are translated

2. Keep throwing InvalidOperationException with standardized messages only

• ➕ No new public type to document
• ➕ Zero consumer changes if they already match on message (not recommended)
• ➖ Consumers can’t reliably classify transport drops vs application bugs
• ➖ Encourages brittle message-based handling

3. Change API to TryGetStream()/nullable Stream instead of throwing

• ➕ Avoids exceptions for control flow
• ➕ Makes disconnection handling explicit at call sites
• ➖ Breaking change to IStreamTransport and all implementations/callers
• ➖ Harder to retrofit across existing consumer code

Recommendation: The PR’s approach (introducing TransportNotConnectedException derived from InvalidOperationException and throwing it at transport/device boundaries) is the right compatibility/perf tradeoff and improves diagnosability. Consider additionally wrapping the SerialPort.BaseStream access in a try/catch that translates the framework InvalidOperationException into TransportNotConnectedException to handle the rare race where the port closes between the IsOpen check and BaseStream getter.

File Changes

Enhancement (1)

TransportNotConnectedException.cs Introduce TransportNotConnectedException for dropped/not-connected transports +58/-0

Introduce TransportNotConnectedException for dropped/not-connected transports

• Adds a new exception type derived from InvalidOperationException with constructors and detailed docs. Provides a typed, classifiable signal while preserving compatibility for existing InvalidOperationException catches.

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

---

Bug fix (3)

SerialStreamTransport.cs Throw typed exception when serial port is non-null but closed +32/-2

Throw typed exception when serial port is non-null but closed

• Adds an internal test seam to inject a SerialPort instance for closed-port test scenarios. Updates Stream to guard on _serialPort?.IsOpen before touching BaseStream and throws TransportNotConnectedException with a transport-state message.

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

---

TcpStreamTransport.cs Standardize TCP Stream failure to TransportNotConnectedException +7/-2

Standardize TCP Stream failure to TransportNotConnectedException

• Updates Stream to throw TransportNotConnectedException when _networkStream is null, aligning not-connected behavior across transport types. Enhances XML docs to reflect the typed exception and disposal behavior.

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

---

DaqifiDevice.cs Detect silently dropped transport before dereferencing Stream +12/-0

Detect silently dropped transport before dereferencing Stream

• Adds a transport-level IsConnected check inside ExecuteTextCommandCoreAsync’s lock-protected validation. Throws TransportNotConnectedException when the transport has dropped even if device status still reports Connected.

src/Daqifi.Core/Device/DaqifiDevice.cs

---

Tests (3)

SerialStreamTransportTests.cs Add regression coverage for closed-serial-port Stream access +38/-3

Add regression coverage for closed-serial-port Stream access

• Updates the existing not-connected Stream test to assert assignability (ThrowsAny<InvalidOperationException>) while adding explicit assertions for TransportNotConnectedException. Adds a regression test that injects a constructed-but-unopened SerialPort to simulate a closed port and verifies the typed exception and non-leaking message.

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

---

TcpStreamTransportTests.cs Assert typed not-connected exception for TCP Stream access +16/-3

Assert typed not-connected exception for TCP Stream access

• Updates the not-connected Stream test to assert assignability for backward compatibility. Adds a new test that expects TransportNotConnectedException when accessing Stream before a TCP connection is established.

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

---

DaqifiDeviceWithTransportTests.cs Test device behavior when transport drops without status event +36/-0

Test device behavior when transport drops without status event

• Adds a MockStreamTransport helper to simulate a silent connection loss (no StatusChanged event). Introduces a new test ensuring InitializeAsync/ExecuteTextCommand surfaces TransportNotConnectedException and transitions the device to DeviceState.Error.

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

---

Documentation (1)

IStreamTransport.cs Document TransportNotConnectedException on IStreamTransport.Stream +4/-0

Document TransportNotConnectedException on IStreamTransport.Stream

• Extends the Stream property XML docs to specify the new TransportNotConnectedException contract for never-connected or dropped transports.

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

---

[tylerkron]

Thanks @qodo-code-review — both findings were valid and are addressed in e293b5d.

1. Serial Stream race / NRE (Action required) — fixed. Agreed: the getter read _serialPort twice (the ?.IsOpen check, then .BaseStream), so a concurrent Disconnect()/Dispose() nulling the field in its finally could throw NullReferenceException instead of the typed signal. The original single _serialPort?.BaseStream was null-safe and my refactor regressed that.

• Capture into a local: var port = _serialPort; and use it consistently.
• Also implemented the high-level recommendation (alternative feat: init of project structure with CICD #1): wrap port.BaseStream in a try/catch (InvalidOperationException) that translates to TransportNotConnectedException, preserving the original as InnerException. This closes the exact TOCTOU bug: SerialStreamTransport.Stream leaks raw InvalidOperationException when serial port closes mid-operation #238 is about — the port closing between the IsOpen check and the BaseStream getter. SerialPort.BaseStream's only InvalidOperationException is the "only available when the port is open" case, so the translation can't mask an unrelated error.

2. Test seam leaks SerialPort (Review recommended) — fixed. Made the implementation match its documented ownership contract: SetSerialPortForTesting now disposes the previously held port on swap/clear (no-op on same-instance re-assign), and added a test asserting assign → same-ref early-return → swap-dispose → clear-dispose.

One note on coverage: the new try/catch translation path is defensive-only and not unit-testable through the seam — SerialPort.IsOpen/BaseStream are non-virtual, so the "open at check, throws at access" state can't be forced deterministically without real hardware. I verified the happy path and the closed-port path end-to-end against a physical device on COM3 instead.

All 1013 tests pass on net9.0 and net10.0; Release build clean with warnings-as-errors.