fix(producer): wait on idle instead of Sleep in StopSafely

[tylerkron]

What was wrong

MessageProducer.StopSafely() (called by Disconnect() and Dispose()) waited for pending commands by checking the queue every 10 ms. Two consequences:

• Disconnecting while commands were still queued sat on that 10 ms tick instead of returning as soon as the last write finished.
• It only looked at the queue, not at the write in progress. A command that had left the queue but was stuck in a write (for example, a device not draining its USB buffer) counted as "sent", so StopSafely reported success even though the command never went out.

How it was fixed

The producer's background thread now signals an event when it goes idle — nothing queued and no write in progress. StopSafely waits on that event with the caller's timeout, so it returns the moment the last write completes. If a write is still stuck when the timeout expires, it returns false and force-stops, as it already did for a full queue.

Normal disconnects behave the same, just without the polling delay. The only visible difference is the stuck-write case: StopSafely now returns false instead of true. No production caller in Core reads that return value.

Tests

• The existing drain test also asserts the producer is idle and stopped afterwards.
• One write held in progress plus one queued: StopSafely waits for both, then returns true.
• One write that never completes: StopSafely times out and returns false.

Pairs with #739, which removes the test-side sleeps. Both touch MessageProducerTests.cs in different places and merge cleanly in either order.

Verification

• Full suite, net9.0 + net10.0: 4340 passed, 3 skipped, 0 failed.
• Producer, device and OperationSerializer tests 20× under 4-core CPU load: 0 failures.
• No hardware attached; no bench run.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Disconnect can drop stop commands ✗ Dismissed 🔗 Cross-repo conflict ☼ Reliability

Description

SignalIdleIfQuiet can publish _idle while Send is between resetting the event and enqueueing,
or let a waiter consume the signal after work is queued but before its post-set check resets it,
while StopSafely trusts that signal without atomically revalidating the queue or active write.
Desktop and Avalonia enqueue the stop-streaming command immediately before Core disconnect during
teardown, so either interleaving can stop the producer and close the transport while that accepted
command remains queued and unwritten.

Code

src/Daqifi.Core/Communication/Producers/MessageProducer.cs[R392-395]

+            _idle.Set();
+            if (!_messageQueue.IsEmpty)
+            {
+                _idle.Reset();

Relevance

●●● Strong

Recent producer history accepts concurrency and lifecycle reliability fixes; this race can drop
queued teardown commands.

PR-#260
PR-#514

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Send resets _idle and enqueues in separate operations, while SignalIdleIfQuiet sets the event
before its second queue check, allowing idle to be observed either during the reset-to-enqueue gap
or briefly after a concurrent enqueue. StopSafely treats a successful wait as sufficient and
transitions to stopped without atomically revalidating that the queue is empty and no write is
active, despite success meaning that all pending messages were sent; Desktop and Avalonia expose
this race in normal teardown by calling StopStreaming, which queues the Core stop command,
immediately before cleanup calls Core Disconnect.

src/Daqifi.Core/Communication/Producers/MessageProducer.cs[232-237]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[388-397]
src/Daqifi.Core/Device/Internal/StreamingSessionController.cs[100-109]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[195-208]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[229-238]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[388-398]
src/Daqifi.Core/Communication/Producers/IMessageProducer.cs[19-25]
External repo: daqifi/daqifi-desktop, Daqifi.Desktop/Device/AbstractStreamingDevice.cs [398-423]
External repo: daqifi/daqifi-desktop, Daqifi.Desktop/Device/AbstractStreamingDevice.cs [1256-1273]
External repo: daqifi/daqifi-avalonia, Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs [702-729]
External repo: daqifi/daqifi-avalonia, Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs [2052-2074]

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 idle event can be observed as signaled when a concurrent `Send` has been accepted but is not yet safely drained: `Send` resets the event separately from enqueueing, and `SignalIdleIfQuiet` publishes idle before its post-set queue check. `StopSafely` can then stop the producer and return success while the queued stop-streaming command remains unwritten.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[190-208]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[229-238]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[232-237]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[388-398]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[388-397]
- src/Daqifi.Core.Tests/Communication/Producers/MessageProducerTests.cs[125-178]

## Recommended Fix
Use one shared synchronization mechanism to coordinate the reset-and-enqueue transition in `Send`, the empty-check-and-set transition in `SignalIdleIfQuiet`, and the final running-to-stopped transition. After the idle wait, atomically revalidate that the queue is empty and no write is active before stopping; otherwise continue waiting within the original timeout. Add deterministic concurrency coverage that pauses `Send` between resetting the event and enqueueing, runs idle signaling concurrently so enqueue occurs between the initial empty check and idle publication, and verifies that `StopSafely` cannot return until the message drains.

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

---

Context sources

✅ Cross-repo context — repo relationships

  Explored: repo: daqifi/daqifi-python-test-suite (sha: 010c9d6d)

  Explored: repo: daqifi/daqifi-avalonia (sha: 25df7b82)

  Explored: repo: daqifi/daqifi-desktop (sha: 36995fed)

Review mode: ⚖️ Balanced: This is a concurrency-sensitive runtime change coordinating sends, queue state, idle signaling, and shutdown behavior, so it warrants a careful single-pass review.

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1be2ec7

[tylerkron]

Reviewed (Claude): approve after fixing a real race where a Send right before Disconnect could be dropped (Send's reset+enqueue and the idle signal now share a lock). Qodo-clean on 1be2ec7, CI green — ready for review.

[qodo-code-review]

PR Summary by Qodo

Wait for producer idle state during safe shutdown

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Replaces queue polling with an idle event covering queued and in-flight writes.
• Synchronizes enqueueing with idle signaling to prevent premature shutdown.
• Adds regression coverage for draining and stuck-write timeout behavior.

Diagram

sequenceDiagram
    actor Caller
    participant Producer as Message Producer
    participant Idle as Idle Event
    participant Queue as Message Queue
    participant Worker as Producer Thread
    participant Stream as Output Stream
    Caller->>Producer: Send message
    Producer->>Idle: Reset under lock
    Producer->>Queue: Enqueue under lock
    Worker->>Queue: Dequeue batch
    Worker->>Stream: Write messages
    Worker->>Idle: Set when quiet
    Caller->>Producer: StopSafely
    Producer->>Idle: Wait with timeout
    alt Idle signaled
        Idle-->>Producer: Drain complete
        Producer-->>Caller: Stop and return true
    else Timeout
        Producer-->>Caller: Force-stop and return false
    end

Loading

High-Level Assessment

The waitable idle event is the appropriate approach because StopSafely is synchronous and the producer already tracks queue and write activity. Sharing a short lock between enqueueing and idle signaling closes the lost-signal race; continued polling would retain latency, while introducing tasks or additional counters would add lifecycle complexity without improving the shutdown contract.

Files changed (2) +128 / -18

Bug fix (1) +71 / -18

MessageProducer.csReplace shutdown polling with synchronized idle signaling +71/-18

Replace shutdown polling with synchronized idle signaling

• Adds a waitable idle event representing an empty queue with no write in progress, allowing StopSafely to wait directly instead of polling. Synchronizes Send with idle signaling to prevent a newly queued message from being mistaken for an idle producer, and disposes the new event with the producer.

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

Tests (1) +57 / -0

MessageProducerTests.csCover safe shutdown with active and blocked writes +57/-0

Cover safe shutdown with active and blocked writes

• Extends the existing drain test with final idle and stopped-state assertions. Adds regression tests proving StopSafely waits for an active write plus queued work and returns false when an in-flight write exceeds the timeout.

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1be2ec7