chore: wire ILogger into MessageProducer (closes #222)

[tylerkron]

Summary

Closes #222.

MessageProducer<T> swallowed two distinct exception paths behind placeholder // TODO: Add proper logging system in future step comments — the per-message write failure and the background-thread loop guard. That "future step" has effectively happened: the project already references Microsoft.Extensions.Logging.Abstractions and injects ILogger<FirmwareUpdateService>. MessageProducer was the last major component still silently swallowing.

Silent swallowing here is a debugging black hole: if writes start failing mid-stream, the only signal a desktop client sees is "samples stopped flowing." This surfaces those exceptions through ILogger<MessageProducer<T>> without changing the public API for existing callers.

Changes

• Optional logger injection — constructor now accepts ILogger<MessageProducer<T>>? logger = null, defaulting to NullLogger<MessageProducer<T>>.Instance. Nullable + default keeps the three existing single-arg call sites (DaqifiDevice, SerialDeviceFinder) source-compatible and behaviourally unchanged.
• Both TODOs replaced — per-message write failure → LogWarning(ex, …) (keeps draining the queue); background-loop guard → LogError(ex, …) (keeps the loop running).
• Lifecycle visibility (bonus) — LogInformation on a clean loop exit, LogError on an abnormal one. The last-resort handler wraps its own logging call so a faulting logger can never escalate into an unhandled background-thread exception (which would crash the host process).

Tests

• MessageProducer_WhenWriteThrows_ShouldLogWarning — a throwing stream causes a Warning carrying the original IOException.
• MessageProducer_WithNoLogger_WhenWriteThrows_ShouldNotThrowToCaller — the default (no-logger) path still silently continues, proving unchanged behaviour for existing consumers.
• MessageProducer_WhenStoppedNormally_ShouldLogCleanExit — a normal stop emits the clean-exit Information log.

No Moq in this project, so the tests use a small hand-written CaptureLogger + throwing stream, matching the existing ErrorThrowingStream pattern.

Out of scope

Plumbing ILogger through every other class (e.g. DaqifiDevice, SerialDeviceFinder) — this PR is specifically about closing the MessageProducer TODOs, per the issue.

Validation

• dotnet build — clean (0 warnings / 0 errors; the project builds with TreatWarningsAsErrors).
• dotnet test (net10.0) — 1167 passed, 2 skipped, 0 failed.

Acceptance criteria

• Both TODO comments removed; exceptions are logged.
• MessageProducer constructor accepts an optional ILogger<MessageProducer<T>>.
• Default behaviour (no logger passed) is unchanged for existing consumers.
• Tests cover the case where a write throws — assert the logger is invoked.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Wire ILogger into MessageProducer to surface background write failures
✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Description

• Add optional ILogger> with NullLogger default to preserve existing callers.
• Log per-message write failures and unexpected loop errors without stopping the producer.
• Add unit tests asserting warning/error/info logs and unchanged no-logger behavior.

Diagram

graph TD
  Callers(["Existing callers"]) --> Producer[["MessageProducer<T>"]] --> Loop(("Background loop")) --> Stream[("Output stream")]
  Loop --> Logger(["ILogger / NullLogger"])
  Tests(["Producer tests"]) --> Producer

  subgraph Legend
    direction LR
    _caller(["Caller"]) ~~~ _comp[["Component"]] ~~~ _thread(("Thread/loop")) ~~~ _io[("I/O target")
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Require ILogger and update all call sites

• ➕ Enforces consistent observability across all producer instantiations
• ➕ Avoids silent misconfiguration if someone forgets to pass a logger
• ➖ Breaking constructor change for existing consumers
• ➖ Forces broad plumbing beyond the scope of closing the TODOs

2. Expose write failures via callback/event instead of logging

• ➕ Keeps logging policy out of the producer
• ➕ Allows callers to react programmatically (e.g., reconnect/backoff)
• ➖ Expands the public API surface and increases usage complexity
• ➖ Still needs logging somewhere, so pushes the same work downstream

Recommendation: The PR’s approach (optional ILogger with a NullLogger default) is the best fit for the stated goal: it surfaces previously swallowed exceptions without forcing API breaks or broad dependency injection plumbing. Keeping the loop alive while logging warning/error events is appropriate for a long-running background producer and preserves the prior “don’t crash the app” behavior.

Files changed (2) +157 / -20

Enhancement (1) +50 / -20

MessageProducer.csAdd optional ILogger and log guarded background-loop failures +50/-20

Add optional ILogger and log guarded background-loop failures

• Adds an optional ILogger<MessageProducer<T>> constructor parameter defaulting to NullLogger. Replaces two silent exception-swallowing TODO paths by logging per-message write failures as warnings and unexpected loop exceptions as errors, plus information/error lifecycle logs on clean/abnormal loop termination with a last-resort guard to prevent logger failures from crashing the process.

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

Tests (1) +107 / -0

MessageProducerTests.csAdd tests for warning logging, no-logger behavior, and clean stop logs +107/-0

Add tests for warning logging, no-logger behavior, and clean stop logs

• Introduces a hand-rolled CaptureLogger<T> to assert emitted log levels and exceptions without Moq, plus a ThrowOnWriteStream to simulate mid-stream failures. Adds tests verifying that write exceptions are logged as warnings, that omitting a logger preserves non-throwing behavior, and that a normal stop produces an information lifecycle log.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 chore: wire ILogger into MessageProducer (resolve stale logging TODOs)

1. Faulting logger kills loop ✓ Resolved 🐞 Bug ☼ Reliability

Description

If an injected ILogger throws during LogWarning/LogError, that exception can escape the inner guards
and end ProcessMessages without clearing _isRunning, leaving the producer reporting IsRunning=true
while its background thread is dead. In that state, Send() keeps enqueueing messages and
StopSafely() can spin until timeout because the queue is no longer drained.

Code

src/Daqifi.Core/Communication/Producers/MessageProducer.cs[R165-197]

+                        catch (Exception ex)
+                        {
+                            // Surface the failure but keep draining the queue so a single
+                            // bad write doesn't stall the remaining messages.
+                            _logger.LogWarning(ex, "Failed to write message to the stream; continuing with remaining queued messages.");
+                        }
                    }
                }
