Skip to content

Fix Mocha saga fault handling so OnReplyFault fires - #10313

Open
alisan3 wants to merge 11 commits into
ChilliCream:mainfrom
alisan3:ali/mocha-saga-fault-replies
Open

Fix Mocha saga fault handling so OnReplyFault fires#10313
alisan3 wants to merge 11 commits into
ChilliCream:mainfrom
alisan3:ali/mocha-saga-fault-replies

Conversation

@alisan3

@alisan3 alisan3 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Problem

When a saga dispatches a command with .Send(...) and that command's consumer fails terminally, the saga is never notified. OnFault() never fires, so the saga waits in its send state until its timeout, or forever if none is configured. The failure is invisible: nothing is logged, nothing is dead-lettered, and the payload is discarded.

Reproduced on main against the in-memory transport, so this is not transport specific.

Root cause

Three defects, the first two each independently sufficient.

1. The fault reply carried no headers. ReceiveFaultMiddleware.ReplyToSenderAsync was a hand-rolled copy of DefaultMessageBus.ReplyAsync that never adopted the header-copying contract the success path uses through TryCreateResponseOptions. The saga-id header stamped by the saga's send activity was dropped, so the reply could not be correlated back to the saga.

2. OnFault registered the wrong route kind. It went through OnEvent, producing a subscribe route bound to the saga's own endpoint. Fault replies are dispatched point to point to the shared reply endpoint, so the route could never select them. Nothing publishes a NotAcknowledgedEvent either, so that subscription could not fire in any configuration.

3. Unclaimed replies were discarded silently. ReplyConsumer threw PromiseNotFound, caught its own exception, and handed it to SetException, which is a no-op when no promise exists. That is why the failure produced no signal at all.

Changes

Collapse the duplicate reply path. TryCreateResponseOptions only ever read IMessageContext members, so it now accepts one, letting the fault middleware reuse it and dispatch through IMessageBus.ReplyAsync. ReplyOptions gains a MessageKind so the fault path keeps its own kind instead of forking the dispatch code. This removes the reply-endpoint lookup and pooled DispatchContext handling from the middleware, leaving one reply-dispatch implementation.

Bind saga fault transitions to the reply route. OnFault now registers through OnReply instead of OnEvent, so it becomes a saga-id gated reply route alongside OnReply and OnAnyReply and needs no dedicated machinery. The signature is unchanged; only the body and the documentation differ from main.

Reject OnAnyReply without a fault transition. A fault reply also satisfies OnAnyReply, whose transition is keyed on object, NotAcknowledgedEvent's base type, so a state declaring only OnAnyReply would transition as though a failed step had succeeded. This is rejected at initialization rather than resolved at runtime, so neither the routing conditions nor the transition lookup need a special case for faults. DuringAny().OnFault() satisfies it for the whole machine.

Return the pooled dispatch context after a successful reply. ReplyAsync returned its pooled DispatchContext only from a catch, so a reply that succeeded never gave it back and every reply allocated a fresh context. The five sibling operations on the bus all use finally. Pre-existing, but this branch routes fault replies through ReplyAsync too, so it sends more traffic through the defect. finally does not catch, so the exception still reaches the caller exactly as it did when the catch rethrew it.

Report unclaimed replies. A reply is expected to complete no promise whenever a saga route owns it, so the warning is raised only when ReplyConsumer is the sole consumer selected for the message. SetException now reports whether it matched a promise, mirroring CompletePromise.

Breaking changes

DeferredResponseManager.SetException returns bool instead of void. Source compatible, binary breaking.

No saga API is renamed. An earlier revision of this branch renamed OnFault to OnReplyFault; that was reverted, because the extensions layer names concepts over the core descriptor's channel-level primitives (OnTimeout is OnRequest<SagaTimedOutEvent>, not OnRequestTimeout), and because not covering publish is a missing feature rather than a naming boundary.

Verification

