[CI] Fix testClientsLifeCycleForSingleProject

More robust test for closed clients holder. Also changes
IllegalStateException to AlreadyClosedException for both closed manager
and holder.

Resolves: #128707

Author: ywangd

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientsManager.java

@@ -9,6 +9,7 @@
 
 package org.elasticsearch.repositories.s3;
 
+import org.apache.lucene.store.AlreadyClosedException;
 import org.elasticsearch.cluster.ClusterChangedEvent;
 import org.elasticsearch.cluster.ClusterStateApplier;
 import org.elasticsearch.cluster.metadata.ProjectId;
@@ -293,7 +294,7 @@ S3ClientSettings singleClientSettings(RepositoryMetadata repositoryMetadata) {
          * @param repositoryMetadata The metadata of the repository for which the Amazon S3 client is required.
          * @return An {@link AmazonS3Reference} instance corresponding to the repository metadata.
          * @throws IllegalArgumentException If no client settings exist for the given repository metadata.
-         * @throws IllegalStateException If the client manager is closed and a new client cannot be created.
+         * @throws AlreadyClosedException If either the clients manager or the holder is closed
          */
         final AmazonS3Reference client(RepositoryMetadata repositoryMetadata) {
             final var clientKey = clientKey(repositoryMetadata);
@@ -313,12 +314,12 @@ final AmazonS3Reference client(RepositoryMetadata repositoryMetadata) {
                 }
                 if (closed.get()) {
                     // Not adding a new client once the clients holder is closed since there won't be anything to close it
-                    throw new IllegalStateException("Project [" + projectId() + "] clients holder is closed");
+                    throw new AlreadyClosedException("Project [" + projectId() + "] clients holder is closed");
                 }
                 if (managerClosed.get()) {
                     // This clients holder must be added after the manager is closed. It must have no cached clients.
                     assert clientsCache.isEmpty() : "expect empty cache, but got " + clientsCache;
-                    throw new IllegalStateException("s3 clients manager is closed");
+                    throw new AlreadyClosedException("s3 clients manager is closed");
                 }
                 // The close() method maybe called after we checked it, it is ok since we are already inside the synchronized block.
                 // The close method calls clearCache() which will clear the newly added client.
@@ -347,6 +348,11 @@ void doClearCache() {}
          */
         public final void close() {
             if (closed.compareAndSet(false, true)) {
+                try {
+                    Thread.sleep(1000);
+                } catch (InterruptedException e) {
+                    throw new RuntimeException(e);
+                }
                 clearCache();
             }
         }

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientsManagerTests.java

@@ -14,6 +14,7 @@
 import software.amazon.awssdk.identity.spi.AwsCredentialsIdentity;
 import software.amazon.awssdk.regions.Region;
 
+import org.apache.lucene.store.AlreadyClosedException;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.metadata.Metadata;
 import org.elasticsearch.cluster.metadata.ProjectId;
@@ -203,12 +204,15 @@ public void testClientsLifeCycleForSingleProject() throws Exception {
         }
         assertClientNotFound(projectId, clientName);
 
-        assertBusy(() -> assertTrue(clientsHolder.isClosed()));
-        final var e = expectThrows(
-            IllegalStateException.class,
-            () -> clientsHolder.client(createRepositoryMetadata(randomFrom(clientName, anotherClientName)))
-        );
-        assertThat(e.getMessage(), containsString("Project [" + projectId + "] clients holder is closed"));
+        assertBusy(() -> {
+            assertTrue(clientsHolder.isClosed());
+            final var e = expectThrows(AlreadyClosedException.class, () -> {
+                var client = clientsHolder.client(createRepositoryMetadata(randomFrom(clientName, anotherClientName)));
+                client.decRef();
+            });
+            assertThat(e.getMessage(), containsString("Project [" + projectId + "] clients holder is closed"));
+        });
+
     }
 
     public void testClientsForMultipleProjects() throws InterruptedException {
@@ -298,16 +302,16 @@ public void testClientsHolderAfterManagerClosed() {
         final ProjectId projectId = randomUniqueProjectId();
         final String clientName = randomFrom(clientNames);
 
-        s3Service.close();
+        s3Service.close()
         assertTrue(s3ClientsManager.isManagerClosed());
         // New holder can be added after the manager is closed, but no actual client can be created
         updateProjectInClusterState(projectId, newProjectClientsSecrets(projectId, clientName));
         try (var clientsHolder = s3ClientsManager.getPerProjectClientsHolders().get(projectId)) {
             assertNotNull(clientsHolder);
             assertFalse(clientsHolder.isClosed());
 
-            final IllegalStateException e = expectThrows(
-                IllegalStateException.class,
+            final var e = expectThrows(
+                AlreadyClosedException.class,
                 () -> s3ClientsManager.client(projectId, createRepositoryMetadata(clientName))
             );
             assertThat(e.getMessage(), containsString("s3 clients manager is closed"));

muted-tests.yml

@@ -504,9 +504,6 @@ tests:
 - class: org.elasticsearch.xpack.esql.expression.function.scalar.string.RLikeTests
   method: testEvaluateInManyThreads {TestCase=100 random code points matches self case insensitive with keyword}
   issue: https://github.com/elastic/elasticsearch/issues/128706
-- class: org.elasticsearch.repositories.s3.S3ClientsManagerTests
-  method: testClientsLifeCycleForSingleProject
-  issue: https://github.com/elastic/elasticsearch/issues/128707
 - class: org.elasticsearch.xpack.esql.expression.function.scalar.string.RLikeTests
   method: testCrankyEvaluateBlockWithNulls {TestCase=100 random code points matches self case insensitive with text}
   issue: https://github.com/elastic/elasticsearch/issues/128710