Repository navigation
CAMEL-26434: camel-core - a route called from a doCatch handles its own failure after the doTry called a route that failed unhandled - #28786
Conversation
…wn failure after the doTry called a route that failed unhandled A route called from the doTry (such as with direct) that fails unhandled marks the exchange as failure handled and as not handled by the error handler before the exception comes back to the caller. CatchProcessor cleared only the exception, so the error handler of a route called from the doCatch gave its failure straight back: no redelivery, no dead letter channel, no onException, and the exchange failed with that route's exception, while the same doCatch works when the doTry fails in the same route. CatchProcessor now clears both marks when the doCatch takes over the exception. When the block is done, failure handled is set again (one way), and a handled mark only when it was set and true and the block left it unset (the "set and true" snapshot that CAMEL-26333 uses for the doFinally block). The "not handled" mark of the caught failure is not set again, as when the doTry failed in the same route, so a doTry and doCatch inside a handled onException no longer make the exchange fail with the caught exception. The route continues after the doTry as before when the doCatch block does not call a failing route. A later doTry in the same route, after such a doCatch, is fixed the same way, and the copies made later by a split and the like no longer carry "not handled". Upgrade guide entry for 4.24. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…upgrade guide as a list, comments - DoCatchCalledRouteAfterUnhandledCalleeTest: a route called from the doCatch handles its failure and the doFinally has two steps (both run), with its in-route control (CAMEL-26333 is on main now). - The asynchronous test compares the thread of the called route with the thread that sent, instead of matching the delayer's thread name. - CatchProcessor: say that no known case reaches the restore of a true handled mark, kept for symmetry with FinallyProcessor. - Upgrade guide: a lead paragraph and a list of the other effects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 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, 23.9s total)Total reactor time: 23.9s
Top 20 slowest modules:
|
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: Thanks @allthingssecurity, a careful fix: CatchProcessor now clears the callee's failure marks while the doCatch block runs, as FinallyProcessor does since #28756, and the in-route controls show both paths now behave the same. Leaving it as a comment for now: the branch predates #28756 and the doCatch + two-step doFinally test promised for that point is still missing (finding 1). The rest are follow-ups and small items.
What I checked:
- The clear-on-take-over and one-way restore in the callback, against
RedeliveryErrorHandler,PipelineHelper.continueProcessing, theAbstractExchangecopy constructor and the other readers of the "not handled" mark (BridgeExceptionHandlerToErrorHandler,DefaultErrorRegistry,ShareUnitOfWorkAggregationStrategy); they now match the in-route case. CAMEL-2577/7275, CAMEL-25114 and CAMEL-26332/26333 are not reverted; the cost is on the failure path only, and the async path is covered. - Ran 134 doTry/doCatch/doFinally tests of camel-core, 0 failures, on main (with #28756) + this PR + #28779, plus probes against in-route controls: a DLC-handled callee followed by a two-step
doFinally, and a called route with aroutingSlip/multicastundermaximumRedeliveries(2)(3 attempts, as the control). - #28779 merges cleanly in either order (only the upgrade guide is shared) and complements this PR: this one makes the called route's error handler run at all, #28779 gives its EIP outputs one.
Findings:
-
Please rebase on main and add the
doCatch+ two-stepdoFinallytest now. #28756 was merged on 2026-10-31 (ba044ff), and the branch is still on 0fd7d9a. The description promises this test "once #28756 is merged". I checked the case on the merged tree and it passes, so it should be a quick addition. -
Please file the follow-up JIRA described in the PR and link it from CAMEL-26434. Here is one more case for it, reproduced on the merged tree. A route with a DLC calls
direct:inner. Indirect:inner, thedoTrycalls a route that fails unhandled and thedoCatchthrows. The outer DLC does not run and the exchange fails with thedoCatch's exception. With the in-route control (thedoTrythrows indirect:inneritself), the outer DLC gets the message. This happens becausefailureHandledis set again atCatchProcessor.java:214-223. That is the same on main, so it is not a regression, but it is the same root cause as the follow-up. -
(minor) Upgrade guide,
camel-4x-upgrade-guide-4_24.adoc:198: the entry is 3 paragraphs, and the second one lists 8 secondary effects in a single sentence chain. Consider a short lead paragraph followed by a bullet list (laterdoTry/onCompletion/ copies bysplitand the like /onException/bridgeErrorHandler/ error registry and JMX). It would scan better next to the #28779 and #28756 entries for the same area. -
(minor)
CatchProcessor.java:218: the restore of atruehandled mark is defensive, and the description says no test reaches it. Keeping it for symmetry withFinallyProcessoris fine. Consider saying in the comment that it is unreachable today, so a later reader does not hunt for the case. -
(minor, test)
DoCatchCalledRouteAfterUnhandledCalleeTest.java:281asserts that the thread name contains"Delay", which ties the test to the delayer's thread naming.assertNotEquals(startThread, ...), as inasynchronousCallee, would check the same thing without depending on the name. ThegetReceivedCounter()checks after a synchronoustemplate.sendare deterministic, so they are fine as they are.
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.
|
The follow-up is filed and linked from CAMEL-26434: CAMEL-26447. It has your nested Claude Code on behalf of allthingssecurity |
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: Approve. Thanks @allthingssecurity, a95c66f covers all five points: the branch has #28756, doFinallyWithTwoStepsAfterDoCatchCalledRouteHandledItsFailure and its control are in, CAMEL-26447 is filed for the follow-up (with the outer dead letter channel case), the upgrade guide is a list, the comment at CatchProcessor.java:189 says the true-mark restore is kept for symmetry, and the thread assertion no longer depends on the delayer's thread name. CI is green.
Small thing: the PR body still says the follow-up JIRA "is drafted and will be linked"; it is CAMEL-26447 now.
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.
|
Thanks for the review. Fixes are in a95c66f. The branch now includes main (merge commits bdef13d and baedff3, no rebase).
Tests: the class now has 23 tests, and 14 of them fail with the Claude Code on behalf of allthingssecurity |
gnodet-bot
left a comment
There was a problem hiding this comment.
Correct and carefully scoped fix. CatchProcessor now clears callee failure marks before the doCatch block runs and restores them in the async done() callback, mirroring FinallyProcessor's pattern from #28756 with the necessary additional handling of the "failure handled but not error-handler-handled" case.
Fix analysis — CatchProcessor.java
The snapshot/restore logic is sound. When a callee fails unhandled (default error handler), its error handler sets errorHandlerHandled=false and failureHandled=true. Without this fix, both stale marks leak into the doCatch block's route call, causing RedeliveryErrorHandler to bail out early (line 789 checks isErrorHandlerHandledSet and returns immediately if set). The fix correctly clears both marks before the doCatch processor runs.
The asymmetry with FinallyProcessor is correct and necessary: CatchProcessor must clear and restore both marks because unhandled callees set both; FinallyProcessor only triggers on errorHandlerHandled (set AND true) because by the time doFinally runs, the doCatch block has already consumed the exception.
The restore condition for errorHandlerHandled — guarded by !ext.isErrorHandlerHandledSet() — is precisely right: if a DLC inside the doCatch block already handled the failure, that outcome must be preserved, not overwritten by the callee's stale false.
All captured variables (ext, failureHandled, errorHandlerHandled, restore) are declared final, which is correct for the async callback closure.
Tests
Comprehensive: viaCallee routes exercise the bug path (callee's error handler sets both marks), inRoute variants confirm unaffected baseline, async variants cover the asyncDelayed() case, and the nested case verifies DLC-handled inner failures stop the caller correctly. Each test runs in its own CamelContext (via ContextTestSupport), so mock endpoint counts and AtomicInteger attempt counters are properly isolated.
Upgrade guide
Complete and well-explained. The three scenarios (no-handler, default-EH, DLC) are all covered.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Description
CAMEL-26434
From the side question in the review of #28756 (CAMEL-26333).
When the
doTryblock calls a route (such as withdirect) that fails unhandled (default error handler, noonException), a route called from thedoCatchcannot handle its own failure with its error handler. Its dead letter channel or handledonExceptiondoes not run, and the exchange fails with that route's exception. When thedoTryfails in the same route, the samedoCatchworks:On main,
direct:viaCalleefails withIOException: in catchandmock:deadgets nothing. With.throwException(new IllegalArgumentException(...))in thedoTryinstead of.to("direct:failing"),mock:deadgets the message and the exchange does not fail. The same holds for a handledonExceptionin the called route.With this PR the
doCatchbehaves as when thedoTryfailed in the same route. Two points differ from what the JIRA's proposal might suggest, on purpose:doCatchhandles its failure, the rest of thedoCatchblock and the steps after thedoTrydo not run, as for any failure that a called route handles (CAMEL-19441) and as in the in-route case. The caller continues after thedoTryas before when thedoCatchblock does not call a failing route (or the called route continues its failure);doCatchhandled that failure; setting "not handled" again made adoTry/doCatchinside a handledonExceptionfail the exchange with the caught exception (see below).Cause: the callee's error handler marks the exchange as failure handled (
ExchangeExtension.isFailureHandled() == true) and as not handled by the error handler (errorHandlerHandled=false) before the exception goes back to the caller.CatchProcessorclears the exception but not these marks. In the route called from thedoCatch,RedeliveryErrorHandler.isDonetreats a failure handled exchange as done, so the error handler gives the failure straight back (no redelivery, no dead letter channel, noonException), andprepareExchangeAfterFailurekeeps a "not handled" that is already set. It is the same kind of cause as CAMEL-26333 (#28756) for thedoFinallyblock, with the marks of an unhandled callee failure instead of a handled one.Changes
CatchProcessorclears both marks when thedoCatchtakes over the exception (only when one of them is set), as the JIRA proposes. When the block is done:FinallyProcessorin CAMEL-26333: camel-core - doFinally runs only its first step after a called route's error handler handled a failure #28756. When a route called from the block handled its failure, itstruestays, so the rest of the block and the caller after thedoTrystop, as for any failure that a called route handles (CAMEL-19441) and as when thedoTryfailed in the same route;doCatchhandled that failure. This is a difference from main, on purpose: with it set again, adoTry/doCatchinside anonException(...).handled(true), whosedoTrycalls a failing route, made the exchange fail with the exception thedoCatchcaught (the outer error handler saw "not handled" and putCamelExceptionCaughtback). The new testdoTryInHandledOnExceptionCallsRouteThatFailscovers it.doCatchtakes over, nothing changes. This is the case when thedoTryfailed in the same route, unless an earlierdoCatchof the route caught a callee failure: failure handled is set again after that one, so a laterdoTryin the same route had the same bug on main, and is fixed too (testsecondDoTryAfterCaughtCalleeFailure).doCatchafter an unhandled callee failure, all in the direction of the in-route case:onExceptionor a context-scoped one, logs the exhausted failure, andCamelFailureRouteIdnames that route (testredeliveryOfRouteCalledFromDoCatchAfterUnhandledCallee: 3 attempts withmaximumRedeliveries(2), 1 on main);doCatchthat only logs, the error registry now records the caught callee failure as handled (CatchProcessorfires the failure handled event before the marks are set again), and the JMXfailuresHandledcounters of the steps inside the block no longer count it; as for an in-route failure;split,multicast,recipientList,enrich,wireTap) take over the handled mark but not failure handled. They used to inherit "not handled", so a route called from them handled its failure with its dead letter channel but the exception came back; now it stays handled, as in the in-route case (testsplitAfterCaughtCalleeFailure);bridgeErrorHandler=trueon the consumer, a failure of such an exchange that is not handled (adoCatchthat fails, or a later failure in the route) now goes to the route's error handler, as an exchange with the bodyError processing exchange, instead of only being logged (BridgeExceptionHandlerToErrorHandleronly logs when the mark is "not handled");ResumableCompletionand the route'sfailuresHandledcounter see the same as before.doTry). The routes called from thedoCatchhave error handlers of their own; thedoCatchchildren are still not wrapped in one.doTry, adoTryin anonCompletion, the copies made bysplitand the like, theonExceptioncase,bridgeErrorHandler, the error registry and the JMX counters), and the way back:noErrorHandler()on the called route (which applies to every caller of that route).With #28756
#28756 (CAMEL-26333) is merged, and main is merged into this branch. A
doCatchwhose called route handles its failure, followed by adoFinallywith two steps, runs both steps, as when thedoTryfails in the same route (testdoFinallyWithTwoStepsAfterDoCatchCalledRouteHandledItsFailureand its control). Without this PR the exchange fails with the called route's exception there.Tests
New
DoCatchCalledRouteAfterUnhandledCalleeTest(camel-core, 23 tests, JUnit assertions as the other doTry tests of the package, no sleeps). Most cases through a failing callee have a control where thedoTryfails in the same route, with the same expected outcome:onExceptionin the route called from thedoCatch, each with its in-route control. The handled failure stops the rest of thedoCatchblock and the caller, in both;onExceptionin the route called from thedoCatch(the rest of the block and the caller then run), with its control;doCatchwhose error handler redelivers and does not handle: 3 attempts, the exchange fails with that route's exception, with its control;doCatchthat fails asynchronously (a delay withasyncDelayed; the test checks it continued on another thread than the one that sent), with its control;doCatchthat handles its failure, followed by adoFinallywith two steps: both steps run, with its control;doTry/doCatchinside a handledonException, whosedoTrycalls the failing route: theonExceptionhandles the failure, with its control;doTryin the route after adoCatchcaught the callee failure: the route called from the seconddoCatchhandles its failure, with its control;splitafter adoCatchthat caught the callee failure, calling the route with the dead letter channel: the failure stays handled and the route continues after the split, with its control;doCatchruns on another thread than the one that started the route (checked);doTry(no in-route control): the innerdoCatchcalls the route with the dead letter channel (the outerdoCatchdoes not run); and the innerdoCatchfails, so the outerdoCatchtakes over and calls that route (the outerdoCatchclears the marks again);doTrywhen thedoCatchdoes not call a failing route, and adoCatchthat throws fails the exchange with its exception: failure handled as before, no "not handled" mark (these two pass on main except for that mark).Without the fix, 14 of the 23 tests fail; the 9 in-route controls pass on main as well. Mutants of the fix, each failing at least one test: no restore after the block, clearing only one of the two marks, a restore that always sets the earlier handled mark back (over what the block set), restoring failure handled only after a handled failure, setting "not handled" back. The restore of a
truehandled mark is defensive (a failure that an error handler handled has no exception left to catch): no known case reaches it (a handledonExceptionwhose handler throws does not either, checked with a scratch test), so no test reaches it, and removing it fails no test. The comment inCatchProcessorsays so; it is kept for symmetry withFinallyProcessor.org.apache.camel.processor.**(witherrorhandler,onexception,exceptionpolicyand the other subpackages) plus the doTry/doCatch/doFinally, redelivery, error handler and exception tests oforg.apache.camel.issues: 2980 tests, 0 failures, 24 skipped. After the review commit and the merge of main f901a8c: the doTry/doCatch/doFinally and split tests of camel-core, 410 tests, 0 failures, 4 skipped; the new class 20 times in one JVM again: 460 tests, 0 failures.Follow-up JIRA
Filed as CAMEL-26447 (linked from CAMEL-26434), with the case from the review.
The failure handled mark of the unhandled callee failure still stays on the exchange after the
doTry(as on main, and on purpose in this PR). Two consequences remain, reproduced on main and with this PR (scratch test, not in the PR):doTry, is not handled by the calling route'sonException(the exchange fails with it), while it is when thedoTryfailed in the same route (withbridgeErrorHandler=truethe bridge now gives it to the error handler, see above). Likewise a route called later with a plain.to("direct:x")still cannot handle its own failure, while the same route called throughenrichor asplitnow can, as the copy does not carry failure handled;doFinallyafter such adoCatch(or after the unhandled callee without a matchingdoCatch) cannot handle its own failure either. CAMEL-26333: camel-core - doFinally runs only its first step after a called route's error handler handled a failure #28756 clears the marks around thedoFinallyonly after a handled callee failure.Clearing failure handled for good when a
doCatchcatches the exception would fix both, but it changes what the readers of that mark see at the end of the exchange: the idempotent consumer (IdempotentOnCompletion) then confirms the message instead of removing it,ResumableCompletionrecords the offset, and the JMX and MicrometerfailuresHandledcounters no longer count it. That needs its own JIRA and upgrade guide entry, so I kept this PR to thedoCatchblock. The review found one more case with the same cause, also on main: a route with a dead letter channel calls a route whosedoTrycalls a route that fails unhandled and whosedoCatchthrows; the outer dead letter channel does not run (it does when thedoTrythrows in the inner route itself), because thedoCatchsets failure handled again when its block fails. The follow-up JIRA with these cases is drafted and will be linked from CAMEL-26434.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