allthingssecurity opened a new pull request, #26932: URL: https://github.com/apache/camel/pull/26932
# Description [CAMEL-25052](https://issues.apache.org/jira/browse/CAMEL-25052) Since CAMEL-22784 (#20686, backported to 4.14.x in #20780), `FileLockClusterView` runs its leadership check as a chain of one-shot tasks: each `tryLock` schedules the next one. The `ScheduledFuture` is no longer stored in `task`, so `doStop()` cancels nothing, and `tryLock` checks the view state only when it begins. Before that change, `task.cancel(true)` interrupted a running check. Two things follow: - **A view stopped during a check can take the lock.** A follower that sees the old leader go away opens the lock and data files and calls `FileChannel.tryLock`. If its view is stopped meanwhile (a master route stops, the last clustered route is removed, the CamelContext stops in a JVM that keeps running), the check still takes the OS lock, sets the member to LEADER and fires the leadership event, on a stopped view. The lock stays held until the view is started again or the JVM exits, so no other node can become the leader and the clustered routes run nowhere. The window is the acquisition I/O, which is longer on the network storage this service targets. - **A quick stop and start adds a chain.** The chain of the previous start is still scheduled, sees the view started again and keeps running next to the new one. Three quick restarts give four checks per interval. Affected: 4.14.5 and later, and 4.17.0 and later. This change: - Every start and stop increments a generation number. A check belongs to the generation of the start that scheduled it. Once that generation is over it ends without rescheduling itself. This ends the chain on stop, keeps one chain per start, and replaces the dead `task` field. - The acquisition first checks the generation, so a check whose view was stopped while it read the cluster data does not open the lock file. It opens the files and takes the OS lock into local variables, then re-checks the generation and only then publishes the lock and files to the view and sets LEADER. If the view was stopped before the lock was published, it releases the lock and closes the files instead, and fires no leadership event. - `doStop()` increments the generation and takes over the lock and files, then truncates, releases and closes them. The release and close are in a `finally` block, so they also happen when the truncate task fails or times out (`clusterDataTaskTimeout` on each of `clusterDataTaskMaxAttempts` attempts, the hanging NFS case). Before, the fields kept the lock in that case and the next start released it; now that `doStop()` clears the fields, it must release the lock itself. - The leadership-lost path of a check (lock no longer valid) and the check's update of the member to FOLLOWER only act if the check's generation is still current, under the same lock. So they cannot overwrite STOPPED, and they cannot release or close the lock and files that `doStop()` has taken over. The lost path takes the lock and files over from the view in the same way as `doStop()` and releases them outside the lock. The three fields are `volatile`, as the check and the cluster data tasks read them without the lock. - The generation and the hand-over are guarded by a small `ReentrantLock` that is only held for these field updates. It is never held during file I/O or while listeners are notified, so a stop does not wait for a slow check (with the `clusterDataTaskTimeout` retries this can take a long time on NFS), and no new lock ordering with the cluster view or `ClusteredRoutePolicy` locks is introduced. - `doStop()` sets the member to STOPPED as it takes over the lock, so a leadership event that is dispatched concurrently reads `isLeader() == false`. The listeners in camel-master and `ClusteredRoutePolicy` read the local member's state rather than the event argument. - Side effect: a check that runs after its view and its `FileLockClusterService` have been stopped no longer reschedules itself, so it no longer re-creates the service's scheduler through `getExecutor()`. A check that passed its final generation test just before the stop can still call `getExecutor()` after the service has stopped and create a new scheduler; this window is a few instructions long. It is deliberately not closed by scheduling under the state lock: `getExecutor()` takes the service's lock, which `FileLockClusterService.stop()` holds while it stops the view, so that would deadlock. Not changed: - `doStop()` still does not fire a leadership event (the behaviour CAMEL-24545 settled on for ZooKeeper). - A check that published the lock just before the stop still fires its leadership event and writes its first heartbeat after the stop. The member is STOPPED by then, so the in-tree listeners, which read `isLeader()`, see no leader, but a listener that trusts the event argument sees one. Likewise a leader's heartbeat write that is running when the view stops can land after `doStop()` has truncated the data file, which delays the takeover by another node by up to `heartbeatTimeoutMultiplier` intervals. This heartbeat race exists on main too. No upgrade guide entry: the visible change is that a stopped view no longer takes or keeps the lock. Tests: new `FileLockClusterViewStopTest` in camel-core, next to `FileLockClusteredRoutePolicyTest`. It uses `FileLockClusterService` subclasses in the same package, with no sleeps. - `testViewStoppedDuringLeadershipCheckDoesNotKeepTheLock`: A is the leader and B follows. A's context stops. B's next check is held by a latch in `createRandomAccessFile` just before it opens the lock file. B's view is released, then the latch is released. After B's check has finished (a no-op task on B's single-thread scheduler), B's scheduler queue must be empty (the chain ended), B must not report leadership, the lock file must be free, and a third node C must become the leader. - `testQuickRestartDoesNotAddLeadershipChecks`: the checks run on a scheduler owned by the test. After three quick release/retain cycles of the view, the four queued checks (the first start and three restarts) must drop back to one pending check and stay there. - `testStopReleasesTheLockWhenTheDataFileCannotBeTruncated`: the view is the leader. The leadership checks are paused, and the cluster data tasks are sent to an executor whose only thread is blocked, with `clusterDataTaskMaxAttempts=1` and a 200 ms `clusterDataTaskTimeout`. Releasing the view must fail (the truncate timed out), and the lock file must be free afterwards. Without the main-code change all three tests fail: ``` testViewStoppedDuringLeadershipCheckDoesNotKeepTheLock the stopped view still schedules leadership checks ==> expected: <true> but was: <false> testQuickRestartDoesNotAddLeadershipChecks ConditionTimeoutException: the scheduler queue did not drop back to 1 within 10 seconds testStopReleasesTheLockWhenTheDataFileCannotBeTruncated the stopped view reports leadership ==> expected: <false> but was: <true> ``` With the change, the file cluster tests pass: the whole camel-file suite (22 tests), `FileLock*` and `*Cluster*` in camel-core (38 tests, including `FileLockClusteredRoutePolicyTest` and the `ClusteredRoutePolicy*` tests), and the whole camel-master suite (30 tests, including `FileLockClusterServiceBasicFailoverTest` and `FileLockClusterServiceAdvancedFailoverTest`). 0 failures. Found with a TLA+ model of two and three nodes (the check, view stop and start, the OS lock, and the camel-master listener). For the current code `NoStoppedHolder`, `StoppedNotLeader`, `EventuallyLeader` and `OneChain` are violated. The fixed model holds all of them. I then reproduced both effects against the real classes, including a probe from a separate JVM that found the lock still held. # 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]
