fix(device): Send() racing a disconnect now fails typed, not with an NRE

[tylerkron]

Closes #497.

Not merging — this is for your review.

What was wrong

Disconnect a device while anything was still sending to it and Send could throw a NullReferenceException straight out of a public API. Lose the same race a fraction later and you instead got a bare InvalidOperationException reading "Message producer is not running. Call Start() first." — an internal lifecycle detail, not something a caller can act on.

Either way you did not get DeviceNotConnectedException, which is the exception this library tells you to catch for exactly this situation. So an ordinary shutdown surfaced as a confusing crash, and there was no way to tell it apart from a real defect in your own code without matching on message text.

It was also not limited to you calling Disconnect(). Every auto-reconnect attempt tears the send path down the same way, so any long-lived sender — a telemetry loop, a UI poll — was exposed on every reconnect, including ones it never asked for.

How it was fixed

SendNow now reads the message producer once into a local, instead of null-checking the field and then dereferencing it a second time. That closes the NullReferenceException window by construction. If teardown gets to the producer after that read, the resulting failure is translated into DeviceNotConnectedException with IsShuttingDown set — so a shutdown race reports as a shutdown race.

Things you may want to push back on:

• The translation is exception-type-based. It catches what a stopped or disposed producer throws. Non-lifecycle failures (an IOException, say) keep their own type, and a DeviceNotConnectedException from the producer is rethrown untouched, so this cannot mask a real fault — but it does depend on the producer's choice of exception type. Tests pin that against the real MessageProducer<string>, so a change there fails here rather than silently escaping.
• ObjectDisposedException must be caught before InvalidOperationException, since it derives from it. That ordering is load-bearing and a test pins it, but it is the kind of thing a future edit can quietly break.
• SendViaProducer is internal, not private, purely so the translation can be driven deterministically in tests. Same seam the registry already uses for DeviceConnector.
• This is not a lock. A lock spanning the send path and teardown would fix the race structurally, but it puts contention on the hot send path and risks deadlock against the locks teardown already takes. Snapshot-and-translate was the smaller change.

Verification

