fix(#572): close the ShardStatusWatcher lost-wakeup race rather than narrowing it - #573
Merged
Conversation
#568 gave ShardStatusWatcher a snapshot re-check so a state published before the wait was set up could still be found. It narrowed the window but could not close it, because the snapshot it re-read was written by the very publication walk it was racing. ShardStateTracker subscribed itself first and updated its state map from OnNext on the block's consumer thread, so a watcher could subscribe too late for the walk AND read the snapshot before that walk had recorded the state. Neither path saw it. With nothing further published for that shard -- exactly a high water agent that has reached the head and has nothing left to detect -- the wait could only end in a timeout, however generous. Marten's HighWaterAgentTests.skips_multiple_gaps_and_keeps_advancing hit this reproducibly on 2.36.0 and never on 2.35.0, because the new snapshot path let the earlier waits complete instantly and the test raced ahead into the window. Two changes close it: PublishAsync now records the state synchronously, before posting to the block. Delivery to listeners stays asynchronous, but "what the tracker knows" is now synchronous with the caller, so a snapshot read can never lag a publication that has already happened. That also removes the second writer of the state map -- the consumer thread replaying an older state could momentarily walk it backwards. Subscribing and reading the snapshot is now one atomic step (SubscribeAndCaptureCurrentStates) taken under the same lock that records the state and captures the listener list for a walk. That totally orders the two sides: either the watcher wins the lock and the following walk sees it as a listener, or the publication wins and the state is in the snapshot handed back. Observers are still notified outside the lock, so user code in OnNext cannot deadlock the tracker by publishing or subscribing. The listener list and state map were also both read-modify-write on plain fields, so two concurrent Subscribe calls could drop one of the subscribers outright -- a permanently lost wakeup rather than a delayed one. All mutations now take the lock, and HighWaterMark is read and written volatile. Reported from JasperFx/marten#5054. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 30, 2026
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 #572.
The remaining hole after #568
#568gaveShardStatusWatchera snapshot re-check so a state published before the wait was set up could still be found. It narrowed the window but could not close it, because the snapshot it re-read was written by the very publication walk it was racing.ShardStateTrackersubscribed itself as the first listener and updated_statesfrom its ownOnNext, on the block's consumer thread. So:publishcaptures the listener list and begins the walk;Subscribe— too late for that walk, noOnNext;CurrentStates()— but the tracker's ownOnNexthasn't run yet, so the state isn't in the snapshot either.Both paths miss. If nothing further is ever published for that shard — exactly a high water agent that has reached the head and has nothing left to detect — the wait can only end in a timeout, however generous. That is why raising Marten's per-step budget from 2s to 10s didn't help: the wakeup was lost, not late.
The fix
PublishAsyncrecords the state synchronously, before posting to the block. Delivery to listeners stays asynchronous, but "what the tracker knows" is now synchronous with the caller, so a snapshot read can never lag a publication that has already happened. This also removes the second writer of the state map — the consumer thread replaying an older state could momentarily walkCurrentStatebackwards.Subscribe-then-snapshot is now one atomic step (
SubscribeAndCaptureCurrentStates) taken under the same lock that records the state and that captures the listener list for a walk. That totally orders the two sides: either the watcher wins the lock and the following walk sees it as a listener, or the publication wins and the state is in the snapshot handed back. There is no longer an interleaving where neither path sees it.Observers are still notified outside the lock, so user code in
OnNextcannot deadlock the tracker by publishing or subscribing from a callback.Incidental, and a real bug on its own:
_listenersand_stateswere both read-modify-write on plain fields. Two concurrentSubscribecalls could drop one subscriber outright — a permanently lost wakeup rather than a delayed one. All mutations now take the lock, andHighWaterMarkis read/written volatile.Tests
Five new tests in
ShardStateTrackerTests. Three of them fail againstmainas it stands:watcher_sees_a_state_that_is_published_but_not_yet_delivered— parks the delivery thread inside an observer so the next publication is provably queued and un-walked, then sets up the wait. Nothing can arrive atOnNextwhile the gate is shut, so the watcher must find it in the snapshot or burn its whole timeout on a mark already reached. This is the deterministic form of ShardStatusWatcher can still miss an already-published state after #568 (lost wakeup, not a late one) #572.wait_for_shard_condition_sees_a_state_that_is_published_but_not_yet_delivered— same, throughWaitForShardCondition.concurrent_subscribers_are_never_silently_dropped— 100 subscribers registering in parallel must all be notified.Plus two that document the invariant the fix rests on: the state map is never behind a publication the caller has already made, and never regresses to an older state still in flight.
EventTests(642) andEventStoreTests(72) are green.@JasperFx/marten#5054 — happy to have this validated against the Marten suite before it ships.
🤖 Generated with Claude Code