fix(devices): a dropped WiFi device is never torn down, and Core's background failures are invisible by tylerkron · Pull Request #172 · daqifi/daqifi-app

tylerkron · 2026-09-01T19:09:15Z

Not merging — for review. Closes #167.

What accumulates, and what the user hits

A device connected over WiFi/TCP that drops — router reboot, AP roam, cable to the AP pulled, a silently dead socket — is never torn down in the desktop shell. Nothing subscribed to the drop, because there was nothing to subscribe to: Core reports ConnectionStatus.Lost, and the app's entire response was to clear IsStreaming and write one warning (AbstractStreamingDevice.cs:603→:623). The only teardown path, CheckIfSerialDeviceWasRemoved, filters ConnectedDevices.OfType<SerialStreamingDevice>() — serial only, by construction.

So per drop, and permanently for the process:

The wrapper stays in ConnectionManager.ConnectedDevices — a static singleton. Through it, its Core device and that device's transport stay reachable. The only code that disposes them is AbstractStreamingDevice.CleanupConnection, reached only from Connect() and Disconnect(), and nothing was calling Disconnect(). For WiFi that is an open socket per drop.
Its channels stay in LoggingManager.SubscribedChannels with IsActive = true and their OnChannelUpdated handler still attached. DaqifiViewModel rebuilds ActiveChannels from that list, and CanToggleLogging = ActiveChannels.Count > 0 — so the app still believes it can start a logging session, and still lists the dead device's inputs as live.
Reconnecting the same board is then blocked by its own corpse: CheckForDuplicateDevice matches the stale entry on serial number and the user gets the duplicate-device prompt.

What the user sees first is the frozen sample counter, with no notification and no route back except disconnecting by hand. The mobile shell was already fine (MobileShellViewModel:710); this was desktop-only.

Gap B, same IDevice surface: Core 1.7.0 raises ErrorOccurred and SendFailed and this app consumed neither (SendFailed had zero grep hits in the repo). A dead read loop, a failed decode, an exhausted reconnect and a SCPI command that never reached the device were all indistinguishable from a quiet device.

The premise was verified before implementing, not assumed. #167 came from a source-level audit; I re-derived it and it holds. IsConnected has three consumers — DeviceTileViewModel re-renders a border, DeviceLogsViewModel greys out SD buttons, MobileShellViewModel tears down — and none of the three is the desktop shell's device lifetime.

The fix

IDevice.ConnectionLost + ConnectionLostEventArgs, raised by AbstractStreamingDevice.OnCoreStatusChanged.
ConnectionManager.OnDeviceConnectionLost releases the channels, calls Disconnect(device) (which disposes the Core device and its transport), and sets LastDisconnectReason before raising NotifyConnection, so the dialog names the device and the reason instead of saying "Device disconnected unexpectedly."
SubscribeDeviceEvents / UnsubscribeDeviceEvents, mirrored, wired at Connect and at both teardowns (Disconnect, Reboot).
AbstractStreamingDevice.Diagnostics.cs forwards Core's ErrorOccurred/SendFailed with this as sender, so a log line names the device the way the user sees it.
Two accept-time guards on Connect (see Review below): a device whose transport is already gone when Connect() returns is refused rather than added, and a drop during the one-second post-connect settle aborts the connect instead of completing it.

The IDeviceWatcher stays. This port's macOS/Linux USB hotplug detection lives there and is strictly better than upstream's pre-deletion state. The two paths are redundant, not conflicting: both skip a device under firmware update, both now converge on one TearDownDroppedDevice, and whichever arrives second finds the device already gone and returns. The serial path also now names the device in its notification, which it did not before.

Four places this deliberately does not copy upstream

Disconnected is not treated as a drop. Upstream's set is Lost | Failed | Disconnected. At Core 1.7.0 Disconnected is produced only by DisconnectCore and MarkDisconnectedWithoutTeardown, both under a caller-issued teardown — and every app-initiated teardown here detaches the handler before touching the Core device. Acting on it could only re-enter teardown on a device already being torn down. Failed is handled although unreachable today (Core only reports it when auto-reconnect exhausts, and this app leaves ReconnectOptions at its disabled default), so enabling reconnect later cannot silently reintroduce the untorn-down device.
IsAppBug(StatusNotification) => true. Upstream enumerates the Core 1.4.0 set, where that member did not exist, and its default arm routes it to Warning. Core 1.7.0 raises it for exactly one thing: a StatusChanged subscriber that threw. Every subscriber to that event is this app's own code, and the usual cause is a cross-thread mutation of bound state from Core's transport thread — a real defect, on the drop path, where it is hardest to notice. fix(devices): a dropped WiFi device is never torn down, and Core's background failures are invisible #167 asked for this to be a decision rather than a default; it is, and a test fails if Core adds a sixth member so the next one is a decision too.
RebindDiagnostics has no upstream counterpart. Upstream binds the Core subscription lazily on first subscriber only. Here Connect() is a template that can run twice on one wrapper, so the bridge is re-pointed on connect and released in CleanupConnection — otherwise a wrapper with a live subscriber holds the Core device cleanup just disposed, which is the same leak shape this PR is about.
ReconcileDeviceAfterUpdate is untouched. It still does teardown only. Its existing comment explains why raising NotifyConnection there would pop a second, contradictory dialog over the firmware coordinator's own; adding LastDisconnectReason does not change that.

