Fix the race in SideEffectGateTimeoutTests.a_timeout_that_did_not_reach_the_prior_mark_pauses_the_shard - #609
Merged
Conversation
…e_shard ShardStateTracker publishes through a Block<ShardState>, so a subscribed observer runs on the block's consumer thread and is not guaranteed to have run by the time StartAgentAsync returns. The test collected Paused states into a List and asserted on it immediately, which is a race it lost on net9 in CI while a compliance-only PR was in flight (#608). The failure message is what identifies it rather than a wrong count: Shouldly.ShouldAssertException : [ShardName: Trip:V2:All, Sequence: 60, Action: Paused] should have single item but had 1 items A one-element collection reported as failing a single-item assertion is not possible unless the collection changed between the check and the message being rendered. The list was empty when ShouldHaveSingleItem ran and held the state a moment later when Shouldly enumerated it again to build the message. Waits for the publication through a TaskCompletionSource bounded by the class's TestTimeout, which was declared for exactly this and never used. Deliberate narrowing: the exactly-one-publication assertion is gone, because "exactly one so far" cannot be asserted against an asynchronous publisher without an arbitrary sleep, and the test's subject is that a supervisor sees the pause with the right sequence and reason -- not the publication count. net9.0: the class 8/8 consecutive runs, EventTests 657/0/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VpDCvJcBDZerieJB4JEHde
Member
Author
|
Reproduced locally on |
jeremydmiller
added a commit
that referenced
this pull request
Aug 3, 2026
One conflict: main's #609 fixed a race in SideEffectGateTimeoutTests, which this branch deletes. Resolved as the delete -- #609 fixed how that suite observed a Paused-on-timeout state, and this branch retires the start-blocking timeout that produced it, so there is no longer anything for the suite to assert. Its lesson does apply here, though, and caught a real race in this branch's own a_gated_start_returns_immediately test: ShardStateTracker publishes through a Block<ShardState>, so a subscribed observer is not guaranteed to have run by the time StartAllAsync returns, and that test read the collected states straight after the call. It now waits for the Started state instead, bounded by the class TestTimeout. net9.0 and net10.0 both green: EventTests 663, the gate suites 20/20 over five consecutive net9 runs (the TFM #609 lost on). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremydmiller
added a commit
that referenced
this pull request
Aug 3, 2026
CI reproduced this on net9 as "Sequence contains no matching element" from the Last() call: ShardStateTracker publishes through a Block<ShardState>, so a subscribed observer runs on the block's consumer thread and is not guaranteed to have run by the time StartAllAsync returns. The test read the collected states straight after the call, which is the same race main's #609 fixed in the suite this branch deletes. The change was written while resolving that conflict but never staged -- `git diff HEAD~1` compares against the WORKING TREE, so the check that was supposed to confirm it landed in the merge commit was reading the uncommitted edit back to itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremydmiller
added a commit
that referenced
this pull request
Aug 3, 2026
…ath (#598, #610) (#615) * fix: never let one property getter lose a whole OptionsDescription (#590) An OptionsDescription is a diagnostic view built by calling arbitrary property getters on somebody else's configuration object, so any one of them can throw -- and losing the entire description over one bad property is a terrible trade. Reported from the field as JasperFx/wolverine#3740, where AzureServiceBusTransport.HostName threw a NullReferenceException for credential-based connections and a monitored service could consequently never build its ServiceCapabilities snapshot at all. - Skip set-only properties and indexers, neither of which can be read via PropertyInfo.GetValue(subject) (ArgumentException / TargetParameterCountException). Wolverine.Pulsar's PulsarTransport has a this[Uri] indexer, which was enough to make it undescribable. - Catch whatever a getter throws -- including from the [ChildDescription], [DescribeAsStringArray] and [DescribeAsConfigurationState] branches -- and record OptionsValue.Unreadable(...) in its place. - Report the exception TYPE only, never the message: descriptions get shipped to monitoring consoles and work hard to keep secrets out, and exception messages habitually quote the offending configuration value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RainvDHdde5JUAiGEPEgWw * feat: move the blue/green side-effect gate warm-up out of the agent start path (#598, #610) The #480 gate ran its side-effect-suppressed warm-up as a bounded replay INSIDE startContinuousShardAsync, so a shard was not started -- and therefore, to a distributing host, not assigned -- until the replay finished. #596 made that survivable; this removes it. The agent now starts immediately and carries the warm-up itself: it runs Continuous from its own persisted progress with side effects suppressed, clamps its loading ceiling to the prior version's mark so no page straddles it, and enables side effects the moment COMMITTED progression reaches that mark. Crash safety is unchanged and comes from the same place -- the trigger is "persisted progress < mark", so an interrupted warm-up resumes suppressed. Suppression is a first-class flag on the agent rather than Rebuild mode. Rebuild mode also switches on the #525 deferred-write accumulator, whose flush waits for a range reaching the agent's high-water -- which a clamped warm-up never reaches -- so a deferred window would never flush, progression would stall below the mark and the gate would never lift. The executions keep the real mode for their own bookkeeping and pass Rebuild down only for the per-slice apply, which is where side effects are raised. Also: - MaxConcurrentSideEffectGateWarmupsPerDatabase, the warm-up concurrency control both issues asked for. Waiting for a slot never blocks the agent -- it is started, assigned and heartbeating throughout. - ShardState.SideEffectsSuppressed / SideEffectGateMark, so an operator can tell "running but not emitting side effects" from "running normally" without pod logs. - The optimized-replay shortcut is now guarded on suppression. A gated fresh deploy starts Continuous at LastCommitted 0, which is exactly that shortcut's trigger, and a store replay executor replays to its own high-water rather than the gate mark. - DaemonSettings.SideEffectGateTimeout is obsolete and ignored: there is no start-blocking wait left to bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test): wait for the Started state instead of snapshotting it CI reproduced this on net9 as "Sequence contains no matching element" from the Last() call: ShardStateTracker publishes through a Block<ShardState>, so a subscribed observer runs on the block's consumer thread and is not guaranteed to have run by the time StartAllAsync returns. The test read the collected states straight after the call, which is the same race main's #609 fixed in the suite this branch deletes. The change was written while resolving that conflict but never staged -- `git diff HEAD~1` compares against the WORKING TREE, so the check that was supposed to confirm it landed in the merge commit was reading the uncommitted edit back to itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 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
1 participant
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.
Test-only. This test went red on net9 during #608 — a compliance-suite PR that touches nothing
EventTestsreferences — and passed on the rerun. It is a real race, not infrastructure noise.Diagnosis
ShardStateTrackerpublishes through aBlock<ShardState>, so a subscribed observer runs on the block's consumer thread and is not guaranteed to have run by the timeStartAgentAsyncreturns. The test collectedPausedstates into aList<ShardState>and asserted on it immediately.What pins it down is the failure message, not a wrong count:
A one-element collection reported as failing a single-item assertion is impossible unless the collection changed between the check and the message being rendered. The list was empty when
ShouldHaveSingleItemran, and held the state a moment later when Shouldly enumerated it again to build the message. So the shard did pause, correctly, with the right sequence — the test just looked too early.Fix
Wait for the publication through a
TaskCompletionSource, bounded by the class'sTestTimeout— which was declared at the top of the file for exactly this purpose and never used.One deliberate narrowing: the exactly-one-publication assertion is gone. "Exactly one so far" cannot be asserted against an asynchronous publisher without an arbitrary sleep, and the test's subject — stated in its own comment — is that a supervisor sees the pause with the right sequence and reason, not the publication count.
Verification
net9.0: the class 8/8 consecutive runs,
EventTests657 / 0 / 0.🤖 Generated with Claude Code
https://claude.ai/code/session_01VpDCvJcBDZerieJB4JEHde