CAMEL-24583: camel-master - do not start the delegated consumer after leadership is lost (CAMEL-24584) - #26028
Merged
davsclaus merged 2 commits intoSep 2, 2026
Conversation
henrik242
changed the title
Contributor
PR review summaryAI-generated review on behalf of atiaomar1978-hub Verdict: Approve in principle — this is a well-scoped, production-motivated fix with strong tests. The two-commit split (camel-support first, then camel-master) is the right dependency order. What works well
Inline notes (non-blocking follow-ups)
Testing / CICI had not reported checks on the branch at review time. Locally the described matrix (7 new master tests + 2 BackgroundTask tests) is appropriate for this change set. Thanks @henrik242 for the thorough PR description and the transparent “known pre-existing issues” section — that made review much easier. |
davsclaus
reviewed
Sep 2, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Reviewed against this project's rule files (build/test/style conventions, commit/PR conventions, AI-attribution rules) and by building + running the changed modules in an isolated worktree: camel-support (BackgroundTaskTest, 8/8 pass) and camel-master (27/27 pass, including the new MasterConsumerLeadershipTest run 3x with no flakiness). The fix itself is well-reasoned and well-tested — I traced the lock.lockInterruptibly() + cancelLeaderTask(true) design in startDelegatedConsumer/doStop and didn't find a correctness bug in the leadership state machine.
Two non-blocking findings:
- Missing AI co-authorship trailer. The PR description states "Claude Code on behalf of henrik242", but the single commit (
f7cea19c2b) has noCo-authored-bytrailer. Per this project's AI-agent attribution rule, AI-assisted commits should carry one. - PR description vs. actual commit history. The description explains two ordered commits (camel-support first, camel-master second) with a specific rationale for that order ("the reverse order would leave a failing test at the intermediate commit"). The PR actually contains a single squashed commit touching both modules. Not wrong, just worth reconciling so a reviewer trying to verify the described ordering isn't misled.
One inline note below on a lock-ordering point the author already flagged transparently in "Notes for reviewers" — confirmed by reading AbstractCamelClusterView/BaseService, recommending a follow-up JIRA rather than blocking this PR on it.
This review does not replace CodeRabbit/Sourcery/SonarCloud or a dedicated concurrency review.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Croway
requested changes
Sep 2, 2026
Croway
left a comment
Croway
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR, the direction (never start the delegate after leadership was lost, retry on the next term) is the right one. Reviewing the locking changes though, there are three problems I think need to be addressed before this can go in. Details inline.
- Lock-order inversion between
BaseService.lockand the cluster view'sStampedLockthat can deadlock a leadership event overlappingstop(). - The delegated consumer is now created and started while holding
BaseService.lock, which blocks stop/suspend/resume and every cluster event for the duration of a slow start. - The new "Giving up after N attempts" log never fires with default settings, because the iteration/time budget exhausts after ~5 s (one attempt), so
backOffMaxAttemptsis effectively ignored.
Claude Code on behalf of Croway
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 563 tested, 26 compile-only — current: 562 all testedMaveniverse Scalpel detected 589 affected modules (current approach: 562).
|
henrik242
force-pushed
the
camel-24583-master-leadership-race
branch
from
September 2, 2026 15:30
f7cea19 to
9c85639
Compare
…ompleted or exhausted The future from schedule() was never cancelled, so a finished task kept being re-run as a no-op for the life of the executor. A caller cannot fix the exhausted case itself, as the supplier is never invoked again.
… leadership is lost Also makes backOffMaxAttempts bound the start attempts, which the default 5s time budget of the task ended before the second attempt.
henrik242
force-pushed
the
camel-24583-master-leadership-race
branch
from
September 2, 2026 16:12
9c85639 to
e53dc94
Compare
davsclaus
approved these changes
Sep 2, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Approving. I re-reviewed the current diff against this project's rules (build/test/style, commit/PR conventions, AI-attribution) and against the earlier CHANGES_REQUESTED review, and traced the leadership state machine plus the referenced APIs (BaseService.lock = ReentrantLock, AbstractCamelClusterView = StampedLock, withUnlimitedDuration, ExecutorServiceManager.shutdown with awaitTermination=0).
The three previously requested changes are all addressed in the current diff:
- Start no longer runs under
BaseService.lock—startDelegatedConsumerclaims under the lock, releases it, runscreateConsumer()/startService()off-lock, then re-acquires to publishdelegatedConsumer(and stops the freshly-started consumer if leadership was lost in the meantime). Exactly the shape suggested. backOffMaxAttemptsis now effective —.withUnlimitedDuration()means the default 5s budget no longer ends the task before the second attempt; the give-up ERROR is guarded bymaxAttempts > 0;testAllConfiguredStartAttemptsAreMadecovers it at default-scale timings.- Lock-order inversion (view
StampedLockvs consumerlock) — mitigated via the unlockedisRunAllowed()fast path, cancelling the pending task beforeremoveEventListenerindoStop, and moving the start off the lock.
One residual, non-blocking note: the lock-order inversion is mitigated but not fully eliminated — a narrow window remains where the event thread passes the fast isRunAllowed() check, then stop() transitions to STOPPING and holds lock while removeEventListener needs the view write lock, and the event thread parks on lock while holding the view read lock. This is genuinely pre-existing (the old "taken" path already re-entered the consumer lock from under the view read lock) and is transparently disclosed in the PR body and an inline comment, with a follow-up JIRA proposed. A complete fix means moving leadership state off BaseService.lock, which is out of scope here — reasonable to track separately.
Everything else checks out: CI green (build 17 & 25); strong, infra-free test coverage of the pending/in-progress/flap/stop-cancel/exhausted-recovery/attempt-count cases; no Thread.sleep (awaitility + latches); JUnit assertions correctly match the camel-master module convention; upgrade-guide entries present for both camel-support and camel-master behavior changes; commit messages follow CAMEL-XXXX: and the branch is the two ordered commits as described.
This review does not replace CodeRabbit/Sourcery/SonarCloud or a dedicated concurrency review.
This review was generated by an AI agent on behalf of davsclaus and may contain inaccuracies. Please verify all suggestions before applying.
Croway
approved these changes
Sep 2, 2026
davsclaus
added a commit
that referenced
this pull request
Sep 3, 2026
… leadership is lost (CAMEL-24584) (#26052) MasterConsumer started the delegated consumer from a BackgroundTask scheduled after the leadership-taken event. A leadership-lost event arriving during that window was dropped because delegatedConsumer was still null, leaving the consumer running on a non-leader node. Leadership is now tracked under the consumer lock; the scheduled task re-checks it before starting, the lost event is dispatched unconditionally to cancel a pending start, the delegate is created off the lock and published only if leadership still holds, and delegatedConsumer is published only after a successful start. backOffMaxAttempts now correctly bounds the attempts. Also fixes CAMEL-24584: BackgroundTask.schedule now cancels its repeating schedule once the task has completed or run out of budget, instead of re-running as a no-op for the life of the executor. Closes #26028 (cherry picked from commit f899b39) Co-authored-by: henrik242 <henrik242@users.noreply.github.com>
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
4 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.
Fixes CAMEL-24583 and CAMEL-24584.
Problem
MasterConsumerstarts the delegated consumer from aBackgroundTaskscheduled one second after theleadership-taken event. A leadership-lost event arriving inside that window was dropped, because the
listener only dispatched it
else if (delegatedConsumer != null)and that field is still null while thestart is pending. Nothing cancelled the pending task, so it started the consumer on a node that was no
longer the leader, and no further event was coming to stop it.
Seen in production with a Google Pub/Sub delegate behind
KubernetesClusterService: a three secondmembership flap left subscribers running on the follower pod, feeding routes whose error handlers had been
stopped, so messages were dropped rather than redelivered to the leader.
Changes
CAMEL-24584 - camel-support (first commit)
BackgroundTask.schedulecancels the future it creates once the task is completed or exhausted. Itpreviously stayed armed and the task kept being re-run as a no-op for the life of the executor.
CAMEL-24583 - camel-master (second commit)
starting the delegate.
cancels a pending start instead of being dropped.
otherwise it is stopped again.
delegatedConsumeris published only after a successful start, so a failed start no longer makes everylater leadership term a no-op.
backOffMaxAttemptsnow bounds the attempts. The task also carried the five second default duration ofits budget, which ended it before the second attempt at any
backOffDelayof five seconds or more.Testing
MasterConsumerLeadershipTestuses a fake cluster view whose leadership can be flapped on demand, andcovers the pending start, a loss while the start is in progress, a flap after a successful start, repeated
taken events, cancellation on stop, exhausted-retry recovery, and the configured attempt count. Two tests
added to
BackgroundTaskTest. All of them were checked against unpatchedmainand fail there.Known pre-existing issues
Found while working on this. None are introduced here and none are fixed here.
doStopholds the consumer'sBaseServicelock and then needs the view's write lock viaremoveEventListener, whileAbstractCamelClusterViewdispatches events holding its read lock and then needs the consumer lock.The inversion predates this change, but dispatching the lost event unconditionally widens exposure to
it. Mitigated here by an unlocked
isRunAllowed()fast path in the listener, by running the delegatedstart off the lock, and by cancelling the pending task before the listener is removed. The constraint
is now written down in
doStopso it survives the merge. It still deserves a follow-up JIRA, since aproper fix means moving the leadership state off
BaseService.lock.TaskManagerRegistryentries leak when a future is cancelled by the caller.runTaskWrapperonlyunregisters from inside a later run, which cannot happen once the future is cancelled. This affects
camel-sjms today and camel-master after this change. Better fixed by a
BackgroundTaskcanceloperation that also deregisters.
This change makes that state recoverable and logs it at ERROR, but adds no retry trigger.
One note on the diff rather than a pre-existing issue: the delegated consumer is created once per task and
reused across its attempts, so
addStartupListenerand the resume strategy are wired once. A consumer thatfailed to start is deliberately not shut down, since
DefaultConsumer.doShutdownwould also shut down theroute's processor, which the next attempt still needs.