Repository navigation
feat(modules): module endpoints and view packs go live; modules update independently of the platform (slice 6) - #6128
Conversation
…e independently of the platform (slice 6) Policy module-live-update-default, stacked on #6127. - ModuleEndpointDataSource: held modules' HTTP endpoints are mapped onto a private route builder per generation and re-mapped on ModuleContexts.VersionChanged (change token → routing rebuilds); a colliding re-map is not published. Endpoints are no longer a blocker. - MeshNodeProviderAttribute.Views + IViewContributionSource: view packs' control → view registrations are re-read by LayoutClient from the modules' current generations; AddViews folded into the mesh hub stays a named blocker. - ModuleLandingService refuses (by name) a bundle carrying a MeshWeaver.* assembly the running platform ships. - The portal host owns mesh-hub @-autocomplete (AddMeshNavigation), which used to ride the Blazor.Graph view pack and made it restart-required. Tests: ModuleEndpointsSwapLiveTest (2), ModuleViewsSwapTest (2), ModulesUpdateIndependentlyOfThePlatformTest (2). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results 22 files 22 suites 44m 56s ⏱️ Results for commit c49611a. ♻️ This comment has been updated with latest results. |
…round services, keyed services, registry eviction (slice 7) - Hosted services are no longer part of a generation's root-service shape: one ModuleHostedServicesHost starts each module's CURRENT generation's hosted registrations; a swap stops the old set and starts the new one, whatever its size (the real AI N->N+1 differed by exactly one added hosted service). - Keyed root services are forwarded under the module's key (a module-owned key stays a blocker). - MeshContentTypeRegistry evicts a collectible context's types on Unloading — the heap dump's only strong root of the swapped-out AI generation. Measured locally on the incident pair (AI from Plugins b7a083d98~1 -> current): NodeType using ThreadPreparation.Group fails CS0117 on N, swap answers Live, compiles Ok on N+1, N collected. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
feat(modules): the real MeshWeaver.AI update swaps live — added background services, keyed services, registry eviction (slice 7)
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Live module update, slice 6 (the PR body says this head also carries slice 7, #6142, folded into the branch): held modules' HTTP endpoints move behind ModuleEndpointDataSource, re-mapped from the current generations on ModuleContexts.VersionChanged with a colliding re-map left unpublished; view packs move to MeshNodeProviderAttribute.Views read through the new IViewContributionSource and re-assembled by LayoutClient whenever the source version moves; the self-update check activates a landed generation live first (new SelfUpdateOutcome.ActivatedLive) and restarts only what did not go live; the in-mesh compile reference set and InstalledModulesFingerprint re-key on the current module generations; the static-node catalog re-takes on version changes; MeshBuilder slots held modules' nodes and registers current-generation indirections for mesh-hub configuration. Reviewed from the diff alone; nothing was executed. THE DIFF IS INCOMPLETE: 4 patches are cut mid-file (LiveModuleUpdate.md, MeshBuilder.cs, ModuleContexts.cs, ModuleLoadContext.cs) and 24 files are omitted entirely - among them ModuleServices.cs, ModuleServiceForwarders.cs, ModuleServiceProvider.cs, ModuleLiveActivation.cs, ModuleLandingService.cs, PendingModuleActivations.cs, StaticMeshNodeListProvider.cs, ReflectionCacheEviction.cs, JsonMemberAccessorCacheEviction.cs, IMeshContentTypeRegistry.cs, ModuleRestartRequiredAttribute.cs, PluginCatalogConfigurationExtensions.cs and all twelve test files - so the retire/quiescence core of ModuleContexts, the root-service scope machinery, the ModuleLiveActivation seam the new self-update path calls into, the platform-assembly landing refusal and most of slice 7 were NOT read and nothing is asserted about them. The diff carries no project file, so the new MeshWeaver.Mesh.Contract to MeshWeaver.Layout.Client reference wiring was not verified either. No text in the diff addresses a reviewer or carries instruction-like content. Findings: 3 blocking, 2 should-fix, 3 questions, 1 nit.
Findings: 3 blocking · 2 should-fix · 3 question · 1 nit
Internal review of e88d1c3626cd3b40cebe74b42dd4132bbf3996a2 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| var contexts = ModuleContexts; | ||
| ConfigureDefaultNodeHub(config => | ||
| (contexts.Current(name)?.Contributions?.AllDefaultNodeHubConfigurations ?? []) | ||
| .Aggregate(config, (c, configure) => configure(c))); |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
RegisterDefaultNodeHubConfigurations registers the current-generation indirection for a held module only when the BOOT generation already contributes default-node-hub configurations - the early return at the guard `held.Contributions?.AllDefaultNodeHubConfigurations is not { Count: > 0 }`. A live swap to a generation that ADDS such configuration is not refused: no blocker in MeasuredLiveUpdateBlockers names every-per-node-hub configuration (it is treated as re-appliable), and HubsToRecycle recycles every per-node hub because the new generation carries `DefaultNodeHubConfigurations.Count > 0` - but no indirection was ever registered, so each recycled hub rebuilds without the new configuration while the swap outcome is Live: a contribution is silently dropped. Mesh-hub configuration is registered unconditionally (RegisterMeshHubConfigurations) and a root-service shape change is refused in ModuleContexts.PrepareServices; the per-node-hub seam has neither guard. Registering the indirection for every held module - it aggregates to nothing while the current generation contributes none - closes the gap.
There was a problem hiding this comment.
Agreed, and fixed in 91baa3e. RegisterDefaultNodeHubConfigurations now registers the current-generation indirection for EVERY held module; the boot-only AllDefaultNodeHubConfigurations is not { Count: > 0 } early return is gone. The indirection aggregates to nothing while the current generation contributes none, exactly like RegisterMeshHubConfigurations.
New test ModuleAddsNodeHubConfigurationSwapTest.AGenerationThatAddsPerNodeHubConfiguration_ReachesHubsBuiltAfterTheSwap: g1 contributes no per-node-hub configuration, and g2's builder hook adds one that registers a type. After ModuleLiveUpdater.Swap reports Live, the mesh's DefaultNodeHubConfiguration applied to a fresh hub configuration resolves that type. With the old guard restored as a mutant, the test fails ("Expected value not to be "), and with the fix it passes. MeshWeaver.Mesh.Contract and MeshWeaver.Compiler.Pipeline.Test build with -c Release -warnaserror (0/0). The five module-swap test classes pass 18/18.
| // module-live-update-default) — never onto the app directly, where they would be fixed for the | ||
| // life of the process. Image-bound modules map as before. | ||
| var held = app.Services.GetService<ModuleContexts>(); | ||
| if (held is not null |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
The dynamic endpoint data source is created only when some held module already carries MeshEndpointProviderAttribute at boot (`held.Generations.Any(g => g.Assembly.GetCustomAttributes<MeshEndpointProviderAttribute>().Any())`), and it is created once at startup - MapMeshModuleEndpoints does not run again. The data source is the only seam that re-maps a held module's endpoints after a swap, and no swap-time blocker names endpoint mapping (ModuleContributions.MapsEndpoints is applied by the host at endpoint-mapping time; ModuleLiveUpdater's blockers do not cover it): on a host where no held module maps endpoints at boot, a later generation of a held module that ADDS the attribute swaps in Live with its endpoints silently absent - the direct loop is skipped for held modules by the ReferenceEquals check and never re-runs - and the self-update records ActivatedLive, so no restart is ever taken. Creating the data source whenever ModuleContexts is present (it maps zero endpoints until a generation contributes some) closes the gap.
There was a problem hiding this comment.
Agreed, and fixed in 91baa3e. MapMeshModuleEndpoints now creates ModuleEndpointDataSource whenever ModuleContexts is present, not only when a held module carries MeshEndpointProviderAttribute at boot. It maps zero endpoints until a generation contributes some, and its boot log line is written only when it maps any.
New test ModuleEndpointsSwapLiveTest.AGenerationThatAddsEndpoints_IsMappedLive_EvenWhenBootMappedNone: g0 has no endpoint attribute, so /live-endpoint answers 404. After committing g1, which adds the attribute, the route answers v1 in the running TestServer. With the old Generations.Any(...) guard restored as a mutant, this case fails (1 of 3 in the class), and with the fix the class passes 3/3. MeshWeaver.Hosting.AspNetCore and Memex.Portal.Shared.Test build with -c Release -warnaserror (0/0).
| .Where(p => !string.IsNullOrEmpty(p)) | ||
| .ToImmutableHashSet(StringComparer.OrdinalIgnoreCase); | ||
| return perNode | ||
| .Where(h => paths.Contains(ActivationRecycle.BoundNodeType(h)!) || paths.Contains(ActivationRecycle.PathOf(h))) |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
The predicate `paths.Contains(ActivationRecycle.BoundNodeType(h)!)` silences the nullable warning on BoundNodeType(h) with the null-forgiving operator; the repository's hard rules allow no `!` (or #pragma) used to silence a warning. The non-null invariant is already established by the perNode filter above (`Where(h => ActivationRecycle.BoundNodeType(h) is not null)`), so a shape that binds that value - for example a pattern match inside the predicate - carries the invariant without the assertion.
There was a problem hiding this comment.
Resolved by bringing in the #6123/#6127 review fixes, merged into this branch in 0d196ac (fix commit e69cc18 on #6123). HubsToRecycle now projects each hub once to (Hub, NodeType: ActivationRecycle.BoundNodeType(h)), filters NodeType is not null, and the predicate binds it with a pattern: (x.NodeType is { } nodeType && paths.Contains(nodeType)) || …. There is no ! left in ModuleLiveUpdater.cs (grep ')!' finds nothing). Head is 91baa3e.
| // half-honoured (MeshWeaver#2908). | ||
| if (_logger is not null) | ||
| if (_logger is not null && catalog is null) | ||
| foreach (var collision in StaticNodeProviderExtensions.DescribeStaticProviderCollisions(providerList)) |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
Build gates the provider-collision warnings with `if (_logger is not null && catalog is null)`, but the invalidation subscription (`_modules?.VersionChanged.Subscribe(_ => Volatile.Write(ref catalog, null))`) also drives the field to null on every live swap, so the warnings the comment promises once per mesh are re-emitted by the first Build after EVERY swap, and can double under the benign rebuild race in Current. A null field cannot distinguish a first build from an invalidated one; a dedicated first-build flag is needed.
There was a problem hiding this comment.
Resolved by the #6127 review fix 694228a, merged into this branch in 0d196ac. The collision warnings are no longer emitted from Build at all. They run once, in the StaticNodeQueryProvider constructor, right after the first Build, and the comment there says why: Build re-runs after every live swap and cannot tell a first build from a rebuild. So the VersionChanged invalidation, and the benign rebuild race in Current, can no longer re-emit them. #6127 added A_catalog_rebuilt_after_a_module_swap_does_NOT_warn_again to StaticNodePrecedenceTest, which emits two warnings on the old code and one on the fix. Head is 91baa3e.
| /// <inheritdoc /> | ||
| public void Dispose() | ||
| { | ||
| pipeline.Dispose(); |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
Dispose tears the pipeline down before completing the subject: `pipeline.Dispose(); jobs.OnCompleted();`. With the subscription gone, OnCompleted reaches nobody; a job already in flight loses the subscription that completes its AsyncSubject (Concat's inner is disposed with the pipeline, so the `.Do` that calls OnNext/OnCompleted never runs), and a job queued behind it is dropped the same way - that Swap observable then never emits and never completes, and a caller waiting on it (the self-update live-first path) hangs with no outcome and no restart fallback. The queue has to be completed while the subscription still drains it, or the pending subjects have to be completed with a Failed outcome here.
There was a problem hiding this comment.
Resolved by the #6123 review fix e69cc18, merged into this branch in 0d196ac. Dispose now closes intake first (jobs.OnCompleted()), then disposes the pipeline, then answers every job it abandoned with a shutting-down Failed outcome (foreach (var job in unanswered.Keys) Answer(job, Disposing(job))). A Swap requested after disposal gets the same answer. #6123's ModuleLiveSwapTest.ADisposedUpdater_AnswersEveryQueuedAndLateSwap_WithAShutdownRefusal pins this: it hit its 60 s timeout against the old order. That test passes on this branch's head 91baa3e, as part of the 18/18 run over the five module-swap classes.
| issuing.Post(new DisposeRequest { Reason = because }, o => o.WithTarget(new Address(ActivationRecycle.PathOf(hub)))); | ||
|
|
||
| var dead = hubs.Count == 0 | ||
| ? Observable.Return(Unit.Default) |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
`dead` waits on `h.RunLevelChanged.LastOrDefaultAsync()` per recycled hub; LastOrDefaultAsync emits only when its source COMPLETES, so retiring the old generations depends on RunLevelChanged completing when the hub dies. That completion semantics is not in this diff; if the stream is a hot sequence that does not complete on hub teardown, `dead` never emits, every swap with recycled hubs eats the 60s RecycleBudget timeout, and the catch branch reports the old generation KEPT loaded - a leak plus a 60s delay on every live swap. The hub-recycling tests that would settle this are among the omitted files.
There was a problem hiding this comment.
It does complete, so dead emits. MessageHub.RunLevel's setter (src/MeshWeaver.Messaging.Hub/MessageHub.cs, ~L318-331) publishes each transition on the runLevelChanged BehaviorSubject and calls runLevelChanged.OnCompleted() when the level reaches Dead, with the comment "Dead is terminal: complete the source so .LastAsync() … terminates". LastOrDefaultAsync over a recycled hub therefore emits when that hub dies, not after the 60 s RecycleBudget. The same answer was given on #6123's identical thread, and the swap tests that recycle hubs (ModuleLiveSwapTest and others) complete within seconds: the five module-swap classes ran 18/18 in about 11 s on 91baa3e, which a 60 s budget per swap would not allow. No code change.
| return pool.InvokeBlocking(_ => LoadAndCommit(name, old, target)) | ||
| .SelectMany(plan => plan.Outcome is { } refused | ||
| ? Observable.Return(refused) | ||
| : RecycleAndRetire(name, plan, reason)); |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The dependent loop reads `dependent.Contributions?.LiveUpdateBlockers() is { IsEmpty: false }`: a dependent whose Contributions were never recorded counts as having NO blockers and is re-loaded and committed as part of the swap, while the same state on the primary module is a RestartRequired (its running generation's contributions were never recorded). The fail-safe direction differs between a module and its dependents with no visible reason for the difference.
There was a problem hiding this comment.
No good reason for the difference, so it is fixed in 91baa3e. The dependent loop now uses the same fail-safe as the module itself: (dependent.Contributions?.LiveUpdateBlockers() ?? UnrecordedContributions) is { IsEmpty: false }. A dependent whose contributions were never recorded is therefore a RestartRequired naming it ("its dependent X its running generation's contributions were never recorded"), never "no blockers". The one reason string is now a single immutable constant used by both checks. MeshWeaver.Graph builds with -c Release -warnaserror (0/0), and the module-swap classes pass 18/18. No dedicated test constructs a dependent with unrecorded contributions.
| this.contexts = contexts; | ||
| this.createApplicationBuilder = createApplicationBuilder; | ||
| this.logger = logger; | ||
| endpoints = Map(); |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The constructor publishes its boot map (`endpoints = Map();`) without a collision check - FindRouteCollisions runs only in Remap - and MapMeshModuleEndpoints adds the data source straight to the route builder in the visible hunk. Whether the unchanged tail of MapMeshModuleEndpoints re-checks collisions across all data sources cannot be read from this diff; if it does not, a held module can publish a route colliding with an existing one at boot (ambiguous matching at request time), where the direct path's marker-scoped collision refusal the same hunk references would have refused it.
There was a problem hiding this comment.
The boot map is covered. MapMeshModuleEndpoints adds the data source to ((IEndpointRouteBuilder)app).DataSources, so its endpoints, each carrying MeshModuleEndpointMetadata, become part of the host's composite EndpointDataSource. RegisterCollisionCheck runs on ApplicationStarted over app.Services.GetRequiredService<EndpointDataSource>().Endpoints, which is that composite, and it is registered whenever contributed > 0, and contributed includes dynamicEndpoints.Count. So a held module's boot route that collides with an existing one gets the same Critical log, StopApplication and throw as a directly mapped one. Only re-maps after a swap need Remap's own check, because by then the startup check has already run. With 91baa3e, a boot that maps zero dynamic endpoints registers no startup check, which is correct because there is nothing to collide; a later generation's routes still go through Remap's refusal (ASwapWhoseRouteCollidesWithTheHost_IsNotPublished_… passes). No code change for this thread.
| } | ||
|
|
||
| /// <summary>How many endpoints the held modules currently contribute.</summary> | ||
| public int Count => endpoints.Count; |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
`Count => endpoints.Count` reads the field plainly while `Endpoints` reads it with `Volatile.Read(ref endpoints)`; MapMeshModuleEndpoints logs Count at boot, so the two accessors of one field disagree on visibility for no reason - Count should use the same Volatile.Read.
There was a problem hiding this comment.
Fixed in 91baa3e: Count => Volatile.Read(ref endpoints).Count, the same visibility as Endpoints. MeshWeaver.Hosting.AspNetCore builds with -c Release -warnaserror (0/0).
…into HEAD # Conflicts: # src/MeshWeaver.Mesh.Contract/ModuleServiceForwarders.cs
… is served after a live swap Review of #6128: the per-node-hub indirection and the dynamic endpoint source were registered only when the BOOT generation contributed, so a later generation that added either swapped in Live with the contribution silently absent. Both now exist for every held module (they map nothing until a generation contributes). A dependent with unrecorded contributions is a restart, as the module itself is; ModuleEndpointDataSource.Count reads with the same visibility as Endpoints. Two new tests, each red on the mutant that restores the boot-only guard. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…module-live-endpoints-views # Conflicts: # src/MeshWeaver.Graph/Configuration/ModuleLiveUpdater.cs
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Slice 6 of live module update (the PR body says this head also carries slice 7, #6142, folded into the branch): held modules' HTTP endpoints move behind the new ModuleEndpointDataSource, re-mapped from current generations on ModuleContexts.VersionChanged with colliding or throwing re-maps left unpublished; view packs move to MeshNodeProviderAttribute.Views, read live through IViewContributionSource (implemented by ModuleContexts) and re-assembled by LayoutClient when the version moves; the self-update check activates a landed generation live first (new SelfUpdateOutcome.ActivatedLive, appended) and restarts only what did not go live, naming the reason; the in-mesh compile reference set re-keys on module MVIDs and InstalledModulesFingerprint reads live; the static-node catalog re-takes on version changes with provider-collision warnings moved to construction; MeshBuilder slots held modules' nodes and registers current-generation indirections for mesh-hub and per-node-hub configuration; NodeAssemblyLoadContext, script sessions and the kernel resolve modules through the registry. Re-checked against the previous head's posted review (e88d1c36): both previous blockings are verifiably fixed in this diff (the per-node-hub indirection is now registered for EVERY held module; the dynamic endpoint source is created whenever ModuleContexts exists), the dependent fail-safe now treats unrecorded contributions as a restart, the static-node collision warnings are emitted once at construction, and the Count accessor uses Volatile.Read. Two previous findings could not be re-verified because their code sits beyond this head's truncation: the null-forgiving ! in HubsToRecycle (previously blocking) and the Dispose teardown order — both are asked as questions below. THE DIFF IS INCOMPLETE: 5 patches truncated (LiveModuleUpdate.md, MeshBuilder.cs, ModuleContexts.cs, ModuleLoadContext.cs, ModuleLiveUpdater.cs) and 26 files omitted — among them ModuleServices.cs, ModuleServiceForwarders.cs, ModuleServiceProvider.cs, ModuleLiveActivation.cs, ModuleLandingService.cs, PendingModuleActivations.cs, StaticMeshNodeListProvider.cs, ReflectionCacheEviction.cs, JsonMemberAccessorCacheEviction.cs, IMeshContentTypeRegistry.cs, ModuleRestartRequiredAttribute.cs, PluginCatalogConfigurationExtensions.cs and all twelve test files — so the root-service scope machinery, the retire/quiescence core and Resolve of ModuleContexts, the live-activation seam the new self-update path calls into, the landing refusal and most of slice 7 were NOT read and nothing is asserted about them. The diff carries no project file, so the new Mesh.Contract-to-Layout reference wiring was not verified. Reviewed from the diff alone; nothing was executed; the test counts in the PR body are unverified. No text in the diff addresses a reviewer or carries instruction-like content. Findings: 1 blocking, 1 should-fix, 4 questions.
Findings: 1 blocking · 1 should-fix · 4 question · 0 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
question src/MeshWeaver.Graph/Configuration/ModuleLiveUpdater.cs
The previous review round flagged paths.Contains(ActivationRecycle.BoundNodeType(h)!) in HubsToRecycle as a hard-rule violation — a null-forgiving ! silencing a nullable warning, where the non-null invariant is already established by the perNode filter above it. HubsToRecycle's body lies beyond this head's truncation (the patch keeps the first 20000 of 23180 characters and cuts inside that method's doc comment), so whether the operator was removed cannot be verified from this diff; a shape that binds the non-null value — for example a pattern match inside the predicate — carries the invariant without the assertion.
question src/MeshWeaver.Graph/Configuration/ModuleLiveUpdater.cs
The previous round flagged the teardown order in Dispose (pipeline.Dispose(); jobs.OnCompleted();) as leaving in-flight and queued jobs' AsyncSubjects unanswered, so a caller of Swap never receives an outcome. This head adds the unanswered registry, the Disposing refusal and the disposed check in Swap — the machinery for exactly that fix, and the field comment says 'or Dispose with a refusal' — but Dispose's body itself is in the truncated tail and cannot be read from this diff; whether every unanswered job is answered there, and in what order relative to tearing down the pipeline subscription, is unverified.
Internal review of 91baa3e36bb67d975e9396ea570bef6c4ea78ee0 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| contexts.Commit(previous); | ||
| foreach (var abandoned in committed) | ||
| contexts.Discard(abandoned); | ||
| return refused with { Outcome = Fail(name, old.Location, target, $"a dependent did not re-bind to N+1: {ex.GetType().Name}: {ex.Message}") }; |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
LoadAndCommit commits N+1 (contexts.Commit(fresh)) before the dependent loop; Commit bumps VersionChanged, which ModuleEndpointDataSource subscribes to and re-maps from contexts.Generations, so from that point the new generation's endpoints are published and routable while the swap can still fail. When a dependent's reload or PrepareServices fails, the catch re-commits the serving generations and calls contexts.Discard on every committed generation — Discard unloads immediately with no quiescence wait, and its own doc justifies that with 'loaded but never committed', which is false on this path. Two consequences the design's own contract excludes: requests routed in that window execute the rolled-back generation's code while N serves the hubs (a half-swapped state; 'never a half-swapped state' and 'N keeps serving' are this change's stated invariants), and the unload bypasses the AlcLeaseRegistry positive-quiescence rule every other unload in this change follows. Deferring the endpoint publication until the swap has fully committed, or routing the rollback's discards through the retire path, closes it.
There was a problem hiding this comment.
Agreed, and fixed at the root rather than by deferring the endpoint source. The bug was in the swap itself (#6123's LoadAndCommit): N+1 became current before its dependents re-bound, so EVERY reader of the generations (endpoints here, but also the view registrations and the static-node catalog) could see a swap that could still fail, and the rollback then Discard-ed a generation requests may have reached.
The swap is now staged and committed as one step:
- Every generation of a swap loads into one
ModuleSwapStageviaModuleContexts.LoadStaged. A staged context binds the STAGED module generation (ResolveForDependentconsults the requester's stage first), so dependents re-bind to N+1 while nothing current moves. - Only after the module and every dependent have loaded, materialised and prepared their services does
CommitAllmake the whole set current, with ONEVersionChangedemission.ModuleEndpointDataSourcetherefore re-maps once, from a complete swap. - A failure before that calls
DiscardStaged. None of those generations was ever current or routed to, soDiscard's "loaded but never committed" now holds literally, and there is no commit-then-unload path left. The serving generations never stop serving, so nothing is re-committed. LoadStagedis a distinct name, not aLoadoverload, because an added overload turns every existing<see cref="Load"/>into CS0419 under-warnaserror.
Commits: 4ea31b3 and 2daa092 on feat/module-live-swap (#6123), merged up as 6b13341 (#6127) and 5789e7b (this PR).
Tests: AStagedSwap_RebindsTheDependentToN1_WhileNothingCurrentMoves_ThenCommitsInOneStep (a staged dependent sees lib v2 while Current is unchanged and no VersionChanged fired; then exactly one emission on commit) and AStagedSwapThatIsDiscarded_LeavesTheServingGenerationsCurrent_AndPublishesNothing. Negative control: with the stage lookup removed from ResolveForDependent, the rebind case fails. All module-swap classes pass on this head: 27/27 in Compiler.Pipeline.Test and 10/10 in Memex.Portal.Shared.Test; every touched project builds -c Release -warnaserror with 0 warnings. Not covered by a test: a dependent whose re-load fails end-to-end through ModuleLiveUpdater; that path is now a single DiscardStaged call.
|
|
||
| var result = hooks.Aggregate(capture, (builder, hook) => hook(builder)); | ||
|
|
||
| var blockers = ImmutableList.CreateBuilder<string>(); |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
capture.ModuleContexts.Dispose() runs only on the success path. When a hook throws at hooks.Aggregate, the exception propagates to ModuleContributions.Of's catch and the capture builder — including any module generations a hook installed into the capture registry before throwing (itself a named blocker: 'it installs modules itself (InstallAssemblies)') — is left undisposed: those collectible contexts then unload only at GC, unobserved by CollectibleContextUnloads, outside the retire path's sentinel. A try/finally around the Aggregate restores the symmetry the success path has.
There was a problem hiding this comment.
Agreed, fixed. CaptureBuilderHooks now runs the hooks inside a try/finally that disposes capture.ModuleContexts on every path, and it reads "did a hook install modules" inside the try, before the dispose. A hook that installs modules and then throws no longer leaves those collectible contexts to the GC. The exception still propagates unchanged to ModuleContributions.Of's caller.
Commit 6b13341 on feat/module-live-root-services (#6127, where this file lives), merged into this PR as 5789e7b. Builds -c Release -warnaserror with 0 warnings; ModuleBuilderHookSwapTest and the other swap classes pass. Not added: a test for the throwing-hook path. The capture's registry is not attached to CollectibleContextUnloads, so there is nothing to observe an unload through from a test.
| { | ||
| // The mesh-level contributions first — mesh-hub type registrations and mesh types — so | ||
| // a hub that re-instantiates below already resolves the new generation's types. | ||
| foreach (var generation in plan.Serving) |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
dead waits on h.RunLevelChanged.LastOrDefaultAsync() per recycled hub, merged and last — LastOrDefaultAsync emits only when its source COMPLETES, so retiring the old generations depends on RunLevelChanged completing when the hub dies. That completion semantics is not in this diff, and the hub-recycling tests that would settle it are among the omitted files. If the stream is hot and does not complete on teardown, every swap with recycled hubs eats the 60s RecycleBudget timeout and the catch branch reports the old generation KEPT loaded — a leak plus a 60s delay on every live swap.
There was a problem hiding this comment.
RunLevelChanged does complete when the hub dies; it is not a hot, never-completing stream. src/MeshWeaver.Messaging.Hub/MessageHub.cs, the RunLevel setter (around lines 318-331):
runLevel = value;
runLevelChanged.OnNext(value);
// Dead is terminal: complete the source so `.LastAsync()`, `.ToTask()` and every other
// completion-shaped composition over it terminates instead of hanging on a hub that will
// never emit again.
if (value == MessageHubRunLevel.Dead)
runLevelChanged.OnCompleted();The backing subject is a synchronized BehaviorSubject, so a subscriber that arrives after the hub is already Dead gets the terminal value and the completion at once, with no race against the teardown. So LastOrDefaultAsync per recycled hub ends when that hub dies, and the 60 s RecycleBudget is only reached when a hub really does not die. In that case the old generation is deliberately kept loaded and the outcome says so. AModuleUpdate_GoesLiveInTheRunningProcess_AndTheOldGenerationIsCollected (ModuleLiveSwapTest) recycles hubs and asserts the old generation is actually collected, which could not pass if every swap ate the timeout. It passes on this head (5789e7b), with the other module-swap classes at 27/27. No change made.
| this.createApplicationBuilder = createApplicationBuilder; | ||
| this.logger = logger; | ||
| endpoints = Map(); | ||
| // Lives as long as the registry — the same mesh as the host. |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The constructor publishes its boot map (endpoints = Map()) with no collision check — FindRouteCollisions runs only in Remap — and MapMeshModuleEndpoints adds the data source straight to the route builder in the visible hunk. Whether the unchanged tail of MapMeshModuleEndpoints re-checks collisions across all data sources at boot cannot be read from this diff; if it does not, a held module can publish a route colliding with an existing one at boot (ambiguous match at request time), where the direct path's marker-scoped collision refusal the same hunk references would have refused it.
There was a problem hiding this comment.
The boot map is checked, by the host's startup refusal rather than by the source itself. In MapMeshModuleEndpoints, the dynamic source is added to the app's DataSources and its count goes into contributed; when that is non-zero, RegisterCollisionCheck runs FindRouteCollisions at ApplicationStarted over the composite EndpointDataSource, which includes the dynamic source's endpoints. Every endpoint the dynamic source maps carries MeshModuleEndpointMetadata, so a collision with a host route is in scope. On a collision the host logs Critical, calls StopApplication() and throws. Live re-maps are checked by Remap itself, which is why the constructor does not repeat the check.
Now pinned by a test, commit 5789e7b: AHeldModuleWhoseBootRouteCollidesWithTheHost_IsRefusedAtStartup (ModuleEndpointsSwapLiveTest) starts a TestServer host whose held module maps GET /host-route at boot, the same pattern the host maps, and asserts the app is stopping. The control in the same test, a non-colliding held module, keeps running. Negative control: with the dynamic source's endpoints not counted toward contributed (so the startup check is never registered), the test fails. ModuleEndpointsSwapLiveTest + ModuleUpdatesGoLiveTest: 10/10.
…s never current while a dependent can still fail #6128 review (blocking): LoadAndCommit made N+1 current before re-binding its dependents, so every reader of the generations saw N+1 while the swap could still fail, and the rollback then Discard-ed a generation requests had been routed to, bypassing the quiescence wait. Every generation of a swap now loads into one ModuleSwapStage (a staged context binds the STAGED module generation), and CommitAll makes the set current only after all of them loaded, materialised and prepared; a failure before that unloads the staged set, which nothing reached. LoadStaged is a distinct name, not a Load overload, so no <see cref="Load"/> turns into CS0419. Tests: two staged-swap cases in ModulesRunInTheirOwnContextTest; with the staged binding removed the rebind case fails. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ule-live-root-services Resolves ModuleLiveUpdater: the dependent loop keeps this branch's service preparation inside the stage. CommitAll emits ONE VersionChanged on this branch. Also: CaptureBuilderHooks disposes the capture's module registry on every path (try/finally) so a hook that installs modules and then throws leaves no collectible context to the GC; the swap tests drop Current(...)! for a throwing helper. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…into feat/module-live-endpoints-views #6128 review: N+1 is no longer current, and so its endpoints are no longer published, while a dependent can still fail (staged swap from #6123, one VersionChanged per commit). Adds a startup test that a held module's BOOT route colliding with the host is refused by the startup check (negative control: not counting the dynamic source's endpoints leaves it running), and drops the remaining null-forgiving operators and the redundant CS1591 pragma in the swap tests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 0) 3 files 3 suites 3m 7s ⏱️ Results for commit c49611a. ♻️ This comment has been updated with latest results. |
Test Results (shard 2) 5 files 5 suites 3m 53s ⏱️ Results for commit c49611a. ♻️ This comment has been updated with latest results. |
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: inherited from its base 'main': 8 pull requests of Systemorph/MeshWeaver fail identically — 'lane / Automatic review answered' concluded failure: Process completed with exit code 1. — the base 'main' moved from 00a03cc (what the red run tested) to 60aa837. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: 'lane / Automatic review answered' is red on run 37313355550, a head that tested main at 00a03cc; main has since moved to 60aa837 and its newest run of this check is green (established by today's executed update-branch decisions for #6097, #6115, #6132, #6139 and #6141 on this same fingerprint) — the red is the base's at the time, not this diff's. Merging the current base in gives a new head whose run re-tests against the fixed base; the open review findings travel on the new head's own review. It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
Test Results (shard 1)1 858 tests 1 856 ✅ 5m 38s ⏱️ Results for commit c49611a. ♻️ This comment has been updated with latest results. |
Test Results (shard 3)2 398 tests 2 398 ✅ 6m 44s ⏱️ Results for commit c49611a. ♻️ This comment has been updated with latest results. |
Test Results (shard 5) 2 files 2 suites 8m 39s ⏱️ Results for commit c49611a. ♻️ This comment has been updated with latest results. |
Test Results (shard 4) 4 files 4 suites 16m 53s ⏱️ Results for commit c49611a. ♻️ This comment has been updated with latest results. |
… not a literal 30 s TestTimeoutLiteralRatchetGuard counted 381 against its baseline of 380 — this file's TimeSpan.FromSeconds(30) was the new site. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: inherited from its base 'main': 4 pull requests of Systemorph/MeshWeaver fail identically — 'lane / Automatic review answered' concluded failure: Process completed with exit code 1. — the base 'main' moved from b8f6793 (what the red run tested) to 8c08fa4. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: Runs 37419883459/37419883473 tested this head against main at b8f6793; main has since moved to 8c08fa4 and this pass's own base reading names Systemorph/MeshWeaver main green, so this is a stale base, not this diff. The 'lane / Automatic review answered' failure is byte-identical across four heads of this repository and nothing in the evidence (no log line, no changed file) points at this diff; the same signature on this pull request's earlier head f2f28e5 was cleared by an approved, executed update-branch at 04:12. Approving so the head merges the green, moved base and its gates re… It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
Conflicts with #6202/#6206 (module types served from the modules wherever asked): - ModuleServiceForwarders.AddModuleOwned: main replaced the per-node forwarders with ModuleOwnedRootSource; this branch had taught them keyed registrations. Taken main's deletion and carried the keyed half into the root source: a KeyedService for a type a module declares is answered by key (ModuleServices.KeyedRegistrationsElsewhere). - ModuleServices.Scope: Contributed joins the index-keyed routes, and a keyed registration keeps its own key on every route (both intents). - RegistrationsElsewhere never counts a keyed registration among a type's unkeyed positions (ResolveAt would point past the end) nor serves a keyed contribution as an unkeyed one. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Slice 6 of live module update, with the folded slice 7: held modules' HTTP endpoints move behind a dynamic ModuleEndpointDataSource that re-maps each module from its current generation on ModuleContexts.VersionChanged and fires its change token (per-module refusal with an exchange-aware joint collision decision, subscription bound to the host, snapshot/subscription window closed by a version re-check); view registrations move to MeshNodeProviderAttribute.Views and are re-read by LayoutClient through IViewContributionSource (implemented by ModuleContexts); one ModuleHostedServicesHost replaces the per-registration hosted forwarders so hosted services leave the swap shape; keyed registrations forward under their own key, with a blocker for keys owned by the module; MeshContentTypeRegistry evicts a collectible context's types on Unloading; the landing path refuses bundles carrying platform assemblies by name; the portal owns AddMeshNavigation. Tests execute for real: TestServer routing, collectible-context collection drains, negative controls for every refusal. Checked the Remap choice/pending/settled composition and unchanged detection, the change-token swap/cancel pattern and its lock ordering, the keyed path end to end (Scope keying, unkeyed/keyed counting in ElsewhereByKey, Resolve by the registration's own key, Shape keys), HandOverHosted now starting the new generation even when the old had no services, hosted start/stop sequencing and fault isolation, registry eviction atomicity against a successor's claim, and the landing refusal's filename normalization. Could not verify from the diff: that MeshWeaver.Mesh.Contract already carries the project references for its new MeshWeaver.Layout(.Client) usages (no csproj for it is in the diff), ModuleContexts' commit-versus-Version ordering (both new caches assume the data lands before the version moves), and the two reachability questions asked below. No blocking findings.
Findings: 0 blocking · 1 should-fix · 2 question · 1 nit
Internal review of 2e53dc6908146ff5288a38cf4a6412efdf00565c — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| // RuntimeType and so passes an instance check). A root registration would hold either for the | ||
| // life of the process, and with it the module's collectible load context. | ||
| if (descriptor.IsKeyedService && descriptor.ServiceKey is { } key | ||
| && (IsOwned(key.GetType(), module) || (key is Type keyType && IsOwned(keyType, module)))) |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
The keyed-key guard refuses only a key whose type comes from the registering module itself. A key whose type belongs to a DIFFERENT held module — the same marker-type shape this guard was written for, e.g. AddKeyedSingleton<IService>(typeof(AnotherModulesMarker), ...), or an instance of another module's type — passes the IsOwned check and is forwarded to the root as ServiceDescriptor.KeyedSingleton/KeyedTransient(type, key, ...) in ModuleServiceForwarders.AddForwarders, so the root holds that key object for the life of the process and pins the OTHER module's collectible load context after it swaps — the exact leak the guard was added to close. The owning module's own probe cannot see it (the registration is not its own, and Shape records the key's ToString, which is stable across the other module's swap). Refusing any key whose type's assembly is collectible — or belongs to any held context — not only this module's, would close the hole.
There was a problem hiding this comment.
Valid — fixed in c988a3e. The keyed-key guard now refuses any key whose type (or, for a Type key, the type it names — generic arguments and element types included) comes from the registering module OR from ANY collectible assembly (PinsACollectibleContext: IsOwned(type, module) || type.Assembly.IsCollectible || …). That covers another held module's marker type and instances of another module's (or a compiled NodeType's) types, which the root would otherwise hold past that context's swap. AModuleTypeAsServiceKeyIsABlockerTest now also loads a second collectible context and asserts AddKeyedSingleton<IDisposable>(typeof(OtherMarker), …) is named as a blocker; the plain-string-key control still forwards. Release -warnaserror clean; test 1/1.
| foreach (var module in app.Services.GetServices<InstalledModuleAssembly>()) | ||
| foreach (var attribute in module.Assembly.GetCustomAttributes<MeshEndpointProviderAttribute>()) | ||
| { | ||
| if (held?.Current(module.Assembly.GetName().Name ?? "") is { } generation |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
This skip keeps a held module off the direct map only while the enumerated assembly IS its current generation; it is re-evaluated per module inside this loop, so its soundness rests on no live-swap commit landing between the dynamic source's snapshot above and this check. If one does land there, the boot assembly is no longer Current, the skip fails, and that assembly's endpoints are mapped directly onto the app — fixed for the life of the process (silently stale after later swaps when the new generation renames its routes; a fail-fast startup collision when the patterns coincide) while the dynamic source also maps the new generation. Is MapMeshModuleEndpoints guaranteed to run before any commit can land, and can it ever be called on an already-started host?
There was a problem hiding this comment.
Valid concern — removed at the root in c988a3e rather than argued. The skip no longer depends on timing: it is keyed on the module being HELD (held?.Current(name) is not null), not on the enumerated assembly being its current generation. The dynamic source maps every held module's CURRENT generation (it enumerates ModuleContexts.Generations), so a held module's endpoints belong to it alone; with the identity check, a swap committing between the source's snapshot and that line would have mapped the old generation onto the app permanently. With the name check, no interleaving can put a held module on the direct map. ModuleEndpointsSwapLiveTest 8/8; Hosting.AspNetCore Release -warnaserror clean.
| if (!moduleHostedServicesHostRegistered) | ||
| { | ||
| moduleHostedServicesHostRegistered = true; | ||
| services.AddSingleton<IHostedService>(sp => new ModuleHostedServicesHost( |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
ModuleHostedServicesHost is registered only inside InstallModuleServices' clean branch, guarded by a builder-instance flag. If every held module installed at boot probes with root-service blockers and a module whose services probe cleanly first appears later through the live landing path, this AddSingleton runs on a root collection whose container is already built — a silent no-op — and no ModuleHostedServicesHost ever exists, so that module's hosted services silently never start (the per-registration forwarders this replaced had the same no-op shape, so the hole is inherited rather than new). Can a held module's first clean InstallModuleServices happen after the root container is built? If not, a comment pinning that invariant would settle it.
There was a problem hiding this comment.
No — a held module's first clean InstallModuleServices cannot happen after the root container is built, and c988a3e adds the comment pinning that invariant at the flag. InstallModuleServices has one caller, the builder's install loop (MeshBuilder.cs ~439), which runs before the container exists. The live path never installs a module for the first time: ModuleLiveUpdater always swaps FROM a running generation and calls ModuleContexts.PrepareServices(old, fresh, …), which refuses when the running generation had root-service blockers (went straight into the root container at boot) and when it had no forwarders (the new generation registers root services and the running one did not — the root's forwarders are laid out at boot) — both route to a restart, where the builder runs again on a fresh collection.
| contributed += dynamicEndpoints.Count; | ||
| if (dynamicEndpoints.Count > 0) | ||
| logger.LogInformation( | ||
| "Mapped {Count} endpoint(s) from modules held in their own load contexts, re-mapped on every live swap", |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The arguments of this LogInformation call sit at the same indent as the call itself: the message line and the dynamicEndpoints.Count line are flush with logger.LogInformation( instead of one level deeper.
There was a problem hiding this comment.
Valid — fixed in c988a3e (the message and argument lines are now indented one level under logger.LogInformation().
…; held-module endpoint skip keyed on held, not identity - ModuleServices: a keyed registration whose key (or key Type) comes from any collectible assembly — not only the registering module's — stays private, so the root never pins another module's generation. Pinned in AModuleTypeAsServiceKeyIsABlockerTest. - MapMeshModuleEndpoints skips every HELD module (the dynamic source maps its current generation), so a swap committing mid-boot cannot map an old generation onto the app for good. - MeshBuilder: comment pins the boot-only invariant for the hosted-services host. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Head c988a3e8 carries slice 6 plus the folded slice 7 and resolves every finding of the posted review of 2e53dc69: the keyed-key blocker now refuses ANY key that pins a collectible context (own module's or another's, Type keys via the key-is-Type branch) — covered by AModuleTypeAsServiceKeyIsABlockerTest; the direct-map skip is keyed on the module being HELD by name rather than on the enumerated assembly being the current generation, so a commit between the source's snapshot and the loop can no longer map a stale generation onto the app, and the source is created whenever modules are held at all so a later generation that adds endpoints is mapped (both test-pinned); the ModuleHostedServicesHost registration invariant is pinned by a comment; the log indentation is fixed. New since that head: the exchange-aware joint collision decision in Remap (pending maps decided together, per-module fallback in name order against the choice built so far — traced against the route-exchange test), the boot snapshot/subscription window closed by a version re-check with an afterSnapshot test seam, host-bound subscription disposal, sequential hosted start/stop through InSequence with fault isolation, keyed forwarding under the registration's own key end to end (Scope keying, the unkeyed-count fix in ElsewhereByKey, Resolve by the descriptor's own key, the Shape key suffix), the per-node-hub indirection registered for every held module, content-type registry eviction on Unloading (ConditionalWeakTable; fired by the explicit Unload, so the registry's map cannot be both the root and the reason eviction never runs), the adopt-path landing refusal for bundled platform assemblies, portal-owned AddMeshNavigation with a dedupe test, and the de-flaked holder-first test helper (deterministic comparer, fail-loud guard). Checked the change-token swap/cancel pattern and the gate/remapGate lock separation, HandOverHosted starting the new generation unconditionally, the eviction's interaction with a successor's claim, the landing refusal's own-name exception and image-bound filter, and LayoutClient's version-keyed cache including its benign torn-write cases. Could not verify from the diff: MeshWeaver.Mesh.Contract's project references for its new MeshWeaver.Layout(.Client) usages (no csproj for it among the files); Commit's data-before-Version ordering that both new caches and the pre-existing static-node catalog rely on (Bump/Commit internals not shown); the framework internals the group mapping leans on (RouteGroupBuilder data-source propagation into the private route builder) — exercised by the TestServer tests but not visible here; the PrepareServices invariant the new comment claims; and the CI and measured claims in the PR body. No blocking findings; one question.
Findings: 0 blocking · 0 should-fix · 1 question · 0 nit
Internal review of c988a3e886a0fb7e73a1c965adfe3eae83f81d8a — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| public IEnumerable<IComponentRegistration> RegistrationsFor( | ||
| Service service, Func<Service, IEnumerable<ServiceRegistration>> registrationAccessor) | ||
| { | ||
| if (service is KeyedService { ServiceKey: { } key } keyed) |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The keyed stand-in path added here — a keyed registration of a type the module DECLARES (route ModuleOwned, never forwarded into the root, so the root holds no explicit descriptor for it), asked for by key — is the one shape of the new keyed forwarding with no test in this diff. ModuleRootServicesSwapTest resolves ILiveGreeter under the key keyed-greeter: a platform-declared interface, which Route() sends to Proxy and AddForwarders forwards as an explicit root KeyedSingleton, so the registration source is never consulted. KeyedStandIns depends on KeyedRegistrationsElsewhere finding the owner generation of the type, and the predicate that determines the owner sits outside this diff — it must be keyed-aware for a type whose only registrations are keyed (the unkeyed position count inside the owner branch is, but the owner lookup itself is not visible here). Is this path covered by a test elsewhere, and does the owner determination see keyed-only declarations? The measured claim in the PR body (Azure.Blob's keyed IStreamProviderFactory) covers the forwarded shape, not this one.
There was a problem hiding this comment.
Both answered. Owner determination is keyed-agnostic by construction: ElsewhereByKey picks the owner as generations.FirstOrDefault(g => IsOwned(type, g.Context)). That is a check on the TYPE's load context, independent of what is registered, so a type whose only registration is keyed is still owned by its declaring generation. The keyed branch then looks for a ModuleOwned registration of that type matching the key. On coverage you were right that the path had no test. 984c665 adds one: the test module now declares IOwnGreeter and registers it ONLY keyed (AddKeyedSingleton<IOwnGreeter, OwnGreeter>("own-keyed")). The new AKeyedServiceOfATypeTheModuleDeclares_ResolvesOnTheRootByItsKey resolves it on the root with GetRequiredKeyedService(type, "own-keyed") and asserts own v1. Because no root descriptor exists, that answer can only come from ModuleOwnedRootSource.KeyedStandIns. ModuleRootServicesSwapTest 5/5, including the existing swap test with its unload assertion. Release -warnaserror clean.
…the root by its key Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Head 984c6656 is slice 6 (module endpoints and view packs go live; modules update independently of the platform) plus the folded slice 7 (one ModuleHostedServicesHost, keyed root services, content-type registry eviction), and the keyed stand-in question from the posted review of c988a3e is closed: AKeyedServiceOfATypeTheModuleDeclares_ResolvesOnTheRootByItsKey executes the keyed stand-in path of ModuleOwnedRootSource — a module-declared type whose only registration is keyed, asked by its key on the root, through the owner determination KeyedRegistrationsElsewhere depends on — the exact shape that review asked about. Checked as it stands at this head: the joint-then-solo collision decision in ModuleEndpointDataSource (traced against the exchange and per-module refusal tests, including the unchanged early return that fires no token when a candidate is refused and nothing else changed), the version-before-snapshot window with subscribe and re-check and its afterSnapshot seam, host-bound subscription disposal, the held-by-name direct-map skip with the source created whenever modules are held, the per-module boot-collision refusal at startup, sequential hosted start and stop through InSequence with fault isolation, keyed forwarding under the registration's own key end to end (the !IsKeyedService guard in Scope, the unkeyed-count fix in ElsewhereByKey, Resolve by the descriptor's own key, the Shape key suffix, the PinsACollectibleContext blocker for own, foreign and Type keys), the per-node-hub indirection registered for every held module, registry eviction on Unloading (ConditionalWeakTable; the explicit Unload fires Unloading before the collection check, and a successor's claim in a different context survives eviction), the adopt-path landing refusal for bundled platform assemblies with its own-name exception and image-bound filter, portal-owned AddMeshNavigation with the dedupe test, and the deterministic holder-first comparer replacing the one-in-257 hash search. Could not verify from the diff: MeshWeaver.Mesh.Contract's project references for its new MeshWeaver.Layout(.Client) usages (no csproj for that project among the files); the owner-determination code above the ElsewhereByKey hunk (outside the diff — now exercised by the new test, but not visible here); the reconciler-wiring tests named in the documentation (PackagesAutoUpdateTest, ModuleReloadByRestartTest, ModuleBundleFloorHoldTest) and the mutation-check claim, all outside this diff; the RouteGroupBuilder data-source propagation into the private route builder (framework internals — exercised by the TestServer tests); the PrepareServices invariant the ModuleHostedServicesHost registration comment claims; the measured and CI claims in the PR body; and several spans of the item's patch rendering are masked, so a few lines — notably the running-blockers expression in ModuleLiveUpdater — were reconstructed from the surrounding visible text (Restart takes ImmutableList<string> and receives running; the UnrecordedContributions field; the adjacent comment), which pins it as (LiveUpdateBlockers() ?? UnrecordedContributions).AddRange(old.RootServiceBlockers…) with the unrecorded fail-safe kept, rather than read literally. No blocking findings; one nit.
Findings: 0 blocking · 0 should-fix · 0 question · 1 nit
Internal review of 984c66562f13a81eebc66f3df4cda0188d9ac30c — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| { | ||
| var type = HeldGeneration(Module).Assembly.GetType("MeshWeaver.Test.LiveServices.IOwnGreeter") | ||
| ?? throw new Xunit.Sdk.XunitException("IOwnGreeter is not in the held module"); | ||
| var greeter = Mesh.ServiceProvider.GetRequiredKeyedService(type, "own-keyed"); |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The own-keyed test added in this diff pins the boot-time resolution only (own v1). Nothing asserts the module-declared keyed path after a swap: the live-swap test pins post-swap behavior for the forwarded keyed shape (keyed.Greet() returns the v2 answer after the g2 swap), but the own-keyed stand-in (IOwnGreeter under own-keyed) is never asked again. KeyedStandIns' resolvers read contexts.ModuleScope(ownerName) per call, so the path follows a swap by construction; resolving IOwnGreeter by own-keyed once more inside the existing live-swap test and expecting own v2 would pin that against a future scope cache or hoisted resolver — this being the path the new test was added to cover.
There was a problem hiding this comment.
Correct — the post-swap own-keyed path is unasserted: after the g2 swap only the forwarded keyed shape (keyed-greeter) is checked; IOwnGreeter under own-keyed is asked at boot only.
Not changed from here: this PR's owner thread on the control instance has no push surface for a pull-request item, so the test change is the author's. The addition, inside AModuleWithRootServices_SwapsLive_… right after the keyed.Greet() v2 assertion:
OwnKeyedGreeting().Should().Be("own v2",
"a module-declared KEYED service asked by its key on the root follows the swap too");
// …
[MethodImpl(MethodImplOptions.NoInlining)]
private string? OwnKeyedGreeting()
{
var type = HeldGeneration(Module).Assembly.GetType("MeshWeaver.Test.LiveServices.IOwnGreeter")
?? throw new Xunit.Sdk.XunitException("IOwnGreeter is not in the held module");
return (string?)type.GetMethod("Greet")!.Invoke(Mesh.ServiceProvider.GetRequiredKeyedService(type, "own-keyed"), null);
}Two cautions. Read the type from the CURRENT generation at call time — after the swap IOwnGreeter is a different Type — and keep it out of the test's locals, so nothing the test holds keeps N alive for the closing weakN assertion. As written it pins what the nit names: a resolver or scope hoisted per owner name would answer [PERSON_NAME]'s type from N's scope and fail. Calling the helper once BEFORE the swap as well would additionally test whether the root's container retains N's keyed stand-in registration, against the same collection assertion — that is not established from this diff, and a failure there would be a real retention finding, not a test defect.
Relayed from the owner thread Hosting/Triage/pull-request/systemorph-meshweaver-6128/_Thread/owner-systemorph-meshweaver-6128 by the PR steward.
There was a problem hiding this comment.
Fixed in c49611a: the live-swap test now resolves IOwnGreeter by 'own-keyed' again after the g2 swap (through the held generation's type, in a NoInlining helper so nothing pins generation N for the unload assertion) and expects 'own v2'. The boot-time test reuses the same helper.
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: stale: 'Consolidate test results', 'stage-gate / Stage gate: may the suites start' red on a head that tested 'main' at a6d8afc — green on the base's newest run, so a gate the base has fixed since, not the diff ('Consolidate test results' concluded failure) — the base 'main' moved from a6d8afc (what the red run tested) to 484e61a. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: Stale base, not the diff: run 37560835006 failed 'Consolidate test results' and 'stage-gate / Stage gate: may the suites start' on a head that tested base 'main' at a6d8afc, while the base's newest run is GREEN on those same gates and 'main' has since moved to 484e61a — a gate the base has fixed since, which the diff cannot reach. The proposal carries no log line naming a changed file, and the failed checks are aggregation/stage gates, not a compile error or failing test in the diff. The measured remedy for a stale base is to merge the fixed 'main' into the branch (update-branch), pr… It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
…ap (own v2) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Live module update — slice 6: endpoints and view packs go live; modules update independently of the platform
Stacked on #6127 → #6123 → #6121 (the diff includes them until they merge). Policy
module-live-update-default; manualDoc/Architecture/LiveModuleUpdate→ "slice 6".What changes
ModuleEndpointDataSource(MeshWeaver.Hosting.AspNetCore): a held module's HTTP endpoints map onto a private route builder per generation (same authenticated-by-default group + module marker asMapMeshModuleEndpoints) and are re-mapped from the current generations onModuleContexts.VersionChanged, firing the data source's change token so routing rebuilds its matcher. A re-map that collides with another route is NOT published (previous endpoints keep serving; Critical log naming both). Image-bound modules map as before. → Courses, Mail, Mcp, Teams, WhatsApp no longer blocked by endpoints.MeshNodeProviderAttribute.Views(new virtual member with a default — no implementer obligation) +IViewContributionSource(MeshWeaver.Layout): a view pack's control → view registrations are re-read byLayoutClientfrom the modules' CURRENT generations whenever the source's version moves. Image-bound modules'Viewsfold into the mesh hub asAddViewsalways did.AddViewsinsideHubConfigurationsremains a named blocker; the packs convert in MeshWeaver.Plugins.ModuleLandingServicerefuses, by name, a bundle carrying aMeshWeaver.*assembly the running platform ships (adopt path; the registry shelf is unaffected). The packer already drops them with--platform-app.AddMeshNavigationon the mesh hub (idempotent) — it used to ride the Blazor.Graph view pack's mesh-hub configuration and was what made that pack restart-required.Tests (local, Release, real counts)
ModuleEndpointsSwapLiveTest2/2 (real ASP.NET Core routing on TestServer: route answers from N+1 after the swap, N collected; negative control: a colliding re-map is not published, the old route serves),ModuleViewsSwapTest2/2 (sameILayoutClientresolves N+1's view after the swap; negative control:AddViewsthroughHubConfigurationsis a named blocker),ModulesUpdateIndependentlyOfThePlatformTest2/2 (platform fixed; M goes N → N+1 → N+2 live through the real landing path — N+2 recorded against an older platform build/floor; N+3 with a floor above the platform is declined BY NAME by the reconciler's decision while a sibling updates live in the same pass; a bundle with a platform assembly is refused naming it, the same bundle without it lands).MeshWeaver.Compiler.Pipeline.Test1216/1216,Memex.Portal.Shared.Test2298 passed / 0 failed / 1 skipped,MeshWeaver.Layout.Test616/616,MeshWeaver.Graph.Test2508/2508,MeshWeaver.Hosting.Test1097 / 1 failed / 1 skipped — the failure isAGatedSweepHearsItsOwnCompileTest.AProvisionalSweep_ReportsCompiled_ThoughItsStampIsHeld, the known intermittent tracked in MeshPublicationGate.HeldCount reads 0 immediately after the pre-warmer reports a stamp held — AProvisionalSweep_ReportsCompiled_ThoughItsStampIsHeld fails intermittently in CI #6050 (no module is installed in that test).-warnaserror0/0 on every touched project; all 41 shipped module projects of MeshWeaver.Plugins build Release-warnaserroragainst this core.Not established
Pairs-with: none — no public type or member on
mainis removed.Implementers: none —
MeshNodeProviderAttribute.Viewsis a virtual member with a default on an abstract class;IViewContributionSourceis a new interface.Mirror-sync: none — no i18n key added or changed.
Also carries slice 7 (#6142, folded into this branch — its base was unprotected, so arming auto-merge merged it into this branch, not into main). The real MeshWeaver.AI update swaps live: hosted services are not part of the root-service shape (one
ModuleHostedServicesHostper process starts the current generation's set), keyed root services are forwarded, andMeshContentTypeRegistryevicts a collectible context's types onUnloading. Measured locally on the incident pair (AI from Pluginsb7a083d98~1→ current): NodeType usingThreadPreparation.GroupCS0117on N → swapLive→Okon N+1, N collected. Tests:ModuleRootServicesSwapTest.AnUpdateThatAddsAHostedService_SwapsLive_AndStartsIt,ContentTypeRegistryReleasesAnUnloadedGenerationTest(mutation-checked). Local Release: Compiler.Pipeline 1218/0, Memex.Portal.Shared 2299/0, Graph 2508/0, Layout 616/0, Messaging.Hub 567/0, Hosting 1099/1 (#6050 known flake).🤖 Generated with Claude Code