CAMEL-25002: camel-core - Loop EIP: a negative or huge loop count must not break graceful shutdown - #26860
Open
allthingssecurity wants to merge 1 commit into
Conversation
…t not break graceful shutdown
LoopProcessor added the evaluated loop count to its pending task counter
without checking the sign. A count of zero or less runs no iteration, and
nothing compensated a negative add, so after a single message with, for
example, loop(header("n")) and n=-1, getPendingExchangesSize() returned -1
for the rest of the route's life. This regressed in CAMEL-16794, which
switched to a LongAdder with an unconditional add(count) and dropped the
clamp CAMEL-15578 had.
DefaultShutdownStrategy adds the pending sizes of the route's services to its
inflight count, in int, and only waits while the sum is positive. The same
LoopProcessor is a child of several route services and is counted once for
each of them. The negative value cancelled real inflight exchanges, so a
graceful shutdown stopped the route immediately and the inflight exchange
failed with a RejectedExecutionException. A huge loop count (from about 2^29)
had the same effect, because the int sum overflowed.
A loop that breaks on shutdown also left its remaining iterations pending,
as the release added in CAMEL-19738 only ran when the loop stopped due to an
exception, so every later shutdown waited for its full timeout.
LoopProcessor now only adds a positive count, releases the iterations left
exactly once whenever the loop ends (normally, on an exception, or when it
breaks on shutdown), and getPendingExchangesSize() no longer wraps or goes
negative. DefaultShutdownStrategy sums the pending sizes in a long, ignores
negative sizes, and caps the result at Integer.MAX_VALUE.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
oscerd
reviewed
Sep 25, 2026
oscerd
left a comment
oscerd
left a comment
Contributor
There was a problem hiding this comment.
I traced the full pending-counter lifecycle against the head revision and the root-cause analysis holds up.
LoopProcessor
- The counter is now only incremented for a positive count, and
getPendingExchangesSize()readstaskCount.sum()clamped to[0, Integer.MAX_VALUE]instead of the wrappingintValue(). - The accounting partitions cleanly: iterations that actually run are decremented one-by-one in the async callback (
index++; taskCount.decrement()), and the iterations that never run (gap = count - index) are released in a singletaskCount.add(-gap). Their sum is exactlycount, so a completed loop nets to zero and an early exit (exception,breakOnShutdown,continueProcessingfalse, predicate false) releases exactly the remainder — including the first-run break case wheregap == count. releasePendingTasks()is reached from both the done branch andhandleException, and thependingTasksReleasedguard makes the second call a no-op. That covers the specific case called out in the comment —callback.done(false)throwing after a normal completion re-enters via the catch — so there is no double subtraction.LoopStateis instantiated per exchange, soindex/count/pendingTasksReleasedare per-execution; onlytaskCountis shared, so the guard is not a concurrency hazard.
DefaultShutdownStrategy
getPendingInflightExchangesnow accumulates in along, clamps each service's contribution withMath.max(0, …)so one service cannot cancel another's inflight work, and caps the result atInteger.MAX_VALUE; the per-route sum with the inflight-repository size is widened and capped the same way. That independently closes both the negative-size and the ~2^29 overflow paths, and the cap still returns a positive value so the strategy keeps waiting.- Both halves are needed, matching the test matrix in the description.
I also checked the other open shutdown-related EIP PRs (#26870, #26851, #26859, #26868) — none of them touch DefaultShutdownStrategy or LoopProcessor, so there is no overlap to reconcile here.
The logic looks correct and the three new tests are deterministic (Awaitility, no sleeps). CI has not been triggered on this PR yet (fork PR awaiting a maintainer to approve the workflow run) — I'll confirm once the build is green.
This review was generated with AI assistance and reviewed/issued by the human operator. Claude Code on behalf of oscerd
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:
🔬 Scalpel shadow comparison — Scalpel: 557 of 694 tested, 28 compile-only — current: 558 all testedMaveniverse Scalpel detected 557 affected modules (current approach: 558). Skip-tests mode would test 557 modules (3 direct + 554 downstream), skip tests for 28 (generated code, meta-modules) Modules only in current approach (1)
Modules Scalpel would test (557)
Modules with tests skipped (28)
Build reactor — dependencies compiled but only changed modules were tested (3 modules, 13.9s total)Total reactor time: 13.9s
Top 20 slowest modules:
|
oscerd
approved these changes
Sep 25, 2026
oscerd
left a comment
oscerd
left a comment
Contributor
There was a problem hiding this comment.
CI is now green, and this is the same revision (f920f79) I traced in detail above, so the earlier verification stands: the pending-counter accounting is correct (sign-guarded add, per-iteration decrement vs a single bulk add(-gap) release, the exactly-once pendingTasksReleased guard covering the callback-throws-after-completion case, and per-exchange LoopState state), and the DefaultShutdownStrategy hardening (long accumulation, per-service Math.max(0, …), Integer.MAX_VALUE cap) independently closes both the negative-count and the ~2^29 overflow paths. The three new tests are deterministic.
LGTM.
This review was generated with AI assistance and reviewed/issued by the human operator. Claude Code on behalf of oscerd
5 tasks
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
2 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.
Description
CAMEL-25002
The Loop EIP could break graceful shutdown. A single message with a negative loop count, or an inflight exchange with a huge loop count, made the shutdown strategy stop waiting for inflight exchanges. The route was then stopped at once, and the inflight exchange failed with a
RejectedExecutionException.LoopProcessoradded the evaluated count to its pending task counter without checking the sign. A count of 0 or less runs no iteration, and nothing ever compensated a negative add. So after one message withloop(header("n"))andn=-1,getPendingExchangesSize()returned-1for the rest of the route's life.DefaultShutdownStrategyadds the pending sizes of a route's services to its inflight count, inint, and waits only while the sum is positive. The sameLoopProcessoris a child of several route services and is counted once for each (4 times in a plain route), so the negative value cancelled real inflight exchanges. A count from about 2^29 had the same effect, because theintsum overflowed. This regressed in CAMEL-16794, which switched to aLongAdderwith an unconditionaladd(count)and dropped theMath.max(count - index, 0)clamp that CAMEL-15578 had. It is the same symptom as CAMEL-18713 (loopDoWhile), whose fix did not cover the negative-count path.This change:
LoopProcessoronly adds a positive count to the pending counter. TheLOOP_SIZEproperty is unchanged.LoopProcessorreleases the iterations left exactly once whenever the loop ends. Before, it only did so when the loop ended because of an exception (the release CAMEL-19738 added). A loop that broke out on shutdown (breakOnShutdown) left its remaining iterations pending, so the shutdown waited for its full timeout.LoopBreakOnShutdownTestnow takes 1.1 s instead of 10.1 s, which was its 10 s shutdown timeout.add(-gap)call. The old code ran onedecrement()per iteration, which takes more than 10 s on the routing thread for a count nearInteger.MAX_VALUE.getPendingExchangesSize()keeps the sum between 0 andInteger.MAX_VALUE.LongAdder.intValue()wrapped.DefaultShutdownStrategy.getPendingInflightExchangessums in along, ignores negative sizes so that one service cannot cancel the pending exchanges of others, and caps the result. The per-route sum with the inflight repository size is capped the same way.I did not de-duplicate the services that are counted more than once in
getPendingInflightExchanges. With the sums capped, that only makes the numbers in the "Waiting as there are still N inflight and pending exchanges" log message higher, as before. Changing it would also change the counts reported for the aggregator and the wire tap.Tests: new
LoopPendingExchangesShutdownTest:testNegativeCountLeavesNoPendingTasks: counts-1,0and3leavegetPendingExchangesSize() == 0.testGracefulShutdownWaitsForInflightAfterNegativeCount: one message withn=-1, thencontext.stop()while a second exchange is inflight. The stop must wait and the exchange must complete.testGracefulShutdownWaitsForInflightWithHugeCount: the same with abreakOnShutdownloop andn=Integer.MAX_VALUE, and after the stop the loop has no pending iterations left.The inflight exchanges wait with Awaitility until the context is stopping, and the tests use no sleeps. With the fix they pass deterministically. Without it, the second test relies on the shutdown finishing its first pass before the inflight exchange wakes up, which it practically always does.
Without the fix, all 3 fail:
With only the
DefaultShutdownStrategypart of the fix (LoopProcessorunchanged), the first and third tests still fail (expected: <0> but was: <2147483646>for the iterations abreakOnShutdownloop left pending).With the fix, all 3 pass.
Loop*,*Shutdown*,AsyncLoop*,*PendingExchanges*in camel-core: 67 tests, 0 failures, 2 skipped (the existing*ManualTests).Found with a Lean 4 model of the loop's pending counter and the shutdown strategy's wait decision, then reproduced against the real classes.
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 and tested the affected core modules, including the formatter and import-sort plugins. I did not run the full root build. No generated files are affected.)
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) on behalf of allthingssecurity. The commit carries a
Co-Authored-Bytrailer.🤖 Generated with Claude Code