Skip to content

fix(#568): close the WaitForShardState race against an already-published state - #570

Merged
jeremydmiller merged 1 commit into
mainfrom
fix/568-wait-for-shard-state-race
Jul 26, 2026
Merged

fix(#568): close the WaitForShardState race against an already-published state#570
jeremydmiller merged 1 commit into
mainfrom
fix/568-wait-for-shard-state-race

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Closes #568.

The bug

ShardStateTracker.WaitForShardState could miss the very state it was waiting for and then burn its full timeout (1 minute by default), even though the state had been published before the wait was ever set up.

Publication is asynchronous: PublishAsync posts to a Block<ShardState> and returns, and the consumer thread later walks the listener list as it stood when that delivery started. So between WaitForShardState's check of the state map and ShardStatusWatcher's subscription, the consumer can deliver the state to the listeners that exist at that moment. The watcher subscribes a moment too late, and since it only ever inspected states arriving at OnNext — never re-reading the map — the state is simply gone. If nothing else is published for that shard, the wait sits until the timeout:

System.TimeoutException : Shard Trip:All did not reach sequence number 45 in the time allowed

The window widens exactly when the machine is busy, which is why it read as a flaky test rather than a bug. This matters beyond our own tests — WaitForShardState / WaitForShardCondition / WaitForHighWaterMark are the documented way user code (and Marten's and Polecat's suites) waits on daemon progress, and "publish then wait" is the natural shape to write.

The fix

ShardStatusWatcher re-checks tracker.CurrentStates() against its condition immediately after subscribing. The state is therefore either delivered to OnNext or found in the snapshot — it can't fall between the two. TrySetResult makes the double hit harmless: whichever path wins, the other is a no-op.

That also closes WaitForShardCondition's wider gap for free — it never consulted current state at all, so a condition already satisfied by every known state still waited on the next publication.

A user condition that throws on some unrelated shard's state has always just meant "no match" (ShardStateTracker.publish swallows and logs whatever a listener throws), so the snapshot re-check preserves that contract rather than throwing out of WaitForShardCondition.

Also tightened the watcher's own bookkeeping while in there: TrySetException on the timeout and error paths, and dispose the timeout CancellationTokenSource once the wait resolves instead of leaving a live timer behind for every wait.

Tests

Seven tests in ShardStateTrackerTests. Each publishes and then waits for the delivery to land in the tracker's snapshot before waiting — the deterministic form of the race, since the state is provably already published and nothing further will be published for that shard.

Two of them isolate the bug and fail on main (each burning its full 5s timeout), pass with the fix:

  • shard_status_watcher_completes_from_the_already_published_state — straight at the watcher, since WaitForShardState's own pre-check would otherwise mask it
  • wait_for_shard_condition_does_not_hang_on_an_already_published_state

The rest guard the surrounding behavior: WaitForShardState / WaitForHighWaterMark end to end, that a wait whose state/condition hasn't happened yet still waits and then completes on publication, and that a throwing condition is still just a miss.

Full EventTests suite: 628 passed, 0 failed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QYfvCxHCUHo9MriHQocuoD

…hed state

ShardStateTracker publication is asynchronous — PublishAsync posts to a Block
and returns, and the consumer thread walks the listener list as it stood when
it started. A ShardStatusWatcher that subscribes after its state was already
delivered never sees it, because it only ever inspected states arriving at
OnNext and never re-read the tracker's state map. If nothing else was published
for that shard, the wait burned its full timeout (1 minute by default) and then
claimed the shard never reached a sequence it had reached before the wait was
even set up. The window widens exactly when the machine is busy, so it read as
a flaky test rather than a bug.

ShardStatusWatcher now re-checks tracker.CurrentStates() against its condition
immediately after subscribing, so the state is either delivered to OnNext or
found in the snapshot — never lost between the two. TrySetResult makes the
double hit harmless. That also closes WaitForShardCondition's wider gap for
free: it never consulted current state at all, so a condition already satisfied
by every known state still waited on the next publication.

A user condition that throws on some unrelated shard's state has always just
meant "no match" (ShardStateTracker.publish swallows and logs whatever a
listener throws), so the snapshot re-check preserves that instead of throwing
out of WaitForShardCondition.

Also tightened the watcher's own bookkeeping: TrySetException on the timeout
and error paths, and dispose the timeout CancellationTokenSource once the wait
resolves rather than leaving a timer per wait.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYfvCxHCUHo9MriHQocuoD
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ShardStateTracker.WaitForShardState can miss an already-published state and hang for the full timeout

1 participant