feat(device): serialize operations per device so concurrent callers stop corrupting each other's replies (closes #342) - #421
Conversation
…re indivisible sequences (closes #342) Core serialized text exchanges but nothing else, so a Send() from another thread could be written onto the wire while a text query owned the stream and have its reply collected as part of that query's answer. Every concurrent consumer had to build its own gate to avoid it. Core now owns it: - RunExclusiveAsync wraps a sequence of commands so nothing splits it. Reentrant on the same flow, so a body can call the SD/diagnostic methods that open a text exchange of their own, and can Disconnect. - Send() defers rather than blocks while another flow owns the device. It still returns immediately; the message goes out, in order, afterwards. - One lock, not two. RunExclusiveAsync and the text exchange share the existing per-device semaphore, so there is no ordering to get wrong. - The text exchange lets the outbound queue drain before it takes the stream, so replies to earlier commands go to the protobuf consumer. - The producer-less direct-write path is serialized; it was the one place SCPI bytes could genuinely interleave mid-command. The MCP server's hand-rolled gate shrinks to the connection registry, so two devices now run genuinely in parallel instead of behind one process-wide semaphore. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoSerialize operations per device with RunExclusiveAsync and deferred Send()
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
WiFi leg: attempted again, still blocked by the bench unit's WiFi stack. The unit had recovered on its own (3/3 ping, 0% loss, TCP 9760 open), so I retook the bench and went straight at the branch build over WiFi — the run that actually matters. It never got a measurement. The device left the network during the very first TCP connect, surfacing as an 8-second channel-configuration timeout, then 100% packet loss. No second attempt was made, per the don't-hammer-it rule. Immediately afterwards, over USB, the same branch build passed every phase again — 15/15 clean replies, 48/48 indivisible sequences with 0 overlaps, 1506 samples with no silent second, empty SCPI error queue. That is the third consecutive identical USB pass, so the unit is healthy and this is the transport wedging under cumulative connect churn, not a fault in the change. The PR body now records both WiFi attempts and what each did and did not show. Net position:
Re-running the WiFi leg after a physical power cycle is the one piece of verification still missing. The mechanism is not transport-specific — the lock sits above the transport and the code path is identical for serial and TCP — but I would rather flag that than let USB stand in for it silently. No code changed since the last review. |
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… round 1)
Both findings were real and both attacked the point of the change.
The outbound drain was not a barrier. It waited for QueuedMessageCount to
reach zero, but MessageProducer takes a message off the queue BEFORE it
writes it, so the count reads zero while the write is still going out. A
text exchange could still take the stream mid-write and collect that
command's reply as its own. MessageProducer now tracks the drain batch
itself and exposes IsIdle ("nothing queued and nothing being written"),
which is what the barrier waits on. IsIdle is a default interface member
carrying the old queue-only answer, so existing implementations compile and
behave unchanged.
Deferred sends could be overtaken. FlushDeferredSends cleared the
"deferring" flag before replaying, so a send arriving mid-replay saw
nothing in flight and went straight out ahead of messages parked before it.
Deferral now stays on for the whole replay, which still runs outside the
gate so Send() keeps not blocking; the flush drains in rounds and only
stops deferring in the same locked moment it observes an empty list. A
sender fast enough to refill every round is bounded by finishing the last
round under the gate.
Both are covered by tests confirmed to fail without their fix — the
ordering one reproduces the exact inversion (A1, B, A2, A3).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 1 — both bugs were real, both fixed in cf16f66Neither was a false positive, and both were in the synchronization this PR adds. Detailed replies are on the two inline threads; the short version: 1. The drain was not a barrier. It waited for
2. Deferred sends could be overtaken. Both are pinned by tests confirmed to fail without their fix — I reverted each fix in turn and watched the test go red, then restored it:
Writing the second one honestly cost a round: my first version sampled once at 400 ms and passed even with the fix reverted, because by then the exchange had finished and restarted the consumer. It now samples across the whole window. Full suite green: 2355 Core tests × net9.0/net10.0, 23 MCP tests. Still outstanding, unchanged: the WiFi bench leg. USB remains decisive (15/15 replies corrupted on |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit cf16f66 |
… across I/O (Qodo round 2) Round 1's bound on the flush was paid for in the wrong currency. Capping the rounds and finishing the last one under _deferralGate meant a concurrent Send() could block on replay I/O for as long as that took — measured at 462ms in the new test — which is exactly the guarantee deferral was chosen over waiting to provide. The backlog is now a queue that is drained one message at a time: the message is dequeued under the gate, replayed outside it, always, with no final under-gate stretch. What keeps ordering is no longer the flag alone but the backlog itself — it stays non-null (possibly empty) for the whole drain, and a send arriving mid-drain sees it and parks behind it instead of overtaking. It is nulled only in the same locked moment the drain observes it empty. Termination is still bounded, but by handing off rather than by blocking: after 64 messages the rest goes to a background empty exclusive operation, which is itself a flush. Going through the operation lock is the point — a bare background replay could write into a text exchange that started meanwhile. The backlog stays non-null across the handoff, so ordering survives it. Teardown now discards parked sends: they were addressed to a session that no longer exists, and a backlog outliving its drainer would leave the next session deferring into it. Also fixes a test that discarded the bool from its phase-boundary wait, so the competing sender could race a replay that never started and the ordering assertion would pass vacuously. The result is asserted, and the wait is shorter than the join that follows so the failure names itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 2 — both findings valid, fixed in 53036b6Both were consequences of my round-1 fixes, and the first one was the more embarrassing: I bought termination with the exact guarantee this design exists to provide. 1. Flush could block The backlog became a queue drained one message at a time: dequeued under the gate, replayed outside it, always. Ordering is now preserved by the backlog rather than by the flag — it stays non-null for the whole drain, Termination is still bounded, but by handing off rather than blocking: past 64 messages the remainder goes to a background empty exclusive operation, which is itself a flush. Routing it through the operation lock matters — a bare background replay could write into a text exchange that started meanwhile. Teardown now also discards parked sends: they belonged to a session that no longer exists, and a backlog outliving its drainer would leave the next session deferring into one nobody drains. 2. Test ignored its wait timeout. Same class of mistake I caught in my own drain test last round, so no defence for it. The competitor now captures the wait result, refuses to send if the replay never started, and the main thread asserts it. The helper wait is deliberately shorter than the join that follows, so a phase that never fires names itself instead of surfacing as a join timeout. Applied to the new probe thread too. Verified in both directions
Full suite green: 2356 Core × net9.0/net10.0, 23 MCP. The serialization suite was run three extra times to check the timing-sensitive assertions aren't flaky — 18/18 each time. Unchanged: the WiFi bench leg is still outstanding and the flag stays on this PR. USB remains decisive (15/15 corrupted on |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 53036b6 |
…o round 3) Last round's teardown change dropped the parked backlog but left the "an operation owns the device" flag set. That is the same hazard by a different route, and a worse one: with the flag stale, every Send() on the next session parks into a fresh backlog that nothing will drain, and because Send() reports success the moment it parks, the caller is told the command is on its way while it silently goes nowhere. Teardown is exactly where it gets stranded. The operation whose exit path would normally clear the flag is the one that failed to finish inside the bounded wait — so the disconnect proceeds without the lock and the flag outlives the session. That is the case Disconnect exists to handle, and it lands on the reconnect path from #379. Both halves now reset together under one lock. Swept the rest of the teardown path. MarkDisconnectedWithoutTeardown — the route taken when the LIFECYCLE lock is abandoned — is deliberately left alone: it tears nothing down, so the session is still owned by the stuck holder, and clearing deferral there would let sends bypass an operation that is genuinely still running on a live session. It reports Disconnected, so Send() throws rather than parking, and the stuck holder's own teardown resets the state when it unwinds. Test drives the abandoned-lock path specifically; a clean disconnect resets the flag via the operation's own exit path and would pass either way. Without the fix it reproduces the silent loss exactly: "'DIO:PORt:STATe' never reached the wire. Writes: ". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 3 — valid, fixed in 9b08889This one was my own round-2 reasoning left half-applied. I identified that a backlog outliving its drainer is a hazard, dropped the backlog, and left the flag — which reproduces the identical hazard by the other route, and a worse one. The failure mode is silent message loss. With the flag stale, every Both halves now reset together under one lock. Why clearing it mid-operation is safe (that being the case it fires in): the wedged operation still holds the semaphore, so no new operation can begin before it releases, and its own exit path runs before that release. It cannot clear the flag out from under a later operation. Swept the rest of the teardown path, as suggested. One other route reaches "disconnected" without the normal cleanup — Nothing else in the teardown path carries per-session state that the lock-held route resets and the abandoned route skips — VerifiedThe test drives the abandoned-lock path specifically. A clean disconnect resets the flag via the operation's own exit path and would pass either way, so it would have proved nothing. Without the fix: Full suite green: 2357 Core × net9.0/net10.0, 23 MCP. Unchanged: the WiFi bench leg is still outstanding and the flag stays on this PR. All three review rounds have been unit-testable, so no hardware was involved in any of them. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 9b08889 |
WiFi leg complete — the last outstanding item on this PRRun after a power cycle, branch first. The PR body now carries the full before/after for both transports; the caveat about re-running WiFi is gone because it has been run. WiFi (192.168.1.30, TCP 9760),
The headline over TCP is not the pollution, it is the 11 empty replies on Nothing deadlocked at the higher and far more variable latency (RTT 29–215 ms), and a live stream kept flowing while three threads drove DIO — the property the operation lock must never break. The one imperfect number, since I would rather point at it than have someone find it: the branch still shows a single stray line, A pattern I am explicitly not claiming as a result: both After the drop the unit still passed a full run over USB (15/15 clean, 0 stray lines, streaming fine, empty error queue), so the transport wedged, not the device. It did come back reporting 16 channels instead of 32 — digital channels missing — which is a device-side artifact of the wedge and clears on a power cycle. The WiFi stack will need another power cycle before the next agent's WiFi run. No code changed for any of this. The PR is unchanged since 9b08889. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 9b08889 |
… (Qodo round 4) The hazard is real: a flow that acquired the operation lock and then had the transport torn down and replaced underneath it kept bypassing deferral, writing straight into the new session while an unrelated operation owned it. But the cause was not the unconditional reset, and the proposed remedy — gating that reset on teardown having acquired the lock — makes things strictly worse. TryDeferSend asks about ownership BEFORE it looks at _operationInFlight, so the stale flow's bypass never consulted that flag at all. Leaving the flag set does not quiet the stale flow; it only silences every other flow, whose sends then park in a backlog with no drainer. That is round 3's silent loss plus the stale writer, and the round-3 test reproduces it verbatim against the proposed change. What was actually wrong was the ownership predicate. Holding the semaphore and owning the current session are different questions, and teardown is where they diverge. Ownership is now stamped with a session generation that every teardown retires: - Re-entrancy keeps asking the session-blind question. A flow that holds the semaphore must never be told to wait for it — ExecuteTextCommandAsync waits without a timeout, so a wrong answer there is a hang, not a degradation. - Send()'s bypass asks the session-scoped one. A flow from a retired session is not the owner of the current one and queues behind its rules. This also settles the round-3 asymmetry, which was right in outcome and wrong in reasoning. The principle is not "is an operation running" but "did the session end". FinishDisconnect: it ended, so the deferral state describes something that no longer exists — reset unconditionally. MarkDisconnectedWithoutTeardown: nothing was torn down, the stuck holder still owns a live transport, so the state is still live — leave it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 4 — hazard real, attribution wrong, fixed at the root in 8882518Three separate claims here, and they don't all land the same way. 1. My round-3 safety argument was incomplete — conceded. I argued "that operation still holds the semaphore, so no new operation can begin before it releases." That is about new operations. Plain 2. The hazard is real. A flow that acquired the operation lock and then had the transport torn down and replaced underneath it kept bypassing deferral, writing straight into the new session while an unrelated operation owned it. 3. But the proposed remedy does not fix it, and makes things worse. The actual cause, and the fixThe ownership predicate. Holding the semaphore and owning the current session are different questions, and teardown is exactly where they diverge. Ownership is now stamped with a session generation that every teardown retires:
Settling the round-3 asymmetryThis is the part worth recording, since it was the real question. The round-3 conclusions were right in outcome and wrong in reasoning. The principle is not "is an operation running" — it is "did the session end":
Both calls follow from one rule, which is what was missing before. Verified
Full suite green: 2358 Core × net9.0/net10.0, 23 MCP. Serialization suite run three extra times for flakiness — 20/20 each. Bench evidence is unaffected — this changes which flows may skip deferral, not the serialization the WiFi and USB runs measured, and no hardware was involved. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 8882518 |
1 participant
Closes #342.
Why
Core only ever serialized one thing: text queries (SD listings, diagnostics, capability reads). Everything else —
EnableChannel,SetDioValue,StartStreaming, network config — went straight out with no coordination at all.That combination has a sharp edge. While a text query is running, Core stops the normal reader and puts a temporary text reader on the stream to collect the reply. If another thread sends a command in that window, the device's answer to that command lands in the query's reply. The caller gets someone else's data back and has no way to tell.
It is not theoretical. On the bench, a status poll every 200 ms from two background threads was enough to corrupt every single query reply (below). And because the contract was never written down, every concurrent consumer had to discover this and build its own lock — our own MCP server included.
What
Core now owns this, and the contract is written down in
docs/DEVICE_INTERFACES.md:device.RunExclusiveAsync(...). That is the one thing a single call cannot express — "set the direction, then drive the pin" needs to happen with nothing in between.The MCP server's hand-rolled semaphore shrank to just the connection registry. A nice side effect: it used to be one process-wide lock, so two devices could never be operated at once. Now they can.
How
The interesting decisions:
One lock, not two.
RunExclusiveAsynctakes the same per-device semaphore text queries already use. A second, outer lock would have needed an ordering, and the code that would have to respect that ordering — disconnect, dispose, the reconnect loop, every SD operation — is exactly the code that must never deadlock. Reusing the existing lock means there is no ordering to get wrong. It is made reentrant per logical flow (theAsyncLocaltechnique already in this file), so an exclusive block can freely call the SD and diagnostic methods that open a text query of their own, and can callDisconnect.Send()defers instead of blocking.Send()is documented fire-and-forget, and making it wait on a lock would have been a silent contract change and a great way to stall a UI thread. So while another thread owns the device,Send()parks the message and returns immediately; the owner flushes it, in order, on the way out. The caller sees the same non-blocking call it always did — the message just reaches the device slightly later, which beats having it corrupt somebody's query. Called out in the docs.The reader is untouched. Streaming callbacks, the read loop and frame decode never take this lock, so a live stream keeps flowing while control operations run. Verified on hardware.
Also: the text query now lets the outbound queue drain before it takes the stream (so replies to earlier commands go to the normal reader where they belong), and the producer-less direct-write path got a lock — that was the one place SCPI bytes could genuinely interleave mid-command.
RunExclusiveAsyncis onDaqifiDevicerather thanIDevice, deliberately: adding it to the interface would break every external implementer for an API that only the concrete device can honour.Bench evidence
Nyquist 1, FW 3.7.2, over both transports. Same harness built twice — once against
origin/main, once against this branch — so it is a true before/after. Realistic load: two background threads doing a status poll plus DIO writes every 200 ms, while a foreground loop runs 15 text queries.The probe is
SYSTem:ERRor:COUNt?, whose clean reply is a single bare integer. Anything else in the reply is somebody else's data. The harness self-calibrates first by running the probe with no concurrent traffic (5/5 clean on both builds), so a firmware quirk can't be mistaken for the bug.USB (
/dev/cu.usbmodem1101)origin/mainA corrupted reply on
mainlooks like this — the real answer (0) followed by 50+ lines belonging to another thread:The branch run was repeated end-to-end and passed identically both times. Phase A also got ~4x faster (81.6s → 18.3s), because on
mainevery polluted query kept collecting until its inactivity timeout.Under a deliberately pathological load (~200 fire-and-forget queries/sec, far past anything real), the branch cut stray lines from 23,410 to 3,123 rather than to zero: replies to the deferred burst can still land in the next query. That is inherent to sending queries fire-and-forget —
Send()gives you nothing to match a reply against — and the fix for it is to use the…Asyncquery methods, which serialize properly. Recording it here rather than hiding it.WiFi (192.168.1.30, TCP 9760) — run after a power cycle, branch first.
origin/mainThe headline on
mainover WiFi is not the pollution, it is the 11 empty replies: the query's own answer never arrived at all. TCP delivers the interleaved traffic in different-sized chunks than USB CDC, so a command written into the exchange window doesn't just add a line — it can consume the reply the caller was waiting for. On this branch every query got its answer.Nothing deadlocked at the higher, more variable latency (RTT ranged 29–215 ms), and a live stream kept flowing throughout while three threads drove DIO — the specific thing the operation lock must never block.
The one imperfect number is the single stray line on the branch, and it is the fire-and-forget-query residual described above, not a serialization failure. The giveaway is the ordering:
["0,\"No error\"", "0"]— the stray line arrives before the real answer, i.e. it is a late reply to an earlier deferred command, which WiFi's ~100 ms round trip makes far likelier to land in the next query's window than USB's microseconds. Onmainthe pattern is the opposite: the real answer followed by foreign lines written into the window.One caveat, stated plainly. Both
origin/mainWiFi runs (this one and an earlier attempt) ended with the unit dropping off the network during the streaming phase; both branch runs completed all phases. I am not claiming the branch is more stable — this unit's WiFi stack is known to wedge under cumulative connect churn, themainrun was the second connect of each session, and two samples is not evidence. Noting it only so the pattern isn't mistaken for a clean sweep. After the drop the unit still passed a full run over USB, so it was the transport, not the device.Tests
14 new tests in
DaqifiDeviceOperationSerializationTests, covering mutual exclusion, lock release on a throwing body, reentrancy, a nested text query inside an exclusive block,Disconnectfrom inside one, teardown while an operation is in flight, deferral (held back, non-blocking, in-order, owner not deferred), and that the inbound path is never blocked.Full suite green: 2352 Core tests × net9.0/net10.0, 23 MCP tests.
Worth calling out — reviewing my own diff turned up a bug I had introduced: the new queue-drain sat inside the reader-swap's
try/finally, so a cancelled query would "restart" a reader that was never stopped and subscribe the inbound handler twice, dispatching every frame twice for the rest of the session. Fixed, with a regression test that I confirmed fails without the fix.🤖 Generated with Claude Code