[#3728] Stop losing events appended during prepare-commit, and fix the guards that hid it

[MateuszNaKodach]

While investigating why the saga was stuck during #5009 development, the same root cause turned out to hit two other components that also register for PREPARE_COMMIT lazily. One of them loses events silently on JPA. This PR fixes those two, and repairs the test double that was hiding them.

No change to UnitOfWork or to the phase rule itself.

---

Background: the rule, and who trips over it

ProcessingContext rejects a registration for a phase whose order is at or below the phase currently running:

// UnitOfWork.UnitOfWorkProcessingContext#on
if (current != null && phase.order() <= current.order()) {
    throw new IllegalStateException("Failed to register handler in phase ...");
}

That rule is fine. The problem is that a handler is not always invoked in the phase it thinks it is. Events published with a ProcessingContext are queued by SimpleEventBus and drained during PREPARE_COMMIT, and SubscribingEventProcessor hands them to its components in that same context:

sequenceDiagram
    participant CH as Command handler
    participant UoW as UnitOfWork
    participant Bus as SimpleEventBus
    participant SEP as SubscribingEventProcessor
    participant H as Your event handler

    CH->>Bus: publish(context, events)
    Note over Bus: queued, hook registered for PREPARE_COMMIT
    UoW->>Bus: PREPARE_COMMIT
    Bus->>SEP: notify(events, context)
    SEP->>H: handle(event, context)
    Note over H: running INSIDE PREPARE_COMMIT

Loading

So any component reached that way cannot register for PREPARE_COMMIT. That is what blocked the saga write, and PR #5009 fixes it by registering for SAGA_WRITE instead. But the saga is not the only component that registers for PREPARE_COMMIT lazily.

---

Bug 1: the event store hands its batch over too early, and JPA loses the events

DefaultEventStoreTransaction collects appended events in a queue and hands the whole queue to the EventStorageEngine in a step registered on first append, for PREPARE_COMMIT. Two things go wrong.

1a. First append during PREPARE_COMMIT throws

attachAppendEventsStep() registers lazily, so a component whose first append happens in that phase hits the identical rejection the saga hit. No saga involved, plain event append.

1b. Append after the step already ran is silently dropped

If something appended earlier, the queue exists, computeResourceIfAbsent short-circuits, and the event is added to a list the engine already received. Whether it survives depends entirely on how the engine treats that list:

Engine	Behaviour	Result
InMemoryEventStorageEngine	closes over the list, re-reads it in commit()	late event is stored (by luck)
AggregateBasedJpaEventStorageEngine	forEach(em::persist) then em.flush() inside appendEvents; commit() is a no-op	late event is silently lost

Meanwhile SimpleEventBus.processEventsInPhase has already delivered that event to every subscriber. Store and subscribers diverge, with nothing logged.

sequenceDiagram
    participant H as Subscribing handler
    participant Tx as DefaultEventStoreTransaction
    participant Q as event queue
    participant E as JPA storage engine

    Note over Tx,E: PREPARE_COMMIT
    Tx->>Q: read queue [e0]
    Tx->>E: appendEvents([e0])
    E->>E: persist + flush  (e0 only)
    H->>Tx: appendEvent(e1)
    Tx->>Q: add e1  (queue now [e0, e1])
    Note over E: never called again. e1 is gone.
    Note over H: subscribers already saw e1

Loading

This is why no existing test caught it: the in-memory engine hides it.

The fix

The batch is handed over in a phase of its own, EventStoreTransaction.Phases.APPEND_EVENTS, at order 29000.

That is deliberately the last slot before COMMIT, not the first one above PREPARE_COMMIT. SAGA_WRITE sits at 25000, and a saga is exactly the kind of component that may append events. Placing the hand-off last means every phase a caller puts in the gap still gets to append.

graph LR
    A["PRE_INVOCATION<br/>-10000"] --> B["INVOCATION<br/>0"]
    B --> C["POST_INVOCATION<br/>10000"]
    C --> D["PREPARE_COMMIT<br/>20000<br/><i>event delivery, token store</i>"]
    D --> E["SAGA_WRITE<br/>25000<br/><i>saga written</i>"]
    E --> F["APPEND_EVENTS<br/>29000<br/><i>batch to storage engine</i>"]
    F --> G["COMMIT<br/>30000<br/><i>transaction commits</i>"]
    G --> H["AFTER_COMMIT<br/>40000"]

Loading

Appending after that phase is now refused rather than queued into a batch that is already gone.

How it was proved

DefaultEventStoreTransactionTest.AppendingDuringPrepareCommit.

The key move is that the test asserts what the engine was handed, not only what ended up stored, and runs against EagerFlushEventStorageEngine (an InMemoryEventStorageEngine that snapshots its batch with List.copyOf, modelling what the JPA engine does).

anEventAppendedByALaterPrepareCommitActionJoinsTheSameBatch before the fix:

Expecting actual:
  [["event-0"]]
to contain exactly (and in same order):
  [["event-0", "event-1"]]

The engine received one event. event-1 was dropped, after subscribers had seen it.

aFirstEventAppendedDuringPrepareCommitIsStored before the fix:

java.lang.IllegalStateException: Failed to register handler in phase PREPARE_COMMIT (20000).
ProcessingContext is already in phase PREPARE_COMMIT (20000).

Both green after. A third test, appendingOnceTheBatchWasHandedToTheStorageEngineIsRejected, pins the new loud failure.

Alternative that was rejected

Opening a second AppendTransaction for the late events does not work: its consistency marker predates the first batch, so InMemoryEventStorageEngine reports a false conflict and AggregateBasedJpaEventStorageEngine restarts sequence numbers and collides on its own primary key. Nothing can produce a fresh marker mid-context, because the append position is only updated after commit.

One assertion moved

SimpleEventStoreTest.subscriberExceptionRollsBackTransaction became subscriberExceptionPreventsTheEventsFromReachingTheStorageEngine. A failing subscriber used to leave an append transaction to roll back; it now leaves none, because the engine is not asked for one until every subscriber has been notified. The test asserts that directly, which is the stronger claim.

---

Bug 2: SimpleEventBus accepts a publish it can never deliver

if (context.isCommitted()) { throw new IllegalStateException("... already been committed ..."); }

isCommitted() is status == COMPLETED, true only once every phase has finished. During COMMIT and AFTER_COMMIT it is false, so the guard never fires where it was written to fire.

What happened instead depended on whether anything had published earlier in the same context:

• First publish: queue absent, so the bus tries to register its prepare-commit hook and the context rejects it. An error about phase registration rather than about publishing, but at least an error.
• Later publish: queue exists, computeResourceIfAbsent returns it, events are added, and nothing ever reads them again. Accepted and silently dropped.

The fix

The right question is not which phase the context is in, but whether the queue has already been drained, and only the bus knows that. It now records it, the same way DefaultEventStoreTransaction records prepareCommitExecuted.

The placement matters:

context.onPrepareCommit(ctx -> {
    CompletableFuture<Void> delivery = processEventsInPhase(queuedEvents, ctx, ...);
    ctx.putResource(eventsDeliveredKey, Boolean.TRUE);   // <- right here
    return delivery;
});

processEventsInPhase loops while the queue keeps growing, so it catches anything a synchronous subscriber publishes. The instant it returns, that loop is over and no later addition will be picked up, even though subscribers may still be running. Setting the flag at that exact point is what turns a publish from a still-running asynchronous subscriber into a failure rather than a second silent drop. Setting it in whenComplete would have left that case as broken as before.

Note this deliberately does not need a public phase accessor. The phase would not be precise enough: during PREPARE_COMMIT publishing is fine while the drain is running and wrong once it is not, and no phase can tell those apart.

isCommitted() stays as the coarser case, since it still gives a better message for a publish from a completion handler in a context that never published anything.

How it was proved

SimpleEventBusTest.publishingOnceTheQueueWasDrainedIsRejectedRatherThanQueuedAgain publishes during INVOCATION, then again from a COMMIT-phase action. Before the fix:

java.lang.AssertionError:
Expecting actual not to be null

That is the captured throwable being null: no exception at all. The publish was accepted and the event vanished. After the fix it throws IllegalStateException, and the recording listener confirms only event1 was ever delivered.

---

Bug 3: StubProcessingContext did not apply the rules it claims to

Both the saga work and this PR had to write their phase tests against a real UnitOfWork. The reason is recorded in 8a6e44f753's commit message: the stub assigns currentPhase after running that phase's actions, so during a PREPARE_COMMIT action it still reports the earlier phase and accepts registrations production rejects. It also threw IllegalArgumentException with its own wording, so a test asserting on that pinned the stub instead of the framework.

Working around a broken test double once is fine; leaving it broken means everyone after has to know to work around it too.

It now enters a phase before running its actions, and rejects the same registrations with the same exception type and message as UnitOfWork. Behaviour is otherwise unchanged and the whole messaging suite passes, so nothing was leaning on the old leniency.

The javadoc now states where the stub still stops, so the next reader does not have to find out by experiment: a phase's actions are chained rather than run in parallel, and a registration for a later phase that a moveToPhase call is already past is not picked up.

publishingAfterContextCommittedThrowsException changes with it. The behaviour it describes is unchanged, but the reason is now visible: the bus cannot register the prepare-commit hook that would have delivered the events, and the message names the phase that blocked it.

---

Documentation

The rule had no documentation anywhere: no @throws on ProcessingLifecycle.on, no comment at the throw, nothing in the reference guide. A caller could only discover it by hitting the exception, and could not check beforehand either, since Axon Framework 5 has no public equivalent of Axon Framework 4's UnitOfWork.phase().

Both the javadoc and processing-context.adoc now state the rule and the way out of it: a component invoked from within a late phase registers for a custom phase in the gap above it. The reference guide names the saga write and the event store hand-off as the two users, so the pattern reads as intended design rather than something invented twice.

---

Verification

• Full reactor: green (4495 messaging tests)
• -Pintegration-test verify -pl integrationtests: green (84 + 83)
• -Pexamples verify: green

One pre-existing failure is unrelated to this PR: AnnotatedEventHandlerInterceptorTest.interceptorIsInvokedBeforeEventHandler (handler invoked three times, interceptor once). It fails identically on 411a58bd2e with none of these commits applied.

---

Open question for the reviewer

Moving the hand-off to 29000 surfaces an asymmetry that was previously invisible: the event store now accepts appends up to 29000, but the event bus stops delivering at 20000. An event appended from a gap phase such as SAGA_WRITE would therefore be stored but never reach subscribers.

With Bug 2 fixed that case now throws instead of diverging silently, which I believe is the right default. But if appending events from SAGA_WRITE is something a saga should be able to do, the bus's delivery hook needs to move up as well. I would rather agree that than decide it unilaterally.

[MateuszNaKodach]

Superseded by #5014. GitHub closed this automatically when the head branch was renamed from feat/saga-repository-v1 to bug/3728/prepare-commit-event-loss. Same four commits, rebased onto the current feat/saga-repository.