ILM Explain: valid JSON on truncated step info

docs/changelog/137638.yaml

@@ -0,0 +1,6 @@
+pr: 137638
+summary: "ILM Explain: valid JSON on truncated step info"
+area: ILM+SLM
+type: bug
+issues:
+ - 135458

server/src/main/java/org/elasticsearch/cluster/metadata/LifecycleExecutionState.java

@@ -287,15 +287,16 @@ public Map<String, String> asMap() {
         return Collections.unmodifiableMap(result);
     }
 
-    public static String truncateWithExplanation(String input) {
-        if (input != null && input.length() > MAXIMUM_STEP_INFO_STRING_LENGTH) {
-            return Strings.cleanTruncate(input, MAXIMUM_STEP_INFO_STRING_LENGTH)
-                + "... ("
-                + (input.length() - MAXIMUM_STEP_INFO_STRING_LENGTH)
-                + " chars truncated)";
-        } else {
-            return input;
+    public static String potentiallyTruncateLongJsonWithExplanation(String json) {
+        if (json == null) {
+            return null;
         }
+        // Avoid no-op truncations: we must append `"}` (2 chars) to the end to keep JSON valid
+        if (json.length() <= MAXIMUM_STEP_INFO_STRING_LENGTH + 2) {
+            return json;
+        }
+        final int actuallyRemovedCharacters = json.length() - MAXIMUM_STEP_INFO_STRING_LENGTH - 2;
+        return Strings.cleanTruncate(json, MAXIMUM_STEP_INFO_STRING_LENGTH) + "... (" + actuallyRemovedCharacters + " chars truncated)\"}";
     }
 
     public static class Builder {
@@ -340,7 +341,7 @@ public Builder setFailedStep(String failedStep) {
         }
 
         public Builder setStepInfo(String stepInfo) {
-            this.stepInfo = truncateWithExplanation(stepInfo);
+            this.stepInfo = potentiallyTruncateLongJsonWithExplanation(stepInfo);
             return this;
         }
 

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

@@ -8,6 +8,7 @@
 package org.elasticsearch.xpack.core.ilm;
 
 import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.EqualsHashCodeTestUtils;
 
@@ -30,11 +31,11 @@ public void testTruncatingStepInfo() {
         assertThat(custom.get("step_info"), equalTo(state.stepInfo()));
         String longStepInfo = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 100);
         LifecycleExecutionState newState = LifecycleExecutionState.builder(state).setStepInfo(longStepInfo).build();
-        // Length includes the post suffix
-        assertThat(newState.stepInfo().length(), equalTo(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 25));
+        // Length includes the post suffix (`... (X chars truncated)`)
+        assertThat(newState.stepInfo().length(), equalTo(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 26));
         assertThat(
-            newState.stepInfo().substring(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH, 1049),
-            equalTo("... (100 chars truncated)")
+            newState.stepInfo().substring(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH, 1050),
+            equalTo("... (98 chars truncated)\"}")
         );
     }
 
@@ -145,6 +146,38 @@ public void testGetCurrentStepKey() {
         assertNull(error6.getMessage());
     }
 
+    public void testPotentiallyTruncateLongJsonWithExplanationNotTruncated() {
+        // +2 because with the JSON ending in `"}`, we'll add it back anyway so it's a NOP
+        final String input = randomAlphaOfLengthBetween(0, LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 2);
+        assertSame(input, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
+    }
+
+    public void testPotentiallyTruncateLongJsonWithExplanationOneCharTruncated() {
+        final String jsonBaseFormat = "{\"key\": \"%s\"}";
+        final int baseLength = Strings.format(jsonBaseFormat, "").length();
+        final String value = randomAlphanumericOfLength(
+            // +3 because +1 and +2 are no-ops, they will truncate the end of JSON `"}` and then add it back,
+            // so we need another char to truncate.
+            LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - baseLength + 3
+        );
+        final String input = Strings.format(jsonBaseFormat, value);
+        final String expectedOutput = Strings.format(jsonBaseFormat, value.substring(0, value.length() - 1) + "... (1 chars truncated)");
+        assertEquals(expectedOutput, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
+    }
+
+    public void testPotentiallyTruncateLongJsonWithExplanationTwoCharsTruncated() {
+        final String jsonBaseFormat = "{\"key\": \"%s\"}";
+        final int baseLength = Strings.format(jsonBaseFormat, "").length();
+        final String value = randomAlphanumericOfLength(
+            // +4 because +1 and +2 are no-ops, they will truncate the end of JSON `"}` and then add it back,
+            // so we need another two chars to truncate.
+            LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - baseLength + 4
+        );
+        final String input = Strings.format(jsonBaseFormat, value);
+        final String expectedOutput = Strings.format(jsonBaseFormat, value.substring(0, value.length() - 2) + "... (2 chars truncated)");
+        assertEquals(expectedOutput, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
+    }
+
     private LifecycleExecutionState mutate(LifecycleExecutionState toMutate) {
         LifecycleExecutionState.Builder newState = LifecycleExecutionState.builder(toMutate);
         switch (randomIntBetween(0, 18)) {

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

@@ -87,13 +87,7 @@ public static ILMHistoryItem failure(
     ) {
         Objects.requireNonNull(error, "ILM failures require an attached exception");
         String fullErrorString = exceptionToString(error);
-        String truncatedErrorString = LifecycleExecutionState.truncateWithExplanation(fullErrorString);
-        if (truncatedErrorString.equals(fullErrorString) == false) {
-            // Append a closing quote and closing brace to attempt to make it valid JSON.
-            // There is no requirement that it actually be valid JSON, so this is
-            // best-effort, but does not cause problems if it is still invalid.
-            truncatedErrorString += "\"}";
-        }
+        String truncatedErrorString = LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(fullErrorString);
         return new ILMHistoryItem(index, policyId, timestamp, indexAge, false, executionState, truncatedErrorString);
     }
 

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

@@ -7,13 +7,20 @@
 
 package org.elasticsearch.xpack.ilm.action;
 
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
 import org.elasticsearch.cluster.metadata.ProjectMetadata;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.index.IndexVersion;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xcontent.NamedXContentRegistry;
 import org.elasticsearch.xcontent.ParseField;
+import org.elasticsearch.xcontent.ToXContent;
+import org.elasticsearch.xcontent.XContentFactory;
+import org.elasticsearch.xcontent.XContentParser;
+import org.elasticsearch.xcontent.XContentParserConfiguration;
+import org.elasticsearch.xcontent.XContentType;
 import org.elasticsearch.xpack.core.ilm.ErrorStep;
 import org.elasticsearch.xpack.core.ilm.IndexLifecycleExplainResponse;
 import org.elasticsearch.xpack.core.ilm.IndexLifecycleMetadata;
@@ -31,6 +38,7 @@
 
 import static org.elasticsearch.cluster.metadata.LifecycleExecutionState.ILM_CUSTOM_METADATA_KEY;
 import static org.elasticsearch.xpack.ilm.action.TransportExplainLifecycleAction.getIndexLifecycleExplainResponse;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
@@ -217,6 +225,84 @@ public void testGetIndexLifecycleExplainResponse_rolloverOnlyIfHasDocuments_adds
         assertThat(rolloverAction.getConditions().getMinDocs(), is(1L));
     }
 
+    public void testPreviousStepInfoTruncationDoesNotBreakExplainJson() throws Exception {
+        final String policyName = "policy";
+        final String indexName = "index";
+
+        final String longReasonMessage = "a".repeat(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH);
+        final String errorJsonWhichWillBeTruncated = Strings.toString((builder, params) -> {
+            ElasticsearchException.generateThrowableXContent(
+                builder,
+                ToXContent.EMPTY_PARAMS,
+                new IllegalArgumentException(longReasonMessage)
+            );
+            return builder;
+        });
+
+        final LifecycleExecutionState stateWithTruncatedStepInfo = LifecycleExecutionState.builder()
+            .setPhase("hot")
+            .setAction("rollover")
+            .setStep("some_step")
+            .setPhaseDefinition(PHASE_DEFINITION)
+            .setStepInfo(errorJsonWhichWillBeTruncated)
+            .build();
+
+        // Simulate transition to next step where previous_step_info is copied
+        final LifecycleExecutionState stateWithPreviousStepInfo = LifecycleExecutionState.builder(stateWithTruncatedStepInfo)
+            .setPreviousStepInfo(stateWithTruncatedStepInfo.stepInfo())
+            .setStepInfo(null)
+            .build();
+
+        final IndexMetadata indexMetadata = IndexMetadata.builder(indexName)
+            .settings(settings(IndexVersion.current()).put(LifecycleSettings.LIFECYCLE_NAME, policyName))
+            .numberOfShards(1)
+            .numberOfReplicas(0)
+            .putCustom(ILM_CUSTOM_METADATA_KEY, stateWithPreviousStepInfo.asMap())
+            .build();
+
+        final ProjectMetadata project = ProjectMetadata.builder(randomProjectIdOrDefault())
+            .put(indexMetadata, true)
+            .putCustom(
+                IndexLifecycleMetadata.TYPE,
+                new IndexLifecycleMetadata(
+                    Map.of(policyName, LifecyclePolicyMetadataTests.createRandomPolicyMetadata(policyName)),
+                    randomFrom(OperationMode.values())
+                )
+            )
+            .build();
+
+        final IndexLifecycleExplainResponse response = TransportExplainLifecycleAction.getIndexLifecycleExplainResponse(
+            indexName,
+            project,
+            false,
+            true,
+            REGISTRY,
+            randomBoolean()
+        );
+
+        final String serialized = Strings.toString(response);
+        // test we produce valid JSON
+        try (
+            XContentParser p = XContentFactory.xContent(XContentType.JSON)
+                .createParser(XContentParserConfiguration.EMPTY.withRegistry(REGISTRY), serialized)
+        ) {
+            final IndexLifecycleExplainResponse deserialized = IndexLifecycleExplainResponse.PARSER.apply(p, null);
+            assertThat(deserialized.toString(), equalTo(response.toString()));
+            final String actualPreviousStepInfo = deserialized.getPreviousStepInfo().utf8ToString();
+
+            final String expectedPreviousStepInfoFormat = """
+                {"type":"illegal_argument_exception","reason":"%s"}""";
+            final int jsonCharsBeforeReasonValue = Strings.format(expectedPreviousStepInfoFormat, "").length() - 2; // -2 for `"}`
+            final String expectedReason = Strings.format(
+                "%s... (%d chars truncated)",
+                "a".repeat(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - jsonCharsBeforeReasonValue),
+                jsonCharsBeforeReasonValue
+            );
+            final String expectedPreviousStepInfo = Strings.format(expectedPreviousStepInfoFormat, expectedReason);
+            assertThat(actualPreviousStepInfo, equalTo(expectedPreviousStepInfo));
+        }
+    }
+
     private static IndexLifecycleMetadata createIndexLifecycleMetadata() {
         return new IndexLifecycleMetadata(
             Map.of(POLICY_NAME, LifecyclePolicyMetadataTests.createRandomPolicyMetadata(POLICY_NAME)),