fix(node): suppress OTA activity while the node is offline - #4168
Merged
Conversation
The update paths guarded only against disposal, so a node that went offline kept collecting update info, downloading images and reacting to OTA status changes until it was disposed. The guard now covers both states: it is raised on goingOffline and lowered again on online, and internal.closed becomes internal.suppressUpdates to say what it actually gates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the node-side OTA provider logic by suppressing SoftwareUpdateManager activity while a ServerNode is offline, resuming when it comes back online, and by clarifying intent via a rename from internal.closed to internal.suppressUpdates.
Changes:
- Add lifecycle handling for
goingOffline/onlineto toggle OTA suppression. - Replace
internal.closedguards withinternal.suppressUpdatesin update paths and status handling. - Tighten one undefined check from
== undefinedto=== undefined.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
suppressUpdates started out lowered, so update work could run between initialize() and the first online transition — the very window the flag exists to cover. It now starts raised and #nodeOnline lowers it. Going offline also left the announcement instance alive. OtaAnnouncements runs its own timers and writes to peers, so it kept announcing while offline regardless of the flag. It is now closed on goingOffline, alongside the update check timer; #nodeOnline already installs a fresh instance. The queue timer is left running: #triggerQueuedUpdate returns immediately while suppressed, and it re-arms itself only when there is queue work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/node/src/behavior/system/software-update/SoftwareUpdateManager.ts:216
#nodeGoingOfflineis a reactor onNodeLifecycle.goingOffline(anAsyncObservableawaited byNode.stop()), but itawaitsannouncements.close().OtaAnnouncements.close()awaits any in-flight announcement promise, which can block the node’s offline transition until network timeouts complete. Sinceclose()already stops timers before its internal await, you can fire-and-forget the close here (with a.catch) to stop future announcements without delaying the lifecycle transition.
// Announcements run on their own timers and write to peers, so the suppression flag alone does not stop them.
// #nodeOnline installs a fresh instance.
await this.internal.announcements?.close();
this.internal.announcements = undefined;
packages/node/src/behavior/system/software-update/SoftwareUpdateManager.ts:212
suppressUpdatesis raised ongoingOffline, andqueryUpdates()now returns early when suppressed, butcheckForUpdates()still unconditionally runs#cleanupObsoleteUpdates()afterward (seecheckForUpdatesaround line ~401). This means storage scans/deletions can still run while the node is offline/suppressed (e.g. ifcheckForUpdates()is invoked directly, or if an in-flight timer callback continues aftercheckForUpdateTimer.stop()). Consider adding an earlyif (this.internal.suppressUpdates) return;guard incheckForUpdates()(and/or at the top of#cleanupObsoleteUpdates()).
async #nodeGoingOffline() {
this.internal.suppressUpdates = true;
this.internal.checkForUpdateTimer?.stop();
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
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.
Minor hardening in
SoftwareUpdateManager.The update paths guarded only against disposal (
internal.closed), so a node that had gone offline but was not yet disposed kept doing update work: collecting per-peer update info, downloading images, walking the consent list, firing queued updates, and reacting to OTA status changes.The guard now covers both states — raised on
lifecycle.goingOffline, lowered again onlifecycle.onlineso a node that comes back resumes normally.internal.closedis renamed tointernal.suppressUpdatessince it no longer means "disposed";[Symbol.asyncDispose]still raises it, so the previous behavior is a subset.Two gaps found in review and fixed on top:
initialize()and the first online transition — the very window it exists to cover. It now starts raised.OtaAnnouncementsinstance alive. It runs its own timers and writes to peers, so it kept announcing while offline regardless of the flag. It is now closed ongoingOfflinealong with the update-check timer;#nodeOnlinealready installs a fresh instance.The queue timer is deliberately left running:
#triggerQueuedUpdatereturns immediately while suppressed and re-arms itself only when there is queue work, so stopping it would gain nothing and could delay a resumed queue.Also tightens one
== undefinedto=== undefined.Checklist
npm testpasses (full suite, all packages)npm run format-verifyandnpm run lintpassnpm run build -- --cleanpassesSoftwareUpdateManagerhas no existing unit-test harness for lifecycle transitionsNo log file attached: this is not an issue-driven fix but a proactive hardening of the offline path.
🤖 Generated with Claude Code