Mobile gets the two diagnostics events via RegisterConnectedDevice but deliberately not ConnectionLost: MobileShellViewModel.HandleConnectionLost already tears down and does more than this class can (it returns the shell to its device-list state). Wiring both would be two teardowns racing one drop for no gain.

Review

Qodo found four across two rounds, all accepted and fixed. Three of them are windows this PR's own teardown opened, which is worth stating plainly:

A drop during Connect's one-second post-connect settle completed the connect anyway. The loss handler goes live before that delay and runs on the UI thread while the continuation is suspended, so the device could be torn down and the continuation would still publish Connected and SetDeviceContext for hardware that was gone. Unreachable before this PR, because nothing tore down. Now guarded.
A drop between device.Connect() returning and the handler being wired was unobservable — and the dead wrapper was accepted into ConnectedDevices anyway, recreating the exact stale entry this PR removes. Subscribing earlier does not fix it (the handler early-returns on !ConnectedDevices.Contains(device)); re-reading the device's own IsConnected at the accept point does. The refused device is Disconnect()ed so its transport handle is released rather than leaked.
The channel snapshot could abort teardown. DataChannels is a plain List Core rebuilds wholesale, and TearDownDroppedDevice runs inside an action posted to the dispatcher — so a throw there would take the whole teardown with it and leave the device connected, with nothing to catch it. (I had found and fixed this one independently before the review landed.)
Round 2: the same race, without the exception. Guarding the copy handles the throw but not the quieter half — a read landing between Core's Clear and its AddRange comes back short, or empty, without throwing anything, and every channel missed that way stays in LoggingManager active for the process lifetime. So teardown no longer asks the device which channels are its own; it asks the logger, whose SubscribedChannels is published copy-on-write and therefore cannot be observed mid-rebuild, filtering by serial number ordinally — the same discriminator LoggingManager.Unsubscribe resolves by. That is also strictly more complete: it catches a subscription left over from an earlier session of the same device, which the device's own list would no longer mention. A device that never reported a serial cannot be matched that way (a blank serial would sweep up every other unidentified device's channels), so that case alone keeps the device's own list.

Verification

Tests: 27 → 65. 38 new, all green (dotnet test), and confirmed green on CI's Linux job.

The teardown suite is the regression evidence. With OnDeviceConnectionLost neutered to reproduce main's behaviour, 8 of its 10 cases fail — removed-from-list, channels-released, channels-released-before-the-device-clears-them, reason-names-the-device, only-the-dropped-device, subscribed-twice, second-report-is-a-no-op, and the one that goes through ConnectionManager.Connect() itself. Each assertion is literally the state the app was left in. The two that still pass are the ones asserting nothing happens (firmware-update exemption, explicitly-disconnected device unwired) — they pass vacuously against main and are guards on the fix, not proof of the bug.

ConnectionManager gets an internal test constructor replacing exactly three things that make the singleton unconstructible in a test host: its real serial-poller watcher, LoggingManager.Instance (whose ctor needs App.ServiceProvider), and the UI marshal (Avalonia's dispatcher binds to one thread and queues onto it forever when nothing pumps). Teardown, the firmware carve-out, the subscription bookkeeping and the notification are all production code.

A_drop_while_the_connection_is_settling_aborts_the_connect is the one timing-dependent case: it fires the drop from a background task that waits until the device is in ConnectedDevices, then pauses 50 ms — ~50 ms into a 1000 ms window. Both failure directions are red rather than green (too early: nothing is wired, so no teardown; too late: the device is still in the list), and it held over 8 consecutive runs.

Channels_are_released_even_if_the_device_list_is_being_rebuilt clears DataChannels to reproduce Core's rebuild interval exactly. It fails against the previous implementation — verified by reverting the lookup: 64 passed, 1 failed.

DescribeCommand is pinned directly on the case it exists for: SYSTem:COMMunicate:LAN:PASs "hunter2" logs as SYSTem:COMMunicate:LAN:PASs, asserted to not contain the password.

Bench (real Nq1, fw 3.7.2, USB, /dev/cu.usbmodem1101). The physical unplug that produces Lost needs a hand on the cable, so it was not reproduced. What the bench covers is the false-positive direction — the risk this change introduces. A headless harness driving the app's own SerialStreamingDevice path, with all three events subscribed before Connect() (so RebindDiagnostics's connect-side attach is the code under test), over 5 connect/disconnect cycles:

cycle 1..5: Connect()=True in 4263-4322 ms, IsConnected-immediately=True,
            serial=9090539562006014104, channels=32
            after 3 s idle + Disconnect()=True: IsConnected=False

connects the new accept-guard would have REJECTED: 0 / 5   (expected 0)
ConnectionLost raises across all cycles:            0       (expected 0)
ErrorOccurred: 0   SendFailed: 0

The first line is the one that matters for guard #2 above: a false rejection there would stop the app connecting at all, and IsConnected read true immediately after Connect() on 5 of 5. ErrorOccurred/SendFailed were attached across real sessions with real device traffic (the metadata above came back over the wire) and raised zero times — no failure was induced, so the handlers' log formatting and severity are covered by the unit tests, not by the bench. Board left disconnected with nothing enabled and nothing streaming; board lock held for each run and released.

Build gate. Measured on the merge-base and on this head with the same cleared-obj discipline and the identical command, because the count moves with build state:

	merge-base	this head
Desktop, total warnings	278	278
Desktop, distinct warning sites	209	209
iOS Release `iossimulator-arm64`	1235	1235
`dotnet test`	27 passed	65 passed

Flat on both heads, 0 errors. (A first pass added one CS8618 from the test constructor; fixed rather than shipped.) git status --short -- '**/packages.lock.json' is empty — restore was run without -r. The Android head does not build on this Mac (no workload); CI covers it.

Collision check

Diffed #159, #161, #163, #164 and #170 first. #170 is the only adjacency: it edits AbstractStreamingDevice.Connect() (adds a HydrateDeviceMetadata call after OnCoreDeviceInitialized()) and HydrateDeviceMetadata itself. This PR's Connect() edit is one line ~10 lines earlier, right after CoreDevice = coreDevice; — textually adjacent, semantically independent. #165 (channel enablement) was left alone. No .portomatic/map/ edit: neither new file has a map row today, so nothing there is made stale, and three open PRs are editing UI.yaml.

Sizing

git diff --numstat origin/main...HEAD: 1677 insertions, 14 deletions across 11 files — 797 lines in 4 new test files, 880 insertions / 14 deletions in 7 app files.

Noticed, deliberately not included

ConnectionManager.Disconnect and the post-connect duplicate-rejection path both drop a device without disposing it, so each leaks its transport handle — upstream fixed this with (device as IDisposable)?.Dispose(). It is the same class of bug, but disposing there changes the lifetime of wrappers the connection dialog holds and reconnects, which I cannot validate from here. Worth its own issue rather than a rider on this one.

🤖 Generated with Claude Code

…'s background failures A WiFi/TCP device that dropped stayed in ConnectedDevices forever. Nothing in the desktop shell subscribed to the drop, because there was nothing to subscribe to: Core's ConnectionStatus.Lost only cleared IsStreaming and wrote one warning. The one teardown path, CheckIfSerialDeviceWasRemoved, filters ConnectedDevices.OfType<SerialStreamingDevice>() — serial only. Per drop, permanently for the process: the wrapper, its Core device and that device's transport (for WiFi, an open socket) stay reachable from a static singleton, because the only code that disposes them is Disconnect, which nothing was calling. Its channels stay in LoggingManager marked active, still counting toward CanToggleLogging and still listed as live inputs. Adds IDevice.ConnectionLost, raised by AbstractStreamingDevice on Core's Lost and Failed (not Disconnected — at Core 1.7.0 that is only ever a caller-issued teardown, whose handler is already detached), and a ConnectionManager handler that releases the channels, disconnects, and notifies naming the device and the reason. The IDeviceWatcher stays: the two paths are redundant, not conflicting, and both now converge on one TearDownDroppedDevice. Also ports IDevice.ErrorOccurred / SendFailed, which Core 1.7.0 raises and this app never consumed, so a dead read loop, a failed decode, an exhausted reconnect and a dropped SCPI command stop being indistinguishable from silence. DescribeCommand logs the verb only, which is what keeps the WiFi password out of the log. IsAppBug routes StatusNotification to Error rather than inheriting upstream's Core-1.4.0 default — every StatusChanged subscriber is this app's own code. Closes #167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The test constructor left DuplicateDeviceHandler unassigned, which is a CS8618 — the same one the singleton constructor already carries. Assign it explicitly to the state the singleton is in until the connection dialog installs one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

tylerkron · 2026-09-01T19:09:23Z

/agentic_review

qodo-code-review · 2026-09-01T19:10:04Z

PR Summary by Qodo

Tear down dropped devices and surface Core failures

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Tear down unexpectedly dropped devices, release channels, and notify users with specific reasons.
• Surface Core background and send failures with privacy-safe, severity-aware logging.
• Add regression coverage for teardown ordering, idempotency, event wiring, and diagnostics policy.

Diagram

graph TD
    Core["Core Events"] --> Wrapper["Device Wrapper"] --> Kind{"Event Kind"}
    Kind -->|Lost or Failed| Manager["Connection Manager"] --> Channels["Logging Channels"]
    Manager -->|Disconnect| Transport["Device Transport"]
    Manager -->|Notify| Dialog["User Dialog"]
    Kind -->|Background Failure| AppLog["App Log"]

High-Level Assessment

The event-driven approach is appropriate because Core is the authoritative observer of spontaneous transport loss, while ConnectionManager owns desktop device lifetime and channel subscriptions. Watching IsConnected would duplicate property-observation behavior and provide less precise reasons, while subscribing ConnectionManager directly to Core would break the device-wrapper abstraction. Retaining the serial watcher as an idempotent secondary signal also preserves platform hotplug behavior without creating competing teardown implementations.

Files changed (11) +1364 / -14

Enhancement (3) +268 / -0

AbstractStreamingDevice.Diagnostics.csBridge Core diagnostic events through device wrappers +205/-0

Bridge Core diagnostic events through device wrappers
• Forwards Core ErrorOccurred and SendFailed events with the wrapper as sender. Thread-safe, lazy subscription bookkeeping rebinds across reconnects and detaches from disposed Core devices.
Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.Diagnostics.cs

ConnectionLostEventArgs.csDefine unexpected connection-loss details +19/-0

Define unexpected connection-loss details
• Adds event arguments carrying a concise human-readable reason for an unexpected transport loss.
Daqifi.Avalonia/Daqifi.Desktop/Device/ConnectionLostEventArgs.cs

IDevice.csExtend devices with loss and diagnostic events +44/-0

Extend devices with loss and diagnostic events
• Adds ConnectionLost, ErrorOccurred, and SendFailed contracts to the device abstraction. Documents event semantics, threading requirements, and subscription lifetimes.
Daqifi.Avalonia/Daqifi.Desktop/Device/IDevice.cs

Bug fix (3) +496 / -14

ConnectionManager.csCoordinate dropped-device teardown and Core diagnostics +433/-10

Coordinate dropped-device teardown and Core diagnostics

• Subscribes desktop devices to connection-loss and diagnostic events, marshals teardown to the UI thread, releases channels before disconnecting, and provides device-specific notifications. Unifies serial and Core drop paths, preserves firmware-update behavior, logs background failures by severity, and redacts command arguments from send-failure logs.

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs

AbstractStreamingDevice.csEmit terminal connection-loss events and rebind diagnostics +53/-3

Emit terminal connection-loss events and rebind diagnostics
• Raises ConnectionLost for Core Lost and Failed states after settling wrapper state, while excluding explicit Disconnected and active Retrying states. Rebinds diagnostic forwarding when Core device sessions are created or cleaned up.
Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs

DaqifiViewModel.csShow device-specific disconnect notifications +10/-1

Show device-specific disconnect notifications
• Uses ConnectionManager's detailed disconnect reason in the error dialog with the previous generic message as fallback. Clears the reason after display to prevent stale notification text.
Daqifi.Avalonia/Daqifi.Desktop/ViewModels/DaqifiViewModel.cs

Tests (4) +592 / -0

ConnectionManagerDiagnosticsTests.csTest diagnostic redaction and severity policy +115/-0

Test diagnostic redaction and severity policy
• Adds tests ensuring failed commands log only their bounded SCPI verb and never arguments such as WiFi passwords. Verifies every Core error source is deliberately classified as an application bug or environmental warning.
Daqifi.Avalonia.Tests/ConnectionManagerDiagnosticsTests.cs

ConnectionManagerTeardownTests.csTest complete dropped-device teardown behavior +244/-0

Test complete dropped-device teardown behavior
• Covers event wiring, connected-list removal, channel release ordering, user notification, firmware-update exclusions, idempotency, and isolation between devices. Also verifies explicit disconnect removes event subscriptions.
Daqifi.Avalonia.Tests/ConnectionManagerTeardownTests.cs

AbstractStreamingDeviceConnectionLostTests.csTest terminal connection-loss signaling +127/-0

Test terminal connection-loss signaling
• Verifies Lost and Failed statuses raise ConnectionLost with the correct sender and reason. Confirms non-terminal and app-initiated disconnect statuses do not emit false drop events.
Daqifi.Avalonia.Tests/Device/AbstractStreamingDeviceConnectionLostTests.cs

DroppedDeviceTestDoubles.csAdd transport-free device teardown test doubles +106/-0

Add transport-free device teardown test doubles
• Introduces a controllable streaming-device double that can replay Core status transitions without a live transport. Adds identity-preserving fake channels for teardown assertions.
Daqifi.Avalonia.Tests/Device/DroppedDeviceTestDoubles.cs

Other (1) +8 / -0

Daqifi.Avalonia.csprojExpose application internals to unit tests +8/-0

Expose application internals to unit tests
• Grants the test assembly access to internal constructors and policy helpers used to exercise teardown without application singletons or a live UI dispatcher.
Daqifi.Avalonia/Daqifi.Avalonia.csproj

…down TearDownDroppedDevice runs inside an action posted to the dispatcher, so an exception copying DataChannels would take the whole teardown with it and leave the device connected — the exact state this fixes — with nothing to catch it. DataChannels is a plain List that Core's background events rebuild wholesale, which is the same hazard ActiveChannelCount already guards. Snapshot it behind a catch and let the disconnect proceed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

qodo-code-review · 2026-09-01T19:13:15Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. ~~Racy snapshot leaks channels~~ ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

SnapshotChannels can observe an empty or partial DataChannels list while Core performs Clear
followed by AddRange, and its exception path also deliberately returns an empty list.
TearDownDroppedDevice then disconnects and clears the device without unsubscribing the omitted
channels, leaving them active in LoggingManager for the process lifetime.

Code

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[R861-864]

+            return device.DataChannels?.ToList() ?? [];
+        }
+        catch (Exception ex)
+        {

Relevance

●●● Strong
Accepted precedents recognize mutable DataChannels races and require safe snapshots to preserve
teardown reliability.
PR-#125
PR-#43

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The teardown unsubscribes only channels returned by SnapshotChannels, while Disconnect has no
logging-subscription cleanup. Core mutates the same plain list through separate Clear and
AddRange operations, so the copy can legitimately observe no channels without throwing; if copying
does throw, the new catch explicitly produces the same empty result. LoggingManager.Unsubscribe is
the only shown path that deactivates and removes each subscription and detaches its update handler.
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[830-869]
Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1998-2038]
Daqifi.Avalonia/Daqifi.Desktop/Loggers/LoggingManager.cs[514-542]
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[508-518]

