fix: bind UDP discovery per-NIC and skip virtual adapters (#179)

[cptkoolbeenz]

Closes #179.

Summary

• WiFiDeviceFinder now binds one UdpClient per NIC at (LocalAddress, 0) for both send and receive. The OS routes each broadcast out the intended adapter; replies arrive on the same socket; the socket's bound IP becomes the authoritative LocalInterfaceAddress (no more subnet-match guessing across virtual NICs that share a subnet).
• GetAllNetworkInterfaces now skips virtual/tunnel adapters by Name/Description (vEthernet, Hyper-V, WSL, VirtualBox, VMware, TAP).
• Removed the now-dead FindLocalInterfaceForRemoteAddress helper and unused SubnetMask field on NetworkInterfaceInfo.
• Added IsVirtualOrTunnelInterface (internal static, InternalsVisibleTo already in place) plus xUnit Theory cases covering common virtual-adapter shapes and physical-adapter negatives.

Why

On Windows hosts with WSL2 mirrored networking (or Hyper-V/VPN/TAP) the virtual NIC frequently shares a /24 with the real WiFi/Ethernet adapter. Sending broadcasts from a single UdpClient bound to IPAddress.Any lets Windows pick the wrong egress NIC, and the subnet-match reply heuristic could attribute a reply to the virtual NIC. Discovery would silently return zero devices even though the device was reachable via TCP from the same host.

Behavior note

DeviceDiscovered may now be invoked concurrently from multiple per-NIC receive loops (was strictly sequential before). The existing public contract did not guarantee single-threaded invocation; subscribers should already be thread-safe.

Test plan

• dotnet build Daqifi.Core.sln — clean (net8.0 + net9.0), 0 warnings
• dotnet test Daqifi.Core.sln — 830 passed / 0 failed / 2 skipped (both target frameworks)
• New tests IsVirtualOrTunnelInterface_* cover vEthernet (WSL), vEthernet (Default Switch), Ethernet 3 w/ Hyper-V description, Ethernet 4 w/ WSL description, VirtualBox, VMware, TAP-Windows — plus Wi-Fi/Ethernet/WLAN negatives and null inputs
• Manually verified on a Windows host (Hyper-V vEthernet (Default Switch) 172.25.176.1 + Intel 10G Ethernet 3 192.168.1.133): DAQiFi-95A7 at 192.168.1.160 was discovered and LocalInterfaceAddress correctly populated as 192.168.1.133 (the real NIC, not the Hyper-V adapter)
• Strict WSL2-mirrored-on-same-subnet repro is best-validated by issue reporter

🤖 Generated with Claude Code

[qodo-code-review]

Review Summary by Qodo

Bind UDP discovery per-NIC and skip virtual adapters

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Bind one UdpClient per NIC to prevent Windows routing broadcasts to wrong adapter
• Filter virtual/tunnel adapters (WSL2, Hyper-V, VirtualBox, VMware, TAP) by name/description
• Replace subnet-match heuristic with socket's bound IP as authoritative local interface
• Add concurrent per-NIC receive loops with thread-safe device deduplication
• Remove unused FindLocalInterfaceForRemoteAddress and SubnetMask field

Diagram

flowchart LR
  A["Single UdpClient<br/>IPAddress.Any"] -->|Old: Wrong NIC<br/>picked by Windows| B["Broadcast<br/>sent"]
  C["Per-NIC UdpClient<br/>bound to LocalAddress"] -->|New: OS routes<br/>to correct NIC| D["Broadcast<br/>sent"]
  E["Virtual Adapter<br/>Filter"] -->|Skip vEthernet,<br/>Hyper-V, etc| F["GetAllNetworkInterfaces"]
  G["Socket's bound IP"] -->|Authoritative<br/>LocalInterfaceAddress| H["ParseDeviceInfo"]

Loading

File Changes

1. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs 🐞 Bug fix +98/-91

Per-NIC UDP binding and virtual adapter filtering

• Replaced single UdpTransport with per-NIC UdpClient instances bound to individual local addresses
• Removed SubnetMask field from NetworkInterfaceInfo struct
• Deleted FindLocalInterfaceForRemoteAddress method and replaced with IsVirtualOrTunnelInterface
 predicate
• Added ReceiveLoopAsync method for concurrent per-NIC receive handling with thread-safe
 deduplication
• Integrated virtual adapter filtering in GetAllNetworkInterfaces using name/description patterns

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

---

2. src/Daqifi.Core.Tests/Device/Discovery/WiFiDeviceFinderTests.cs 🧪 Tests +33/-0

Unit tests for virtual adapter detection

• Added Theory test for IsVirtualOrTunnelInterface covering 7 virtual adapter types (vEthernet,
 Hyper-V, VirtualBox, VMware, TAP)
• Added Theory test for IsVirtualOrTunnelInterface with 4 physical adapter negatives (Wi-Fi,
 Ethernet, WLAN)
• Added Fact test for null input handling in IsVirtualOrTunnelInterface

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

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. No mixed-NIC selection test ☑ 📎 Requirement gap ⚙ Maintainability

Description

The added tests only validate IsVirtualOrTunnelInterface() string matching and do not test a mixed
NIC list (physical + virtual) to ensure enumeration/selection excludes virtual adapters and uses
remaining eligible NICs. This leaves the multi-NIC + virtual-adapter scenario unverified by
automated tests as required.

Code

src/Daqifi.Core.Tests/Device/Discovery/WiFiDeviceFinderTests.cs[R103-134]

+    // Virtual / tunnel adapter filter — issue #179.
+    // WSL2 mirrored networking, Hyper-V vEthernet, VPN/TAP adapters frequently share a /24
+    // subnet with the real WiFi/Ethernet NIC and cause Windows routing to pick the wrong egress
+    // for broadcasts, silently breaking discovery.
+    [Theory]
+    [InlineData("vEthernet (WSL)", "Hyper-V Virtual Ethernet Adapter")]
+    [InlineData("vEthernet (Default Switch)", "Hyper-V Virtual Ethernet Adapter")]
+    [InlineData("Ethernet 3", "Hyper-V Virtual Ethernet Adapter #2")]
+    [InlineData("Ethernet 4", "WSL Virtual Ethernet Adapter")]
+    [InlineData("VirtualBox Host-Only Network", "VirtualBox Host-Only Ethernet Adapter")]
+    [InlineData("VMware Network Adapter VMnet1", "VMware Virtual Ethernet Adapter for VMnet1")]
+    [InlineData("OpenVPN TAP-Windows6", "TAP-Windows Adapter V9")]
+    public void IsVirtualOrTunnelInterface_VirtualAdapters_ReturnsTrue(string name, string description)
+    {
+        Assert.True(WiFiDeviceFinder.IsVirtualOrTunnelInterface(name, description));
+    }
+
+    [Theory]
+    [InlineData("Wi-Fi", "Intel(R) Wi-Fi 6 AX201 160MHz")]
+    [InlineData("Ethernet", "Realtek PCIe GbE Family Controller")]
+    [InlineData("Ethernet 2", "Intel(R) Ethernet Connection I219-LM")]
+    [InlineData("WLAN", "Qualcomm Atheros QCA9377 Wireless Network Adapter")]
+    public void IsVirtualOrTunnelInterface_PhysicalAdapters_ReturnsFalse(string name, string description)
+    {
+        Assert.False(WiFiDeviceFinder.IsVirtualOrTunnelInterface(name, description));
+    }
+
+    [Fact]
+    public void IsVirtualOrTunnelInterface_NullInputs_ReturnsFalse()
+    {
+        Assert.False(WiFiDeviceFinder.IsVirtualOrTunnelInterface(null, null));
+    }

Evidence

PR Compliance ID 7 requires an automated test that supplies a mixed NIC set and asserts virtual
adapters are excluded and eligible NICs are used. The new tests added in this PR only cover
IsVirtualOrTunnelInterface() outcomes for name/description pairs and do not exercise NIC
enumeration/selection behavior with a mixed list.

Automated test covers virtual-adapter filtering and multi-NIC selection
src/Daqifi.Core.Tests/Device/Discovery/WiFiDeviceFinderTests.cs[103-134]

Agent prompt

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

## Issue description
Compliance requires an automated test that validates the multi-NIC + virtual-adapter scenario end-to-end for NIC selection (virtual adapters excluded, eligible physical adapters used). Current tests only validate the string predicate `IsVirtualOrTunnelInterface()`.

## Issue Context
`GetAllNetworkInterfaces()` uses `NetworkInterface.GetAllNetworkInterfaces()` directly, which is hard to control in unit tests. To test selection logic with a mixed NIC list, introduce a seam (e.g., injectable/provider-based enumeration) or add an internal helper that accepts an enumerable of NIC metadata and returns filtered `NetworkInterfaceInfo` items.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/WiFiDeviceFinderTests.cs[103-134]
- src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[330-410]

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

---

2. Unhandled event stalls discovery ☑ 🐞 Bug ☼ Reliability

Description

ReceiveLoopAsync invokes DeviceDiscovered subscribers without exception handling; if a subscriber
throws, that receive task faults. Because DiscoverAsync awaits Task.WhenAll across all per-NIC
receive loops and the default overload uses an infinite timeout, discovery may never complete unless
the caller cancels.

Code

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[R177-182]

+                var receiveTasks = perNicClients
+                    .Select(c => ReceiveLoopAsync(c.Client, c.LocalAddress, discoveredDevices, cts.Token))
+                    .ToArray();
+
+                await Task.WhenAll(receiveTasks);
            }

Evidence

DiscoverAsync(CancellationToken) uses an infinite timeout by default, so receive loops can run
forever unless the caller cancels. The method then awaits Task.WhenAll over the per-NIC receive
tasks; if any task faults (e.g., from an exception thrown by DeviceDiscovered?.Invoke), Task.WhenAll
cannot complete until *all* tasks complete, which may never happen with an infinite timeout.

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[102-105]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[177-182]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[204-248]

Agent prompt

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

### Issue description
`ReceiveLoopAsync` calls `OnDeviceDiscovered(deviceInfo)` without guarding against exceptions thrown by event subscribers. With per-NIC parallel receive loops awaited via `Task.WhenAll`, a faulted receive task can prevent `DiscoverAsync` from completing when using the default infinite-timeout overload.

### Issue Context
This is specific to the new per-NIC parallel receive-loop implementation.

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[204-248]
- src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[177-182]

### Suggested fix
- Ensure `ReceiveLoopAsync` never faults due to subscriber exceptions:
 - Wrap `OnDeviceDiscovered(deviceInfo)` in `try/catch` and continue the loop (optionally capture/log the exception if a logger exists).
 - Alternatively, wrap the entire per-packet processing section in a broad `try/catch` to prevent any unexpected exception from faulting the task.
- (Optional) If you prefer to fail fast, pass a shared `CancellationTokenSource` into `ReceiveLoopAsync` and cancel it when a receive loop hits an unrecoverable exception, so all loops terminate and `Task.WhenAll` can complete.

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

