Fix PolicyStepsRegistry's cachedSteps null handling (#84588)

Author: joegallo

docs/changelog/84588.yaml

@@ -0,0 +1,5 @@
+pr: 84588
+summary: Fix `PolicyStepsRegistry's` `cachedSteps` null handling
+area: ILM+SLM
+type: bug
+issues: []

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -305,14 +305,30 @@ private List<Step> parseStepsFromPhase(String policy, String currentPhase, Strin
         return phaseSteps;
     }
 
+    /**
+     * Read-only internal helper for getStep that returns a non-null step if one is cached for the provided
+     * IndexMetadata and StepKey, and null otherwise.
+     */
     @Nullable
-    public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKey) {
+    private Step getCachedStep(final IndexMetadata indexMetadata, final Step.StepKey stepKey) {
         final Tuple<IndexMetadata, Step> cachedStep = cachedSteps.get(indexMetadata.getIndex());
         // n.b. we're using instance equality here for the IndexMetadata rather than object equality because it's fast,
         // this means that we're erring on the side of cache misses (if the IndexMetadata changed in any way, it'll be
         // a new instance, so we'll miss-and-repopulate the cache for the index in question)
-        if (cachedStep != null && cachedStep.v1() == indexMetadata && cachedStep.v2().getKey().equals(stepKey)) {
-            return cachedStep.v2();
+        if (cachedStep != null && cachedStep.v1() == indexMetadata) {
+            assert cachedStep.v2() != null : "null steps should never be cached in the policy step registry";
+            if (cachedStep.v2() != null && cachedStep.v2().getKey().equals(stepKey)) {
+                return cachedStep.v2();
+            }
+        }
+        return null;
+    }
+
+    @Nullable
+    public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKey) {
+        final Step cachedStep = getCachedStep(indexMetadata, stepKey);
+        if (cachedStep != null) {
+            return cachedStep;
         }
 
         if (ErrorStep.NAME.equals(stepKey.getName())) {
@@ -354,7 +370,12 @@ public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKe
 
         // Return the step that matches the given stepKey or else null if we couldn't find it
         final Step s = phaseSteps.stream().filter(step -> step.getKey().equals(stepKey)).findFirst().orElse(null);
-        cachedSteps.put(indexMetadata.getIndex(), Tuple.tuple(indexMetadata, s));
+        if (s != null) {
+            cachedSteps.put(indexMetadata.getIndex(), Tuple.tuple(indexMetadata, s));
+            // assert that the cache works as expected -- that is, if we put something into the cache,
+            // we should get back the same thing if we were to invoke getStep again with the same arguments
+            assert s == getCachedStep(indexMetadata, stepKey) : "policy step registry cache failed sanity check";
+        }
         return s;
     }
 

x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistryTests.java

@@ -191,11 +191,10 @@ public void testGetStepUnknownStepKey() {
         SortedMap<String, LifecyclePolicyMetadata> metas = new TreeMap<>();
         metas.put("policy", policyMetadata);
         PolicyStepsRegistry registry = new PolicyStepsRegistry(metas, null, null, REGISTRY, client, null);
-        Step actualStep = registry.getStep(
-            indexMetadata,
-            new Step.StepKey(step.getKey().getPhase(), step.getKey().getAction(), step.getKey().getName() + "-bad")
-        );
-        assertNull(actualStep);
+        Step.StepKey badStepKey = new Step.StepKey(step.getKey().getPhase(), step.getKey().getAction(), step.getKey().getName() + "-bad");
+        assertNull(registry.getStep(indexMetadata, badStepKey));
+        // repeat the test to make sure that nulls don't poison the registry's cache
+        assertNull(registry.getStep(indexMetadata, badStepKey));
     }
 
     public void testUpdateFromNothingToSomethingToNothing() throws Exception {