Mocha.Tests 4040/4040, Mocha.Sagas.Tests 620, Mocha.Transport.InMemory.Tests 792/792, Postgres and RabbitMQ topology suites green.

Each fix was checked against the code it replaces: reverting the fault middleware alone fails the header-propagation test 4/4, reverting the reply consumer alone fails the discarded-reply test with no warning written, and reverting the pooling fix alone fails the pooling test with 2 contexts rented against 1 returned. The exception-path test for ReplyAsync passes against the old code as well, which is what makes it evidence that only the success path changed.

Coverage added for both command shapes on the send leg, with and without a response type, and the publish leg is characterised as currently uncovered.

Scope of OnFault

It fires when the inbound message carried a ResponseAddress, which is the real discriminator rather than the verb used. Saga .Send and RequestAsync qualify; saga .Publish and a bare bus.SendAsync do not.

Three distinct failures, only the first of which has a mechanism today:

Failure Signal Reaches the saga
A command the saga sent fails downstream NotAcknowledgedEvent on the reply endpoint yes, via OnFault
A subscriber of an event the saga published fails original envelope to the error endpoint no
The saga's own transition throws original envelope to the saga's error endpoint no, and by construction it cannot notify itself

The documentation on OnFault now states both exclusions.

Follow-ups, not addressed here

  • Publish fault coverage. FaultAddress is plumbed end to end through both transports but read by nothing, which is the natural channel for it. The open question is fan-in semantics when several subscribers of one event fail.
  • A saga is never notified when its own transition fails terminally. State is rolled back correctly, but the saga stays in its waiting state and the message sits on an error queue with no saga-id header, since the event was published by someone else.
  • ReceiveRedeliveryMiddleware skips delayed redelivery whenever a ResponseAddress is present, so saga sends get weaker retry protection than publishes, for a reason that does not apply to a saga.
  • Every end-to-end test here is in-memory; a RabbitMQ saga reply/fault test would close that gap.

Saga_Should_ReceiveReply_When_SendUsedWithTypedOnReply failed twice during this work and could not be reproduced across five subsequent full runs, so it is unexplained. It reproduces on main and is not introduced here, but see #9947 for prior work on saga test flakiness.

🤖 Generated with Claude Code

alisan3 and others added 5 commits August 15, 2026 15:46
The fault reply path in ReceiveFaultMiddleware was a hand-rolled copy of
DefaultMessageBus.ReplyAsync that never adopted the header-copying contract
the success path uses. Because the inbound headers were dropped, the saga-id
header stamped by a saga's send activity was lost, so a fault reply could not
be correlated back to the saga that dispatched the failing command.

Collapse the duplicate: TryCreateResponseOptions only ever read IMessageContext
members, so it now accepts one, which lets the fault middleware reuse it and
dispatch through IMessageBus.ReplyAsync. ReplyOptions gains a MessageKind so
the fault path keeps its own kind instead of forking the dispatch code.

This removes the reply-endpoint lookup and pooled DispatchContext handling
from the middleware, leaving a single reply-dispatch implementation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OnFault registered its transition through OnEvent, which produced a
subscribe route bound to the saga's own endpoint. Fault replies are
dispatched point-to-point to the shared reply endpoint, so the route could
never select them. Nothing publishes a NotAcknowledgedEvent either, so the
subscription could not fire in any configuration.

Register it through OnReply instead, so it becomes a saga-id gated reply
route alongside OnReply and OnAnyReply and needs no dedicated machinery.

Rename it to OnReplyFault. It only covers fault replies to messages the saga
dispatched, not failures of events it published, and the old name claimed the
general concept. There is no compatibility shim: the method never fired, so no
behavior can depend on it, and this is the last point at which that is true.

A fault reply also satisfies OnAnyReply, whose transition is keyed on object,
NotAcknowledgedEvent's base type. A state declaring only OnAnyReply would
therefore transition as though a failed step had succeeded. Reject that at
initialization rather than resolving it at runtime, so neither the routing
conditions nor the transition lookup need a special case for faults.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ReplyConsumer threw PromiseNotFound, caught its own exception, and handed it
to SetException, which is a no-op when no promise exists. An unclaimed reply
was therefore acked and discarded without a trace, which is why a saga stuck
on a dropped reply gave no signal other than its own timeout.