---

[cptkoolbeenz]

/improve

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to 9aeb434

Category	Suggestion	Impact
Possible issue	✅ Add bind fallback to ephemeral port Suggestion Impact: The commit wrapped the UDP client bind to the well-known discovery port in a try/catch for SocketException and, on failure, bound to an ephemeral port (0) before sending the discovery query. code diff: @@ -166,7 +166,17 @@ ExclusiveAddressUse = false }; udp.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); - udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); + try + { + udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); + } + catch (SocketException) + { + // Fall back to an ephemeral port if the well-known discovery port + // can't be acquired on this platform/NIC (e.g. another process holds + // it). Devices that reply to source-port still reach us either way. + udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, 0)); + } await udp.SendAsync(_queryCommandBytes, _queryCommandBytes.Length, interfaceInfo.BroadcastEndpoint); Add a fallback mechanism to bind to an ephemeral port if binding to the fixed discovery port fails. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [38-47] udp = new UdpClient { EnableBroadcast = true, ExclusiveAddressUse = false }; udp.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); -udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); + +try +{ + // Preferred: bind to the well-known discovery port. + udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); +} +catch (SocketException) +{ + // Fallback: bind to an ephemeral port so discovery can still work on platforms + // that don't allow multiple binds to the same UDP port. + udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, 0)); +} + await udp.SendAsync(_queryCommandBytes, _queryCommandBytes.Length, interfaceInfo.BroadcastEndpoint); perNicClients.Add((udp, interfaceInfo.LocalAddress)); added = true; [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: Provides a solid error handling fallback for socket binding issues that could prevent discovery on certain platforms or scenarios.	Medium
	✅ Serialize discovery event callbacks Suggestion Impact: The commit moved OnDeviceDiscovered(deviceInfo) into the lock block (and removed the call outside it), ensuring DeviceDiscovered events fire sequentially across parallel per-NIC receive loops. code diff: @@ -271,9 +281,12 @@ continue; } discoveredDevices.Add(deviceInfo); - } - - OnDeviceDiscovered(deviceInfo); + + // Invoke under the lock so DeviceDiscovered fires sequentially across the + // parallel per-NIC receive loops, matching the original (single-socket) + // sequential-callback contract that subscribers may depend on. + OnDeviceDiscovered(deviceInfo); + } } Move the OnDeviceDiscovered invocation inside the lock block to serialize event firing across the parallel receive loops. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [137-146] lock (discoveredDevices) { if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) { continue; } + discoveredDevices.Add(deviceInfo); + + // Serialize callbacks to avoid concurrent subscriber execution across NIC receive loops. + OnDeviceDiscovered(deviceInfo); } -OnDeviceDiscovered(deviceInfo); - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: Addresses a valid concurrency issue introduced by the PR's parallel execution, protecting non-thread-safe subscribers from concurrent invocations.	Medium
Update

---

Previous suggestions

✅ Suggestions up to commit 0dac743

