fix(transport): a throwing StatusChanged subscriber no longer kills auto-reconnect or leaks the port handle

[tylerkron]

What was wrong

Pull the USB cable on a device whose StatusChanged handler throws — the classic case being a WPF or WinForms app that updates a bound property from the handler, which throws a cross-thread InvalidOperationException because the event is documented as firing on a background thread — and three things went wrong at once, silently:

• Automatic reconnection never started. ReconnectOptions.Enabled did nothing at all, because the exception escaped the Lost notification and skipped the reconnect start that runs right after it.
• The serial port (or socket) stayed claimed for the life of the process. The transport nulls its handle field before notifying, so unwinding past the dispose meant a later Disconnect()/Dispose() found null and skipped it too. Re-plugging the device then fails with "Access is denied".
• Nothing was reported. The exception disappeared into a background watchdog's catch, so the consumer was left with a dead, unreconnectable device and no error anywhere.

How it was fixed

StatusChanged was the last event on that path still raised unguarded — ErrorOccurred, SendFailed, the reconnect events and the classified-message events are all already isolated. It now goes through the same kind of guard: the transition always completes, whatever happens after it still runs, and the subscriber's exception is reported on ErrorOccurred (and the device logger) under a new DeviceErrorSource.StatusNotification. Both transports additionally dispose their handle in a finally, so a consumer subscribed directly to a bare transport can't leak it either.

Things you may want to push back on:

• Isolation is per raise, not per subscriber. The first handler to throw still ends that one notification for the rest of the invocation list. That matches every other raiser in the class; making it per-subscriber would be a behaviour change beyond the issue.
• It applies to every transition, not just Lost. A throwing subscriber used to make Connect() itself throw after the device was fully connected, which is the same bug wearing a different hat.
• DeviceErrorSource gains a member (StatusNotification = 4). Additive, but it is a public enum.
• The transports swallow rather than rethrow. Their callers are a watchdog timer thread and the reader/writer loops, all of which already absorb exceptions, so rethrowing would only relocate it. The device layer is where the failure is actually surfaced; the transports, which carry no ILogger, additionally emit the same best-effort Trace line DeviceFinderBase uses for its isolated raises, so a bare-transport consumer isn't left with silence.

Verification

