Skip to content

feat(device): connected-device registry with duplicate-across-transport detection - #381

Merged
tylerkron merged 7 commits into
mainfrom
claude/device-connection-impl-912a7a
Jul 22, 2026
Merged

tylerkron merged 7 commits into
mainfrom
claude/device-connection-impl-912a7a

Conversation

@tylerkron

@tylerkron tylerkron commented Jul 22, 2026 •

Copy link
Copy Markdown
Contributor

Closes #380.

The problem, plainly

A DAQiFi box can be reached two ways at the same time — plugged in over USB and sitting on the WiFi network. To discovery those look like two completely different devices: one has a COM port, the other has an IP address. Nothing in Core knew they were the same physical unit.

So an app would happily connect to both, and the user would end up with one box showing up twice in their device list, two connections competing for the same hardware, and duplicated data. The desktop app hit this in the field and solved it in its own UI layer. Every other consumer had to solve it again — the in-repo MCP server hand-rolled two ConcurrentDictionarys plus its own stale-handle eviction, and still had no cross-transport detection at all.

This PR moves that whole layer into Core.

What a user experiences now

"I plugged it in, and it's also on WiFi." The second connection is recognized as the same box before anything is opened. The app hands back the connection it already has instead of making a second one. By default the existing connection quietly wins; if the app wants to ask the user ("It's connected over USB — switch to WiFi?"), it can, and the answer can be keep, switch, or cancel.

"Switch this to WiFi." The new connection is opened first, and only then is the old one dropped. If the WiFi address turns out not to answer, the user keeps the USB session they had instead of ending up with nothing. (This is a bug this PR originally shipped with — see Findings below.)

"I'm done with this device." Removing it disconnects and disposes it properly, and the serial port is actually released — verified by reconnecting to the same port immediately afterward.

Talking to the MCP agent. The one place with visible behavior change today. Before, asking the agent to connect to a device already connected under a different id opened a redundant second connection. Now it returns the live one and tells the agent which id to use going forward.

For the desktop app nothing changes yet — it still has its own ConnectionManager. This is the Core layer it migrates onto, which is the point: that logic stops being desktop-only and stops being reimplemented per consumer.

What a developer writes

Before, you tracked devices yourself:

var connected = new ConcurrentDictionary<string, DaqifiDevice>();
// ...plus your own duplicate check, stale eviction, and disposal

Now:

using var registry = new DaqifiDeviceRegistry();
registry.DeviceAdded   += (_, e) => list.Add(e.Registration);
registry.DeviceRemoved += (_, e) => list.Remove(e.Registration);   // e.Reason says why

var result = await registry.ConnectAsync(info);
if (result.Outcome == DeviceRegistrationOutcome.DuplicateRejected)
    log($"Already connected as {result.Key}");

The registry owns the devices — it disconnects and disposes anything it removes or rejects, so you never double-dispose or leak a port.

How it works

Type Role
DaqifiDeviceRegistry The live set: add/remove events, ConnectAsync/Register/Remove/Clear, stale pruning
DeviceIdentity Fingerprint of a physical unit: serial number → MAC → USB LocationKey
DuplicateDeviceCheck / DuplicateDeviceAction The policy callback's input, and its KeepExisting/SwitchToNew/Cancel answer
DeviceRegistration / DeviceRegistrationResult An entry in the set, and how an attempt ended

Two concepts kept deliberately separate. A key is the handle you look a device up by; it defaults to the identity key, but a consumer that already mints its own device ids (the MCP server, a UI list) can supply one. An identity is only ever used for duplicate detection.

Identity matching is decided by the first discriminator both sides report. Serial numbers compare case-insensitively and decisively — different serials are different boxes, whatever a weaker discriminator says. MAC comparison ignores separators. The USB location key is the last resort for identical units reporting no serial. A device reporting none of the three never matches another, so two unidentifiable devices stay two devices — the worst possible failure here would be silently merging two real boxes into one.

The check runs twice: pre-connect from IDeviceInfo (free to reject — nothing is open yet) and again post-connect from DeviceMetadata, because a serial-port device's serial number is often only known once it has answered its first status message. That mirrors desktop's ConnectionManager.cs:95 / :131 pair.

Deltas from the desktop reference implementation

  • The policy is consulted on the post-connect check too (desktop rejects unconditionally there), so SwitchToNew works when the duplicate is only discoverable after connecting.
  • An existing entry under the requested key also counts as a duplicate hit. That is what lets the MCP server's stale-handle/reconnect logic disappear.
  • The policy is always invoked without the registry lock held, so a policy that blocks on a user prompt never blocks other threads — and it is asked once per connection attempt, not twice.
  • SwitchToNew opens the replacement before dropping the original (desktop drops first).
  • The registry owns disposal of everything it removes, prunes, or rejects.

MCP server

