fix(producer): surface write failures via a SendFailed event

[tylerkron]

Summary

Closes #408.

MessageProducer<T>.Send() is fire-and-forget: the message goes onto a queue drained by a background thread, and if stream.Write fails, the failure was only logged at Warning. Nothing propagated to the caller, so a command that never reached the device looked identical, from the caller's point of view, to one that was delivered.

• Added MessageSendFailedEventArgs<T> and a SendFailed event on IMessageProducer<T>/MessageProducer<T>, raised (from the background thread, wrapped so a throwing subscriber can't kill the loop) for every failed write. Carries the failing message, the exception, and an IsTimeout flag.
• Added a forwarding SendFailed event on DaqifiDevice (mirroring the existing ChannelsPopulated-style concrete-class event), plus a log-warning subscriber wired at every place DaqifiDevice constructs a producer — so real device usage gets visibility today without needing feat: device-level ErrorOccurred event — background read errors and per-frame decode failures are currently invisible #378's broader device-level error surface.
• Gave a write timeout its own log message text, distinct from other write failures, so "device is busy/flow-controlled" is greppable separately (per the issue's suggestion SCPI Commands #3). Timeout classification for the health sink (fix(transport): detect a physically dropped serial/TCP connection so ConnectionStatus.Lost is reachable #403) is unchanged — a timeout still isn't reported as an ITransportHealthSink fault.
• Documented on Send()/IMessageProducer<T>.Send that delivery is not guaranteed, and added a "Delivery Failures" section to docs/DEVICE_INTERFACES.md.

This is intentionally scoped to option 2/3 from the issue (something to observe, plus greppable logs) rather than #378's full device-level ErrorOccurred surface, since #378 is still open — DaqifiDevice.SendFailed gives real callers a signal today and can be superseded/wired into that broader surface later.

Test plan

• New unit tests: SendFailed raised on write failure (non-timeout and timeout), timeout gets distinct log text, a throwing SendFailed subscriber doesn't kill the background loop, no event on a successful write, and DaqifiDevice.SendFailed/log-warning wiring.
• Full Daqifi.Core.Tests suite: 2191 passed, 2 skipped (pre-existing), 0 failed.
• Bench-tested against a real Nyquist device (/dev/cu.usbmodem1101): connect → stream 3s @ 10 Hz on channels 0+1 → stop → disconnect, exit code 0, no regression.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Surface producer write failures via SendFailed events

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

AI Description

• Add SendFailed event to producers to surface background write failures.
• Forward failures via DaqifiDevice.SendFailed and log distinct timeout warnings.
• Add unit tests and documentation on fire-and-forget delivery semantics.

Diagram

graph TD
  A["Caller code"] --> B["DaqifiDevice"] --> C["MessageProducer<T>"] --> D[("Device Stream")]
  D -- "Write throws" --> C
  C -- "Log warning" --> E["ILogger"]
  C -- "Raise" --> F(("Producer SendFailed")) --> B
  B -- "Log warning" --> E
  B -- "Forward" --> G(("Device SendFailed")) --> H["App subscriber"]

  subgraph Legend
    direction LR
    _cmp["Component"] ~~~ _evt(("Event")) ~~~ _io[("I/O Stream")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make Send() awaitable (Task/ValueTask with per-message completion)

• ➕ Gives callers an explicit delivery outcome per message
• ➕ Naturally supports retry/timeout policies at call sites
• ➕ Avoids background-thread event handling for many consumers
• ➖ Breaking API change and larger refactor across device/producers
• ➖ Risky to retrofit into existing fire-and-forget design
• ➖ Harder to preserve current throughput/queue semantics

2. Central device-level ErrorOccurred surface (#378-style)

• ➕ Single, consistent error channel across producer/consumer/transport
• ➕ Easier for apps to subscribe once and handle all device faults
• ➖ Broader scope than issue reliability: MessageProducer swallows write failures, so a command that never reached the device looks delivered #408; likely delays delivery of observability
• ➖ Requires decisions on severity, state transitions, and error taxonomy

3. Inject a failure sink/callback interface instead of events

• ➕ More controllable than events (ordering, backpressure, single handler)
• ➕ Easier to test deterministically without multi-subscriber concerns
• ➖ Adds plumbing and DI-style configuration requirements
• ➖ Less idiomatic for current event-heavy API surface (ChannelsPopulated, etc.)

Recommendation: Keep the PR’s approach: an observational SendFailed event plus timeout-distinct logging is a minimal, non-breaking way to surface dropped writes immediately, and it composes well with future work (e.g., wiring into a broader ErrorOccurred surface later). The main alternative (awaitable Send) is cleaner semantically but is a significantly larger, breaking design shift.

Files changed (7) +336 / -4

Enhancement (3) +108 / -0

IMessageProducer.csExpose SendFailed event and clarify Send() delivery semantics +16/-0

Expose SendFailed event and clarify Send() delivery semantics

• Updates Send() XML docs to explicitly describe fire-and-forget behavior and lack of delivery guarantee. Adds SendFailed event contract documenting background-thread raising and continued queue draining.

src/Daqifi.Core/Communication/Producers/IMessageProducer.cs

MessageSendFailedEventArgs.csAdd MessageSendFailedEventArgs<T> payload for failed deliveries +57/-0

Add MessageSendFailedEventArgs<T> payload for failed deliveries

• Introduces a new EventArgs type carrying the failed message, the exception, an IsTimeout convenience flag, and a UTC timestamp. Documents the rationale and that the event is purely observational (does not stop draining).

src/Daqifi.Core/Communication/Producers/MessageSendFailedEventArgs.cs

DaqifiDevice.csForward producer SendFailed via DaqifiDevice.SendFailed and log warning +35/-0

Forward producer SendFailed via DaqifiDevice.SendFailed and log warning

• Adds DaqifiDevice.SendFailed event and wires MessageProducer.SendFailed in both stream-backed and transport-backed constructors/Connect() path. Implements a handler that logs a warning with timeout metadata and forwards the event using SafeLog to avoid breaking the producer loop.

src/Daqifi.Core/Device/DaqifiDevice.cs

Bug fix (1) +23 / -2

MessageProducer.csRaise SendFailed on write exceptions; distinguish timeout warnings +23/-2

Raise SendFailed on write exceptions; distinguish timeout warnings

• Adds a SendFailed event to MessageProducer and raises it for each failed Stream.Write, guarded so subscriber exceptions cannot kill the background loop. Updates logging to emit a distinct warning message for TimeoutException versus other write failures, while keeping health-sink timeout classification unchanged.

src/Daqifi.Core/Communication/Producers/MessageProducer.cs

Tests (2) +187 / -2

MessageProducerTests.csAdd unit coverage for SendFailed behavior and timeout log text +96/-2

Add unit coverage for SendFailed behavior and timeout log text

• Adds tests asserting SendFailed is raised on write failure (timeout and non-timeout), that timeouts produce distinct warning text, that a throwing SendFailed subscriber does not break draining/StopSafely, and that no event is raised on successful writes. Enhances the ThrowOnWriteStream helper to support configurable exception types.

src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs

DaqifiDeviceWithMessageProducerTests.csVerify DaqifiDevice logs and raises SendFailed on producer write failure +91/-0

Verify DaqifiDevice logs and raises SendFailed on producer write failure

• Adds a throwing stream and in-memory logger to validate that device usage logs a warning for failed queued writes and forwards the failure through DaqifiDevice.SendFailed. Ensures callers have an observable signal despite fire-and-forget Send().

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

Documentation (1) +18 / -0

DEVICE_INTERFACES.mdDocument fire-and-forget Send() and how to observe delivery failures +18/-0

Document fire-and-forget Send() and how to observe delivery failures

• Adds a new "Delivery Failures" section explaining that Send() does not guarantee delivery and does not throw on background write failures. Documents the DaqifiDevice.SendFailed event and notes that the producer continues draining the queue after failures.

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. SendFailed handler not removed ✓ Resolved 🐞 Bug ☼ Reliability

Description

DaqifiDevice subscribes to MessageProducer.SendFailed but never unsubscribes when Disconnect()
abandons the producer reference; if StopSafely() returns while the producer thread is still alive
(e.g., blocked in Stream.Write/Flush), the producer keeps a strong reference to the device via the
delegate, potentially preventing GC and raising SendFailed/log warnings after disconnect/dispose.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[484]

+            _messageProducer.SendFailed += OnMessageSendFailed;

Relevance

●●● Strong

Team often fixes teardown/thread-lifetime hazards; prior PRs add explicit unsubscribe/stop/dispose
hardening in similar loops.

PR-#196
PR-#384
PR-#260

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The device now subscribes to the producer event, but Disconnect() only stops and nulls the producer
without unsubscribing; because the producer runs a background thread and writes synchronously to the
stream, a lingering thread keeps the producer alive, and the producer’s event delegate keeps the
device alive.

src/Daqifi.Core/Device/DaqifiDevice.cs[471-529]
src/Daqifi.Core/Device/DaqifiDevice.cs[574-617]
src/Daqifi.Core/Device/DaqifiDevice.cs[1429-1441]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[66-80]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[164-242]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[261-270]

Agent prompt

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

### Issue description
`DaqifiDevice` subscribes `OnMessageSendFailed` to `_messageProducer.SendFailed`, but `Disconnect()` later calls `_messageProducer?.StopSafely()` and then sets `_messageProducer = null` without removing the handler. If the producer thread is still alive (e.g., blocked in synchronous stream I/O), the producer retains a strong reference to the device via the event delegate and can continue calling into the device after teardown.

### Issue Context
- `MessageProducer` uses a dedicated background `Thread` and performs synchronous `_stream.Write(...)` and `_stream.Flush()`, which can block.
- `Disconnect()` nulls the producer field (so a subsequent `Dispose()` also can’t reach/dispose it via the field).

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[477-529]
- src/Daqifi.Core/Device/DaqifiDevice.cs[574-617]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1432-1441]

### Suggested fix
1. In `Disconnect()`, before clearing `_messageProducer`, capture it into a local variable.
2. If non-null, detach the handler (`producer.SendFailed -= OnMessageSendFailed`) before/while stopping.
3. Ensure the old producer is disposed via the local variable (even if the field is later nulled), then set `_messageProducer = null`.
4. Consider applying the same local-variable pattern for `_messageConsumer` as well (optional), but the critical part is detaching `SendFailed` and disposing the abandoned producer instance.

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

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4c6e56c