CAMEL-24150, CAMEL-24151: Saga EIP fixes — shared service lifecycle, code review findings#24834
Merged
Conversation
gnodet
approved these changes
Jul 17, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Claude Code on behalf of gnodet
LGTM — removes incorrect shared-service lifecycle management from individual route processors.
The saga service is a context-level singleton added via context.addService(), which makes the CamelContext its lifecycle owner. SagaProcessor.doStart()/doStop() calling ServiceHelper.startService()/stopService() on the shared service was a lifecycle ownership violation — stopping one saga route would shut down the executor, producer template, and LRA client for every other saga route in the context.
Review notes:
-
Pure deletion — 12 lines removed, 0 production lines added. The safest kind of fix.
-
doStart()removal is safe —context.addService(sagaService)already starts the service before any route processor'sdoStart()runs. -
doStop()removal is the fix — prevents one route shutdown from cascading to all saga routes. Particularly important with the sharedProducerTemplateadded in CAMEL-24146 (#24830) — that would also be stopped prematurely. -
Tests — structural check (service stays started after route stop) plus functional check (compensation on route-b works after route-a stops). Both scenarios directly exercise the bug.
Nice capstone to the saga fix series (CAMEL-24144 through CAMEL-24150).
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
changed the title
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 551 tested, 28 compile-only — current: 550 all testedMaveniverse Scalpel detected 579 affected modules (current approach: 550).
|
2 tasks
oscerd
reviewed
Jul 17, 2026
oscerd
left a comment
oscerd
left a comment
Contributor
There was a problem hiding this comment.
The lifecycle change is correct for the common context.addService(sagaService) path (covered by the new SagaSharedServiceRouteStopTest and by camel-lra), and the M1–L9 cleanups look sound — timeout ScheduledFuture cancellation, terminal status on exceptional completion, the failedFuture/ifPresent/message tidy-ups, and the LRAClient <400 acceptance.
One thing worth verifying before merge, though: this deletes SagaProcessor.doStart()/doStop() (which did ServiceHelper.startService(sagaService) — SagaProcessor.java:175-181 on main). That looks like the only place a saga service resolved as a registry bean (rather than via context.addService) got started. The kamelet-main/JBang path binds it exactly that way — SagaDownloader does registry bind, not addService, and SagaReifier.resolveSagaService() falls through to mandatoryFindSingleByType(CamelSagaService.class) for registry beans with no addService. If registry Service beans aren't auto-started elsewhere, getExecutorService()/getProducerTemplate() would be null at the first saga step → NPE. The new test only exercises the addService path, so it wouldn't catch this. Could you confirm the registry-bound saga path still starts the service (e.g. run a JBang saga route)? If it doesn't, having SagaDownloader use context.addService(...) would cover it.
Minor: removing the CamelContext constructor param from SagaProcessor + its six subclasses (and SagaProcessorBuilder.camelContext()) is a breaking signature change on public-ish processor classes — worth an upgrade-guide line. And as with #24832, this collides with the other three in-flight saga PRs on InMemorySagaCoordinator, so it'll need rebasing whenever it lands relative to them.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…ocessor The SagaProcessor.doStop() was calling ServiceHelper.stopService() on the context-level CamelSagaService. Since this service is shared across all saga routes, stopping one route would shut down the saga service for the entire CamelContext, breaking compensation on all other routes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
davsclaus
force-pushed
the
fix/CAMEL-24150
branch
from
July 17, 2026 12:17
8df630c to
79ed90d
Compare
gnodet
approved these changes
Jul 17, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Claude Code review on behalf of gnodet
Re-reviewed after rebase (3 new commits replacing the previous changeset). All 15 files reviewed.
CAMEL-24150: Shared service lifecycle
The core fix — removing ServiceHelper.startService/stopService(sagaService) from SagaProcessor.doStart()/doStop() — is correct. The saga service is context-scoped, not route-scoped. Stopping one route was stopping the shared service, breaking all other saga routes.
The new SagaSharedServiceRouteStopTest properly validates both that the service stays started and that compensation on a remaining route still works after stopping another route. Good use of MockEndpoint.assertIsSatisfied(context, 10, TimeUnit.SECONDS) per project guidelines.
CAMEL-24151: Code review findings (M1–M7, L1–L9)
All well-implemented:
| Finding | Change | Verdict |
|---|---|---|
| M1 | LRAClient accepts 2xx/3xx (>=400 = error), includes status+body+URL in error msgs | ✅ Correct — 201/204 are valid HTTP success |
| M2 | Timeout ScheduledFutures stored and cancelled on compensate/complete |
✅ Correct — prevents post-completion timeout triggering compensation |
| M3 | whenComplete replaces thenApply for terminal status + saga cleanup |
✅ Correct — ensures status is always set, even on exceptional path; prevents saga leaks |
| M4 | WARN at reifier time for MANUAL completion without timeout | ✅ Correct — catches a common misconfiguration |
| M5 | ifNotException wraps code.run() in try/catch |
✅ Correct — prevents uncaught exceptions from swallowing the error |
| M6 | Route setup moved from setCamelContext() to doStart() |
✅ Correct — aligns with Camel lifecycle |
| L1 | toString() returns id not literal "id" |
✅ |
| L3 | CompletableFuture.failedFuture() replaces supplyAsync(() -> throw) |
✅ Cleaner, avoids ForkJoinPool |
| L4 | ? vs & separator in LRAClient join URL |
✅ Prevents double-? |
| L5 | Validate coordinatorUrl/localParticipantUrl at doStart | ✅ Fail-fast |
| L6 | ifPresent() instead of map() for side effects |
✅ Correct Optional usage |
| L7 | Removed unused CamelContext from SagaProcessor hierarchy | ✅ Consistent across all 6 subclasses + builder + reifier |
| L8 | Doc: "no default timeout exists" (was misleading) | ✅ |
| L9 | Error message says "cannot compensate" on compensate path | ✅ |
One note
The constructor signature change for SagaProcessor and its subclasses is technically a public API change (the subclass constructors are public). Since these are created through SagaProcessorBuilder, real-world impact should be zero, and the justification (removing an unused parameter) is sound. Worth noting in the upgrade guide if not already there.
LGTM 👍
… bind Use context.addService() so the InMemorySagaService is properly lifecycle-managed by the CamelContext. The old registry bind approach would leave the service unstarted now that SagaProcessor.doStart() no longer calls ServiceHelper.startService() on the shared saga service. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Contributor
Author
|
Thanks for the thorough review @oscerd! Both points are now addressed in the latest commit:
Claude Code on behalf of Claus Ibsen |
gnodet
approved these changes
Jul 17, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Re-review after new commit (fabcd70): SagaDownloader uses addService instead of registry bind ✅
Good improvement — this directly addresses the shared service lifecycle theme of CAMEL-24150:
-
addService()instead ofbind()— properly integratesInMemorySagaServicewith the CamelContext lifecycle (start/stop/shutdown), whereas the oldmain.bind("inMemorySagaService", ...)only put it in the registry with no lifecycle management. This was exactly the class of bug this PR set out to fix. -
Idempotency guard —
ctx.hasService(CamelSagaService.class) == nullprevents adding duplicate saga services if the reifier runs multiple times (e.g., multiple saga routes in the same context). -
Removed
KameletMainparameter —registerDownloadReifiers()no longer needs a reference toKameletMain, which is a cleaner API. The reifier lambda captures the route'sCamelContextdirectly, which is the right scope. -
try-catch on
addService()— correct sinceaddServicecan throw; wrapping inRuntimeExceptionis appropriate for a reifier context where checked exceptions can't propagate.
Still LGTM with this addition.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SagaDownloader(JBang/kamelet-main path) to usecontext.addService()instead ofmain.bind(), ensuring theInMemorySagaServiceis properly lifecycle-managed. Without this, removingSagaProcessor.doStart()would leave the registry-bound service unstarted. (Addresses @oscerd's review feedback.)camel-core,camel-saga, andcamel-lra:ScheduledFutures stored and cancelled on compensate/completewhenCompleteso terminal status is always set, even on exceptional completionifNotExceptionwrapscode.run()in try/catch to prevent exception swallowingLRASagaService.setCamelContext()todoStart()SagaProcessor.toString()returns actual id, not literal"id"putIfAbsentcode; consistentConcurrentHashMapusageCompletableFuture.failedFuture()replacessupplyAsync(() -> throw)?URL risk in LRAClient joincoordinatorUrlandlocalParticipantUrlatdoStartOptional.ifPresent()instead of.map()for side effectsCamelContextparameter from SagaProcessor hierarchyTest plan
SagaTest,SagaFailuresTest,SagaTimeoutTest,SagaPropagationTest,SagaOptionsTest,SagaRemoveHeadersTest,SagaSharedServiceRouteStopTest,SagaComponentTest)camel-lratests passcamel-sagatests passcamel-kamelet-mainbuilds cleanly with SagaDownloader change🤖 Generated with Claude Code
Claude Code on behalf of davsclaus