Avoid counting snapshot failures twice in SLM (#136759) (#136849)

We came across a scenario where 3 snapshot failures were counted as 5
"invocations since last success", resulting in a premature yellow SLM
health indicator. The three snapshot failures completed at virtually the
same time. Our theory is that the listener of the first snapshot failure
already processed the other two snapshot failures (incrementing the
`invocationsSinceLastSuccess`), but the listeners of those other two
snapshots then incremented that field too. There we two warning logs
indicating that the snapshots weren't found in the registered set,
confirming our hypothesis.

We simply avoid incrementing `invocationsSinceLastSuccess` if the
listener failed with an exception and the snapshot isn't registered
anymore; assuming that another listener has already incremented the
field.

Author: nielsbauman

docs/changelog/136759.yaml

@@ -0,0 +1,5 @@
+pr: 136759
+summary: Avoid counting snapshot failures twice in SLM
+area: ILM+SLM
+type: bug
+issues: []

x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java

@@ -497,7 +497,8 @@ public ClusterState execute(ClusterState currentState) throws Exception {
             final SnapshotLifecyclePolicyMetadata.Builder newPolicyMetadata = SnapshotLifecyclePolicyMetadata.builder(policyMetadata);
             SnapshotLifecycleStats newStats = snapMeta.getStats();
 
-            if (registeredSnapshots.contains(snapshotId) == false) {
+            final boolean snapshotIsRegistered = registeredSnapshots.contains(snapshotId);
+            if (snapshotIsRegistered == false) {
                 logger.warn(
                     "Snapshot [{}] not found in registered set after snapshot completion. This means snapshot was"
                         + " recorded as a failure by another snapshot's cleanup run.",
@@ -564,7 +565,11 @@ public ClusterState execute(ClusterState currentState) throws Exception {
                         exception.map(SnapshotLifecycleTask::exceptionToString).orElse(null)
                     )
                 );
-                newPolicyMetadata.incrementInvocationsSinceLastSuccess();
+                // If the snapshot was not registered, it means it was already counted as a failure by another snapshot's cleanup run
+                // so we should not increment the invocationsSinceLastSuccess again.
+                if (snapshotIsRegistered) {
+                    newPolicyMetadata.incrementInvocationsSinceLastSuccess();
+                }
             } else {
                 newStats = newStats.withTakenIncremented(policyName);
                 newPolicyMetadata.setLastSuccess(

x-pack/plugin/slm/src/test/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTaskTests.java

@@ -528,7 +528,8 @@ public void testCleanUpRegisteredInitiatedByFailure() throws Exception {
                 inferredFailureSnapshot,
                 snapshotInfoSuccess.snapshotId(),
                 snapshotInfoFailure1.snapshotId(),
-                snapshotInfoFailure2.snapshotId()
+                snapshotInfoFailure2.snapshotId(),
+                initiatingSnapshot
             )
         );
         var inProgress = Map.of(policyId, List.of(stillRunning));