• 13 new tests across the drop path (serial I/O-fault and presence-poll drops, TCP over a real loopback socket where the peer observing the close proves the handle was really released, and an exception whose own ToString() throws) and the device (connect/disconnect/drop, the ErrorOccurred report, a throwing ErrorOccurred handler on top of a throwing StatusChanged one, and the reconnect loop starting and completing). Each was re-run against the un-fixed code first — all failed — so they pin the bug rather than the fix.
• Full suite green on net9.0 (2915 Core + 43 Mcp) and net10.0 (2915), 0 failures, 0 warnings.
• Bench, fw 3.7.2 on /dev/cu.usbmodem1101: four connect → status → stream → disconnect cycles across the review rounds, exit 0, 1186–1187 samples in 3 s at 500 Hz (this unit's known ~79% clock ratio), clean Disconnected transition through the new raise path. Non-destructive; serial only.

closes #494

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Isolate StatusChanged exceptions to preserve reconnect and always release handles

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Guard StatusChanged so subscriber exceptions cannot break status transitions or reconnect start.
• Always dispose serial/tcp handles in finally on connection-lost paths to prevent OS-handle
 leaks.
• Add tests and docs covering drop-path isolation and new StatusNotification error source.

Diagram

graph TD
T(["Serial/TCP transport"]) --> D["DaqifiDevice"] --> S["Status setter"] --> N["Raise StatusChanged (guard)"] --> R["Begin reconnect"]
N --> E["ErrorOccurred (StatusNotification)"]
T --> H[("Port/Socket handle")] --> F["Dispose in finally"]
subgraph Legend
  direction LR
  _svc(["Service/Component"]) ~~~ _res[("OS handle")] ~~~ _evt["Event surface"]
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Per-subscriber isolation (invoke each handler independently)

• ➕ One bad subscriber does not prevent other subscribers from being notified
• ➕ More closely matches “best-effort notification” expectations for events
• ➖ Behavior change vs existing event-raising semantics in this library
• ➖ More complex to implement correctly (ordering, exception aggregation, performance)

2. Marshal StatusChanged onto a captured SynchronizationContext

• ➕ Avoids common UI cross-thread exceptions at the source
• ➕ Makes the event easier to consume from WPF/WinForms
• ➖ Implicit threading model change; can introduce deadlocks/reentrancy if marshaled synchronously
• ➖ Harder to reason about ordering relative to reconnect and transport teardown

3. Dispose the transport handle before raising StatusChanged

• ➕ Eliminates handle-leak risk without relying on exception handling
• ➕ Simplifies transport drop-path correctness
• ➖ Can be an observable behavior change for consumers expecting handle validity during notification
• ➖ May complicate diagnostics if consumers rely on transport state while handling Lost

Recommendation: The PR’s approach (guard the device-level StatusChanged raise + ensure transport disposal in finally) is the best minimal-risk fix: it preserves existing event semantics (per-raise isolation), keeps reconnect behavior deterministic, and closes the OS-handle leak even for consumers using transports directly. The main alternatives were considered but would introduce larger behavioral/threading changes.

Files changed (8) +464 / -11

Enhancement (1) +11 / -0

DeviceErrorSource.csAdd StatusNotification error source for StatusChanged subscriber failures +11/-0

Add StatusNotification error source for StatusChanged subscriber failures

• Adds DeviceErrorSource.StatusNotification to classify failures from StatusChanged subscribers without rolling back the status transition.

src/Daqifi.Core/Device/DeviceErrorSource.cs

Bug fix (3) +82 / -11

SerialStreamTransport.csAlways dispose serial port in finally when connection is lost +20/-4

Always dispose serial port in finally when connection is lost

• Moves StatusChanged raising into a guarded try/catch and disposes the extracted SerialPort in a finally block. Prevents subscriber exceptions from leaking the OS serial handle during drop handling.

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

TcpStreamTransport.csAlways dispose TCP stream/client in finally when connection is lost +17/-5

Always dispose TCP stream/client in finally when connection is lost

• Mirrors the serial fix: raises StatusChanged in a try/catch and disposes NetworkStream/TcpClient in finally. Ensures subscriber exceptions cannot leak sockets on the drop path.

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

DaqifiDevice.csGuard device StatusChanged and surface subscriber errors via ErrorOccurred +45/-2

Guard device StatusChanged and surface subscriber errors via ErrorOccurred

• Replaces the direct StatusChanged invocation with RaiseStatusChanged, which catches subscriber exceptions and reports them via RaiseDeviceError. Adds XML remarks documenting background-thread behavior and the new isolation guarantee.

src/Daqifi.Core/Device/DaqifiDevice.cs

Tests (3) +362 / -0

DropPathSubscriberIsolationTests.csAdd transport drop-path tests for subscriber isolation and handle release +175/-0

Add transport drop-path tests for subscriber isolation and handle release

• Introduces new tests ensuring a throwing transport StatusChanged subscriber cannot prevent serial port/socket disposal on drops. Covers both serial I/O-fault and presence-probe drops and validates TCP socket closure via peer FIN.

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

DeviceReconnectTests.csTest that StatusChanged exceptions do not prevent reconnect loop start +24/-0

Test that StatusChanged exceptions do not prevent reconnect loop start

• Adds a regression test verifying that a throwing device StatusChanged subscriber does not stop BeginReconnectIfEnabled from starting and completing a reconnect cycle.

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

DeviceStatusChangedIsolationTests.csAdd device-level tests for StatusChanged exception isolation and reporting +163/-0

Add device-level tests for StatusChanged exception isolation and reporting

• Adds tests proving StatusChanged subscriber exceptions do not escape Connect/Disconnect or drop handling, that device status still updates to Lost, and that the exception is reported via ErrorOccurred as StatusNotification (even if ErrorOccurred subscribers also throw).

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

Documentation (1) +9 / -0

DEVICE_INTERFACES.mdDocument StatusChanged isolation and new error source +9/-0

Document StatusChanged isolation and new error source

• Adds guidance that StatusChanged runs on a background thread and is now isolated from subscriber exceptions. Documents that failures are surfaced via ErrorOccurred as DeviceErrorSource.StatusNotification.

docs/DEVICE_INTERFACES.md

[qodo-code-review]

Code Review by Qodo

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

1. FIN-only TCP assertion ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The new TCP isolation test asserts that the peer observes a graceful FIN (Receive returns 0), but a
correct socket release can also legitimately surface as a connection-reset SocketException on some
platforms/states, causing a false test failure. This makes the test brittle even when
TcpStreamTransport correctly disposes the stream/client in the drop path.

Code

src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[R201-202]

+        server.Client.ReceiveTimeout = 5000;
+        Assert.Equal(0, server.Client.Receive(new byte[1]));

Relevance

●●● Strong

Team often hardens tests against platform variance/flakiness; allowing EOF or reset avoids brittle
TCP close assertions.

PR-#237
PR-#415

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test currently treats only a FIN (0-byte receive) as proof of socket release. The drop path
closes by disposing the stream/client; peer-observed close semantics can be EOF (0) or a reset
exception, so the FIN-only assertion can produce false failures without indicating a resource leak.

src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[174-203]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[401-427]

Agent prompt

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

### Issue description
`TcpTransport_WhenTheSubscribersExceptionCannotRenderItself_TheDropPathIsStillContained` currently proves socket release by asserting `Socket.Receive(...)` returns `0` (FIN). A correct close can also be observed as a `SocketException` (e.g., connection reset), which still indicates the socket was released, so the test can fail despite correct behavior.

### Issue Context
The transport drop path disposes `NetworkStream`/`TcpClient` but does not explicitly enforce a FIN-vs-RST observation on the peer. The test should accept either outcome while still rejecting timeouts (which would indicate the socket wasn’t actually closed).

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[201-203]

### Suggested change
Wrap the `Receive` in `Record.Exception`, and assert one of:
- receive returns `0`, OR
- exception is a `SocketException` with an error consistent with peer closure/reset (but **not** timeout).
Example shape:
```csharp
server.Client.ReceiveTimeout = 5000;
var buffer = new byte[1];
int? received = null;
var ex = Record.Exception(() => received = server.Client.Receive(buffer));

if (ex is null)
{
   Assert.Equal(0, received);
}
else
{
   var se = Assert.IsType<SocketException>(ex);
   Assert.NotEqual(SocketError.TimedOut, se.SocketErrorCode);
}
```
(Adjust the allowed error codes if you want to be stricter, e.g., allow only `ConnectionReset`/`ConnectionAborted`.)

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

---

2. Unsafe trace message build ✓ Resolved 🐞 Bug ☼ Reliability

Description

SerialStreamTransport/TcpStreamTransport build the interpolated trace message (including ex)
outside SafeTrace, so if exception stringification throws, the drop-path can still let an
exception escape HandleConnectionLost. Resource disposal still happens in finally, but the
caller thread (watchdog / I/O loop) can still be disrupted despite the intent to swallow the
subscriber failure.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R633-636]

