Repository navigation
fix(#1782 gap 4): a fetch MISS stays loud and countable - #2041
Merged
Merged
Conversation
Adoption's only evidence was a log line — and the miss that matters most had none at all. When the registry's index does not advertise a package for this lane, `PluginBundleClient.Adopt` returned the bare integer 0, the SAME value a fully successful adoption returns, with no log and no counter. The compile that followed looked exactly like normal behaviour. That is the consumer's view of the 2026-08-20 outage: an empty index, every consumer quietly compiling, nothing anywhere saying so. With instance-level pre-bake giving way to lazy compile-on-access (#1746), the fetch path becomes the PRIMARY way assemblies arrive, so the metric that proves it works is the only reason anyone would notice it stopping — and that metric is the one thing this change refuses to lose. - `BundleAdoptionKind` names the six ways an attempt can end, because every one of them used to be the same 0. `NotAdvertised` (the silent one) now logs at Warning and names how many packages ARE advertised — which separates "the index is empty" from "my grant excludes me". - `Download` returns a typed result instead of `byte[]?`, which had collapsed "404 for this lane" and "the registry is down" into one null. - `BundleAdoptionLedger` counts the outcomes: process-scoped, bounded, thread-safe, a diagnostic that nothing decides from. A PARTIAL adoption counts as a miss — rounding "adopted 3 of 12" up to "adopted" is how a regression hides inside a success. And an empty ledger says "nothing was attempted", never "no misses": the absence of the lane must not look like the success of it. - `bundle_adoption` health check surfaces it to an operator. Degraded on a miss, never Unhealthy — compiling is correct behaviour, and failing readiness would turn a distribution regression into an outage. The integer return of Adopt/AdoptModule is deliberately unchanged: adoption must never fail an install. Tests: BundleAdoptionMissTest (7) — four against the real client with the registry stubbed at the HttpMessageHandler (no sockets), three pure over the ledger. Non-vacuity: with Miss() reduced to a bare zero, 4 of the 7 fail with "Expected collection to contain a single item, but found 0". Gap 2 (advertising packages the registry has not itself installed) is NOT addressed here — #1948 unioned published modules into the index and #1950 fixed the record visibility; what remains is the entitlement-anchor design question, which needs a maintainer decision rather than an invented answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses #1782 gap 4 by making plugin-bundle adoption “misses” observable and countable (instead of silently returning 0), and surfacing that signal via a bounded in-process ledger plus a health check in the Memex distributed host.
Changes:
- Introduces
BundleAdoptionKind+BundleAdoptionLedgerto classify and record adoption outcomes (including partial adoption as a miss). - Updates
PluginBundleClientto log/record “not advertised / not served / fetch failed / declined / no assemblies” outcomes instead of silently returning0. - Adds a
bundle_adoptionhealth check that reports Healthy vs Degraded based on recorded misses.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.PluginCatalog.Test/BundleAdoptionMissTest.cs | Adds tests asserting that key miss scenarios are recorded (and stay distinguishable). |
| src/MeshWeaver.PluginCatalog/PluginCatalogConfigurationExtensions.cs | Registers BundleAdoptionLedger as a process-scoped singleton for diagnostics. |
| src/MeshWeaver.PluginCatalog/PluginBundleClient.cs | Records/logs adoption misses and returns typed fetch outcomes to keep reasons intact. |
| src/MeshWeaver.PluginCatalog/BundleAdoption.cs | Adds adoption outcome taxonomy + bounded ledger with a human-readable summary. |
| memex/aspire/Memex.Portal.Distributed/Program.cs | Registers the bundle_adoption health check. |
| memex/aspire/Memex.Portal.Distributed/BundleAdoptionHealthCheck.cs | Implements health check reporting Healthy vs Degraded based on ledger misses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| private ImmutableList<BundleAdoptionOutcome> outcomes = ImmutableList<BundleAdoptionOutcome>.Empty; | ||
|
|
||
| /// <summary>Records one attempt. Thread-safe; never throws.</summary> |
Comment on lines
367
to
+375
| /// <summary> | ||
| /// Downloads the bundle, or emits null when the registry has none for this plugin/version. | ||
| /// </summary> | ||
| private IObservable<byte[]?> Download(string pluginId, string version) => | ||
| /// <summary> | ||
| /// A fetch's outcome: the bytes, or the NAMED reason there are none. A bare <c>byte[]?</c> | ||
| /// collapsed "404 for this lane" and "the registry is down" into the same null, and the caller | ||
| /// then collapsed that into the same 0 as a successful adoption. | ||
| /// </summary> | ||
| private sealed record FetchResult(byte[]? Bytes, BundleAdoptionKind Kind, string? Reason = null); |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
…s-loud-and-countable # Conflicts: # src/MeshWeaver.PluginCatalog/PluginCatalogConfigurationExtensions.cs
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.
Addresses gap 4 of #1782. Gap 2 is deliberately not in scope — see the bottom.
What it was
Adoption's only evidence was a log line, and the miss that matters most had none at all:
0is also what a fully successful adoption returns. So "the registry does not advertise this package for my lane" was indistinguishable from "I adopted everything there was", and the compile that followed looked exactly like normal behaviour.That is the consumer's view of the 2026-08-20 outage: an empty index, every consumer quietly compiling, nothing anywhere saying so. And it is what the issue means by:
With instance-level pre-bake giving way to lazy compile-on-access (#1746), the fetch path becomes the primary way assemblies arrive — so that metric is the only reason anyone would notice it stopping.
Two smaller collapses fell out of the same read:
Downloadreturnedbyte[]?, folding "404 for this lane" and "the registry is down" into one null; and a partial adoption ("adopted 3 of 12") reported as an unqualified success while the other nine compiled here.What changed
BundleAdoptionKindnames the six ways an attempt can end, because every one of them used to be the same0.NotAdvertisedlogs at Warning and names how many packages ARE advertised to this instance — which is what separates "the index is empty" (a registry problem) from "my grant excludes me" (an entitlement problem).Downloadreturns a typed result, so the reason survives to the caller.BundleAdoptionLedgercounts the outcomes: process-scoped, bounded (500), thread-safe, appended underImmutableInterlocked. A diagnostic — nothing decides anything from it.bundle_adoptionhealth check surfaces it without opening a portal. Degraded on a miss, neverUnhealthy: compiling is correct, always-available behaviour, and failing readiness would turn a distribution regression into an outage — strictly worse than the regression.The integer return of
Adopt/AdoptModuleis deliberately unchanged: adoption must never fail an install, and widening the contract would ripple through five call sites for no gain.Tests
test/MeshWeaver.PluginCatalog.Test/BundleAdoptionMissTest.cs— four against the real client with the registry stubbed at theHttpMessageHandler(no sockets, no ports, so the real index read, real decline gate and real accounting all run), three pure over the ledger.Non-vacuity —
Miss()reduced to a bare zero (the pre-fix shape):For the maintainer — gap 2 is a design question, not a plumbing one
Gap 2 ("the index advertises only THIS instance's installs") has moved a long way since the issue was written and I did not close it, because what is left is a decision rather than an implementation:
[]regardless.AuthenticatedInstance.Allowsmatches against the install record'sPackageManifest.Source; with no record there is nothing to match on, and inventing an anchor would weaken what fix(#1772): the bundle routes authorize per package, not just authenticate #1777 established.Worth noting that gap 4's diagnostics make gap 2's symptom visible for the first time: an instance asking for a package the registry cannot advertise now says so by name, instead of silently compiling.
🤖 Generated with Claude Code