Agent prompt

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

## Issue description
`SnapshotChannels` takes a one-shot copy of an unsynchronized list that Core rebuilds using `Clear` and `AddRange`. It can therefore return an empty/partial snapshot—or return empty after an exception—causing teardown to leave channel subscriptions active permanently.

## Issue Context
Disconnect clears `DataChannels` but does not remove channels from `LoggingManager`; only explicit channel unsubscription performs that cleanup. A valid empty snapshot during the `Clear`/`AddRange` interval cannot be fixed reliably with exception handling alone.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[830-870]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1998-2038]
- Daqifi.Avalonia/Daqifi.Desktop/Loggers/LoggingManager.cs[514-542]

Synchronize channel rebuilding and teardown snapshotting through shared locking or provide an atomic snapshot API. Ensure teardown can identify and unsubscribe every channel belonging to the dropped device even when rebuilding is concurrent, and add a regression test for snapshots taken during the clear/repopulate interval.

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

2. ~~Settling drop restores connected state~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

A loss during Connect's one-second settling delay now tears down and removes the device, but the
suspended continuation still unconditionally sets ConnectionStatus to Connected and installs
telemetry context for that dead device. This leaves connection state and diagnostics claiming
success after the new teardown has completed.

Code

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[R443-446]

+            // Only now: these events bridge to the Core device, which does not exist until
+            // device.Connect() above has created it, and the teardown handler must not fire for a
+            // device that has not been accepted into ConnectedDevices yet.
+            SubscribeDeviceEvents(device);