_connected, the stale-handle eviction, and the disposal paths collapse onto the registry. _discovered and _gate stay (the latter pending #342). connect_device now returns the id of the connection that won, so an agent reconnecting a unit already live on another transport is handed that connection instead of opening a redundant second one — the tool description says so.

Testing

Unit: ports of desktop's DuplicateDeviceDetectionTests cases, plus identity fallbacks, both check phases, every policy action, SwitchToNew ordering, events, key minting, pruning, disposal ownership, and concurrency. Full suite green on net9.0 and net10.0 (1819 Core + 23 MCP).

Hardware, against a Nyquist 1 (fw 3.7.2) reachable over USB and WiFi at once — full detail in the validation comment:

Scenario Result
USB connect Registered, MAC merged in from post-connect metadata, 32 channels
Same unit on WiFi, default policy DuplicateRejected, same handle, no TCP connection opened
SwitchToNew ReplacedExisting — TCP opened first, then USB dropped; new transport live
Serial port after the switch free, reconnects immediately
USB attempted while WiFi is live DuplicateRejected (reverse direction)
MCP agent end to end connect, reconnect-reuse, cross-transport reuse, disconnect, actionable errors, shutdown

Findings the hardware testing produced

1. SwitchToNew could lose a working connection — fixed in 0a59df3. The pre-connect check dropped the existing registration before opening the replacement, so when a WiFi TCP connect timed out the registry was left empty and the exception escaped: a user clicking "switch to WiFi" lost their healthy USB session. Now the approved replacement is recorded and the drop deferred until the new connection is live. The unit tests were structurally blind to this — every one used a stub connector that always succeeded. Trade-off: switching to a connection needing the same resource the old one holds now fails rather than freeing it first; that case is degenerate (a dead handle is pruned before the check, and forcing a fresh connection to a port you already hold is Remove() then ConnectAsync()), and it beats losing a live connection.

2. Core never notices a physical USB unplug — filed as #382, pre-existing and out of scope here. SerialStreamTransport.IsConnected is the OS handle's IsOpen, which stays true after the cable is pulled; consumer read failures and producer write failures never feed back into transport status. So ConnectionStatus.Lost is unreachable over serial, and this registry's stale pruning cannot fire for an unplug. Behavior is unchanged from the MCP server's previous tracking, which gated on the same flag — but the docs now state the limit plainly instead of implying pruning covers it. It is a hard blocker for #379.

Scope

MVP per the issue: registry + duplicate policy. Automatic reconnect stays with #379, per-device operation serialization with #342, and the registry surfaces whatever type the factory settles on in #333. Aggregate status strings and UI concerns stay in consumers.

🤖 Generated with Claude Code

…rt detection (closes #380)

Core created devices and forgot them: `DaqifiDeviceFactory.Connect*` returned
per-device objects and nothing tracked the live set or correlated device
identity, so every consumer managing more than one device built its own
registry — the in-repo MCP server hand-rolled two `ConcurrentDictionary`s plus
stale-handle eviction, and the desktop app's `ConnectionManager` additionally
implemented the piece Core lacked entirely: recognizing the same physical unit
reached over both USB and WiFi.

Adds `DaqifiDeviceRegistry`, a thread-safe live set that owns the lifetime of
what it holds:

- `DeviceIdentity` fingerprints a physical unit as serial -> MAC -> USB
  `LocationKey`. Matching is decided by the first discriminator both sides
  report (serial compares case-insensitively and decisively, MAC ignores
  separators), so blank-identity devices never falsely match each other.
- The duplicate check runs pre-connect from `IDeviceInfo` and again
  post-connect from `DeviceMetadata`, since a serial-port device's serial
  number is often only known once it has answered its first status message.
- `DuplicatePolicy` exposes desktop's `KeepExisting | SwitchToNew | Cancel`
  semantics; with no callback the existing connection wins. Unlike desktop's
  reference implementation the policy is consulted on the post-connect check
  too, and is always invoked without the registry lock held so a policy that
  blocks on a user prompt cannot block other threads.
- Keys (lookup handles, defaulting to the identity key) are kept distinct from
  identities (duplicate fingerprints), so a consumer with its own device ids
  can supply them.

The MCP server's `_connected` dictionary, stale-handle eviction, and disposal
paths collapse onto the registry; its `_gate` stays pending #342. `connect_device`
now reports the id of the connection that won, so an agent reconnecting a unit
already live on another transport is handed that connection instead of opening
a redundant second one.

Includes ports of desktop's `DuplicateDeviceDetectionTests` cases plus identity
fallback, phase, policy-action, event, pruning, and concurrency coverage, and a
multi-device section in `docs/DEVICE_INTERFACES.md`.

Bench-tested against a Nyquist 1 (fw 3.7.2) on /dev/cu.usbmodem1101: real
discovery -> connect registers under `sn:9090539562006014104` with the MAC
merged in from post-connect metadata; reconnecting the same discovery result is
rejected without reopening the port; a blank-serial `IDeviceInfo` still resolves
its identity post-connect, and the same unit presented as a WiFi device is
rejected before any TCP attempt while a different serial is not; removal and
disposal release the serial port.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron
tylerkron requested a review from a team as a code owner July 22, 2026 13:30
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add connected-device registry with cross-transport duplicate detection

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add DaqifiDeviceRegistry to track live connections, own disposal, and prune stale devices.
• Introduce DeviceIdentity matching and a duplicate-policy callback checked pre/post connect.
• Refactor MCP server to use the registry and document multi-device usage.
Diagram

graph TD
  A["Consumer (MCP/App)"] --> B["DaqifiDeviceRegistry"] --> C["DeviceConnector (factory)"] --> D["DaqifiDevice"]
  E["Discovery (IDeviceInfo)"] --> B
  B --> F["DeviceIdentity"]
  B --> G{{"DuplicatePolicy"}}
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Maintain separate indexes (by key + by identity)
  • ➕ Avoid O(n) scans over _devices.Values during duplicate checks
  • ➕ Makes identity lookups explicit and potentially faster for large device counts
  • ➖ More state to keep consistent (two maps) and higher bug surface area
  • ➖ More complex replace/remove/prune paths and concurrency invariants
2. Per-identity connect serialization (singleflight)
  • ➕ Prevents the "two concurrent ConnectAsync calls may both open" behavior
  • ➕ Reduces redundant transient connections under contention
  • ➖ Adds complexity (async locks keyed by identity) and potential deadlocks/timeouts
  • ➖ Harder to define behavior when identity is incomplete pre-connect

Recommendation: Current approach is a good default for a small-to-moderate live set: one lock, clear ownership rules, policy invoked out-of-lock, and comprehensive tests. Consider adding an identity index only if performance data shows many concurrent devices or frequent duplicate checks; consider singleflight only if redundant double-connects become a practical issue.

Files changed (11) +2366 / -39

Enhancement (6) +1365 / -0
DaqifiDeviceRegistry.csImplement thread-safe connected-device registry with duplicate policy and ownership +814/-0

Implement thread-safe connected-device registry with duplicate policy and ownership

• Adds 'DaqifiDeviceRegistry' to manage a live set of connected devices keyed by caller-provided or identity-derived keys. Performs duplicate checks before connect (from discovery) and after connect (from device metadata), consults a user policy without holding the lock, prunes disconnected devices, raises add/remove events, and owns disconnect/disposal of removed or rejected devices.

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs

DeviceIdentity.csAdd cross-transport physical device fingerprint and matching logic +224/-0

Add cross-transport physical device fingerprint and matching logic

• Introduces 'DeviceIdentity' as a normalized serial/MAC/location-key fingerprint with deterministic matching rules and a derived stable key. Supports construction from discovery or metadata plus merging discovery-only discriminators into post-connect identity.

src/Daqifi.Core/Device/DeviceIdentity.cs

DeviceRegistration.csAdd registry entry type for key/device/identity/discovery metadata +53/-0

Add registry entry type for key/device/identity/discovery metadata

• Defines 'DeviceRegistration' as the registry’s stored entry, exposing the key, connected device, resolved identity, and optional discovery info. Provides a transport convenience property and diagnostic 'ToString()'.

src/Daqifi.Core/Device/DeviceRegistration.cs

DeviceRegistrationResult.csAdd registration outcome/result model for Connect/Register APIs +84/-0

Add registration outcome/result model for Connect/Register APIs

• Defines 'DeviceRegistrationOutcome' and 'DeviceRegistrationResult' to report whether a device was registered, replaced, rejected as duplicate, or canceled. Exposes convenience properties for key/device access and flags indicating duplicate/registered status.

src/Daqifi.Core/Device/DeviceRegistrationResult.cs

DeviceRegistryEventArgs.csAdd registry removal reasons and add/remove event payloads +82/-0

Add registry removal reasons and add/remove event payloads

• Introduces 'DeviceRemovalReason' and event args for 'DeviceAdded'/'DeviceRemoved'. Clarifies that removed devices are disconnected/disposed before notifications and should be treated as dead handles by subscribers.

src/Daqifi.Core/Device/DeviceRegistryEventArgs.cs

DuplicateDeviceCheck.csAdd duplicate policy types and check context (phase, transports, devices) +108/-0

Add duplicate policy types and check context (phase, transports, devices)

• Defines the duplicate-policy action enum, the check phase (before/after connect), and the 'DuplicateDeviceCheck' payload passed to callbacks. Provides access to both existing registration context and the new identity/device info/device when available.

src/Daqifi.Core/Device/DuplicateDeviceCheck.cs

Refactor (1) +39 / -38
DaqifiAgent.csRefactor MCP agent to use 'DaqifiDeviceRegistry' for connection tracking +39/-38

Refactor MCP agent to use 'DaqifiDeviceRegistry' for connection tracking

• Replaces the agent’s '_connected' dictionary and bespoke stale-handle eviction with 'DaqifiDeviceRegistry'. Connect now returns the registry’s canonical key (which may differ on duplicate), disconnect delegates teardown to the registry, list/require paths use registry snapshots/lookups, and shutdown clears the registry after best-effort SD logging stop.

src/Daqifi.Mcp/DaqifiAgent.cs

Tests (2) +884 / -0
DaqifiDeviceRegistryTests.csAdd comprehensive registry tests (duplicates, keys, pruning, events, concurrency) +700/-0

Add comprehensive registry tests (duplicates, keys, pruning, events, concurrency)

• Introduces an extensive test suite for 'DaqifiDeviceRegistry', including pre/post-connect duplicate detection, policy actions (keep/switch/cancel), key semantics, stale pruning, disposal behavior, and event resilience. Includes concurrency tests to validate thread safety and correct duplicate outcomes under contention.

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

DeviceIdentityTests.csAdd 'DeviceIdentity' construction, matching, and merge tests +184/-0

Add 'DeviceIdentity' construction, matching, and merge tests

• Covers normalization (trimming/blank handling), key derivation preference order, discriminator matching semantics (serial/MAC/location), and merge behavior for filling missing discriminators. Validates that empty identities never match and that stronger discriminators dominate weaker ones.

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

Documentation (2) +78 / -1
DEVICE_INTERFACES.mdDocument multi-device registry usage and duplicate handling +77/-0

Document multi-device registry usage and duplicate handling

• Adds a new section introducing 'DaqifiDeviceRegistry', including examples, key-vs-identity explanation, duplicate policy behavior, and liveness/ownership semantics. Updates the feature list to advertise the multi-device registry and cross-transport deduplication.

docs/DEVICE_INTERFACES.md

DaqifiTools.csClarify connect tool behavior when duplicates are detected +1/-1

Clarify connect tool behavior when duplicates are detected

• Updates the 'connect_device' tool description to explain that connecting an already-connected physical device returns the existing connection and callers should use the returned 'device_id' for subsequent calls.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

@qodo-code-review

qodo-code-review Bot commented Jul 22, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Stale duplicate result ✓ Resolved 🐞 Bug ☼ Reliability
Description
DaqifiDeviceRegistry can return DeviceRegistrationOutcome.DuplicateRejected with an existing
registration that has been removed/replaced (and possibly disposed) while DuplicatePolicy runs,
violating the contract that DuplicateRejected returns the live existing connection. This can surface
as callers receiving a dead handle even though the registry is supposed to be thread-safe and policy
is allowed to block/reenter registry operations.
Code

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[R548-571]

+                var action = InvokePolicy(new DuplicateDeviceCheck(
+                    existing!,
+                    identity,
+                    deviceInfo,
+                    device,
+                    connectionType,
+                    DuplicateCheckPhase.AfterConnect));
+
+                switch (action)
+                {
+                    case DuplicateDeviceAction.SwitchToNew:
+                        RemoveRegistration(existing!, DeviceRemovalReason.Replaced);
+                        replacedExisting = true;
+                        continue;
+
+                    case DuplicateDeviceAction.Cancel:
+                        DisposeDevice(device);
+                        return new DeviceRegistrationResult(DeviceRegistrationOutcome.Canceled, null);
+
+                    default:
+                        DisposeDevice(device);
+                        return new DeviceRegistrationResult(
+                            DeviceRegistrationOutcome.DuplicateRejected, existing);
+                }
Relevance

⭐⭐⭐ High

Team frequently accepts concurrency/race fixes to prevent stale handles (e.g., cancellation/TOCTOU
races fixed in PR #295, #248).

PR-#295
PR-#248

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The registry explicitly runs the duplicate policy without holding _lock, but then returns the
previously-captured existing registration on DuplicateRejected without any re-check, so
concurrent removal/replacement during the policy window can make the returned registration stale or
disposed despite docs promising a live existing connection.

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[38-43]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[175-177]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[200-236]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[507-571]

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

## Issue description
`DaqifiDeviceRegistry` captures an `existing` registration under `_lock`, releases the lock to invoke `DuplicatePolicy`, and then returns that captured reference on `DuplicateRejected` without re-checking that it is still present/current. If another thread (or the policy itself) calls `Remove`, `Clear`, `PruneDisconnected`, or reconnects/replaces entries while the policy blocks, the returned `existing` can be no longer registered and its device may already be disposed.

## Issue Context
- The API docs state that on `DuplicateRejected` the result carries “the existing registration — the live connection”.
- The remarks explicitly allow the policy to run without the internal lock and to block.
- Both the pre-connect and post-connect duplicate paths return the previously-captured `existing` reference.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[194-236]
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[507-571]

### Suggested fix approach
1. After `InvokePolicy(...)` returns (for both BeforeConnect and AfterConnect paths), reacquire `_lock` and revalidate:
  - that `existing` is still the current registration for its key (and/or still matches by identity), and
  - that the device is still connected (optional, depending on intended semantics).
2. If the previously captured `existing` is no longer present/current, do **not** return it:
  - BeforeConnect: treat as no-duplicate now and proceed (break out to connect).
  - AfterConnect: loop again and attempt to register/add the new device.
3. When returning `DuplicateRejected`, return the *current* registration instance fetched under the lock (not the stale captured one).
4. Add a concurrency/regression test where `DuplicatePolicy` removes the `Existing` registration during the callback and verify:
  - the call does not return a removed/disposed registration, and
  - behavior is consistent (either proceeds to connect/register or returns the new current existing).

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



Remediation recommended

2. Missing cancellation recheck ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New
Description
DaqifiDeviceRegistry.ConnectAsync only checks cancellationToken at method entry, so if
DuplicatePolicy blocks and the token is canceled later, ConnectAsync can still invoke _connector and
begin/attempt connection work before cancellation is observed. This can delay cancellation and may
start transport activity for connectors that don’t immediately honor the token.
Code

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[R261-272]

+        // Re-check immediately before opening anything. The policy above runs without the lock and
+        // may block indefinitely on a user prompt, so a registry disposed during that window would
+        // otherwise still reach the connector and start fresh transport activity on the way down.
+        // This narrows that window to a few instructions rather than closing it: a Dispose landing
+        // between here and the connector, or during the connect itself, still lets the connection
+        // open, and RegisterCore then throws and disposes it. Guaranteeing no transport activity
+        // outlives the registry needs in-flight tracking on Dispose, which is out of scope here.
+        lock (_lock)
+        {
+            ThrowIfDisposed();
+        }
+
Relevance

⭐⭐⭐ High

Team previously accepted adding second cancellation recheck right before state-changing Send/IO to
narrow race window (PR #329).

PR-#329
PR-#295

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The method checks cancellation only at the start, then may call DuplicatePolicy (which may block
indefinitely) and later proceeds to the connector after only re-checking disposal, not cancellation.

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[188-197]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[223-229]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[261-275]

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

### Issue description
`DaqifiDeviceRegistry.ConnectAsync` checks `cancellationToken` only once at the start of the method. Because `DuplicatePolicy` is explicitly allowed to block (e.g., user prompt), cancellation can be requested during that window, but the method will still proceed to invoke `_connector`.

### Issue Context
The PR added a second disposed-state check immediately before `_connector` to prevent transport activity during shutdown. The same risk exists for cancellation: after a potentially long/blocking policy callback, the method should re-check cancellation as close as possible to invoking `_connector`.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[261-274]

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


3. Connect after disposal race ✓ Resolved 🐞 Bug ☼ Reliability
Description
DaqifiDeviceRegistry.ConnectAsync can still invoke _connector and open a new connection after
another thread has disposed the registry, because it breaks out of the pre-connect loop and proceeds
to connect without a final lock-synchronized ThrowIfDisposed(). This can cause unnecessary/unsafe
transport activity during shutdown even though RegisterCore will later throw ObjectDisposedException
and dispose the newly-opened device.
Code

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[R209-221]

+            lock (_lock)
+            {
+                ThrowIfDisposed();
+                stale = PruneLocked();
+                existing = FindDuplicateLocked(key, discoveryIdentity);
+            }
+
+            CompleteRemoval(stale, DeviceRemovalReason.Disconnected);
+
+            if (existing == null)
+            {
+                break;
+            }
Relevance

⭐⭐⭐ High

Team recently accepted disposal race hardening; PR #364 fixed disposed-check synchronization to
prevent post-dispose operations.

PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
ConnectAsync performs its disposal check under _lock in the pre-connect loop and then breaks when no
duplicate exists; after that break it proceeds to call the connector. Dispose() sets _disposed under
the same lock, so disposal can occur between the loop break and the connector call, allowing a
connection attempt to start after disposal.

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[188-264]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[469-488]
PR-#364

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

### Issue description
`DaqifiDeviceRegistry.ConnectAsync` can begin a connection after the registry has been disposed because the last `ThrowIfDisposed()` check happens inside the loop’s locked section, but once the code hits `if (existing == null) { break; }` it proceeds to call `_connector(...)` without another lock-synchronized disposed check. A concurrent `Dispose()` can set `_disposed = true` under `_lock` in that gap.

### Issue Context
- `Dispose()` sets `_disposed = true` under `_lock`, so a disposed check taken under the same lock immediately before `_connector` is sufficient to ensure we never start a new connection *after* disposal has already begun/completed.
- `RegisterCore` already checks `ThrowIfDisposed()` and will dispose the device on exception, but that still allows transient hardware/network activity.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[204-264]

### Suggested change
1. Right before invoking `_connector(...)`, take `_lock` and call `ThrowIfDisposed()` (or otherwise read `_disposed` under `_lock`).
2. Keep the lock hold extremely short (check only) so you do not hold `_lock` across the awaited connection.

Example shape:
```csharp
lock (_lock)
{
   ThrowIfDisposed();
}
var device = await _connector(...);
```

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


4. Ordering test is tautological ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen records
overWifi.IsConnected when DeviceRemoved fires, but overWifi is created via
ConnectedDevice(...) which calls Connect() immediately, so the assertion will pass regardless of
whether the registry actually waits to drop the old connection until the replacement is opened. This
weakens regression coverage for the critical guarantee that a failed replacement connect must not
cost the existing session.
Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[R321-345]

+        var overUsb = ConnectedDevice("usb", serialNumber);
+        var overWifi = ConnectedDevice("wifi", serialNumber);
+        var policyCalls = new List<DuplicateCheckPhase>();
+        var connectedWhileOldStillRegistered = false;
+        var connects = new List<int>();
+        using var registry = RegistryReturning(connects, overUsb, overWifi);
+
+        await registry.ConnectAsync(UsbInfo(serialNumber));
+
+        registry.DuplicatePolicy = check =>
+        {
+            policyCalls.Add(check.Phase);
+            return DuplicateDeviceAction.SwitchToNew;
+        };
+        registry.DeviceRemoved += (_, _) =>
+        {
+            // At the moment the old registration is dropped, the replacement must already be live.
+            connectedWhileOldStillRegistered = overWifi.IsConnected;
+        };
+
+        var result = await registry.ConnectAsync(WifiInfo(serialNumber));
+
+        Assert.Equal(DeviceRegistrationOutcome.ReplacedExisting, result.Outcome);
+        Assert.True(connectedWhileOldStillRegistered);
+        Assert.False(overUsb.IsConnected);
Relevance

⭐⭐⭐ High

Team frequently accepts strengthening non-meaningful/flaky tests to assert real behavior (e.g.,
PR350, PR366).

PR-#350
PR-#366

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test uses ConnectedDevice(...) to construct overWifi, which calls Connect() during setup;
therefore checking overWifi.IsConnected during DeviceRemoved cannot prove anything about when
the registry opened the replacement connection.

src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[19-33]
src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[318-349]

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 test `ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen` does not actually verify ordering because the replacement device (`overWifi`) is already connected before the second `ConnectAsync` runs.

## Issue Context
`ConnectedDevice(...)` calls `device.Connect()` as part of test setup, so `overWifi.IsConnected` is always true when `DeviceRemoved` fires.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[19-33]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[318-349]

## Suggested fix approach
Update the test to ensure the replacement becomes connected *only when the connector is invoked* (or later), and assert that the old registration is not removed until after that point.

One option:
1. Create `overWifi` as a disconnected `new DaqifiDevice("wifi")`.
2. In the registry connector delegate, set metadata, then call `overWifi.Connect()` immediately before returning it.
3. Use a `TaskCompletionSource`/barrier inside the connector to pause before connecting/returning, and while paused assert:
  - the old device is still registered/connected
  - no `DeviceRemoved` event has fired
4. Release the barrier and then assert the removal happens only after the connector proceeds.

This makes the ordering guarantee observable and prevents false-positive passes.

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


Grey Divider

Previous review results

Review updated until commit 154499e

Results up to commit e1827d4 ⚖️ Balanced


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


Action required
1. Stale duplicate result ✓ Resolved 🐞 Bug ☼ Reliability
Description
DaqifiDeviceRegistry can return DeviceRegistrationOutcome.DuplicateRejected with an existing
registration that has been removed/replaced (and possibly disposed) while DuplicatePolicy runs,
violating the contract that DuplicateRejected returns the live existing connection. This can surface
as callers receiving a dead handle even though the registry is supposed to be thread-safe and policy
is allowed to block/reenter registry operations.
Code

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[R548-571]

+                var action = InvokePolicy(new DuplicateDeviceCheck(
+                    existing!,
+                    identity,
+                    deviceInfo,
+                    device,
+                    connectionType,
+                    DuplicateCheckPhase.AfterConnect));
+
+                switch (action)
+                {
+                    case DuplicateDeviceAction.SwitchToNew:
+                        RemoveRegistration(existing!, DeviceRemovalReason.Replaced);
+                        replacedExisting = true;
+                        continue;
+
+                    case DuplicateDeviceAction.Cancel:
+                        DisposeDevice(device);
+                        return new DeviceRegistrationResult(DeviceRegistrationOutcome.Canceled, null);
+
+                    default:
+                        DisposeDevice(device);
+                        return new DeviceRegistrationResult(
+                            DeviceRegistrationOutcome.DuplicateRejected, existing);
+                }
Relevance

⭐⭐⭐ High

Team frequently accepts concurrency/race fixes to prevent stale handles (e.g., cancellation/TOCTOU
races fixed in PR #295, #248).

PR-#295
PR-#248

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The registry explicitly runs the duplicate policy without holding _lock, but then returns the
previously-captured existing registration on DuplicateRejected without any re-check, so
concurrent removal/replacement during the policy window can make the returned registration stale or
disposed despite docs promising a live existing connection.

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[38-43]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[175-177]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[200-236]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[507-571]

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

## Issue description
`DaqifiDeviceRegistry` captures an `existing` registration under `_lock`, releases the lock to invoke `DuplicatePolicy`, and then returns that captured reference on `DuplicateRejected` without re-checking that it is still present/current. If another thread (or the policy itself) calls `Remove`, `Clear`, `PruneDisconnected`, or reconnects/replaces entries while the policy blocks, the returned `existing` can be no longer registered and its device may already be disposed.

## Issue Context
- The API docs state that on `DuplicateRejected` the result carries “the existing registration — the live connection”.
- The remarks explicitly allow the policy to run without the internal lock and to block.
- Both the pre-connect and post-connect duplicate paths return the previously-captured `existing` reference.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[194-236]
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[507-571]

### Suggested fix approach
1. After `InvokePolicy(...)` returns (for both BeforeConnect and AfterConnect paths), reacquire `_lock` and revalidate:
  - that `existing` is still the current registration for its key (and/or still matches by identity), and
  - that the device is still connected (optional, depending on intended semantics).
2. If the previously captured `existing` is no longer present/current, do **not** return it:
  - BeforeConnect: treat as no-duplicate now and proceed (break out to connect).
  - AfterConnect: loop again and attempt to register/add the new device.
3. When returning `DuplicateRejected`, return the *current* registration instance fetched under the lock (not the stale captured one).
4. Add a concurrency/regression test where `DuplicatePolicy` removes the `Existing` registration during the callback and verify:
  - the call does not return a removed/disposed registration, and
  - behavior is consistent (either proceeds to connect/register or returns the new current existing).

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


Results up to commit 4390b72 ⚖️ Balanced


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


Remediation recommended
1. Ordering test is tautological ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen records
overWifi.IsConnected when DeviceRemoved fires, but overWifi is created via
ConnectedDevice(...) which calls Connect() immediately, so the assertion will pass regardless of
whether the registry actually waits to drop the old connection until the replacement is opened. This
weakens regression coverage for the critical guarantee that a failed replacement connect must not
cost the existing session.
Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[R321-345]

+        var overUsb = ConnectedDevice("usb", serialNumber);
+        var overWifi = ConnectedDevice("wifi", serialNumber);
+        var policyCalls = new List<DuplicateCheckPhase>();
+        var connectedWhileOldStillRegistered = false;
+        var connects = new List<int>();
+        using var registry = RegistryReturning(connects, overUsb, overWifi);
+
+        await registry.ConnectAsync(UsbInfo(serialNumber));
+
+        registry.DuplicatePolicy = check =>
+        {
+            policyCalls.Add(check.Phase);
+            return DuplicateDeviceAction.SwitchToNew;
+        };
+        registry.DeviceRemoved += (_, _) =>
+        {
+            // At the moment the old registration is dropped, the replacement must already be live.
+            connectedWhileOldStillRegistered = overWifi.IsConnected;
+        };
+
+        var result = await registry.ConnectAsync(WifiInfo(serialNumber));
+
+        Assert.Equal(DeviceRegistrationOutcome.ReplacedExisting, result.Outcome);
+        Assert.True(connectedWhileOldStillRegistered);
+        Assert.False(overUsb.IsConnected);
Relevance

⭐⭐⭐ High

Team frequently accepts strengthening non-meaningful/flaky tests to assert real behavior (e.g.,
PR350, PR366).

PR-#350
PR-#366

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test uses ConnectedDevice(...) to construct overWifi, which calls Connect() during setup;
therefore checking overWifi.IsConnected during DeviceRemoved cannot prove anything about when
the registry opened the replacement connection.

src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[19-33]
src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[318-349]

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 test `ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen` does not actually verify ordering because the replacement device (`overWifi`) is already connected before the second `ConnectAsync` runs.

## Issue Context
`ConnectedDevice(...)` calls `device.Connect()` as part of test setup, so `overWifi.IsConnected` is always true when `DeviceRemoved` fires.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[19-33]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[318-349]

## Suggested fix approach
Update the test to ensure the replacement becomes connected *only when the connector is invoked* (or later), and assert that the old registration is not removed until after that point.

One option:
1. Create `overWifi` as a disconnected `new DaqifiDevice("wifi")`.
2. In the registry connector delegate, set metadata, then call `overWifi.Connect()` immediately before returning it.
3. Use a `TaskCompletionSource`/barrier inside the connector to pause before connecting/returning, and while paused assert:
  - the old device is still registered/connected
  - no `DeviceRemoved` event has fired
4. Release the barrier and then assert the removal happens only after the connector proceeds.

This makes the ordering guarantee observable and prevents false-positive passes.

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


Results up to commit 6bcdd1f ⚖️ Balanced


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


Remediation recommended
1. Connect after disposal race ✓ Resolved 🐞 Bug ☼ Reliability
Description
DaqifiDeviceRegistry.ConnectAsync can still invoke _connector and open a new connection after
another thread has disposed the registry, because it breaks out of the pre-connect loop and proceeds
to connect without a final lock-synchronized ThrowIfDisposed(). This can cause unnecessary/unsafe
transport activity during shutdown even though RegisterCore will later throw ObjectDisposedException
and dispose the newly-opened device.
Code

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[R209-221]

+            lock (_lock)
+            {
+                ThrowIfDisposed();
+                stale = PruneLocked();
+                existing = FindDuplicateLocked(key, discoveryIdentity);
+            }
+
+            CompleteRemoval(stale, DeviceRemovalReason.Disconnected);
+
+            if (existing == null)
+            {
+                break;
+            }
Relevance

⭐⭐⭐ High

Team recently accepted disposal race hardening; PR #364 fixed disposed-check synchronization to
prevent post-dispose operations.

PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
ConnectAsync performs its disposal check under _lock in the pre-connect loop and then breaks when no
duplicate exists; after that break it proceeds to call the connector. Dispose() sets _disposed under
the same lock, so disposal can occur between the loop break and the connector call, allowing a
connection attempt to start after disposal.

src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[188-264]
src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[469-488]
PR-#364

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

### Issue description
`DaqifiDeviceRegistry.ConnectAsync` can begin a connection after the registry has been disposed because the last `ThrowIfDisposed()` check happens inside the loop’s locked section, but once the code hits `if (existing == null) { break; }` it proceeds to call `_connector(...)` without another lock-synchronized disposed check. A concurrent `Dispose()` can set `_disposed = true` under `_lock` in that gap.

### Issue Context
- `Dispose()` sets `_disposed = true` under `_lock`, so a disposed check taken under the same lock immediately before `_connector` is sufficient to ensure we never start a new connection *after* disposal has already begun/completed.
- `RegisterCore` already checks `ThrowIfDisposed()` and will dispose the device on exception, but that still allows transient hardware/network activity.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[204-264]

### Suggested change
1. Right before invoking `_connector(...)`, take `_lock` and call `ThrowIfDisposed()` (or otherwise read `_disposed` under `_lock`).
2. Keep the lock hold extremely short (check only) so you do not hold `_lock` across the awaited connection.

Example shape:
```csharp
lock (_lock)
{
   ThrowIfDisposed();
}
var device = await _connector(...);
```

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


Qodo Logo

…actually detect

Bench-testing a physical USB unplug showed the device keeps reporting
IsConnected: SerialStreamTransport.IsConnected is the OS handle's IsOpen, which
stays true after the cable is pulled, and neither the consumer's read failures
nor the producer's write failures feed back into transport status. So stale
pruning cannot fire for an unplugged serial device, and the docs claiming a
unit that was unplugged "never blocks a later reconnect" overstated it.

Behavior is unchanged either way — the MCP server's previous hand-rolled
tracking gated on the same IsConnected flag — but a reader should not have to
discover the limit with a cable in their hand.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tylerkron and others added 2 commits July 22, 2026 08:39
…ment is open

The pre-connect duplicate check dropped the existing registration before
opening the replacement, so a SwitchToNew whose new transport failed to connect
left the caller with neither: the exception propagated out of ConnectAsync and
the registry was empty.

Caught on the bench with the real cross-transport case this feature exists for.
A Nyquist 1 was connected over USB and also answering discovery at
192.168.1.30; asking to switch to WiFi disposed the healthy USB session, then
the TCP connect to the advertised data port timed out — the device broadcasts
on UDP 30303 but is not listening on 9760 (ProbeTcpDataPortAsync finds no port
answering even with no USB session in flight, so that part is environmental,
not a Core bug).

Dropping first was chosen to match desktop's ConnectionManager and because the
existing connection may hold the resource the new one needs. But USB -> WiFi is
exactly the case where it does not, and it is the case the policy exists for.
Now the pre-connect check records the approved replacement and defers: the
registration step drops it once the new connection is live, and skips a second
policy call so a consumer prompting its user still sees one dialog. When the
replacement cannot connect, the original survives untouched.

Same-resource switches (replacing a live connection on the same serial port)
now fail rather than freeing the port first. That case is degenerate — a dead
handle is pruned before the check, and forcing a fresh connection to a port you
already hold is spelled Remove() then ConnectAsync() — and it is a far better
trade than losing a working connection.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…de-off

Verified end to end on a Nyquist 1 reachable over USB and WiFi at once: the
replacement TCP session is established before the USB registration is dropped,
the freed serial port reopens, and the reverse direction (USB attempted while
WiFi is live) is rejected as a duplicate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

Hardware validation: full cross-transport case

Completed against a Nyquist 1 (fw 3.7.2) reachable over USB and WiFi simultaneously — Serial /dev/cu.usbmodem1101 and WiFi 192.168.1.30, both reporting serial 9090539562006014104.

Scenario Result
USB connect Registered under sn:9090539562006014104; MAC merged in from post-connect metadata (serial discovery reports none); 32 channels
Same unit on WiFi, default policy DuplicateRejected, same device handle, live set stays at 1 — no TCP connection opened
SwitchToNew ReplacedExisting: TCP session established first, then the USB registration dropped (DeviceRemoved/Replaced); new transport WiFi, 32 channels
Serial port after the switch free — a fresh ConnectSerialAsync succeeds
USB attempted while WiFi is live DuplicateRejected (reverse direction), connection stays on WiFi
Dispose live set empty, connection closed

Also verified earlier in the same session: a blank-serial IDeviceInfo resolving its identity from metadata post-connect; a different serial correctly not matched; and the MCP agent end to end (connect, reconnect-same-id reuse, cross-transport reuse, disconnect, actionable errors, shutdown releasing the port).

Two findings this testing produced

1. SwitchToNew could lose a working connection — fixed in 0a59df3. The pre-connect check dropped the existing registration before opening the replacement, so when the WiFi TCP connect timed out the registry was left empty and the exception escaped: a user clicking "switch to WiFi" lost their healthy USB session. The pre-connect check now records the approved replacement and defers the drop until the new connection is live, and skips a second policy call so a consumer prompting its user still sees one dialog. Two tests pin the ordering, including one asserting the replacement is already connected at the moment the old registration is removed. Trade-off: switching to a connection needing the same resource the old one holds now fails rather than freeing it first — that case is degenerate (a dead handle is pruned before the check; forcing a fresh connection to a port you hold is Remove() then ConnectAsync()), and it beats losing a live connection.

2. Core never notices a physical USB unplug — filed as #382, pre-existing and out of scope here. SerialStreamTransport.IsConnected is the OS handle's IsOpen, which stays true after the cable is pulled; consumer read failures and producer write failures never feed back into transport status; so ConnectionStatus.Lost is unreachable over serial and this registry's stale pruning cannot fire for an unplug. Behavior is unchanged from the MCP server's previous hand-rolled tracking, which gated on the same flag, but the registry docs now state the limit explicitly instead of implying pruning covers it. It is a hard blocker for #379.

@tylerkron

Copy link
Copy Markdown
Contributor Author

Downstream follow-up: daqifi-desktop#752 tracks moving desktop's ConnectionManager duplicate detection onto DeviceIdentity (and later the registry itself), staged by blocker. It supersedes a now-stale "deliberately not filed" entry in daqifi-desktop#708, which had recorded this as app-level policy back when Core had no registry.

Note desktop consumes Core as a NuGet package, so it needs a release containing this, not just the merge.

@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

Comment on lines +321 to +345
var overUsb = ConnectedDevice("usb", serialNumber);
var overWifi = ConnectedDevice("wifi", serialNumber);
var policyCalls = new List<DuplicateCheckPhase>();
var connectedWhileOldStillRegistered = false;
var connects = new List<int>();
using var registry = RegistryReturning(connects, overUsb, overWifi);

await registry.ConnectAsync(UsbInfo(serialNumber));

registry.DuplicatePolicy = check =>
{
policyCalls.Add(check.Phase);
return DuplicateDeviceAction.SwitchToNew;
};
registry.DeviceRemoved += (_, _) =>
{
// At the moment the old registration is dropped, the replacement must already be live.
connectedWhileOldStillRegistered = overWifi.IsConnected;
};

var result = await registry.ConnectAsync(WifiInfo(serialNumber));

Assert.Equal(DeviceRegistrationOutcome.ReplacedExisting, result.Outcome);
Assert.True(connectedWhileOldStillRegistered);
Assert.False(overUsb.IsConnected);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Ordering test is tautological 🐞 Bug ⚙ Maintainability

ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen records
overWifi.IsConnected when DeviceRemoved fires, but overWifi is created via
ConnectedDevice(...) which calls Connect() immediately, so the assertion will pass regardless of
whether the registry actually waits to drop the old connection until the replacement is opened. This
weakens regression coverage for the critical guarantee that a failed replacement connect must not
cost the existing session.
Agent Prompt
## Issue description
The test `ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen` does not actually verify ordering because the replacement device (`overWifi`) is already connected before the second `ConnectAsync` runs.

## Issue Context
`ConnectedDevice(...)` calls `device.Connect()` as part of test setup, so `overWifi.IsConnected` is always true when `DeviceRemoved` fires.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[19-33]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceRegistryTests.cs[318-349]

## Suggested fix approach
Update the test to ensure the replacement becomes connected *only when the connector is invoked* (or later), and assert that the old registration is not removed until after that point.

One option:
1. Create `overWifi` as a disconnected `new DaqifiDevice("wifi")`.
2. In the registry connector delegate, set metadata, then call `overWifi.Connect()` immediately before returning it.
3. Use a `TaskCompletionSource`/barrier inside the connector to pause before connecting/returning, and while paused assert:
   - the old device is still registered/connected
   - no `DeviceRemoved` event has fired
4. Release the barrier and then assert the removal happens only after the connector proceeds.

This makes the ordering guarantee observable and prevents false-positive passes.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted and fixed in 6bcdd1f. Correct call — the assertion was vacuous.

ConnectedDevice(...) calls Connect() during setup, so overWifi.IsConnected was true before the second ConnectAsync ever ran and the check could not fail. The replacement is now built disconnected and opens inside the connector delegate, so IsConnected at DeviceRemoved time genuinely reflects whether the registry waited.

I took the simpler of your two suggested shapes — connect inside the connector rather than a TaskCompletionSource barrier — because it already makes the ordering observable, and I would rather not add cross-thread synchronization to a test that does not need it.

Rather than trust the reasoning, I verified the test is a real guard by reintroducing the drop-first ordering (RemoveRegistration before the connector runs) and re-running:

ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen [FAIL]
ConnectAsync_SwitchToNew_ReplacesTheExistingConnection               [FAIL]
ConnectAsync_SwitchToNew_WhenTheNewConnectionFails_KeepsTheExistingOne [FAIL]

All three fail with the regression reintroduced, where this one previously passed. Then reverted; full suite green on net9.0 and net10.0 (1821 Core + 23 MCP).

Worth noting this is the second instance of the same blind spot in this PR: my stubs were connected before the code under test ran, so the tests could not observe ordering. The other SwitchToNew test happens to have been a genuine guard already — it fails when the connector throws — but that was luck rather than design, and it is why the guarantee this test covers was worth pinning properly.

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 4390b72

Two findings from Qodo's review, both accepted.

1. The registry captured the existing registration under the lock, released
   the lock to run DuplicatePolicy, then returned that captured reference on
   DuplicateRejected without re-checking it. The policy is documented as free
   to block on a user prompt, so the window is long and a concurrent
   Remove/Clear/prune — or the policy itself — could leave the caller holding a
   disposed handle while the contract promises the live connection.

   Both the pre-connect and post-connect paths now revalidate that the captured
   registration is still the entry filed under its key before handing it back.
   When it has gone, the check is re-resolved against the current set instead:
   pre-connect that means connecting after all, post-connect it means keeping
   the device we just opened rather than disposing it in favour of an entry that
   no longer exists.

2. ConnectAsync_SwitchToNew_DropsTheOldConnectionOnlyAfterTheNewOneIsOpen was
   tautological. It asserted the replacement was connected when DeviceRemoved
   fired, but the test helper connects the device during setup, so it was true
   regardless of ordering. The replacement now opens inside the connector and
   nowhere earlier. Verified by reintroducing the drop-first ordering: all three
   SwitchToNew tests fail, where previously this one passed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

Comment on lines +209 to +221
lock (_lock)
{
ThrowIfDisposed();
stale = PruneLocked();
existing = FindDuplicateLocked(key, discoveryIdentity);
}

CompleteRemoval(stale, DeviceRemovalReason.Disconnected);

if (existing == null)
{
break;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Connect after disposal race 🐞 Bug ☼ Reliability

DaqifiDeviceRegistry.ConnectAsync can still invoke _connector and open a new connection after
another thread has disposed the registry, because it breaks out of the pre-connect loop and proceeds
to connect without a final lock-synchronized ThrowIfDisposed(). This can cause unnecessary/unsafe
transport activity during shutdown even though RegisterCore will later throw ObjectDisposedException
and dispose the newly-opened device.
Agent Prompt
### Issue description
`DaqifiDeviceRegistry.ConnectAsync` can begin a connection after the registry has been disposed because the last `ThrowIfDisposed()` check happens inside the loop’s locked section, but once the code hits `if (existing == null) { break; }` it proceeds to call `_connector(...)` without another lock-synchronized disposed check. A concurrent `Dispose()` can set `_disposed = true` under `_lock` in that gap.

### Issue Context
- `Dispose()` sets `_disposed = true` under `_lock`, so a disposed check taken under the same lock immediately before `_connector` is sufficient to ensure we never start a new connection *after* disposal has already begun/completed.
- `RegisterCore` already checks `ThrowIfDisposed()` and will dispose the device on exception, but that still allows transient hardware/network activity.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[204-264]

### Suggested change
1. Right before invoking `_connector(...)`, take `_lock` and call `ThrowIfDisposed()` (or otherwise read `_disposed` under `_lock`).
2. Keep the lock hold extremely short (check only) so you do not hold `_lock` across the awaited connection.

Example shape:
```csharp
lock (_lock)
{
    ThrowIfDisposed();
}
var device = await _connector(...);
```

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted and fixed in 4eab983 — with one correction to the framing.

The race is real, and worse than the description suggests in one respect: on the SwitchToNew path the unguarded gap spans the entire duplicate-policy call, which is documented as free to block on a user prompt. So this is not a narrow instruction-width window — an app disposing the registry while a duplicate dialog is up would reliably go on to open a connection.

Verified rather than assumed. With the check removed, the regression test observes connectorRuns == 2 on an already-disposed registry: the connection genuinely opens, and RegisterCore throws afterwards. Test added as ConnectAsync_RegistryDisposedWhileThePolicyDecides_NeverReachesTheConnector.

Where I would push back: the suggested fix narrows the window, it does not close it, and I would rather the code not claim otherwise. A Dispose() landing between the new check and _connector, or during the connect itself, still opens a connection that the registration step then throws away. Closing it properly needs in-flight tracking — a drain or a lifetime token cancelled on Dispose — which changes what Dispose means (does it block? cancel? for how long?) and belongs with the session/lifecycle work in #342 rather than being smuggled into this PR.

So the fix is in, and the comment above it plus Dispose's remarks now say plainly what is and is not guaranteed: nothing leaks either way, but transport activity started before disposal does complete. Callers needing connect attempts to stop promptly should cancel the token they passed in — which already works today.

Full suite green on net9.0 and net10.0 (1822 Core + 23 MCP).

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 6bcdd1f

…posed

The pre-connect loop's disposed check ran under the lock, but the path from
there to the connector was unguarded — and on the SwitchToNew path that gap
spans the entire duplicate-policy call, which is documented as free to block on
a user prompt. An app disposing the registry while that prompt was up would
still go on to open a connection, which the registration step then threw away.

Re-checks under the lock immediately before connecting. This narrows the window
rather than closing it: a Dispose landing between that check and the connector,
or during the connect itself, still lets the connection open and be disposed by
the registration step. Guaranteeing no transport activity outlives the registry
needs in-flight tracking on Dispose, which belongs with the session/lifecycle
work in #342; Dispose's docs now state what it does and does not wait for.

Verified the regression test is a real guard by removing the check: the
connector runs a second time (connectorRuns 2 rather than 1) with the registry
already disposed.

Reported by Qodo.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

Comment on lines +261 to +272
// Re-check immediately before opening anything. The policy above runs without the lock and
// may block indefinitely on a user prompt, so a registry disposed during that window would
// otherwise still reach the connector and start fresh transport activity on the way down.
// This narrows that window to a few instructions rather than closing it: a Dispose landing
// between here and the connector, or during the connect itself, still lets the connection
// open, and RegisterCore then throws and disposes it. Guaranteeing no transport activity
// outlives the registry needs in-flight tracking on Dispose, which is out of scope here.
lock (_lock)
{
ThrowIfDisposed();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Missing cancellation recheck 🐞 Bug ☼ Reliability

DaqifiDeviceRegistry.ConnectAsync only checks cancellationToken at method entry, so if
DuplicatePolicy blocks and the token is canceled later, ConnectAsync can still invoke _connector and
begin/attempt connection work before cancellation is observed. This can delay cancellation and may
start transport activity for connectors that don’t immediately honor the token.
Agent Prompt
### Issue description
`DaqifiDeviceRegistry.ConnectAsync` checks `cancellationToken` only once at the start of the method. Because `DuplicatePolicy` is explicitly allowed to block (e.g., user prompt), cancellation can be requested during that window, but the method will still proceed to invoke `_connector`.

### Issue Context
The PR added a second disposed-state check immediately before `_connector` to prevent transport activity during shutdown. The same risk exists for cancellation: after a potentially long/blocking policy callback, the method should re-check cancellation as close as possible to invoking `_connector`.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceRegistry.cs[261-274]

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted and fixed in 154499e, with one clarification on severity.

The gap is real and is the exact analogue of the disposal check — the token was read only at method entry, so a cancellation requested while DuplicatePolicy blocks was not observed before _connector.

But no transport activity actually started on the production path. DaqifiDeviceFactory.ConnectFromDeviceInfoAsync calls cancellationToken.ThrowIfCancellationRequested() at its own entry, so the default connector already threw before opening anything. Your description hedges this correctly ("connectors that don't immediately honor the token"), and it is worth stating plainly so the severity is not overread: this was a latent contract gap, not a live leak.

Fixed anyway, for two reasons that outweigh the one-line cost:

  1. ConnectAsync documents its own cancellation behaviour, so it should not depend on the callee to honour it — the connector is a seam, and a custom one need not check.
  2. It was asymmetric with the disposed guard added immediately above it. A reader would reasonably wonder why one shutdown signal is checked there and the other is not.

Regression test ConnectAsync_CancelledWhileThePolicyDecides_NeverReachesTheConnector cancels inside the policy; the stub connector deliberately ignores the token, so it proves the registry does the checking. Verified as a real guard by removing the line — no OperationCanceledException surfaces and the test fails.

Full suite green on net9.0 and net10.0 (1823 Core + 23 MCP).

@qodo-code-review

Copy link
Copy Markdown

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

…connecting

ConnectAsync checked the token only at method entry, so a cancellation
requested while DuplicatePolicy blocked on a user prompt was not observed until
the connector itself looked at it. The default connector does check
(DaqifiDeviceFactory.ConnectFromDeviceInfoAsync throws at entry), so no
transport activity actually started on the production path — but relying on the
callee for a contract this method states is the wrong way round, and it left
the cancellation guard asymmetric with the disposed guard added alongside it.

Verified the regression test is a real guard by removing the check: the stub
connector ignores the token, so no OperationCanceledException surfaces and the
test fails.

Reported by Qodo.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 154499e

@tylerkron

Copy link
Copy Markdown
Contributor Author

Review rounds complete — final state

Four rounds of /agentic_review. Latest: 🐞 Bugs (0), 📘 Rule violations (0), all four findings ✓ Resolved.

Finding Verdict Fix
Stale duplicate result Accepted — real 6bcdd1f: both duplicate paths revalidate the captured registration after the policy call
Ordering test is tautological Accepted — the assertion could not fail 6bcdd1f: replacement now opens inside the connector, not during setup
Connect after disposal race Accepted, severity raised — the gap spanned the whole policy call 4eab983: disposed re-check immediately before connecting
Missing cancellation recheck Accepted, severity lowered — the production connector already threw 154499e: token re-checked alongside the disposed check

Each fix was verified by reintroducing the defect and confirming the new test fails — not by assuming. The disposal one, for example, shows connectorRuns == 2 on an already-disposed registry without the guard.

Two places I deliberately did not go as far as suggested, both documented in the code rather than left implicit:

  • The disposal and cancellation guards narrow their window rather than closing it. Either signal landing between the check and the connector, or during the connect, still opens a connection that registration then disposes. A hard guarantee needs in-flight tracking on Dispose, which changes what Dispose means and belongs with investigate: per-device operation serialization for concurrent consumers #342.
  • Revalidation covers the policy window, not the instruction-width one. Any returned handle can be removed by another thread immediately after return; that is inherent to handing out a handle.

Re-validated on hardware after the review fixes

ConnectAsync changed materially across those three commits, so the earlier hardware validation was stale. Re-ran the full cross-transport flow against the Nyquist 1 on the final commit: USB connect → WiFi duplicate rejected → SwitchToNew moving the live session to WiFi → serial port freed → reverse direction rejected → dispose. Clean, twice consecutively.

One run in three failed at the switch with InvalidOperationException: Cannot start the consumer: a previous consumer thread has not yet exited. Investigated rather than retried: WiFi-only connects via the factory were 3/3 clean, and USB-then-WiFi via the factory with no registry involved was also 3/3 clean, so it is not this code. Filed as #383 — an intermittent race in Core's consumer swap. Worth noting the registry handled it correctly: the failed switch left the USB session intact, which is exactly the guarantee 0a59df3 added.

@tylerkron
tylerkron merged commit 58cd08a into main Jul 22, 2026
1 check passed
@tylerkron
tylerkron deleted the claude/device-connection-impl-912a7a branch July 22, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: connected-device registry with duplicate-across-transport detection (same physical unit via USB and WiFi)

1 participant