Report it instead. A reply is expected to complete no promise whenever a saga
route owns it, so the warning is raised only when this consumer is the sole
one selected for the message. Saga replies also carry no correlation id, since
that is a per-hop routing key and is never propagated on send, so the same
ownership check covers the uncorrelated case rather than treating it as an
error on its own.

SetException now reports whether it matched a promise, mirroring
CompletePromise, so the caller can tell the two cases apart.

The try/catch remains, faulting a waiting requester on a malformed reply
rather than leaving it to time out, and still never throws: an exception here
would reach the fault middleware on the reply endpoint and fault a fault.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment added with the reply diagnostics claimed that a saga reply carries
no correlation id because one is never propagated on send. That is wrong.
PropagateCorrelationIds declines to inherit the ambient correlation id, but
DispatchContext.Initialize then generates a fresh one, so a reply does echo a
correlation id. It simply matches no promise, because only RequestAsync
registers one.

The behaviour is unaffected: an unclaimed reply is recognised by the ownership
check either way. Only the stated reason was wrong, and the branch it described
handles replies from foreign producers rather than the common case.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OnReplyFault was only exercised through a request carrying a response type. Add
the command variant, handled by SendConsumer rather than the request consumer,
which both faults and acknowledges through the same reply endpoint.

Characterise the publish leg as well, which is not covered. A published event
carries no reply address, so a failing subscriber is routed to the error
endpoint and the saga is never told, holding its state until it times out. The
saga header does survive onto the error endpoint, which is what any correlated
workaround, or a later fix, has to build on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An earlier commit on this branch renamed OnFault to OnReplyFault because it
only covers fault replies. That encoded a coverage gap into the name.

The extensions layer names concepts over the channel-level primitives of the
core descriptor: OnTimeout is OnRequest<SagaTimedOutEvent> and is not called
OnRequestTimeout. OnFault belongs to that vocabulary, and not covering publish
is a missing feature rather than a naming boundary.

The narrower name also survives neither plausible future. If publish faults are
delivered as a NotAcknowledgedEvent on the reply endpoint, the same route and
the same transition catch them and OnReplyFault becomes too narrow. If fan-in
semantics warrant a separate hook, the symmetric pair is OnSendFault and
OnPublishFault, where a channel name sits oddly beside a verb name.

The scope is documented instead. Neither a failing subscriber of a published
event nor a failing saga transition produces a reply, so neither reaches this
transition, and the summary now says so.

This removes the public API rename from the branch: OnFault keeps the signature
it has on main, and only its body and documentation change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alisan3
alisan3 force-pushed the ali/mocha-saga-fault-replies branch from 3426b39 to 1ffbbc3 Compare August 30, 2026 11:00
ReplyAsync returned its pooled DispatchContext only from a catch block, so a
reply that succeeded never gave the context back. The pool stayed empty and
every reply allocated a fresh context, which is the opposite of what the pool
is for. The five sibling operations on the bus all use finally; this one was
the exception.

Pre-existing, but this branch routes fault replies through ReplyAsync as well,
so it sends more traffic through the defect.

Dispatch middleware serializes what it needs rather than retaining the context,
which the publish and send paths already rely on, so finally is safe here for
the same reason it is safe there.

finally does not catch, so the exception still reaches the caller as it did
when the catch rethrew it. The second test pins that contract by throwing from
a dispatch middleware and asserting both the exception and the balanced pool.
It passes against the old code too, which is what makes it evidence that only
the success path changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alisan3
alisan3 force-pushed the ali/mocha-saga-fault-replies branch from c983773 to 510e1f5 Compare August 30, 2026 11:16
@alisan3
alisan3 marked this pull request as ready for review August 30, 2026 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant