Fix built-in roles sync losing updates (elastic#142433) (elastic#142892)

Built-in role sync requests arriving while another sync was
in progress were silently discarded. This caused roles to go
missing when cluster change events fired in quick succession
during bootstrap.

Replace the synchronizationInProgress AtomicBoolean with a
lock-free AtomicInteger state machine (RolesSync) that tracks
idle, syncing, and syncing_pending states. When a sync
completes and updates are pending, it automatically retries
with the latest roles.

Author: ebarlas

docs/changelog/142433.yaml

@@ -0,0 +1,6 @@
+area: Security
+issues: []
+pr: 142433
+summary: Fix built-in roles sync to retry on lock contention instead of silently discarding
+  pending updates
+type: bug

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java

@@ -54,7 +54,6 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.Executor;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static java.util.stream.Collectors.toMap;
@@ -130,7 +129,7 @@ public void taskSucceeded(MarkRolesAsSyncedTask task, Map<String, String> value)
     private final QueryableBuiltInRoles.Provider rolesProvider;
     private final NativeRolesStore nativeRolesStore;
     private final Executor executor;
-    private final AtomicBoolean synchronizationInProgress = new AtomicBoolean(false);
+    private final RolesSync sync = new RolesSync();
 
     private volatile boolean securityIndexDeleted = false;
 
@@ -217,33 +216,91 @@ public void clusterChanged(ClusterChangedEvent event) {
      * @return {@code true} if the synchronization of built-in roles is in progress, {@code false} otherwise
      */
     public boolean isSynchronizationInProgress() {
-        return synchronizationInProgress.get();
+        return sync.inProgress();
     }
 
     private void syncBuiltInRoles(final QueryableBuiltInRoles roles) {
-        if (synchronizationInProgress.compareAndSet(false, true)) {
-            try {
-                final Map<String, String> indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state());
-                if (roles.rolesDigest().equals(indexedRolesDigests)) {
-                    logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization");
+        if (sync.startSync()) {
+            doSyncBuiltInRoles(roles);
+        }
+    }
+
+    private void doSyncBuiltInRoles(final QueryableBuiltInRoles roles) {
+        try {
+            final Map<String, String> indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state());
+            if (roles.rolesDigest().equals(indexedRolesDigests)) {
+                logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization");
+                resetFailedSyncAttempts();
+                endSyncAndRetryIfNeeded();
+            } else {
+                executor.execute(() -> applyRoleChanges(indexedRolesDigests, roles, ActionListener.wrap(v -> {
+                    logger.info("Successfully synced [{}] built-in roles to .security index", roles.roleDescriptors().size());
                     resetFailedSyncAttempts();
-                    synchronizationInProgress.set(false);
+                    endSyncAndRetryIfNeeded();
+                }, e -> {
+                    handleException(e);
+                    endSyncAndRetryIfNeeded();
+                })));
+            }
+        } catch (Exception e) {
+            logger.error("Failed to sync built-in roles", e);
+            failedSyncAttempts.incrementAndGet();
+            endSyncAndRetryIfNeeded();
+        }
+    }
+
+    private void endSyncAndRetryIfNeeded() {
+        if (sync.endSync()) {
+            boolean shouldRetry = false;
+            try {
+                shouldRetry = shouldSyncBuiltInRoles(clusterService.state());
+            } catch (Exception e) {
+                logger.warn("Failed to evaluate retry conditions for built-in roles synchronization", e);
+            }
+            if (shouldRetry) {
+                logger.debug("Retrying synchronization of built-in roles due to pending changes");
+                doSyncBuiltInRoles(rolesProvider.getRoles());
+            } else {
+                endSyncAndRetryIfNeeded();
+            }
+        }
+    }
+
+    static class RolesSync {
+        private static final int IDLE = 0;
+        private static final int RUNNING = 1;
+        private static final int RUNNING_PENDING = 2;
+
+        private final AtomicInteger state = new AtomicInteger(IDLE);
+
+        boolean startSync() {
+            while (true) {
+                int s = state.get();
+                if (s == IDLE) {
+                    if (state.compareAndSet(IDLE, RUNNING)) return true;
+                } else if (s == RUNNING) {
+                    if (state.compareAndSet(RUNNING, RUNNING_PENDING)) return false;
                 } else {
-                    executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> {
-                        logger.info("Successfully synced [{}] built-in roles to .security index", roles.roleDescriptors().size());
-                        resetFailedSyncAttempts();
-                        synchronizationInProgress.set(false);
-                    }, e -> {
-                        handleException(e);
-                        synchronizationInProgress.set(false);
-                    })));
+                    return false; // already RUNNING_PENDING
                 }
-            } catch (Exception e) {
-                logger.error("Failed to sync built-in roles", e);
-                failedSyncAttempts.incrementAndGet();
-                synchronizationInProgress.set(false);
             }
         }
+
+        boolean endSync() {
+            while (true) {
+                int s = state.get();
+                assert s != IDLE : "endSync should only be called when a sync is in progress";
+                if (s == RUNNING_PENDING) {
+                    if (state.compareAndSet(RUNNING_PENDING, RUNNING)) return true;
+                } else {
+                    if (state.compareAndSet(RUNNING, IDLE)) return false;
+                }
+            }
+        }
+
+        boolean inProgress() {
+            return state.get() != IDLE;
+        }
     }
 
     private void handleException(Exception e) {
@@ -371,7 +428,7 @@ private boolean shouldSyncBuiltInRoles(final ClusterState state) {
         return true;
     }
 
-    private void doSyncBuiltinRoles(
+    private void applyRoleChanges(
         final Map<String, String> indexedRolesDigests,
         final QueryableBuiltInRoles roles,
         final ActionListener<Void> listener

x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java

@@ -7,6 +7,8 @@
 package org.elasticsearch.test;
 
 import org.elasticsearch.ResourceAlreadyExistsException;
+import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequest;
+import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.elasticsearch.action.admin.cluster.node.info.NodeInfo;
 import org.elasticsearch.action.admin.cluster.node.info.NodesInfoResponse;
 import org.elasticsearch.action.admin.cluster.node.info.PluginsAndModules;
@@ -53,6 +55,7 @@
 import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
 import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_MAIN_ALIAS;
 import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.is;
 
 /**
  * Base class to run tests against a cluster with X-Pack installed and security enabled.
@@ -444,7 +447,12 @@ protected void createSecurityIndexWithWaitForActiveShards() {
         try {
             client.admin().indices().create(createIndexRequest).actionGet();
         } catch (ResourceAlreadyExistsException e) {
-            logger.info("Security index already exists, ignoring.", e);
+            logger.info("Security index already exists, waiting for it to become available.", e);
+            ClusterHealthRequest healthRequest = new ClusterHealthRequest(TEST_REQUEST_TIMEOUT, SECURITY_MAIN_ALIAS).waitForActiveShards(
+                ActiveShardCount.ALL
+            );
+            ClusterHealthResponse healthResponse = client.admin().cluster().health(healthRequest).actionGet();
+            assertThat(healthResponse.isTimedOut(), is(false));
         }
     }
 

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java

@@ -683,6 +683,76 @@ private void mockNativeRolesStoreWithFailure(
             .deleteRoles(eq(rolesToDelete), eq(WriteRequest.RefreshPolicy.IMMEDIATE), eq(false), any(ActionListener.class));
     }
 
+    public void testRolesSyncBasicAcquireRelease() {
+        var sync = new QueryableBuiltInRolesSynchronizer.RolesSync();
+        assertFalse(sync.inProgress());
+
+        assertTrue(sync.startSync());
+        assertTrue(sync.inProgress());
+
+        assertFalse(sync.endSync());
+        assertFalse(sync.inProgress());
+    }
+
+    public void testRolesSyncContentionSetsPending() {
+        var sync = new QueryableBuiltInRolesSynchronizer.RolesSync();
+        assertTrue(sync.startSync());
+
+        // Second caller is rejected but marks pending
+        assertFalse(sync.startSync());
+        assertTrue(sync.inProgress());
+
+        // endSync sees pending, clears it, keeps lock held
+        assertTrue(sync.endSync());
+        assertTrue(sync.inProgress());
+
+        // No more pending — release
+        assertFalse(sync.endSync());
+        assertFalse(sync.inProgress());
+    }
+
+    public void testRolesSyncMultiplePendingCoalesce() {
+        var sync = new QueryableBuiltInRolesSynchronizer.RolesSync();
+        assertTrue(sync.startSync());
+
+        // Multiple concurrent arrivals all set pending, but only one retry results
+        assertFalse(sync.startSync());
+        assertFalse(sync.startSync());
+        assertFalse(sync.startSync());
+
+        assertTrue(sync.endSync());   // one retry
+        assertFalse(sync.endSync());  // done
+        assertFalse(sync.inProgress());
+    }
+
+    public void testRolesSyncPendingDuringRetry() {
+        var sync = new QueryableBuiltInRolesSynchronizer.RolesSync();
+        assertTrue(sync.startSync());
+        assertFalse(sync.startSync()); // pending
+
+        assertTrue(sync.endSync());    // retry (lock still held)
+
+        // New arrival during retry
+        assertFalse(sync.startSync());
+
+        assertTrue(sync.endSync());    // second retry
+        assertFalse(sync.endSync());   // done
+        assertFalse(sync.inProgress());
+    }
+
+    public void testRolesSyncReacquireAfterFullRelease() {
+        var sync = new QueryableBuiltInRolesSynchronizer.RolesSync();
+        assertTrue(sync.startSync());
+        assertFalse(sync.endSync());
+        assertFalse(sync.inProgress());
+
+        // Can acquire again after full release
+        assertTrue(sync.startSync());
+        assertTrue(sync.inProgress());
+        assertFalse(sync.endSync());
+        assertFalse(sync.inProgress());
+    }
+
     private static ClusterState.Builder createClusterStateWithOpenSecurityIndex() {
         return SecurityIndexManagerTests.createClusterState(
             TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7,