fix(device): cap the backlog of sends parked during a long exclusive operation

[tylerkron]

What was wrong

Send() never blocks — when another flow owns the device it parks the message and returns, and the message goes out in order once that flow finishes. The queue it parked into had no limit.

That is fine for a text query, which owns the device for milliseconds. It is not fine for the operations that own it for a long time: an SD card download is allowed thirty minutes, and a firmware update longer. A UI or agent sending DIO/PWM/status commands at even 10 Hz throughout a download parked about 18,000 closures — a couple of megabytes, plus every message object they held alive — and then, the moment the download ended, replayed all of them at the device in one burst. So the memory growth came with a second surprise: thousands of commands the caller had long since superseded, all arriving at once at a device that had moved on.

How it was fixed

The backlog is capped at DaqifiDevice.DefaultMaxDeferredSends (1024) and overflows by discarding its oldest entries. Drop-oldest is the right way round for what actually gets parked — these are level-setting commands ("set this pin", "set that duty cycle"), where the newest instruction is the one the caller currently wants and the superseded one is worth less than the memory it costs. Discards are counted by a new DroppedDeferredSendCount property, mirroring the existing DroppedLiveSampleCount, and the first overflow of each backlog logs one warning so this is not something you have to already know about to notice.

Things you may want to push back on:

• 1024 is a judgement call. It is far above any backlog an ordinary text query can build, so the cap only ever engages on the long exclusive operations it exists for — but it still means a burst of up to 1024 replayed commands at the end of a download, just no longer 18,000. Coalescing per-command ("only the latest write to this pin survives") would fix the burst too, and is deliberately not done here: it needs a notion of message identity the outbound layer doesn't have.
• The cap is a constant, not an option. There is no DeviceConnectionOptions knob for it. internal virtual MaxDeferredSends exists purely as a test seam.
• Drops are silent apart from the counter and one log line. They do not raise SendFailed. Raising a public event from inside the deferral gate would put consumer code under a lock that Send() promises not to block on, and moving it outside makes ordering murky; the counter is the pattern the library already uses for the identical drop-oldest decision on live samples.
• DefaultMaxDeferredSends and DroppedDeferredSendCount are new public members. Additive only — no interface changed, no existing signature touched.

Verification

