From c0417b33c21836bc1a8aab647b9b1b6fa23123eb Mon Sep 17 00:00:00 2001
From: David Turner <david.turner@elastic.co>
Date: Mon, 22 Apr 2024 09:28:55 +0100
Subject: [PATCH] Forbid `null` ack timeout (#107653)

Today a cluster state update task with `null` ack timeout implies the
timeout is zero. This implicit behaviour is trappy, so this commit
forbids it.

Relates #107044
---
 .../cluster/metadata/MetadataUpdateSettingsServiceIT.java    | 5 +++--
 .../org/elasticsearch/cluster/service/MasterService.java     | 3 +++
 .../java/org/elasticsearch/upgrades/SystemIndexMigrator.java | 3 ++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/server/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java
index 59f4905d5924b..6e928a42e142b 100644
--- a/server/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java
+++ b/server/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java
@@ -15,6 +15,7 @@
 import org.elasticsearch.cluster.routing.ShardRoutingState;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.engine.Engine;
@@ -40,7 +41,7 @@ public void testThatNonDynamicSettingChangesTakeEffect() throws Exception {
         MetadataUpdateSettingsService metadataUpdateSettingsService = internalCluster().getCurrentMasterNodeInstance(
             MetadataUpdateSettingsService.class
         );
-        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest();
+        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest().ackTimeout(TimeValue.ZERO);
         List<Index> indices = new ArrayList<>();
         for (IndicesService indicesService : internalCluster().getInstances(IndicesService.class)) {
             for (IndexService indexService : indicesService) {
@@ -108,7 +109,7 @@ public void testThatNonDynamicSettingChangesDoNotUnncessesarilyCauseReopens() th
         MetadataUpdateSettingsService metadataUpdateSettingsService = internalCluster().getCurrentMasterNodeInstance(
             MetadataUpdateSettingsService.class
         );
-        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest();
+        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest().ackTimeout(TimeValue.ZERO);
         List<Index> indices = new ArrayList<>();
         for (IndicesService indicesService : internalCluster().getInstances(IndicesService.class)) {
             for (IndexService indexService : indicesService) {
diff --git a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java
index 48917ca84e89b..a9f891e555f21 100644
--- a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java
+++ b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java
@@ -46,6 +46,7 @@
 import org.elasticsearch.core.Releasables;
 import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.node.Node;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.tasks.TaskAwareRequest;
@@ -703,9 +704,11 @@ private static class TaskAckListener {
             this.countDown = new CountDown(countDown + 1); // we also wait for onCommit to be called
         }
 
+        @UpdateForV9 // properly forbid ackTimeout == null after enough time has passed to be sure it's not used in production
         public void onCommit(TimeValue commitTime) {
             TimeValue ackTimeout = contextPreservingAckListener.ackTimeout();
             if (ackTimeout == null) {
+                assert false : "ackTimeout must always be present: " + contextPreservingAckListener;
                 ackTimeout = TimeValue.ZERO;
             }
             final TimeValue timeLeft = TimeValue.timeValueNanos(Math.max(0, ackTimeout.nanos() - commitTime.nanos()));
diff --git a/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java b/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java
index 968e64fcc3888..3dcdf558e6dfc 100644
--- a/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java
+++ b/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java
@@ -35,6 +35,7 @@
 import org.elasticsearch.common.settings.IndexScopedSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.core.SuppressForbidden;
+import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.index.reindex.BulkByScrollResponse;
@@ -537,7 +538,7 @@ private void setWriteBlock(Index index, boolean readOnlyValue, ActionListener<Ac
         final Settings readOnlySettings = Settings.builder().put(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey(), readOnlyValue).build();
         UpdateSettingsClusterStateUpdateRequest updateSettingsRequest = new UpdateSettingsClusterStateUpdateRequest().indices(
             new Index[] { index }
-        ).settings(readOnlySettings).setPreserveExisting(false);
+        ).settings(readOnlySettings).setPreserveExisting(false).ackTimeout(TimeValue.ZERO);
 
         metadataUpdateSettingsService.updateSettings(updateSettingsRequest, listener);
     }