Adjust unpromotable shard refresh request validation to allow RefreshResult.NO_REFRESH (#129176)

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

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

@@ -23,7 +23,6 @@ import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.index.engine.Engine;
 import org.elasticsearch.index.shard.IndexShard;
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.injection.guice.Inject;
@@ -127,10 +126,7 @@ public class TransportShardRefreshAction extends TransportReplicationAction<
             ActionListener<Void> listener
         ) {
             var primaryTerm = replicaRequest.primaryRefreshResult.primaryTerm();
-            assert Engine.UNKNOWN_PRIMARY_TERM < primaryTerm : primaryTerm;
-
             var generation = replicaRequest.primaryRefreshResult.generation();
-            assert Engine.RefreshResult.UNKNOWN_GENERATION < generation : generation;
 
             transportService.sendRequest(
                 transportService.getLocalNode(),

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

@@ -22,6 +22,7 @@ import org.elasticsearch.cluster.metadata.MetadataCreateIndexService;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.index.engine.Engine;
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.injection.guice.Inject;
 import org.elasticsearch.node.NodeClosedException;
@@ -154,12 +155,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

@@ -63,7 +63,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

@@ -446,7 +446,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

@@ -67,6 +67,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.containsString;
@@ -521,6 +522,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;
@@ -1280,6 +1282,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,