Relevance

●●● Strong
Recent accepted precedents consistently guard stale asynchronous continuations from overwriting
post-teardown state.
PR-#155
PR-#2

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR activates loss handling at line 446 and then yields for one second. The loss handler can run
on the UI thread during that yield and call Disconnect, which removes the device and clears
telemetry, but lines 449-459 execute afterward without checking membership or IsConnected,
restoring connected status and dead-device context.
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[441-459]
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[754-801]
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[468-497]

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 newly subscribed loss handler can remove a device while `Connect` is awaiting its one-second delay. When the delay completes, `Connect` continues as though the device were still accepted and connected.

## Issue Context
Before publishing successful status, telemetry, or breadcrumbs, verify that the same device remains in `ConnectedDevices` and is still connected. Treat a teardown during settling as an aborted connection rather than success.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[441-458]
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[743-801]

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

3. ~~Channel snapshot can abort teardown~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

device.DataChannels?.ToList() still enumerates a mutable List that Core concurrently clears and
repopulates, so it can throw InvalidOperationException before Disconnect executes. That
exception escapes the posted callback and leaves the dropped device, transport, and subscribed
channels retained—the exact leak this teardown is intended to prevent.

Code

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[R793-796]

+        foreach (var channel in device.DataChannels?.ToList() ?? [])
+        {
+            UnsubscribeChannel(channel);
+        }

