allthingssecurity opened a new pull request, #29585: URL: https://github.com/apache/camel/pull/29585
# Description [CAMEL-26798](https://issues.apache.org/jira/browse/CAMEL-26798) Follow-up of #29532 (CAMEL-26784, `HazelcastRoutePolicy` releases the leadership when its route is removed), now merged; this is one commit on current main. It keeps what #29532 added after its review (the leadership threads told to end are kept and interrupted when the policy stops, the JMX route is one of the remaining routes after a removal). About the suggestion in the review of #29532 (in `takeLeadership`, wait for the policy lock with a `tryLock` loop that gives up once the leadership was released, so a stop does not wait about twice the executor shutdown timeout): with this change the leadership thread no longer takes the lifecycle lock of the service at all. The leader flag, the routes and the leadership thread are guarded by a lock of the policy that `stop()` does not hold while it waits for the thread, and a stop interrupts the thread before it takes that lock (`stopWhileAConsumerStartsAtTakeoverDoesNotWait`), so the wait the suggestion was about cannot happen here. When a node takes the leadership, `HazelcastRoutePolicy` starts the consumers it stopped while the node was a follower; its leadership thread then holds the Hazelcast lock until the policy stops (or, since #29532, until its routes are removed). Four problems in that start: - **A failed start was never retried.** When the start of a consumer failed (a broker not reachable yet at takeover), the exception was logged and nothing tried again: the node held the lock, so no other node could take over, and it did not consume. A failure also skipped the consumers of the other routes of the policy. Same defect as CAMEL-26540 (`MasterRoutePolicy`, #29123) and CAMEL-25315 (`MasterConsumer`), which retry while the node is the leader. - **A takeover could start the consumer of a route being stopped or suspended.** The route service stops (or suspends) the consumer and only then calls the policy's `onStop` (`onSuspend`), which forgets the route. A leadership taken in between started the consumer again: the route ended stopped or suspended with a running consumer, and two consumers after the next start. - **A leadership taken while a route started could be lost.** `onStart` read the leader flag without a lock, then stopped the consumer and remembered the route. A leadership taken in between found no consumer to start, and the route then stopped its consumer: the leader did not consume and the other nodes waited for the lock. - **A stop during a takeover waited.** The consumers were started under the lifecycle lock of the service, which `stop()` holds while it waits for the leadership thread, so the stop waited for the consumer starts or for the 10 seconds of `shutdownGraceful`. Change: - the leader flag, the route sets and the leadership thread handle are guarded by a lock of the policy (`leadershipLock`), taken by `onStart`, `onStop`, `onSuspend`, `onResume`, `onRemove`, `setLeader` and the leadership thread, only for short sections. It is not the lifecycle lock of the service. `onInit`, `doStart` and `doStop` take the lifecycle lock first (lock order: lifecycle, then leadership; the leadership thread takes only the leadership lock); - at takeover the flag is set and the routes to start are taken under that lock; the consumers are then started without any lock (a consumer start can take long), each on its own, and checked again under the lock: the route is forgotten only once its consumer started; a consumer started for a route that began to stop or suspend meanwhile is stopped again. A route whose consumer may not be started (`RoutePolicySupport.isResumeOrStartConsumerAllowed`, as the throttling and scheduled route policies use) is skipped; - while it holds the lock, the leadership thread tries the failed starts again every 5 seconds (it waits on its latch with that timeout). Each failure goes to the exception handler (WARN by default) with the route id and the attempt number; the recovery is logged at INFO; - `doStop`, and `onRemove` while the CamelContext stops, tell the leadership thread to end and interrupt it before they take the lock, so a consumer start in flight does not delay the stop; - a consumer whose restart fails when its route is resumed after a takeover (from #29532) is tried again too. What stays as it was: a leader that keeps failing keeps the lock (as with `MasterRoutePolicy`); the follower's consumer is still stopped after the route started it (changing that is a separate design question). A route that starts to stop between the check and the consumer start is caught by the check after the start, not before; and if the user starts the route again in that window, the check after the start can stop the new consumer, which `onStart` of the leader then hands to the leadership thread to start again. The leadership thread still calls `setLeader(true)`, so a subclass sees the takeover as before; it takes the leadership only for the running leadership task, so a task that ended its leadership (its routes were removed) does not take it for a route added meanwhile. A suspend that the shutdown strategy has begun while the route status is still started is not seen by the check: if the takeover starts the consumer after the strategy suspended it, the route ends suspended with a running consumer, as before. The restart in `onResume` still runs under the policy lock (it is on the route's own thread). Tests, in `HazelcastRoutePolicyLeadershipTest` (embedded Hazelcast member, the other node is the test thread holding the lock; no sleeps): - `consumerWhoseStartFailsAtTakeoverIsStartedAgain`: two routes on one policy, the consumer of one fails to start until a flag is cleared: the other must start meanwhile, the failed one must be tried again and start once it can; - `consumerOfARouteBeingStoppedIsNotStartedAtTakeover` and `consumerOfARouteBeingSuspendedIsNotStartedAtTakeover`: a second route policy, listed first, takes the leadership for this node inside `onStop` / `onSuspend`: the consumer must stay stopped, and start with the route (resume); - `consumerStartedWhileItsRouteStopsIsStoppedAgain`: the route is stopped after the check and before the consumer start: the consumer must be stopped again; - `stopWhileAConsumerStartsAtTakeoverDoesNotWait`: the consumer start at takeover blocks until interrupted: the stop of the policy must finish within 5 seconds; - `leadershipTakenWhileTheRouteStartsIsNotLost`: replays the interleaving that was possible before (the leadership is taken between the flag read and the stop of the consumer); it only fails on the old code; - `leadershipTakenWhileRoutesStartNeverLeavesTheLeaderIdle`: races `onStart` against `setLeader(true)` for 1000 rounds, without hooks. Against main (with #29532 merged; only the test hook `setRetryDelay` added, doing nothing), 6 of the 7 fail (the race test is probabilistic and passed in that run); against the first version of this change (lock held during the starts, own status check, no check after the start) the stop, the suspend and the stop-during-start tests failed. With the change the 7 pass, and the 9 tests of #29532 too: 140/140 and 180/180 over 20 runs in one JVM; the camel-hazelcast unit tests pass (295 run, 0 failures; 1 error in `HazelcastUserConfigSerializationFilterWarningTest.warnsForUserClientConfigWithoutFilter`, whose client cannot reach the member's non-loopback address in my sandbox, as on main; it does not use the route policy). The file is the same on the maintenance branches, so a backport would take #29532 first. Found with Specula (TLA+ model checked against traces of the real code) and a TLA+ model of the policy (two nodes, route start, leadership thread, shutdown), and confirmed with the tests above. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it): [CAMEL-26798](https://issues.apache.org/jira/browse/CAMEL-26798). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built the core modules and camel-hazelcast with the formatter and import-sort plugins and ran the camel-hazelcast tests; no generated file changes. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) 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-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