+            // ErrorOccurred; this transport carries no logger, so a consumer working against a bare
+            // transport gets the same best-effort trace DeviceFinderBase gives its event raises.
+            SafeTrace(
+                $"[{nameof(SerialStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {ex}");

Relevance

●●● Strong

Team previously accepted making trace/logging fully non-throwing to preserve isolation boundaries.

PR-#354

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The call site interpolates {ex} before entering SafeTrace, while SafeTrace only wraps
Trace.WriteLine, leaving message construction outside the containment boundary. TcpStreamTransport
mirrors the same pattern.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[620-674]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[408-452]
PR-#354

Agent prompt

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

### Issue description
`HandleConnectionLost` tries to swallow `OnStatusChanged` failures and only trace best-effort. However, it interpolates `{ex}` outside `SafeTrace`, so any exception thrown during message construction can escape the `catch` block.

### Issue Context
`SafeTrace` only guards `Trace.WriteLine(...)`, not the creation of the string passed to it.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[633-672]
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[417-450]

### Suggested fix
Change `SafeTrace` to accept an `Exception` and build the full message inside its internal `try/catch`, e.g.:

```csharp
private static void SafeTrace(string prefix, Exception ex)
{
   try
   {
       System.Diagnostics.Trace.WriteLine($"{prefix}{ex}");
   }
   catch
   {
   }
}
```

Then call it without interpolating `{ex}` at the call site (or wrap the interpolation itself in a local `try/catch`). Apply the same pattern to both Serial and TCP transports.

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