Relevance

●●● Strong
Accepted precedents specifically address mutable channel enumeration aborting teardown and require
safe snapshots or fallback handling.
PR-#125
PR-#43

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

DataChannels is a plain List<IChannel>, and SyncChannelsFromCore mutates it with Clear() and
AddRange() from Core event processing. ToList() enumerates that same list; if it throws, control
never reaches Disconnect at line 798 or the notification at lines 800-801. Past PR #153 documents
the same live-DataChannels enumeration race elsewhere in this repository.

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[791-801]
Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1998-2038]
PR-#153

Agent prompt

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

## Issue description
Creating a LINQ snapshot does not make enumeration of `DataChannels` safe while Core mutates the underlying list. A concurrent channel refresh can abort dropped-device teardown before the device is disconnected or removed.

## Issue Context
Introduce shared synchronization or a device-owned snapshot API used by both channel mutation and teardown. Also ensure an inability to release one channel cannot prevent `Disconnect` and removal from running.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[791-801]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1998-2038]

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

View high (1)

4. ~~Early drops escape teardown~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

Connect does not subscribe to ConnectionLost until after the Core connection succeeds, duplicate
checking completes, and the device is added, so a transport loss during that interval is never
observed by ConnectionManager. The method then adds the already-disconnected wrapper and reports a
successful connection, recreating the stale-device and duplicate-reconnect problem this PR targets.

Code

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[R443-446]

+            // Only now: these events bridge to the Core device, which does not exist until
+            // device.Connect() above has created it, and the teardown handler must not fire for a
+            // device that has not been accepted into ConnectedDevices yet.
+            SubscribeDeviceEvents(device);

Relevance

●●● Strong
Accepted history prioritizes handling connection races and missed early lifecycle events.
PR-#4
PR-#150

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The Core connection is established at lines 424-429, but manager event wiring is deferred until
lines 443-446 after post-connect work and list insertion. OnCoreStatusChanged raises
ConnectionLost immediately on a lost/failed status, while OnDeviceConnectionLost is the only
desktop handler that removes the wrapper; therefore a loss in the unwired interval is permanently
missed.
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[424-446]
Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[631-651]
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[743-773]

Agent prompt

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

## Issue description
A device can report `ConnectionLost` after `device.Connect()` succeeds but before `SubscribeDeviceEvents` runs. With no manager subscriber present, the loss is discarded and the disconnected wrapper is subsequently accepted into `ConnectedDevices`.

## Issue Context
The wrapper events can be subscribed before Core exists: diagnostics are rebound when `Connect()` assigns `CoreDevice`. Ensure failed/aborted setup also removes those subscriptions, and revalidate connection state before accepting the device.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[424-446]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[399-405]
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[743-773]

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

Context sources

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit b479876

Results up to commit 4376c02 ⚖️ Balanced

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

1. ~~Settling drop restores connected state~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

A loss during Connect's one-second settling delay now tears down and removes the device, but the
suspended continuation still unconditionally sets ConnectionStatus to Connected and installs
telemetry context for that dead device. This leaves connection state and diagnostics claiming
success after the new teardown has completed.

Code

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[R443-446]

+            // Only now: these events bridge to the Core device, which does not exist until
+            // device.Connect() above has created it, and the teardown handler must not fire for a
+            // device that has not been accepted into ConnectedDevices yet.
+            SubscribeDeviceEvents(device);

Relevance

