Resequence draining using PublishAsync appears problematic

[JonathanS-NTI]

I am seeing an issue with the ResequencerSaga in certain scenarios. I tried to create somewhat representative test but am not sure on the appropriate fix.

The test queues sequence 3 in Pending, then atomically publishes [1, 2, 4, 5] to a sequential local queue. When processing 2, ShouldProceed republishes 3, but that message is appended behind the existing 4 and 5. The observed result is: [1, 2, 4, 5, 3]

instead of: [1, 2, 3, 4, 5]

The root cause is that ShouldProceed drains Pending via bus.PublishAsync(next), and bus there is the current MessageContext (wired in by ShouldProceedGuardFrame.cs), so the drained message is a cascading message that doesn't flush until the current envelope finishes — landing at the back of the queue, behind messages already sitting there.

The key to a deterministic repro: you must get a backlog into the queue atomically. Publishing 1, 2, 4, 5 in a loop from the test races against the listener. Instead, return them all as an OutgoingMessages collection from a single handler — Wolverine flushes those as one batch, guaranteeing the queue holds [1,2,4,5] before any is processed.

[Copilot]

Pull request overview

Adds a deterministic regression test to reproduce a message-ordering issue when ResequencerSaga<T> drains Pending via PublishAsync while a sequential local queue already has a backlog, plus supporting test infrastructure to enqueue a batch in a controlled way.

Changes:

• Introduces EnqueueSequencedBatch + handler returning OutgoingMessages to create a controlled backlog from a single handler execution.
• Routes all published messages to a named local queue (sequenced) and configures it as strictly sequential to make ordering deterministic.
• Adds a new test asserting that a replayed pending message is processed in-sequence even with an existing backlog, and tightens existing assertions to ordered list checks.

Suppressed comments (1)

src/Testing/SlowTests/Persistence/Sagas/resequencer_saga_in_memory.cs:142

• This new test asserts that the resequencer replays the pending message (3) ahead of an already-queued backlog (4, 5), but the current implementation of ResequencerSaga.ShouldProceed republishes drained Pending items via bus.PublishAsync(next) as a cascading message (Saga.cs:116-118), which does not flush until the current envelope completes. With a sequential local queue and an existing backlog, that behavior will enqueue 3 behind 4 and 5, so this test should fail until the core behavior is changed.

If this PR is intended to land as a repro-only test, consider skipping it to keep CI green; otherwise, please include the product fix in the same PR.

    [Fact]
    public async Task replayed_pending_message_is_processed_in_sequence_behind_a_backlog()
    {
        var sagaId = Guid.NewGuid();

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jeremydmiller]

Thanks for this — the repro is exactly right, and the note about needing OutgoingMessages to get the backlog in atomically is what made it deterministic. It reproduced first try locally.

Your diagnosis is correct, and the TrackedSession records show there is a second defect behind it that would have survived fixing the one you found.

What the records show

seq 11  Sent              SequencedCommand(4)     <- 4 and 5 already queued
seq 12  Sent              SequencedCommand(5)
seq 20  ExecutionFinished SequencedCommand(2)
seq 21  Sent              SequencedCommand(3)     <- 3 sent only as 2 completes
seq 22  MessageSucceeded  SequencedCommand(2)
seq 23  Received          SequencedCommand(4)     <- and it is HANDLED, not deferred
seq 31  Received          SequencedCommand(3)     <- 3 arrives last

Your half is seq 21: the replay is a cascading message, so it is Sent between ExecutionFinished and MessageSucceeded for envelope 2, landing behind whatever was already queued.

The second defect

Instrumenting the guard:

ShouldProceed(order=2) entered: LastSequence=1 Pending=[3]
  DRAIN: republishing order=3, LastSequence advanced to 3 BEFORE it is handled
ShouldProceed(order=4) entered: LastSequence=3 Pending=[]

ShouldProceed advances LastSequence at publish time:

Pending.Remove(next);
LastSequence = next.Order!.Value;   // before `next` has been handled
await bus.PublishAsync(next);

So message 4 satisfies Order == LastSequence + 1 and executes. The counter stops meaning "what has been handled", and the guard stops guarding. Had we only moved the replay to the front of the queue, this scenario would have passed while the invariant stayed broken.

And it fails silently

When 3 finally arrives, Order (3) <= LastSequence (5) sends it down the "already processed in sequence, allow re-published messages through" branch, so it is handled. The saga ends at LastSequence=5 with Pending empty — indistinguishable from a healthy run — while ProcessedOrders is out of order. Nothing throws and nothing logs, which is why this is easy to hit in production without noticing.

Fix

Hand back one pending message and do not advance LastSequence; the replayed message's own ShouldProceed advances it and hands back the next, so the chain continues itself. Messages already queued out of order are then deferred into Pending and replayed in turn instead of sailing through — correctness stops depending on queue ordering.

One thing worth passing on about testing this

The obvious generalization does not reproduce it. A scrambled batch on its own — [5,3,1,4,2], [4,5,1,2,3], [5,4,3,2,1] — passes against the broken code. I wrote that theory first and checked it against an unfixed build before trusting it. If every out-of-order message is already in Pending by the time the gap fills, there is nothing queued behind the replay.

Your test's shape is the one that matters, and it needs both halves: a message seeded into Pending in an earlier transaction, and a higher-numbered backlog behind the message that fills the gap. That generalizes to 20/50/100-message shuffles, 10 of which fail without the fix.

What happens to this PR

I have opened #4174 to carry this forward. Your two commits are cherry-picked there with your authorship intact, so your repro leads the history and the fix builds on top of it, and your six tests are unchanged. I am closing this one in favour of that — not because anything here was wrong, but because it needs the Saga.cs change alongside your tests.

Thanks again. This was a real bug with a silent failure mode, and it would have been considerably harder to find without a repro this deterministic.

[jeremydmiller]

Superseded by #4174, which carries your commits forward with your authorship intact. See the analysis above.