Fix stranded snapshot markers on publish failure

[Sukriti0717]

Description

Adds retryOrFailOnClusterManagerFailOver helper that retries a failed cluster-state publish (marker removal) with bounded exponential backoff (1s/2s/4s, max 3 retries) when the node is still the elected cluster-manager. Without this, a publish failure on a stable cluster-manager strands the in-progress snapshot marker forever, blocking index deletion and close.

Also fixes a latent stale-read bug in stateWithoutSnapshotV2: the inner execute() was reading from the captured outer state parameter instead of the live currentState, which could clobber concurrent
cluster-state changes. The method is refactored into a task factory (createStateWithoutSnapshotV2Task) that makes this bug structurally impossible to reintroduce.

The retry behavior is gated behind the SNAPSHOT_RESILIENCE feature flag (default off). The stale-read fix is unconditional.

Depends on #22516 (PR-C). Will rebase once that merges.

Testing

• RetryOrFailOnClusterManagerFailOverTests: 10 unit tests covering helper decision logic, stale-state bug fix, and retried-task behavior.
• All tests pass with -Dtests.iters=20 for seed stability.

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 5577c6f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Duplicate settings registration SNAPSHOT_CLEANUP_RETRIES_SETTING and SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING update consumers are registered unconditionally in the constructor, but they are also added to ClusterSettings.BUILT_IN_CLUSTER_SETTINGS. If SnapshotsService is instantiated more than once against the same ClusterService (or if the constructor is invoked on non-cluster-manager nodes concurrently with another service), this registration path executes on every node and may cause issues in tests where multiple instances share the same ClusterSettings. Also, unlike MAX_CONCURRENT_SNAPSHOT_OPERATIONS_SETTING which is gated on DiscoveryNode.isClusterManagerNode(settings), these are registered on all nodes, which is inconsistent with the other cluster-manager-only settings above. maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); Fallback runs before assertion In retryOrFailOnClusterManagerFailOver, when an unexpected exception (not NotClusterManagerException and not FailedToCommitClusterStateException) is received, failoverFallback.run() is invoked and then assert false is triggered. In assertions-enabled test runs, this causes an AssertionError to be thrown after the fallback has already executed, potentially leaving callers in an inconsistent state (fallback side-effects applied but caller sees an exception). Consider running the assert first or removing the fallback in this branch. if (ExceptionsHelper.unwrap(e, FailedToCommitClusterStateException.class) == null) { logger.error("Unexpected failure during cluster state update", e); failoverFallback.run(); assert false : new AssertionError("Unexpected failure during cluster state update", e); return; } Retries ignore cluster-manager loss retryOrFailOnClusterManagerFailOver schedules a retry solely based on the exception type and attempt count; it does not check whether the local node is still the elected cluster-manager at scheduling time. If the node lost cluster-manager status between the failure and the retry, the retried task will still be submitted and will only no-op inside execute(). The createRemoveFailedSnapshotTask path in particular will silently commit a no-op cluster state and invoke clusterStateProcessed with the original failure, which may not be the desired failover semantics. Consider checking clusterService.state().nodes().isLocalNodeElectedClusterManager() before scheduling and invoking the fallback if false. void retryOrFailOnClusterManagerFailOver( Exception e, int attempt, String source, Supplier<ClusterStateUpdateTask> taskFactory, Runnable failoverFallback ) { if (ExceptionsHelper.unwrap(e, NotClusterManagerException.class) != null) { failoverFallback.run(); return; } if (ExceptionsHelper.unwrap(e, FailedToCommitClusterStateException.class) == null) { logger.error("Unexpected failure during cluster state update", e); failoverFallback.run(); assert false : new AssertionError("Unexpected failure during cluster state update", e); return; } if (attempt >= maxRetries) { logger.warn("Exhausted {} retries for [{}], falling back to failover handling", maxRetries, source); failoverFallback.run(); return; } final int nextAttempt = attempt + 1; final TimeValue delay = computeBackoff(retryBackoff, attempt); logger.info("Publish failed for [{}] (attempt {}), scheduling retry in [{}]", source, nextAttempt, delay); threadPool.schedule(() -> clusterService.submitStateUpdateTask(source, taskFactory.get()), delay, ThreadPool.Names.GENERIC); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5577c6f

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard scheduled retry against rejection The scheduled retry runs on the GENERIC thread pool without any guard for shutdown or node role change. If the node is shutting down or the ThreadPool is terminated, threadPool.schedule can throw OpenSearchRejectedExecutionException, which will propagate out of onFailure and be swallowed silently, stranding the snapshot marker. Wrap the schedule call in a try/catch that runs failoverFallback on rejection. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3359-3367] if (attempt >= maxRetries) { logger.warn("Exhausted {} retries for [{}], falling back to failover handling", maxRetries, source); failoverFallback.run(); return; } final int nextAttempt = attempt + 1; final TimeValue delay = computeBackoff(retryBackoff, attempt); logger.info("Publish failed for [{}] (attempt {}), scheduling retry in [{}]", source, nextAttempt, delay); -threadPool.schedule(() -> clusterService.submitStateUpdateTask(source, taskFactory.get()), delay, ThreadPool.Names.GENERIC); +try { + threadPool.schedule(() -> clusterService.submitStateUpdateTask(source, taskFactory.get()), delay, ThreadPool.Names.GENERIC); +} catch (Exception scheduleFailure) { + logger.warn(() -> new ParameterizedMessage("failed to schedule retry for [{}], falling back", source), scheduleFailure); + failoverFallback.run(); +} Suggestion importance[1-10]: 7 __ Why: Valid concern: if threadPool.schedule throws OpenSearchRejectedExecutionException during shutdown, the snapshot marker could be stranded. Wrapping in try/catch to invoke the fallback improves robustness.	Medium
	Prevent overflow to negative backoff The multiplication base.millis() * (1L << 30) can still overflow to a negative value for large base values (e.g., configured backoff of a few seconds), and Math.min on a negative product returns the negative number, producing a negative delay which threadPool.schedule may reject. Compute the shift first and clamp before multiplication, or use Math.multiplyExact inside a try/catch. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3374-3377] static TimeValue computeBackoff(TimeValue base, int attempt) { - final long delayMillis = Math.min(base.millis() * (1L << Math.min(attempt, 30)), TimeValue.timeValueDays(1).millis()); - return TimeValue.timeValueMillis(delayMillis); + final long maxMillis = TimeValue.timeValueDays(1).millis(); + final int shift = Math.min(attempt, 30); + long delayMillis; + try { + delayMillis = Math.multiplyExact(base.millis(), 1L << shift); + } catch (ArithmeticException e) { + delayMillis = maxMillis; + } + return TimeValue.timeValueMillis(Math.min(delayMillis, maxMillis)); } Suggestion importance[1-10]: 7 __ Why: Correct edge case: for large base values, base.millis() * (1L << 30) can overflow producing a negative delay. Using multiplyExact with a fallback provides safer clamping.	Medium