●●● Strong
Recent accepted precedents consistently guard stale asynchronous continuations from overwriting
post-teardown state.
PR-#155
PR-#2

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR activates loss handling at line 446 and then yields for one second. The loss handler can run
on the UI thread during that yield and call Disconnect, which removes the device and clears
telemetry, but lines 449-459 execute afterward without checking membership or IsConnected,
restoring connected status and dead-device context.
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[441-459]
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[754-801]
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[468-497]

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 newly subscribed loss handler can remove a device while `Connect` is awaiting its one-second delay. When the delay completes, `Connect` continues as though the device were still accepted and connected.

## Issue Context
Before publishing successful status, telemetry, or breadcrumbs, verify that the same device remains in `ConnectedDevices` and is still connected. Treat a teardown during settling as an aborted connection rather than success.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[441-458]
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[743-801]

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

2. ~~Channel snapshot can abort teardown~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

device.DataChannels?.ToList() still enumerates a mutable List that Core concurrently clears and
repopulates, so it can throw InvalidOperationException before Disconnect executes. That
exception escapes the posted callback and leaves the dropped device, transport, and subscribed
channels retained—the exact leak this teardown is intended to prevent.

Code

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[R793-796]

+        foreach (var channel in device.DataChannels?.ToList() ?? [])
+        {
+            UnsubscribeChannel(channel);
+        }

Relevance

●●● Strong
Accepted precedents specifically address mutable channel enumeration aborting teardown and require
safe snapshots or fallback handling.
PR-#125
PR-#43

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

DataChannels is a plain List<IChannel>, and SyncChannelsFromCore mutates it with Clear() and
AddRange() from Core event processing. ToList() enumerates that same list; if it throws, control
never reaches Disconnect at line 798 or the notification at lines 800-801. Past PR #153 documents
the same live-DataChannels enumeration race elsewhere in this repository.

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[791-801]
Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1998-2038]
PR-#153

Agent prompt

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

## Issue description
Creating a LINQ snapshot does not make enumeration of `DataChannels` safe while Core mutates the underlying list. A concurrent channel refresh can abort dropped-device teardown before the device is disconnected or removed.

## Issue Context
Introduce shared synchronization or a device-owned snapshot API used by both channel mutation and teardown. Also ensure an inability to release one channel cannot prevent `Disconnect` and removal from running.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[791-801]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1998-2038]

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

3. ~~Early drops escape teardown~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

Connect does not subscribe to ConnectionLost until after the Core connection succeeds, duplicate
checking completes, and the device is added, so a transport loss during that interval is never
observed by ConnectionManager. The method then adds the already-disconnected wrapper and reports a
successful connection, recreating the stale-device and duplicate-reconnect problem this PR targets.

Code

Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[R443-446]

+            // Only now: these events bridge to the Core device, which does not exist until
+            // device.Connect() above has created it, and the teardown handler must not fire for a
+            // device that has not been accepted into ConnectedDevices yet.
+            SubscribeDeviceEvents(device);

Relevance

●●● Strong
Accepted history prioritizes handling connection races and missed early lifecycle events.
PR-#4
PR-#150

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The Core connection is established at lines 424-429, but manager event wiring is deferred until
lines 443-446 after post-connect work and list insertion. OnCoreStatusChanged raises
ConnectionLost immediately on a lost/failed status, while OnDeviceConnectionLost is the only
desktop handler that removes the wrapper; therefore a loss in the unwired interval is permanently
missed.
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[424-446]
Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[631-651]
Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[743-773]

Agent prompt

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

## Issue description
A device can report `ConnectionLost` after `device.Connect()` succeeds but before `SubscribeDeviceEvents` runs. With no manager subscriber present, the loss is discarded and the disconnected wrapper is subsequently accepted into `ConnectedDevices`.

## Issue Context
The wrapper events can be subscribed before Core exists: diagnostics are rebound when `Connect()` assigns `CoreDevice`. Ensure failed/aborted setup also removes those subscriptions, and revalidate connection state before accepting the device.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[424-446]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[399-405]
- Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs[743-773]

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

…eady dropped Two windows Qodo found, both opened or widened by this PR's own teardown. Between device.Connect() returning and SubscribeDeviceEvents wiring the loss handler, a drop is unobservable — there is no subscriber, and even with one the handler would find the device not yet in ConnectedDevices and return. So subscribing earlier does not close it; re-reading the device's own IsConnected does. Accepting a wrapper whose transport is already gone would put exactly the stale entry into ConnectedDevices that this PR exists to remove. The rejected device is disconnected rather than left holding its transport handle. During Connect's one-second post-connect settle the handler IS live, and it runs on the UI thread while the continuation is suspended. Without a guard the continuation resumed and published Connected plus SetDeviceContext for hardware that had just been torn down. Both now report Error, which is what every other failure in this method reports and what keeps the connection dialog open. Bench (Nq1 fw 3.7.2, USB, 5 connect/disconnect cycles): the accept-guard's predicate read true immediately after Connect() on all 5, so it rejects no healthy connect; 0 ConnectionLost, 0 ErrorOccurred, 0 SendFailed throughout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

