Skip to content

CAMEL-25005: camel-core - Saga EIP: do not lose the compensation of a step that joins while the in-memory saga ends - #26865

Open
allthingssecurity wants to merge 1 commit into
apache:mainfrom
allthingssecurity:camel-saga-late-enlist-compensation
Open

allthingssecurity wants to merge 1 commit into
apache:mainfrom
allthingssecurity:camel-saga-late-enlist-compensation

Conversation

@allthingssecurity

Copy link
Copy Markdown
Contributor

Description

CAMEL-25005

InMemorySagaCoordinator.beginStep was check-then-act without a lock:

  1. it read the status and continued if it was RUNNING;
  2. it evaluated the step's saga options (user expressions);
  3. it added the step to the enlistments and returned a completed future, so the step's action ran.

complete(), compensate() and the timeout task switch the status with compareAndSet(RUNNING, ...), and doFinalize then takes a snapshot of the enlistments. Nothing ordered that snapshot after the enlistment of a step that had already passed the check. Such a step:

  • got a successful beginStep, so its action ran (for example, the payment was taken);
  • was never passed to its compensation (or completion) endpoint;
  • while the saga ended as COMPENSATED (or COMPLETED) and was removed.

The window covers the option evaluation. The saga timeout makes it reachable with synchronous routes only: the saga times out while a joining step is between the check and the enlistment. Without any forcing (timeout 1 ms, owner work about 1 ms, plain simple option), 1 of 5000 sagas ended compensated with the payment step executed and never compensated.

This change:

  • beginStep still evaluates the options outside of any lock. Then, under a lock of the coordinator, it checks the status again and enlists the step (and schedules its timeout).
  • complete(), compensate() and the timeout task change the status and take the snapshot they finalize under the same lock. The snapshot is passed to the finalization instead of reading the list later.

A step that loses the race now fails with IllegalStateException("Cannot begin: status is ..."), as it does when it arrives a moment later. The public doCompensate/doComplete/doFinalize methods keep their signatures and behaviour. Timeouts can also no longer be scheduled after cancelTimeouts() ran.

Tests: new SagaJoinDuringTimeoutTest. A MANDATORY step joins a saga with a 100 ms timeout. Its option expression waits (Awaitility) until the saga's compensation is running, which puts the enlistment after the snapshot deterministically. Without the fix:

AssertionError: mock://payment Received message count. Expected: <0> but was: <1>

(the payment ran, and mock:compensate-payment never received anything). With the fix, the step fails to begin and the test passes. All *Saga* tests in camel-core pass (36 tests). The reproduction: payment step action executed=0, and the stress run found no lost compensations in 5000 sagas.

Found with a TLA+ model of the in-memory saga (coordinator, timeout, synchronous and asynchronous participants), then reproduced against the real classes. With this change, "every step whose beginStep succeeded is completed or compensated exactly once" holds, along with "never both" and termination.

This is independent of CAMEL-25006, the other in-memory saga fix I am sending, where a REQUIRED/SUPPORTS step of a saga that has already ended starts a new saga (a regression from CAMEL-24144). Each fixes its own property in the model, and both together satisfy all of them.

Target

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.
    (I built and tested the affected modules, including the formatter and import-sort plugins. I did not run the full root build.)

AI-assisted contributions

  • If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., Co-authored-by trailers) and the PR description identifies the AI tool used.
    This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a Co-Authored-By trailer.

Claude Code on behalf of allthingssecurity

🤖 Generated with Claude Code

… step that joins while the in-memory saga ends

InMemorySagaCoordinator.beginStep checked that the saga was RUNNING,
evaluated the step's options, and then added the step to the
enlistments, without any lock. complete(), compensate() and the
timeout task changed the status with a compareAndSet and then
finalized a snapshot of the enlistments. A step that passed the check
before the saga ended, but enlisted after the snapshot, got a
successful beginStep, so its action ran, but it was never passed to its
compensation or completion endpoint. The window includes the option
expressions, and the saga timeout makes it reachable with synchronous
routes only.

beginStep now checks the status again and enlists the step under a
lock of the coordinator, and complete(), compensate() and the timeout
task change the status and take the snapshot they finalize under the
same lock. A step that loses the race fails with "Cannot begin: status
is ...", as it does when it arrives a moment later.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@oscerd oscerd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traced the locking — this correctly closes a real money-safety race in the Saga EIP.

  • The single coordinator lock now serializes the two operations that were previously unordered: beginStep's status-check-plus-enlist, and the status change + enlistment snapshot in end(status). Because end does the compareAndSet(RUNNING, …) and new ArrayList<>(enlistments) under the same lock beginStep holds while it enlists, every step that successfully began is guaranteed to be in the finalized snapshot — so the "beginStep succeeded (action ran) but the step was never compensated/completed" window is gone.
  • beginStep evaluating the step options outside the lock and then re-checking status under it is the right shape: no user expression runs while holding the coordinator lock, and a step that loses the race is failed with IllegalStateException("Cannot begin: status is …") so its action never runs — identical to arriving a moment later.
  • Finalization (doCompensate/doComplete/doFinalize) receives the snapshot and runs outside the lock, so the compensation/completion endpoints don't execute under it. The timeout is scheduled under the lock but its task only takes the lock when it later fires, so there's no nested-lock deadlock, and preventing scheduling after cancelTimeouts() is a good extra guard. Keeping the public doCompensate/doComplete/doFinalize signatures (delegating with the live list) preserves compatibility while the internal paths use the snapshot.

I don't see a remaining path where a began step escapes finalization. This is also independent of the other two Saga PRs (#26866, #26869) — different files, no reconciliation needed. LGTM.

(CI has not been triggered yet — fork PR awaiting a maintainer to approve the workflow run; I'll confirm green before it merges.)

This review was generated with AI assistance and reviewed/issued by the human operator. Claude Code on behalf of oscerd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants