Restore send ordering in BatchedSender, and order tracked records honestly (GH-3825) - #3834
Merged
Merged
Conversation
…estly (GH-3825) GH-3825 reported two of six Azure Service Bus end_to_end session tests failing deterministically. Nothing was ever lost -- every message arrived. The order was wrong, and it was wrong for two independent reasons. 1. BatchedSender lost enqueue order. The serializing stage ran at Environment.ProcessorCount, so envelopes reached the batching block in serialization-completion order rather than enqueue order. Before the switch from TPL Dataflow to Channels (8576ce5) this was an ActionBlock, whose MaxDegreeOfParallelism defaults to 1; the rewrite raised it and the ordering guarantee went with it. That silently broke the FIFO contract behind Azure Service Bus sessions, SQS FIFO message groups, and global partitioning -- for every transport that sends through BatchedSender. Pinned back to 1. Everything downstream was already serial by default (Endpoint.MessageBatchMaxDegreeOfParallelism is 1). 2. TrackedSession mis-reported the order of its own records. AllRecordsInOrder() sorted by SessionTime, which is _stopwatch.ElapsedMilliseconds -- whole milliseconds. An entire receive batch shares one value, and OrderBy then falls back to the enumeration order of _envelopes, a Guid-keyed cache with no relationship to when anything happened. Every ordering assertion written against Received/Sent anywhere in the suite was riding on that. EnvelopeRecord now carries a monotonic Sequence assigned at construction, and both AllRecordsInOrder overloads sort on it. The two were separated by recording the handler's own execution order alongside session.Received: handled was always in order, only the report scrambled. BatchedSenderTests gains a regression test that uses a serializer with descending per-envelope delays, so any parallelism in that stage reorders every run rather than occasionally. Verified red before this change and green after. CoreTests 2247/0, ASB 315/0, MartenTests 548/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHAuhdWS3XeAk16swV9G8m
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 #3825.
The issue said "session handling, 2 of 6 fail deterministically." The symptom half was right; the cause was not. Nothing was ever lost — every message arrived, in the wrong order. And it was wrong for two independent reasons.
Recording the handler's own execution order next to
session.Receivedsplit them apart:1.
BatchedSenderlost enqueue orderThe serializing stage ran at
Environment.ProcessorCount, so envelopes reached the batching block in serialization-completion order rather than enqueue order.git log -Lon the constructor shows this was not a design decision. Before the Channels rewrite (8576ce526, "First cut at switching over to Channels in place of TPL DataFlow") it was anActionBlock:TPL Dataflow's
MaxDegreeOfParallelismdefaults to 1, so that block was strictly ordered. The port raised it toEnvironment.ProcessorCountand the guarantee went with it — silently breaking the FIFO contract behind Azure Service Bus sessions, SQS FIFO message groups, and global partitioning, for every transport that sends throughBatchedSender.Demonstrated with a buffered publisher and an inline one side by side, same group id:
B1, B6, B5, B4, B2, B3B1, B4, B3, B2, B5, B6B5, B3, B4, B6, B2, B1I1..I6I1..I6I1..I6Pinned back to 1. Everything downstream was already serial by default (
Endpoint.MessageBatchMaxDegreeOfParallelismis 1).2.
TrackedSessionmis-reported the order of its own recordsSessionTimeis_stopwatch.ElapsedMilliseconds. An entire receive batch shares one value, and the stable sort then falls back to enumerating_envelopes— a Guid-keyed cache with no relationship to when anything happened. Every ordering assertion written againstReceived/Sentanywhere in the suite was riding on that.EnvelopeRecordnow carries a monotonicSequenceassigned in the constructor (so every construction site is covered), and bothAllRecordsInOrderoverloads sort on it.Verification
BatchedSenderTests.preserves_enqueue_order_through_to_the_outgoing_batchuses a serializer with descending per-envelope delays, so any parallelism in that stage reorders every run rather than occasionally. Confirmed red before the change, green after.end_to_end6/6, untagged fromCategory=Flaky.wolverine.slnx -c Releaseclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01WHAuhdWS3XeAk16swV9G8m