Trigger search replica sync when scaling down to search-only

[HUSTERGS]

Description

This change triggers a final Remote Store segment sync on search-only replicas when an index transitions into search-only mode.

During scale down, primary shards already perform a final sync/flush and wait for Remote Store sync before the final metadata transition. Search-only replicas, however, are not part of checkpoint publishing and normally catch up through IndexService.AsyncReplicationTask.

Once index.blocks.search_only=true is applied, the regular replication task stops because AsyncReplicationTask#shouldRun() returns false. If a search-only replica has not already pulled the latest Remote Store segments before that metadata update, it can continue serving stale results after scale down completes.

This PR fixes that gap by:

• Detecting the metadata transition from non-search-only to search-only in IndexService#updateMetadata.
• Passing that transition into updateReplicationTask.
• Recreating AsyncReplicationTask as before.
• Triggering a one-time forced segment sync through AsyncReplicationTask#forceSyncSegments.
• Reusing maybeSyncSegments(true) so the existing search-only shard filtering and Remote Store sync path are preserved.
• Checking mustReschedule() before enqueueing and again at execution time to respect index lifecycle conditions.

The forced sync is asynchronous and does not add search replica catch-up to the scale-down critical path.

Related Issues

Resolves #21369

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit e0cac9f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Trigger forced segment sync on scale-down to search-only in IndexService Relevant files: server/src/main/java/org/opensearch/index/IndexService.java Sub-PR theme: Integration test for search replica catch-up on scale-down Relevant files: server/src/internalClusterTest/java/org/opensearch/action/admin/indices/scale/searchonly/ScaleIndexIT.java

