test: the owner-disposing NACK test fences on the owner ACCEPTING the patch - #5688
Merged
Merged
Conversation
… patch, not on the caller arming its watch NackReachesTheWaiterDuringTeardownTest ejected #5664 from the merge queue (run 36019287652, shard 4): watch armed at +52 ms, Mesh.Dispose() 1 ms later, owner Dead within the next poll, watch still armed, no framework warning in the window. The fence waited for LatePatchResponseRegistry.ArmedCount > 0. The caller arms that watch BEFORE it posts (#2882), so it proves the patch was SENT, not that the owner's handler ran — and the handler is what registers the disposal NACK. A dispose landing between the two leaves an owner that owes nothing; the assertion then fails on a precondition the test never established. Evidence (linux/arm64 container, --cpus=2): - 0 failures in 400+ loops (alone, 40x in one process, whole Graph.Test in bulk; at the failing commit 6b7974d and at main) - holding the post back 120 ms after the watch arms: 40/40 red with the identical assertion text - same delayed build with this fence: 40/40 green; normal build 40/40 The fence now waits for PATCH_MERGE_DISPATCHED@<owner> on the tree's request-fate ledger, stamped strictly after RegisterOwnerDisposingNack — the same "accepted is observed" wait DisposalRaceNackTest makes. The MeshWeaver.Plugins twin (TeardownTwinParityTest) must carry the same body; ported in the paired Plugins PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues remain.
Review effort: Lite
Findings: None
What changed in this PR
This pull request hardens a teardown race regression test by waiting for confirmed owner acceptance and documents the synchronization rule.
Changes:
- Fences on
PATCH_MERGE_DISPATCHEDafter NACK registration. - Documents why an armed watch does not prove owner acceptance.
| File | Description |
|---|---|
test/MeshWeaver.Graph.Test/NackReachesTheWaiterDuringTeardownTest.cs |
Adds the owner-acceptance synchronization fence. |
src/MeshWeaver.Documentation/Data/Architecture/TeardownVerdictsAreCausal.md |
Records the race and synchronization guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
NackReachesTheWaiterDuringTeardownTest.OwnerDisposingUnderMeshTeardown_StillAnswersTheWaitingCallerejected #5664 from the merge queue (run 36019287652, job 107701388653, shard 4). This fixes the test's own synchronisation. There is no product change.Root cause
The fence before
Mesh.Dispose()waited forLatePatchResponseRegistry.ArmedCount > 0. The caller arms that watch before it posts (#2882: register, then post), so an armed watch proves the patch was sent. It does not prove the owner's patch handler ran, and that handler is what registers theOwnerDisposingdisposal NACK. If the dispose lands between the two, the patch reaches an owner that never registered a NACK. The owner goesDeadowing nothing, and the assertion fails on a precondition the test never established.The CI trace fits this. The watch was armed at +52 ms, dispose ran 1 ms later, the owner was
Deadwithin the next 50 ms poll, and the window held no framework warning.Evidence (linux/arm64 container,
--cpus=2)6b7974d8and atmainFix
The fence now waits for
PATCH_MERGE_DISPATCHED@<owner>inMesh.DescribeRequestFate(armedId). The handler stamps that stage strictly afterRegisterOwnerDisposingNack. This is the same "accepted is observed, never assumed" waitDisposalRaceNackTestmakes onENQUEUED@<victim>. The finding is recorded inDoc/Architecture/TeardownVerdictsAreCausal→ "A third: an ARMED watch is not an ACCEPTED patch".Cross-repo
MeshWeaver.Plugins'TeardownTwinParityTestcompares this body against its twinsrc/MeshWeaver.Hosting.Monolith.Test/NackReachesTheWaiterDuringTeardownTest.cs. The twin needs the same body, so it gets a paired Plugins PR that lands after this one.Pairs-with: none — no public surface is removed (test + doc only)
🤖 Generated with Claude Code