Repository navigation
fix(self-update): a check that finds nothing must SAY so, and a dead event channel must not freeze an install (#2553, #2494) - #2657
Merged
Conversation
…event channel must not freeze an install (#2553, #2494) Two issues, one defect seen from both ends: a self-update check could complete having reported NOTHING, and the check could stop happening at all without reporting that either. An install three builds behind and an install perfectly up to date produced identical evidence — healthy pods, no errors, an Updates tab reading "No newer version detected yet" — so nobody could tell them apart. #2553 — every check now reports exactly one verdict * RunOnce returns a SelfUpdateVerdict, not Unit. The two paths that used to be bare Rx `Where` clauses — a policy of None, and "no candidate is newer" — were the two most silent paths in the service; they now name their outcome. * DefaultIfEmpty(NoOutcome) is the postcondition asserted rather than hoped for: a pipeline that somehow produces nothing reports THAT, at Warning, naming itself as a defect in this service. * The verdict is both logged AND stamped on Admin/UpdatePolicy (LastCheckedAt / LastCheckVerdict / LastCheckTrigger). Both, because a log line depends on a per-category log level a deployment may simply not have set — which is exactly what happened: every line this service had to say was LogInformation, and the portal image caps its logger prefix at Warning. * The Updates tab stops asserting a fact it never established. "No newer version detected yet" is now said only by an install that actually completed a check; one that has not says so instead. #2494 — a bounded safety net, and the first report that can name a dead channel * SelfUpdate:SafetyNetCheckInterval (default 1h, 0 disables), rendered by the chart. NOT the removed poll: it passes through MinRollInterval like any other check, so it can only shorten DISCOVERY, never change the roll cadence. Events stay the fast path; this bounds how long a broken driver can hide. * A safety-net check that finds a release on an install that has seen NO build-completion event warns and names the chain to check. All three clauses matter: "no events yet" alone is routine and warning on it would train people to ignore the line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes self-update observability and liveness by ensuring every self-update check yields an explicit verdict, persisting that verdict durably, and adding a bounded safety-net interval so a dead build-event channel cannot stall updates indefinitely.
Changes:
- Introduces
SelfUpdateVerdict/SelfUpdateOutcome/SelfUpdateTriggerand updatesSelfUpdateHostedServiceto always emit, log, and record exactly one verdict per check (including “no newer release” and “updates disabled”). - Adds
SafetyNetCheckIntervalto periodically re-check when events never arrive, plus warning diagnostics when the safety net finds a newer release but no build events were ever observed. - Updates UI copy and localization to distinguish “never checked” from “checked and up to date,” and adds/updates tests plus deployment config keys/docs.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/SelfUpdateCheckVerdictTest.cs | Adds integration tests asserting per-check verdict delivery via logs/node stamps and safety-net behavior. |
| test/Memex.Portal.Shared.Test/SelfUpdateVerdictTest.cs | Adds unit tests for verdict factories and key predicates (e.g., FoundNewerRelease). |
| test/Memex.Portal.Shared.Test/ComboVerdictRecordingTest.cs | Updates UI rendering tests to separate “never checked” vs “checked and no newer.” |
| src/MeshWeaver.Messaging.Hub/Localization/strings.en.json | Adds ui.updateNeverChecked string. |
| src/MeshWeaver.Messaging.Hub/Localization/strings.de.json | Adds German translation for ui.updateNeverChecked. |
| src/MeshWeaver.Hosting/SelfUpdate/SelfUpdateOptions.cs | Adds SafetyNetCheckInterval option and documentation. |
| memex/Memex.Portal.Shared/Settings/UpdatePolicySettingsTab.cs | Changes status text logic for empty tag to account for “never checked.” |
| memex/Memex.Portal.Shared/SelfUpdate/UpdatePolicyNodeType.cs | Persists LastCheckedAt / LastCheckVerdict / LastCheckTrigger on policy node. |
| memex/Memex.Portal.Shared/SelfUpdate/SelfUpdateVerdict.cs | Adds verdict/outcome/trigger types and message factories. |
| memex/Memex.Portal.Shared/SelfUpdate/SelfUpdateHostedService.cs | Refactors pipeline to produce verdicts, add safety-net ticks, and record/report outcomes. |
| deploy/helm/templates/memex-portal/config.yaml | Wires SelfUpdate__SafetyNetCheckInterval with a safe default. |
| deploy/aks/values.aks.yaml | Documents and sets default SelfUpdate__SafetyNetCheckInterval. |
| deploy/aks/SELF-UPDATE.md | Updates operational documentation for event-driven checks + safety net + verdict stamping. |
| .github/workflows/main-cd.yml | Updates CD “healed” issue comment to reflect event/safety-net pickup. |
Suppressed comments (1)
memex/Memex.Portal.Shared/SelfUpdate/SelfUpdateHostedService.cs:1
.IgnoreElements().Select(_ => SelfUpdateVerdict.NoOutcome())is intentionally unreachable and is only being used to coerce the observable’s generic type beforeConcat. That makes the pipeline harder to reason about (and invites copy/paste of a no-op mapping). A clearer option is to use an operator expressly for type coercion while emitting nothing (e.g., cast/empty transformation) so the sequencing intent ('run RecordAvailable, then GateThenApply') is obvious without relying on unreachable code.
using Microsoft.Extensions.Configuration;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+179
to
184
| return content.LastCheckedAt is { } checkedAt | ||
| ? localize("ui.updateNoNewerVersion", []) | ||
| + " " + localize("ui.updateCheckedAt", | ||
| [DisplayTimeExtensions.ToDisplayTime(checkedAt, zoneId).ToString("yyyy-MM-dd HH:mm")]) | ||
| : localize("ui.updateNeverChecked", []); | ||
|
|
Comment on lines
+48
to
+55
| /// <summary> | ||
| /// How many build-completion events this process has observed. The ONLY evidence that the event | ||
| /// channel is alive, and it exists solely to make the dead-channel report in | ||
| /// <see cref="ReportCheck"/> possible — nothing else can distinguish "nothing published" from | ||
| /// "something published and nothing told us". Mutable instance state, read with | ||
| /// <see cref="Volatile"/> because the watch and the check run on different pool threads. | ||
| /// </summary> | ||
| private int _buildEventsSeen; |
| BuildCompletionTicks(), | ||
| policy.DistinctUntilChanged(content => content.Policy).Skip(1).Select(_ => -3L))) | ||
| Observable.Return(SelfUpdateTrigger.Startup), | ||
| BuildCompletionTicks().Do(_ => Interlocked.Increment(ref _buildEventsSeen)), |
| # and every joint of it fails SILENTLY. An install whose event channel is dead | ||
| # is indistinguishable from one that is up to date: healthy pods, no errors, a | ||
| # check that simply never runs. memex sat three builds behind for 7 h exactly | ||
| # so. SelfUpdate__SafetyNetCheckInterval (default 1h; empty or 0 disables) |
Same trap the MinRollInterval comment three lines up already documents: the key binds to a TimeSpan, so an empty string is a FormatException in the configuration binder, not "inert". Say 00:00:00. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Test Results (shard 2)931 tests - 26 931 ✅ - 26 4m 50s ⏱️ -19s Results for commit 2827487. ± Comparison against base commit ecef6db. This pull request removes 84 and adds 58 tests. Note that renamed tests count towards both. |
Contributor
Test Results (shard 1)1 065 tests +45 1 065 ✅ +45 6m 44s ⏱️ +19s Results for commit 2827487. ± Comparison against base commit ecef6db. This pull request removes 59 and adds 104 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Contributor
Test Results 42 files ± 0 42 suites ±0 33m 41s ⏱️ - 1m 8s Results for commit 2827487. ± Comparison against base commit ecef6db. This pull request removes 1 and adds 35 tests. Note that renamed tests count towards both. |
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.
Fixes the two halves of the same defect: a self-update check could complete having reported nothing, and the check could stop happening at all without reporting that either. An install three builds behind and an install perfectly up to date produced identical evidence — healthy pods, no errors, an Updates tab reading "No newer version detected yet" — which is why memex sat 7 h and 3 builds behind while every signal said it was fine.
Refs #2553, #2494. Neither issue is closed by this PR: both carry a deployment half that is not mine to do (below).
#2553 — every check reports exactly one verdict
RunOncereturnedIObservable<Unit>, so completing with no emission was a legitimate result. Two paths did exactly that, and they are the two most common outcomes a healthy install has:RunOncenow returns aSelfUpdateVerdict. Every branch names its outcome, so "we asked and the answer was no" is a different observation from "nothing ever asked".framework-mvid.txt/_completeprecedent.DefaultIfEmpty(SelfUpdateVerdict.NoOutcome())means a pipeline that somehow produces nothing reports that, at Warning, naming itself as a defect in this service. A future filter cannot re-create the silence.Admin/UpdatePolicy(LastCheckedAt/LastCheckVerdict/LastCheckTrigger). Both, and the reason is the measured one: every line this service had to say wasLogInformation, and the portal image'sappsettings.jsoncaps its logger prefix atWarningwith no entry for this category (see "Deployment half" below). A log-only fix would have shipped straight back into the same void.StatusMarkdownreturnedui.updateNoNewerVersionwheneverLatestAvailableTagwas empty — which is also the state of an install that has never checked. Those are now two different sentences.The bookkeeping write cannot feed itself: the trigger reads
policy.DistinctUntilChanged(c => c.Policy)and the source isDistinctUntilChanged((Policy, RequireCiGreen)), so a write touching only these three fields emits nothing downstream — the #223 write-storm shape is structurally excluded rather than avoided by convention.#2494 — a bounded safety net, and the first report that can name a dead channel
SelfUpdate:SafetyNetCheckInterval(default01:00:00,00:00:00disables), rendered by the chart and declared invalues.aks.yaml.This is not the removed poll returning. A poll drives the update; this bounds how long a broken driver can hide. It passes through
MinRollIntervallike any other check, so it can only ever shorten discovery — never change the roll cadence. Events stay the fast path and still decide the latency.The justification is the failure mode, not the latency: every event reaches an install across a chain nothing re-verifies — a GitHub webhook registration, a
WebhookInboxallowlist slot, an HMAC secret that must match byte-for-byte — and every joint of it fails silently (an unlisted target answers 404 exactly like a wrong URL; a mismatched secret still answers 2xx and drops the delivery).And the diagnosis half: a safety-net check that finds a release on an install that has seen no build-completion event at all warns and names the chain to check. All three clauses matter — "no events yet" alone is routine on an install whose modules rarely build, and warning on it would train people to ignore the line that means something.
ABuildCompletionDrivenCheck_NeverWarnsAboutTheEventChannelis the control.Tests — run against unfixed code FIRST
SelfUpdateCheckVerdictTest(6 tests) asserts the delivery: a log line that reached a logger, or a field that landed on the node. Never a return value — asserting the thing that already lied is worthless.Against
origin/main(ecef6dbd9) in a clean worktree, with only the two edits needed to compile there (SafetyNetCheckIntervaland theSelfUpdateTriggerenum do not exist yet; the node-stamp test was dropped for the same reason):Five of five time out waiting for a verdict line that pre-fix code never emits — which is the issue, reproduced.
On this branch:
ComboVerdictRecordingTest.StatusMarkdown_NoTagAtAll_SaysNothingDetectedwas replaced, not adjusted: it pinned the claim that has no evidence behind it. It is now two tests, one per fact.🚨 The deployment half — not in this PR, and not mine to do
Memex.Portal.Shared.SelfUpdatehas no log-level entry.MeshWeaver.Plugins/src/Memex.Portal.Distributed/appsettings.jsonsets"Memex": "Warning"and explicitly surfaces the siblingMemex.Portal.Shared.ShippedReleaseSeedHostedServiceat Information — but nothing for this service. That is the direct cause of memex self-update is silent: 3 builds behind, roll floor satisfied 5h ago, and ZERO SelfUpdate log lines in 6.7h #2553's "ZERO SelfUpdate log lines": the mechanism was working and reporting into a void. The node stamp above makes the verdict readable without it; the entry is still worth adding, in that repo.memexlists zero webhook targets (WebhookInbox__Targets__0/1both"", per 🚨 Prod cannot roll: CD's announcement 404s and self-update is EVENT-DRIVEN (no poll) — PLATFORM_WEBHOOK_URL ≠ memex WebhookInbox:Targets #2494's last comment). This PR bounds the damage; it does not fix the channel.🤖 Generated with Claude Code