fix(monitoring): harden ONT/CM/Cellular Disable/Enable toggle (review follow-ups) - #1048
Draft
Optic00 wants to merge 2 commits into
Draft
fix(monitoring): harden ONT/CM/Cellular Disable/Enable toggle (review follow-ups)#1048Optic00 wants to merge 2 commits into
Optic00 wants to merge 2 commits into
Conversation
Optic00
marked this pull request as draft
July 22, 2026 17:43
Contributor
Author
|
Moving this to draft / on hold — no rush, syncing with what's already in
Plan for the next dev cycle: rebase onto fresh |
Optic00
force-pushed
the
feature/monitoring-toggle-hardening
branch
from
July 25, 2026 15:21
1251b77 to
a9561c7
Compare
… follow-ups) Pattern-wide robustness fixes from the adversarial review of the toggle, applied across all three device types. Ozark-Connect#1 Atomic anti-resurrection guard: UpdateXxxPollResultAsync now performs a single conditional ExecuteUpdate WHERE Id == id && Enabled instead of a read-then-SaveChanges, closing the TOCTOU where a Disable committing between the read and the write could still overwrite a paused row's frozen state. Repo tests moved from the EF InMemory provider to SQLite in-memory (InMemory cannot execute ExecuteUpdate) + a relational mid-poll-disable regression per type. Ozark-Connect#2 Disabled devices are no longer contacted or falsely reported on the service paths: PollCmAsync / PollModemAsync / the ONT manual poll short-circuit when the config is disabled (Cellular returns a clear 'disabled' message instead of a false 'polled successfully'), and the Cellular card skips auto-polling a disabled config on initial load and when paging between modems. The dashboard-card side of this (Refresh button state, server-side refresh guard, timer config reload) is already covered by 618c2e9 and is not duplicated here. The non-persisting Test/Probe buttons are unchanged. Ozark-Connect#3 Removed the redundant 'Enable polling' checkbox from the ONT/CM/Cellular edit forms (the row Disable/Enable toggle is the single control) and stopped SaveXxxConfigurationAsync from copying Enabled on update, so a stale open edit form can no longer resurrect a row the toggle just disabled. Starlink untouched. Ozark-Connect#4 A persistence exception is no longer swallowed into the same 'false' the guard uses for a disabled row: the poll success helpers rethrow, so a real DB write failure on an enabled device surfaces via the error path instead of silently dropping valid stats. Both call sites already run inside a catch that records the failure on the config, so the poll loops stay alive.
Starlink got its own Disable/Enable row toggle after the ONT/CM/Cellular hardening was written, and it shipped with the same four gaps this branch already closed for the other three device types. Ozark-Connect#1 UpdateStarlinkPollResultAsync is now a single conditional ExecuteUpdate WHERE Id == id && Enabled instead of a read-then-SaveChanges, so a Disable committing between the read and the write can no longer let an in-flight poll overwrite the paused terminal's frozen state. StarlinkRepositoryTests moved to SQLite in-memory (the EF InMemory provider cannot execute ExecuteUpdate) plus a relational mid-poll-disable regression. Ozark-Connect#2 PollStarlinkAsync short-circuits on a disabled config, so the manual Refresh path no longer reaches the dish of a paused terminal. The stats panel already guards this client-side; this is the server-side half. Ozark-Connect#3 Removed the 'Enable polling' checkbox from the Starlink edit form (the row toggle is the single control) and stopped SaveStarlinkConfigurationAsync from copying Enabled on update, so a stale open edit form can no longer resurrect a terminal the toggle just disabled. SetStarlinkEnabledAsync is the sole writer. Ozark-Connect#4 UpdateConfigSuccessAsync rethrows a persistence failure instead of returning the same false the guard uses for a disabled row, so a real DB write failure surfaces via the poll error path. The call site already runs inside a catch that records the failure, so the poll loop stays alive.
Optic00
force-pushed
the
feature/monitoring-toggle-hardening
branch
from
July 27, 2026 17:26
a9561c7 to
2dda7b0
Compare
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.
Pattern-wide robustness follow-ups to the device-monitoring Disable/Enable toggle (#1044 ONT, #1046 CM, #1047 Cellular), applied across all three device types. From an adversarial review of the toggle; two of these (#1, #4) were flagged as good catches.
#1 - Make the anti-resurrection guard atomic
UpdateXxxPollResultAsyncreadEnabledand then calledSaveChangesseparately - a TOCTOU where aDisablecommitting in between let an in-flight poll still overwrite the paused row's frozenLastError/LastPolled. It now does a single conditionalExecuteUpdate(...).Where(x => x.Id == id && x.Enabled)(returnsrows > 0), so the check and the write are one atomic statement. Repo tests moved from the EF InMemory provider to SQLite in-memory (InMemory can't executeExecuteUpdate), plus a relational mid-poll-disable regression per type.#2 - Don't contact / falsely report a disabled device on the manual + dashboard paths
The public poll entry points fetched the device before the guard ran, so refreshing a disabled device still hit the network - and Cellular returned
(true, "polled successfully")while nothing persisted.PollCmAsync,PollModemAsync, and the ONT manual poll now short-circuit when the config is disabled (Cellular returns a clear "modem is disabled" message), and the CM/Cellular/ONT stats panels skip auto-polling a disabled config. The non-persisting Test/Probe buttons are unchanged.#3 - One control for enable/disable (UX change)
Each edit form still had its own "Enable polling" checkbox, competing with the new row toggle - open the edit form, toggle the row off, click Update, and the stale form value re-enabled it. Removed that checkbox from the ONT/CM/Cellular edit forms (the row Disable/Enable button is now the single control) and stopped
SaveXxxConfigurationAsyncfrom writingEnabledon update, soSetXxxEnabledAsyncis the sole writer. Starlink's edit form is untouched (it has no toggle). New tests assert Save preserves the DBEnabledvalue.#4 - A persistence exception is no longer mistaken for "disabled"
The CM/Cellular poll success helpers caught every repository exception and returned
false- the same value the guard uses for a disabled row - so a transient DB write failure on an enabled device silently dropped valid stats (and Cellular reported success). They now rethrow, so a real failure surfaces via the poll error path;falseonly ever means "disabled" (matching how ONT already behaved).Tests
NetworkOptimizer.Storage.Tests183/183 andNetworkOptimizer.Web.Tests933/933 green; solution builds clean (0 warnings).Off
devafter #1044/#1046/#1047 merged.