Skip to content

CAMEL-25006: camel-core - Saga EIP: fail a REQUIRED or SUPPORTS step of a saga that has already ended - #26866

Open
allthingssecurity wants to merge 1 commit into
apache:mainfrom
allthingssecurity:camel-saga-ended-saga-id
Open

allthingssecurity wants to merge 1 commit into
apache:mainfrom
allthingssecurity:camel-saga-ended-saga-id

Conversation

@allthingssecurity

Copy link
Copy Markdown
Contributor

Description

CAMEL-25006

Since CAMEL-24144 (4.22.0), InMemorySagaCoordinator removes itself from InMemorySagaService once it is completed or compensated. getSaga(id) then returns null, and the saga processors treat null as "this exchange is not in a saga". As a result, after a saga timed out and was compensated:

  • a REQUIRED step on an exchange of that saga started a new saga of its own and completed it;
  • a SUPPORTS step ran outside of any saga.

For example:

from("direct:order").saga().timeout(200, MILLISECONDS).compensation("direct:cancelOrder")
    .to("direct:slowCall")            // outlasts the saga timeout
    .to("direct:payment");
from("direct:payment").saga()        // REQUIRED
    .compensation("direct:refund").completion("direct:confirmPayment")
    .to("direct:takePayment");

The order was compensated, but the payment was taken and confirmed, and nothing will ever refund it. Before CAMEL-24144, the payment step failed with Cannot begin: status is COMPENSATED. It still fails that way when it arrives while the saga is still being compensated, so the outcome depended on timing.

This change: RequiredSagaProcessor and SupportsSagaProcessor fail with IllegalStateException("Cannot begin: saga <id> is not active or not known") when the exchange carries a saga id that the saga service no longer knows. The id is the one from the exchange's internal state, or from the header for services that support it, as today.

  • The saga is still removed from the service, so the memory fix of CAMEL-24144 is kept (SagaCoordinatorCleanupTest is unchanged).
  • MANDATORY already fails.
  • REQUIRES_NEW, NOT_SUPPORTED and NEVER are unchanged, since they do not need the outer saga to be active.
  • LRASagaService.getSaga never returns null, so LRA is unaffected.
  • The 4.23 upgrade guide has a short entry next to the other saga entry.

Tests: two new tests in SagaTimeoutTest. The owner route waits (Awaitility, on the saga service) until its saga has timed out, been compensated and been removed, and then calls a REQUIRED or SUPPORTS step. Without the fix:

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

With the fix both pass. All *Saga* tests in camel-core pass (37 tests). The camel-lra tests could not be run offline here.

Found with a TLA+ model of the in-memory saga (coordinator, timeout, participants with each propagation), then reproduced against the real classes. With this change, the property "a participant carrying the saga's id never runs in another saga, or outside a saga, while that saga compensates" holds, with synchronous and asynchronous participants and timeouts. The reproduction now fails the payment step: compOwner=1 paymentAction=0 complPayment=0.

This is separate from the check-then-enlist race in InMemorySagaCoordinator.beginStep (a step that joins while the saga is finalized is never compensated), which I am sending as its own fix.

The check is in SagaProcessor, so it applies to any CamelSagaService whose getSaga returns no coordinator for the exchange's saga id. LRASagaService.getSaga never returns null, so camel-lra is not affected. The upgrade-guide entry also mentions custom services that read the Long-Running-Action header.

Related, not changed here: when compensate()/complete() of a new saga fails, handleSagaCompletion does not restore the previous saga of the exchange, so the exchange keeps the id of the ended saga. A later REQUIRED/SUPPORTS step on that exchange (for example after a doCatch) now fails, as before 4.22. I can send a small follow-up that restores the previous saga on that path too.

As the regression is in 4.22, this may be worth a backport to camel-4.22.x.

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

…of a saga that has already ended

Since CAMEL-24144 (4.22.0) InMemorySagaService removes a saga once it is
completed or compensated, and getSaga(id) then returns null. The saga
processors treat null as "the exchange is not in a saga". So after a
saga timed out and was compensated, a REQUIRED step on an exchange of
that saga started a new saga of its own and completed it, and a
SUPPORTS step ran outside of any saga. For example, a payment was taken
and confirmed although the order it belonged to was compensated.
Before CAMEL-24144, and still while the saga is being compensated, such
a step failed with "Cannot begin: status is ...".

RequiredSagaProcessor and SupportsSagaProcessor now fail with
IllegalStateException when the exchange carries the id of a saga that
the saga service no longer knows. The saga is still removed from the
service, so this keeps the memory fix of CAMEL-24144. MANDATORY already
fails, and REQUIRES_NEW, NOT_SUPPORTED and NEVER are unchanged.

Regression from CAMEL-24144.

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.

Good companion fix to CAMEL-24144. Traced it and the logic is exactly right:

checkSagaIsActive fails only in the precise case that matters — coordinator == null (the service no longer knows the saga) and getCurrentSagaId(exchange) != null (the exchange is carrying an ended saga's id). That distinguishes the regression case (a step of a saga that already completed/compensated, which must fail with Cannot begin: saga … is not active or not known) from a genuine no-saga exchange, where the id is null and RequiredSagaProcessor still starts a new saga / SupportsSagaProcessor still runs outside one. So the fix closes the "order compensated, payment taken and never refunded" hole without breaking the legitimate new-saga path.

Other things that check out:

  • It reuses the same getCurrentSagaId(exchange) source the processors already use, so it doesn't invent a new notion of saga membership.
  • The saga is still removed from the service, so CAMEL-24144's memory fix stands (and SagaCoordinatorCleanupTest is untouched).
  • Scoping is correct: MANDATORY already failed, and REQUIRES_NEW/NOT_SUPPORTED/NEVER legitimately don't need the outer saga, so they're left alone.
  • Different files from #26865 and #26869, so the three Saga PRs don't collide.

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