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 5d0316d)

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 used to construct the ClusterStateObserver. The StoredState comparison in ClusterStateObserver uses the persistent node ID (via getClusterManagerNodeId()), while ClusterManagerNodeChangePredicate compares ephemeral IDs. This is consistent with the original behavior, but the two different ID types being passed in the same retry call could be confusing and error-prone if the constructor signature changes. This is not a bug in the current code but worth noting for clarity. 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, Missing @Nullable The new ClusterStateObserver constructors accept clusterManagerNodeId as a String that can be null (as documented and used in tests), but the parameter is not annotated with @Nullable. The existing constructor that takes ClusterState uses @Nullable for timeout. Omitting @Nullable on clusterManagerNodeId may cause confusion for callers about whether null is a valid input. public ClusterStateObserver( String clusterManagerNodeId, long version, ClusterService clusterService, @Nullable TimeValue timeout, Logger logger, ThreadContext contextHolder ) { this(clusterManagerNodeId, version, clusterService.getClusterApplierService(), timeout, logger, contextHolder); } public ClusterStateObserver( String clusterManagerNodeId, long version, ClusterApplierService clusterApplierService, @Nullable TimeValue timeout, Logger logger, ThreadContext contextHolder ) { this.clusterApplierService = clusterApplierService; this.threadPool = clusterApplierService.threadPool(); this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); this.timeOutValue = timeout; if (timeOutValue != null) { this.startTimeMS = threadPool.relativeTimeInMillis(); } this.logger = logger; this.contextHolder = contextHolder; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5d0316d

Explore these optional code suggestions:

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

---

Previous suggestions

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

Suggestions up to commit 52f23ab

Category	Suggestion	Impact
General	Replace DiscoveryNode parameter with primitive string ID The retry method receives the full DiscoveryNode clusterManagerNode object and extracts getId() from it inside retry. This still retains the DiscoveryNode reference in the closure. Since only the persistent node ID string is needed for ClusterStateObserver, the DiscoveryNode parameter in retry should be replaced with a pre-extracted String persistentNodeId to fully achieve the goal of reducing object retention. 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 correctly identifies that passing the full DiscoveryNode object to retry still retains a reference to it in the closure, partially defeating the PR's goal of reducing ClusterState retention. Replacing DiscoveryNode with a pre-extracted String persistentNodeId would more completely achieve the stated objective, making this a valid improvement to the PR's approach.	Low

Suggestions up to commit 670bd25

Category	Suggestion	Impact
Possible issue	Fix inconsistent node ID types in retry logic The predicate is built using the ephemeral ID (getEphemeralId()), but the ClusterStateObserver is constructed using the persistent ID (getId()). These are different identifiers and may not match correctly across restarts or reconnections. Both the predicate and the observer should use the same type of node ID (either both ephemeral or both persistent) to ensure consistent behavior. 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)); } Suggestion importance[1-10]: 6 __ Why: The predicate uses getEphemeralId() while the observer uses getId() (persistent ID), which are different identifiers. This inconsistency could cause subtle bugs, though the improved_code changes the retry method signature to accept persistentNodeId as a String which doesn't match the actual retry method signature in the PR (which still takes DiscoveryNode).	Low
	Clarify persistent vs ephemeral node ID usage The new constructor accepting String clusterManagerNodeId uses the persistent node ID, but StoredState comparison logic in isOlderOrDifferentMaster uses clusterManagerNodeId (persistent ID from getClusterManagerNodeId()). However, in TransportClusterManagerNodeAction, the retryOnMasterChange method extracts the ephemeral ID for the predicate but the persistent ID for the observer. Verify that StoredState consistently uses persistent node IDs and that the comparison in isOlderOrDifferentMaster aligns with what is passed to this constructor. server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java [120-129] public ClusterStateObserver( - String clusterManagerNodeId, + String clusterManagerPersistentNodeId, long version, ClusterService clusterService, @Nullable TimeValue timeout, Logger logger, ThreadContext contextHolder ) { - this(clusterManagerNodeId, version, clusterService.getClusterApplierService(), timeout, logger, contextHolder); + this(clusterManagerPersistentNodeId, version, clusterService.getClusterApplierService(), timeout, logger, contextHolder); } Suggestion importance[1-10]: 2 __ Why: The suggestion only renames the parameter to clusterManagerPersistentNodeId without changing any logic, and the improved_code is functionally identical to the existing_code. The actual concern about consistent ID usage is valid but addressed more directly in suggestion 2.	Low
General	Avoid retaining DiscoveryNode object in retry closure The retry method still accepts a full DiscoveryNode clusterManagerNode parameter, but only uses clusterManagerNode.getId() from it. Since the goal of this PR is to avoid retaining the full ClusterState in closures, the DiscoveryNode object itself is also a non-trivial object that could be simplified. Consider changing the parameter to accept String clusterManagerPersistentNodeId directly to fully achieve the memory reduction goal. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [297-314] private void retry( final long stateVersion, - final DiscoveryNode clusterManagerNode, + final String clusterManagerPersistentNodeId, final Throwable failure, final Predicate<ClusterState> statePredicate ) { ... - final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; this.observer = new ClusterStateObserver( - persistentNodeId, + clusterManagerPersistentNodeId, stateVersion, ... Suggestion importance[1-10]: 5 __ Why: The suggestion is valid — passing String clusterManagerPersistentNodeId instead of DiscoveryNode would better achieve the PR's goal of reducing object retention in closures. However, the existing_code snippet uses ... placeholders making it hard to verify exact line correspondence, and the change would require updating all call sites.	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.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.28%. Comparing base (6082a2a) to head (5d0316d).

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20858      +/-   ##
============================================
- Coverage     73.28%   73.28%   -0.01%     
- Complexity    72490    72537      +47     
============================================
  Files          5819     5819              
  Lines        331398   331421      +23     
  Branches      47887    47890       +3     
============================================
+ Hits         242875   242878       +3     
- Misses        68984    69056      +72     
+ Partials      19539    19487      -52     

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

Persistent review updated to latest commit fd80fda

[github-actions]

❌ Gradle check result for fd80fda: 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 fd80fda

[github-actions]

❌ Gradle check result for fd80fda: 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 670bd25

[github-actions]

❌ Gradle check result for 670bd25: 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 52f23ab

[github-actions]

❌ Gradle check result for 52f23ab: null

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 892ee63

[github-actions]

Persistent review updated to latest commit f61b6a9

[github-actions]

Persistent review updated to latest commit f61b6a9

[github-actions]

✅ Gradle check result for f61b6a9: SUCCESS

[github-actions]

Persistent review updated to latest commit 5d0316d

[github-actions]

✅ Gradle check result for 5d0316d: SUCCESS