feat(devices): surface Core's background failures in the app log - #809
Merged
Conversation
Contributor
PR Summary by QodoSurface Core background device failures in the app log
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
tylerkron
force-pushed
the
feat/surface-core-background-failures
branch
from
August 3, 2026 17:51
42a2499 to
2c3e35a
Compare
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
49 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
tylerkron
force-pushed
the
feat/surface-core-background-failures
branch
from
August 3, 2026 18:23
94e7f31 to
32f8e63
Compare
Core 1.4.0 made two previously invisible failure classes observable and the desktop subscribed to neither, so they still vanished silently: - IDevice.ErrorOccurred (daqifi-core#378) - faults on a device's read loop, parse, subscriber dispatch, or per-frame stream decode. A stream that could not be read and a stream that could not be decoded both presented to the user as a device that had simply stopped sending. - DaqifiDevice.SendFailed (daqifi-core#413) - sending is fire-and-forget, so a SCPI command could fail to reach the device with no error, no log, and no user-visible signal, leaving the app's idea of device state silently diverging from the device's. Neither Core device the desktop builds is given an ILogger (the serial one is constructed bare, the WiFi one via DeviceConnectionOptions with Logger unset), so Core's own warnings for both went to a null sink. These events are currently the only way either failure can be seen at all. Both are wired in ConnectionManager where ConnectionLost is already attached, and detached at the same two teardown sites. Subscribe/unsubscribe now go through a single mirrored pair of methods so a newly wired event cannot be attached at connect and forgotten at teardown (the leak fixed in #795). Severity: default Warning; Error is reserved for DeviceErrorSource.Unknown. Every source Core actually raises describes the link or the device, not the app - a read that failed because the cable came out, a frame the device garbled, a reconnect that ran out of attempts against a powered-off unit - and routing those to Error would file a Sentry event on every unplug, which is how #775, #779 and #801 buried real bugs. Unknown is unreachable from any Core 1.4.0 path, so one arriving means Core hit a failure it could not classify: expected volume zero, high signal, the same call already made for SerialPortConnectFailure.Unknown in #801. A source value this build does not recognise means the desktop is behind Core, not that the device misbehaved, and stays a Warning. Core's SuppressedCount is reported in the message rather than adding a second throttle on top of Core's. Send-failure logging reports the SCPI verb only. Core's SetNetworkWifiPassword embeds the user's WiFi password in the command payload, and DAQiFiAppLog.log must never contain it. The desktop wrapper re-exposes both Core events on IDevice from a new partial file, forwarding with itself as sender so a log line can name the device the way the user sees it. The Core subscription is attached on the first desktop subscriber and released on the last, and the attached instance is remembered so the release cannot miss it across a reconnect. Observability only - no retry or recovery behavior; auto-reconnect is #804. Closes #805 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo rule violation: a `string` constant named WIFI_PASSWORD read as a stored credential. It never was one - it is a synthetic marker fed to SetNetworkWifiPassword purely so the test can assert those argument bytes never reach the log. Renamed to SENTINEL_COMMAND_ARGUMENT and documented why SecureString does not apply: nothing secret is being protected, and Core's SetNetworkWifiPassword takes a plain string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tylerkron
force-pushed
the
feat/surface-core-background-failures
branch
from
August 3, 2026 21:04
32f8e63 to
8f64b97
Compare
📊 Code Coverage ReportSummarySummary
CoverageDAQiFi - 57.6%
Daqifi.Desktop.Common - 46%
Coverage report generated by ReportGenerator • View full report in build artifacts |
Contributor
Author
Rebased onto main (post-#812), and a bench-status noteRebased after #812 merged, with the unit gate re-run after the rebase rather than before. All three PRs were already reported textually mergeable, but #808 and #810 modify the same file #812 touched, so "no conflict" was not sufficient reason to skip re-testing. Hardware gate: partially blocked, and NOT on anything in these PRsDevice-gated FlaUI run on the chain-1 tip: 19/23. Four failures, none attributable to this work:
The last two are device state, not code. Established by control, not assumption:
Since the identical code passed earlier and fails now, the variable is the bench, not the branch. A Core-level probe on the same port confirms the device is still alive at the transport layer — connects, initializes, firmware 3.7.2, reports This needs a physical power-cycle of the bench device, which I can't perform. I deliberately did not force re-enumeration via Once the device is power-cycled the device-gated suite should be re-run against these branches to close out the gate. Everything else — build, unit gate, Qodo — is green. |
Contributor
Author
Hardware gate resultRun with parallelism disabled (
The extra failure on #809 is not #809
Identical code passing earlier and failing now, plus Status
Everything else on all three: build clean, unit gates green, Qodo clean. |
This was referenced Aug 3, 2026
Contributor
Author
Hardware gate: clearedRe-run on a freshly power-cycled device with The one outstanding question was
All three pass. The full-suite failure is a harness defect, not a branch one: a preceding test leaves its app running ( I was wrong twice on the way to this, and both are worth recording so the next person does not repeat them: I first attributed these failures to an SD-induced device wedge (excluding the SD tests made it worse), and then to device degradation across the session (a fresh device reproduced it identically). Neither held up. Final state, all three PRs
Every remaining device-gated failure is accounted for: the two SD imports are the known firmware SD-heap defect (firmware#703) on files dated June/July, and #809's third is the leaked-app harness issue above, disproved by isolated re-run. |
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
2 participants
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.
Closes #805
Core 1.4.0 made two previously invisible failure classes observable, and the desktop subscribed to neither — so they still vanished silently. This subscribes to both and routes them to
AppLoggerwith a severity chosen so they do not become Sentry noise.Observability only. No retry or recovery behavior — auto-reconnect is #804.
What was silent
IDevice.ErrorOccurred(daqifi-core#378) — faults on a device's read loop, parse, subscriber dispatch, or per-frame stream decode. A stream that could not be read and a stream that could not be decoded both presented to the user as a device that had simply stopped sending.DaqifiDevice.SendFailed(daqifi-core#413) — sending is fire-and-forget, so a SCPI command could fail to reach the device with no error, no log, and no user-visible signal, leaving the app's idea of device state silently diverging from the device's.Existing silent-failure symptom this would have surfaced: Core does log both internally (
_logger.LogWarning), but neither Core device the desktop builds is given anILogger—SerialStreamingDevice.CreateCoreDeviceconstructsnew CoreStreamingDevice(name, transport)bare, and the WiFi path buildsDeviceConnectionOptionswithout settingLogger. Both therefore went to Core's null sink. So today these events are the only way either failure can be observed at all, and any past report of "the device ignored my command" (e.g. #589's neighbourhood) had no evidence trail whatsoever. Wiring Core'sILoggerinto the device options would be a reasonable follow-up, but it is out of scope here.Severity mapping and reasoning
Default Warning. Error is reserved for
DeviceErrorSource.Unknown.DeviceErrorSourceMessageConsumerConnectionStatus.Lost— so Error here would file a Sentry event on every unplug.StreamDecodeReconnectConnectionLostteardown and its dialog.Unknownv1.4.0: onlyMessageConsumer,StreamDecode,Reconnectare ever passed). One arriving means Core hit a failure it could not classify — expected volume zero, high signal. This is deliberately the same call already made forSerialPortConnectFailure.Unknownin #801.SendFailedis always Warning, timeout or not: a write fails because the port closed, the device went away, or the device stopped draining its receive buffer (IsTimeout). All three are conditions of the link. The timeout/hard-failure distinction is still written into the message, because "busy device" and "gone device" are diagnosed differently.Known under-reporting, accepted deliberately. Core folds "an exception thrown while dispatching a parsed message to a subscriber" into
MessageConsumer. That subcase is an app bug (a desktop handler threw). Core does not separate it, and the only way to split it here would be an exception-type allow-list — which is precisely the mechanism that went stale three times (#775, #779, #801). It is still written toDAQiFiAppLog.logwith its stack trace; escalating it needs a Core-side source split, not a brittle type check here.Throttling is Core's.
SuppressedCountis reported in the message ("N further like failure(s) suppressed by Core's throttle") rather than adding a second throttle on top.What
MessageSendFailedEventArgs<string>actually exposesRead at
v1.4.0:src/Daqifi.Core/Communication/Producers/MessageSendFailedEventArgs.cs:IOutboundMessage<string> Message— the message whose write failed;Message.Datais the SCPI command stringException Error— the exception the write threwbool IsTimeout— precomputederror is TimeoutExceptionDateTime Timestamp— UTC, set at constructionNo
SuppressedCountand no throttle on this one; the producer raises per failed message.Security note:
Message.Datacannot be logged verbatim.ScpiMessageProducer.SetNetworkWifiPasswordproducesSYSTem:COMMunicate:LAN:PASs "<password>", so a failed send of that command would put the user's plaintext WiFi password intoDAQiFiAppLog.log. Only the SCPI verb (everything before the first space, capped at 64 chars) is logged, which is what a diagnosis actually needs. There is a test asserting the password does not appear in the log message.Wiring and the leak
Both events are attached in
ConnectionManager.Connectat the same pointConnectionLostis, and detached at the same two teardown sites (Disconnect,Reboot). Those three call sites now go through a mirroredSubscribeDeviceEvents/UnsubscribeDeviceEventspair, so a newly wired event cannot be attached at connect and forgotten at teardown — the shape fixed in #795.AbstractStreamingDevice.csis owned by a concurrent PR, so the wrapper's re-exposure lives in a new partial file,AbstractStreamingDevice.Diagnostics.cs. It forwards withthisas the sender so a log line can name the device the way the user sees it (DeviceDisplayName) rather than the Core-internal object. The Core subscription is attached on the first desktop subscriber and released on the last, and the attached Core instance is remembered separately fromCoreDeviceso the release cannot miss it if a reconnect replaced it.Tests
18 new tests; suite goes 874 → 892 passing, 0 failing.
dotnet build -tl:onis clean with zero new warnings.IAppLogger— every case checks bothError(Exception, string)andError(string), because both reach Sentry (CaptureExceptionand the synthesizedAppLogErrorExceptionrespectively). "Did not throw" is true of every arm and is exactly how bug: SCPI-init connect-classification arm is dead code against Core 1.3.0 (typed ScpiInitializationErrorException no longer derives from InvalidOperationException) #775/bug: SD-download stall watchdog is dead code over USB serial — Core's plain TimeoutException hits the classifier's default (Sentry) arm #779/SerialStreamingDevice.LogConnectFailure will route routine connect failures to Sentry once Core's SerialPortConnectException lands #801 stayed green.IsAppBug_ClassifiesExactlyTheSourcesCoreDeclares— a tripwire that fails by name if a Core bump adds or removes aDeviceErrorSource, instead of letting a new source silently inherit the default arm.DisconnectandReboot: raise both events after teardown and assert nothing at all was logged.Connect_SubscribesToBackgroundFailureEventsdrives the realConnectpath so the wiring point itself is covered.IStreamingDevicecannot prove that half), including that one of two subscribers leaving does not silence the other.ConnectionManagergained an internal test constructor taking anIAppLogger; the existingAppLogger.Instance.Xcalls in that file becameAppLogger.Xagainst the new injected property. That is required for the level assertions — the singleton is process-wide and MSTest parallelizes test classes, so a shared sink would collect other classes' logging.Not bench-tested
Per instruction, no hardware was touched — no COM port opened, no app launched, no UI harness run. The "unplug mid-command" bench check from the issue's verification list is still outstanding.
Rebase note
Stacked on #807. #806 already merged; this branch is rebased onto it.
When #807 squash-merges, this branch will go CONFLICTING because it carries #807's pre-squash commits. Do not merge
mainin and resolve by hand — rebase off the parent instead:where
bda8634is the tip ofchore/delete-wmi-unplug-watcherthis branch is currently cut from.🤖 Generated with Claude Code