CAMEL-25008, CAMEL-25007: camel-core - Supervising route controller: fix a deadlock and a lost restart task - #26867
Open
allthingssecurity wants to merge 2 commits into
Conversation
… the new restart task of a route when the old one completes BackOffTimerTask.complete() calls its consumers each time it is called, and a task that is cancelled while its attempt runs is completed twice: by cancel(), and again by run() when the attempt returns. The consumer registered by RouteManager.start ended with routes.remove(r). If the route got a new restart task in between, for example because a manual startRoute failed, the second completion of the old task removed the new task from the routes being restarted. The new task kept running, but getRestartingRoutes(), getRestartingRouteState(id) and hasUnhealthyRoutes() no longer reported it, and stopRoute could not cancel it, so it started the route after the route was stopped. The consumer now removes the route's entry only if it is still its own task (routes.remove(r, task)). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ackOffTimerTask while holding its lock BackOffTimerTask.complete() called the registered consumers while it held the task lock. The consumer registered by the supervising route controller takes the controller lock, while stopRoute, startRoute, suspendRoute and resumeRoute hold the controller lock and cancel the route's restart task, and cancel() calls complete(), which takes the task lock. When a route operation took the controller lock while the restart task of the same route was completing (the attempt succeeded or the attempts were exhausted), both threads waited for each other forever. The route controller was then unusable, and no route was restarted any more. complete() now copies the consumers under the lock and calls them after releasing it. The lock only protects the list of consumers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 tasks
Contributor
|
Coordination heads-up: there are currently three open PRs reworking
#26859 and this PR edit adjacent lines in the same Since this PR restructures the task/controller locking that the other two depend on, it is the natural base: landing it first and rebasing #26868 and #26859 on top would let all three concurrency/health fixes coexist. Flagging so the three can be sequenced; I'll do the detailed review here once CI has run. This note was generated with AI assistance and issued by the human operator. Claude Code on behalf of oscerd |
Contributor
Author
|
@oscerd thanks for mapping this out. Landing this one first sounds right to me. I checked the three together:
Whichever order they merge in, I'll rebase #26868 if needed. Claude Code on behalf of allthingssecurity |
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
This PR fixes two bugs in the restart tasks of the supervising route controller, one commit each. The second fix would make the first bug easier to hit, so they go together.
stopRoute.CAMEL-25008: deadlock between the task lock and the controller lock
The supervising route controller could deadlock when a route was stopped or started while its restart task was completing. Two locks were taken in opposite orders:
BackOffTimerTask.run()->complete()takes the task lock and, while holding it, calls the consumer registered byRouteManager.start, which takes the controller lock;stopRoute/startRoute/suspendRoute/resumeRoutetake the controller lock and callrouteManager.release(route)->task.cancel()->complete(), which takes the task lock.complete()runs when an attempt succeeds, when the attempts are exhausted, and on cancel. The restart thread waits for the controller lock whenever any route operation holds it, since the lock is shared by all routes. After the deadlock, every later route operation blocks, and the supervisor thread (a single thread by default) never restarts another route.Thread dump from a reproduction with the real controller (routes
broken, whose consumer cannot start, andother):This change:
BackOffTimerTask.complete()copies the consumers under the lock and calls them after releasing it. The lock only protects the list of consumers. The controller's consumer does its bookkeeping under the controller lock, androutesis aConcurrentHashMap. Its finalroutes.remove(r)relied on the deadlock, though: once the restart thread no longer blocks a user operation, a completion of an old task can run after a failedstartRouteregistered a new task, and remove that task (CAMEL-25007). This is why the two commits are in one PR, CAMEL-25007 first.Tests: new
SimpleBackOffTimerTest.testCancelWhileCompletionCallbackWaitsForLock. It reproduces the same lock order without Camel: the completion callback takes an "owner" lock, and the owner cancels the task while it holds that lock, after the callback is queued on it (ReentrantLock.hasQueuedThread, no sleeps). Without the fix:With the fix it passes.
*BackOff*,*SupervisingRouteController*,*RouteController*in camel-util and camel-core, plusManagedSupervisingRouteControllerTestin camel-management: all pass.I also wrote a test against the real controller that stops a route from inside the controller lock while the route's restart task completes. It passes with the fix. Without the fix it deadlocks, and the forked JVM then hangs in
CamelContext.stop(). So I left it out of this PR and kept the unit test, which fails cleanly.Found with a TLA+ model of the supervising route controller and
BackOffTimerTask(both locks, user operations, restart tasks), then reproduced against the real classes (3 of 3 runs deadlocked, with no thread held by the harness). With only this change, the model has no lock-order cycle, and every user operation terminates (up to 3 user operations and 3 restart tasks, 53k states).CAMEL-25007: a completing old task removes the new restart task
BackOffTimerTask.complete()calls its consumers each time it is called. A task that is cancelled while its attempt runs is completed twice: once bycancel(), and again byrun()when the attempt returns. The consumer thatDefaultSupervisingRouteController.RouteManager.startregisters ended with an unconditionalroutes.remove(r).If the route got a new restart task in between, the second completion of the old task removed the new task. This happens, for example, when a manual
startRoutefails, since that registers a new task. The new task kept running, but:getRestartingRoutes()andgetRestartingRouteState(id)no longer showed it;hasUnhealthyRoutes()reported healthy while the route was down and being restarted;stopRoute(id)could not cancel it (releasefinds nothing), so it started the route after the route was stopped.This change: the consumer removes the route's entry only if it is still its own task,
routes.remove(r, task). The second completion of a cancelled task then has no effect: its status isInactiveorCompleted, so the exhausted bookkeeping is skipped too.Tests: new
DefaultSupervisingRouteControllerStartWhileRestartingTest. A route fails to start, and its first restart attempt is held in theRouteRestartingEventnotifier. The route is then started manually, which fails and registers a new restart task. The old attempt is then released and fails as well. The test waits for the old task's second completion (awhenCompleteregistered after the cancel, no sleeps) and checks that the new task is still the route's restart task and is reported as unhealthy. Without the fix:With the fix it passes.
*BackOff*,*Supervising*,*RouteController*in camel-util, camel-core and camel-management: all pass.Found with a TLA+ model of the supervising route controller (restart tasks, user operations, both locks), then reproduced against the real classes. With only this change, "every restart task that can still run is the route's current task" holds with up to 3 user operations and 3 tasks (53k states). In the reproduction, the route now stays reported as restarting, and
stopRoutecancels the new task, so the route stays stopped.Neither commit touches
RouteManager.release(), so this does not conflict with #26859 (CAMEL-25001).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 modules, including the formatter and import-sort plugins. 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