allthingssecurity opened a new pull request, #26867: URL: https://github.com/apache/camel/pull/26867
# 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. - [CAMEL-25008](https://issues.apache.org/jira/browse/CAMEL-25008): the controller deadlocks when a route is stopped or started while its restart task completes. - [CAMEL-25007](https://issues.apache.org/jira/browse/CAMEL-25007): the completion of an old restart task removes the route's new restart task, which then restarts the route unsupervised, even after `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: - restart thread: `BackOffTimerTask.run()` -> `complete()` takes the task lock and, while holding it, calls the consumer registered by `RouteManager.start`, which takes the controller lock; - user thread: `stopRoute`/`startRoute`/`suspendRoute`/`resumeRoute` take the controller lock and call `routeManager.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, and `other`): ``` "operator": at org.apache.camel.util.backoff.BackOffTimerTask.complete(BackOffTimerTask.java:220) at org.apache.camel.util.backoff.BackOffTimerTask.cancel(BackOffTimerTask.java:144) at org.apache.camel.impl.engine.DefaultSupervisingRouteController$RouteManager.release(DefaultSupervisingRouteController.java:756) at org.apache.camel.impl.engine.DefaultSupervisingRouteController.doStopRoute(DefaultSupervisingRouteController.java:451) "Camel (camel-1) thread #1 - SupervisingRouteController": at org.apache.camel.impl.engine.DefaultSupervisingRouteController$RouteManager.lambda$start$2(DefaultSupervisingRouteController.java:711) at org.apache.camel.util.backoff.BackOffTimerTask.complete(BackOffTimerTask.java:222) at org.apache.camel.util.backoff.BackOffTimerTask.run(BackOffTimerTask.java:183) ``` 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, and `routes` is a `ConcurrentHashMap`. Its final `routes.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 failed `startRoute` registered 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: ``` AssertionFailedError: Cancelling the task should not deadlock ==> Unexpected exception thrown: java.util.concurrent.TimeoutException ``` With the fix it passes. `*BackOff*,*SupervisingRouteController*,*RouteController*` in camel-util and camel-core, plus `ManagedSupervisingRouteControllerTest` in 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 by `cancel()`, and again by `run()` when the attempt returns. The consumer that `DefaultSupervisingRouteController.RouteManager.start` registers ended with an unconditional `routes.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 `startRoute` fails, since that registers a new task. The new task kept running, but: - `getRestartingRoutes()` and `getRestartingRouteState(id)` no longer showed it; - `hasUnhealthyRoutes()` reported healthy while the route was down and being restarted; - `stopRoute(id)` could not cancel it (`release` finds 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 is `Inactive` or `Completed`, so the exhausted bookkeeping is skipped too. Tests: new `DefaultSupervisingRouteControllerStartWhileRestartingTest`. A route fails to start, and its first restart attempt is held in the `RouteRestartingEvent` notifier. 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 (a `whenComplete` registered 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: ``` AssertionFailedError: expected: <BackOffTimerTask[name=SupervisingRouteController, status=Active, currentAttempts=1, ...]> but was: <null> ``` 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 `stopRoute` cancels the new task, so the route stays stopped. Neither commit touches `RouteManager.release()`, so this does not conflict with #26859 (CAMEL-25001). # 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). # 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 and tested the affected modules, including the formatter and import-sort plugins. 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]