General	Ensure fallback runs when flag disabled When the feature flag is disabled, onFailure becomes a silent no-op (only the earlier logger.warn fires), so the v2 snapshot marker is stranded on any publish failure — the same bug the PR aims to fix. The fallback path must run unconditionally so behavior at least matches pre-PR semantics when the flag is off. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [2531-2542] if (FeatureFlags.isEnabled(FeatureFlags.SNAPSHOT_RESILIENCE_SETTING)) { retryOrFailOnClusterManagerFailOver( e, attempt, source, () -> createStateWithoutSnapshotV2Task(source, attempt + 1), () -> { logger.error("Giving up on removing v2 snapshot state after {} attempts", attempt + 1); } ); +} else { + logger.error("Giving up on removing v2 snapshot state (resilience disabled)", e); } Suggestion importance[1-10]: 6 __ Why: Reasonable observation that when the feature flag is disabled, the onFailure path becomes a near no-op. However, the original code also lacked fallback behavior here, so this is a mild improvement rather than a regression fix.	Low
	Restrict setting consumers to cluster-manager These initializations run for every node (data, coordinating, etc.), but the earlier block that registers cluster-manager-only appliers is guarded by DiscoveryNode.isClusterManagerNode(settings). Registering dynamic setting consumers on non-cluster-manager nodes wastes resources and, more importantly, differs from the pattern used for MAX_CONCURRENT_SNAPSHOT_OPERATIONS_SETTING. Move these inside the cluster-manager guard to match existing conventions and avoid unnecessary state on non-cluster-manager nodes. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [352-355] -maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); -retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +if (DiscoveryNode.isClusterManagerNode(settings)) { + maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); + retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +} Suggestion importance[1-10]: 4 __ Why: Reasonable style/consistency suggestion to match the existing pattern for cluster-manager-only settings, but the practical impact is minimal since these are lightweight consumers.	Low

