allthingssecurity opened a new pull request, #28804: URL: https://github.com/apache/camel/pull/28804
# Description [CAMEL-26448](https://issues.apache.org/jira/browse/CAMEL-26448) `context.removeRoute("a")` can deadlock with the supervising route controller when route `a` is being restarted (it failed to start and waits for its next restart attempt). `removeRoute` takes the route lock of the CamelContext and shuts the route down. The route policy of the controller (`ManagedRoutePolicy.onRemove`) then calls `routeManager.release(route)` (added by CAMEL-25071 in 4.23.0), which cancels the restart task of the route. `BackOffTimerTask.cancel()` completes the task in the calling thread, so the `whenComplete` callback of the controller runs in the removing thread, still under the route lock, and it takes the lock of the controller. The controller holds its lock while it calls into the CamelContext, which takes the route lock: the restart attempt (lock, re-check that the task is still active, `super.startRoute`), and the manual start, stop, suspend and resume of the routes it controls (`doStartRoute` / `doStopRoute`). So when the restart attempt of route `a` has taken the lock and passed its re-check, and waits for the route lock that `removeRoute` holds, and `removeRoute` then cancels the task and waits for the lock of the controller, both wait forever. `removeRoute` never returns and the route lock is never released, so later route operations and the CamelContext stop hang too. The same happens with a manual `stopRoute("b")` (or start/suspend/resume of a route the controller knows) on another thread instead of the restart attempt. This can happen when a route that keeps failing to start (its broker or database is down) is removed while its restart attempt runs: a reload in development mode, a Kamelet or route template that is removed, or `removeRoute` from user code. The attempt has to start in the window between the removal taking the route lock and cancelling the task, which is as long as the removal takes to shut the route down (close its connections). The `whenComplete` callback needs the lock only for an exhausted task (it then moves the route out of the supervision; otherwise it only removes the task from the route manager's map). The change takes the lock only when the task is exhausted and is still the restart task of the route (`routes.get(route) == task` in the route manager), and checks both again under the lock. `release()` removes the task from that map before it cancels it, so a cancelled task completes without the lock, and `removeRoute` no longer takes the lock of the controller under the route lock. The check under the lock keeps what main did when a manual stop or start releases the route while its last restart attempt fails: the route is not marked exhausted (main re-read the status under the lock for that). The ownership check also covers a cancelled task whose status the timer thread overwrites with `Exhausted` afterwards (the field is not volatile). `ManagedRoutePolicy.onRemove` also calls `lock.lock()`, but that `lock` is the policy's own lock, inherited from `BaseService` (it hides the field of the controller; `javap` shows `getfield` on the policy), so it is not part of the cycle and is left as is. Taking the lock of the controller there would deadlock with a manual operation on another route that runs at the same time (a mutant in the model below). Because of that, the `routes` `TreeSet` that `onRemove` changes is not guarded by the same lock as the other code that reads it; that is a separate thread-safety gap (also `nonSupervisedRoutes`, and `onInit` adds to both without a lock), not changed here; I can file it as a follow-up JIRA. ## How it was found With a TLA+ model of the two locks (`tla/1101c/SupervisingCtxLock.tla` in our verification workspace; each action is one locked step of the code): the threads are `removeRoute("a")`, the restart attempt (of route `a`, or of another route), and a manual stop of route `b`. `NoDeadlock` is violated in 8 states: ``` 1 rm: lock C removeRoute("a") (AbstractCamelContext:1638) 2 rm: shut down the services and endpoint of route a (RouteService:328-338) 3 rm: onRemove -> release(a) (RouteService:341, DefaultSupervisingRouteController:969) 4 rt: isRunAllowed() check (DefaultSupervisingRouteController:681) 5 rt: lock S (:691) 6 rt: re-check: task still active (:695) 7 rm: task.cancel() -> status Inactive (:773, BackOffTimerTask:138) 8 rm: whenComplete wants S (:724) | rt wants C (super.startRoute, AbstractCamelContext:1435) -> deadlock ``` With the change the model has no deadlock and every operation terminates (`Termination`, weak fairness per thread) with all three threads; witness configurations (with the change) show the restart attempt still starts the route, and sees the cancellation when that comes first. A route that is not being restarted has no task to cancel, and the model holds for it before the change too. ## Tests `DefaultSupervisingRouteControllerRemoveRouteDeadlockTest` (new) forces the interleaving: the endpoint of route `a` holds its shutdown (which `removeRoute` runs under the route lock) until the other thread waits for that lock (checked with `ThreadMXBean`, the lock owner is the removing thread), then lets it continue: - `testRemoveRestartingRouteWhileItsRestartAttemptRuns`: route `a` fails to start and is restarted every 10 ms; its restart attempt waits for the route lock while `a` is removed; - `testRemoveRestartingRouteWhileAnotherRouteIsBeingStopped`: route `a` waits for its next restart attempt; `routeController.stopRoute("b")` waits for the route lock while `a` is removed; - `testRemoveStoppedRouteWhileAnotherRouteIsBeingStopped`: control, the same with a route that is not being restarted (it passes on main as well); - `testStopRouteWhileItsLastRestartAttemptFails`: the last restart attempt of route `a` fails while the test holds the lock of the controller (package-private `getLock()`, for tests), and route `a` is stopped before the exhausted task gets the lock; the route must not be reported exhausted or unhealthy. It passes on main and fails with a version of this change that does not check again under the lock (found by our red-team review). The restart-attempt test also stops route `b` afterwards (bounded) to show that the attempt released the lock of the controller. The class has a `@Timeout`. The two removal tests of a restarting route fail on main: `removeRoute` does not return within 10 seconds, and `ThreadMXBean.findDeadlockedThreads()` reports the removing thread and the other thread each waiting for the `ReentrantLock` the other holds. The test then leaves the CamelContext running instead of hanging in its stop. With the change all four pass; the test class ran 20 times in one JVM: 80 tests, 0 failures. Without any hook, a probe removes a route that fails to start, is restarted every millisecond, and whose endpoint takes 20 ms to shut down (as closing a connection does): on main the removal deadlocked in 200 of 200 runs, with the change in 0 of 200. With an endpoint that shuts down at once, 300 removals did not deadlock on main: the window is the time the removal takes to shut the route down. The supervising route controller tests of camel-core, camel-main and camel-management and the route policy, route controller and route removal tests of camel-core pass: camel-core 122 tests, camel-main 4, camel-management 13, camel-support 3, camel-util 10. Two local-environment failures, which also happen without this change: `BrowseRemoveRouteTest` (3 errors from an older `camel-browse` snapshot jar in the local repository), and 9 camel-management tests that fail only because our local run forces all test classes into one JVM (they share the MBean server); each passes when run alone. The formatter and import-sort plugins ran. No upgrade guide entry: nothing changes for users apart from the deadlock. One log line is new in practice: a restart attempt that waited for the route lock while its route was removed now goes on, finds no route to start, and logs `Route: a started after N attempts` (before, it deadlocked). Affected: 4.23.0 and later. The release in `onRemove` came with CAMEL-25071; on 4.22.x, 4.18.x and 4.14.x `onRemove` does not cancel the task, so only `camel-4.23.x` needs the backport. camel-4.23.x builds with JDK 17: the backport of the test must use `Thread.getId()` instead of `threadId()` (3 places). _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]