Category	Suggestion	Impact
Possible issue	Detect virtual adapters more reliably Search for virtualization keywords in both name and description. This prevents missing virtual interfaces when their description is generic or empty. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [217-234] internal static bool IsVirtualOrTunnelInterface(string? name, string? description) { var n = (name ?? string.Empty).Trim(); var d = (description ?? string.Empty).Trim(); + static bool Contains(string haystack, string needle) => + haystack.IndexOf(needle, StringComparison.OrdinalIgnoreCase) >= 0; + // Use specific TAP prefixes ("TAP-Windows", "TAP-", "TAP " with trailing space) rather // than a generic "TAP" substring to avoid false positives on legitimate physical NIC // descriptions that happen to contain those three letters. return n.StartsWith("vEthernet", StringComparison.OrdinalIgnoreCase) || - d.IndexOf("vEthernet", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("Hyper-V", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("WSL", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("VirtualBox", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("VMware", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("TAP-Windows", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("TAP-", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("TAP ", StringComparison.OrdinalIgnoreCase) >= 0; + Contains(n, "vEthernet") || Contains(d, "vEthernet") || + Contains(n, "Hyper-V") || Contains(d, "Hyper-V") || + Contains(n, "WSL") || Contains(d, "WSL") || + Contains(n, "VirtualBox") || Contains(d, "VirtualBox") || + Contains(n, "VMware") || Contains(d, "VMware") || + Contains(n, "TAP-Windows") || Contains(d, "TAP-Windows") || + Contains(n, "TAP-") || Contains(d, "TAP-") || + Contains(n, "TAP ") || Contains(d, "TAP "); } Suggestion importance[1-10]: 7 __ Why: Checking both name and description ensures virtual network interfaces are reliably filtered out, addressing edge cases where virtualization details might only appear in the interface's name.	Medium
	✅ Prevent receive loop task faults Suggestion Impact: Added a generic catch block around ReceiveAsync so unexpected exception types cannot fault the receive task and abort/hang discovery. code diff: @@ -239,6 +249,12 @@ { break; } + catch + { + // Catch-all so an unexpected exception type cannot fault this receive task + // and hang DiscoverAsync via Task.WhenAll under the infinite-timeout overload. + break; + } Add a catch (Exception) block around ReceiveAsync. This prevents unexpected network exceptions from crashing the receive loop and aborting the discovery process. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [91-147] private async Task ReceiveLoopAsync(UdpClient udp, IPAddress localAddress, List<IDeviceInfo> discoveredDevices, CancellationToken cancellationToken) { while (!cancellationToken.IsCancellationRequested) { UdpReceiveResult result; try { result = await udp.ReceiveAsync(cancellationToken); } catch (OperationCanceledException) { break; } catch (SocketException) { break; } catch (ObjectDisposedException) { break; } + catch (Exception) + { + // Ensure this receive loop never faults and brings down Task.WhenAll(). + break; + } - // Defense-in-depth: any unexpected exception in payload processing or subscriber - // dispatch must not fault this receive task. With parallel per-NIC loops awaited - // via Task.WhenAll under the infinite-timeout overload, a single faulted task would - // hang DiscoverAsync indefinitely. try { var receivedText = Encoding.ASCII.GetString(result.Buffer); if (!IsValidDiscoveryMessage(receivedText)) { continue; } var deviceInfo = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, localAddress); if (deviceInfo == null) { continue; } lock (discoveredDevices) { if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) { continue; } discoveredDevices.Add(deviceInfo); } OnDeviceDiscovered(deviceInfo); } catch { // Swallow malformed payloads and subscriber exceptions; keep receiving. } } } Suggestion importance[1-10]: 6 __ Why: Adding a catch-all exception block for network operations enhances robustness by preventing unhandled exceptions from terminating the entire discovery process.	Low

✅ Suggestions up to commit 487872b

Category	Suggestion	Impact
Possible issue	✅ Dispose sockets on all failures Suggestion Impact: The commit added an `added` success flag, broadened the exception handler to `catch` all exceptions, and moved disposal into a `finally` block so any failure during socket setup/send disposes the UdpClient while continuing discovery on other NICs. code diff: foreach (var interfaceInfo in networkInterfaces) { UdpClient? udp = null; + var added = false; try { // Bind to the discovery port (with ReuseAddress) rather than an ephemeral @@ -168,10 +169,19 @@ udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); await udp.SendAsync(_queryCommandBytes, _queryCommandBytes.Length, interfaceInfo.BroadcastEndpoint); perNicClients.Add((udp, interfaceInfo.LocalAddress)); + added = true; } - catch (SocketException) + catch { - udp?.Dispose(); + // Skip this NIC; continue with others. Any setup or send failure is + // recoverable at the discovery level. + } + finally + { + if (!added) + { + udp?.Dispose(); + } } Use a broader try/catch block and a success flag to dispose of the UdpClient on any setup failure. This prevents resource leaks and premature abortion of the discovery loop. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [28-51] foreach (var interfaceInfo in networkInterfaces) { UdpClient? udp = null; + var added = false; + try { - // Bind to the discovery port (with ReuseAddress) rather than an ephemeral - // port so devices that target the well-known port for replies still reach - // us. Per-NIC sockets coexist on the same port via distinct (LocalAddress, - // port) tuples + SO_REUSEADDR. udp = new UdpClient { EnableBroadcast = true, ExclusiveAddressUse = false }; udp.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); + await udp.SendAsync(_queryCommandBytes, _queryCommandBytes.Length, interfaceInfo.BroadcastEndpoint); + perNicClients.Add((udp, interfaceInfo.LocalAddress)); + added = true; } - catch (SocketException) + catch { - udp?.Dispose(); + // Skip this NIC; continue with others. + } + finally + { + if (!added) + { + udp?.Dispose(); + } } } Suggestion importance[1-10]: 8 __ Why: Broadening the exception handling ensures that unexpected errors do not prematurely abort the entire network interface iteration or leak socket resources.	Medium
	✅ Keep receive loop from faulting Suggestion Impact: The commit added a defense-in-depth try/catch around the receive-loop payload decoding/validation, device parsing, duplicate-check/add, and subscriber dispatch to prevent unexpected exceptions (including malformed payloads) from faulting the background receive task. code diff: - var receivedText = Encoding.ASCII.GetString(result.Buffer); - if (!IsValidDiscoveryMessage(receivedText)) - { - continue; - } - - var deviceInfo = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, localAddress); - if (deviceInfo == null) - { - continue; - } - - lock (discoveredDevices) - { - if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) + // Defense-in-depth: any unexpected exception in payload processing or subscriber + // dispatch must not fault this receive task. With parallel per-NIC loops awaited + // via Task.WhenAll under the infinite-timeout overload, a single faulted task would + // hang DiscoverAsync indefinitely. + try + { + var receivedText = Encoding.ASCII.GetString(result.Buffer); + if (!IsValidDiscoveryMessage(receivedText)) { continue; } - discoveredDevices.Add(deviceInfo); - } - - // Subscriber exceptions must not fault the receive task — with parallel per-NIC - // loops awaited via Task.WhenAll under an infinite-timeout overload, a single - // faulted task would hang DiscoverAsync indefinitely. - try - { + + var deviceInfo = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, localAddress); + if (deviceInfo == null) + { + continue; + } + + lock (discoveredDevices) + { + if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) + { + continue; + } + discoveredDevices.Add(deviceInfo); + } + OnDeviceDiscovered(deviceInfo); } catch { - // Swallow subscriber exceptions; continue receiving on this NIC. + // Swallow malformed payloads and subscriber exceptions; keep receiving. } Wrap the message processing logic in ReceiveLoopAsync within a try...catch block. This ensures that parsing errors from malformed UDP payloads do not terminate the background task. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [81-136] private async Task ReceiveLoopAsync(UdpClient udp, IPAddress localAddress, List<IDeviceInfo> discoveredDevices, CancellationToken cancellationToken) { while (!cancellationToken.IsCancellationRequested) { UdpReceiveResult result; try { result = await udp.ReceiveAsync(cancellationToken); } - catch (OperationCanceledException) + catch (OperationCanceledException) { break; } + catch (SocketException) { break; } + catch (ObjectDisposedException) { break; } + + try { - break; - } - catch (SocketException) - { - break; - } - catch (ObjectDisposedException) - { - break; - } - - var receivedText = Encoding.ASCII.GetString(result.Buffer); - if (!IsValidDiscoveryMessage(receivedText)) - { - continue; - } - - var deviceInfo = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, localAddress); - if (deviceInfo == null) - { - continue; - } - - lock (discoveredDevices) - { - if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) + var receivedText = Encoding.ASCII.GetString(result.Buffer); + if (!IsValidDiscoveryMessage(receivedText)) { continue; } - discoveredDevices.Add(deviceInfo); - } - // Subscriber exceptions must not fault the receive task — with parallel per-NIC - // loops awaited via Task.WhenAll under an infinite-timeout overload, a single - // faulted task would hang DiscoverAsync indefinitely. - try - { - OnDeviceDiscovered(deviceInfo); + var deviceInfo = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, localAddress); + if (deviceInfo == null) + { + continue; + } + + lock (discoveredDevices) + { + if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) + { + continue; + } + discoveredDevices.Add(deviceInfo); + } + + try + { + OnDeviceDiscovered(deviceInfo); + } + catch + { + // Swallow subscriber exceptions; continue receiving on this NIC. + } } catch { - // Swallow subscriber exceptions; continue receiving on this NIC. + // Ignore malformed/hostile UDP payloads; keep the receive loop alive. + continue; } } } Suggestion importance[1-10]: 8 __ Why: Handling exceptions during payload parsing is crucial to prevent malformed UDP packets from crashing the entire receive loop and halting discovery.	Medium
General	✅ Reduce adapter filter false positives Suggestion Impact: Updated IsVirtualOrTunnelInterface to trim name/description and replaced the generic "TAP" substring check with more specific TAP prefix checks (TAP-Windows, TAP-, TAP ) to reduce false positives. code diff: @@ -448,16 +459,21 @@ /// </summary> internal static bool IsVirtualOrTunnelInterface(string? name, string? description) { - var n = name ?? string.Empty; - var d = description ?? string.Empty; - + var n = (name ?? string.Empty).Trim(); + var d = (description ?? string.Empty).Trim(); + + // Use specific TAP prefixes ("TAP-Windows", "TAP-", "TAP " with trailing space) rather + // than a generic "TAP" substring to avoid false positives on legitimate physical NIC + // descriptions that happen to contain those three letters. return n.StartsWith("vEthernet", StringComparison.OrdinalIgnoreCase) || d.IndexOf("vEthernet", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("Hyper-V", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("WSL", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("VirtualBox", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("VMware", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("TAP", StringComparison.OrdinalIgnoreCase) >= 0; + d.IndexOf("TAP-Windows", StringComparison.OrdinalIgnoreCase) >= 0 || + d.IndexOf("TAP-", StringComparison.OrdinalIgnoreCase) >= 0 || + d.IndexOf("TAP ", StringComparison.OrdinalIgnoreCase) >= 0; } Update IsVirtualOrTunnelInterface to look for specific TAP adapter prefixes like TAP- or TAP-Windows instead of a generic TAP substring. This reduces the chance of falsely excluding legitimate physical network interfaces. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [206-218] internal static bool IsVirtualOrTunnelInterface(string? name, string? description) { - var n = name ?? string.Empty; - var d = description ?? string.Empty; + var n = (name ?? string.Empty).Trim(); + var d = (description ?? string.Empty).Trim(); return n.StartsWith("vEthernet", StringComparison.OrdinalIgnoreCase) || d.IndexOf("vEthernet", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("Hyper-V", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("WSL", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("VirtualBox", StringComparison.OrdinalIgnoreCase) >= 0 || d.IndexOf("VMware", StringComparison.OrdinalIgnoreCase) >= 0 || - d.IndexOf("TAP", StringComparison.OrdinalIgnoreCase) >= 0; + d.IndexOf("TAP-Windows", StringComparison.OrdinalIgnoreCase) >= 0 || + d.IndexOf("TAP ", StringComparison.OrdinalIgnoreCase) >= 0 || + d.IndexOf("TAP-", StringComparison.OrdinalIgnoreCase) >= 0; } Suggestion importance[1-10]: 7 __ Why: Refining the "TAP" substring matching reduces the risk of false positives, which could inadvertently exclude valid physical network adapters from discovery.	Medium

✅ Suggestions up to commit 47c705f

Category	Suggestion	Impact
Possible issue	Bind sockets to discovery port Ensure per-NIC sockets bind to _discoveryPort and enable address/port reuse instead of using an ephemeral port. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [33-38] -udp = new UdpClient(new IPEndPoint(interfaceInfo.LocalAddress, 0)) +udp = new UdpClient { - EnableBroadcast = true + EnableBroadcast = true, + ExclusiveAddressUse = false }; +udp.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); +udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); await udp.SendAsync(_queryCommandBytes, _queryCommandBytes.Length, interfaceInfo.BroadcastEndpoint); perNicClients.Add((udp, interfaceInfo.LocalAddress)); Suggestion importance[1-10]: 9 __ Why: Binding to an ephemeral port instead of the specific _discoveryPort could break discovery if devices are hardcoded to respond to that port, which is a potential critical regression.	High
	✅ Avoid indefinite discovery hang Suggestion Impact: The commit did not add an effective timeout/cancel-after for infinite timeouts, but it did address a related indefinite-hang risk under the infinite-timeout overload by preventing per-NIC receive tasks from faulting due to malformed payloads or subscriber exceptions (wrapping payload processing and OnDeviceDiscovered in a broad try/catch). code diff: + // Defense-in-depth: any unexpected exception in payload processing or subscriber + // dispatch must not fault this receive task. With parallel per-NIC loops awaited + // via Task.WhenAll under the infinite-timeout overload, a single faulted task would + // hang DiscoverAsync indefinitely. + try + { + var receivedText = Encoding.ASCII.GetString(result.Buffer); + if (!IsValidDiscoveryMessage(receivedText)) { continue; } - discoveredDevices.Add(deviceInfo); - } - - // Subscriber exceptions must not fault the receive task — with parallel per-NIC - // loops awaited via Task.WhenAll under an infinite-timeout overload, a single - // faulted task would hang DiscoverAsync indefinitely. - try - { + + var deviceInfo = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, localAddress); + if (deviceInfo == null) + { + continue; + } + + lock (discoveredDevices) + { + if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) + { + continue; + } + discoveredDevices.Add(deviceInfo); + } + OnDeviceDiscovered(deviceInfo); } catch { - // Swallow subscriber exceptions; continue receiving on this NIC. + // Swallow malformed payloads and subscriber exceptions; keep receiving. } Enforce a maximum timeout or an idle timeout even when the provided timeout is infinite to prevent indefinite hanging. src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs [52-56] +var effectiveTimeout = timeout == Timeout.InfiniteTimeSpan + ? TimeSpan.FromSeconds(5) + : timeout; + +using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); +cts.CancelAfter(effectiveTimeout); + var receiveTasks = perNicClients .Select(c => ReceiveLoopAsync(c.Client, c.LocalAddress, discoveredDevices, cts.Token)) .ToArray(); await Task.WhenAll(receiveTasks); Suggestion importance[1-10]: 1 __ Why: The method's infinite wait behavior when timeout is infinite was already present in the original code, and changing it would alter the API's intended semantics.	Low

[cptkoolbeenz]

Pass 1 skip notes:

• Avoid indefinite discovery hang (importance 1/10) — SKIPPED. Qodo's own "Why" explicitly states the change "would alter the API's intended semantics". The infinite-timeout overload is part of the public contract and matches pre-fix behavior; callers who want a bounded scan use DiscoverAsync(TimeSpan).

[cptkoolbeenz]

/improve

[qodo-code-review]

Persistent suggestions updated to latest commit 487872b

[cptkoolbeenz]

/improve

[qodo-code-review]

Persistent suggestions updated to latest commit 0dac743

[cptkoolbeenz]

/improve

[qodo-code-review]

Persistent suggestions updated to latest commit 9aeb434

[cptkoolbeenz]

/improve

[qodo-code-review]

PR Code Suggestions ✨

No code suggestions found for the PR.

[cptkoolbeenz]

/improve

[qodo-code-review]

PR Code Suggestions ✨

No code suggestions found for the PR.

[cptkoolbeenz]

/review

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 179 - Partially compliant Compliant requirements: Bind UDP discovery per NIC (not IPAddress.Any) so each broadcast egresses the intended adapter and replies arrive on the correct socket. Filter out virtual/tunnel adapters (e.g., WSL2 mirrored vEthernet, Hyper-V, VPN/TAP, VirtualBox, VMware) so they do not interfere with discovery. Ensure LocalInterfaceAddress reflects the NIC that actually received the reply (avoid subnet-match guessing). Add a unit or integration test that covers virtual-adapter filtering / mixed NIC lists. Non-compliant requirements: Validate discovery on problematic Windows host setups (WSL2 mirrored on same /24, Hyper-V on same subnet). Requires further human verification: Maintain existing behavior for single-NIC scenarios (regression check). Validate discovery on problematic Windows host setups (WSL2 mirrored on same /24, Hyper-V on same subnet).

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Concurrency ReceiveLoopAsync runs per-NIC tasks in parallel and locks discoveredDevices while invoking OnDeviceDiscovered. This avoids races and preserves sequential callback ordering, but it also means subscriber code runs under a lock, which can deadlock or block discovery if event handlers call back into discovery/other locked paths or are slow. Consider releasing the lock before invoking callbacks (e.g., add to list under lock, then invoke outside) while still preventing duplicates (e.g., maintain a separate concurrent key-set). /// <summary> /// Receives discovery responses on a single NIC-bound socket until cancellation. /// The socket's bound IP is the authoritative LocalInterfaceAddress for any reply it receives. /// </summary> private async Task ReceiveLoopAsync(UdpClient udp, IPAddress localAddress, List<IDeviceInfo> discoveredDevices, CancellationToken cancellationToken) { while (!cancellationToken.IsCancellationRequested) { UdpReceiveResult result; try { result = await udp.ReceiveAsync(cancellationToken); } catch (OperationCanceledException) { break; } catch (SocketException) { break; } catch (ObjectDisposedException) { break; } catch { // Catch-all so an unexpected exception type cannot fault this receive task // and hang DiscoverAsync via Task.WhenAll under the infinite-timeout overload. break; } // Defense-in-depth: any unexpected exception in payload processing or subscriber // dispatch must not fault this receive task. With parallel per-NIC loops awaited // via Task.WhenAll under the infinite-timeout overload, a single faulted task would // hang DiscoverAsync indefinitely. try { var receivedText = Encoding.ASCII.GetString(result.Buffer); if (!IsValidDiscoveryMessage(receivedText)) { continue; } var deviceInfo = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, localAddress); if (deviceInfo == null) { continue; } lock (discoveredDevices) { if (discoveredDevices.Any(d => IsDuplicateDevice(d, deviceInfo))) { continue; } discoveredDevices.Add(deviceInfo); // Invoke under the lock so DeviceDiscovered fires sequentially across the // parallel per-NIC receive loops, matching the original (single-socket) // sequential-callback contract that subscribers may depend on. OnDeviceDiscovered(deviceInfo); } } catch { // Swallow malformed payloads and subscriber exceptions; keep receiving. } } Socket binding behavior Per-NIC sockets attempt to bind (LocalAddress, _discoveryPort) with ReuseAddress, falling back to ephemeral port on SocketException. Verify target devices actually reply to the well-known port vs source port across all firmware versions; if some devices only reply to the well-known port, the fallback-to-ephemeral path could reintroduce “zero replies” behavior under port contention. If possible, log/telemetry or a clearer distinction between “port busy” vs “other bind failure” would help diagnose. // Bind a separate UdpClient per NIC so the OS routes each broadcast out the intended adapter // and replies arrive on the socket whose bound IP is the actual local interface — avoiding // misrouting on hosts where virtual NICs (WSL2 mirrored, Hyper-V) share a subnet with the real one. var perNicClients = new List<(UdpClient Client, IPAddress LocalAddress)>(); try { foreach (var interfaceInfo in networkInterfaces) { UdpClient? udp = null; var added = false; try { // Bind to the discovery port (with ReuseAddress) rather than an ephemeral // port so devices that target the well-known port for replies still reach // us. Per-NIC sockets coexist on the same port via distinct (LocalAddress, // port) tuples + SO_REUSEADDR. udp = new UdpClient { EnableBroadcast = true, ExclusiveAddressUse = false }; udp.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); try { udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, _discoveryPort)); } catch (SocketException) { // Fall back to an ephemeral port if the well-known discovery port // can't be acquired on this platform/NIC (e.g. another process holds // it). Devices that reply to source-port still reach us either way. udp.Client.Bind(new IPEndPoint(interfaceInfo.LocalAddress, 0)); } await udp.SendAsync(_queryCommandBytes, _queryCommandBytes.Length, interfaceInfo.BroadcastEndpoint); perNicClients.Add((udp, interfaceInfo.LocalAddress)); added = true; } catch { // Skip this NIC; continue with others. Any setup or send failure is // recoverable at the discovery level. } finally { if (!added) { udp?.Dispose(); } } } if (perNicClients.Count == 0) { OnDiscoveryCompleted(); return discoveredDevices; } var receiveTasks = perNicClients .Select(c => ReceiveLoopAsync(c.Client, c.LocalAddress, discoveredDevices, cts.Token)) .ToArray(); await Task.WhenAll(receiveTasks); } Adapter filtering IsVirtualOrTunnelInterface relies on substring keywords (WSL, Hyper-V, TAP, etc.) in name/description. This is pragmatic but can produce false positives/negatives on localized OS strings or uncommon adapter naming. Consider whether an opt-out/override is needed (config knob) or whether additional signals (e.g., NetworkInterfaceType, known interface IDs where available) should be used to reduce accidental exclusion of legitimate NICs. /// <summary> /// Returns true if a NIC matching the given metadata should be included in discovery. /// Centralizes the filter — Up + IPv4-capable + Ethernet/Wireless80211 + non-virtual — /// so a mixed-NIC list can be exercised in unit tests without instantiating real /// <see cref="NetworkInterface"/> objects. Internal for testing. /// </summary> internal static bool ShouldIncludeInterface( string? name, string? description, OperationalStatus operationalStatus, NetworkInterfaceType interfaceType, bool supportsIPv4) { if (operationalStatus != OperationalStatus.Up) { return false; } if (!supportsIPv4) { return false; } if (interfaceType != NetworkInterfaceType.Ethernet && interfaceType != NetworkInterfaceType.Wireless80211) { return false; } // Skip virtual/tunnel adapters (WSL2 mirrored vEthernet, Hyper-V, VirtualBox, VMware, TAP) // that frequently share a subnet with the real adapter and cause Windows routing to pick // the wrong egress NIC for broadcasts. See issue #179. if (IsVirtualOrTunnelInterface(name, description)) { return false; } return true; } /// <summary> /// Returns true if the adapter looks like a virtual/tunnel interface that should be skipped /// (WSL2 mirrored vEthernet, Hyper-V, VirtualBox, VMware, TAP). Internal for testing. /// </summary> internal static bool IsVirtualOrTunnelInterface(string? name, string? description) { var n = (name ?? string.Empty).Trim(); var d = (description ?? string.Empty).Trim(); static bool Contains(string haystack, string needle) => haystack.IndexOf(needle, StringComparison.OrdinalIgnoreCase) >= 0; // Match keywords against BOTH name and description — virtualization markers can show // up in either field depending on platform/locale. Use specific TAP prefixes to avoid // false positives on legitimate physical NICs whose description happens to contain // those three letters. return Contains(n, "vEthernet") || Contains(d, "vEthernet") || Contains(n, "Hyper-V") || Contains(d, "Hyper-V") || Contains(n, "WSL") || Contains(d, "WSL") || Contains(n, "VirtualBox") || Contains(d, "VirtualBox") || Contains(n, "VMware") || Contains(d, "VMware") || Contains(n, "TAP-Windows") || Contains(d, "TAP-Windows") || Contains(n, "TAP-") || Contains(d, "TAP-") || Contains(n, "TAP ") || Contains(d, "TAP "); }