[tylerkron]

Not merging. This is for review.

What was untested, and why it matters

On macOS and Linux, SerialPortPollingDeviceWatcher is the only code that notices a USB device being unplugged. It samples SerialPort.GetPortNames() on a timer and raises DeviceRemoved when a port it saw before is gone. ConnectionManager then runs CheckIfSerialDeviceWasRemoved.

It can go wrong in two ways, and the user pays for both:

• A missed removal. An unplugged device keeps showing as connected, and a logging session keeps "running" with nothing coming in.
• A spurious removal. The removal path goes looking for a device to tear down.

The class has three guards against the second failure:

• a generation counter, so a poll that is still reading when Stop/Start happens cannot use a stale reading;
• a reentrancy gate;
• a rule that a failed enumeration keeps the last snapshot. Without it, a failed read would look like every device being unplugged at once.

None of these had a test. The class had no test at all. Proof: a case-folded search of Daqifi.Avalonia.Tests for serialportpolling returns 0 files. The same search for diskspacemonitor (positive control) finds DiskSpace/DiskSpaceMonitorTests.cs. ConnectionManagerTeardownTests uses NoOpDeviceWatcher, so this class is never exercised there either.

What this adds

This PR adds one test file, Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs, with 16 cases (14 facts, plus one theory with 2 rows). There are no production changes. The tests pin what the class does today:

Area	Pinned behaviour
What counts as a removal	A dropped port fires once, and later identical polls stay quiet. An added port is not a removal. A swap (same count, different port) is a removal. Port names compare case-insensitively. Start takes the baseline synchronously, before any poll.
Enumeration failures	A failed read is not reported as "everything unplugged". The pre-outage snapshot survives the outage, so a removal during it is reported on the first good read. The warning is logged once per outage, and again for a second outage. A failed baseline read makes the first good read the baseline, with no event.
Lifecycle	Ports removed while stopped are not reported after a restart. A poll still reading across Stop+Start cannot poison the new baseline. A second Start is a no-op. Stop before Start and a double Dispose are harmless. Start after Dispose throws. A non-positive poll interval is rejected (ParamName == "pollInterval").
Throwing subscriber	It is logged at Error as "Serial port hotplug poll failed.", and polling continues and reports the next removal.

No sleeps. The tests drive the real timer and synchronise on the injected port provider:

• A poll calls the provider only after it passes the reentrancy gate.
• The gate is released in the poll's finally, after the event has been raised.
• So once the provider has been called for the next poll, the previous poll has finished completely, event included.

"No event was raised" is asserted only after that has been observed. The only wall-clock value is a 10 s failure timeout on that wait.

The fake IAppLogger is minimal and private. It can fold into the shared RecordingAppLogger from #401 once that merges. This PR does not depend on #401.

Evidence the tests bite: mutation testing

Every test passes on unmodified main, as characterisation tests should. So I mutated the production file 13 times. Each mutation was a single fresh edit, and the file was restored and touched between runs. For every run I confirmed that 16 tests executed.

#	Mutation	Result
1	interval guard <= → <	killed: A_poll_interval_that_is_not_positive_is_rejected(0)
2	remove the _timer != null early return in Start	killed: A_second_Start_is_a_no_op
3	drop _generation != generation from the in-lock check	killed: …cannot_poison_the_new_baseline
4	!IsSubsetOf → Count >	killed: One_port_swapped_for_another…
5	OrdinalIgnoreCase → Ordinal	killed: Port_names_are_compared_ignoring_case
6	drop the _enumerationFailureLogged = false reset	killed: An_enumeration_outage_is_logged_once…
7	a failed read returns an empty set instead of null	killed: A_failed_read_is_not_mistaken…
8	the new reading never becomes the baseline	killed: 6 tests
9	never release the reentrancy gate	killed: 10 tests (bounded-wait timeouts)
10	Start takes no baseline	killed: 6 tests
11	the poll's catch stops logging	killed: A_throwing_subscriber…
12	drop the ObjectDisposedException guard in Start	killed: Start_after_Dispose_throws
13	drop the generation re-check just before the event is raised	survives, documented

12 of 13 killed. Mutation 13 survives, and the test's doc comment says why. That re-check only covers a Stop that lands between the lock being released and the event being raised. A test cannot hold that window open without adding a hook to production code, which this PR does not do. The stale-reading test deliberately uses a superset reading. A stale reading that drops ports would be caught by both checks, so it could not tell whether the in-lock check had been removed. The superset reading can.

