Repository navigation
Conversation
…(live update, slice 1) Policy module-live-update-default: a module update goes live in the running process by default. This slice lands the foundation — the load contexts: - ModuleLoadContext: collectible context per module generation; platform first (one copy of every platform contract), then another module's current generation (dependency edges recorded), then the generation's own directory; natives via the ModuleNativeAssets candidates; IPlatformLoadContext; Autofac + STJ cache eviction. - ModuleContexts: the mesh's registry — Load / Commit / Retire (unload on a positive quiescence signal, tracked on CollectibleContextUnloads), Resolve, DependentsOf; disposed with the mesh. - MeshBuilder.InstallModules loads every module the image does not bind (TRUSTED_PLATFORM_ASSEMBLIES) into its own context and commits it only after its contributions materialised; a failed generation is unloaded and never takes the name, so the previous generation / image copy stay reachable. - NodeType contexts and kernel script sessions bind modules through ModuleContexts.Resolve (the current generation). - JsonMemberAccessorCacheEviction moves to MeshWeaver.Mesh.Contract (public) so a module context can use it. Doc: Architecture/LiveModuleUpdate; policy row module-live-update-default. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… swap and live-first activation (slice 2) Policy module-live-update-default, slice 2 (stacked on #6121): - ModuleContributions + LiveUpdateBlockers: what a generation contributes and what the running process cannot re-apply (root services, mesh-hub configuration, address types, builder hook, endpoints) plus a declared [assembly: ModuleRestartRequired("why")]. - ModuleLiveUpdateGuard: a module that blocks a live swap must declare it (blank refused). - Re-appliable seams: module nodes served from the current generation in the seed tier; every-per-node-hub configuration through an indirection; InstalledModuleAssembly transient for context-held modules, so the compile reference set (now keyed by module MVIDs) and InstalledModulesFingerprint (now live) follow a swap. No registration captures a boot Assembly strongly (one did and rooted generation N forever). - ModuleLiveUpdater (Graph): load N+1, refuse with N untouched on any blocker/failure, commit N+1 and re-load dependents (all-or-nothing), recycle the bound per-node hubs, retire the old generations once those hubs are dead. Serial; open-vocabulary outcomes. - ModuleLiveActivation (PluginCatalog) + SelfUpdateHostedService.ConsiderRestart: live first; only what does not go live takes the automatic, approval-free restart, whose announcement names the modules and reasons. New verdict ActivatedLive. - PendingModuleActivations prefers the registry's current generation over the AppDomain's (two generations coexist until the old one is collected). Tests: ModuleLiveSwapTest (8), ModuleLiveUpdateGuardTest (4), ModuleUpdatesGoLiveTest (6). Doc: Architecture/LiveModuleUpdate (slice 2 + the measured 4-of-41 classification). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results 22 files ± 0 22 suites ±0 42m 21s ⏱️ -19s Results for commit 2daa092. ± Comparison against base commit 00a03cc. This pull request removes 35 and adds 61 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…ncing Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 2) 5 files ± 0 5 suites ±0 3m 41s ⏱️ -48s Results for commit 2daa092. ± Comparison against base commit 00a03cc. This pull request removes 1243 and adds 1223 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 1) 4 files ± 0 4 suites ±0 5m 36s ⏱️ +30s Results for commit 2daa092. ± Comparison against base commit 00a03cc. This pull request removes 1222 and adds 1244 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 3)2 283 tests - 131 2 283 ✅ - 131 6m 14s ⏱️ +33s Results for commit 2daa092. ± Comparison against base commit 00a03cc. This pull request removes 704 and adds 571 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Slice 2 of live module update (stacked on #6121): every module the image does not bind now boots into its own collectible ModuleLoadContext tracked by the new ModuleContexts registry; ModuleLiveUpdater swaps a landed generation N+1 in live — blocker-checked refusal, all-or-nothing commit with dependent re-load and rollback, per-node-hub recycle, lease-quiesced retire, serialised through a subject and Concat — and SelfUpdateHostedService.ConsiderRestart now activates live first (ModuleLiveActivation), taking the automatic restart only for what did not go live, with the reasons named in the restart announcement and a new appended ActivatedLive verdict. The compile seams follow a swap: InstalledModuleAssembly is transient for context-held modules, MeshNodeCompilationService's reference set is keyed by module MVIDs, InstalledModulesFingerprint reads live, node lists and per-node-hub configuration go through indirections, and PendingModuleActivations prefers the registry's serving generation over the AppDomain-derived map.
Reviewed from the diff alone. THE DIFF IS INCOMPLETE: the ConfiguredModuleActivationTest.cs patch is truncated (first 9574 of 12829 characters kept) and three new test files are omitted entirely — ModuleLiveSwapTest.cs, ModuleLiveUpdateGuardTest.cs and ModulesRunInTheirOwnContextTest.cs — so nothing is asserted about their content, and the test results claimed in the PR body are unverified. Surfaces outside these hunks (ModuleActivationStatus, ModuleGenerationPin, AlcLeaseRegistry, ActivationRecycle, RunLevelChanged, CompileReferences, the fingerprint's Compute) could not be read either. In what is visible, checked: the swap pipeline's refusal and rollback paths and its serialisation, the live-first decision path with its null-catalog and undetermined fallbacks, the MVID-keyed reference cache, the live fingerprint, the pending-reader registry fix, and the consistency of the appended enum member with its factories and tests.
REQUEST_CHANGES on two blocking findings: ModuleLiveUpdater.Swap is an ungated, arbitrary-path live code-load reachable from in-mesh NodeType code — no permission or origin gate where the restart lane it complements has one, no platform-link check on the swapped bytes, and the loaded context is platform-classified — and HubsToRecycle uses a null-forgiving ! against the repository's rule banning ! to silence a warning. Also one should-fix (Dispose abandons in-flight swaps, breaking the class's always-one-outcome contract), one question (the recycle wait presumes RunLevelChanged completes when a hub dies), one nit (a pragma disabling CS1591 opens the new test file).
Findings: 2 blocking · 1 should-fix · 1 question · 1 nit
Internal review of b92a615bd0ae9e6bb375a984bfddafd9af69a8ef — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| /// <param name="entryLocation">The landed generation's entry DLL.</param> | ||
| /// <param name="reason">Why the swap is asked for — carried into every recycled hub's | ||
| /// <c>[QUIESCE-START]</c>.</param> | ||
| public IObservable<ModuleSwapOutcome> Swap(string entryLocation, string reason) => |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
ModuleLiveUpdater.Swap is an ungated, arbitrary-path code-load on the mesh container. AddGraph registers it as a plain singleton (GraphConfigurationExtensions.cs), so any code that can reach the mesh's services can resolve it — including in-mesh NodeTypes: the PR's own PluginCatalogConfigurationExtensions comment documents that a NodeType's layout area resolves mesh singletons from hub.ServiceProvider. Swap validates only that some generation whose name matches the target DLL's file name is currently held (ModuleContexts.Load takes the module name from the target path, and Run refuses only when contexts.Current(name) is null); there is no permission check and no origin gate — although the restart lane this change complements is gated, per the PR's own LiveModuleUpdate doc (the routed Restart carries origin: self-update and is accepted only on a node created by system-security) — and no platform-link or floor check on the swapped bytes, where the boot path (MeshBuilder.TryLoad) runs ModulePlatformLink.Check before loading. Materialising the new generation executes its attribute code (ModuleContributions.Of runs the MeshNodeProviderAttribute getters), and it executes in a ModuleLoadContext marked IPlatformLoadContext, which the in-mesh impersonation guard classifies with the platform. Net effect: code from the lower-trust in-mesh class can make the running process load and execute bytes from any readable path as platform-trusted code, or roll a held module back to any old generation directory on disk, by naming the DLL after a held module. The designed caller is safe — ModuleLiveActivation swaps only the landing service's pinned copy of a landed generation — so gating Swap like the restart lane, or restricting it to the landing service's pinned paths plus a platform-link check, closes this without changing the designed flow.
There was a problem hiding this comment.
Fixed in e69cc18 — in part. The other part is answered below.
Fixed: the platform-link check. LoadAndCommit now runs the same fail-closed ModulePlatformLink.Check that boot runs (MeshBuilder.TryLoad) before contexts.Load. It runs against the application closure, the target's directory and every held generation's directory. That is before any byte of N+1 executes, and so before ModuleContributions.Of runs attribute code. A refusal returns a Failed outcome that names the link report, and N keeps serving.
Not changed: an origin/permission gate on Swap. The trust boundary this finding describes is the mesh's service container, and it predates this PR. ModuleContexts is already a resolvable mesh singleton on main, with a public Load/Commit that loads any path into a ModuleLoadContext : IPlatformLoadContext. Code that can resolve mesh singletons can therefore already do everything Swap does, without going through Swap. A check inside Swap alone would be a gate on one door of a room with another open door. In-mesh code reaching platform services through hub.ServiceProvider is the in-mesh impersonation boundary's concern (Doc/Architecture/InMeshImpersonation), and it should be closed there for every platform singleton, not here for one. The designed caller (ModuleLiveActivation) only ever passes the landing service's pinned copy of a landed generation.
| .Where(h => paths.Contains(ActivationRecycle.BoundNodeType(h)!) || paths.Contains(ActivationRecycle.PathOf(h))) | ||
| .ToImmutableList(); | ||
| } | ||
|
|
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
HubsToRecycle silences a nullable warning with the null-forgiving operator: paths.Contains(ActivationRecycle.BoundNodeType(h)!). MeshWeaver's rules ban ! used to silence a warning. The value is guarded — perNode is built a few lines above with Where(h => ActivationRecycle.BoundNodeType(h) is not null) — so this is mechanical to fix without behaviour change: project each hub to its non-null node type once and filter on that pair, instead of recomputing the lookup and suppressing its nullability.
There was a problem hiding this comment.
Fixed in e69cc18. HubsToRecycle now projects each hub once to (Hub, NodeType), filters on NodeType is not null, and the path filter pattern-matches x.NodeType is { } nodeType. The ! is gone and behaviour is unchanged.
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void Dispose() |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
Dispose tears the swap pipeline down in the wrong order: pipeline.Dispose() runs before jobs.OnCompleted(). Disposing the Concat subscription abandons every queued or in-flight job, and a job's AsyncSubject then never receives OnNext or OnCompleted — so a caller awaiting Swap's observable is left with neither an outcome nor a completion, where the class promises everywhere else that it is cold, emits one outcome and never throws through the observable. At mesh teardown this can hang the SelfUpdateHostedService LiveFirst chain instead of answering it. Completing the subject before disposing the pipeline — and failing any still-pending results — keeps the always-one-outcome contract.
There was a problem hiding this comment.
Fixed in e69cc18. Dispose now closes intake (jobs.OnCompleted()) first, then disposes the pipeline, then answers every job it abandoned. Each job sits in an unanswered set, and whoever removes it answers it: the pipeline with its outcome, or Dispose with a Failed "the mesh is shutting down — a restart activates the landed generation" outcome. A Swap asked for after disposal is answered the same way at once. So every caller gets exactly one outcome.
New regression test: ModuleLiveSwapTest.ADisposedUpdater_AnswersEveryQueuedAndLateSwap_WithAShutdownRefusal. Negative control: with the old dispose order it fails at the 60 s timeout on the queued swap. With the fix the class is 9/9 green.
| .Merge() | ||
| .LastOrDefaultAsync() | ||
| .Select(_ => Unit.Default); | ||
|
|
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
RecycleAndRetire waits for the recycled hubs to die with h.RunLevelChanged.LastOrDefaultAsync(), merged and reduced — but LastOrDefaultAsync only emits when the source COMPLETES, so this presumes RunLevelChanged completes when a hub is disposed. If it is a hot stream that does not complete on hub death, every hub-recycling swap would run the full 60-second RecycleBudget, hit the Timeout catch, and report the old generation KEPT loaded — a leak on every swap that touches hubs, not just a teardown edge. RunLevelChanged's semantics are outside this diff, and ModuleLiveSwapTest.cs — the test that would show it — is one of the omitted patches, so this could not be verified from the review materials.
There was a problem hiding this comment.
RunLevelChanged does complete when a hub dies. MessageHub.RunLevel's setter calls runLevelChanged.OnCompleted() on the transition to MessageHubRunLevel.Dead, so .LastOrDefaultAsync() terminates for exactly that purpose (see the comment there). It is also a BehaviorSubject, so a hub that is already dead when the swap subscribes replays Dead and completes at once.
The swap tests measure it. AModuleUpdate_GoesLiveInTheRunningProcess_AndTheOldGenerationIsCollected recycles at least one hub, and its old generation is really collected. Under the leak this question describes, the retire step would never run and the drain would report the context retained. No change needed.
| @@ -0,0 +1,390 @@ | |||
| #pragma warning disable CS1591 | |||
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The new test file opens with pragma warning disable CS1591. The stated repository rule bans pragma used to silence a warning; whether test projects carry an established exemption for missing-XML-doc disables cannot be verified from this diff (the other test projects are not in it).
There was a problem hiding this comment.
Fixed in e69cc18: the pragma is removed. test/Directory.Build.props already carries CS1591 in NoWarn for the whole test tree, so the pragma suppressed nothing, and Memex.Portal.Shared.Test builds clean with -c Release -warnaserror without it.
Test Results (shard 5) 2 files ±0 2 suites ±0 8m 33s ⏱️ + 1m 1s Results for commit 2daa092. ± Comparison against base commit 00a03cc. This pull request removes 3 and adds 5 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 4) 4 files ± 0 4 suites ±0 15m 17s ⏱️ -52s Results for commit 2daa092. ± Comparison against base commit 00a03cc. This pull request removes 591 and adds 746 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…ry caller at disposal, and silences nothing - ModuleLiveUpdater.LoadAndCommit runs the SAME fail-closed ModulePlatformLink probe boot runs (MeshBuilder.TryLoad) before any byte of N+1 executes; a refusal is a Failed outcome, N keeps serving. - Dispose closes intake, then the pipeline, then answers every job it abandoned — a queued or late Swap now emits a shutdown refusal instead of never completing (ModuleLiveSwapTest negative control: the old order times out at 60 s). - HubsToRecycle projects each hub to its non-null node type once — no `!`. - ModuleUpdatesGoLiveTest drops its CS1591 pragma (test/Directory.Build.props already NoWarns it). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
systemorph-com
Bot
added
the
thread:owner-systemorph-meshweaver-6123-108d63
|
🔗 |
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Slice 2 of the live module update, stacked on #6121 so the diff also carries slice 1's load contexts: every module the image does not bind now boots into its own collectible ModuleLoadContext held by the new ModuleContexts registry; ModuleLiveUpdater swaps a landed generation in live — fail-closed platform-link probe before any byte runs, blocker refusal, all-or-nothing commit with dependent re-load and rollback, per-node-hub recycle, lease-quiesced retire, serial through a subject and Concat — and SelfUpdateHostedService.ConsiderRestart now activates live first through ModuleLiveActivation, taking the automatic restart only for what did not go live, with the reasons named in the announcement and a new appended ActivatedLive verdict. The compile seams follow a swap: InstalledModuleAssembly is transient for context-held modules, MeshNodeCompilationService's reference set is keyed by module MVIDs, InstalledModulesFingerprint reads live, the static node list and the per-node-hub configuration read the current generation through indirections, and PendingModuleActivations prefers the registry's serving generation over the AppDomain-derived map.
Reviewed from the diff alone, which is INCOMPLETE: ModuleLiveUpdater.cs is truncated (the last ~760 characters — the tail of InMeshBuildsReference and the whole Dispose method — are not readable, so the teardown path and the always-one-outcome contract the class's own comments promise could not be verified; the previous head's Dispose-order finding could not be re-checked), ConfiguredModuleActivationTest.cs is truncated (its last ~5,460 characters), and ModuleLiveSwapTest.cs, ModuleLiveUpdateGuardTest.cs and ModulesRunInTheirOwnContextTest.cs are omitted entirely — nothing is asserted about their content, and every test result claimed in the PR body remains unverified. Surfaces outside these hunks (ModuleActivationStatus, ModuleGenerationPin, AlcLeaseRegistry and UnloadWhenQuiesced, ActivationRecycle.BoundNodeType and PathOf, RunLevelChanged, ModulePlatformLink and ModulePlatformSurface, MeshScriptEnvironment's per-session module reference guarantee, CollectibleContextUnloads, HostedHubsCollection) could not be read either. In what is visible, checked: the swap pipeline's refusal, rollback and serialisation paths; the live-first decision with its null-catalog and undetermined fallbacks; the MVID-keyed reference cache; the live fingerprint; the node-list and per-node-hub indirections; the pending-reader registry override; the appended enum member with its factories and tests; and the coherence of the measured-blocker vocabulary across guard, registry, doc and tests. The previous head's null-forgiving operator in HubsToRecycle and its test-file pragma are fixed in this head; the ungated Swap entry point is not.
REQUEST_CHANGES on the one blocking finding, with one should-fix (the swap publishes a generation as current before its contributions are recorded), one question (the recycle wait presumes RunLevelChanged completes when a hub dies), and one nit (a null-forgiving operator in the new test file).
Findings: 1 blocking · 1 should-fix · 1 question · 1 nit
Internal review of e69cc18f60654798b2d466409385a2a7a898b5b8 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| /// <param name="reason">Why the swap is asked for — carried into every recycled hub's | ||
| /// <c>[QUIESCE-START]</c>.</param> | ||
| public IObservable<ModuleSwapOutcome> Swap(string entryLocation, string reason) => | ||
| Observable.Defer(() => |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
ModuleLiveUpdater.Swap is an ungated, arbitrary-path live code load on the mesh container. AddGraph registers the updater as a plain singleton (GraphConfigurationExtensions.cs), and mesh singletons are resolvable from per-node hub service providers — the PR's own PluginCatalogConfigurationExtensions comment documents that a NodeType's layout area resolves services from hub.ServiceProvider. Swap checks only that the module named by the target DLL's file name is currently held (Run refuses solely when contexts.Current(name) is null); it then loads and runs those bytes: contexts.Load loads the DLL from whatever path the caller handed in, ModuleContributions.Of executes its MeshNodeProviderAttribute getters, and Commit makes the result the process's serving generation. The platform-link probe this head adds in LoadAndCommit is a compatibility check — whether the bytes link against this process's platform surface — not a check of who is asking or where the bytes came from. Meanwhile the restart this change complements is gated by the PR's own doc (LiveModuleUpdate.md: the routed Restart carries origin: self-update and is accepted only on a node created by system-security), and the loaded generation runs in a ModuleLoadContext marked IPlatformLoadContext, which the same doc says makes the in-mesh impersonation guard classify it with the platform — a marker the doc calls authentic precisely because the platform controls who gets platform classification. Net effect: lower-trust in-mesh code that can resolve the singleton can make the running process load and execute bytes from any readable path as platform-classified code, or roll a held module back to any older generation directory on disk, with none of the provenance or identity checks the landing and restart lanes apply. The designed caller is safe — ModuleLiveActivation feeds only the landing service's pinned path for a landed generation — so restricting Swap to that provenance and/or gating it like the restart lane closes this without changing the designed flow.
There was a problem hiding this comment.
Fixed in 197fb97, and fixed where the problem starts this time. My round-1 answer only defended the surface; this change removes it.
ModuleLiveUpdater.Swap(path, reason) is now internal. MeshWeaver.Graph grants InternalsVisibleTo to MeshWeaver.PluginCatalog only, plus the test suites that already held a grant. The csproj comment says why. In-mesh code (a NodeType's layout area, a script) is compiled in its own assembly and holds no grant, so it can resolve the singleton but cannot name a path.
The only remaining way in is ModuleLiveActivation.ActivatePending(reason). It takes no path. It reads the activation record and PendingModuleActivations, then swaps only the pinned copy (ModuleGenerationPin.PinnedLoadPath) of a generation the landing service landed. That is the provenance you described as the designed caller, and it is now the only caller that compiles. I didn't add an IsGlobalAdmin check inside Swap. The updater runs on the FileSystem pool with no request identity, so an identity check there would read an ambient context that the designed caller does not carry. Identity belongs on whatever asks for an activation pass. Gating the path-taking primitive would be the wrong layer, and the primitive is now unreachable from outside the platform anyway.
The Swap XML doc and LiveModuleUpdate.md (slice 2 table) both record this.
|
|
||
| var dependents = contexts.DependentsOf(name); | ||
| contexts.Commit(fresh); | ||
| contexts.SetContributions(fresh, contributions); |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
The swap publishes its generations through non-atomic seams. LoadAndCommit calls contexts.Commit(fresh) and only then contexts.SetContributions(fresh, contributions) — and again per dependent inside the re-load loop — so between the two statements the registry answers Current(name) with a generation whose Contributions is null, while ModuleContexts's own class doc promises contributions are materialised before a generation is made current. Readers treat that state as empty rather than in-flight: ModuleContexts.CurrentNodes returns an empty list for null contributions, so StaticMeshNodeListProvider.WithCurrentModuleGenerations emits nothing for the module's nodes — a hub built in the window sees the module contributing no nodes at all — and the per-node-hub indirection registered by MeshBuilder.RegisterDefaultNodeHubConfigurations likewise reads an empty collection. The same null state is read inconsistently inside the swap itself: Run refuses when the module's own contributions are null (never recorded — conservative) while the dependent loop treats a dependent's null contributions as swappable. A concurrent compile reading the InstalledModuleAssembly set can also straddle the per-generation commits and compose a mixed-generation reference set. On a serving mesh the window is only a couple of statements and later reads self-heal, but TryLoadIntoOwnContext at boot deliberately holds it open across attribute materialisation, so the seam is real; publishing the generation and its contributions as one atomic record (or letting Commit carry the contributions) would close it.
There was a problem hiding this comment.
Fixed in 197fb97. LoadAndCommit now calls SetContributions before Commit, for the module and for every re-loaded dependent. A not-yet-current generation is invisible to readers, because CurrentNodes, StaticMeshNodeListProvider and the per-node-hub indirection all read through Current(name). So recording its contributions early is unobservable, and no reader can see a current generation whose contributions are null. The ModuleContexts class doc already promised this ordering, and the swap now follows it.
The inconsistency you found inside the swap is fixed in the same commit. A dependent whose contributions were never recorded is now judged the same way as the module itself: unknown, so the swap answers RestartRequired and never treats it as swappable.
Not changed here: boot's TryLoadIntoOwnContext still commits before it materialises. That is deliberate and its comment says why: a module's attribute getters can touch a sibling's types, which resolve through the CURRENT generation. At boot the window is before the mesh serves. The live swap no longer opens that window, and the dependents it re-loads bind the module's new generation, which is already current with its contributions. On the mixed-generation compile: each Commit is now a single atomic registry write, and the dependents are re-committed inside the same pipeline step, so a compile can read between two of those writes. That leaves N+1 of the module with N of a dependent, and the dependent's N is already bound to N+1, which is consistent. I'm noting this rather than claiming a test for it.
| var dead = hubs.Count == 0 | ||
| ? Observable.Return(Unit.Default) | ||
| : hubs.Select(h => h.RunLevelChanged.LastOrDefaultAsync().Select(_ => Unit.Default)) | ||
| .Merge() |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
RecycleAndRetire waits for the recycled hubs to die with hubs.Select(h => h.RunLevelChanged.LastOrDefaultAsync()...).Merge().LastOrDefaultAsync() — a shape that emits only when every hub's RunLevelChanged COMPLETES. If RunLevelChanged is a hot stream that does not complete when a hub is disposed, no hub-touching swap ever observes death: each runs the full 60-second RecycleBudget, falls into the Timeout catch, and answers Live with the old generation KEPT loaded — a leak plus a 60s stall of the serial pipeline on every hub-touching swap, while the caller still reads the outcome as live. RunLevelChanged's completion semantics are outside this diff, and ModuleLiveSwapTest.cs — the test that would settle it — is one of the omitted patches, so this could not be verified either way.
There was a problem hiding this comment.
This doesn't happen, and here is the behaviour that settles it. MessageHub.RunLevel's setter calls runLevelChanged.OnCompleted() when the hub reaches Dead (src/MeshWeaver.Messaging.Hub/MessageHub.cs, the setter around line 326, commented "Dead is terminal: complete the source so .LastAsync() … terminates"). The source is a BehaviorSubject, so a hub that is already dead when we subscribe replays Dead and completes at once. So hubs.Select(h => h.RunLevelChanged.LastOrDefaultAsync()).Merge().LastOrDefaultAsync() emits exactly when the last recycled hub dies, with no 60 s stall.
ModuleLiveSwapTest measures it. AModuleUpdate_GoesLiveInTheRunningProcess_AndTheOldGenerationIsCollected asserts Recycled > 0 and then that generation N is really collected. If the wait had timed out, the outcome text would say "KEPT loaded" and N would stay alive. The class passes 9/9 on 197fb97 (--filter-class MeshWeaver.Graph.Test.ModuleLiveSwapTest, total 9, failed 0).
| Contexts.Current(Module)!.Location.Should().Contain($"{Module}@", "the landed generation is the current one"); | ||
| AssertNothingStuck(); | ||
| } | ||
|
|
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
Contexts.Current(Module)!.Location uses the null-forgiving operator to silence a nullability warning, which the repository's stated rules ban. Whether test projects carry an established exemption for the operator cannot be verified from this diff (the other test projects are not in it).
There was a problem hiding this comment.
Fixed in 197fb97. (Contexts.Current(Module)?.Location).Should().Contain(...) now fails with an assertion message when the module isn't held, instead of throwing an NRE. I swept the same pattern out of both suites in this PR as well: ModuleUpdatesGoLiveTest (LastCheckVerdict! → .OfType<string>()) and ModuleLiveSwapTest (six sites: ?. + assertion, or ?? throw naming what was missing). Both classes are green on that head: 6/6 and 9/9.
…tion can be swapped; contributions are recorded before a generation is made current - ModuleLiveUpdater.Swap loads and runs the bytes at a path as platform-classified code; as a public method on a mesh singleton it let any code that resolves the updater name the bytes or roll a module back to an older directory. It is now internal, granted to MeshWeaver.PluginCatalog alone, whose ModuleLiveActivation.ActivatePending takes no path and swaps only the pinned copy of what the landing service landed (#6123 review). - SetContributions runs BEFORE Commit for the module and every re-loaded dependent, so no reader sees Current(name) with null contributions (read as "contributes nothing"). - A dependent whose contributions were never recorded is judged like the module itself: unknown, so the swap answers RestartRequired instead of treating it as swappable. - Tests: no null-forgiving operator in the swap and go-live suites. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ve-root-services # Conflicts: # src/MeshWeaver.Graph/Configuration/ModuleLiveUpdater.cs
…module-live-endpoints-views # Conflicts: # src/MeshWeaver.Graph/Configuration/ModuleLiveUpdater.cs
…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>
…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>
… throwing helper, not Current(...)! Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Drain: closing as superseded. Merging |
Pull request was closed
1 participant
Live module update — slice 2: the swap, and live-first activation
Stacked on #6121 (slice 1, the load contexts) — this PR's diff includes that commit until #6121 merges. Policy
module-live-update-default; manualDoc/Architecture/LiveModuleUpdate.The rule this makes true
A landed module generation now goes live in the running process first. A restart is taken only when the module declares
[assembly: ModuleRestartRequired("<why>")], when its contributions measure as not re-appliable, or when the live swap fails at runtime. In every one of those cases N keeps serving (never a half-swapped state), the reason is recorded by name, and the existing automatic, approval-free restart is taken (the self-update lane's routed Restart, #4607 — no confirmation).What changes
ModuleContributions/LiveUpdateBlockers()(Mesh.Contract): per-generation contributions and what cannot be re-applied in-process: root services (WithGlobalServiceRegistry), the mesh hub's configuration, address types, the builder hook, HTTP endpoints. Nodes and every-per-node-hub configuration ARE re-appliable.ModuleRestartRequiredAttribute+ModuleLiveUpdateGuard: the explicit, justified exception; the guard fails a module that blocks a live swap without declaring it (or with a blank reason).InstalledModuleAssemblytransient for context-held modules, soMeshNodeCompilationService's reference set (now keyed by module MVIDs) andInstalledModulesFingerprint(now read live) follow a swap → stale-fingerprint builds rebuild against N+1. No registration captures a bootAssemblystrongly (one did — it rooted N forever; the collection assertion caught it).ModuleLiveUpdater(Graph): load N+1 → refuse on any blocker of N / N+1 / a dependent, or a load/materialise failure → commit N+1 and re-load dependents (all-or-nothing rollback) → recycle this process's per-node hubs bound to the module → retire the old generations once those hubs are dead (lease quiescence, never a timer). Serial (subject +Concat). Outcomes: open vocabularyModuleSwapKind.ModuleLiveActivation(PluginCatalog) +SelfUpdateHostedService.ConsiderRestart: live first; only what does not go live takes the restart; the restart announcement names the modules and reasons; new verdictActivatedLive(appended).PendingModuleActivationsprefers the registry's current generation over the AppDomain's (two generations coexist until the old one is collected — the AppDomain alone read that as "unknown" and hid a genuinely pending module; caught byAnUpdateArrivingWhileTheRestartIsInFlight…).🚨 Measured: 4 of 41 shipped modules are live-updatable today
Running the guard's own measurement over every module the Plugins packages ship (built from Plugins
origin/mainf78c46725against this core): Import, Maps, Northwind.Application, OgCard are live; 37 block on root services / mesh-hub configuration / the builder hook / endpoints (table in the doc). Those 37 fall back to the automatic restart until their surfaces are converted — platform follow-up work, named in the doc.MeshWeaver.AIblocks on the builder hook and has dependents bound to it.Tests (local, Release, real counts)
ModuleLiveSwapTest8/8 (running monolith mesh): N→N+1 live with N's context really collected; negative control (a held reference into N → RETAINED by context name); second update mid-swap applied after it, newest serves; read in flight across the swap answered; mesh-hub-configuring N+1 refused with N serving; throwing N+1 →Failed, N serving; declared[ModuleRestartRequired]never swapped; NodeType written against N+1's member fails on N (CS0117 — the 2026-10-05 incident, negative half) and compiles after the swap in the same process.ModuleLiveUpdateGuardTest4/4: nodes-only passes; undeclared blocker FAILS naming module + measurement (negative control); declared passes; blank reason fails.ModuleUpdatesGoLiveTest6/6 (realLandModule+ real self-update decision): live update → 0 restarts, serves v2; restart-required → exactly 1 restart, N serving; injected live failure → exactly 1 restart, reason recorded; two updates before the restart → 1 restart, record names the newest; update during an in-flight restart → no second restart (deferred); negative control: no check runs → the guard names the stuck landed-not-loaded module. Mutation check: forcing the old restart path makes the live test fail (Restarts 1, expected 0).MeshWeaver.Compiler.Pipeline.Test1207/1207;Memex.Portal.Shared.Test2294/2295 passed, 0 failed, 1 skipped;MeshWeaver.Graph.Test(Teardown/Collectible/Module) 95/95 on slice 1.-warnaserror0/0: Mesh.Contract, Compiler.Pipeline, Graph, PluginCatalog, Kernel.Hub, Hosting, Hosting.Monolith, Hosting.Orleans, Hosting.AspNetCore, Documentation, Memex.Portal.Shared, and the four touched test projects.Not established
PendingRestartmarker is still only cleared by a boot (each check now decides from its own process).NotHeld→ still a restart.maineither (IMeshService unresolved), so no control existed.Recycle after deploy: none beyond the roll — the new path takes effect on the next landing.
Pairs-with: none — no public type or member is removed (
InstalledModulesFingerprint.Hashkeeps its signature;SelfUpdateOutcome.ActivatedLiveis appended).Implementers: none — no interface member added.
Mirror-sync: none — no i18n key added or changed.
🤖 Generated with Claude Code