Repository navigation
fix(#1778): a floor between automatic rolls, since a roll is a pod restart - #1780
Merged
Merged
Conversation
…start Closes #1778. This is my regression from #1773: that PR renamed SelfUpdateOptions.PollInterval and removed the recurring poll, which left `SelfUpdate__PollInterval: "1.00:00:00"` in deploy/aks/values.aks.yaml binding to nothing — a deliberate one-roll-per-day production throttle that still READ like a live throttle while being inert. I grepped .cs for the renamed option and grepped the charts only for the option I deleted; a rename is exactly as breaking as a deletion for a `__`-bound key. Without pacing, publication frequency is portal-RESTART frequency, one for one, and each restart drops the live circuits of everyone on the portal. The floor: * SelfUpdateOptions.MinRollInterval, bound from SelfUpdate:* like the rest, DEFAULT 1 HOUR; * checked at the roll decision — inside the floor, log and defer; * a deferred roll is re-decided by the next publication event, exactly like an availability hold, so the floor introduces NO timer. 🚨 Restart-surviving state, because a successful roll RESTARTS THE PROCESS: an in-memory "last rolled at" is empty exactly when the floor needs it. IDeploymentUpdater grows LastRolledAtAsync, answered in MeshWeaver.SelfUpdate.Aks by an annotation the patch itself stamps on the Deployment, and "never" by the platform's DetectOnly fallback (an install with no patcher cannot roll anyway). 🚨 NOT process uptime, which is the tempting third option and is wrong for CRASH RECOVERY: a pod that comes back on an OLD image has a young process but an old deployment, and uptime would make it wait out a floor it satisfied hours ago — precisely when it most needs the newer image. The annotation gives the right answer: old stamp, floor elapsed, roll at once. AnOldStampRollsImmediately_TheCrashRecoveryCase pins it. 🚨 The value is 1h, NOT the 24h prod had. Restoring the prior number is not the neutral default it looks like: with hourly publication, a daily floor means a merged fix reaches prod the NEXT DAY — strictly worse than the 3 h window the maintainer rejected, so "restore prior behaviour" silently answers their complaint with "no". Half the 2026-08-10 rationale for 24h is also obsolete: adopt-before-compile took a prod roll from 80 compiles / 64.8 s to 0 compiles / 32.1 s, leaving the restart cost as the only live argument, which is what this bounds. values.aks.yaml carries the 24h/6h/1h trade in prose so the number can be changed without re-deriving it, and the floor paces only the AUTOMATIC cadence — `kubectl rollout restart` and a main-cd dispatch both still apply an urgent fix immediately. Timing, verified rather than assumed: prod runs 3.0.0-rc4.ci.4147 (commit 324e61a), an ANCESTOR of #1773 — so the portals still carry the old poller and are still on the 24 h throttle. The unpaced behaviour is not live yet; it begins with the first post-#1773 image prod rolls onto, which is the deadline this wants to be inside. Verified: whole solution Release -warnaserror 0 Error(s); 18/18 self-update tests, including four new ones for the floor (fresh roll defers, old stamp rolls immediately, never-rolled is not held, zero floor opts out). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjujCq1BsJkNKuZ4wXodo1
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a restart-pacing “floor” to self-update so event-driven publication checks don’t translate into unbounded pod restarts (and circuit drops), using restart-surviving state (a Deployment annotation) to handle crash-recovery correctly.
Changes:
- Introduces
SelfUpdateOptions.MinRollIntervaland enforces it inSelfUpdateHostedServicebefore applying an update. - Extends
IDeploymentUpdaterwithLastRolledAtAsyncand implements it for AKS via a Deployment annotation stamped during patch. - Updates AKS Helm values to replace the inert
SelfUpdate__PollIntervalwithSelfUpdate__MinRollInterval, and adds/adjusts monolith tests for the new floor behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/SelfUpdateRollFloorTest.cs | New tests pin floor semantics (fresh roll defers, crash-recovery rolls immediately, never-rolled not held, zero floor opts out). |
| test/MeshWeaver.Hosting.Monolith.Test/SelfUpdatePollerResilienceTest.cs | Updates test fake to implement new IDeploymentUpdater.LastRolledAtAsync (currently with a doc-comment formatting bug). |
| test/MeshWeaver.Hosting.Monolith.Test/SelfUpdateAvailabilityGateTest.cs | Updates test fake to implement new IDeploymentUpdater.LastRolledAtAsync (currently with a doc-comment formatting bug). |
| src/MeshWeaver.SelfUpdate.Aks/KubernetesDeploymentUpdater.cs | Reads last-roll stamp from Deployment annotation and stamps the annotation during patch. |
| src/MeshWeaver.Hosting/SelfUpdate/SelfUpdateOptions.cs | Adds the MinRollInterval option with detailed rationale in docs. |
| src/MeshWeaver.Hosting/SelfUpdate/IDeploymentUpdater.cs | Adds LastRolledAtAsync to support restart-surviving floor state. |
| memex/Memex.Portal.Shared/SelfUpdate/UnavailableUpdateMechanics.cs | Detect-only updater reports “never rolled” for floor purposes. |
| memex/Memex.Portal.Shared/SelfUpdate/SelfUpdateHostedService.cs | Enforces the floor at apply time via LastRolledAtAsync before patching. |
| deploy/aks/values.aks.yaml | Replaces inert poll interval key with SelfUpdate__MinRollInterval and documents the tradeoffs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Test Results (shard 1)2 247 tests +84 2 144 ✅ +84 8m 39s ⏱️ +32s Results for commit 608d438. ± Comparison against base commit 674ae64. This pull request removes 65 and adds 149 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 2)2 451 tests - 66 2 447 ✅ - 66 7m 11s ⏱️ -14s Results for commit 608d438. ± Comparison against base commit 674ae64. This pull request removes 135 and adds 69 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
… when the floor is off
Four findings, all real:
* The test helper waited a fixed 3 s even where a roll was EXPECTED — the exact "never Task.Delay
to wait for propagation" rule this repo states, broken by me. Split in two: ExpectRollAsync
waits on the Patched stream with a timeout, and only ExpectNoRollAsync keeps a bounded delay,
which is the sanctioned "confirm nothing happened" shape because there is no positive signal to
wait for. Suite time 15 s -> 7 s, and the roll cases are no longer timing-sensitive under load.
* A zero MinRollInterval no longer calls LastRolledAtAsync at all: that is a Kubernetes GET in the
AKS implementation whose answer cannot change the decision when the floor is disabled, so it was
an API call and a failure point on the happy path for nothing.
* The doc comments I added to the two IDeploymentUpdater fakes had stray blank lines inside them,
detaching them from the member they document (CS1587 territory). Made contiguous.
Verified: whole solution Release -warnaserror 0 Error(s); 18/18 self-update tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjujCq1BsJkNKuZ4wXodo1
Contributor
Author
|
All four fixed in 608d438 — every one was substantive, and the The fixed delay was me breaking a rule I have been citing all day: this repo's guidance is to wait on the condition, never the clock. Split in two — The zero-floor stamp read is gone: The doc comments did have stray blank lines inside them, detaching them from the member. The build tolerated it, so thank you for catching what Whole solution |
rbuergi
added a commit
that referenced
this pull request
Aug 17, 2026
#1780 shipped SelfUpdate__MinRollInterval while this branch was open, so the paragraph telling the reader to wait for a floor before tightening the tick described a world that no longer exists. Rewritten to state the invariant that now holds — the tick and the floor are matched at 1h and must move together, in that order — and to keep the renamed-key trap, which is the part that cost real time. The values.aks.yaml conflict resolved in main's favour: it replaces the dead key with the real one plus the 24h/6h/1h trade, which subsumes this branch's prose about deleting it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 21, 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.
Closes #1778. This is my regression from #1773 — I am fixing it, not reporting it.
What I broke
#1773 renamed
SelfUpdateOptions.PollIntervaland removed the recurring poll. That leftSelfUpdate__PollInterval: "1.00:00:00"indeploy/aks/values.aks.yaml— a deliberate one-roll-per-day production throttle — binding to nothing, while still reading like a live throttle to anyone opening the file.The cause is specific: I grepped
.csfor the renamed option, and grepped the charts only for the option I deleted. A rename is exactly as breaking as a deletion for a__-bound key.Without pacing, publication frequency is portal-restart frequency, one for one — and each restart drops the live circuits of everyone using the portal.
The floor
SelfUpdateOptions.MinRollInterval, bound fromSelfUpdate:*like the rest, default 1 hour.🚨 Restart-surviving state, and why not uptime
A successful roll restarts the process, so an in-memory "last rolled at" is empty exactly when the floor needs it.
IDeploymentUpdatergrowsLastRolledAtAsync, answered inMeshWeaver.SelfUpdate.Aksby an annotation the patch itself stamps on the Deployment, and "never" by the platform'sDetectOnlyfallback (an install with no patcher cannot roll anyway).Not process uptime — the tempting third option, wrong for crash recovery: a pod that comes back on an OLD image has a young process but an old deployment, and uptime would make it wait out a floor it satisfied hours ago, precisely when it most needs the newer image. The annotation gives the right answer.
AnOldStampRollsImmediately_TheCrashRecoveryCasepins exactly that.🚨 The value is 1 h, deliberately not the 24 h prod had
Restoring the prior number is not the neutral default it looks like. With hourly publication, a daily floor means a merged fix reaches prod the next day — strictly worse than the 3 h window the maintainer rejected. "Restore prior behaviour" would silently answer their complaint with "no".
Half the 2026-08-10 rationale for 24 h is also obsolete: adopt-before-compile took a prod roll from 80 compiles / 64.8 s to 0 compiles / 32.1 s, leaving the restart cost as the only live argument — which is what this bounds.
values.aks.yamlcarries that trade in prose, so the number can be changed without re-deriving it. And the floor paces only the automatic cadence —kubectl rollout restart(startup pass takes the newest image) and amain-cd.ymldispatch both still apply an urgent fix immediately.Timing, verified rather than assumed
Prod runs
3.0.0-rc4.ci.4147(commit324e61a), an ancestor of #1773 — so the portals still carry the old poller and are still on the 24 h throttle. The unpaced behaviour is not live yet; it begins with the first post-#1773 image prod rolls onto, which is the deadline this wants to be inside.Verification
-c Release -p:CIRun=true -warnaserror→0 Error(s).🤖 Generated with Claude Code
https://claude.ai/code/session_01VjujCq1BsJkNKuZ4wXodo1