+                catch (Exception ex)
+                {
+                    // Protect the background thread from unexpected exceptions so the
+                    // producer keeps running rather than dying silently.
+                    _logger.LogError(ex, "Unexpected error in the MessageProducer background loop; the loop will continue running.");
+                }
+            }
+
+            _logger.LogInformation("MessageProducer background loop exited cleanly after a stop was requested.");
+        }
+        catch (Exception ex)
+        {
+            // Reaching here means an exception escaped the inner guards (most likely
+            // the logger itself faulted). The background thread is ending abnormally.
+            // This is the thread's last-resort handler: an unhandled exception here
+            // would crash the host process, so logging must never be allowed to throw.
+            try
+            {
+                _logger.LogError(ex, "MessageProducer background loop terminated abnormally.");
            }
-            catch (Exception)
+            catch
            {
-                // Protect the background thread from unexpected exceptions
-                // TODO: Add proper logging system in future step
+                // Nothing safe left to do from a dying background thread.
            }
        }

Relevance

⭐⭐⭐ High

Team frequently accepts defensive loop-hardening to prevent faulted threads and stale state (PRs
#180, #248; state consistency in #214).

PR-#180
PR-#248
PR-#214

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ProcessMessages invokes _logger inside exception handlers without guarding those calls; if logging
throws, the method returns via the outer catch and never clears _isRunning. Send() uses _isRunning
to decide whether it can enqueue, and StopSafely() waits on the queue becoming empty—something that
won’t happen once the background thread has exited.

src/Daqifi.Core/Communication/Producers/MessageProducer.cs[48-141]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[93-121]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[146-198]

Agent prompt

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

### Issue description
`MessageProducer<T>.ProcessMessages()` calls `_logger.LogWarning(...)` / `_logger.LogError(...)` inside catch blocks without protecting against exceptions thrown by the logger itself. If the logger throws, `ProcessMessages()` can exit while `_isRunning` remains `true`, causing the producer to accept messages that will never be processed and making `StopSafely()` potentially wait until timeout.

### Issue Context
This PR added logging specifically in the exception paths inside the background thread. Logger implementations are not guaranteed to be non-throwing (custom test loggers, disposed logging providers during shutdown, etc.). The current outer `catch` prevents process-crashing unhandled exceptions, but it still allows the thread to die and leaves the object in an inconsistent “running” state.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[48-141]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[146-198]

Suggested approach:
- Wrap *each* `_logger.Log*` call in a small `SafeLog(Action log)` helper that swallows exceptions so logging cannot terminate the loop.
- Additionally, in the last-resort outer `catch`, set `_isRunning = false` (and possibly `_messageAvailable.Set()`) before returning so callers can’t keep enqueueing against a dead producer.

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

---

[tylerkron]

Response to Qodo review

🐞 Bug #1 — "Faulting logger kills loop" — ✅ Agreed and fixed (commit 02e3486)

Good catch — this goes one level deeper than the outer-handler hardening already in the PR. The original last-resort catch only prevented a throwing logger from crashing the process; it didn't stop a logger that throws from LogWarning/LogError in the inner handlers from unwinding the loop and returning with _isRunning still true. As Qodo notes, that leaves a dead background thread advertising IsRunning == true, so Send() keeps enqueueing messages that never drain and StopSafely() blocks until timeout.

Implemented the suggested two-part fix:

• SafeLog helper — every logger call in ProcessMessages is now routed through a helper that swallows logger exceptions, so a faulting logger can no longer unwind the loop. This is the primary fix and keeps _isRunning accurate.
• Clear _isRunning in the last-resort catch — defense-in-depth so that if anything ever does escape the loop, the producer doesn't report a running state with a dead thread.

Added a regression test (MessageProducer_WhenLoggerThrows_ShouldKeepDrainingAndStopCleanly): with an always-throwing logger and always-failing writes, the queue still fully drains, StopSafely(2000) returns true (no hang), and IsRunning is false afterward.

Validation: build clean (0 warnings under TreatWarningsAsErrors); full suite 1168 passed / 2 skipped / 0 failed on net10.0, producer tests green on both net9.0 and net10.0.

No other findings to address — Qodo reported 1 bug, 0 rule violations, 0 requirement gaps, and endorsed the optional-ILogger-with-NullLogger-default approach as the best fit for the ticket.