Fail the shard copy in the local cluster state in LocalShardStateAction

[msfroh]

Description

In clusterless mode, localShardFailed was a no-op. IndicesClusterStateService puts a failed shard in its failedShardsCache and does not create it again until the shard's routing entry names a different allocation. With the entry left as it was, whatever builds the local cluster state kept reusing it, so the copy never came back and the state kept reporting it as started.

localShardFailed now does locally what AllocationService#applyFailedShards does on a cluster manager: it moves the copy to unassigned with ALLOCATION_FAILED, the message and failure, and an incremented failed-allocation count. The copy is looked up by allocation id in the state being updated, not by equality with the routing that failed, since that routing may be out of date (for example, marked started since). A failure for an allocation that is no longer in the state changes nothing. Primary terms and in-sync allocation ids are left alone, and so is re-allocation; those belong to whatever builds the local cluster state. The listener is completed once the update is applied, or with the failure.

Related Issues

N/A

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 🔍

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 Behavioral change in failedShard The extracted failedShard factory sets failedNodeIds to Collections.emptySet() when previous == null, but the original inline logic in AllocationService#applyFailedShards (before this refactor) also produced an empty set in that branch — so this preserves behavior there. However, when previous != null, the new code adds failedShard.currentNodeId() to the set unconditionally; if currentNodeId() is null (the shard is already unassigned when failed again), a null entry gets inserted into a HashSet. The old code behaved the same way, so this is not strictly a regression, but it's worth confirming callers always pass an assigned shard. In LocalShardStateAction.fail, failedShard is always the current (assigned) copy, so this is safe there; verify no other new caller passes an unassigned routing. public static UnassignedInfo failedShard( ShardRouting failedShard, String message, @Nullable Exception failure, long unassignedTimeNanos, long unassignedTimeMillis ) { UnassignedInfo previous = failedShard.unassignedInfo(); int failedAllocations = previous != null ? previous.getNumFailedAllocations() : 0; final Set<String> failedNodeIds; if (previous != null) { failedNodeIds = new HashSet<>(previous.getFailedNodeIds().size() + 1); failedNodeIds.addAll(previous.getFailedNodeIds()); failedNodeIds.add(failedShard.currentNodeId()); } else { failedNodeIds = Collections.emptySet(); } return new UnassignedInfo( Reason.ALLOCATION_FAILED, "failed shard on node [" + failedShard.currentNodeId() + "]: " + message, failure, failedAllocations + 1, unassignedTimeNanos, unassignedTimeMillis, false, AllocationStatus.NO_ATTEMPT, failedNodeIds ); } Possible NPE on relocation target lookup In fail, when failed.isRelocationTarget() is true, the code calls find(copies, failed.allocationId().getRelocationId()) to locate the source. For a relocation target routing, allocationId().getRelocationId() is the source's allocation id, so find should return the source. If, however, failed was derived from a stale snapshot and the source has since been replaced in copies, this returns null and the branch silently does nothing (relocation is not cancelled), leaving the state inconsistent. Since failShard already looks up current by allocation id before calling fail, this is unlikely in practice, but worth adding an assertion or explicit handling. if (failed.isRelocationTarget()) { // Cancels the relocation and leaves nothing unassigned. ShardRouting source = find(copies, failed.allocationId().getRelocationId()); if (source != null) { replace(copies, source, source.cancelRelocation()); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Always record the failing node id When there is no previous UnassignedInfo, the failed node id is dropped on the floor, so the first failure never records the node it failed on. The pre-existing behavior in AllocationService had the same bug, but since this is now a reusable helper it should always include failedShard.currentNodeId() in failedNodeIds (guarded against null for shards without a current node). server/src/main/java/org/opensearch/cluster/routing/UnassignedInfo.java [340-347] - failedNodeIds = new HashSet<>(previous.getFailedNodeIds().size() + 1); - failedNodeIds.addAll(previous.getFailedNodeIds()); - failedNodeIds.add(failedShard.currentNodeId()); - } else { - failedNodeIds = Collections.emptySet(); - } + failedNodeIds = new HashSet<>((previous != null ? previous.getFailedNodeIds().size() : 0) + 1); + if (previous != null) { + failedNodeIds.addAll(previous.getFailedNodeIds()); + } + if (failedShard.currentNodeId() != null) { + failedNodeIds.add(failedShard.currentNodeId()); + } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that when previous is null, the first failure's node id is not recorded in failedNodeIds. However, this preserves pre-existing behavior from AllocationService, and changing it could alter semantics beyond the scope of this PR. The observation is valid but the impact is moderate.	Low
General	Clarify handling of derived relocation targets recoveringReplicas returns the allocation ids of relocation targets as well, but find resolves those to the derived relocation-target ShardRouting. Calling fail with a relocation target is handled, but the subsequent replace on a derived target then searches via copies.indexOf(existing) which returns -1 (the derived instance is not in the list) and only recovers by re-finding the source. This path works but is fragile; consider resolving to the actual source entry here, or document that fail must receive a copy obtained via find. server/src/main/java/org/opensearch/cluster/action/shard/LocalShardStateAction.java [157-159] for (String recovering : recoveringReplicas(copies)) { ShardRouting replica = find(copies, recovering); if (replica != null && replica.initializing()) { + // replica may be a derived relocation target; fail() and replace() handle that via the source entry. Suggestion importance[1-10]: 3 __ Why: The suggestion only proposes adding a comment to document existing behavior, which is a minor documentation improvement with low impact.	Low

[github-actions]

❌ Gradle check result for 1ac5449: 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]

✅ Gradle check result for 1ac5449: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 84.69388% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.83%. Comparing base (c1c1be1) to head (1ac5449).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
...ch/cluster/action/shard/LocalShardStateAction.java	82.55%	3 Missing and 12 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #23220      +/-   ##
============================================
+ Coverage     71.77%   71.83%   +0.06%     
- Complexity    77953    78061     +108     
============================================
  Files          6185     6185              
  Lines        361363   361432      +69     
  Branches      52596    52618      +22     
============================================
+ Hits         259370   259643     +273     
+ Misses        81440    81148     -292     
- Partials      20553    20641      +88     

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