Seed the departed node's inbox rows already owned (GH-3729) - #3732
Merged
Conversation
…-3729) `rows_released_after_the_listener_is_already_running_are_still_recovered` failed about 40% of the time, and the flake was entirely in its own setup. The rows were stored claimable (`OwnerId = TransportConstants.AnyNode`) and only reassigned to the out-of-cluster node a database round-trip later: var seeded = await seedDormantMessagesAsync(listener, 5); await store.ReassignIncomingAsync(departedNode, seeded); The listener's recovery loop polls every `ScheduledJobPollingTime`, which this test sets to 250ms. A sweep landing between those two statements claimed the rows -- perfectly correct behaviour, they were owned by AnyNode at that instant. But the very next assertion exists to prove the listener leaves other nodes' rows alone: tracking.Count.ShouldBe(0, "The listener must not touch inbox rows that are still owned by another node"); So the test failed on rows it had itself published as claimable, not on any product behaviour. Worth being explicit that the ownership check was never at fault. `seedDormantMessagesAsync` now takes the owner to persist the rows WITH -- `StoreIncomingAsync` writes `envelope.OwnerId` straight through (DatabasePersistence.cs:78) -- so the rows are owned by the departed node from the moment they exist and the window is gone. The parameter is required rather than defaulted because `TransportConstants.AnyNode` is a static readonly field, not a constant, so it cannot be a default parameter value; both call sites now say which owner they mean. Verified: 12 consecutive green runs of the class (~12s each). At the previous ~40% rate that is p ~ 0.002. `dotnet build wolverine.slnx -c Release` clean with 0 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
This was referenced Jul 30, 2026
Open
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.
Fixes #3729.
multi_node_exclusive_listener_recovery.rows_released_after_the_listener_is_already_running_are_still_recoveredfailed roughly 40% of the time, and the flake was entirely in its own setup. The ownership check was never at fault — worth saying plainly, because #3729 raised the possibility that this was a real ownership race.The window
The rows were stored claimable and only reassigned to the out-of-cluster node a database round-trip later:
The listener's recovery loop polls every
ScheduledJobPollingTime, which this test deliberately sets to 250ms. A sweep landing between those two statements claimed the rows — entirely correct, they were owned byAnyNodeat that instant. But the next assertion exists to prove the opposite:So the test failed on rows it had itself published as claimable.
The fix
seedDormantMessagesAsyncnow takes the owner to persist the rows with.StoreIncomingAsyncwritesenvelope.OwnerIdstraight through (DatabasePersistence.cs:78), so the rows belong to the departed node from the moment they exist and there is no window at all.The parameter is required rather than defaulted because
TransportConstants.AnyNodeis astatic readonlyfield, not a constant, so it cannot be a default parameter value (CS1736). Both call sites now state which owner they mean, which reads better here anyway — the two tests want opposite things.Verification
12 consecutive green runs of the class, ~12s each. At the previous ~40% failure rate that is p ≈ 0.002.
Worth recording how nearly this went wrong: my first attempt didn't compile (the CS1736 above), and the 10 runs I did against the stale binary came back 4/10 failed — which looks exactly like "the fix didn't work" rather than "the fix was never built." Always confirm
Build succeededbefore believing a test result.dotnet build wolverine.slnx -c Release: clean, 0 warnings.🤖 Generated with Claude Code