I did not run one further mutation: removing the poll's catch entirely. An exception thrown from a timer callback terminates the test host, so that "kill" would take down the whole run.

Verification

• dotnet build Desktop: exit 0.
• iOS restore, then Release iossimulator-arm64 --no-restore build: exit 0.
• dotnet test Daqifi.Avalonia.Tests: 1009 passed, 0 failed (16 of them new).
• packages.lock.json: unchanged.
• Test code only. No production file is in the diff, so there is no first-party production warning delta.
• No device involved, so no bench run.

Scope

• Touches no file in any of the open PRs, nor DaqifiViewModel.cs, and nothing in Devices pane paints one or two frames with the settings drawer forced open on every tab visit (pre-OnLoaded DataContext) #397's scope.
• VersionNotification was also untested, but I passed over it: fix(update): the version check queries daqifi-desktop, reads the wrong assembly, and Sentry-errors on any prerelease tag #345 slates it for deletion.
• No defect found. Nothing filed.

Diffstat: one new test file, +459/−0; production untouched. Net-additive because it is a test-only coverage PR: nothing got smaller, one previously unguarded class now has its guards pinned.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Characterize POSIX serial-port removal polling

🧪 Tests 🕐 20-40 Minutes

AI Description

• Adds 16 characterization cases for POSIX serial-port removal polling.
• Covers removal semantics, enumeration outages, lifecycle races, and subscriber failures.
• Uses synchronized test doubles instead of sleeps; production code remains unchanged.

Diagram

sequenceDiagram
    participant Tests as Watcher tests
    participant Watcher as Port watcher
    participant Timer as Poll timer
    participant Ports as Port table
    participant Sink as Event subscriber
    participant Logger as App logger
    Tests->>Ports: Configure snapshots
    Tests->>Watcher: Control lifecycle
    Watcher->>Ports: Capture baseline
    Watcher->>Timer: Schedule polling
    Timer->>Watcher: Trigger poll
    Watcher->>Ports: Read current ports
    alt Port removed
        Watcher-->>Sink: DeviceRemoved
    else Enumeration fails
        Watcher-->>Logger: Record failure
    end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Inject a polling scheduler

• ➕ Eliminates dependence on wall-clock timer scheduling.
• ➕ Allows exact control over every callback and race boundary.
• ➕ Could reduce test runtime and timeout-related CI risk.
• ➖ Requires production-code changes for a test-only coverage PR.
• ➖ Tests a scheduler abstraction instead of the real Timer integration.
• ➖ Adds an architectural seam that may not benefit other consumers.

Recommendation: Keep the current provider-synchronized real-timer approach for this characterization PR. It avoids sleeps, preserves production behavior, and validates the actual Timer integration; introduce a scheduler abstraction only if these tests demonstrate CI instability.

Files changed (1) +459 / -0

Tests (1) +459 / -0

SerialPortPollingDeviceWatcherTests.csAdd deterministic characterization coverage for POSIX USB removal polling +459/-0

Add deterministic characterization coverage for POSIX USB removal polling

• Adds 16 cases covering removal detection, case-insensitive snapshots, enumeration outage recovery, lifecycle idempotency, stale-poll rejection, argument validation, and throwing subscribers. Synchronized port and logger test doubles drive the real timer without sleep-based assertions.

Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. A failed race test strands a worker 🐞 Bug ☼ Reliability

Description

PortTable.Read waits on h.Release without a timeout, while the race test calls release.Set()
only on its success path. If the 10-second assertion or either lifecycle call fails after the
callback claims the held read, watcher disposal does not wait for or cancel that callback, so a
thread-pool worker and the fixture remain retained for the rest of the test process.

Code

Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs[R102-105]

+            if (held is { } h)
+            {
+                h.Entered.Set();
+                h.Release.Wait();

Relevance

●●● Strong

Guaranteed cleanup is needed; otherwise failed assertions can permanently block a worker and retain
test resources.

PR-#256
PR-#247

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The fake provider blocks indefinitely on the release event after claiming the held read, and the
test does not place its sole release call in guaranteed cleanup. Production disposal only disposes
the timer and explicitly does not wait for an in-flight callback, so disposing the harness cannot
unblock this provider call.

Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs[80-106]
Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs[362-367]
Daqifi.Avalonia/Daqifi.Desktop/Services/DeviceWatcher/SerialPortPollingDeviceWatcher.cs[145-157]

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 held provider read is released only on the race test's success path. An assertion or lifecycle failure can therefore leave the timer callback blocked indefinitely even after the watcher is disposed.

## Fix Focus Areas
- Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs[102-106]
- Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs[362-367]

## Recommended Fix
Wrap the race-test operations after `HoldNextRead` in a `try`/`finally` and call `release.Set()` from the `finally` block, making the release idempotent and guaranteed on every failure path.

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

---

Context sources

✅ Cross-repo context — repo relationships

Review mode: ⚖️ Balanced: This is a substantial new runtime-behavior test suite exercising timer concurrency, lifecycle races, exception handling, and failure semantics, so correctness has real risk despite being confined to one file.

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 ↗

[tylerkron]

/agentic_review

[qodo-code-review]

1. A failed race test strands a worker 🐞 Bug ☼ Reliability

PortTable.Read waits on h.Release without a timeout, while the race test calls release.Set()
only on its success path. If the 10-second assertion or either lifecycle call fails after the
callback claims the held read, watcher disposal does not wait for or cancel that callback, so a
thread-pool worker and the fixture remain retained for the rest of the test process.

Agent Prompt

## Issue description
The held provider read is released only on the race test's success path. An assertion or lifecycle failure can therefore leave the timer callback blocked indefinitely even after the watcher is disposed.

## Fix Focus Areas
- Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs[102-106]
- Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs[362-367]

## Recommended Fix
Wrap the race-test operations after `HoldNextRead` in a `try`/`finally` and call `release.Set()` from the `finally` block, making the release idempotent and guaranteed on every failure path.

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

[tylerkron]

Valid. Conceded. The held read is released only on the success path, and disposing the watcher does not wait for an in-flight callback, so a failed assertion there would leave a thread-pool worker blocked for the rest of the run. The failure path only matters when the test is already failing. The code under test is unaffected.

Fixed in 5950ff6 (the test body is wrapped in try/finally, and release.Set() runs in the finally; Set is idempotent). Gate green on that commit: desktop exit 0, iOS Release exit 0, tests 1009/1009. It could not be pushed here: this PR is in the merge queue, and the queue rejects pushes to a queued branch (GH006). The shepherd did not dequeue it. The commit is parked on branch coverage-gap-serial-port-polling-release-fix. Leaving this thread unresolved because the fix is not on this PR's head.

[tylerkron]

#404 merged without this fix. The same commit, cherry-picked unchanged onto main, is now #407. A forced mid-test failure leaves 1 worker blocked without the fix and 0 with it.

[qodo-code-review]

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

[tylerkron]

N-way merge build: green, and it is the tree the merge queue will produce. (Shepherd, 2026-09-19 ~18:35Z. Nothing was merged or pushed to any PR.)

What was merged. main 24ff98b plus all 14 open non-Dependabot PRs (#390–#396, #398–#404), using the heads the queue holds. #402, #403 and #404 had never been built together with anything.

• Checkpoint. main + fix(#327): DevicesMobileView's 65 bindings were all on reflection — one declared scope, one inferred, and nothing constructs the view yet #390–fix(#327): LoggedDataPanePrototype's 102 bindings were all on reflection — root scope, two declared template scopes, two existing item scopes #396 + test(coverage-gap): pin DiskSpaceMonitorCoordinator — the class that decides whether a full disk stops your session (13 cases, 12/12 mutants killed) #398–test: nine private IAppLogger fakes → one RecordingAppLogger (net −240, test code only) #401 gives tree b0f2033, byte-identical to sweep-13's tree. So the test(#327): one dialog-DataContext guard — the shared helper follows new+DataContext, DuplicateDeviceDialog's 412-line copy goes (−369) #387/fix(#327): LoggedSessionsMobileView's 51 bindings were all on reflection — one declared scope, two inferred, three $parent hops #389 content that landed in main matches what the sweep merged, and its 1039/1039 still stands.
• All 14 heads. Tree ae51873, with no conflicts. Merging in forward and in reverse order gives the same tree.
• Re-checked against the newer main. main moved to d88a841 while this ran (fix(#327): DevicesMobileView's 65 bindings were all on reflection — one declared scope, one inferred, and nothing constructs the view yet #390–fix(#327): ChannelsMobileView's 76 bindings were all on reflection — two declared templates, three cast hops, and a drawer label bound to a member IChannel never had #392 merged). That main plus the 11 PRs still queued gives the same tree, ae51873.

Build gate (under the build.1 lock). Success is read from the exit code, not from grepping the output.

• Desktop: exit 0. 166 warnings, 0 errors (all-project total; not a baseline).
• iOS restore: exit 0.
• iOS Release iossimulator-arm64 --no-restore: exit 0. The real build took 2m08s, with 167 warnings and 0 errors.

Tests. dotnet test Daqifi.Avalonia.Tests: 1057/1057 passed. That reconciles exactly: 1039 (tree b0f2033) + 0 (#402) + 2 (#403) + 16 (#404) = 1057. packages.lock.json did not drift.

Hazards checked.

• chore(dead-code): delete 218 unreferenced MaterialColors shades, ConnectionManager.IsDisconnected, DaqifiViewModel.VersionName and a notify for a nonexistent property (+3/−232) #402's deleted members (the fix(layering): a view-model was hand-writing SCPI bytes at the transport #189 removed-member hazard). I grepped the merged tree's .cs and .axaml:
  • IsDisconnected, _isDisconnected, VersionName and _versionName have no remaining reference. The only hit is an unrelated local variable, deviceIsDisconnected, in DaqifiViewModel.
  • The only VersionNumber left is data.VersionNumber, the property on the version-check result, not the removed notify.
  • MaterialColors is referenced only from ChannelColorManager, and only via its 38 …500/…700 constants, all of which survive.
• Top-level types arriving twice. None. refactor(abstraction-police): the connection dialog asks Core for serial port names instead of importing System.IO.Ports (+2/−2 prod) #403 adds ConnectionDialogManualPortCheckTests and test(coverage-gap): pin SerialPortPollingDeviceWatcher — the only USB-unplug detector on macOS/Linux (16 cases, 12/13 mutants killed, 1 documented) #404 adds SerialPortPollingDeviceWatcherTests, and neither name exists anywhere else. test(coverage-gap): pin SerialPortPollingDeviceWatcher — the only USB-unplug detector on macOS/Linux (16 cases, 12/13 mutants killed, 1 documented) #404's PortTable, Harness and RecordingLogger are nested private classes. They do not clash with the same-named nested classes in ChannelCommandDisconnectRaceTests or test(coverage-gap): pin DiskSpaceMonitorCoordinator — the class that decides whether a full disk stops your session (13 cases, 12/12 mutants killed) #398's DiskSpaceMonitorCoordinatorTests, or with test: nine private IAppLogger fakes → one RecordingAppLogger (net −240, test code only) #401's RecordingAppLogger. test(coverage-gap): pin DiskSpaceMonitorCoordinator — the class that decides whether a full disk stops your session (13 cases, 12/12 mutants killed) #398's and test(coverage-gap): pin SerialPortPollingDeviceWatcher — the only USB-unplug detector on macOS/Linux (16 cases, 12/13 mutants killed, 1 documented) #404's private RecordingLoggers are both candidates to fold into RecordingAppLogger later.
• refactor(abstraction-police): the connection dialog asks Core for serial port names instead of importing System.IO.Ports (+2/−2 prod) #403 relies on ConnectionManagerSingletonCollection, which is present in the merged tree.

This build agrees with sweep-13 on the 11 PRs it covered, so they get no new comment.

🤖 Generated with Claude Code

[tylerkron]

Shepherd status: NOT marked ready. One valid Qodo finding is open, and its fix is blocked by the merge queue.

• Qodo round on fd65a8f (the shepherd's own request, 18:22Z; graded Balanced; the summary was updated at 18:24:08Z): 1 Bug (Medium). The Stop/Start race test releases its held read only on the success path. Conceded; see the inline thread. It is test hygiene on a failure path only: the code under test is unaffected, and the passing run is unaffected.
• The fix is 5950ff6 (try/finally around the test body). Gate green locally: desktop exit 0, iOS Release exit 0, 1009/1009. It is not on this PR: the PR is queued (position 10), and GH006 rejects every push to a queued branch. The shepherd did not dequeue. It is parked on branch coverage-gap-serial-port-polling-release-fix. Your call: dequeue so it can land here (this resets CI and needs a Qodo round), or let this merge as-is and take the fix as a one-commit follow-up.
• Body numbers re-derived on the final head:
  • 16 cases holds. It is 14 facts, not 15, plus a 2-row theory; corrected in the body, with no commit.
  • 1009 tests matches CI's log on fd65a8f.
  • All 13 mutations were re-run by the shepherd, one fresh edit each with the file restored and touched afterwards. They reproduce the table exactly: 12 killed, with the same kill counts (1,1,1,1,1,1,1,6,10,6,1,1), and polish(mobile): landscape Profiles layout, channels icon glyph, in-content git SHA #13 survives.
• CI on fd65a8f: all jobs green, but that green certifies fd65a8f, which does not include the fix above.