Repository navigation
fix(retention): prebuilt-bundle sweep is report-only, and its registration is now asserted (#3963) - #3972
Merged
Conversation
…, and its registration is asserted #3963 reported `AddPrebuiltBundleRetention` as having zero callers — never run on any portal. Re-measured, that is FALSE, and the error is a denominator one: the issue's own sweep was `grep -rn ... src/ test/`, and the registration lives in the repo's THIRD code root, `memex/Memex.Portal.Shared/MemexConfiguration.cs:259`, inside `ConfigureMemexMesh` — the boot path BOTH portal hosts call. `MeshBuilder.ConfigureServices` runs against the HOST service collection, so the hosted service does start. The real defect is the opposite direction, and it is live. `PrebuiltBundleRetention.Delete` defaults to `true`, and the disarming key was templated NOWHERE: the portal ConfigMap enumerates keys explicitly, so a values file, an overlay or a Deployment record's extraPortalConfig setting `PreWarm__PrebuiltBundleRetention__Delete` reached no container, silently. Measured on the live control instance 2026-09-10, `memex` and `memex-cloud` both mount `PreWarm__PrebuiltBundleRoot=/data/prebuilt-bundles` and carry no Delete key anywhere; values.aks.yaml and the homebrew defaults mount the same root. Every one of them swept ARMED, against defaults nobody had read a report for. - deploy/helm: bind `PreWarm__PrebuiltBundleRetention__Delete` with `default "false"` and declare it in values.yaml — the fleet becomes report-only and the knob becomes reachable. THIS CHANGE DELETES NOTHING; it removes strictly less than before. - PrebuiltBundleRetentionIsRegisteredTest: drives the real ConfigureMemexMesh and asserts an IHostedService descriptor for the sweep, with the sibling AddModuleGenerationsGc asserted first as the positive control. Verified in both directions — removing the registration fails it on the named assertion while the control still passes. - PrebuiltBundleRetentionArmingGuard: deletion is never armed by omission. Reads the code default off the type, then requires the chart binding with `default "false"`. Verified non-vacuous against a removed line and a flipped default. - TheReportNamesItsDenominator...: "examined 3, collectable 0" and "examined 0" must not produce the same report. - Doc/Architecture/PrebuiltBundleRetention: the whole finding, including the gap the protected set cannot see — a satellite CI gate's MW_PLATFORM_REF is neither a Deployment record nor an instance report, so past 30 days a satellite's pinned build is collectable (the #3876 / Memex#219 shape). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The Helm wiring, tests, and documentation consistently align to make deletion opt-in while asserting both boot-path registration and safe configuration defaults.
Pull request overview
This PR makes the prebuilt-bundle retention sweep report-only by default and adds guardrails to ensure (1) the deletion knob is actually templated into the portal ConfigMap and (2) the sweep’s registration on the portal boot path is asserted by tests. It also documents the operational intent and the known “satellite CI pin” protection gap.
Changes:
- Helm: template
PreWarm__PrebuiltBundleRetention__Deleteinto the portal ConfigMap with adefault "false"and declare the key invalues.yamlto keep the surface reviewable. - Tests: add a chart-arming guard test, a boot-path registration assertion, and a report-denominator assertion to prevent “examined nothing” from reading as “clean”.
- Docs: add an architecture page plus a What’s New entry describing the new safe-by-default behavior and how to roll out deletion.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Test/PrebuiltBundleRetentionTest.cs | Adds a test that the sweep summary names its denominator so an unexamined/empty store can’t read as “nothing collectable”. |
| test/MeshWeaver.Hosting.Test/PrebuiltBundleRetentionArmingGuard.cs | New guard test that enforces the Helm template binds the delete key with default "false" and that values.yaml declares it. |
| test/Memex.Portal.Shared.Test/PrebuiltBundleRetentionIsRegisteredTest.cs | New test asserting ConfigureMemexMesh registers the retention sweep as an IHostedService, with AddModuleGenerationsGc as a positive control. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-10-the-bundle-store-cleanup-now-reports-before-it-removes.md | User-facing release note describing report-only-by-default and safer rollout. |
| src/MeshWeaver.Documentation/Data/Architecture/PrebuiltBundleRetention.md | Architecture documentation for where it’s registered, arming semantics, report format, keep rules, and known gap. |
| src/MeshWeaver.Documentation/Data/Architecture.md | Adds the new architecture page to the topic map. |
| deploy/helm/values.yaml | Declares PreWarm__PrebuiltBundleRetention__Delete: "false" under config.memex_portal. |
| deploy/helm/templates/memex-portal/config.yaml | Renders PreWarm__PrebuiltBundleRetention__Delete into the ConfigMap with ` |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 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
Contributor
Contributor
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 #3963 — but not as filed. The premise is falsified, and the real defect points the other way.
The zero-callers finding does NOT hold
AddPrebuiltBundleRetentionhas a caller, onmain, since the commit that created the service:It sits inside
ConfigureMemexMesh— unconditionally, two lines below its siblingAddModuleGenerationsGc— and both portal hosts call it (Memex.Portal.Distributed/Program.cs:392,Memex.Portal.Monolith/Program.cs:79), both referencingmemex/Memex.Portal.Sharedby project.MeshHostApplicationBuilderis constructed asbase(x => x.Invoke(Host.Services), address), soMeshBuilder.ConfigureServicesruns against the hostIServiceCollectionandAddHostedService<…>really does reach the generic host's roster.The issue's measurement was
grep -rn AddPrebuiltBundleRetention src/ test/. This repo has three roots that ship code —src/,test/andmemex/— and the registration is in the third. UsingAddAssemblyCacheRetentionas the positive control confirmed the error rather than catching it: that one reads as registered on the same restricted search only because it happens to be duplicated per host in MeshWeaver.Plugins.The real defect: deletion was armed, and could not be turned off
PrebuiltBundleRetention.Deletedefaults totrue, andPreWarm__PrebuiltBundleRetention__Deletewas templated nowhere. The portal ConfigMap enumerates every key explicitly — no catch-allrangeover.Values.config— so a values file, a deployment overlay or aHosting/Deploymentrecord'sextraPortalConfigsetting it reached no container, silently. Same silent-drop shape as #1660 WS3 and #2203, except this omission deletes.Measured read-only on the live control instance, 2026-09-10:
PreWarm__PrebuiltBundleRoot…Retention__Deletememex(record v75)/data/prebuilt-bundles(extraPortalConfig and inlineEnv)memex-cloud/data/prebuilt-bundlesdeploy/aks/values.aks.yaml:276anddeploy/homebrew/share/values.local.defaults.yaml:60mount the same root, so every AKS instance and every memex-local install was in the same state.What this changes
This change deletes nothing. It removes strictly less than before: previously armed, now report-only.
deploy/helm/templates/memex-portal/config.yaml— bindPreWarm__PrebuiltBundleRetention__Deletewith an explicit| default "false", mirroringAssemblyCache__Retention__Delete. This both disarms the fleet and makes the knob reachable for the first time.deploy/helm/values.yaml— declare it, so the reviewable surface says so and the existingEveryPreWarmKeyInValues_IsTemplatedInTheConfigMapguard keeps the halves tied.The sweep still runs on every boot and daily after, still plans, still writes its ledger line. Arming becomes a per-instance decision taken against a real report — the rule the sibling's own comment already states.
The controls, in both directions
1. The registration is asserted —
PrebuiltBundleRetentionIsRegisteredTest(Memex.Portal.Shared.Test). Drives the realConfigureMemexMeshand asserts anIHostedServicedescriptor whose implementation type is the sweep — not that the type exists, not that it resolves: registering it as a plainAddSingletoncompiles, resolves and never runs. The siblingAddModuleGenerationsGcregistration is asserted first as the positive control, so a harness that reached no registration at all fails naming itself instead of letting the subject's absence read as a pass.Verified by removing line 259 and rebuilding:
…with the positive control still passing. Restored,
Passed! - Failed: 0, Passed: 1.2. Deletion is never armed by omission —
PrebuiltBundleRetentionArmingGuard(MeshWeaver.Hosting.Test). Reads the shipped default offPrebuiltBundleRetention.Default.Deleterather than assuming it, and only then requires the chart binding, followingPrebuiltBundleRetentionExtensions.DeleteConfigKeyrather than a copied string. Proven non-vacuous against two mutations: binding line removed ⇒ no match;default "false"flipped to"true"⇒ match but assertion fails.3. The report names its denominator.
TheReportNamesItsDenominator_SoAnUnexaminedStoreCannotReadAsACleanOneasserts a populated store reads3 identity directory(ies) … collectable=0while an empty one reads0 identity directory(ies) … collectable=0, and that the two strings differ — so a sweep aimed at an unmounted share can never read as a clean one.The gap this does NOT close, named
DeploymentPinnedReferencesbuilds the protected pin set from exactly two things:Hosting/Deploymentrecords'pinnedImageTag, andHosting/ModuleInventoryreports filed by registered instances. A satellite CI gate pinned to an older platform build is neither — anMW_PLATFORM_REFin Plugins/Reinsurance/SocialMedia/Manufacturing/Crm/Education files no report, holds no record and writes no adoption stamp into this mesh. Such an identity is protected only by rule 3 (a clean, non-pre-release_releasesmarker) or rule 7 (the 30-day window) — and satellite pins areX.Y.Z-ci.<n>pre-releases, so rule 3 never applies. Past 30 days, a satellite's pinned platform build is collectable. That is the #3876 / Memex#219 shape exactly, and it is why arming on the registry (memex-cloud) is the dangerous half. Written up in the doc page with what closing it needs.Also noted rather than fixed:
PrebuiltBundleRetentionStatusis written and has no reader, so "read one report" is break-glass today (a file on the data volume, or Loki).Work product
Doc/Architecture/PrebuiltBundleRetention— registration and how to prove it, the two-way default, the report format, the eight keep rules and the gap, and the four-step rollout. Listed inData/Architecture.md;ArchitectureTopicMapTest+DocumentationLinkIntegrityTestgreen.Verification
dotnet build -c Release -warnaserror,0 Error(s)/0 Warning(s):MeshWeaver.Hosting.Test,Memex.Portal.Shared.Test,MeshWeaver.Documentation.Test.Tests: Hosting.Test
~PrebuiltBundleRetention53/53; Shared.Test registration control 1/1; Documentation.Test topic-map + link-integrity +PlatformBakeLaneGuard+TestTimeoutLiteralRatchetGuard+ What's New 21/21.No public type or member removed; no public interface member added; no i18n catalog key touched. No
TimeSpantimeout literal added.🤖 Generated with Claude Code
https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7