fix(#2778): the OwnerDisposing NACK is handed to the waiter, not posted and hoped for by rbuergi · Pull Request #2868 · Systemorph/MeshWeaver

rbuergi · 2026-08-31T02:36:59Z

Closes #2778.

The owner mints an explicit "this patch was NOT applied — safe to retry" NACK when it goes down mid-write. Delivering it is the entire point of minting it. It was delivered like this:

var parent = hub.Configuration.ParentHub;
if (parent is not null && parent.RunLevel < MessageHubRunLevel.DisposeHostedHubs)
    parent.Post(resp, o => o.ResponseFor(request));

With the levels ordered Starting, Started, Quiescing, DisposeHostedHubs, …, that guard is open for the first three and shut for the rest — so the one moment a whole batch of owner hubs goes down with patches in flight is the one moment none of their NACKs is delivered. The caller then burns the full 31 s WriteVerdictBound waiting for an answer that had already been written and thrown away.

The guard justified itself with "during a whole-mesh teardown the parent is past that mark too, the post is skipped, and nobody is waiting." That is not something the code could verify, and it is false exactly when it matters.

The post was never the seam this NACK was designed to arrive on

A verdict that lands after UpdateRemote's ~2 s bounded wait has no pending Observe callback left. So posting it is only a way of reaching LatePatchResponseRegistry the long way round — routed to the caller, into the cache hub's PatchDataResponse handler, and finally Dispatch. That handler's own comment already names this NACK as the thing it exists for:

"the ONLY place a post-optimistic-emit verdict (above all the OwnerDisposing disposal NACK) can still be observed"

Within one mesh the registry is a singleton both hubs already share, so the owner can hand the verdict over directly: same waiter, no hub woken, no message routed. And Dispatch returns whether a caller was armed — so "nobody is waiting" stops being an assumption and becomes a checked fact.

ILatePatchVerdictSink, new in MeshWeaver.Data.Contract. The owner side lives in MeshWeaver.Data, which cannot reference MeshWeaver.Mesh.Contract — that closes the cycle Data → Mesh.Contract → Layout → Data. PatchDataResponse already lives in Data.Contract, so the verb belongs there too.
LatePatchResponseRegistry implements it — its existing Dispatch already has the signature. Registered as a factory over the concrete singleton, never a second AddSingleton: two instances would mean the owner dispatching into a registry nobody armed, which reads exactly like "nobody was waiting" and would restore the bug in a form no test could see.
The parent post stays, unchanged, as the only route to a caller in another process, which this registry cannot see.

Cost — why the obvious fix was reverted, and why this one is affordable

A previous attempt walked the parent chain to the first ancestor that could still post. It is correct about the caller and was measured at ~10× on teardown, so it was reverted. This shape is a dictionary lookup that misses, so the common teardown — nobody waiting — pays nothing.

Measured on one box, one variable at a time, same 166 tests, DOTNET_PROCESSOR_COUNT=4:

variant	duration	tests
baseline (main-equivalent worktree, measured now)	24 s	166/166
this branch	25 s	166/166
the rejected ancestor walk	176 s	(prior session)

The test, and two things it taught me

NackReachesTheWaiterDuringTeardownTest disposes the mesh, not just the node hub. Disposing only the owner leaves the parent at Started, where the old guard was open — which is why OwnerDisposalNackTest and LateNackReenqueueTest both pass with the defect fully present, and why it survived this long.

🚨 It asserts the armed watch is consumed, not that the caller's callback runs. My first draft asserted the caller's terminal and failed with the fix in place. The reason is sound: a mesh teardown takes the caller's own subscription down along with the owner, so observer.OnError lands on a disposed subscription and is a silent no-op. That is a property of the scenario, not of the framework — asserting it would have meant asserting something no implementation could satisfy. The registry entry is armed while a caller waits and is removed by Dispatch, so it returning to zero is "the verdict reached the waiter". The bound is well under the 30 s watch bound, because an entry that merely expires also ends at zero.

The failing draft is also what proved the fix works end to end: LATE_NACK_REENQUEUE … code=OwnerDisposing appears in its log, which the pre-fix code cannot produce at all.

Mutation-proved: with the direct hand-off removed, the new test fails on a timeout. All 9 sibling NACK / late-watch tests stay green — and LateNackReenqueueTest now exercises the new path, since its caller is armed.

MeshWeaver.Data.Contract, MeshWeaver.Data, MeshWeaver.Mesh.Contract, MeshWeaver.Hosting, MeshWeaver.Hosting.Monolith.Test, MeshWeaver.Content.Test — each -c Release -warnaserror, 0 Warning(s) 0 Error(s).

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

🤖 Generated with Claude Code

…ed and hoped for RegisterOwnerDisposingNack delivered the NACK as `parent.Post(...)` guarded by `parent.RunLevel < DisposeHostedHubs`. With the levels ordered Starting, Started, Quiescing, DisposeHostedHubs, … that guard is open for the first three and shut for the rest — so the one moment a whole BATCH of owner hubs goes down with patches in flight is the one moment none of their NACKs is delivered. The caller then burns the full 31 s WriteVerdictBound for an answer the owner had already minted and discarded. The guard justified itself with "during a whole-mesh teardown the parent is past that mark too, the post is skipped, and NOBODY IS WAITING". That is not something code can verify, and it is false exactly when it matters. 🚨 The post was never the seam this NACK was designed to arrive on. A verdict landing after UpdateRemote's ~2 s bounded wait has NO pending Observe callback, so posting it is only a way of reaching LatePatchResponseRegistry the long way round — routed to the caller, into the cache hub's PatchDataResponse handler, and finally Dispatch. That handler's own comment already names this NACK as what it exists for. Within one mesh the registry is a singleton both hubs share, so the owner can hand the verdict over directly. `Dispatch` also RETURNS whether a caller was armed, so "nobody is waiting" stops being an assumption and becomes a checked fact. - ILatePatchVerdictSink, new in MeshWeaver.Data.Contract. The owner side lives in MeshWeaver.Data, which cannot reference MeshWeaver.Mesh.Contract — that closes the cycle Data → Mesh.Contract → Layout → Data. PatchDataResponse already lives in Data.Contract, so the verb belongs there too. - LatePatchResponseRegistry implements it (its existing Dispatch already has the signature); registered as a FACTORY over the concrete singleton, never a second AddSingleton — two instances would mean dispatching into a registry nobody armed, which reads exactly like "nobody was waiting". - The parent post stays, unchanged and unguarded-differently, as the only route to a caller in ANOTHER process, which this registry cannot see. 🚨 COST — the reason the obvious fix was reverted. Walking the parent chain to the first ancestor that can still post is correct about the caller and was measured at ~10x on teardown (Content.Test 29 s → 176 s). This shape is a dictionary lookup that misses, so the common teardown pays nothing. Measured on one box, one variable, same 166 tests, DOTNET_PROCESSOR_COUNT=4: baseline (main-equivalent worktree) 24 s 166/166 this branch 25 s 166/166 the rejected ancestor walk 176 s (prior session) NackReachesTheWaiterDuringTeardownTest disposes the MESH — not just the node hub — because disposing only the owner leaves the parent at Started, where the old guard was OPEN: OwnerDisposalNackTest and LateNackReenqueueTest both pass with the defect fully present, which is why it survived. 🚨 It asserts the ARMED WATCH is consumed, not that the caller's callback runs. The first draft asserted the caller's terminal and failed WITH the fix: a mesh teardown takes the caller's own subscription down too, so OnError lands on a disposed observer and is a silent no-op. That is a property of the scenario, not of the framework, so asserting it would have been asserting something no implementation could satisfy. The registry entry is armed while a caller waits and removed by Dispatch, so it returning to zero IS "the verdict reached the waiter" — the step that used to be skipped. The bound is well under the 30 s watch bound, because an entry that merely EXPIRES also ends at zero. Mutation-proved: with the direct hand-off removed the new test fails on a timeout. All 9 sibling NACK / late-watch tests stay green — and LateNackReenqueue now exercises the new path, since its caller is armed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes a teardown-edge-case in the cross-hub write pipeline where an owner-minted OwnerDisposing NACK could be dropped exactly during DisposeHostedHubs, causing callers to wait out the full write verdict budget in silence. It introduces an in-mesh “late verdict sink” seam so the owner can hand the verdict directly to the already-armed late-watch registry (when both hubs are in the same process), while preserving the existing parent-post path for cross-process delivery.

Changes:

Add ILatePatchVerdictSink (in MeshWeaver.Data.Contract) and implement it on LatePatchResponseRegistry so late verdicts can be dispatched directly to armed waiters.
Update RegisterOwnerDisposingNack to attempt direct dispatch first and only fall back to the existing parent-post path when no in-mesh waiter is armed.
Add a regression test that disposes the mesh (not only the node hub) to hit the DisposeHostedHubs window, plus a “What’s New” fix entry.

Reviewed changes

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

Show a summary per file

File	Description
test/MeshWeaver.Hosting.Monolith.Test/NackReachesTheWaiterDuringTeardownTest.cs	New regression test proving the armed late-watch is consumed during mesh teardown.
src/MeshWeaver.Mesh.Contract/LatePatchWriteWatch.cs	`LatePatchResponseRegistry` now implements `ILatePatchVerdictSink` (direct dispatch seam).
src/MeshWeaver.Hosting/Persistence/PersistenceExtensions.cs	Registers `ILatePatchVerdictSink` as a factory over the existing `LatePatchResponseRegistry` singleton.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-31-a-write-is-answered-even-when-its-owner-goes-down.md	Adds a user-facing “Fix” entry describing the resolved hang-on-teardown behavior.
src/MeshWeaver.Data/DataExtensions.cs	Owner-disposal NACK path now tries direct verdict dispatch before attempting parent-post.
src/MeshWeaver.Data.Contract/ILatePatchVerdictSink.cs	New contract interface enabling `MeshWeaver.Data` to reach the late-watch registry without adding a reference cycle.

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

github-actions · 2026-08-31T02:43:29Z