---

Previous suggestions

Suggestions up to commit a0f69b9

Category	Suggestion	Impact
Possible issue	Signal not-cluster-manager instead of silent no-op When the local node is no longer the elected cluster-manager, returning currentState silently drops the cleanup task without invoking onNoLongerClusterManager or any failure notification. This leaves callers (and completion listeners for removeFailedSnapshotFromClusterState) hanging. Prefer letting the framework detect the not-cluster-manager condition (which invokes onNoLongerClusterManager) rather than short-circuiting inside execute, or explicitly throw NotClusterManagerException so listeners are properly notified. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [2584-2586] @Override public ClusterState execute(ClusterState currentState) { if (currentState.nodes().isLocalNodeElectedClusterManager() == false) { - return currentState; + throw new NotClusterManagerException("no longer cluster-manager while retrying [" + source + "]"); } SnapshotsInProgress snapshots = currentState.custom(SnapshotsInProgress.TYPE, SnapshotsInProgress.EMPTY); Suggestion importance[1-10]: 7 __ Why: Valid concern: silently returning currentState when not elected cluster-manager bypasses onNoLongerClusterManager and can leave completion listeners hanging. Throwing NotClusterManagerException would trigger proper notification, though the exact framework behavior needs verification.	Medium
General	Prevent long overflow in backoff calc Even with the Math.min(attempt, 30) cap, base.millis() * (1L << 30) can overflow long if base is configured to a large value (e.g., minutes/hours), producing a negative delay before the outer Math.min clamps it. Compute the shift first, then check for overflow (or use Math.multiplyExact with a catch) before comparing to the 1-day cap. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3376-3379] static TimeValue computeBackoff(TimeValue base, int attempt) { - final long delayMillis = Math.min(base.millis() * (1L << Math.min(attempt, 30)), TimeValue.timeValueDays(1).millis()); - return TimeValue.timeValueMillis(delayMillis); + final long maxDelay = TimeValue.timeValueDays(1).millis(); + final int shift = Math.min(attempt, 30); + long delayMillis; + try { + delayMillis = Math.multiplyExact(base.millis(), 1L << shift); + } catch (ArithmeticException ae) { + delayMillis = maxDelay; + } + return TimeValue.timeValueMillis(Math.min(delayMillis, maxDelay)); } Suggestion importance[1-10]: 6 __ Why: Valid edge case: with large base values, base.millis() * (1L << 30) can overflow to a negative value, which the outer Math.min would then treat incorrectly. Using multiplyExact prevents this defensively.	Low
	Scope retry settings to cluster-manager nodes These initializations and update consumers are registered unconditionally, but retries are only meaningful on cluster-manager nodes. More importantly, the previous block already contains a if (DiscoveryNode.isClusterManagerNode(settings)) guard for similar setup; registering update consumers on non-cluster-manager nodes wastes resources and can conflict with tests that don't include a full ClusterSettings. Move this block inside the existing cluster-manager guard. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [352-355] -maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); -retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +if (DiscoveryNode.isClusterManagerNode(settings)) { + maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); + retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +} Suggestion importance[1-10]: 5 __ Why: Reasonable style/consistency improvement to place retry settings inside the existing cluster-manager guard, matching the surrounding pattern. Impact is minor since the settings are cheap to register.	Low
	Preserve original exception in assertion assert false : new AssertionError(...) uses AssertionError as the message object, so the assertion detail will be the AssertionError's toString() and the original cause e is lost from the thrown AssertionError. Pass e directly (or a message string) so the underlying exception is preserved as the assertion's cause when assertions are enabled. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3353-3360] if (ExceptionsHelper.unwrap(e, FailedToCommitClusterStateException.class) == null) { - // Unexpected failure class. Run the fallback first so cleanup always happens, then assert to surface - // the unexpected case in tests/dev (assertions disabled in production, where the fallback has already run). logger.error("Unexpected failure during cluster state update", e); failoverFallback.run(); - assert false : new AssertionError("Unexpected failure during cluster state update", e); + assert false : "Unexpected failure during cluster state update: " + e; return; } Suggestion importance[1-10]: 4 __ Why: Correct observation that wrapping in new AssertionError(...) as the assert message is awkward and doesn't preserve the cause chain well. Minor improvement affecting only diagnostic output when assertions are enabled.	Low

Suggestions up to commit 104557c

Category	Suggestion	Impact
Possible issue	Handle scheduling rejection to avoid stranded markers threadPool.schedule can throw OpenSearchRejectedExecutionException when the node is shutting down, which would silently drop the retry and leave the stranded marker this PR is trying to prevent. Wrap the schedule call and invoke the fallback if scheduling is rejected so cleanup still occurs on shutdown or during transient thread pool issues. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3372-3375] final int nextAttempt = attempt + 1; final TimeValue delay = computeBackoff(retryBackoff, attempt); logger.info("Publish failed for [{}] (attempt {}), scheduling retry in [{}]", source, nextAttempt, delay); -threadPool.schedule(() -> clusterService.submitStateUpdateTask(source, taskFactory.get()), delay, ThreadPool.Names.GENERIC); +try { + threadPool.schedule(() -> clusterService.submitStateUpdateTask(source, taskFactory.get()), delay, ThreadPool.Names.GENERIC); +} catch (Exception scheduleFailure) { + logger.warn(() -> new ParameterizedMessage("Failed to schedule retry for [{}], running fallback", source), scheduleFailure); + failoverFallback.run(); +} Suggestion importance[1-10]: 7 __ Why: A valid concern: if threadPool.schedule throws OpenSearchRejectedExecutionException during shutdown, the retry is silently dropped, defeating the goal of preventing stranded markers. The fallback wrapping is a reasonable safeguard.	Medium
General	Gate initial v2 cleanup on election The execute method in createStateWithoutSnapshotV2Task now short-circuits when the local node is not the elected cluster-manager. However, stateWithoutSnapshotV2 is invoked from the applier path where the passed-in state may not reflect the local election yet, so at first submission the task may also no-op unexpectedly. Consider gating the initial submission on the same election check (or checking the passed state here) to avoid submitting a task that unconditionally no-ops, and to keep behavior consistent between the initial attempt and retries. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [2491-2496] -if (changed) { +if (changed && state.nodes().isLocalNodeElectedClusterManager()) { logger.info("Cleaning up in progress v2 snapshots now"); final String source = "remove in progress snapshot v2 after cluster manager switch"; final int attempt = 0; clusterService.submitStateUpdateTask(source, createStateWithoutSnapshotV2Task(source, attempt)); } Suggestion importance[1-10]: 4 __ Why: The suggestion is reasonable but minor - the task itself already no-ops when not elected, so this is a small optimization for consistency rather than a correctness fix.	Low
	Preserve cause on unexpected-failure assertion Using assert false : new AssertionError(...) passes an AssertionError instance as the detail message to assert, which will call toString() on it rather than throw it. This works but is confusing and loses the cause chain in the resulting AssertionError. Prefer throwing an explicit AssertionError with the original exception as cause so the stack trace is preserved when assertions are enabled. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3359-3366] if (ExceptionsHelper.unwrap(e, FailedToCommitClusterStateException.class) == null) { - // Unexpected failure class. Run the fallback first so cleanup always happens, then assert to surface - // the unexpected case in tests/dev (assertions disabled in production, where the fallback has already run). logger.error("Unexpected failure during cluster state update", e); failoverFallback.run(); - assert false : new AssertionError("Unexpected failure during cluster state update", e); - return; + assert false : "Unexpected failure during cluster state update: " + e; + throw new AssertionError("Unexpected failure during cluster state update", e); } Suggestion importance[1-10]: 3 __ Why: The point about assert false : new AssertionError(...) being confusing is valid, but the improvement is stylistic. Additionally, adding an unconditional throw would change behavior when assertions are disabled.	Low

Suggestions up to commit a9a5f13

Category	Suggestion	Impact
General	Unblock listeners on exhausted v2 retries When the feature flag is disabled, onFailure for the v2 cleanup path now only logs a warning above and does nothing else — this is a behavior regression vs. the original code path which also just logged. However, when retries are exhausted the fallback only logs and never notifies any listeners or triggers failAllListenersOnMasterFailOver, which could leave callers waiting on snapshot completion. Consider invoking failAllListenersOnMasterFailOver(e) in the fallback to unblock listeners consistently with the other retry path. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [2458-2468] if (FeatureFlags.isEnabled(FeatureFlags.SNAPSHOT_RESILIENCE_SETTING)) { retryOrFailOnClusterManagerFailOver( e, attempt, source, () -> createStateWithoutSnapshotV2Task(source, attempt + 1), () -> { logger.error("Giving up on removing v2 snapshot state after {} attempts", attempt + 1); + failAllListenersOnMasterFailOver(e); } ); } Suggestion importance[1-10]: 6 __ Why: Points out a potentially meaningful gap where exhausted retries in the v2 cleanup path only log without notifying listeners, though the original pre-PR behavior also only logged, so this is more of an enhancement than a regression fix.	Low
	Handle lost-leadership case in retried task The retried task will silently no-op if this node is no longer cluster-manager, but no fallback/failover cleanup is invoked in that case. This means listeners may never be notified and no failAllListenersOnMasterFailOver is triggered, potentially leaving callers hanging. Consider invoking the failover fallback (or logging + notifying listeners) when the node has lost cluster-manager status. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [2430-2432] ClusterStateUpdateTask createStateWithoutSnapshotV2Task(String source, int attempt) { return new ClusterStateUpdateTask() { @Override public ClusterState execute(ClusterState currentState) { if (currentState.nodes().isLocalNodeElectedClusterManager() == false) { + logger.debug("Skipping v2 snapshot cleanup on [{}] as node is no longer cluster-manager", source); return currentState; } Suggestion importance[1-10]: 5 __ Why: Valid observation that the no-op path when losing cluster-manager status doesn't notify listeners, but the improved code only adds a debug log which doesn't fully address the concern raised in the suggestion content.	Low
	Register cleanup settings only on cluster-manager These initializations and settings-update consumer registrations run unconditionally, but the surrounding cluster-manager-only block above only registers such consumers when DiscoveryNode.isClusterManagerNode(settings) is true. On non-cluster-manager nodes, clusterService.getClusterSettings() may still work but registering consumers for cluster-manager-only behavior is wasteful and inconsistent. Move these into the cluster-manager-only branch. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [279-282] -maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); -retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +if (DiscoveryNode.isClusterManagerNode(settings)) { + // ... existing block ... + maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); + retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +} Suggestion importance[1-10]: 5 __ Why: Reasonable code organization suggestion for consistency with the surrounding cluster-manager-only block, though the functional impact is minimal since these settings are only used in cluster-manager paths.	Low
	Preserve exception context in assertion assert false : new AssertionError(...) uses the AssertionError as the detail message, so the underlying exception e is lost as a cause and the message becomes the toString of the AssertionError. Use assert false : "message: " + e; or throw with cause explicitly, so the original exception's stack trace is preserved when assertions are enabled. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3286-3293] if (ExceptionsHelper.unwrap(e, FailedToCommitClusterStateException.class) == null) { - // Unexpected failure class. Run the fallback first so cleanup always happens, then assert to surface - // the unexpected case in tests/dev (assertions disabled in production, where the fallback has already run). logger.error("Unexpected failure during cluster state update", e); failoverFallback.run(); - assert false : new AssertionError("Unexpected failure during cluster state update", e); + assert false : "Unexpected failure during cluster state update: " + e; return; } Suggestion importance[1-10]: 4 __ Why: Minor improvement to assertion messaging. The current form still produces a usable message but loses the underlying stack trace when assertions fire. Low impact since assertions are disabled in production.	Low

Suggestions up to commit 1cb6abd

Category	Suggestion	Impact
Possible issue	Prevent backoff overflow and cap delay The exponential backoff retryBackoff.millis() * (1L << attempt) can overflow to a negative value or produce excessively long delays for larger attempt values (e.g., attempt >= 63 overflows, and even attempt around 30 with a 1s base yields ~34 years). Cap the shift and/or the resulting delay to a safe maximum to avoid negative or absurd schedule delays. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3292-3298] if (attempt >= maxRetries) { logger.warn("Exhausted {} retries for [{}], falling back to failover handling", maxRetries, source); failoverFallback.run(); return; } final int nextAttempt = attempt + 1; -final TimeValue delay = TimeValue.timeValueMillis(retryBackoff.millis() * (1L << attempt)); +final int shift = Math.min(attempt, 30); +final long delayMillis = Math.min(retryBackoff.millis() * (1L << shift), TimeValue.timeValueMinutes(5).millis()); +final TimeValue delay = TimeValue.timeValueMillis(delayMillis); Suggestion importance[1-10]: 7 __ Why: Valid concern about potential overflow in 1L << attempt for large attempt values, and capping delay is a reasonable defensive measure, though maxRetries default is small (3) making the risk low in practice.	Medium
	Handle failover in retried task no-op path When retryOrFailOnClusterManagerFailOver schedules a retry, the original failSnapshotCompletionListeners/listener.onFailure are not invoked, which is intended. However, if the retry is later scheduled and the node subsequently loses cluster-manager role before the retry task runs, the retried task's execute returns currentState unchanged and clusterStateProcessed still fires failSnapshotCompletionListeners(snapshot, failure)—but failAllListenersOnMasterFailOver is never called. Ensure that the no-op path (not elected) also triggers failover handling so listeners aren't left dangling. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [2510-2513] @Override -public void onFailure(String src, Exception e) { - logger.warn(() -> new ParameterizedMessage("[{}] failed to remove snapshot metadata", snapshot), e); - if (FeatureFlags.isEnabled(FeatureFlags.SNAPSHOT_RESILIENCE_SETTING)) { - retryOrFailOnClusterManagerFailOver( - e, - attempt, - source, - () -> createRemoveFailedSnapshotTask(source, attempt + 1, snapshot, failure, repositoryData, listener), +public ClusterState execute(ClusterState currentState) { + if (currentState.nodes().isLocalNodeElectedClusterManager() == false) { + failAllListenersOnMasterFailOver(new NotClusterManagerException("no longer cluster-manager during retry")); + return currentState; + } + final ClusterState updatedState = stateWithoutSnapshot(currentState, snapshot); Suggestion importance[1-10]: 6 __ Why: Points to a possible edge case where listeners may not be properly notified when a retry runs after cluster-manager role loss, though clusterStateProcessed may still fire completion listeners.	Low
General	Register retry settings only on cluster-manager nodes These settings are registered unconditionally, but the previous cluster-manager-only block guards similar registrations with DiscoveryNode.isClusterManagerNode(settings). Registering update consumers on non-cluster-manager nodes is generally harmless but inconsistent; more importantly, on nodes that don't use SnapshotsService, this may still fire. Consider moving these inside the existing isClusterManagerNode guard for consistency and to avoid unnecessary listeners on data-only nodes. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [279-282] -maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); -retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); -clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +if (DiscoveryNode.isClusterManagerNode(settings)) { + maxRetries = SNAPSHOT_CLEANUP_RETRIES_SETTING.get(settings); + retryBackoff = SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING.get(settings); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRIES_SETTING, i -> maxRetries = i); + clusterService.getClusterSettings().addSettingsUpdateConsumer(SNAPSHOT_CLEANUP_RETRY_BACKOFF_SETTING, t -> retryBackoff = t); +} Suggestion importance[1-10]: 4 __ Why: Reasonable consistency improvement, but the current unconditional registration is functionally harmless.	Low
	Use nanoTime and relax backoff assertion The factory countDown() is invoked when submitStateUpdateTask is called, but clusterService is a mock so the elapsed time only measures scheduler latency, not the full backoff before submission. Since System.currentTimeMillis is measured after the scheduled task actually runs, this is fine—but using System.nanoTime is more robust against wall-clock adjustments, and the assertion threshold (900ms) may be flaky on slow CI. Consider using nanoTime and a slightly more lenient threshold. server/src/test/java/org/opensearch/snapshots/RetryOrFailOnClusterManagerFailOverTests.java [178-186] +long start = System.nanoTime(); snapshotsService.retryOrFailOnClusterManagerFailOver(new FailedToCommitClusterStateException("test"), 0, "test-source", () -> { taskSubmitted.countDown(); return mock(ClusterStateUpdateTask.class); }, () -> {}); assertTrue(taskSubmitted.await(5, TimeUnit.SECONDS)); -long elapsed = System.currentTimeMillis() - start; -assertTrue("Backoff should be at least ~1s, was " + elapsed + "ms", elapsed >= 900L); +long elapsedMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); +assertTrue("Backoff should be at least ~1s, was " + elapsedMs + "ms", elapsedMs >= 800L); Suggestion importance[1-10]: 3 __ Why: Minor test robustness improvement to reduce potential flakiness on slow CI environments.	Low

Suggestions up to commit 99f35b1

Category	Suggestion	Impact
Possible issue	Avoid assertion short-circuiting fallback execution The assert false statement will throw an AssertionError when assertions are enabled, preventing failoverFallback.run() from being executed and bypassing the intended fallback behavior in test/dev environments. Since the code below indicates fallback should run for unexpected exceptions, remove the assert or restructure so the fallback always runs. This inconsistency is also exposed by testUnexpectedExceptionRunsFallback, where the fallback is never actually invoked despite the test's stated intent. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3234-3239] if (ExceptionsHelper.unwrap(e, FailedToCommitClusterStateException.class) == null) { + logger.error("Unexpected failure during cluster state update", e); assert false : new AssertionError("Unexpected failure during cluster state update", e); - logger.error("Unexpected failure during cluster state update", e); failoverFallback.run(); return; } Suggestion importance[1-10]: 5 __ Why: Valid observation that assert false will throw before failoverFallback.run() when assertions are enabled, but the "improved_code" doesn't actually fix the issue (it just reorders lines while keeping assert before the fallback). Score reflects the correct identification but flawed fix.	Low
General	Handle failure path when feature flag disabled When the feature flag is disabled, onFailure only logs a warning and never attempts fallback handling, leaving the stranded-marker bug unfixed even in the legacy path. Additionally, the retry path never handles NotClusterManagerException cleanup. Ensure some fallback (e.g., calling failAllListenersOnMasterFailOver or clearing state) is executed regardless of the feature flag when the exception is a failover-type exception. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [2455-2466] if (FeatureFlags.isEnabled(FeatureFlags.SNAPSHOT_RESILIENCE_SETTING)) { retryOrFailOnClusterManagerFailOver( e, attempt, source, () -> createStateWithoutSnapshotV2Task(source, attempt + 1), - () -> { - logger.warn("Giving up on removing v2 snapshot state after {} attempts", attempt + 1); - } + () -> logger.warn("Giving up on removing v2 snapshot state after {} attempts", attempt + 1) ); +} else { + logger.warn("Snapshot resilience disabled; not retrying removal of v2 snapshot state"); } Suggestion importance[1-10]: 4 __ Why: Reasonable concern that the legacy path (flag disabled) loses fallback behavior, but the improved code only adds a log message rather than restoring meaningful fallback handling.	Low
	Guard backoff array index against overflow RETRY_BACKOFF has length 3 but MAX_RETRIES is also 3, so indexing RETRY_BACKOFF[attempt] when attempt == 2 accesses index 2 (fine), but if MAX_RETRIES is ever adjusted independently this will throw ArrayIndexOutOfBoundsException. Guard against index overflow by clamping to the last available backoff or asserting the invariant. server/src/main/java/org/opensearch/snapshots/SnapshotsService.java [3245-3248] final int nextAttempt = attempt + 1; -final TimeValue delay = RETRY_BACKOFF[attempt]; +final TimeValue delay = RETRY_BACKOFF[Math.min(attempt, RETRY_BACKOFF.length - 1)]; logger.info("Publish failed for [{}] (attempt {}), scheduling retry in [{}]", source, nextAttempt, delay); threadPool.schedule(() -> clusterService.submitStateUpdateTask(source, taskFactory.get()), delay, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 3 __ Why: Minor defensive coding improvement; currently there is no bug since attempt < MAX_RETRIES guards the index, but the change adds resilience to future modifications.	Low

[github-actions]

✅ Gradle check result for 99f35b1: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 53.24675% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.45%. Comparing base (993f75a) to head (a9a5f13).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ava/org/opensearch/snapshots/SnapshotsService.java	52.00%	35 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22518      +/-   ##
============================================
- Coverage     73.50%   73.45%   -0.06%     
+ Complexity    76507    76465      -42     
============================================
  Files          6104     6104              
  Lines        346618   346670      +52     
  Branches      49888    49895       +7     
============================================
- Hits         254793   254641     -152     
- Misses        71541    71796     +255     
+ Partials      20284    20233      -51     

☔ View full report in Codecov by Harness.
📢 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.

[shourya035]

Please add some integration tests to ensure that the feature works end to end with induced cluster manager failures

[github-actions]

Persistent review updated to latest commit 1cb6abd

[github-actions]

❌ Gradle check result for 1cb6abd: 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 a9a5f13

[github-actions]

✅ Gradle check result for a9a5f13: SUCCESS

[github-actions]

Persistent review updated to latest commit 104557c

[github-actions]

Persistent review updated to latest commit a0f69b9

[github-actions]

Persistent review updated to latest commit 5577c6f

[shourya035]

Why do we need this check?

[shourya035]

We should handle rejections from this block, otherwise this would throw unhandled exceptions

[shourya035]

We should add bounds to prevent this from going into negative values

[shourya035]

These settings should be applied on cluster manager nodes only, just like how we did for MAX_CONCURRENT_SNAPSHOT_OPERATIONS_SETTING

[github-actions]

❌ Gradle check result for 5577c6f: 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?

[shourya035]

Same here, what is the motive behind having the cluster manager active check?

[shourya035]

Can we also assert the state of the snapshot here?

[shourya035]

What is the difference between this test and testSnapshotMarkerCleanedUpAfterClusterManagerFailover?

[shourya035]

We can have this on the existing tests itself. We are deleting indices after every test, if the index deletion goes through, we should be able to assert the contents of this test