Repository navigation
fix(#3510): the retyped-root recycle only runs when it has a job - #3800
Conversation
The disposer was named by measurement in #3510's thread: it is PackageInstaller.SettleRetypedRoot. This is the half of it that is pure loss. The recycle exists for one purpose — after the root's in-package NodeType is REBUILT, tear the root down so the hub that comes back binds the package's own configuration instead of the fallback. An install that wrote nothing rebuilt nothing, and the code already knows that twice, on either side of the recycle: .SelectMany(… result.Written > 0 ? RequestReleases(firstWave) : …) gated .SelectMany(… SettleRetypedRoot(hub, retypedRoot, nodes, logger)) NOT .SelectMany(… result.Written > 0 ? RequestReleases(deferredWave) : …) gated retypedRoot is non-null on EVERY install of a package whose root has a non-static NodeType, so the recycle ran unconditionally between two neighbours that both refuse to act on a no-op install. Measured on CD 34190841613 (2026-09-08): package Video logged `0 written, 21 unchanged` and recycled anyway; Chess was recycled TWICE per bake. Each teardown cut off the correlated work in flight beneath the root — its own PluginGating reconcile — which the quiesce could not answer and force-cancelled at 2 s, surfacing as HubDisposedBeforeResponseException. RecycleDeclineReason(rootPath, written) is the rule, pure, and the decline is SAID: a silent skip and a skip nobody chose read identically in a log. And the positive line stopped over-promising. It ended "Work in flight beneath this root is answered by the teardown, not abandoned." — measurably false in the same trace. A diagnostic asserting a guarantee the code does not keep is worse than one that says nothing: it sends the next reader looking for a different cause, which is what the CD 7950 archaeology had to undo. 🚨 NOT fixed, and why #3510 stays open: an install that DID write still recycles a root whose background pipeline is issuing correlated requests, and the trace reads `RECEIVED runLevel=Quiescing@Chess` — a hub taking new correlated work past quiesce, its own defect and its own family. Widening the 2 s window would be the band-aid this repo refuses. Falsified: make RecycleDeclineReason never decline and only AnInstallThatWroteNothing_DoesNotRecycleAndSaysWhy fails; the control (AnInstallThatWroteSomething_StillRecycles) and the three no-root cases pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The existing XML doc block for the recycle operation is now attached to the wrong member (and SettleRetypedRoot loses its intended XML doc), making generated docs misleading.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the package installer’s “retyped-root” recycle behavior so the root is only recycled when the install actually performed work (i.e., wrote nodes), and adjusts the recycling log message to avoid asserting guarantees the code does not keep.
Changes:
- Adds a pure helper (
RecycleDeclineReason) and gatesSettleRetypedRootonresult.Written > 0, logging an explicit decline reason on no-op installs. - Updates the recycle log message to accurately describe quiesce behavior (pending work may be force-cancelled and surface as
HubDisposedBeforeResponseException). - Adds a focused regression/control test suite for the new decline rule and a corresponding “What’s New” entry.
File summaries
| File | Description |
|---|---|
src/MeshWeaver.PluginCatalog/PackageInstaller.cs |
Adds the decline rule helper, gates root recycling on actual writes, and corrects the recycling diagnostic text. |
test/Memex.Portal.Shared.Test/RetypedRootRecycleNeedsAJobTest.cs |
Pins the rule behavior with regression/control cases and null/whitespace root-path cases. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-09-a-teardown-with-no-job-no-longer-happens.md |
Documents the user-visible operational change and the corrected log semantics. |
Review details
Suppressed comments (1)
src/MeshWeaver.PluginCatalog/PackageInstaller.cs:1606
SettleRetypedRootno longer has an XML doc comment directly above it (the existing doc block was pushed up by the new helper), which makes generated docs harder to follow for this method. Re-add at least a brief summary directly above the method.
private static IObservable<Unit> SettleRetypedRoot(
IMessageHub hub, string? rootPath, IReadOnlyCollection<MeshNode> nodes, int written,
ILogger? logger)
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// skipped), and it WAITS for the root to answer before the install proceeds. Called only when | ||
| /// the placeholder dance actually ran, i.e. when there is a placeholder binding to replace.</para> | ||
| /// </summary> |
Test Results (shard 0)254 tests 254 ✅ 2m 15s ⏱️ Results for commit d41f276. |
Test Results (shard 3)389 tests 389 ✅ 45s ⏱️ Results for commit d41f276. |
Test Results (shard 1)496 tests 496 ✅ 42s ⏱️ Results for commit d41f276. |
Test Results (shard 4)1 861 tests 1 861 ✅ 2m 28s ⏱️ Results for commit d41f276. |
Test Results (shard 2)1 142 tests 1 140 ✅ 3m 41s ⏱️ Results for commit d41f276. |
Test Results (shard 5) 6 files 6 suites 4m 39s ⏱️ Results for commit d41f276. |
Test Results 17 files 17 suites 14m 33s ⏱️ Results for commit d41f276. |
Refs #3510. 🚨 This does not close it — it removes the class of these teardowns that could
accomplish nothing, and corrects a diagnostic that was measurably lying. The case where an install
did write is untouched and is what keeps the issue open.
The disposer is named; this is the half of it that is pure loss
#3510's central question was answered by measurement in the thread: the disposer is
PackageInstaller.SettleRetypedRoot— the installer's own retyped-root recycle — read off #3603's"THE RECYCLE NAMES ITSELF" line on CD run 34190841613 (2026-09-08). Two facts in that same trace are
the subject here:
Videologged0 written, 21 unchangedand recycled its root anyway;Chesswas recycled twice per bake, each teardown killing aPluginGatingreconcile pass.
The inconsistency, in one chain
SettleRetypedRootexists for exactly one purpose: after the root's in-package NodeType has beenrebuilt, tear the root down so the hub that comes back binds the package's own configuration
instead of the fallback. An install that wrote nothing rebuilt nothing — and the code already knows
that, twice, on either side of the recycle:
retypedRootis non-null whenever the package has a root with a non-static NodeType(
placeholderRoot is not null ? root?.Path : null) — which is true on every install of such apackage, including a re-install that changed nothing. So the recycle ran unconditionally between two
neighbours that both refuse to do anything on a no-op install.
The teardown was not free. It cut off the correlated work in flight beneath the root — the root's own
PluginGatingreconcile — which the quiesce could not answer and force-cancelled at its 2 s bound,reaching the issuer as
HubDisposedBeforeResponseException.The fix is to make the recycle agree with its neighbours, not to widen a bound.
RecycleDeclineReason(rootPath, written)is the rule, pure, and the decline is said — a silentskip and a skip nobody chose read identically in a log.
And the log line stopped over-promising
It ended: "Work in flight beneath this root is answered by the teardown, not abandoned."
Measured false in the same trace — the pending
CreateOrUpdateNodeRequestwas neither answered norNACKed; it was force-cancelled at the quiesce bound. A diagnostic that asserts a guarantee the code
does not keep is worse than one that says nothing: it sends the next reader looking for a different
cause, which is precisely what the archaeology on CD 7950 had to undo. It now says what happens —
work gets the quiesce window, and anything pending at that bound is force-cancelled and surfaces as
HubDisposedBeforeResponseException.Tests
RetypedRootRecycleNeedsAJobTest— 5 cases, 22 ms, pure:AnInstallThatWroteNothing_DoesNotRecycleAndSaysWhy— the regression case, and it asserts thesentence, not just the decision;
AnInstallThatWroteSomething_StillRecycles— the control. Without it, a predicate thatdeclined every recycle would pass the case above while silently re-breaking the binding the
recycle exists to fix (PackageInstaller launches every NodeType compile fire-and-forget, then deliberately disposes the root hub those compiles are reading #1732);
its own noise.
Falsified: make
RecycleDeclineReasonnever decline and onlyAnInstallThatWroteNothing_DoesNotRecycleAndSaysWhyfails; the other four still pass.🚨 The test lives in
Memex.Portal.Shared.Testrather than beside the installer because the rule isan
internalofMeshWeaver.PluginCatalog, and that is the only test project in this repositorywhich both references the assembly and appears in its
InternalsVisibleTo(
MeshWeaver.PluginCatalog.Testlives in MeshWeaver.Plugins). Said out loud rather than left as apuzzle.
What this does NOT fix — and why #3510 stays open
An install that did write still recycles a root whose background pipeline is issuing correlated
requests, and the quiesce still force-cancels whatever is outstanding at 2 s. The thread's own
reading names the deeper half: the trace reads
RECEIVED runLevel=Quiescing@Chess, i.e. the rootkept taking correlated work from its own pipeline while being torn down under it — a hub accepting
new work past quiesce, which is its own defect and its own family. Merging that into this change
would be conflating two mechanisms, and widening the 2 s window would be the band-aid this
repository refuses.
Verification
dotnet build -c Release -warnaserror, one project per invocation:MeshWeaver.PluginCatalog,MeshWeaver.Documentation,Memex.Portal.Shared.Test,MeshWeaver.Documentation.Test—0 Warning(s), 0 Error(s) each. Tests below.
Docs
What's New: A teardown with no job no longer happens (
Category: Fix).