Skip to content

The installer started a teardown and handed back without waiting for it - #1153

Merged
rbuergi merged 1 commit into
mainfrom
fix/installer-publish-after-recycle
Aug 10, 2026
Merged

rbuergi merged 1 commit into
mainfrom
fix/installer-publish-after-recycle

Conversation

@rbuergi

@rbuergi rbuergi commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

This greens main. StaleStampRootBindingTest has been red on main in both its cases since it
merged (#1146, 12:54 today) — the PR itself was green, on a merge-ref whose tree main had already
moved past. One defect, two victims, so this is one fix rather than two.

The defect

UpsertNodes recycled the retyped partition root with a fire-and-forget DisposeRequest at write
time. That is wrong twice over, and the two compound.

Mistimed. At that point the in-package NodeType still advertises the compile stamp the node repo
committed, so the re-activation the recycle invites binds the fallback configuration for the
new hub's lifetime — precisely what #1101 stopped the warm from doing, arriving by the other door.

Outlives the install. Install returned while that teardown was still running, so the next touch
of the root landed mid-teardown and got HubDisposingException. Both red tests are exactly that, and
the log names them:

  • cannot create '/content/' — the package that ships assets
  • cannot create 'LayoutAreaReference { Area = Tests }' — the one that does not

In both, the handler's reply died with the hub, so the caller had nothing to wait for and burned its
full 60 s request timeout on work that never happened. Whether a package's files arrived came down to
which side of that dead heat the machine fell on — which is why the same package installed cleanly on
one portal and half-empty on another.

The fix

The recycle moves to where it can do its job. SettleRetypedRoot waits for the in-package type's
rebuild (MayPublishIntoRoot is that wait, and it answers promptly when the rebuild produced nothing
loadable — nothing worth rebinding to, so the recycle is skipped and the rebind watcher takes it once
the type compiles), then posts the DisposeRequest, then waits for the root to answer.

A PingRequest is the whole primitive and ordering is why it works: it is enqueued on the root
behind the DisposeRequest, so it cannot be answered by the hub that is going away — only by the one
that comes after, and answering it is that re-activation. A rejected or undelivered ping means the
teardown is still running, so it retries; the budget is bounded and its Timeout is a graceful sink
that lets the install finish rather than wedging. SyncPackageContent keeps the same wait for the call
sites that do not run SettleRetypedRoot — one ping when the root is already up.

Verification

  • MeshWeaver.PluginCatalog.Test 236/236, from 235/1.
  • The suite drops from 3 m 17 s to 1 m 19 s — the stalls it was absorbing were these 60 s timeouts.
  • dotnet build -c Release -p:CIRun=true -warnaserror clean.

Ruled out along the way, so it does not get re-guessed: the no-content case is not the NodeType-ALC
eviction of #1148 — it fails identically with that fix applied.

What's New entry included (Category: Fix).

🤖 Generated with Claude Code

StaleStampRootBindingTest has been red on main in BOTH its cases since it merged (#1146,
2026-08-10). One defect, two victims — and the same one, so this is one fix, not two.

UpsertNodes recycled the retyped partition root with a fire-and-forget DisposeRequest at write
time. That is wrong twice over, and the two compound.

MISTIMED. At that point the in-package NodeType still advertises the compile stamp the node repo
COMMITTED, so the re-activation the recycle invites binds the FALLBACK configuration for the new
hub's lifetime — precisely what #1101 stopped the warm from doing, arriving by the other door.

OUTLIVES THE INSTALL. Install returned while that teardown was still running, so the next touch of
the root landed mid-teardown and got HubDisposingException. Both red tests are exactly that, and
the log names them: "cannot create '/content/'" for the package that ships assets, "cannot create
'LayoutAreaReference { Area = Tests }'" for the one that does not. In both the handler's reply died
with the hub, so the caller had nothing to wait for and burned its full 60 s request timeout on
work that never happened. Whether a package's files arrived came down to which side of that dead
heat the machine fell on — which is why the same package installed cleanly on one portal and
half-empty on another.

The recycle now runs where it can do its job. SettleRetypedRoot waits for the in-package type's
rebuild (MayPublishIntoRoot is that wait, and it answers promptly when the rebuild produced nothing
loadable — there is nothing worth rebinding to, so the recycle is skipped and the rebind watcher
takes it once the type compiles), THEN posts the DisposeRequest, THEN waits for the root to answer.

A PingRequest is the whole primitive and ORDERING is why it works: it is enqueued on the root
BEHIND the DisposeRequest, so it cannot be answered by the hub that is going away — only by the one
that comes after, and answering it IS that re-activation. A rejected or undelivered ping means the
teardown is still running, so it retries; the budget is bounded and its Timeout is a graceful sink
that lets the install finish rather than wedging. SyncPackageContent keeps the same wait for the
call sites that do not run SettleRetypedRoot — one ping when the root is already up.

MeshWeaver.PluginCatalog.Test 236/236, from 235/1. The suite also drops from 3 m 17 s to 1 m 19 s:
the stalls it was absorbing were these 60 s timeouts.

Ruled out along the way, so it is not re-guessed: the no-content case is NOT the NodeType-ALC
eviction of #1148 — it fails identically with that fix applied.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 10, 2026 15:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a race in PackageInstaller where a retyped partition root could be recycled “fire-and-forget” and the install returned before teardown completed, causing subsequent root touches (notably content publish) to hit a disposing hub and time out.

Changes:

  • Add an ordered “recycle + wait for reactivation” step (SettleRetypedRoot) that runs after the in-package NodeType rebuild has settled.
  • Gate content publishing on root liveness (WaitForRootReady via PingRequest) so publishes don’t land mid-teardown.
  • Add a What’s New entry documenting the user-visible impact and fix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/MeshWeaver.PluginCatalog/PackageInstaller.cs Moves root recycle to a safe point in the install flow and waits for the root to come back (Ping-based), preventing mid-teardown failures and 60s timeouts.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-10-a-plugins-files-arrive-even-when-its-root-restarts.md Documents the fix and the previously observed symptoms for end users.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

701 tests  ±0   701 ✅ ±0   9m 12s ⏱️ - 4m 18s
 10 suites ±0     0 💤 ±0 
 10 files   ±0     0 ❌ ±0 

Results for commit 446e3e1. ± Comparison against base commit 269dcbc.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

1 193 tests  ±0   1 193 ✅ ±0   5m 48s ⏱️ -10s
   11 suites ±0       0 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 446e3e1. ± Comparison against base commit 269dcbc.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 350 tests  ±0   1 345 ✅ +1   10m 0s ⏱️ -23s
   11 suites ±0       5 💤 ±0 
   11 files   ±0       0 ❌  - 1 

Results for commit 446e3e1. ± Comparison against base commit 269dcbc.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

1 773 tests  ±0   1 581 ✅ ±0   6m 2s ⏱️ -18s
   11 suites ±0     192 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 446e3e1. ± Comparison against base commit 269dcbc.

@github-actions

github-actions Bot commented Aug 10, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

   11 files  ±0     11 suites  ±0   7m 22s ⏱️ - 2m 45s
1 984 tests ±0  1 876 ✅ ±0  108 💤 ±0  0 ❌ ±0 
2 161 runs  +1  2 053 ✅ +1  108 💤 ±0  0 ❌ ±0 

Results for commit 446e3e1. ± Comparison against base commit 269dcbc.

♻️ This comment has been updated with latest results.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

2 018 tests  ±0   2 016 ✅ ±0   7m 9s ⏱️ -9s
   11 suites ±0       2 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 446e3e1. ± Comparison against base commit 269dcbc.

@github-actions

github-actions Bot commented Aug 10, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

   65 files  ±0     65 suites  ±0   45m 36s ⏱️ - 8m 3s
9 019 tests ±0  8 712 ✅ +1  307 💤 ±0  0 ❌  - 1 
9 196 runs  +1  8 889 ✅ +2  307 💤 ±0  0 ❌  - 1 

Results for commit 446e3e1. ± Comparison against base commit 269dcbc.

♻️ This comment has been updated with latest results.

@rbuergi

rbuergi commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Independent verification (coordinator session driving the merge train) — ran this PR's branch merged on current main, DOTNET_PROCESSOR_COUNT=4, class runs of StaleStampRootBindingTest:

Root cause confirmed, fix necessary — but PARTIAL: 3/5 green (2 failures, both StaleStampSelfTypedRoot_RootServesItsTypesArea at exactly 2m6s). The write-time fire-and-forget DisposeRequest this PR removes is real and was tearing ShopA/ShopB down ~100 ms after activation in every decoded failure trace; with it gone, green runs drop from 1–2.5 min to 6–8 s. The residual mode: after install returns, the rebind recycle on the freshly landed build tears the root down under the client's SubscribeRequest; the transient outcome reaches the client, and then nothing ever re-probes — the change-feed latch only fires on a post-recycle node write (there is none) and the heartbeat is fire-and-forget, so the client parks the full 120 s frame wait.

The missing piece is client-side (JsonSynchronizationStream.CreateExternalClient): a bounded, NACK-triggered re-probe (6 attempts, 500 ms→8 s backoff, cancelled by keep-alive disposal; the re-sent SubscribeRequest is itself what reactivates the recycled owner — same sanctioned shape as OrleansRoutingService's transient retry, not a timer watchdog). This PR + that re-probe = 10/10 consecutive green class runs. The 94-line patch (one file) is ready — happy to hand it over or open it as a stacked follow-up the moment this merges.

Shard-3 AI.Test failures on this PR's run are FLAKE, not regression: grep -rln "PackageInstaller|AddPluginCatalog|InstallPackage" test/MeshWeaver.AI.Test/ is empty (no path from this diff to those tests); both failing classes are OUTSIDE #1075's isolation set and today (11:56Z) is their first day running parallel on a 4-vCPU runner; both classes ran 4/4 green on this branch locally. FullCrudWorkflow's mode is the post-delete read-verdict race (#1029 tombstone family) — a separate pre-existing defect. Rerunning the failed jobs now.

Note for sequencing: this PR semantically conflicts with #1154 in PackageInstaller.cs (recycle placement). Recommendation: this PR merges first (its post-rebuild recycle + message-based probe is cross-silo-correct); #1154 then rebases keeping its MessageService seams + DisposalRaceNackTest and reconciling its installer half.

@rbuergi
rbuergi merged commit 0e4cc93 into main Aug 10, 2026
28 of 30 checks passed
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
rbuergi added a commit that referenced this pull request Aug 10, 2026
…esolution reverted

The earlier 'Merge origin/main — keep both ledger accounts, adopt main's probe
design' resolved the PackageInstaller.cs conflict in the BRANCH's favour, which
silently undid #1153: SettleRetypedRoot was absent entirely and the write-time
fire-and-forget hub.Post(new DisposeRequest(), ...) was back. #1153 is what
greened main — merging that resolution would have red main again on both halves
of StaleStampRootBindingTest, and the diff read as -158 lines in a file this PR
only wants to add a comment to.

Resolution: main's side for PackageInstaller.cs, so the ordered recycle-and-wait
stands. The branch's genuine contribution to that file — the gate-run 31361446933
fingerprint (the available-areas list IS ConfigureDefaultNodeHub's set, which is
what distinguishes this from a compile failure) — is preserved on top, as is the
same enrichment in PathResolutionService.cs and all of the gate ratchet work.
rbuergi added a commit that referenced this pull request Aug 10, 2026
…so that half is theirs

#1153 landed independently while this branch was in CI, fixing the same installer race.
Its placement is strictly better and mine is deleted rather than merged:

  * RecycleRoot (mine) posted the DisposeRequest at write time and waited for the TEARDOWN
    to finish. SettleRetypedRoot (theirs) gates the recycle on the in-package type having
    a loadable build first — so the fresh hub cannot bind the fallback, the #1101 hazard my
    placement reintroduced — and then waits for the root to come BACK, not merely to die.
    Strictly more correct at both ends. Dropped mine; smaller diff wins.

Kept: the re-ask on the typed transient in the content publish. It is not redundant with
their WaitForRootReady, it is the backstop behind it — a ping probe answers "is the root up
NOW", and the rebind watcher's recycle can still land between that answer and the bytes
arriving. Proactive wait in front, reactive recovery behind.

Untouched by #1153 and the unique value here: the three MessageService seams (a tearing-down
hub that answered nobody), the SynchronizationStream re-arm for a recycle that publishes no
change-feed event, and the ContentService / content-sync handler fixes that stop flattening
a transient into a terminal failure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants