fix: stop asserting which of three racing terminals answered (#1599) by rbuergi · Pull Request #1621 · Systemorph/MeshWeaver

rbuergi · 2026-08-14T20:50:49Z

Closes #1599.

Measured, both arms

branch	`flake-repro` rate mode, 60 iterations
`main`, unmodified	21 failures / 60 — 35% (run 31818285657)
this branch	0 failures / 60 (run 31838634445)

Same instrument, same iteration count, same test. It is also the largest single bucket in CI: 7 of the 26 test-attributed failures across the last 30 failed runs, 4 of them on main (sweep posted on #1384).

What it asserted, and why that could never hold

HandleGetDataRequest has three terminals — the fault arm (hosted-hub creation frozen), the empty-completion arm, and the disposal arm — and they sit behind one Interlocked CAS precisely because they race. Which one wins is deliberately not ordered; the frozen-creation fault legitimately wins once an ancestor's teardown has begun.

The test required the disposal arm's own free text:

failure.Message.Should().Contain("still outstanding",
    "this pins HandleGetDataRequest's DISPOSAL arm specifically…");

so it had to win that coin flip.

The sender-FIFO ordering the class documents is real, but it orders the read ahead of the DisposeRequest at the owner. It never ordered the three terminals against each other — and the class doc claimed the arm-specific assertion was the guard that would catch a violation, which is what made a race look like a diagnostic.

Why dropping it loses nothing

All three arms go through NackSilentRead. So all three produce:

a DeliveryFailure with ErrorType.ShuttingDown,
the prefix GetDataRequest({reference}) at '{hub.Address}': ,
the "shutting down" marker MeshNodeStreamCache.IsTransientOwnerFailure classifies on,
and the same "Retry against the fresh activation." promise.

Only the free text differs. The caller cannot tell them apart, and does not need to.

What the old assertion was for — "a routing NACK must not be able to satisfy this" — is now established directly, and better: the routing layer's failures carry ErrorType.NotFound or a bare exception message (RoutingServiceBase, MonolithRoutingService), never NackSilentRead's prefix and never the retry sentence. The test asserts both.

🚨 Why this is not the band-aid it could be mistaken for

"The test was wrong" is exactly what a band-aid sounds like, so the evidence matters: the mechanism under the test is correct — three terminals, exactly one answer, guaranteed by the CAS — and the three answers are materially identical to the caller. The defect was a test asserting an ordering the design deliberately does not provide.

Ordering the arms instead would add coupling to the read layer #1470/#1471 just reworked, for zero caller-visible difference. The bound is not the problem and neither is the mechanism; the assertion was.

The class remarks now record the 21/60 measurement and say plainly what the FIFO does and does not order, so the next person does not re-derive it from a red shard.

SilentReadNackTest.OwnerDisposedWithReadOutstanding_IsNacked_NotLeftHanging fails 21 of 60 times on unmodified main (flake-repro rate mode, run 31818285657) — 35%, by a wide margin the highest rate measured on this repo, and 7 of the 26 test-attributed CI failures in the last 30 failed runs. WHAT IT ASSERTED, AND WHY THAT COULD NOT HOLD HandleGetDataRequest has THREE terminals — the fault arm (hosted-hub creation frozen), the empty-completion arm, and the disposal arm — and they sit behind ONE Interlocked CAS precisely BECAUSE they race. Which one wins is deliberately not ordered; the frozen-creation fault legitimately wins once an ancestor's teardown has begun. The test required the disposal arm's own free text ("still outstanding") and therefore had to win that coin flip. The sender-FIFO ordering the class documents is real, but it orders the READ ahead of the DisposeRequest at the owner. It never ordered the three terminals against each other — and the class doc claimed the arm-specific assertion was the guard that would catch a violation, which is what made a race look like a diagnostic. WHY DROPPING IT LOSES NOTHING All three arms go through NackSilentRead. So all three produce a DeliveryFailure with ErrorType.ShuttingDown, the same "GetDataRequest({reference}) at '{hub.Address}': " prefix, the same "shutting down" classifier MeshNodeStreamCache.IsTransientOwnerFailure reads, and the same "Retry against the fresh activation." promise. ONLY the free text differs — the caller cannot tell them apart, and does not need to. What the old assertion was FOR — "a routing NACK must not be able to satisfy this" — is now established directly, and better: the routing layer's failures carry ErrorType.NotFound or a bare exception message (RoutingServiceBase, MonolithRoutingService), never NackSilentRead's prefix and never the retry sentence. The test asserts both. 🚨 This is not a weakened assertion, and it is not the band-aid it could be mistaken for. The mechanism under it is correct: three terminals, one answer, guaranteed by the CAS. The defect was a test asserting an ordering the design deliberately does not provide. Ordering the arms instead would add coupling to the read layer #1470/#1471 just reworked, for zero caller-visible difference — the answer is byte-identical apart from prose. Verified locally 3/3; the real gate is flake-repro rate mode on this branch against main's measured 21/60, since this test passes locally either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmBFaHTqhy7h742wkAaJKj

Copilot

Pull request overview

This PR fixes a flaky integration test by removing an assertion that depended on which of three intentionally racing HandleGetDataRequest terminal paths won the CAS, and instead asserts on the stable caller-visible contract (ErrorType.ShuttingDown plus NackSilentRead’s distinctive message prefix + retry promise).

Changes:

Updates SilentReadNackTest’s doc comments to clearly document what FIFO ordering guarantees (and what it does not), including the measured flake rate on main.
Replaces the arm-specific "still outstanding" assertion with contract-level assertions that discriminate handler NACKs from routing-layer failures without pinning a specific terminal arm.

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

github-actions · 2026-08-14T21:05:36Z