Reduce ClusterState retention in retry closures

[HarishNarasimhanK]

Description

1. Goal

In OpenSearch, snapshot deletion is a cluster manager routed operation. When a delete request is received, the cluster manager creates internal callback objects (listeners) to track the operation and notify the caller once it completes. These listeners inadvertently hold a reference to a large in-memory object called ClusterState, which contains the entire cluster's metadata, routing information, and index definitions.

When a snapshot deletion gets stuck or takes a long time to complete, users or automated systems may retry the delete request multiple times. As listeners accumulate from repeated retries, multiple ClusterState objects get pinned on the heap, causing the cluster manager node's memory usage to grow until it runs out of memory.

This change fixes the issue by ensuring that the listeners only hold the small pieces of information they actually need (a version number and a node identifier) instead of the entire ClusterState object. This allows the large ClusterState objects to be garbage collected immediately, preventing the memory buildup.

2. Current Workflow

This section traces the lifecycle of a snapshot delete request from the REST API to the point where the listener is stored in SnapshotsService.

1. A client sends DELETE /_snapshot/{repository}/{snapshot}.

2. The REST layer (RestDeleteSnapshotAction) constructs a DeleteSnapshotRequest and passes it to NodeClient.

3. NodeClient dispatches the request to TransportDeleteSnapshotAction, which extends TransportClusterManagerNodeAction.

4. The base class creates an AsyncSingleAction instance to manage the request lifecycle. AsyncSingleAction fetches the current ClusterState and calls doStart(clusterState).

5. If the local node is the cluster manager, doStart() wraps the original listener using getDelegateForLocalExecute(clusterState). This wrapper contains a lambda for retry logic that references the clusterState parameter. Due to Java lambda capture semantics, the entire ClusterState object is implicitly retained by this lambda for as long as the listener exists.

6. The wrapped listener is passed into TransportDeleteSnapshotAction.clusterManagerOperation(), which calls snapshotsService.deleteSnapshots(request, listener). The listener still carries the captured ClusterState reference inside its retry lambda.

7. Inside SnapshotsService, the deletion is submitted as a cluster state update. Once the cluster state is updated to record the deletion, the listener is stored in the snapshotDeletionListeners map (keyed by the deletion UUID) in order to notify the client when the deletion completes.

3. Issue with Current Workflow

• The listener stored in snapshotDeletionListeners sits in the map until the repository-level deletion reaches a terminal state. If the deletion is stuck (due to slow I/O, stuck segment uploads, large repository cleanup, or any other reason), the listener remains in snapshotDeletionListeners indefinitely, and the captured ClusterState cannot be garbage collected.

• For each subsequent delete request, SnapshotsService adds another listener to snapshotDeletionListeners through the same path. As delete requests accumulate, these listeners pile up, each pinning a ClusterState object on the heap. The cluster manager node's heap usage grows monotonically with each repeated delete, eventually leading to OutOfMemoryError.

4. Requirements

• Reduce the size of the data retained by retry closures. Instead of capturing the full ClusterState object, closures should only hold the minimal primitives required for retry decisions.

• Preserve existing retry behavior and backward compatibility.

5. Approach: Extract Primitives Before Closure Creation

The retry closures in TransportClusterManagerNodeAction only need two pieces of information from the ClusterState to make retry decisions: the cluster state version (a long) and the cluster manager node's ephemeral ID (a String). By extracting these values before creating any lambda or anonymous class, the closures capture only these lightweight primitives. The full ClusterState object is no longer referenced by any closure and becomes eligible for garbage collection immediately.

Sequence Diagram

Implementation Steps

1. In getDelegateForLocalExecute inside AsyncSingleAction, the cluster state version and the cluster manager node are extracted before the lambda is created. The lambda now references only these extracted values, so the full ClusterState is no longer retained.

2. A new overloaded retryOnMasterChange method is added that accepts the version and cluster manager node directly. It extracts the ephemeral ID from the cluster manager node and passes it to the predicate builder.

3. The original retryOnMasterChange method that accepts a full ClusterState is kept as a convenience bridge. It extracts the version and cluster manager node and delegates to the new overload.