tylerkron · 2026-09-01T19:22:57Z

/agentic_review

qodo-code-review · 2026-09-01T19:26:04Z

Code review by qodo was updated up to the latest commit 27d020e

Qodo round 2. Snapshotting DataChannels behind a catch handles the throw but not the quieter half of the same race: Core rebuilds that list with a Clear followed by an AddRange, so a read landing in between comes back short — or empty — without throwing anything. Every channel missed that way stays in LoggingManager marked active for the process lifetime, still counting toward CanToggleLogging. That is the leak this PR exists to close, reintroduced through a race. The logger's own SubscribedChannels has no such window: it is published copy-on-write, so a reader always gets a complete immutable list. Teardown now filters it by the device's serial number, ordinally, which is the same discriminator LoggingManager.Unsubscribe resolves by. A device with no serial cannot be matched that way — a blank serial would sweep up every other unidentified device's channels — so that case alone keeps the device's own list. Test: Channels_are_released_even_if_the_device_list_is_being_rebuilt clears DataChannels to reproduce the interval exactly, and fails against the previous implementation. A_device_with_no_serial_number_falls_back_to_its_own_channel_list pins the fallback, including that it does not touch an identified device's channels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

tylerkron · 2026-09-01T19:31:46Z

/agentic_review

…st commit It still said channels are released before Disconnect because Disconnect clears DataChannels — true when the lookup read DataChannels, but the lookup now reads the logger. The ordering still matters, for the no-serial-number fallback only, so say that instead. The matching test is renamed and widened to assert both halves of what teardown must leave behind: nothing subscribed, nothing connected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

qodo-code-review · 2026-09-01T19:35:25Z

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

tylerkron · 2026-09-01T19:36:25Z

/agentic_review

qodo-code-review · 2026-09-01T19:37:29Z

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

# Conflicts: # Daqifi.Avalonia/Daqifi.Avalonia.csproj

tylerkron and others added 2 commits September 1, 2026 13:01

qodo-code-review Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs

Comment thread Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs

Comment thread Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs Outdated

qodo-code-review Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread Daqifi.Avalonia/Daqifi.Desktop/ConnectionManager.cs

Merge remote-tracking branch 'origin/main' into diag-172

b479876

# Conflicts: # Daqifi.Avalonia/Daqifi.Avalonia.csproj

tylerkron mentioned this pull request Sep 1, 2026

fix(firmware): a successful WiFi update is reported as a failure, and the user is told to re-flash #176

Merged

tylerkron added this pull request to the merge queue Sep 1, 2026

tylerkron mentioned this pull request Sep 1, 2026

fix(firmware): device discovery kept probing the bus during a manual HID flash #178

Merged

Merged via the queue into main with commit 4382067 Sep 1, 2026
7 checks passed

tylerkron deleted the fix/167-wifi-drop-teardown branch September 1, 2026 22:43

tylerkron mentioned this pull request Sep 1, 2026

chore: delete twelve types nothing in the app can reach #177

Merged

This was referenced Sep 2, 2026

fix(startup): a damaged DAQiFiDatabase.db made the app permanently unopenable #181

Merged

test(system-test): five HeadlessBench rows back — and the pane handles #260 said needed a design pass are just a constructor #275

Merged

tylerkron mentioned this pull request Sep 21, 2026

The app's USB-unplug watcher duplicates Core 1.8.0's serial liveness detector; both race to the same teardown #441

Open

Navigation Menu

Repository navigation

fix(devices): a dropped WiFi device is never torn down, and Core's background failures are invisible - #172

fix(devices): a dropped WiFi device is never torn down, and Core's background failures are invisible#172
tylerkron merged 7 commits into
mainfrom
fix/167-wifi-drop-teardown

tylerkron commented Sep 1, 2026 •

edited

Loading

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

qodo-code-review Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

tylerkron commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What accumulates, and what the user hits

The fix

Four places this deliberately does not copy upstream

Review

Verification

Collision check

Sizing

Noticed, deliberately not included

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

tylerkron commented Sep 1, 2026

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tylerkron commented Sep 1, 2026 •

edited

Loading

qodo-code-review Bot commented Sep 1, 2026 •

edited

Loading