fix(#3333): a settled assertion has already unsubscribed by rbuergi · Pull Request #3336 · Systemorph/MeshWeaver

rbuergi · 2026-09-05T02:45:08Z

Fixes the change-feed half of #3333 — and, because the defect is in shared test infrastructure,
closes the same race for every assertion in the fleet.

The defect

ReactiveWait.First is what every terminal assertion in MeshWeaver.Reactive.Assertions is built
on. It disposed its subscription as a continuation on the settled task, and a continuation
structurally cannot get there first:

the task carries RunContinuationsAsynchronously, so TrySetResult queues the awaiting test
the instant it runs;
every call site builds …Take(1).ToList().Timeout(…), and Take(1) disposes its upstream from
inside ForwardOnCompleted — i.e. after it has pushed the value down into the handler.

So the unsubscribe was always last, by a handful of instructions, and the awaiting test resumed while
the assertion was still attached to the stream it was about to measure. On a quiet machine the
producer wins that race. Under six concurrent shards it does not.

What it broke

MeshNodeStreamCache.ReleaseIfUnwatched refuses an entry that still has a live subscriber
(Entry.TryMarkIdleEvicted). At ChangeFeedResetReleasesUpstreamTest.cs:178 the only subscriber left
was the assertion on line 124 — the one that had just returned. The release returned false and
the control arm failed on a state it had never observed (run
33937167117, shard 4).

This was latent for every assertion whose subject is "is anything still watching this?" — a
refcounted cache entry, a released claim, an idle sweep that may only reap an unwatched path. Not
just this one test.

The fix

Dispose inside the handler, ahead of the settle, so "the wait settled" implies "the wait
unsubscribed".

A source that emits synchronously during Subscribe is covered by construction: the caller has no
task to await until First returns, and SingleAssignmentDisposable disposes the real subscription
on assignment — before that return. The late-fault behaviour is unchanged: a fault the task can no
longer carry is still traced, never orphaned onto the finalizer as an UnobservedTaskException.

The XML doc claimed the opposite ("the subscription deliberately OUTLIVES the returned task"). It
predated disposal being added at all and contradicted the body; rewritten to state the contract tests
may now rely on.

The guard

AssertionUnsubscribesBeforeItSettlesTest fails on the unpatched code and passes on the fix:

Expected value to be 0 because the wait had already unsubscribed when it settled …, but found 1.

It widens the window deliberately — the real one is a few instructions, so on a quiet machine a guard
written without that would have passed while the defect was present. A guard that cannot fail is not
a guard.

Verification (local, Release, `-warnaserror`)

suite	result
`MeshWeaver.Testing.Xunit.Test`	21/21 (incl. the new guard)
`MeshWeaver.Hosting.Test` (the failing one)	350/350
`MeshWeaver.Data.Test`	475/475
`MeshWeaver.Messaging.Hub.Test`	367/367
`MeshWeaver.Layout.Test`	445/445
`DocumentationLinkIntegrityTest`	green

1,658 tests across the suites most dependent on the helper, zero failures.

What this does NOT fix

#3333's other arm, BakeEquivalenceTest, is untouched — and its stated cause is corrected
on the issue. The
… is shutting down (RunLevel=DisposeHostedHubs) warnings land 110 ms after the assertion had
already failed (01:51:46.606 vs 01:51:46.716), so they are the harness's own
.Finally(harness.Dispose) reacting to the fault — not #2543, and not the cause. The real shape is a
read-before-write in BakeOutput.CollectOne: the node advertised a LastCompiledVersion the assembly
store did not yet hold. That one still wants an owner.

Pairs-with: none — no public type or member leaves src/; this is a method body, its XML doc, one
doc page, a What's New entry and one new test.

Refs #3333

`ReactiveWait.First` disposed its subscription as a CONTINUATION on the settled task, and a continuation structurally cannot get there first: * the task carries `RunContinuationsAsynchronously`, so `TrySetResult` QUEUES the awaiting test the instant it runs; * every call site builds `…Take(1).ToList().Timeout(…)`, and `Take(1)` disposes its upstream from inside `ForwardOnCompleted` — i.e. AFTER it has pushed the value down into the handler. So the unsubscribe was always last, by a handful of instructions, and the awaiting test resumed while the assertion was still attached to the very stream it was about to measure. On a quiet machine the producer wins that race; under six concurrent shards it does not. That is #3333's change-feed arm. `MeshNodeStreamCache.ReleaseIfUnwatched` refuses an entry with a live subscriber (`Entry.TryMarkIdleEvicted`), and at ChangeFeedResetReleasesUpstreamTest.cs:178 the only subscriber left was the assertion on line 124 — the one that had just returned. The release returned false and the control arm failed on a state it had never observed (run 33937167117, shard 4). The defect was latent for EVERY assertion in the fleet whose subject is "is anything still watching this?" — a refcounted cache entry, a released claim, an idle sweep that may only reap an unwatched path — not just this test. Fix: dispose inside the handler, ahead of the settle, so "the wait settled" IMPLIES "the wait unsubscribed". A source that emits synchronously during Subscribe is covered by construction — the caller has no task to await until `First` returns, and `SingleAssignmentDisposable` disposes the real subscription on assignment, before that return. The late-fault trace is unchanged: a fault the task can no longer carry is still traced rather than orphaned onto the finalizer. The XML doc still claimed the opposite ("the subscription deliberately OUTLIVES the returned task") — it predated the disposal being added at all and contradicted the body. Rewritten to state the contract tests may rely on. Guarded by AssertionUnsubscribesBeforeItSettlesTest, which FAILS on the unpatched code ("Expected value to be 0 … but found 1") and passes on the fix. It widens the window on purpose: the real one is a few instructions, so a guard written without that would have passed while the defect was present. Verified locally, Release: Testing.Xunit.Test 21/21, Hosting.Test 350/350, Data.Test 475/475, Messaging.Hub.Test 367/367, Layout.Test 445/445, DocumentationLinkIntegrityTest green. #3333's OTHER arm (BakeEquivalenceTest) is NOT fixed here and its stated cause is corrected on the issue: the "mesh shutting down mid-bake" warnings land 110 ms AFTER the assertion had already failed, so they are the harness's own `.Finally(harness.Dispose)` reacting to the fault, not #2543. Refs #3333 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The change is narrowly scoped, adds a targeted regression test for the previously-racy behavior, and the updated docs align with the new contract.

Pull request overview

Fixes a race in the reactive test assertion infrastructure where ReactiveWait.First could resume an awaiting test before its upstream subscription had been disposed, causing “is anything still watching?” assertions to intermittently measure their own just-finished wait.

Changes:

Dispose the ReactiveWait.First subscription inside the terminal notification handlers (before completing the TaskCompletionSource) to guarantee “await returned” ⇒ “unsubscribed”.
Add a regression test that deterministically widens the settle-vs-unsubscribe window and fails on the old behavior.
Update docs (architecture + What’s New) to reflect and document the strengthened contract.

File summaries

File	Description
test/MeshWeaver.Testing.Xunit.Test/AssertionUnsubscribesBeforeItSettlesTest.cs	Adds a guard test proving an awaited assertion has already unsubscribed.
src/MeshWeaver.Reactive.Assertions/ReactiveWait.cs	Moves disposal into the notification handlers to remove the settle-before-unsubscribe race.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-05-an-assertion-that-has-returned-is-no-longer-watching.md	Adds a user-facing “Fix” entry describing the behavior change and why it matters for tests.
src/MeshWeaver.Documentation/Data/Architecture/ReactiveTestAssertions.md	Documents the new guarantee (“settled assertion has already unsubscribed”) and rationale.

Review details

Files reviewed: 4/4 changed files
Comments generated: 0
Review effort level: Lite

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

github-actions · 2026-09-05T02:49:24Z