[tylerkron]

What was wrong

When a client stopped a device tool call, the call kept waiting anyway.

• disconnect_device ignored cancellation. DisconnectAsync waited on _gate with the WaitAsync() overload that can't be cancelled, so a client that gave up still sat there until whoever held the registry lock finished.
• configure_*, digital/PWM/DAC writes, and set_sample_rate took no token at all. They called RunExclusiveAsync(_ => …) without one, so a cancel could not stop the wait for the device operation lock.

Discover and version-check cancellation is handled in #765. This PR covers the remaining device tools.

How it was fixed

• Every async tool that talks to a device or waits on _gate now takes a CancellationToken and passes it on. The sync list/status tools are unchanged.
• DisconnectAsync uses _gate.WaitAsync(token).
• Device tools pass the token into RunExclusiveAsync, and each exclusive body checks it once before touching the device.
• A cancel only stops the wait, never a change that has started. Once a tool holds the device, its change finishes. The sync bodies can't be interrupted anyway. configure_analog_channels checks the token before it sends the new channel mask. It deliberately does not pass the token to the capability re-read that follows. Cancelling there would leave a wider channel set running at a rate the firmware won't stream at (the rate check comes after the re-read), and it would drop the text exchange while the device's reply is still arriving. The re-read is bounded (3 s response + 250 ms completion).
• ShutdownAsync still can't be cancelled: it is process teardown, not a tool.

Tests

Full local suite green on net9.0 and net10.0 after merging current main (Daqifi.Mcp.Tests 293, Daqifi.Core.Tests 4356 + 3 skipped).

• Every lock-taking device tool (theory over all 9, called through DaqifiTools so the tool → agent → lock path is covered): the test cancels while the call is parked on the held device lock (it asserts the call hasn't completed before cancelling), then checks the call throws OperationCanceledException rather than McpException, sends no SCPI, skips the capability re-read, and leaves the lock free afterwards.
• DisconnectAsync cancelled while parked on the held registry gate throws within the bound, does not disconnect, and leaves the gate free: a later disconnect still succeeds.
• configure_analog_channels cancelled during the capability re-read still finishes: widening 1 → 4 channels at 12000 Hz reports SampleRateAdjustedFromHz 12000 and a live rate of 5000.
• Mutation-checked: dropping the token from the disconnect gate wait, from one agent lock wait, or from one tool, or passing it to the re-read, each turns the matching test red.

Does not reopen discover / version-check from #765. Out of scope: Idempotent/Title/[Range] contract, download progress, SampleRateAdjustedFromHz, comment diet, CreateDefault discover path.

Merge order

Re-merged onto current main (68f15cb), which now includes #750 (typed structured results) and #765 (discover / version-check cancellation). The merge was textually clean and the change is still needed: on main none of these device tools take a CancellationToken, and DisconnectAsync still uses the uncancellable _gate.WaitAsync(). Daqifi.Mcp.Tests 332/332 on net9.0 and net10.0 after the merge, including #750's structured-content contract tests.

  

[qodo-code-review]

PR Summary by Qodo

Propagate MCP cancellation through device operations

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Propagates client cancellation through device tools, registry waits, and exclusive operation
 locks.
• Cancels analog capability refreshes instead of completing stale text-mode exchanges.
• Adds bounded tests for lock waits, refresh cancellation, and exception transparency.

Diagram

sequenceDiagram
    actor Client as MCP Client
    participant Tools as MCP Tools
    participant Agent as Daqifi Agent
    participant Gate as Registry Gate
    participant Lock as Device Lock
    participant Cap as Capability Reader
    Client->>Tools: Invoke with token
    Tools->>Agent: Forward token
    alt Disconnect device
        Agent->>Gate: WaitAsync(token)
        Gate-->>Agent: Gate acquired
    else Device operation
        Agent->>Lock: RunExclusiveAsync(token)
        Lock-->>Agent: Scoped token
        opt Analog configuration
            Agent->>Cap: ReadAsync(token)
        end
    end
    Client--xTools: Cancel request
    Tools--xAgent: Propagate cancellation
    Agent--xGate: Abort gate wait
    Agent--xLock: Abort lock wait
    Agent--xCap: Abort capability read

Loading

High-Level Assessment

Threading the framework-provided CancellationToken through the existing agent and device APIs is the appropriate approach because those APIs already support cancelable semaphore waits and operations. Polling cancellation or introducing linked token sources would add latency and lifecycle complexity without improving behavior; shutdown correctly remains uncancelable as process teardown.

Files changed (4) +244 / -54

Bug fix (2) +105 / -52

DaqifiAgent.csPropagate cancellation into registry and device operations +75/-32

Propagate cancellation into registry and device operations

• Adds optional cancellation tokens to disconnect, configuration, digital, PWM, DAC, latch, and sample-rate operations. Tokens now control registry-gate acquisition, exclusive device-lock waits, operation bodies, and analog capability-document refreshes; a deterministic registry-gate helper supports testing.

src/Daqifi.Mcp/DaqifiAgent.cs

DaqifiTools.csExpose cancellation tokens on MCP device tools +30/-20

Expose cancellation tokens on MCP device tools

• Adds framework-injectable CancellationToken parameters to the affected asynchronous MCP tools and forwards them into DaqifiAgent. Existing GuardAsync handling continues to let cancellation exceptions propagate unchanged.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

Tests (2) +139 / -2

DeviceDouble.csAdd a cancelable capability-read test hook +14/-2

Add a cancelable capability-read test hook

• Extends the fake streaming device with a pre-read callback that receives the propagated cancellation token. Capability reads now await that hook and explicitly observe cancellation before applying the document.

src/Daqifi.Mcp.Tests/DeviceDouble.cs

DeviceToolCancellationTests.csCover cancellation across device-tool wait boundaries +125/-0

Cover cancellation across device-tool wait boundaries

• Adds bounded tests for cancellation while waiting on the registry gate, device operation lock, and capability refresh. Also verifies tool guards preserve OperationCanceledException instead of wrapping cancellation as an MCP error.

src/Daqifi.Mcp.Tests/DeviceToolCancellationTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. Cancelled configuration leaves invalid rate ✓ Resolved

Description

ConfigureAnalogChannelsAsync changes the enabled channel set before passing the cancellation token
into RefreshCapabilityDocumentAsync, while EnforceSampleRateCap runs only after that cancellable
await. If cancellation interrupts the refresh after channels are widened, the new mask remains
applied with the previous higher rate, which firmware rejects when streaming and consequently
produces no samples.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[353]

+            await RefreshCapabilityDocumentAsync(device, streaming, ct).ConfigureAwait(false);

Relevance

●●● Strong

Accepted history prioritizes channel-cap correctness and cancellation-safe critical sections;
cancellation can leave mutated hardware state before cap enforcement.

PR-#412
PR-#473

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The agent disables and enables channels before the newly cancellable refresh, and channel control
immediately mutates IsEnabled and sends the ADC mask. Cancellation is rethrown before rate
enforcement, even though the capability cap is explicitly scoped to the channel set and the core
documentation states that an over-cap start streams nothing.

src/Daqifi.Mcp/DaqifiAgent.cs[338-353]
src/Daqifi.Mcp/DaqifiAgent.cs[1551-1557]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[484-519]
src/Daqifi.Core/Device/Capabilities/SampleRateCap.cs[7-20]
src/Daqifi.Core/Device/DaqifiDevice.cs[2450-2458]

Agent prompt

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

## Issue description
Cancellation during the capability refresh exits analog configuration after the channel mask has changed but before the live sample rate is validated, leaving a partially applied configuration that may not stream.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[326-358]
- src/Daqifi.Mcp.Tests/DeviceToolCancellationTests.cs[79-101]

## Recommended Fix
Capture the previous analog channel selection before applying the requested one. If the capability refresh is canceled, restore the previous selection and associated capability state before rethrowing the cancellation; extend the cancellation test to verify that both enabled channels and sample rate remain unchanged.

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

---

Context sources

✅ Compliance rules (platform): 13 rules

✅ Cross-repo context — repo relationships

Review mode: 🧠 Deep: This push contains substantial, independent logic changes across device lifecycle, SCPI validation/communication, MCP tools, and cancellation paths, creating a defect density where redundant review is materially valuable.

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 ↗

Previous reviews

Review updated until commit 1c98290

Results up to commit 96de511 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Cancelled configuration leaves invalid rate ✓ Resolved

Description

ConfigureAnalogChannelsAsync changes the enabled channel set before passing the cancellation token
into RefreshCapabilityDocumentAsync, while EnforceSampleRateCap runs only after that cancellable
await. If cancellation interrupts the refresh after channels are widened, the new mask remains
applied with the previous higher rate, which firmware rejects when streaming and consequently
produces no samples.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[353]

+            await RefreshCapabilityDocumentAsync(device, streaming, ct).ConfigureAwait(false);

Relevance

●●● Strong

Accepted history prioritizes channel-cap correctness and cancellation-safe critical sections;
cancellation can leave mutated hardware state before cap enforcement.

PR-#412
PR-#473

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The agent disables and enables channels before the newly cancellable refresh, and channel control
immediately mutates IsEnabled and sends the ADC mask. Cancellation is rethrown before rate
enforcement, even though the capability cap is explicitly scoped to the channel set and the core
documentation states that an over-cap start streams nothing.

src/Daqifi.Mcp/DaqifiAgent.cs[338-353]
src/Daqifi.Mcp/DaqifiAgent.cs[1551-1557]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[484-519]
src/Daqifi.Core/Device/Capabilities/SampleRateCap.cs[7-20]
src/Daqifi.Core/Device/DaqifiDevice.cs[2450-2458]

Agent prompt

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

## Issue description
Cancellation during the capability refresh exits analog configuration after the channel mask has changed but before the live sample rate is validated, leaving a partially applied configuration that may not stream.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[326-358]
- src/Daqifi.Mcp.Tests/DeviceToolCancellationTests.cs[79-101]

## Recommended Fix
Capture the previous analog channel selection before applying the requested one. If the capability refresh is canceled, restore the previous selection and associated capability state before rethrowing the cancellation; extend the cancellation test to verify that both enabled channels and sample rate remain unchanged.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 516de98

[tylerkron]

Qodo-clean, CI green — ready for review

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Qodo-clean, CI green — ready for review