[8.19] Adjust unpromotable shard refresh request validation to allow RefreshResult.NO_REFRESH (#129336)

* When a primary shard uses the read-only engine, it always returns a RefreshResult.NO_REFRESH for refreshes. Since #93600 we added an extra roundtrip to hook unpromotable shard refresh logic. This hook is always executed, even if there are no unpromotable shards, but the UnpromotableShardRefreshRequest would fail if the primary shard returns a RefreshResult.NO_REFRESH result.

Fix to be backported to several versions as it's annoying.

Closes #129036
Backport of #129176 for 8.19.0

* fix import

build-tools-internal/src/main/resources/changelog-schema.json

@@ -86,6 +86,7 @@
             "Rollup",
             "SQL",
             "Search",
+            "Searchable Snapshots",
             "Security",
             "Snapshot/Restore",
             "Stats",

docs/changelog/129176.yaml

@@ -0,0 +1,6 @@
+pr: 129176
+summary: Adjust unpromotable shard refresh request validation to allow `RefreshResult.NO_REFRESH`
+area: Searchable Snapshots
+type: bug
+issues:
+ - 129036

server/src/main/java/org/elasticsearch/action/admin/indices/refresh/TransportShardRefreshAction.java

@@ -118,17 +118,13 @@ public class TransportShardRefreshAction extends TransportReplicationAction<
             IndexShardRoutingTable indexShardRoutingTable,
             ActionListener<Void> listener
         ) {
-            assert replicaRequest.primaryRefreshResult.refreshed() : "primary has not refreshed";
-            UnpromotableShardRefreshRequest unpromotableReplicaRequest = new UnpromotableShardRefreshRequest(
-                indexShardRoutingTable,
-                replicaRequest.primaryRefreshResult.primaryTerm(),
-                replicaRequest.primaryRefreshResult.generation(),
-                false
-            );
+            var primaryTerm = replicaRequest.primaryRefreshResult.primaryTerm();
+            var generation = replicaRequest.primaryRefreshResult.generation();
+
             transportService.sendRequest(
                 transportService.getLocalNode(),
                 TransportUnpromotableShardRefreshAction.NAME,
-                unpromotableReplicaRequest,
+                new UnpromotableShardRefreshRequest(indexShardRoutingTable, primaryTerm, generation, false),
                 new ActionListenerResponseHandler<>(listener.safeMap(r -> null), in -> ActionResponse.Empty.INSTANCE, refreshExecutor)
             );
         }

server/src/main/java/org/elasticsearch/action/admin/indices/refresh/TransportUnpromotableShardRefreshAction.java

@@ -16,6 +16,7 @@ import org.elasticsearch.action.support.broadcast.unpromotable.TransportBroadcas
 import org.elasticsearch.cluster.action.shard.ShardStateAction;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.index.engine.Engine;
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.injection.guice.Inject;
 import org.elasticsearch.tasks.Task;
@@ -73,12 +74,13 @@ public class TransportUnpromotableShardRefreshAction extends TransportBroadcastU
             return;
         }
 
+        var primaryTerm = request.getPrimaryTerm();
+        assert Engine.UNKNOWN_PRIMARY_TERM < primaryTerm : primaryTerm;
+        var segmentGeneration = request.getSegmentGeneration();
+        assert Engine.RefreshResult.UNKNOWN_GENERATION < segmentGeneration : segmentGeneration;
+
         ActionListener.run(responseListener, listener -> {
-            shard.waitForPrimaryTermAndGeneration(
-                request.getPrimaryTerm(),
-                request.getSegmentGeneration(),
-                listener.map(l -> ActionResponse.Empty.INSTANCE)
-            );
+            shard.waitForPrimaryTermAndGeneration(primaryTerm, segmentGeneration, listener.map(l -> ActionResponse.Empty.INSTANCE));
         });
     }
 

server/src/main/java/org/elasticsearch/action/admin/indices/refresh/UnpromotableShardRefreshRequest.java

@@ -47,7 +47,9 @@ public class UnpromotableShardRefreshRequest extends BroadcastUnpromotableReques
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException validationException = super.validate();
-        if (segmentGeneration == Engine.RefreshResult.UNKNOWN_GENERATION) {
+        if (segmentGeneration == Engine.RefreshResult.UNKNOWN_GENERATION && primaryTerm == Engine.UNKNOWN_PRIMARY_TERM) {
+            // read-only primary shards (like searchable snapshot shard) return Engine.RefreshResult.NO_REFRESH during refresh
+        } else if (segmentGeneration == Engine.RefreshResult.UNKNOWN_GENERATION) {
             validationException = addValidationError("segment generation is unknown", validationException);
         }
         return validationException;

server/src/main/java/org/elasticsearch/index/engine/ReadOnlyEngine.java

@@ -445,7 +445,7 @@ public class ReadOnlyEngine extends Engine {
 
     @Override
     public void maybeRefresh(String source, ActionListener<RefreshResult> listener) throws EngineException {
-        listener.onResponse(RefreshResult.NO_REFRESH);
+        ActionListener.completeWith(listener, () -> refresh(source));
     }
 
     @Override

x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/FrozenSearchableSnapshotsIntegTests.java

@@ -71,6 +71,7 @@ import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
 import static org.elasticsearch.search.aggregations.AggregationBuilders.dateHistogram;
 import static org.elasticsearch.snapshots.SearchableSnapshotsSettings.SEARCHABLE_SNAPSHOT_STORE_TYPE;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailuresAndResponse;
 import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshots.SNAPSHOT_RECOVERY_STATE_FACTORY_KEY;
 import static org.hamcrest.Matchers.arrayWithSize;
@@ -542,6 +543,37 @@ public class FrozenSearchableSnapshotsIntegTests extends BaseFrozenSearchableSna
         }
     }
 
+    public void testRefreshPartiallyMountedIndex() throws Exception {
+        internalCluster().ensureAtLeastNumDataNodes(2);
+
+        final var index = "index";
+        createIndex(index, 1, 0);
+        populateIndex(index, 1_000);
+
+        final var repository = "repository";
+        createRepository(repository, FsRepository.TYPE, Settings.builder().put("location", randomRepoPath()));
+
+        final var snapshot = "repository";
+        createFullSnapshot(repository, snapshot);
+
+        assertAcked(indicesAdmin().prepareDelete(index));
+
+        final var partialIndex = "partial-" + index;
+        mountSnapshot(
+            repository,
+            snapshot,
+            index,
+            partialIndex,
+            Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, randomInt(1)).build(),
+            MountSearchableSnapshotRequest.Storage.SHARED_CACHE
+        );
+        ensureGreen(partialIndex);
+
+        // before the fix this would have failed
+        var refreshResult = indicesAdmin().prepareRefresh(partialIndex).execute().actionGet();
+        assertNoFailures(refreshResult);
+    }
+
     public void testTierPreferenceCannotBeRemovedForFrozenIndex() throws Exception {
         final String fsRepoName = randomAlphaOfLength(10);
         final String indexName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT);

x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java

@@ -51,6 +51,7 @@ import org.elasticsearch.index.mapper.DateFieldMapper;
 import org.elasticsearch.index.shard.IndexLongFieldRange;
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.repositories.RepositoryData;
+import org.elasticsearch.repositories.fs.FsRepository;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.search.SearchResponseUtils;
 import org.elasticsearch.snapshots.SnapshotId;
@@ -89,6 +90,7 @@ import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READO
 import static org.elasticsearch.snapshots.SearchableSnapshotsSettings.SEARCHABLE_SNAPSHOT_STORE_TYPE;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
 import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshots.SNAPSHOT_RECOVERY_STATE_FACTORY_KEY;
 import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.containsString;
@@ -1274,6 +1276,37 @@ public class SearchableSnapshotsIntegTests extends BaseSearchableSnapshotsIntegT
         }
     }
 
+    public void testRefreshFullyMountedIndex() throws Exception {
+        internalCluster().ensureAtLeastNumDataNodes(2);
+
+        final var index = "index";
+        createIndex(index, 1, 0);
+        populateIndex(index, 1_000);
+
+        final var repository = "repository";
+        createRepository(repository, FsRepository.TYPE, Settings.builder().put("location", randomRepoPath()));
+
+        final var snapshot = "repository";
+        createFullSnapshot(repository, snapshot);
+
+        assertAcked(indicesAdmin().prepareDelete(index));
+
+        final var fullIndex = "full-" + index;
+        mountSnapshot(
+            repository,
+            snapshot,
+            index,
+            fullIndex,
+            Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, randomInt(1)).build(),
+            MountSearchableSnapshotRequest.Storage.FULL_COPY
+        );
+        ensureGreen(fullIndex);
+
+        // before the fix this would have failed
+        var refreshResult = indicesAdmin().prepareRefresh(fullIndex).execute().actionGet();
+        assertNoFailures(refreshResult);
+    }
+
     private TaskInfo getTaskForActionFromMaster(String action) {
         ListTasksResponse response = client().execute(
             TransportListTasksAction.TYPE,