Repository navigation
CAMEL-26333: camel-core - doFinally runs only its first step after a called route's error handler handled a failure - #28756
Merged
davsclaus merged 2 commits intoOct 31, 2026
Conversation
…called route's error handler handled a failure When a doTry called a route whose error handler handled a failure (a dead letter channel or a handled onException), the doFinally block ran only its first step. The error handler marks the exchange as handled by the error handler and as failure handled; FinallyProcessor cleared the exception before the block but not those marks, and the block's pipeline stops before every later step when the exchange is handled by the error handler. FinallyProcessor now clears both marks while the doFinally block runs, as the onCompletion does, and sets them again when the block is done (the handled mark only if the block did not set it itself). Every step runs, a route that the block calls handles or continues its own failure with its error handler as usual, and the route still stops after the doTry (CAMEL-19441). This also fixes a routing slip or dynamic router that routed to the endpoints of a recipient list: as the first step of such a doFinally, the recipient list only evaluated its recipients into the exchange property CamelEvaluateExpressionResult, which a later routing slip took as its slip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gnodet-bot
reviewed
Oct 31, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Solid fix. The flag snapshot/restore pattern correctly unblocks all doFinally pipeline steps, and the two asymmetries in the restore logic relative to OnCompletionProcessor are both intentional — though they deserve brief inline comments to protect future maintainers.
Backport candidates: the PR description says camel-4.22.x and camel-4.18.x carry the same FinallyProcessor/PipelineHelper code. Please request port/4.22.x and port/4.18.x labels if you'd like these branches covered.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Contributor
|
🧪 CI tested the following changed modules:
🔁 2 tests passed only after a retry on JDK 21 (2 retried attempts) Recovered flaky tests on JDK 21 (2)
🔬 Scalpel shadow comparison — Scalpel: 572 of 713 tested, 26 compile-only — current: 573 all testedMaveniverse Scalpel detected 572 affected modules (current approach: 573). Skip-tests mode would test 572 modules (3 direct + 570 downstream), skip tests for 26 (generated code, meta-modules) Modules only in current approach (1)
Modules Scalpel would test (572)
Modules with tests skipped (26)
Build reactor — dependencies compiled but only changed modules were tested (3 modules, 18.3s total)Total reactor time: 18.3s
Top 20 slowest modules:
|
davsclaus
reviewed
Oct 31, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks, the analysis is spot on and the fix is well-scoped.
I traced it: the callee's error handler (DLC or handled onException) leaves errorHandlerHandled=true and failureHandled=true on the shared exchange. PipelineHelper.continueProcessing then stops the doFinally pipeline after its first step, and isDone() / the "already set" early return in prepareExchangeAfterFailure stop a route called from the doFinally from handling its own failure. Clearing both flags around the block and restoring them afterwards (like OnCompletionProcessor) fixes both. Since doTry/doCatch/doFinally children are never wrapped in the route error handler, plain doTry cases (exception in the try block, onException in the same route, nested doTry) are untouched. The restore logic handles the block's own outcomes correctly (handled by a callee: kept; continued: restored so the caller stops after the doTry; unhandled: fails with the block's exception), and CAMEL-19441 is preserved.
A few suggestions:
- Upgrade guide: as you describe, routes now see the second and later doFinally steps run (and send), and a failing route called from such a doFinally is now handled/continued by its own error handler instead of failing the exchange. Since that is how it has behaved for all of 4.x, please add a short note to
camel-4x-upgrade-guide-4_24.adoc. RoutingSlipInDoFinallyAfterRecipientListTest.routingSlipAfterRedeliveryDoesNotUseTheRecipientListValue: the reduced, model-class route is hard to follow, and the loop overmock:deadexchanges passes trivially if none arrive (the real check is the 10 s timeout). Could you assert the expectedmock:dead/mock:m1counts explicitly, or drop it since the other tests already cover the root cause?- +1 to the two inline comments on
FinallyProcessor(the primitive "set and true" snapshot vs the nullableBooleaninOnCompletionProcessor, and the one-way restore offailureHandled): a one-liner each would help future readers.
Side question, not for this PR: when the doTry's callee fails unhandled, failureHandled=true / errorHandlerHandled=false stay set while a doCatch runs (CatchProcessor only clears the exception), so a route called from the doCatch probably cannot apply its own onException/DLC either. If you can reproduce that, it would make a good follow-up JIRA.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…nts, drop the reduced test Add the upgrade guide entry for the doFinally steps that now run, comment the primitive snapshot and the one-way restore of failureHandled in FinallyProcessor, and drop the reduced routing slip test whose check passed when no exchange reached the dead letter channel: the other tests of the class cover the same root cause with explicit counts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@davsclaus thanks for the review. In f972e99:
Side question: it reproduces on main (baaf532). The doTry calls a route that fails unhandled, and the doCatch then calls a route with a dead letter channel, or with a handled The doTry/doCatch/doFinally, routing slip, recipient list and onCompletion tests pass (408 tests). Claude Code on behalf of allthingssecurity |
gnodet-bot
reviewed
Oct 31, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Following up on my earlier review — both inline findings have been addressed.
- Primitive snapshot comment (
// a primitive "set and true" snapshot, unlike the nullable Boolean of OnCompletionProcessor...) — ✅ added - One-directional failureHandled restore comment (
// one-way restore, unlike OnCompletionProcessor: set again but never cleared...) — ✅ added
davsclaus's suggestions also addressed:
- Upgrade guide entry (
camel-4x-upgrade-guide-4_24.adoc) — ✅ added, accurately describes the behaviour change and the CAMEL-19441 invariant that is preserved routingSlipAfterRedeliveryDoesNotUseTheRecipientListValuetest concern — ✅ resolved by removing the problematic test and replacing it with four focused tests (routingSlipWithDeadLetterChannel,routingSlipWithHandledOnException,dynamicRouterWithDeadLetterChannel,controlCalledRouteDoesNotFail) that each assert explicit mock:dead and recipient counts
Pre-fix validation: static trace confirms assertAllFinallySteps would fail on main (mock:f2, mock:f4 counters = 0, f3 header null) because the pipeline stops after mock:f1 when errorHandlerHandled=true. The fix correctly clears the flags before the pipeline runs.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
Author
|
@davsclaus your side question reproduces on main; it is filed as CAMEL-26434 (with the routes and a table of the four cases), linked to CAMEL-26333. Claude Code on behalf of allthingssecurity |
davsclaus
approved these changes
Oct 31, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
TL;DR: Approve. Every point from the last review is addressed, and I found no blockers.
Thanks for the quick follow-up:
- The upgrade guide entry is clear and keeps the CAMEL-19441 point (the route still stops after the doTry).
- Dropping the reduced test was the right call. The remaining tests assert the
mock:deadcount and each recipient's count explicitly. - The two
FinallyProcessorcomments answer both inline threads, so they can be resolved. - Thanks for filing CAMEL-26434 for the doCatch case.
I traced the flag handling again: the snapshot, the clear, and setting the flags again in FinallyAsyncCallback all behave correctly for the four outcomes of the block (handled, continued, unhandled, no failure). There is no overlap with #28755.
Minor, non-blocking: if (failureHandled) in the restore is always true in practice, as your new comment says. Fine to keep.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
This was referenced Nov 1, 2026
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
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.
Description
CAMEL-26333
When a
doTrycalls a route whose error handler handles a failure (a dead letter channel, oronException(...).handled(true)), thedoFinallyblock runs only its first step and silently skips the rest.When
direct:bfails,mock:f1gets the message, butmock:f2andmock:f4do not, and the headerf3is not set. When it does not fail, all of them run. ThedoFinallyEIP is described (model and catalog) as steps "to always execute after a doTry block completes, regardless of whether an exception was thrown or caught". Main, 4.23.0, 4.22.x and 4.18.x all behave this way (the sameFinallyProcessorandPipelineHelpercode).Cause: the called route's error handler marks the exchange as handled by the error handler (
errorHandlerHandled) and as failure handled (failureHandled).FinallyProcessorclears the exception before it runs the block, but not those marks. ThedoFinallyblock is aPipeline, which always runs its first step and then stops before each later step when the exchange is handled by the error handler (PipelineHelper.continueProcessing, "Handled by the error handler"). And an error handler does not handle a new failure of an exchange that is failure handled (RedeliveryErrorHandler.isDone), so a route called from such adoFinallycould not handle or continue its own failure with its error handler.A second consequence is fixed by the same change. A recipient list is a pipeline of two processors: the first evaluates the recipients into the exchange property
CamelEvaluateExpressionResult, and the second sends and removes it. As the first step of such adoFinally, the recipient list only evaluated, and the value stayed on the exchange. A routing slip or dynamic router that ran later on the same exchange took that value as its slip (RoutingSlip.process), so it sent to the recipient list's endpoints instead of its own. When the value was not a valid list for the routing slip's delimiter, the routing slip sent to onedirect:endpoint without a consumer and blocked for the direct timeout (30 seconds).Changes:
FinallyProcessorclears both marks while thedoFinallyblock runs, asOnCompletionProcessordoes for an onCompletion, and sets them again when the block is done (the handled mark only if the block did not set it itself). Every step of the block runs, a route that the block calls handles or continues its own failure with its error handler as it does when thedoTrydid not fail (a handled failure there stops the rest of the block, as in any route), and the route still stops after thedoTryas before. CAMEL-19441 decided that a caller does not continue routing after a called route handled the failure, and this stays true (the new tests check it: without setting the mark again,mock:endwould get the message).doFinallynow sends to its recipients and removes its property, so nothing stale is left for a routing slip.doFinallyrun (and a recipient list, routing slip ortothere send their messages), once per exchange. A route called from such adoFinallythat fails is now handled (or continued) by its own error handler; before, its error handler left the failure alone because the exchange was still marked as failure handled, and the exchange failed with that exception. Nothing else changes for them: the outcome of thedoTrybody, the dead letter queue and the stop after thedoTryare the same.RoutingSlip.processstill readsCamelEvaluateExpressionResult, because camel-bean's@RoutingSlipand@DynamicRouterannotations hand their result to the routing slip in that property (MethodInfo;RoutingSlipPOJOTestpasses). A dedicated key for that hand-off (the "TODO: Maybe use a new constant than EVALUATE_EXPRESSION_RESULT" from CAMEL-13449) would stop a routing slip from reading another EIP's value at all. It touches camel-bean, so I would file it as a separate minor issue if you want it.Tests:
DoFinallyAfterHandledCalledRouteTest(camel-core, 6 tests, every send bounded, no sleeps): a dead letter channel, the same with asynchronous delayed redelivery (thedoFinallyblock then runs on another thread, which the test checks), and a handledonException. Each checks everydoFinallystep, the dead letter queue, that thedoCatchdoes not run, and that the route does not continue after thedoTry(CAMEL-19441). Two more tests call a failing route from thedoFinally: its dead letter channel handles that failure too (which stops the rest of the block), and itsonException(...).continued(true)continues it (the block then runs to the end). These 5 fail on main. A control where the called route does not fail passes on main as well.RoutingSlipInDoFinallyAfterRecipientListTest(camel-core, 4 tests): a routing slip after the recipient list with a dead letter channel and with a handledonException, and a dynamic router. Each checks themock:deadcount, that each recipient and the routing slip's own endpoint get the message once, and that the recipient list removed its value. These 3 fail on main. A control where the called route does not fail passes on main as well.FinallyProcessornot setting the handled mark again after the block: the 3 bug tests ofDoFinallyAfterHandledCalledRouteTestthen fail onmock:end. With aFinallyProcessorthat clears only the handled mark, the two tests with a failing route called from thedoFinallyfail (the exchange fails with that route's exception).RoutingSlipNestedSlipRedeliveryTest) and the fix for adoCatchafter a continued failure in a called route (another branch of this batch) applied on the same tree: the routing slip, dynamic router, recipient list, doTry/doCatch/doFinally, error handler and onCompletion tests pass (674 tests).*Try*,*Catch*,*Finally*,*OnException*,*Redelivery*,*DeadLetter*,*ErrorHandler*,*Continue*,*RoutingSlip*,*RecipientList*,*DynamicRouter*,*Pipeline*,*OnCompletion*): 984 tests, 0 failures, 10 skipped, includingRoutingSlipPOJOTest. After the review commit (upgrade guide, comments, one reduced test dropped): the*Finally*,*Try*,*Catch*,*RoutingSlip*,*RecipientList*and*OnCompletion*tests of camel-core, 408 tests, 0 failures.SimpleSyntaxHintsTest(5), two OGNL hint tests ofSimpleTest,FileProducerChecksumOfWrittenFileTest(3),FilePollEnrichDynamicFileNameTestandValidatorExternalResourceTest(it downloads a schema). The core modules before camel-core passed, exceptModelWriterTest(4) andYamlModelWriterTest(1) in camel-xml-io and camel-yaml-io, which fail because my offline build regenerates the model writers differently (the regenerated files are not part of the commit).How it was found: a Rocq specification of Camel's error handling, written from the documentation, was compared with real Camel on generated routes. The cases were then reduced and reproduced on HotSpot.
Backport candidates, for the maintainers to label if wanted (contributors cannot set
port/*labels): camel-4.22.x and 4.18.x have the same code (FinallyProcessor,Pipeline,PipelineHelper). Routes see the second and laterdoFinallysteps run where they did not before, so I leave the backport to the reviewers.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.(I built the core modules and ran the tests above, including the formatter and import-sort plugins. No generated file changes. I did not run the full root build.)
AI-assisted contributions
Co-authored-bytrailers) 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-Bytrailer.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code