Fix #5065: make the recording message outbox thread safe - #5085
Merged
Conversation
blue_green_side_effect_gate was not flaky in the usual sense -- it failed 4 out
of 4 runs on clean master in about a second, so "the assertion runs before the
daemon has drained" was the wrong reading.
Instrumenting the blue leg, which has no gate at all, showed the contradiction:
fired=19 distinct=19 progress=20 trackerSeq=20 batches=1 perBatch=[19] docs=20
Progression and the tracker both reached 20 and all 20 aggregates were built, so
every slice was processed and RaiseSideEffects ran for each -- yet the single
message batch held 19, with no duplicates. That is a lost List<T>.Add, not a
suppressed side effect, and it explains the varying shortfall (19/20, 18/20,
7/8, 4/8) that made this look like a timing flake.
The contention is real and measured, not inferred: instrumenting PublishAsync
with thread ids and an in-flight counter shows up to 8 concurrent publishers
across 10 threads into one batch, for a single-stream projection catching up
over 20 streams. So the lock is not masking a timing bug -- the daemon genuinely
raises side effects concurrently.
RecordingMessageOutbox/RecordingMessageBatch in side_effects_in_aggregations.cs
appended to unsynchronized lists for both Batches and Messages. The sibling copy
in Composites/composite_rebuild_suppresses_side_effects.cs already locks both,
so this was a known hazard one copy never picked up. ProjectionUpdateBatch's
CurrentMessageBatch is properly double-check-locked, so batch creation was never
the problem.
Also documents the requirement on IMessageBatch, which is the interface Marten
users implement and said nothing about being called concurrently. In a real
outbox this trap costs silently dropped messages rather than a flaky test.
Verified: 10 consecutive runs of the class all 2/2 green (4/4 red immediately
before), and full DaemonTests on net9.0 is now 257/257 -- previously 256/257
with this as the only failure.
This was referenced Jul 30, 2026
Open
This was referenced Aug 4, 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.
Closes #5065. The issue's premise turns out to be wrong, and so was my first diagnosis on it — this is not a drain-timing flake, and not the gate bug I initially reported. It is a thread-safety defect in the test harness.
Why the "asserts before the daemon drained" reading is wrong
It fails 4 out of 4 runs on clean master in about a second. A drain race does not do that.
What the state actually shows
Instrumenting the blue leg — which has no opt-in and no gate at all, so a gate boundary cannot be involved:
Self-contradictory for any "events were missed" theory:
progress=20/trackerSeq=20— the projection committed all 20docs=20— all 20 aggregates were built, so every slice ran andRaiseSideEffectsfired for eachbatches=1 perBatch=[19],distinct == fired— the one batch holds 19 messages, no duplicatesEvery slice ran, yet a message is missing from the list: a lost
List<T>.Add. That also explains the wandering shortfall — 19/20, 18/20, 7/8, 4/8 — which is what made it read as timing.The contention is measured, not inferred
Instrumenting
PublishAsyncwith thread ids and an in-flight counter:Up to 8 concurrent publishers across 10 threads into a single batch, for one single-stream projection catching up over 20 streams. So the lock in this PR is not papering over a timing bug — the contended access is directly observed.
ProjectionUpdateBatch.CurrentMessageBatchis properly double-check-locked, so batch creation was never the problem.The fix
RecordingMessageOutbox/RecordingMessageBatchinAggregations/side_effects_in_aggregations.csappended to unsynchronizedList<T>for bothBatchesandMessages. The sibling copy inComposites/composite_rebuild_suppresses_side_effects.csalready locks both — a known hazard that this copy never picked up.Also documents the requirement on
IMessageBatch. That is the part worth keeping beyond the test: it is the interface Marten users implement, nothing saidPublishAsyncis called concurrently, and in a real outbox the symptom is silently dropped messages rather than a visibly flaky test.Verification
mdsnippetsgate clean: 22 drifted doc files, exactly the pre-existing master baseline, none of it from this change (neither file contributes snippets).No upstream issue needed
I had reported this as a JasperFx hand-off; that was wrong and I have corrected the issue thread. Nothing here is in JasperFx — the harness is Marten's,
IMessageBatchis Marten's, andCurrentMessageBatchis Marten's and already correct.🤖 Generated with Claude Code