Repository navigation
fix(logging): a device that refuses a logging command ended the app — the disk-space hard stop, and the mode switch with a mixed USB/WiFi fleet - #218
Merged
Conversation
Both paths in #214 reproduce, and they are one root cause: every logging command is issued from a UI property setter in a bare loop, and a throw out of a property setter reaches Dispatcher.UIThread.UnhandledException, which App.OnDispatcherUnhandledException only logs — it never sets Handled, so the process dies mid-session. The fan-out now runs through LoggingFleet, which commands every device, collects the refusals, and lets the caller decide what they mean: - start: the devices that did start keep the session; the refusals are reported. Only when EVERY device refuses is the session unwound, because only then is nothing anywhere recording. - stop: the local session always ends; a device that will not confirm is reported, not obeyed. - mode switch: still all-or-nothing with the rollback it already had, but reported instead of rethrown. Also fixes the latent fifth failure the issue records: both loops walked the live ConnectedDevices collection, so a command that surfaced as a lost connection ended the enumeration with "Collection was modified". closes #214 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
/agentic_review |
PR Summary by QodoPrevent device logging refusals from terminating the app
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…d rollback must be said Qodo round 1, both conceded and both reproduced as failing tests first. 1. Failed SD stop stays active (High). AbstractStreamingDevice.StopSdCardLogging clears IsLoggingToSdCard only after the device answers, and the IsLogging getter ORs _isLogging with that flag — so a refused stop reported success and the toggle sprang straight back ON. The user turns logging off, is told a device did not confirm, and watches the switch refuse to move: exactly the trap this change exists to avoid. A refused command leaves a STALE flag, not evidence, so the app stops asserting that device's SD state until it learns something new. Cleared on the next start and when the device disconnects. 2. Failed rollback reported successful (Medium). If restoring a device's original mode threw, SwitchMode logged it and still reported the fleet restored — while the device stayed in the abandoned mode. LoggingFleet.Start picks each device's command from its actual Mode, so that device would log to its SD card on the next start while the app showed "Stream to App". Stranded devices are now collected and named, and the rollback carries on past one failure rather than abandoning the devices behind it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 821ba21 |
Contributor
Author
|
After state, rendered from the real app (see the PR body's What the user sees now).
|
…ways Qodo round 2, both conceded, both reproduced as failing tests first. Both are defects in round 1's own fix, not in the original crash. 1. Failed retry reactivates toggle. The doubt left by a refused stop was cleared at the TOP of the next start, before any device had been asked anything — so a retry the device also refuses reinstated the stale IsLoggingToSdCard, the getter reported logging active, and the toggle sprang back ON even as the all-refused unwind set _isLogging false. The bookkeeping now runs AFTER the fan-out and is driven by what each device answered: accepted clears the doubt, refused keeps it. 2. Disconnect leaves stale suppression. A stop can fail BY disconnecting, which removes the wrapper from ConnectedDevices mid-fan-out — so the collection- changed cleanup had already run by the time the refusal was recorded, and the entry could never be cleaned up by anything. It did nothing while the device was gone and would have suppressed that same wrapper's freshly reported state if it reconnected. ForgetSdState now records only still-connected devices. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 076e27c |
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not merging — for review.
closes #214
What the user saw
The application exited while they were logging. Not a wrong value, not a blank pane — the process ended, and everything not yet written to the database for that session was lost.
Two ways in, both reproduced here against
origin/main@cd44a11:DiskSpaceMonitorCoordinator.OnCriticalSpaceReachedcalls the host'sStopLogging(), and that writesIsLogging = false. If any connected device's transport has gone, the app closes. The user's only warning about the full disk was the dialog that never got shown.DevicesPanePrototype.axaml:625enables the radio from the selected device'sIsUsb, but the setter applies the mode to every connected device, andSwitchModerefuses SD logging on a WiFi one. The app closes.Root cause — one, not two
Both are the same defect: every logging command is issued from a UI property setter, in a loop with no failure handling. A throw out of a property setter reaches
Dispatcher.UIThread.UnhandledException, andApp.OnDispatcherUnhandledException(App.cs:351) only logs it — it never setse.Handled— so the process dies. Exactly the ground #183 covered for the session row, one loop further down;LoggingManager.cs:188-197already carries the note explaining why this class of throw is fatal here.The escaping frames, measured, not inferred:
Five device calls reach the dispatcher this way:
InitializeStreaming,StopStreaming,StartSdCardLogging,StopSdCardLogging, andSwitchMode. In the mode-switch case the rollback has already put every device back before thethrow;— so the rethrow's only remaining effect was to close the app.A sixth, latent, on the same lines: both loops enumerated the live
ObservableCollection<IStreamingDevice>. A command that surfaces as a lost connection tears its own device out of that collection throughConnectionManager.Disconnect, ending the enumeration withInvalidOperationException: Collection was modified— the same process death wearing a message about nothing. That reproduces too (see the test table).Not a
try/catcharound the crash. The question a refusal actually asks is what it means for the rest of the fleet, and that decision now lives in a named place.The fix
New
Daqifi.Desktop.Device.LoggingFleetissues one command to every device, on a snapshot of the collection, and returns which devices accepted it and which refused and why. The two setters then decide:SelectedLoggingModeis one value for the whole app and the UI cannot show half a fleet in each mode. What is new is surviving it: reported instead of rethrown, and the radio group is re-notified so it stops showing a mode nothing is in.The mixed USB/WiFi fleet specifically: clicking LOG TO DEVICE with a WiFi device connected now leaves every device exactly where it was — streaming to the app — and tells the user which device blocked the switch and that SD logging needs USB. Nothing is left half-switched, and no device's session is interrupted. Whether the mode ought to be per-device instead is a design question, not a crash, and the issue explicitly scopes it out.
Refusal messages are Core's own (
"Device is not connected.","No SD card is installed in the device."), prefixed with the device'sDeviceDisplayName, rather than a second copy of Core's taxonomy maintained here.Reporting reuses the seam #183 built (
ReportLoggingCouldNotStart), generalised toReportLoggingProblem(title, message, isDefect). Device refusals log at Warning, not Error, so they do not file Sentry issues — a device declining a command is an environmental condition the app cannot prevent, the same ruleSdCardFailureClassifieralready applies. The #183 database path keeps its Error level.What review added
Four more behaviours, all found by Qodo across two rounds, all real, and each pinned by a test that fails without it. Two were defects in the original crash fix; two were defects in the fix for those.
AbstractStreamingDevice.StopSdCardLoggingclearsIsLoggingToSdCardonly after the device answers, and theIsLogginggetter ORs_isLoggingwith that flag — so a refused stop reported success and the switch sprang straight back ON, which is the exact trap this change exists to avoid. A refused command leaves a stale flag, not evidence: the app stops asserting that device's SD state until it learns something new. The device-state fallback itself is kept — it is what makes the toggle tell the truth after reconnecting to a device that kept SD-logging across a desktop restart.LoggingFleet.Startpicks each device's command from its actualMode— so on the next start it would log to its SD card while the app showed "Stream to App", producing a run that never reaches the desktop and that the user has no reason to look for on the card. Stranded devices are now named, and the rollback carries on past one failure instead of abandoning the devices behind it.IsLoggingToSdCardagain and the toggle sprang back ON, even as the all-refused unwind set_isLoggingfalse. The bookkeeping now runs after the fan-out and is driven by what each device answered: accepted clears the doubt, refused keeps it.ConnectedDeviceswhile the command is in flight — so the collection-changed cleanup had already run by the time the refusal was recorded, and the entry could never be cleaned up by anything. Only still-connected devices are recorded now, which also closes the leak.Does Core already own this?
No — checked against the pinned 1.7.0 package (
~/.nuget/packages/daqifi.core/1.7.0), not the sibling checkout'smain. Core 1.7.0's device commands are throw-based (DeviceNotConnectedException,SdCardNotPresentException,SdCardBusyException,FeatureNotSupportedException,DeviceCommandFailedException) with no result-typed start/stop, andDaqifiDeviceRegistryis a discovery/registration registry, not a logging-fleet orchestrator. There is no fleet command fan-out in Core at all, so "what happens to the rest of the fleet when one device refuses" is genuinely this app's decision. No duplicated logic.Evidence: the tests fail on main
Daqifi.Avalonia.Tests/ViewModels/DeviceRefusalCrashTests.cs, 16 tests. The 9 crash tests were run against unmodifiedDaqifiViewModel.csfromorigin/mainwith the tests in place: 7 fail, 2 pass. Every one of the 7 fails by throwing out of the setter — the crash itself, not a changed assertion:mainStoppingLogging_WhenTheDeviceRefuses_DoesNotEndTheAppDeviceNotConnectedExceptionStoppingLogging_WhenOneDeviceRefuses_StillStopsTheRestOfTheFleetDeviceNotConnectedExceptionStartingLogging_WhenOneDeviceRefuses_KeepsTheSessionForTheRestDeviceNotConnectedExceptionStartingLogging_WhenEveryDeviceRefuses_PutsTheToggleBackAndSaysSoDeviceNotConnectedExceptionStoppingLogging_WhenADeviceDisconnectsItselfMidCommand_DoesNotEndTheAppInvalidOperationException: Collection was modifiedSwitchingToLogToDevice_WithAWifiDeviceInTheFleet_DoesNotEndTheAppInvalidOperationException: SD Card logging is only available when connected via USBStartingSdCardLogging_WhenTheCardIsMissing_KeepsTheAppAndNamesTheReasonSdCardNotPresentExceptionSwitchingToLogToDevice_WithAnAllUsbFleet_MovesTheWholeFleetStartingLogging_WithNoDeviceConnected_LeavesTheToggleOnThe 7 review tests were checked the same way — against this branch with only the production file(s) for that round stashed. All four review findings are pinned by a test that fails without its fix:
StoppingLogging_WhenAnSdCardStopIsRefused_StillLeavesTheToggleOffSwitchingMode_WhenTheRollbackAlsoFails_NamesTheDeviceItCouldNotPutBackRetryingAfterARefusedSdStop_WhenTheRetryIsRefusedToo_LeavesTheToggleOffAStopThatFailsByDisconnecting_DoesNotStaySuppressedIfTheDeviceComesBackTheir three guards (
ADeviceThatIsSdLoggingOnItsOwn_StillReportsTheSessionAsActive,SwitchingMode_WhenTheRollbackSucceeds_SaysNothingAboutStrandedDevices,RetryingAfterARefusedSdStop_WhenTheRetrySucceeds_ReportsTheSessionActiveAgain) pass either way — they exist so the fixes cannot quietly over-reach.All 16 pass on this branch.
The devices that refuse in these tests are real
AbstractStreamingDevices with no Core device behind them — the state a wrapper is in the moment a link drops — so the exceptions come from the production frames named above, not from a mock. Only the devices that are meant to succeed are stubs (RecordingStreamingDevice), because succeeding is the part a test has to fake: all four logging commands onAbstractStreamingDeviceare non-virtual and all four go throughGetConnectedCoreDevice, so a double either refuses everything or needs a live transport.The test class calls
App.InitializeMobile()— theIsLoggingsetter writesLoggingManager.Instanceon its first line, and that resolves its context factory offApp.ServiceProvider. That makes it a SQLite user, so it sits in aDisableParallelizationcollection to stay out of the suite's existingClearAllPoolsrace rather than adding to it. (Worth noting for future work:DaqifiViewModelis constructible in a test host, which the note inDeleteAllSessionsRecoveryTestssays it is not. That note is stale.)Bench — real hardware
Real Nq1 on
/dev/cu.usbmodem1101(serial9090539562006014104, firmware3.7.2), driven through the app's ownSerialStreamingDeviceandLoggingFleet, with a mixed fleet — and the refusing device placed FIRST, so onmainthe real board would never have been reached at all:That is the mixed-fleet claim confirmed on hardware. Non-destructive throughout — connect, start streaming, stop streaming, disconnect. No firmware or WiFi flash, no SD format or delete, no reboot, no live
SD:GET. The refusal was provoked in software (a wrapper with no Core device), never by putting the board in a state it could not come back from. The throwaway harness was deleted afterwards.What the user sees now
The standing notification, rendered by booting the real app headless through
tools/parity-audit/run.shand driving the actual refusal path — this is the app's own output, not a mock-up. The app is still running, the bell is badged, the toggle went back OFF, and the plot correctly reads "No channels streaming" instead of LOGGING ON over nothing:The other new surface is a
MessageBoxdialog carrying the same sentence. It is not pictured: the capture harness constructs no dialogs, which is issue #213. The harness edit used to drive the notification path was reverted.Gate
Measured in this worktree, resolved DLL path checked (
.../agent-a82aad3313ae6754f/Daqifi.Avalonia.Tests/bin/Debug/net10.0/):Release/iossimulator-arm64✅ · Android not built — there is no Android head on this Mac, so no Android status is claimed; CI covers itdotnet test402/402 (baseline onorigin/mainmeasured at 386 in this worktree, +16 new)diffafter normalising line numbers,third_party/excluded.LoggingFleet.cscontributes none.packages.lock.jsondrift.+1257 / −51across 4 files (2 production, 2 test).Pre-existing, not mine:
DeleteAllSessionsRecoveryTests.DeleteAll_RemovesTheRollbackJournalTheOldPurgeLeftBehindfailed once on theorigin/mainbaseline run withObjectDisposedException: SQLitePCL.sqlite3and passed in isolation — a cross-classSqliteConnection.ClearAllPools()race that predates this branch.Scope
DiskSpaceMonitor.csuntouched — fix(disk-space): the logging gate measured the root filesystem, not the disk being written to #217 owns it (which volume it measures) and DiskSpaceMonitor: IsMonitoring lies after the critical hard stop, and StartMonitoring after Dispose leaks an undisposable timer #208 owns itsIsMonitoring/timer. This crash runs through the disk-space hard stop, but the defect is in the setter the coordinator calls, not in the monitor.gh pr diff 217was read first to confirm no overlap.AbstractStreamingDeviceuntouched: its throws are correct, and Core's typed exceptions are the useful signal. What changed is who catches them.