perf(discovery): Windows discovery no longer slows down with every COM port on the machine

[tylerkron]

What was wrong

On Windows, discovering a DAQiFi over USB got slower with every unrelated COM port on the machine. Before opening a single port, Core asked Windows "which device is COM3? which is COM4? …" one port at a time, and each of those questions is a WMI query costing roughly 100-500 ms. A normal laptop has eight to twelve COM ports (Bluetooth pairs, vendor virtual ports), so the filtering step alone burned one to five seconds of blocking work — and continuous discovery repeated the whole thing every second, so it never caught up. macOS and Linux were never affected; they already ask once and reuse the answer.

How it was fixed

Ask once. A single query now lists every serial port on the machine with the device it belongs to, and both Windows providers read that one map instead of running their own per-port query. The map is reused for two seconds, the same window the macOS provider has always used, so one discovery pass costs one query no matter how many COM ports are present.

The thing worth pushing back on is that a two-second-old map can be wrong about a port that appeared since it was built. The two callers need different things there, so they get different rules. For the port filter, an unknown port simply gets probed the old-fashioned way, so a stale miss costs one probe and can never hide a device — it takes the cached answer. For location keys, a miss is a real answer (no key at all) and it is asked precisely about a port that just showed up, so it forces a fresh map before answering no. The residual risk both share is a stale hit: a device plugged into a COM number that another device just vacated keeps the old identity until the map expires, so it is found on the next pass rather than this one. That is the same trade the macOS provider makes today.

Verification

• 27 new tests. The WMI call is behind an injected query, so the caption parsing, the caching, and both staleness rules are exercised on any platform. Seven mutations confirm the tests catch the regressions: removing the cache fails 6, ignoring refreshOnMiss fails 2, refreshing on every miss fails 1, publishing the map before the query returns fails 1, checking only the first device id fails 1, case-sensitive port keys fail 1, and overwriting instead of appending fails 1. Full suite green on net9.0 (3070 Core + 86 Mcp) and net10.0 (3070), 0 warnings.
• Honest limitation: the WMI path itself is not exercised anywhere. This repo has no Windows machine and CI runs ubuntu-latest only — the same caveat docs/DEVICE_INTERFACES.md already records for WindowsUsbLocationProvider. What is verified by inspection is that the new query is the old one minus its per-port AND Caption LIKE '%(COMn)%' clause, and that matching (COMn) including the parentheses reproduces that clause exactly (notably, (COM9) still does not match (COM90)).
• Bench, non-destructive, Nq1 fw 3.7.2 on /dev/cu.usbmodem1101 — a no-regression check only, since macOS never constructs these providers: serial discovery found sn=9090539562006014104 fw 3.7.2, 3 s @ 500 Hz on channels 0-2 gave 1186 samples (this unit's known ~79 % clock ratio), SD storage query 7.80 GB, clean disconnect. No reboot, format, delete, SD:GET, firmware or LAN writes.

closes #487

Not merging — this is for your review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

perf(discovery): Batch WMI port lookup via cached WindowsPnpPortMap

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Replace per-COM-port WMI queries with one cached port→PnP-device map per discovery pass.
• Share the same map across Windows descriptor and location providers with tailored staleness rules.
• Add unit tests for caption parsing, VID/PID parsing, caching, concurrency, and refresh-on-miss
 behavior.

Diagram

graph TD
  A["WindowsUsbPortDescriptorProvider"] --> C["WindowsPnpPortMap"] --> D{{"WMI: Win32_PnPEntity"}}
  B["WindowsUsbLocationProvider"] --> C
  A --> E["PnpDeviceIdParser"]
  F["WindowsPnpPortMapTests"] --> C
  G["UsbPortDescriptorTests"] --> E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use Windows SetupAPI instead of WMI

• ➕ Typically faster and more reliable than WMI for device enumeration
• ➕ Avoids WMI availability/latency issues
• ➖ More complex P/Invoke surface and error handling
• ➖ Harder to make cross-platform testable without Windows CI/hardware

2. Parallelize per-port WMI queries

• ➕ Smaller refactor; keeps existing query semantics
• ➖ Still scales with COM port count and can overwhelm WMI
• ➖ More thread-pool contention; worse tail latency under load

3. Longer-lived cache with explicit invalidation

• ➕ Even fewer queries during continuous discovery
• ➖ Hard to invalidate correctly on device churn without OS notifications
• ➖ Increases risk window for stale hits/misses

Recommendation: The chosen approach (single batched query + short-lived shared cache with caller-specific refresh-on-miss semantics) is the best balance of performance, correctness, and testability. SetupAPI could outperform WMI, but it would significantly increase platform-specific complexity and is harder to validate without Windows CI. The current design keeps staleness bounded (2s) and avoids reintroducing per-port WMI costs while preserving safety (misses fall back to probing or force refresh only when semantically required).

Files changed (6) +760 / -83

Enhancement (3) +321 / -64

PnpDeviceIdParser.csExtract VID/PID parsing into a pure, unit-testable helper +62/-0

Extract VID/PID parsing into a pure, unit-testable helper

• Adds a dedicated parser to extract USB VID/PID from Windows PnP device instance IDs using a case-insensitive compiled regex. Provides a helper to select the first descriptor-bearing ID from a list, supporting scenarios where multiple entities claim a port.

src/Daqifi.Core/Device/Discovery/PnpDeviceIdParser.cs

WindowsPnpPortMap.csIntroduce shared cached COM-port→device-id map built from one WMI query +252/-0

Introduce shared cached COM-port→device-id map built from one WMI query

• Adds WindowsPnpPortMap which executes a single Win32_PnPEntity (PNPClass='Ports') query to enumerate all port entities, parses captions to map COM names to device instance IDs (supporting multiple entities per port), and caches results for 2s. Implements thread-safe rebuild under a lock, optional refresh-on-miss behavior, and safe failure semantics (retain last good map if rebuild fails).

src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs

WindowsUsbPortDescriptorProvider.csResolve port descriptors from shared map; remove per-port WMI query and regexes +7/-64

Resolve port descriptors from shared map; remove per-port WMI query and regexes

• Removes per-port WMI querying, COM-name validation, and embedded VID/PID regex parsing. Now reads device IDs from WindowsPnpPortMap.Shared and uses PnpDeviceIdParser to classify ports, intentionally not refreshing on misses to avoid reintroducing per-port WMI costs (unknown ports are probed instead).

src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs

Bug fix (1) +10 / -19

WindowsUsbLocationProvider.csUse WindowsPnpPortMap with refresh-on-miss for location lookups +10/-19

Use WindowsPnpPortMap with refresh-on-miss for location lookups

• Replaces the per-port Caption LIKE WMI query with a lookup into the shared WindowsPnpPortMap. Forces a map refresh on miss because an unresolved device ID would otherwise incorrectly produce no location key for newly appeared ports.

src/Daqifi.Core/Device/Discovery/WindowsUsbLocationProvider.cs

Tests (2) +429 / -0

UsbPortDescriptorTests.csAdd unit tests for PnpDeviceIdParser VID/PID parsing and selection +53/-0

Add unit tests for PnpDeviceIdParser VID/PID parsing and selection

• Introduces tests verifying VID/PID extraction from USB PnP device IDs, case-insensitivity, null/empty handling, and skipping non-USB IDs. Adds coverage for selecting the first USB-capable ID when multiple entities claim a port.

src/Daqifi.Core.Tests/Device/Discovery/UsbPortDescriptorTests.cs

WindowsPnpPortMapTests.csAdd comprehensive tests for WindowsPnpPortMap parsing, caching, and refresh rules +376/-0

Add comprehensive tests for WindowsPnpPortMap parsing, caching, and refresh rules

• Adds a new test suite covering caption token parsing (including duplicates, multiple ports, and COM9 vs COM90), case-insensitive lookups, and behavior when captions/IDs are missing. Verifies caching semantics (single query per pass/window), concurrent access behavior, refresh-on-miss vs no-refresh staleness rules, and failure handling (don’t publish empty map on rebuild failure).

src/Daqifi.Core.Tests/Device/Discovery/WindowsPnpPortMapTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. Leaky mutable cache lists ✓ Resolved 🐞 Bug ☼ Reliability

Description

WindowsPnpPortMap.GetDeviceIds returns the cached List<string> instance directly, so internal
callers can still mutate it (via cast/downcast) and corrupt the shared cache. Because the map is
designed for concurrent discovery, such mutation can also race with other threads
reading/enumerating the same list and cause incorrect results or runtime exceptions.

Code

src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[R134-137]

+                if (_map.TryGetValue(portName, out var cached))
+                {
+                    return cached;
+                }

Relevance

●●● Strong

Team has accepted defensive-copy/read-only fixes to prevent callers mutating shared cached
collections.

PR-#318
PR-#389

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cache stores List<string> values and returns those same instances to callers. The test suite
explicitly covers concurrent lookups, reinforcing that returned collections should be safe to share
across threads without risk of mutation.

src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[75-80]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[118-146]
src/Daqifi.Core.Tests/Device/Discovery/WindowsPnpPortMapTests.cs[238-270]

Agent prompt

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

## Issue description
`WindowsPnpPortMap.GetDeviceIds(...)` returns the internal `List<string>` stored in `_map` directly (as `IReadOnlyList<string>`). Even though the static type is read-only, the underlying object is still mutable and can be mutated by internal callers via casts or other means. Because this map is explicitly used from multiple threads, exposing mutable shared state can lead to cache corruption and potential read/iteration races.

## Issue Context
The map is used as a shared cache for Windows discovery providers and is intended to support concurrent lookups.

## Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[75-146]
- src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[175-210]

## Suggested fix
- Make the map values immutable at publication time and return only immutable views:
 - Change `_map` to `Dictionary<string, string[]>` (or `Dictionary<string, IReadOnlyList<string>>` where the values are `ReadOnlyCollection<string>`/`ImmutableArray<string>` stored once).
 - In `BuildMap`, keep building with `List<string>` but convert to `ToArray()` (or `AsReadOnly()` stored) before publishing.
 - Update `GetDeviceIds` to return the stored immutable value.
- Ensure the empty sentinel (`NoDeviceIds`) is also immutable (it already is effectively immutable).

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

---

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5937454

[tylerkron]

Qodo-clean, CI green — ready for review. (2 rounds on head 5937454; round 1's finding — the map published its lists mutable — is fixed and its thread resolved, round 2 came back Bugs (0) and re-checked clean 10 min later. Bench health check re-run after that fix: discovery, 3 s @ 500 Hz → 1186 samples, clean disconnect.)

One note on CI: the first two runs of this head failed on RawCapture_UnderASendHammer_ReadsThePayloadWithNothingInterleaved, which has nothing to do with this change — it fails whenever the runner is slow enough for the hammer to park more than 1024 messages, because the deferred-send backlog is capped at 1024 with a drop-oldest policy and the test waits for HAMMER-0, the very message that gets dropped first. Filed as #516. Third run green with no change to the branch.