---

3. Silent swallowed status errors ✓ Resolved 🐞 Bug ◔ Observability

Description

SerialStreamTransport.HandleConnectionLost (and the same pattern in TcpStreamTransport) now swallows
any exception thrown by OnStatusChanged without logging or surfacing it, so bare-transport
StatusChanged handler bugs (or an overridden OnStatusChanged failure) can become completely silent.
This reduces diagnosability and can mask transport-subclass defects.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R624-627]

        catch (Exception)
        {
-            // The device is already gone; failing to close its handle changes nothing.
+            // A subscriber that throws must not cost us the port handle (issue #494). The reference
+            // has already been taken out of the field, so if this unwound past the dispose below,

Relevance

●●● Strong

Team often isolates subscriber exceptions but still logs best‑effort to avoid silent failures.

PR-#354
PR-#428
PR-#360

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new catch blocks in both transports swallow exceptions from OnStatusChanged without emitting any
log/event, while OnStatusChanged is protected virtual and dispatches StatusChanged, so both
subscriber and override exceptions are suppressed. In contrast, device-level StatusChanged failures
are surfaced via RaiseDeviceError/ErrorOccurred; transports do not have an equivalent reporting
surface, leaving bare-transport consumers without visibility.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[612-646]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[401-431]
src/Daqifi.Core/Device/DaqifiDevice.cs[510-528]
src/Daqifi.Core/Device/DaqifiDevice.cs[3082-3110]
PR-#354

Agent prompt

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

## Issue description
`HandleConnectionLost` in `SerialStreamTransport`/`TcpStreamTransport` now catches and swallows exceptions from `OnStatusChanged(...)` with no logging or reporting. This makes transport-level `StatusChanged` subscriber failures (and failures in overridden `OnStatusChanged`) silent for consumers using transports directly.

## Issue Context
Device-level `DaqifiDevice.StatusChanged` failures are surfaced via `ErrorOccurred` (`RaiseDeviceError`), but transports have no equivalent surface. Past fixes in this repo emphasize that isolation boundaries should still be observable via best-effort logging, and that logging itself must not be allowed to throw.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[612-646]
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[401-431]

## Suggested fix
- Capture the exception in the `catch (Exception ex)`.
- Add best-effort logging inside the catch, e.g. `Trace.WriteLine(...)` wrapped in its own `try/catch` so a throwing `TraceListener` can’t escape (consistent with prior patterns in the repo).
- Keep the existing `finally` disposal behavior unchanged.
- (Optional) Consider narrowing what’s caught to subscriber invocation only (vs. exceptions from overrides), but if you keep catching broadly, ensure it’s at least logged.

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

---

4. Global Trace listener mutation ⊘ Outdated 🐞 Bug ☼ Reliability

Description

The new test mutates process-global Trace.Listeners to capture output; this can cause cross-test
interference in parallel runs (extra captured output or affecting other tests' tracing behavior).
The StringWriter being synchronized does not isolate the global listener registration itself.

Code

src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[R121-124]

+        var captured = new StringWriter();
+        var listener = new TextWriterTraceListener(TextWriter.Synchronized(captured));
+        Trace.Listeners.Add(listener);
+        try

Relevance

● Weak

Similar Trace.Listeners parallel-contamination concern was explicitly rejected previously.

PR-#428

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test explicitly adds and removes a listener from the global Trace.Listeners collection around
the drop simulation and assertions.

src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[110-139]

Agent prompt

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

### Issue description
The test adds/removes a `TextWriterTraceListener` to the global `Trace.Listeners` collection. This is shared across the whole test process and can interfere with concurrently running tests.

### Issue Context
The writer is synchronized, but the global listener collection is still mutated for the duration of the test.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[121-139]

### Suggested fix options
Pick one:
1. Put this test/class into a non-parallel xUnit collection (disable parallelization for that collection).
2. Guard global listener install/remove with a static lock used by any test that touches `Trace.Listeners`.
3. If feasible, avoid global `Trace.Listeners` mutation by capturing via an injectable tracing abstraction (test seam) instead of `System.Diagnostics.Trace`.

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

---

Tip of the day

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

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 720da1b

Results up to commit 21780f5 ⚖️ Balanced

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

1. Silent swallowed status errors ✓ Resolved 🐞 Bug ◔ Observability

Description

SerialStreamTransport.HandleConnectionLost (and the same pattern in TcpStreamTransport) now swallows
any exception thrown by OnStatusChanged without logging or surfacing it, so bare-transport
StatusChanged handler bugs (or an overridden OnStatusChanged failure) can become completely silent.
This reduces diagnosability and can mask transport-subclass defects.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R624-627]

        catch (Exception)
        {
-            // The device is already gone; failing to close its handle changes nothing.
+            // A subscriber that throws must not cost us the port handle (issue #494). The reference
+            // has already been taken out of the field, so if this unwound past the dispose below,

Relevance

●●● Strong

Team often isolates subscriber exceptions but still logs best‑effort to avoid silent failures.

PR-#354
PR-#428
PR-#360

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new catch blocks in both transports swallow exceptions from OnStatusChanged without emitting any
log/event, while OnStatusChanged is protected virtual and dispatches StatusChanged, so both
subscriber and override exceptions are suppressed. In contrast, device-level StatusChanged failures
are surfaced via RaiseDeviceError/ErrorOccurred; transports do not have an equivalent reporting
surface, leaving bare-transport consumers without visibility.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[612-646]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[401-431]
src/Daqifi.Core/Device/DaqifiDevice.cs[510-528]
src/Daqifi.Core/Device/DaqifiDevice.cs[3082-3110]
PR-#354

Agent prompt

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

## Issue description
`HandleConnectionLost` in `SerialStreamTransport`/`TcpStreamTransport` now catches and swallows exceptions from `OnStatusChanged(...)` with no logging or reporting. This makes transport-level `StatusChanged` subscriber failures (and failures in overridden `OnStatusChanged`) silent for consumers using transports directly.

## Issue Context
Device-level `DaqifiDevice.StatusChanged` failures are surfaced via `ErrorOccurred` (`RaiseDeviceError`), but transports have no equivalent surface. Past fixes in this repo emphasize that isolation boundaries should still be observable via best-effort logging, and that logging itself must not be allowed to throw.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[612-646]
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[401-431]

## Suggested fix
- Capture the exception in the `catch (Exception ex)`.
- Add best-effort logging inside the catch, e.g. `Trace.WriteLine(...)` wrapped in its own `try/catch` so a throwing `TraceListener` can’t escape (consistent with prior patterns in the repo).
- Keep the existing `finally` disposal behavior unchanged.
- (Optional) Consider narrowing what’s caught to subscriber invocation only (vs. exceptions from overrides), but if you keep catching broadly, ensure it’s at least logged.

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

---

Results up to commit 482390d ⚖️ Balanced

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

1. Unsafe trace message build ✓ Resolved 🐞 Bug ☼ Reliability

Description

SerialStreamTransport/TcpStreamTransport build the interpolated trace message (including ex)
outside SafeTrace, so if exception stringification throws, the drop-path can still let an
exception escape HandleConnectionLost. Resource disposal still happens in finally, but the
caller thread (watchdog / I/O loop) can still be disrupted despite the intent to swallow the
subscriber failure.

Code

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[R633-636]

+            // ErrorOccurred; this transport carries no logger, so a consumer working against a bare
+            // transport gets the same best-effort trace DeviceFinderBase gives its event raises.
+            SafeTrace(
+                $"[{nameof(SerialStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {ex}");

Relevance

●●● Strong

Team previously accepted making trace/logging fully non-throwing to preserve isolation boundaries.

PR-#354

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The call site interpolates {ex} before entering SafeTrace, while SafeTrace only wraps
Trace.WriteLine, leaving message construction outside the containment boundary. TcpStreamTransport
mirrors the same pattern.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[620-674]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[408-452]
PR-#354

Agent prompt

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

### Issue description
`HandleConnectionLost` tries to swallow `OnStatusChanged` failures and only trace best-effort. However, it interpolates `{ex}` outside `SafeTrace`, so any exception thrown during message construction can escape the `catch` block.

### Issue Context
`SafeTrace` only guards `Trace.WriteLine(...)`, not the creation of the string passed to it.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[633-672]
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[417-450]

### Suggested fix
Change `SafeTrace` to accept an `Exception` and build the full message inside its internal `try/catch`, e.g.:

```csharp
private static void SafeTrace(string prefix, Exception ex)
{
   try
   {
       System.Diagnostics.Trace.WriteLine($"{prefix}{ex}");
   }
   catch
   {
   }
}
```

Then call it without interpolating `{ex}` at the call site (or wrap the interpolation itself in a local `try/catch`). Apply the same pattern to both Serial and TCP transports.

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

---

2. Global Trace listener mutation 🐞 Bug ☼ Reliability

Description

The new test mutates process-global Trace.Listeners to capture output; this can cause cross-test
interference in parallel runs (extra captured output or affecting other tests' tracing behavior).
The StringWriter being synchronized does not isolate the global listener registration itself.

Code

src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[R121-124]

+        var captured = new StringWriter();
+        var listener = new TextWriterTraceListener(TextWriter.Synchronized(captured));
+        Trace.Listeners.Add(listener);
+        try

Relevance

● Weak

Similar Trace.Listeners parallel-contamination concern was explicitly rejected previously.

PR-#428

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test explicitly adds and removes a listener from the global Trace.Listeners collection around
the drop simulation and assertions.

src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[110-139]

Agent prompt

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

### Issue description
The test adds/removes a `TextWriterTraceListener` to the global `Trace.Listeners` collection. This is shared across the whole test process and can interfere with concurrently running tests.

### Issue Context
The writer is synchronized, but the global listener collection is still mutated for the duration of the test.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Communication/Transport/DropPathSubscriberIsolationTests.cs[121-139]

### Suggested fix options
Pick one:
1. Put this test/class into a non-parallel xUnit collection (disable parallelization for that collection).
2. Guard global listener install/remove with a static lock used by any test that touches `Trace.Listeners`.
3. If feasible, avoid global `Trace.Listeners` mutation by capturing via an injectable tracing abstraction (test seam) instead of `System.Diagnostics.Trace`.

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

---

[tylerkron]

/agentic_review

[tylerkron]

Qodo-clean, CI green — ready for review.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 482390d

[tylerkron]

Valid — fixed in 8524922.

Confirmed by reproducing it: an exception whose ToString() throws escapes HandleConnectionLost at DefaultInterpolatedStringHandler.AppendFormatted, i.e. from inside the catch that exists to contain the subscriber. The handle still goes out in the finally, but the watchdog/reader thread is disrupted anyway — which is the thing the isolation is for. And the reasoning is the PR's own: the subscriber's exception came out of consumer code exactly as the subscriber did, so rendering it is no safer than raising to it.

SafeTrace now takes the Exception and composes the whole line inside its guard, in both transports. Two new tests (serial + TCP) pin it; both fail against the pre-fix code with the escape above, and the existing trace-content assertions still pass since the message text is unchanged.

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

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Round 3 triage — one taken, one declined.

"FIN-only TCP assertion" — valid, fixed in 720da1b. Replied on the inline thread and resolved it. Both TCP tests now accept EOF or ConnectionReset as peer-observed close, while still rejecting TimedOut — a leaked socket sends the peer neither a FIN nor an RST, so that outcome has to stay a failure or the test would pass on the very bug it pins.

"Global Trace listener mutation" — declining. This is the settled convention from #428, which Qodo's own relevance note flags (● Weak, "explicitly rejected previously"). The concerns don't land here either way: the listener is added and removed inside a try/finally so it cannot outlive the test; its writer is TextWriter.Synchronized, so concurrent trace traffic cannot corrupt the buffer; and the assertions are Contains, so unrelated traffic landing in the same buffer cannot fail them. In the other direction, a TextWriterTraceListener over a synchronized writer has no way to disturb a concurrently running test — it only ever appends. Serializing the class would cost real suite time to remove a hazard that isn't there.

Verification on 720da1b: full suite green on net9.0 (2915 Core + 43 Mcp) and net10.0 (2915), 0 failures, 0 warnings. Test-only round, so the bench result from 8524922 still stands.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 720da1b

[tylerkron]

Qodo-clean, CI green — ready for review.

Round 4 on head 720da1b: 0 unresolved threads, and all three raised findings struck through as resolved. The one remaining summary line ("Global Trace listener mutation") is the settled #428 convention, declined with reasoning above — Qodo did not re-raise it as a thread.