Repository navigation
GH-4556: apply an ISendMyself published from a projection side effect, on all three stores - #4575
Merged
Merged
Conversation
MessageBus only lets an ISendMyself apply itself when no DeliveryOptions are passed -- the guard that keeps TimeoutMessage from looping. A projection side effect always passes some, at minimum the tenant id, so a ToEndpoint(), DelayedFor() or SignalR ToWebSocketGroup() published out of RaiseSideEffects was routed as the wrapper type, found no subscriber, and was dropped as NoRoutes. The bridge now spots an ISendMyself and applies it against its own ProjectionSideEffectContext: a MessageContext enlisted in the same store session, with the batch's storage, so a durable send still commits with the projection. The metadata is stamped in TrackEnvelopeCorrelation -- the one point every outgoing envelope passes through, whether it leaves via PublishAsync, EndpointFor() or a topic -- rather than mapped onto a single DeliveryOptions that only the outermost send would have seen. Anything the message set on its own options wins. A context per message rather than mutating TenantId/CorrelationId on the shared batch context for the duration of ApplyAsync(), because slices publish concurrently. The MessageBus options == null guard is left alone; it is load bearing. Three things beyond the original diagnosis: - The correlation id is only taken over when the projection actually supplied one. MessageMetadata.CorrelationId is plain null otherwise, so stamping it unconditionally stripped the correlation id that every other side effect message from the same batch carries. It now falls back to the batch context's id, which keeps a whole projection page on one correlation id. - The side effect contexts are CLEARED once flushed. Marten only discards its cached message batch after a SUCCESSFUL commit -- SaveChangesAsync does not reset it in a finally -- so a session retrying a failed commit gets the same batch back and would otherwise accumulate, and re-flush, every context from every failed attempt for as long as the session lived. The list is also lazily allocated now: a batch is built for every commit of every session in any app running IntegrateWithWolverine(), and almost none of them publish an ISendMyself. - Polecat and Fisher had the identical defect. Rather than triplicating the fix into two more byte-identical bridges, the whole behaviour moved into the store agnostic ProjectionSideEffectSink and all three IMessageBatch implementations delegate to it. The metadata-to-DeliveryOptions mapping from GH-2545 was already copy-pasted three ways, which is how GH-4556 came to be fixed on one store and broken on two. Each store's tests publish the ISendMyself and a plain message out of the same slice, so the two paths are compared inside one run -- that control is what catches the ISendMyself path drifting away from the ordinary one again. Co-Authored-By: Anne Erdtsieck <anne.erdtsieck@topicus.nl> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BroadcastToTopicAsync was the one send path that never called TrackEnvelopeCorrelation. MessageRoute.CreateForSending sets no Source, no CorrelationId, no ParentId and no Store, and nothing downstream filled them in, so a topic broadcast went out bare while every other route did not. This surfaced from GH-4556: a projection side effect's metadata rides TrackEnvelopeCorrelation, so ToTopic() was the one ISendMyself wrapper that would have been applied correctly and still carried none of its causation id or custom headers. Separable from the GH-4556 fix on purpose -- it changes behaviour for every topic broadcast in every application, not just projection side effects, so it is its own commit and can be dropped on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Supersedes #4557 and closes #4556. The diagnosis, the
ProjectionSideEffectContextdesign and theoriginal Marten test are @erdtsieck's — this branch carries their commit authorship. What is
added here is the review follow-up plus the two stores the original fix did not reach.
Problem
MessageBus.PublishAsynconly lets anISendMyselfapply itself whenoptions == null— the guardthat keeps
TimeoutMessagefrom looping. A projection side effect always passes delivery options, atminimum the tenant id, so a
ToEndpoint(),DelayedFor()or SignalRToWebSocketGroup()publishedout of
RaiseSideEffectswas routed as the wrapper type, found no subscriber, and was dropped asNoRoutes.Fix
The bridge spots an
ISendMyselfand applies it against its ownProjectionSideEffectContext: aMessageContextenlisted in the same store session, with the batch's storage, so a durable sendstill commits with the projection. The metadata is stamped in
TrackEnvelopeCorrelation— the onepoint every outgoing envelope passes through, whether it leaves via
PublishAsync,EndpointFor()or a topic — rather than mapped onto a single
DeliveryOptionsthat only the outermost send wouldhave seen. Anything the message set on its own options wins. A context per message rather than
mutating
TenantId/CorrelationIdon the shared batch context for the duration ofApplyAsync(),because slices publish concurrently.
The
options == nullguard inMessageBusis left alone; it is load bearing.What this adds over #4557
1. The correlation id no longer goes null.
MessageMetadata.CorrelationIdis plainnullunlessthe projection set one (
CorrelationIdEnabledis literallyis not null), so taking it overunconditionally stamped
nullon the envelope — stripping the correlation id that every otherside-effect message out of the same batch carries. It now falls back to the batch context's id, so a
whole projection page stays on one correlation id.
Reproduced before fixing, with a plain message published from the same slice as the control:
Every store's tests now publish the
ISendMyselfand a plain message out of the same slice, sothe two paths are compared inside a single run. That control is what catches the
ISendMyselfpathdrifting away from the ordinary one again.
2. The side-effect contexts are cleared once flushed. Marten only discards its cached message
batch after a successful commit —
DocumentSessionBase.SaveChangesAsyncdoes not reset it in afinally— so a session retrying a failed commit gets the same batch back, and without a clear itwould accumulate, and re-flush, every context from every failed attempt for as long as the session
lived. Each entry is a full
MessageContext(three envelope lists, one of which is never cleared)plus an
IEnvelopeTransactionholding the session, so that is the one shape of this that growswithout bound. The list is also lazily allocated now: a batch is built for every commit of every
session in any app running
IntegrateWithWolverine(), and almost none of them publish anISendMyself. TheAdd/iterate locking is symmetric.3. Polecat and Fisher had the identical defect.
PolecatToWolverineMessageBatchandFisherToWolverineMessageBatchwere byte-identical to Marten's down to the doc comments, and bothstill dropped every
ISendMyself. Rather than triplicate the fix, the whole behaviour moved into thestore-agnostic
ProjectionSideEffectSinkin core and all threeIMessageBatchimplementationsdelegate to it. The GH-2545 metadata-to-
DeliveryOptionsmapping was already copy-pasted three ways— which is exactly how GH-4556 came to be fixed on one store and broken on two.
The second commit changes core behaviour — it can be dropped on its own
BroadcastToTopicAsyncwas the one send path that never calledTrackEnvelopeCorrelation.MessageRoute.CreateForSendingsets noSource, noCorrelationId, noParentIdand noStore,and nothing downstream filled them in, so a topic broadcast went out bare while every other route
did not. That is why
ToTopic()was the oneISendMyselfwrapper that would have been appliedcorrectly here and still carried none of its causation id or custom headers.
Fixing it is a one-line addition that only adds metadata — but it applies to every topic
broadcast in every application, not just projection side effects, so it is deliberately its own
commit with its own test. Drop
Stamp envelope correlation on a topic broadcastif you would rathertake that separately; the GH-4556 commit stands alone without it.
Verification
Red baseline in a separate worktree built at
origin/mainwith only the new test files copiedin — all nine fail there, for the right reason:
Green on this branch: 4/4 Marten, 2/2 Polecat, 2/2 Fisher, 1/1 CoreTests.
Regression runs:
dotnet build wolverine.slnx -c Release -f net9.0Known gap, deliberately left
MessageMetadata.CurrentUserName/UserNameEnabledis still ignored — by the plain path and theISendMyselfpath alike, on all three stores. Mapping it would make theISendMyselfpath betterthan the ordinary one, so it belongs in its own change alongside the plain mapping, not here.
🤖 Generated with Claude Code