• Proved the test catches the bug: with the fix temporarily reverted, the stress test reproduced the raw InvalidOperationException on 5 of 5 runs, within ~25 reconnect cycles each. The NullReferenceException half is a far narrower window, so the stress test asserts the negative (nothing untyped escapes) rather than claiming to schedule that race; the snapshot is correct by inspection.
• 9 new tests in DaqifiDeviceSendDisconnectRaceTests.cs, driving every branch of the translation against both a throwing double and a real MessageProducer<string> (stopped and disposed), plus the reconnect stress loop and a happy-path guard.
• Full suite green on net9.0 (2902 Core + 43 Mcp) and net10.0 (2902) — 0 failures.
• Bench-validated on the real Nyquist over /dev/cu.usbmodem1101 (fw 3.7.2), non-destructive: two full connect → configure → stream → disconnect cycles, exit 0, 396 samples each (the expected count at this unit's known 79.4% clock ratio). No regression on the common path.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix Send/disconnect race: throw DeviceNotConnectedException instead of NRE/IOE

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Snapshot message producer in SendNow to avoid teardown-time null dereferences
• Translate stopped/disposed producer failures into DeviceNotConnectedException
 (IsShuttingDown=true)
• Add deterministic and stress tests to ensure no untyped exceptions escape during reconnect loops

Diagram

graph TD
  A(("Caller")) --> B["DaqifiDevice.Send"] --> C["SendNow: snapshot producer"] --> D["SendViaProducer"] --> E["IMessageProducer.Send"] --> F["DeviceNotConnectedException\n(IsShuttingDown)"]
  G(("Disconnect/reconnect thread")) --> H["StopMessagePumps"] --> I["Null/stop/dispose producer"]
  I -. "races" .-> C
  subgraph Legend
    direction LR
    _actor(("Thread/Caller")) ~~~ _proc["Method"] ~~~ _exc["Typed exception"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Lock send path + teardown around _messageProducer

• ➕ Eliminates the race structurally rather than translating outcomes
• ➕ Avoids relying on producer exception types/messages
• ➖ Adds contention/latency to the common Send path
• ➖ Risk of deadlocks if send/teardown interact with other locks (connectivity/state locks)

2. Replace mutable field with atomic state + cancellation

• ➕ Clear lifecycle model: producer stays non-null, but becomes logically inactive via cancellation/state
• ➕ Can make teardown idempotent without nulling shared references
• ➖ Bigger refactor across connect/disconnect/reconnect paths
• ➖ Requires careful coordination with existing pump start/stop semantics and tests

3. Queue send requests through a device-level channel that closes on disconnect

• ➕ Single choke point: disconnect closes the channel, Send fails deterministically
• ➕ Avoids producer-specific exception translation
• ➖ More architectural change (introduces a new queue/loop)
• ➖ Potentially changes timing/backpressure characteristics for existing callers

Recommendation: The PR’s approach (snapshot once + translate only lifecycle-related producer failures) is a good minimal-risk fix: it removes the NRE hazard by construction, preserves non-lifecycle exception types (no masking of real faults), and aligns the public API contract around DeviceNotConnectedException for expected teardown races. The heavier alternatives above can further simplify lifecycle semantics but are disproportionate for a targeted race fix.

Files changed (2) +470 / -3

Bug fix (1) +79 / -3

DaqifiDevice.csSnapshot _messageProducer and translate teardown send failures to typed exception +79/-3

Snapshot _messageProducer and translate teardown send failures to typed exception

• Updates SendNow to snapshot the mutable _messageProducer field before null-check/dereference to prevent a disconnect-time race causing NullReferenceException. Adds an internal SendViaProducer helper that rethrows DeviceNotConnectedException unchanged and translates ObjectDisposedException/InvalidOperationException from a stopped/disposed producer into DeviceNotConnectedException with IsShuttingDown set while preserving the original as InnerException; also expands XML docs to describe the race behavior as an expected shutdown outcome.

src/Daqifi.Core/Device/DaqifiDevice.cs

Tests (1) +391 / -0

DaqifiDeviceSendDisconnectRaceTests.csAdd deterministic + stress coverage for Send vs teardown race (#497) +391/-0

Add deterministic + stress coverage for Send vs teardown race (#497)

• Introduces a focused test suite that verifies SendViaProducer translates stopped/disposed producer failures into DeviceNotConnectedException (IsShuttingDown=true), preserves existing DeviceNotConnectedException, and does not translate non-lifecycle failures. Adds a disconnect/reconnect stress loop to assert that no NullReferenceException or other untyped exceptions escape Send during teardown races, plus a happy-path assertion that connected sends still reach the stream.

src/Daqifi.Core.Tests/Device/DaqifiDeviceSendDisconnectRaceTests.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. Stress test retains all exceptions ✓ Resolved 🐞 Bug ➹ Performance

Description

Send_RacingRepeatedDisconnectAndReconnect_OnlySurfacesTypedFailures appends every sender-thread
exception to a shared List<Exception> and only analyzes it at the end, causing avoidable exception
retention, allocation, and lock contention during the stress run. This can make the test itself
dominate the workload (rather than the race) and increase CI variance under heavy scheduling or when
many teardown races occur.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceSendDisconnectRaceTests.cs[R169-174]

+                catch (Exception ex)
+                {
+                    lock (failureGate)
+                    {
+                        failures.Add(ex);
+                    }

Relevance

●●● Strong

Team often accepts test-loop contention/flakiness reductions (yield/backoff, bounded waits) to
stabilize CI.

PR-#411
PR-#430

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The sender loop runs for up to 4 seconds and records every thrown exception by appending to a shared
list under a lock, which can accumulate many exception instances and add lock contention during the
tight loop.

src/Daqifi.Core.Tests/Device/DaqifiDeviceSendDisconnectRaceTests.cs[33-38]
src/Daqifi.Core.Tests/Device/DaqifiDeviceSendDisconnectRaceTests.cs[155-179]

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 stress test stores every exception thrown by the sender loop in an ever-growing `List<Exception>`, protected by a lock. Even though the run is time/cycle-bounded, this can still create unnecessary allocation pressure and lock contention and can distort the “stress” being applied.

### Issue Context
The test only needs to prove that **no untyped exception** (e.g., `NullReferenceException` or anything other than `DeviceNotConnectedException`) escapes. It does not need to retain every typed failure instance.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceSendDisconnectRaceTests.cs[155-216]

### Suggested fix
- Replace `List<Exception> failures` with:
 - An `int typedFailureCount` counter (optional), and
 - A single `Exception? firstUntyped` (or `ExceptionDispatchInfo?`) captured atomically, and/or
 - A small bounded buffer (e.g., keep first N exceptions) if you want diagnostics.
- In the sender loop:
 - If `ex is not DeviceNotConnectedException`, record it (if not already recorded) and set `stop = true` to end early.
 - Avoid locking on every exception; use `Interlocked.CompareExchange` to store the first untyped exception.
- After join:
 - Assert `firstUntyped == null` and also assert that no `NullReferenceException` was captured (can be included in the same check).

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

[tylerkron]

/agentic_review

[tylerkron]

Qodo-clean, CI green — ready for review. (Head 32ea856: Qodo round 2 reports Bugs (0) / Rule violations (0) / Requirement gaps (0) with the round-1 finding struck through, 0 unresolved threads, build passing. The only loop change was to the test file, so the bench validation on /dev/cu.usbmodem1101 above still stands as-is.)

[qodo-code-review]

Code review by qodo was updated up to the latest commit 32ea856