[
https://issues.apache.org/jira/browse/CAMEL-24151?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Claus Ibsen updated CAMEL-24151:
--------------------------------
Fix Version/s: 4.22.0
> camel-saga/camel-lra: medium and low findings from Saga EIP code review (July
> 2026)
> -----------------------------------------------------------------------------------
>
> Key: CAMEL-24151
> URL: https://issues.apache.org/jira/browse/CAMEL-24151
> Project: Camel
> Issue Type: Bug
> Components: camel-core, camel-lra
> Reporter: Federico Mariani
> Assignee: Claus Ibsen
> Priority: Major
> Fix For: 4.22.0
>
>
> Umbrella for the medium/low findings of a July 2026 code review of the Saga
> EIP (model, reifier, processors, in-memory service, saga component,
> camel-lra). The high-severity findings are filed as separate issues.
> h3. Medium
> * *LRAClient accepts only HTTP 200 and throws context-free errors*:
> {{join}}/{{complete}}/{{compensate}} test {{statusCode() != HTTP_OK}}
> (LRAClient.java:141,158,175). Other 2xx codes are legitimate per coordinator
> implementations; 404/410 for an already-finished LRA (e.g. compensated by
> timeout) is a common, meaningful case. Users get a bare
> {{RuntimeCamelException("Cannot complete LRA")}} with no status code,
> response body or LRA id. Fix: accept the 2xx range, include status/body/LRA
> URL in messages.
> * *In-memory step timeout timers never cancelled*: every {{beginStep}} with a
> timeout schedules a compensation task (InMemorySagaCoordinator.java:109-116)
> that is not cancelled on completion. Functionally harmless (CAS fails) but
> retains the coordinator and the enlisting Exchange until the timer fires
> (hours for long timeouts); under exchange pooling the captured exchange may
> have been recycled when the timer runs. Fix: keep and cancel the
> {{ScheduledFuture}}; extract only what is needed from the exchange.
> * *In-memory status machine loose ends*: terminal status is set in
> {{thenApply}} (skipped if the chain completes exceptionally - coordinator
> hangs in COMPENSATING/COMPLETING); {{doCompensate}} marks COMPENSATED even
> when steps exhausted all retries (WARN only). The EIP doc says "a manual
> intervention process should be triggered" - there is no hook, event or
> distinct status (e.g. COMPENSATION_FAILED) for it.
> * *MANUAL completion without a timeout silently leaks the saga*: docs say
> MANUAL should always be combined with a timeout and mention a possible
> default timeout; nothing enforces or warns, and no default-timeout mechanism
> exists in either implementation. Fix: fail fast (or WARN) at reifier time;
> optionally add a configurable default timeout.
> * *ifNotException can swallow synchronous exceptions and hang the exchange*:
> the propagation processors run continuation code inside {{whenComplete}}
> lambdas (SagaProcessor.java:155-172). If {{code.run()}} throws synchronously
> the exception disappears into the already-completed future and the
> {{AsyncCallback}} is never invoked. {{LRASagaCoordinator}} defensively wraps
> exactly one such case (LRASagaCoordinator.java:40-47); the general risk
> remains. Fix: try/catch around {{code.run()}} -> {{exchange.setException(t);
> callback.done(false)}}.
> * *LRASagaService.setCamelContext adds participant REST routes as a side
> effect of the setter* (LRASagaService.java:121-131): routes are added even if
> the service is never started, and failures (e.g. no rest component) surface
> from an unexpected place. Route setup belongs in {{doStart}}.
> * *camel-lra: single shared URI allowlist, never pruned*: {{verifyRequest}}
> validates against one combined set of all compensation and completion URIs
> (LRASagaRoutes.java:135-139), so a completion callback may invoke a
> compensation URI and vice versa, and URIs from removed routes stay allowed
> for the context lifetime.
> h3. Low / cleanups
> * {{SagaProcessor.toString()}} returns the literal string "id"
> (SagaProcessor.java:146-148).
> * {{InMemorySagaCoordinator.beginStep}} does {{putIfAbsent(step, new
> ConcurrentHashMap<>())}} immediately followed by {{computeIfAbsent(step, k ->
> new HashMap<>())}} - the second mapping function is dead code.
> * Failed futures built as {{CompletableFuture.supplyAsync(() -> { throw ex;
> })}} (InMemorySagaCoordinator.java:101-103, LRASagaCoordinator.java:44-46)
> burn a pool task (the first on the common ForkJoinPool) - use
> {{CompletableFuture.failedFuture(ex)}}.
> * {{LRAClient.join}} appends the time limit as {{lra + "?" + "TimeLimit=" +
> ms}} (LRAClient.java:127-129) - double-{{?}} URL if the LRA id URI ever
> carries a query string.
> * Missing {{coordinatorUrl}} surfaces as {{IllegalStateException("Host not
> set")}} from {{LRAUrlBuilder}} during doStart; missing
> {{localParticipantUrl}} only fails at the first join. Validate both at
> doStart with messages naming the configuration options.
> * {{LRASagaService.registerStep}} uses {{Optional.map(set::add)}} for a side
> effect with ignored return - use {{ifPresent}}.
> * {{SagaProcessorBuilder}}/{{SagaProcessor}} accept a {{CamelContext}} that
> is never stored or used.
> * The EIP doc claims "the Saga EIP implementation may have a default timeout
> set on all Sagas" - matches no shipped implementation.
> * {{SagaProducer}}'s "not bound to a saga context: cannot complete" message
> is hardcoded - {{saga:compensate}} failures also say "cannot complete"
> (SagaProducer.java:49-50).
> Found during a broader Saga EIP / camel-lra code review (July 2026).
> _Claude Code on behalf of Croway_
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