⚡ Recommended focus areas for review Race Condition In updateReplicationTask, the new AsyncReplicationTask is created inside the finally block, and forceSyncSegments() is called immediately after. However, forceSyncSegments checks mustReschedule() which depends on the index lifecycle state. Since updateMetadata is synchronized, but forceSyncSegments dispatches work to a thread pool executor asynchronously, there is a potential race where the index could be closed or the task replaced before the async work runs. The second mustReschedule() check inside doRun() mitigates this, but the interaction between the synchronized method and the async dispatch should be carefully validated. private void updateReplicationTask(boolean forceSync) { try { asyncReplicationTask.close(); } finally { asyncReplicationTask = new AsyncReplicationTask(this); if (forceSync) { asyncReplicationTask.forceSyncSegments(); } } } Missed Transition The becameSearchOnly flag is computed before indexSettings.updateIndexMetadata(newIndexMetadata) is called. If the settings update itself is what makes isSearchOnly reflect the new state, the ordering is correct. However, if INDEX_BLOCKS_SEARCH_ONLY_SETTING is read from newIndexMetadata.getSettings() directly (not from indexSettings), this is fine — but it should be verified that updateReplicationTask(becameSearchOnly) is only called inside the if (updateIndexSettings) block, meaning it only fires when settings actually changed. Currently it is inside that block, which is correct, but the becameSearchOnly computation happens outside — if currentIndexMetadata is null (first call), wasSearchOnly is false and isSearchOnly could be true, triggering a forced sync on initial metadata application, which may be unintended. final boolean wasSearchOnly = currentIndexMetadata != null && IndexMetadata.INDEX_BLOCKS_SEARCH_ONLY_SETTING.get(currentIndexMetadata.getSettings()); final boolean isSearchOnly = IndexMetadata.INDEX_BLOCKS_SEARCH_ONLY_SETTING.get(newIndexMetadata.getSettings()); final boolean becameSearchOnly = wasSearchOnly == false && isSearchOnly; final boolean updateIndexSettings = indexSettings.updateIndexMetadata(newIndexMetadata); if (Assertions.ENABLED && currentIndexMetadata != null) { final long currentSettingsVersion = currentIndexMetadata.getSettingsVersion(); final long newSettingsVersion = newIndexMetadata.getSettingsVersion(); if (currentSettingsVersion == newSettingsVersion) { assert updateIndexSettings == false; } else { assert updateIndexSettings; assert currentSettingsVersion < newSettingsVersion : "expected current settings version [" + currentSettingsVersion + "] " + "to be less than new settings version [" + newSettingsVersion + "]"; } } if (updateIndexSettings) { for (final IndexShard shard : this.shards.values()) { try { shard.onSettingsChanged(); } catch (Exception e) { logger.warn( () -> new ParameterizedMessage("[{}] failed to notify shard about setting change", shard.shardId().id()), e ); } } onRefreshIntervalChange(); updateFsyncTaskIfNecessary(); updateReplicationTask(becameSearchOnly); Flaky Test Risk The test disables refresh interval (-1) to prevent the search replica from catching up, then indexes a document and immediately asserts the primary has 2 hits but the search replica still has 1. This assertion at line 97-98 is not wrapped in assertBusy, so if the async replication task happens to sync before the assertion runs, the test could fail intermittently. Consider adding a short sleep or a more deterministic mechanism to ensure the search replica has not yet caught up before triggering scale-down. assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to e0cac9f

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Wrap async replication assertions in assertBusy These assertions are made outside an assertBusy block, but replication to the search replica is asynchronous. The search replica may not yet reflect the latest document count at this point, making the assertion for SEARCH_REPLICA returning 1 potentially flaky. Consider wrapping these in assertBusy to allow for eventual consistency. server/src/internalClusterTest/java/org/opensearch/action/admin/indices/scale/searchonly/ScaleIndexIT.java [97-98] -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +assertBusy(() -> { + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +}); Suggestion importance[1-10]: 7 __ Why: The test intentionally verifies that the search replica has NOT yet caught up (still showing 1 hit) before the scale-down operation, so the SEARCH_REPLICA assertion at count 1 is a deliberate check of the pre-scale state. However, the PRIMARY assertion at count 2 could still be flaky without assertBusy. The suggestion has merit for the primary check but misunderstands the intent of the search replica assertion.	Medium
	Avoid forced sync inside finally block The forceSyncSegments() call is placed inside the finally block, which means it executes even if asyncReplicationTask.close() throws an exception and the new task may be in an inconsistent state. The forced sync should only be triggered after the new task is successfully created, outside the finally block, or the logic should be restructured to handle this safely. server/src/main/java/org/opensearch/index/IndexService.java [1259-1268] private void updateReplicationTask(boolean forceSync) { try { asyncReplicationTask.close(); } finally { asyncReplicationTask = new AsyncReplicationTask(this); - if (forceSync) { - asyncReplicationTask.forceSyncSegments(); - } + } + if (forceSync) { + asyncReplicationTask.forceSyncSegments(); } } Suggestion importance[1-10]: 6 __ Why: The forceSyncSegments() call inside the finally block could execute even if the new task is in an inconsistent state. Moving it outside the finally block ensures it only runs after successful task creation, improving correctness.	Low
General	Log warning when forced sync is skipped The mustReschedule() check is performed twice: once before submitting the task and once inside doRun(). However, between these two checks, the task could be closed (setting mustReschedule() to false), yet the outer check already passed and the runnable is submitted. The inner check correctly guards against this, but if mustReschedule() returns false inside doRun(), the forced sync is silently skipped with no warning or fallback, which could leave the search replica out of sync after scale-down. server/src/main/java/org/opensearch/index/IndexService.java [1634-1651] void forceSyncSegments() { if (mustReschedule() == false) { return; } threadPool.executor(getThreadPool()).execute(new AbstractRunnable() { - ... + @Override + public void onFailure(Exception e) { + logger.warn(() -> new ParameterizedMessage("failed to run forced task {}", AsyncReplicationTask.this), e); + } + @Override protected void doRun() { if (mustReschedule()) { indexService.maybeSyncSegments(true); + } else { + logger.warn(() -> new ParameterizedMessage("skipping forced sync for task {} as it is no longer active", AsyncReplicationTask.this)); } } }); } Suggestion importance[1-10]: 2 __ Why: Adding a warning log when mustReschedule() returns false inside doRun() is a minor observability improvement. The existing behavior is functionally correct since the inner check properly guards against race conditions, and the silent skip is acceptable given the task was already closed.	Low

---

Previous suggestions

Suggestions up to commit b1d72c1

Category	Suggestion	Impact
Possible issue	Move forced sync outside finally block The forceSyncSegments() call is placed inside the finally block, meaning it executes even if asyncReplicationTask.close() throws an exception and the new task may be in an inconsistent state. The forced sync should only be triggered after the new task is successfully created, so it should be moved outside the try-finally construct or into a separate block after the finally. server/src/main/java/org/opensearch/index/IndexService.java [1259-1268] private void updateReplicationTask(boolean forceSync) { try { asyncReplicationTask.close(); } finally { asyncReplicationTask = new AsyncReplicationTask(this); - if (forceSync) { - asyncReplicationTask.forceSyncSegments(); - } + } + if (forceSync) { + asyncReplicationTask.forceSyncSegments(); } } Suggestion importance[1-10]: 6 __ Why: The suggestion is valid - placing forceSyncSegments() inside the finally block means it runs even if close() throws an exception. Moving it outside ensures the sync only happens after successful task creation. However, in practice close() is unlikely to throw, making this a minor robustness improvement.	Low
General	Wrap async assertions in assertBusy These assertions are made outside of assertBusy, but replication to the search replica may be asynchronous. The search replica may not yet reflect the count of 1 at this exact point, potentially causing flaky test failures. Wrap these assertions in assertBusy to allow for eventual consistency. server/src/internalClusterTest/java/org/opensearch/action/admin/indices/scale/searchonly/ScaleIndexIT.java [97-98] -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +assertBusy(() -> { + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +}); Suggestion importance[1-10]: 5 __ Why: The test intentionally disables refresh interval and verifies that the search replica has NOT yet caught up (count=1 vs primary count=2), so wrapping in assertBusy would actually undermine the test's intent. The primary assertion (count=2) is direct, but the search replica assertion (count=1) is checking a deliberate lag state, making this suggestion potentially incorrect for the test's purpose.	Low
	Guard against sync on closed task The mustReschedule() check is performed twice: once before submitting the task and once inside doRun(). Between these two checks, the task could be closed (setting mustReschedule() to false), but the outer check already passed. More critically, if the task is closed between submission and execution, maybeSyncSegments will still be called. Consider using a lifecycle flag or checking the closed state more robustly to avoid executing sync on a closed/replaced task. server/src/main/java/org/opensearch/index/IndexService.java [1634-1651] void forceSyncSegments() { if (mustReschedule() == false) { return; } threadPool.executor(getThreadPool()).execute(new AbstractRunnable() { @Override public void onFailure(Exception e) { logger.warn(() -> new ParameterizedMessage("failed to run forced task {}", AsyncReplicationTask.this), e); } @Override protected void doRun() { - if (mustReschedule()) { + // Re-check under the index service lock or use isClosed() to guard against task replacement + if (mustReschedule() && !isClosed()) { indexService.maybeSyncSegments(true); } } }); } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid race condition concern, but the improved_code introduces isClosed() which may not exist on this class, making the suggestion potentially incorrect. The double-check pattern with mustReschedule() already provides reasonable protection, and the existing runInternal() method uses the same pattern.	Low

Suggestions up to commit ba918d3

Category	Suggestion	Impact
Possible issue	Move forced sync outside finally block The forceSyncSegments() call is placed inside the finally block, which means it executes even if asyncReplicationTask.close() throws an exception and the new task may be in an inconsistent state. The forced sync should only happen after the new task is successfully created, so it should be moved outside the try-finally construct or placed after the finally block. server/src/main/java/org/opensearch/index/IndexService.java [1259-1268] private void updateReplicationTask(boolean forceSync) { try { asyncReplicationTask.close(); } finally { asyncReplicationTask = new AsyncReplicationTask(this); - if (forceSync) { - asyncReplicationTask.forceSyncSegments(); - } + } + if (forceSync) { + asyncReplicationTask.forceSyncSegments(); } } Suggestion importance[1-10]: 6 __ Why: The suggestion is valid - placing forceSyncSegments() inside the finally block means it runs even if close() throws an exception. Moving it outside ensures the sync only happens after successful task creation. However, the finally block guarantees asyncReplicationTask is always assigned, so the risk is relatively low in practice.	Low
General	Wrap async assertions in assertBusy These assertions are made outside of assertBusy, but replication to the search replica may be asynchronous and not yet reflected. The assertion that the search replica still has 1 hit (before scale-down) could be flaky if replication catches up before this point. Consider wrapping in assertBusy or adding a brief stabilization check to ensure the test reliably captures the pre-scale-down state. server/src/internalClusterTest/java/org/opensearch/action/admin/indices/scale/searchonly/ScaleIndexIT.java [97-98] -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +assertBusy(() -> { + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +}); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about test flakiness - the primary assertion at line 97 should be stable, but wrapping both in assertBusy could actually make the test less reliable by allowing the search replica to catch up before the scale-down, defeating the test's purpose of verifying the replica is behind. The test intentionally checks that the search replica has only 1 hit before scale-down.	Low
	Log warning when forced sync is skipped The mustReschedule() check inside doRun() may cause the forced sync to be silently skipped if the task is closed between the outer check and actual execution. Since this is an explicit forced sync triggered by a scale-down event, the sync should proceed regardless of the reschedule state, or at minimum log a warning when it is skipped. server/src/main/java/org/opensearch/index/IndexService.java [1634-1651] void forceSyncSegments() { if (mustReschedule() == false) { return; } threadPool.executor(getThreadPool()).execute(new AbstractRunnable() { @Override public void onFailure(Exception e) { logger.warn(() -> new ParameterizedMessage("failed to run forced task {}", AsyncReplicationTask.this), e); } @Override protected void doRun() { if (mustReschedule()) { indexService.maybeSyncSegments(true); + } else { + logger.warn(() -> new ParameterizedMessage("forced sync skipped for task {}, task is no longer active", AsyncReplicationTask.this)); } } }); } Suggestion importance[1-10]: 3 __ Why: Adding a log warning when the forced sync is skipped is a minor observability improvement. The race condition described is real but the impact is low since the outer mustReschedule() check already guards against most cases, making this primarily a logging/debugging enhancement.	Low

Suggestions up to commit 06b097f

Category	Suggestion	Impact
Possible issue	Move forced sync outside finally block The forceSyncSegments() call is placed inside the finally block, which means it executes even if asyncReplicationTask.close() throws an exception and the new task may be in an inconsistent state. The forced sync should only be triggered after the new task is successfully created, so it should be moved outside the try-finally construct or into a separate block after the finally. server/src/main/java/org/opensearch/index/IndexService.java [1259-1268] private void updateReplicationTask(boolean forceSync) { try { asyncReplicationTask.close(); } finally { asyncReplicationTask = new AsyncReplicationTask(this); - if (forceSync) { - asyncReplicationTask.forceSyncSegments(); - } + } + if (forceSync) { + asyncReplicationTask.forceSyncSegments(); } } Suggestion importance[1-10]: 6 __ Why: The forceSyncSegments() call inside the finally block could execute even if asyncReplicationTask.close() throws, potentially triggering a sync on a partially initialized task. Moving it outside the finally block is a valid correctness improvement, though in practice close() is unlikely to throw.	Low
General	Wrap flaky assertions in retry block These assertions are made outside of assertBusy, but the second document was indexed with IMMEDIATE refresh policy while the refresh interval was set to -1. The search replica may not have replicated the second document yet at this exact point, making the assertion on the search replica potentially flaky. Wrap these assertions in assertBusy to allow for replication lag. server/src/internalClusterTest/java/org/opensearch/action/admin/indices/scale/searchonly/ScaleIndexIT.java [97-98] -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); -assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +assertBusy(() -> { + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.PRIMARY.type()).setSize(0).get(), 2); + assertHitCount(client().prepareSearch(TEST_INDEX).setPreference(Preference.SEARCH_REPLICA.type()).setSize(0).get(), 1); +}); Suggestion importance[1-10]: 5 __ Why: The test intentionally verifies that the search replica has NOT yet received the second document (count=1) at this point, which is the core premise of the test scenario. Wrapping in assertBusy would undermine the test's intent, though the primary assertion (count=2) could theoretically be flaky.	Low
	Remove redundant pre-submission guard check The mustReschedule() check is performed twice: once before submitting the task and once inside doRun(). Between these two checks, the task could be closed (setting mustReschedule() to false), but the outer check already passed, so the task is still submitted. However, the inner check correctly guards against this race. The outer early-return check is misleading because it may skip the sync even when the index is in search-only mode and a sync is needed. Consider removing the outer guard or documenting the intent clearly, since the inner check is sufficient for correctness. server/src/main/java/org/opensearch/index/IndexService.java [1634-1651] void forceSyncSegments() { - if (mustReschedule() == false) { - return; - } threadPool.executor(getThreadPool()).execute(new AbstractRunnable() { @Override public void onFailure(Exception e) { logger.warn(() -> new ParameterizedMessage("failed to run forced task {}", AsyncReplicationTask.this), e); } @Override protected void doRun() { if (mustReschedule()) { indexService.maybeSyncSegments(true); } } }); } Suggestion importance[1-10]: 3 __ Why: The outer mustReschedule() check before submitting the task is a minor optimization/guard, and the inner check inside doRun() is sufficient for correctness. Removing the outer check is a minor style/clarity improvement but could also cause unnecessary thread pool submissions in edge cases.	Low

[github-actions]

❌ Gradle check result for 06b097f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit ba918d3

[github-actions]

❌ Gradle check result for ba918d3: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit b1d72c1

[github-actions]

❌ Gradle check result for b1d72c1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit e0cac9f

[github-actions]

✅ Gradle check result for e0cac9f: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.42%. Comparing base (0b0eae9) to head (e0cac9f).
⚠️ Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/main/java/org/opensearch/index/IndexService.java	68.75%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21370      +/-   ##
============================================
+ Coverage     73.40%   73.42%   +0.01%     
- Complexity    74262    74321      +59     
============================================
  Files          5961     5961              
  Lines        337610   337625      +15     
  Branches      48704    48709       +5     
============================================
+ Hits         247833   247910      +77     
+ Misses        69954    69940      -14     
+ Partials      19823    19775      -48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.