4. The retry method signature is updated to accept the version and cluster manager node instead of the full ClusterState. It uses the persistent node ID and version to construct the cluster state observer via a new primitives-based constructor.

5. A new overloaded build method is added to ClusterManagerNodeChangePredicate that accepts the version and ephemeral ID directly. The existing build method that accepts a full ClusterState is refactored to extract these values and delegate to the new overload.

6. Two new constructors are added to ClusterStateObserver that accept the cluster manager node ID and version as primitives, instead of requiring a full ClusterState object.

7. The StoredState inner class inside ClusterStateObserver is refactored to support construction from primitives. The existing constructor that accepts a ClusterState now delegates to the new primitives-based constructor.

6. Validation

The fix was validated by reproducing the memory retention issue on a local cluster and comparing heap dumps before and after the change.

Reproduction Setup

1. Added a Thread.sleep() call in BlobStoreRepository.doDeleteShardSnapshots() to simulate a long-running deletion that stays stuck.

2. Created 500 indices with heavy mappings (50+ fields each), multiple aliases per index, and a small number of documents per index to inflate the ClusterState size.

3. Created one snapshot per index (500 snapshots total) using a filesystem-based snapshot repository.

4. Spammed delete requests for all snapshots repeatedly, so that listeners accumulate in snapshotDeletionListeners while the deletion is stuck.

5. Captured heap dumps from the cluster manager node and compared the retained size of listener chains.

Results

Before (without fix)

After (with fix)

Related Issues

Resolves #15065

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 b467299)

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: Add primitive overload to ClusterManagerNodeChangePredicate Relevant files: server/src/main/java/org/opensearch/cluster/ClusterManagerNodeChangePredicate.java server/src/test/java/org/opensearch/cluster/ClusterManagerNodeChangePredicateTests.java Sub-PR theme: Add primitive constructors to ClusterStateObserver Relevant files: server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java server/src/test/java/org/opensearch/cluster/ClusterStateObserverTests.java Sub-PR theme: Reduce ClusterState retention in TransportClusterManagerNodeAction retry closures Relevant files: server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java CHANGELOG.md

⚡ Recommended focus areas for review ID Mismatch In retryOnMasterChange, the ephemeral ID is extracted from clusterManagerNode and passed to ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId). However, in retry, the persistent ID (clusterManagerNode.getId()) is extracted and passed to ClusterStateObserver. These two IDs serve different purposes and are consistent with their respective consumers, but the asymmetry (ephemeral ID for the predicate, persistent ID for the observer) is subtle and worth verifying. Specifically, ClusterManagerNodeChangePredicate compares ephemeral IDs while StoredState compares persistent (node) IDs — if the intent is to detect node restarts, ephemeral IDs are correct for the predicate but persistent IDs in StoredState would miss a restart of the same node. This was pre-existing behavior in StoredState, but the new constructor now makes it possible to pass a mismatched ID type by mistake. private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } private void retry( final long stateVersion, final DiscoveryNode clusterManagerNode, final Throwable failure, final Predicate<ClusterState> statePredicate ) { if (observer == null) { final long remainingTimeoutMS = request.clusterManagerNodeTimeout().millis() - (threadPool.relativeTimeInMillis() - startTime); if (remainingTimeoutMS <= 0) { logger.debug(() -> new ParameterizedMessage("timed out before retrying [{}] after failure", actionName), failure); listener.onFailure(new ClusterManagerNotDiscoveredException(failure)); return; } final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; this.observer = new ClusterStateObserver( persistentNodeId, stateVersion, Incomplete Assertion In testClusterStateLatestCheckerHandlesTransportException, after electing localNode as master and asserting listener.isDone(), the test calls listener.get() and only checks assertNotNull. Since Action.clusterManagerOperation is not overridden in this test, the default implementation may return null or throw, making the assertion trivially pass or the test misleadingly succeed without verifying the actual response value. setState(clusterService, ClusterStateCreationUtils.state(localNode, localNode, allNodes)); assertTrue(listener.isDone()); assertNotNull(listener.get()); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b467299

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode object in retry closure The retry method receives the full DiscoveryNode clusterManagerNode object only to extract persistentNodeId for the ClusterStateObserver. Since the goal of this PR is to avoid retaining the full ClusterState, passing the full DiscoveryNode into the retry closure still retains a reference to a DiscoveryNode object. Consider passing only the pre-extracted persistentNodeId (String) directly to retry instead of the full DiscoveryNode. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String persistentNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + if (observer == null) { + ... + this.observer = new ClusterStateObserver( + persistentNodeId, + stateVersion, + clusterService, + TimeValue.timeValueMillis(remainingTimeoutMS), + logger, + threadPool.getThreadContext() + ); + } + ... +} + Suggestion importance[1-10]: 5 __ Why: This is a valid improvement aligned with the PR's goal of reducing object retention in closures. Passing only the persistentNodeId string instead of the full DiscoveryNode to retry avoids retaining the DiscoveryNode object, which is a minor but meaningful memory optimization consistent with the PR's intent.	Low
	Verify consistent node ID type usage The new ClusterStateObserver constructor accepts a clusterManagerNodeId that is the persistent node ID (from DiscoveryNode.getId()), but StoredState compares it against clusterState.nodes().getClusterManagerNodeId() which also returns the persistent ID — this is consistent. However, in retryOnMasterChange, the ephemeralNodeId is passed to ClusterManagerNodeChangePredicate.build while the persistentNodeId is passed to ClusterStateObserver. These two IDs are different and serve different comparison purposes, but it's worth verifying that StoredState.isOlderOrDifferentMaster uses the same ID type as what is stored, to avoid silent mismatches if the logic ever changes. server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java [141] +// Ensure the stored ID matches what StoredState.isOlderOrDifferentMaster compares against +// (persistent node ID from getClusterManagerNodeId()) this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify/ensure correctness via a comment, and the existing_code and improved_code are functionally identical (only a comment is added). The concern about ID type consistency is valid but already correctly handled in the PR — persistentNodeId is used for StoredState and ephemeralNodeId for the predicate.	Low

---

Previous suggestions

Suggestions up to commit b467299

Category	Suggestion	Impact
Possible issue	Inconsistent node ID types passed to observer vs predicate The retry method receives clusterManagerNode (a DiscoveryNode) and passes clusterManagerNode.getId() (persistent ID) to ClusterStateObserver, while ClusterManagerNodeChangePredicate.build is called with clusterManagerNode.getEphemeralId(). The observer's StoredState uses persistent ID for comparison, but the predicate uses ephemeral ID. These two checks are inconsistent and may cause the observer to behave differently from the predicate when detecting cluster manager changes. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + retry(stateVersion, ephemeralNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } Suggestion importance[1-10]: 7 __ Why: This correctly identifies a real inconsistency: retry passes clusterManagerNode.getId() (persistent ID) to ClusterStateObserver while ClusterManagerNodeChangePredicate.build uses ephemeralNodeId. The improved_code proposes changing the retry signature to use ephemeralNodeId instead of DiscoveryNode, which would make both mechanisms consistent. However, this would require changing the retry method signature significantly, and the improved_code doesn't fully reflect all the cascading changes needed.	Medium
	Inconsistent node ID types between observer and predicate The new ClusterStateObserver constructor accepts a clusterManagerNodeId that is the persistent node ID (from DiscoveryNode.getId()), but StoredState compares it against clusterState.nodes().getClusterManagerNodeId() which also returns the persistent ID. However, ClusterManagerNodeChangePredicate.build uses the ephemeral ID (getEphemeralId()). The two mechanisms use different ID types, which could cause inconsistent change detection. Ensure both StoredState and ClusterManagerNodeChangePredicate use the same type of node identifier. server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java [141] +// Ensure consistent use of persistent node ID in StoredState and ephemeral ID in predicate, +// or document clearly which ID type is expected here. this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); Suggestion importance[1-10]: 6 __ Why: This is a valid observation: StoredState uses persistent node ID (getId()) while ClusterManagerNodeChangePredicate uses ephemeral ID (getEphemeralId()). However, the 'improved_code' only adds a comment without actually fixing the inconsistency, making it a documentation-only suggestion rather than a real fix.	Low
General	Test should verify node ID type assumption explicitly The test constructs the observer with masterNode.getId() (persistent ID) and version 1, while the current state has version 5 with the same master. The test asserts the listener is NOT added (immediate acceptance). However, StoredState.isOlderOrDifferentMaster compares clusterManagerNodeId against clusterState.nodes().getClusterManagerNodeId() (persistent ID), so this should work. But if the implementation is later changed to use ephemeral IDs, this test would silently pass for the wrong reason. Consider adding an explicit assertion on the node ID type being used to make the test's intent clear. server/src/test/java/org/opensearch/cluster/ClusterStateObserverTests.java [122-129] -// Construct with persistent node ID and version 1 — newerState has version 5, same master +// Construct with persistent node ID and version 1 — newerState has version 5, same master (persistent ID match) +assertEquals(masterNode.getId(), newerState.nodes().getClusterManagerNodeId()); // verify persistent ID is used final ClusterStateObserver observer = new ClusterStateObserver( masterNode.getId(), 1L, clusterApplierService, TimeValue.timeValueSeconds(30), logger, new ThreadContext(Settings.EMPTY) ); Suggestion importance[1-10]: 2 __ Why: This suggestion adds a defensive assertion to clarify which ID type is used, but it's a minor test improvement that guards against hypothetical future changes. The added assertion doesn't test new behavior and the suggestion is more about documentation/clarity than correctness.	Low

Suggestions up to commit 5d0316d

Category	Suggestion	Impact
Possible issue	Mismatched node ID types in retry predicate ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId) uses the ephemeral ID, but the new ClusterStateObserver constructor stores the persistent node ID (clusterManagerNode.getId()) in StoredState. These two checks use different ID types, so a node restart (same persistent ID, new ephemeral ID) would be detected by the predicate but not by the observer's stored state, leading to inconsistent retry behavior. Both should use the same ID type. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { - final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, persistentNodeId)); } Suggestion importance[1-10]: 7 __ Why: This is a valid concern — ClusterManagerNodeChangePredicate.build uses ephemeral IDs while ClusterStateObserver's StoredState uses persistent IDs. Using persistent IDs consistently in retryOnMasterChange would align the predicate with the observer's comparison logic, preventing potential inconsistencies during node restarts. The improved code accurately reflects the suggested change.	Medium
	Inconsistent node ID types between observer and predicate The new ClusterStateObserver constructor accepts a clusterManagerNodeId that is the persistent node ID (from DiscoveryNode.getId()), but StoredState compares it against clusterState.nodes().getClusterManagerNodeId() which also returns the persistent ID. However, ClusterManagerNodeChangePredicate.build(long, String) uses the ephemeral ID. The two mechanisms use different ID types, which can cause inconsistent change detection. Ensure both StoredState and ClusterManagerNodeChangePredicate use the same type of node identifier (either both persistent or both ephemeral). server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java [141] +// Ensure the clusterManagerNodeId passed here is consistent with what StoredState.isOlderOrDifferentMaster compares against (persistent ID via getClusterManagerNodeId()) this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that StoredState uses persistent node IDs (via getClusterManagerNodeId()) while ClusterManagerNodeChangePredicate.build(long, String) uses ephemeral IDs. However, the 'improved_code' only adds a comment without fixing the actual inconsistency, making it a low-impact change. The underlying concern is valid but the fix is incomplete.	Low

Suggestions up to commit f61b6a9

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode in retry closure The retry method now accepts a DiscoveryNode clusterManagerNode parameter, but this parameter is only used to extract persistentNodeId for the ClusterStateObserver constructor. Passing the full DiscoveryNode object into the retry closure reintroduces the object retention problem this PR aims to solve. Instead, extract the persistent node ID here and pass only the String to retry. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-302] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String persistentNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that passing a DiscoveryNode object into the retry method reintroduces the object retention problem this PR aims to solve. Only the persistent node ID string is needed inside retry for the ClusterStateObserver constructor. The improved code accurately reflects the change needed to pass only primitive/string values instead of the full DiscoveryNode.	Medium
	Avoid retaining DiscoveryNode in block retry closure Similar to the retryOnMasterChange refactoring, passing a DiscoveryNode into the retry method still retains the node object in the closure. Only the persistent node ID string is needed inside retry for the ClusterStateObserver constructor, so extract it here and pass the string instead. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [474-476] final long blockStateVersion = localClusterState.version(); final DiscoveryNode blockClusterManagerNode = localClusterState.nodes().getClusterManagerNode(); -retry(blockStateVersion, blockClusterManagerNode, blockException, newState -> { +final String blockPersistentNodeId = blockClusterManagerNode != null ? blockClusterManagerNode.getId() : null; +final String blockEphemeralNodeId = blockClusterManagerNode != null ? blockClusterManagerNode.getEphemeralId() : null; +retry(blockStateVersion, blockPersistentNodeId, blockException, newState -> { Suggestion importance[1-10]: 6 __ Why: This suggestion is consistent with suggestion 1 and correctly identifies that blockClusterManagerNode (a DiscoveryNode) is passed to retry but only its persistent ID is needed. However, the improved code extracts blockEphemeralNodeId which isn't used in this call path (the block retry uses a custom predicate, not ClusterManagerNodeChangePredicate), making the extraction of blockEphemeralNodeId unnecessary.	Low

Suggestions up to commit f61b6a9

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode in retry closure The retry method now accepts a DiscoveryNode clusterManagerNode parameter, but this parameter is only used to extract persistentNodeId for the ClusterStateObserver constructor. Passing the full DiscoveryNode object into the retry closure partially defeats the purpose of this PR (reducing ClusterState retention), since DiscoveryNode objects can themselves hold references. Consider passing only the pre-extracted persistentNodeId (String) and ephemeralNodeId (String) as primitives to retry, instead of the full DiscoveryNode. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String persistentNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + if (observer == null) { + ... + this.observer = new ClusterStateObserver( + persistentNodeId, + stateVersion, + clusterService, + TimeValue.timeValueMillis(remainingTimeoutMS), + logger, + threadPool.getThreadContext() + ); + } + ... +} + Suggestion importance[1-10]: 6 __ Why: The suggestion is valid and aligns with the PR's goal of reducing object retention in closures. Passing the full DiscoveryNode into retry when only persistentNodeId (a String) is needed does partially undermine the optimization. However, DiscoveryNode is a relatively lightweight object compared to ClusterState, so the impact is moderate.	Low
	Clarify test helper node-building intent When clusterManagerNode is null, otherClusterManagerNode is always added to the nodes builder without being set as the cluster manager. This is fine for the null-master tests, but it means the built state always contains otherClusterManagerNode as a non-master node, which could cause confusion or unexpected behavior if otherClusterManagerNode's ID accidentally matches something. More importantly, when clusterManagerNode == null, you should avoid adding otherClusterManagerNode unconditionally if the intent is to represent a state with no nodes, or at minimum add a comment clarifying the intent. server/src/test/java/org/opensearch/cluster/ClusterManagerNodeChangePredicateTests.java [33-41] private ClusterState buildState(DiscoveryNode clusterManagerNode, long version) { DiscoveryNodes.Builder nodesBuilder = DiscoveryNodes.builder(); if (clusterManagerNode != null) { nodesBuilder.add(clusterManagerNode); nodesBuilder.clusterManagerNodeId(clusterManagerNode.getId()); } + // Always include otherClusterManagerNode as a non-master node for realistic cluster topology nodesBuilder.add(otherClusterManagerNode); return ClusterState.builder(new ClusterName("test")).nodes(nodesBuilder).version(version).build(); } Suggestion importance[1-10]: 1 __ Why: The suggestion only adds a comment to clarify intent without changing any logic. The improved_code is functionally identical to the existing_code, making this a purely cosmetic change with minimal impact.	Low

Suggestions up to commit 892ee63

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode in retry closure The retry method receives the full DiscoveryNode clusterManagerNode object, which defeats the purpose of reducing ClusterState retention in closures — the DiscoveryNode itself can hold references to metadata. Since retry only uses clusterManagerNode to extract getId() for the ClusterStateObserver, consider passing only the persistent node ID string instead of the full DiscoveryNode. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String clusterManagerNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that passing the full DiscoveryNode to retry partially defeats the memory optimization goal of this PR, since DiscoveryNode can hold references. Changing retry to accept a String clusterManagerNodeId instead of DiscoveryNode would be more consistent with the PR's intent of reducing object retention in closures.	Low
Possible issue	Verify consistent node ID type usage The new constructor accepts a String clusterManagerNodeId which is the persistent node ID, but the existing StoredState comparison logic in ClusterStateObserver uses getClusterManagerNodeId() (persistent ID) for comparison. However, in TransportClusterManagerNodeAction, retryOnMasterChange passes clusterManagerNode.getEphemeralId() to ClusterManagerNodeChangePredicate.build() but passes clusterManagerNode.getId() (persistent ID) to the ClusterStateObserver constructor. These two IDs serve different comparison purposes and the inconsistency should be verified to ensure the observer correctly detects master changes when the same node restarts with a new ephemeral ID. server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java [120-129] public ClusterStateObserver( String clusterManagerNodeId, long version, ClusterService clusterService, @Nullable TimeValue timeout, Logger logger, ThreadContext contextHolder ) { this(clusterManagerNodeId, version, clusterService.getClusterApplierService(), timeout, logger, contextHolder); } +// Note: clusterManagerNodeId must be the persistent node ID (getId()), consistent with +// StoredState which uses getClusterManagerNodeId() for comparison. Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify that getId() (persistent ID) is used consistently with StoredState's comparison logic. This is a valid observation but the PR already correctly uses getId() for ClusterStateObserver and getEphemeralId() for ClusterManagerNodeChangePredicate, which is intentional design. The improved_code only adds a comment without changing logic, making this a low-impact suggestion.	Low

[github-actions]

Persistent review updated to latest commit 64a4b05

[github-actions]

❌ Gradle check result for 64a4b05: 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 ee994e5

[github-actions]

❌ Gradle check result for ee994e5: 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 1ceb433

[github-actions]

✅ Gradle check result for 1ceb433: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.16%. Comparing base (dbe98aa) to head (d4b3f62).

Files with missing lines	Patch %	Lines
...a/org/opensearch/cluster/ClusterStateObserver.java	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20858      +/-   ##
============================================
- Coverage     73.21%   73.16%   -0.05%     
+ Complexity    72620    72585      -35     
============================================
  Files          5849     5849              
  Lines        332066   332088      +22     
  Branches      47951    47954       +3     
============================================
- Hits         243109   242984     -125     
- Misses        69456    69645     +189     
+ Partials      19501    19459      -42     

☔ 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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit b8784a1.

Path	Line	Severity	Description
server/src/test/java/org/opensearch/cluster/ClusterStateObserverTests.java	346	low	Trivially-passing assertion: `assertFalse("should not need to add listener", false)` always passes regardless of `listenerAdded.get()`, silently removing the verification that no timeout listener was registered. Should be `assertFalse("should not need to add listener", listenerAdded.get())`. Likely a typo but weakens test coverage for the new primitive constructor path.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit b8784a1

[github-actions]

Persistent review updated to latest commit 9ed0978

[github-actions]

✅ Gradle check result for 9ed0978: SUCCESS

[github-actions]

Persistent review updated to latest commit ebe17a4

[github-actions]

❕ Gradle check result for ebe17a4: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

Persistent review updated to latest commit 4195b48

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

❕ Gradle check result for 6df52d7: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

✅ Gradle check result for e42dc18: SUCCESS

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

❌ Gradle check result for 4957af3: 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]

Failed to generate code suggestions for PR

[github-actions]

✅ Gradle check result for 87ed45d: SUCCESS

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

❌ Gradle check result for 4be892a: 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]

Failed to generate code suggestions for PR

[github-actions]

❌ Gradle check result for 4be892a: 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]

Failed to generate code suggestions for PR

[github-actions]

✅ Gradle check result for 1e72dcf: SUCCESS

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

✅ Gradle check result for d4b3f62: SUCCESS