• 6 new tests in DaqifiDeviceOperationSerializationTests: drop-oldest keeps the newest N in order; a backlog under the cap drops nothing (the guard on the existing ordering guarantees, which are unchanged); 2000 sends hammered through a held operation leave the backlog bounded, sampled as it grows rather than only at the end; the shipped default cap really is the one enforced; an idle device drops nothing even with a cap of one; and the counter accumulates across operations while the warning stays at one per overflowing backlog. Four of the six were re-run against the un-fixed code first — all four failed, the two guards passed — so they pin the bug rather than the fix.
• Full suite green on net9.0 (2908 Core + 43 Mcp) and net10.0 (2908), 0 failures, 0 warnings.
• Bench, fw 3.7.2 on /dev/cu.usbmodem1101: four connect → status → 3 s @ 500 Hz stream → disconnect cycles, exit 0, 1188–1189 samples (this unit's known ~79% clock ratio), sn and firmware unchanged, clean Disconnected. Non-destructive; serial only.

closes #492

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Cap deferred Send() backlog during long exclusive operations

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Cap deferred Send() backlog during long exclusive operations to prevent unbounded memory growth.
• Drop oldest deferred sends on overflow, count drops, and warn once per overflow episode.
• Add regression tests and document the new cap and observability surface.

Diagram

graph TD
A[Caller thread] --> B["DaqifiDevice.Send<T>()"] --> C{"Exclusive op in flight?"}
C -->|No| D["SendNow()"] --> E["IStreamTransport.Write"]
C -->|Yes| F["Deferred queue"] --> G["Cap + drop oldest"] --> H["FlushDeferredSends()"] --> E
G --> I["Warn once (logger)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Coalesce deferred commands by identity (last-write-wins)

• ➕ Reduces/avoids the post-operation burst, not just its maximum size
• ➕ Better matches semantics for level-setting commands (DIO/PWM)
• ➖ Requires a stable notion of command identity across message types that the outbound layer does not currently model
• ➖ More invasive change: impacts ordering guarantees and likely requires new data structures and APIs

2. Make the cap configurable via connection/device options

• ➕ Lets consumers tune memory vs. burst behavior per environment/workload
• ➕ Avoids a new “magic constant” becoming a de facto contract
• ➖ Expands public configuration surface and compatibility/testing matrix
• ➖ Still doesn’t address burst semantics unless paired with coalescing

3. Use a fixed-size ring buffer/deque for deferred sends

• ➕ O(1) drop-oldest at cap without repeated Queue.Dequeue in a loop
• ➕ Makes the cap behavior explicit in the data structure
• ➖ Requires a new implementation (or dependency) and careful testing to preserve ordering/replay behavior
• ➖ Current loop is simple and sufficient at typical caps (1024)

Recommendation: The PR’s approach (bounded backlog with drop-oldest + counter + one warning per overflow episode) is the best near-term fix: it preserves Send() non-blocking behavior and existing ordering guarantees while preventing unbounded memory growth. Coalescing would be a stronger semantic solution to stale-command bursts, but it needs message identity plumbing that is out of scope for a targeted fix.

Files changed (3) +442 / -5

Bug fix (1) +108 / -5

DaqifiDevice.csCap deferred Send() backlog, count drops, and warn once per overflow episode +108/-5

Cap deferred Send() backlog, count drops, and warn once per overflow episode

• Implements a bounded deferred send queue via DefaultMaxDeferredSends and internal MaxDeferredSends (test seam), enforcing drop-oldest when the cap is reached. Adds DroppedDeferredSendCount counter (Interlocked-backed) and a per-backlog overflow warning emitted outside the lock to preserve non-blocking Send() semantics; resets overflow reporting when deferral state is cleared/drained.

src/Daqifi.Core/Device/DaqifiDevice.cs

Tests (1) +325 / -0

DaqifiDeviceOperationSerializationTests.csAdd regression tests for deferred-send backlog capping, drop order, and logging +325/-0

Add regression tests for deferred-send backlog capping, drop order, and logging

• Introduces a focused test suite for issue #492: verifies drop-oldest semantics, bounded backlog under sustained hammering, default cap usage, and no drops on idle devices. Adds a capped test device seam (override MaxDeferredSends), plus a capturing ILogger to assert the “warn once per overflow episode” behavior.

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

Documentation (1) +9 / -0

DEVICE_INTERFACES.mdDocument bounded deferred-send backlog and drop-oldest behavior +9/-0

Document bounded deferred-send backlog and drop-oldest behavior

• Adds documentation describing the deferred Send() backlog cap (DefaultMaxDeferredSends=1024) and why it only matters for long exclusive operations. Explains drop-oldest rationale, how to observe drops via DroppedDeferredSendCount, and what a growing value implies.

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. TCS continuation reentrancy ✓ Resolved 🐞 Bug ☼ Reliability

Description

New backlog-cap tests create TaskCompletionSource instances with default options, allowing
continuations to run inline on SetResult; since SetResult is called from inside RunExclusiveAsync
bodies, this can introduce scheduler-dependent reentrancy and intermittent test flakiness/hangs.
This is inconsistent with other tests in the suite that explicitly use
RunContinuationsAsynchronously for cross-thread signaling.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceOperationSerializationTests.cs[R471-478]

+        var entered = new TaskCompletionSource();
+        var release = new TaskCompletionSource();
+
+        var operation = Task.Run(() => device.RunExclusiveAsync(async _ =>
+        {
+            entered.SetResult();
+            await release.Task;
+        }));

Relevance

●●● Strong

Team repeatedly hardens concurrency tests to avoid hangs/flakes; async TCS continuation safety fits
that pattern.

PR-#421
PR-#411
PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added tests create TaskCompletionSource instances without
TaskCreationOptions.RunContinuationsAsynchronously, and call SetResult() inside the
exclusive-operation delegate, which is the classic scenario where inline continuations can cause
reentrancy. Elsewhere in the test suite, similar signaling is created with
RunContinuationsAsynchronously, demonstrating the established safer pattern in this repo.

src/Daqifi.Core.Tests/Device/DaqifiDeviceOperationSerializationTests.cs[471-478]
src/Daqifi.Core.Tests/Device/DaqifiDeviceAsyncLifecycleTests.cs[148-165]
src/Daqifi.Core.Tests/Device/Discovery/SerialDeviceFinderTests.cs[370-373]

Agent prompt

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

## Issue description
New tests coordinating threads via `TaskCompletionSource` use the default continuation behavior. Because `entered.SetResult()` is called from inside `RunExclusiveAsync(...)`, the awaiting test continuation can run inline on that same thread, creating reentrancy/scheduling-dependent behavior that can intermittently hang or flake under different runners/loads.

## Issue Context
Other tests in this repository already standardize on `TaskCreationOptions.RunContinuationsAsynchronously` for cross-thread signaling to prevent inline continuation execution and reduce nondeterminism.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceOperationSerializationTests.cs[471-685]

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

Round 1 triage — taken, with one note on the reasoning.

"TCS continuation reentrancy" — valid, fixed in 3e6d4d3. All ten TaskCompletionSource instances in the new tests now use TaskCreationOptions.RunContinuationsAsynchronously.

Worth being precise about what the hazard is and isn't, since it changes nothing about the production fix. An inline resumption here could not have made the test's sends bypass deferral: OwnsCurrentSession is an AsyncLocal read, and a task continuation restores the execution context captured at its own await, not the completer's — so the test flow never inherits the operation's ownership regardless of which thread it lands on. What the default TCS does buy is scheduler-dependence: entered.SetResult() inside the exclusive body can run the rest of the test synchronously on the operation's thread, which makes the Send_HammeredThroughoutALongOperation_LeavesTheBacklogBounded loop — 2000 sends and an assertion per iteration — run inside the very operation it is meant to be racing. That is not a flake I observed, but it is real nondeterminism for no benefit, and the repo already standardizes the other way. Taken.

Note the shape came from the existing deferral tests directly above it in the same file, which still use the default TCS; those are left alone as out of scope for this PR.

Full suite re-run green after the change: net9.0 (2908 Core + 43 Mcp) and net10.0 (2908), 0 failures, 0 warnings. Test-only commit, so the bench result on 764be40 still stands.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3e6d4d3

[tylerkron]

Qodo-clean, CI green — ready for review.

Round 2 on head 3e6d4d3: Bugs (0) / Rule violations (0) / Skill insights (0), round 1's finding struck ✓ Resolved, 0 unresolved review threads. Freshness confirmed — the bot posted "review updated up to the latest commit 3e6d4d3", and the in-place-edited review comment carries 8 references to 3e6d4d3 and none to 764be40. Re-checked both surfaces again six minutes later in case a thread was still in flight: unchanged.

CI build pass on 3e6d4d3; MERGEABLE / CLEAN. Full suite green on net9.0 (2908 Core + 43 Mcp) and net10.0 (2908), 0 failures, 0 warnings.

git diff 764be40..3e6d4d3 -- src/Daqifi.Core/ is empty — the review round changed tests only, so the original bench validation on /dev/cu.usbmodem1101 (fw 3.7.2, four connect → status → 3 s @ 500 Hz → disconnect cycles, 1188–1189 samples, sn unchanged, clean Disconnected) stands unchanged.

Not merging — for review.