diff --git a/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryConfiguration.java b/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryConfiguration.java index d829dd48ca..f328640577 100644 --- a/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryConfiguration.java +++ b/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryConfiguration.java @@ -14,6 +14,7 @@ import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.DISABLE_SESSION_FIELD; import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.EMBEDDING_MODEL_ID_FIELD; import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.EMBEDDING_MODEL_TYPE_FIELD; +import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.INDEX_PREFIX_INVALID_CHARACTERS_ERROR; import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.INDEX_SETTINGS_FIELD; import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.INVALID_EMBEDDING_MODEL_TYPE_ERROR; import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.LLM_ID_FIELD; @@ -36,6 +37,8 @@ import java.util.Map; import java.util.UUID; +import org.opensearch.OpenSearchParseException; +import org.opensearch.cluster.metadata.MetadataCreateIndexService; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.common.io.stream.Writeable; @@ -133,6 +136,14 @@ private String buildIndexPrefix(String indexPrefix, boolean useSystemIndex) { ? DEFAULT_MEMORY_INDEX_PREFIX : UUID.randomUUID().toString().replace("-", "").substring(0, 8).toLowerCase(); } + if (indexPrefix.indexOf('\r') >= 0 || indexPrefix.indexOf('\n') >= 0 || indexPrefix.chars().anyMatch(ch -> ch < 32)) { + throw new OpenSearchParseException(INDEX_PREFIX_INVALID_CHARACTERS_ERROR); + } + MetadataCreateIndexService + .validateIndexOrAliasName( + indexPrefix, + (s1, s2) -> new OpenSearchParseException("missing or invalid index prefix [" + s1 + "] " + s2) + ); return indexPrefix; } @@ -615,11 +626,6 @@ public static String extractModelIdFromPipeline(Map pipelineSour if (modelId instanceof String) { return (String) modelId; } - log - .warn( - "Pipeline text_embedding model_id is not a String: {}", - modelId != null ? modelId.getClass().getSimpleName() : "null" - ); } } // Check sparse_encoding processor diff --git a/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryContainerConstants.java b/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryContainerConstants.java index b348877cc8..b514bac737 100644 --- a/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryContainerConstants.java +++ b/common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryContainerConstants.java @@ -134,6 +134,11 @@ public class MemoryContainerConstants { public static final String INVALID_EMBEDDING_MODEL_TYPE_ERROR = "Embedding model type must be either TEXT_EMBEDDING or SPARSE_ENCODING"; public static final String MAX_INFER_SIZE_LIMIT_ERROR = "Maximum infer size cannot exceed 10"; public static final String FIELD_NOT_ALLOWED_SEMANTIC_DISABLED_ERROR = "Field %s is not allowed when semantic storage is disabled"; + public static final String INDEX_PREFIX_INVALID_CHARACTERS_ERROR = "Index prefix must not contain any control characters"; + public static final String BACKEND_ROLE_INVALID_LENGTH_ERROR = "Backend role exceeds maximum length of 128 characters: %s"; + public static final String BACKEND_ROLE_INVALID_CHARACTERS_ERROR = + "Backend role contains invalid characters. Only alphanumeric characters and :+=,.@-_/ are allowed: %s"; + public static final String BACKEND_ROLE_EMPTY_ERROR = "Backend role cannot be empty or blank"; // Model validation error messages public static final String LLM_MODEL_NOT_FOUND_ERROR = "LLM model with ID %s not found"; diff --git a/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInput.java b/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInput.java index 10df37b596..d73c8c132b 100644 --- a/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInput.java +++ b/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInput.java @@ -8,11 +8,16 @@ import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken; import static org.opensearch.ml.common.CommonValue.BACKEND_ROLES_FIELD; import static org.opensearch.ml.common.CommonValue.TENANT_ID_FIELD; +import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.BACKEND_ROLE_EMPTY_ERROR; +import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.BACKEND_ROLE_INVALID_CHARACTERS_ERROR; +import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.BACKEND_ROLE_INVALID_LENGTH_ERROR; import java.io.IOException; import java.util.ArrayList; import java.util.List; +import java.util.regex.Pattern; +import org.opensearch.OpenSearchParseException; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.common.io.stream.Writeable; @@ -58,6 +63,7 @@ public MLCreateMemoryContainerInput( this.configuration = configuration; } this.tenantId = tenantId; + validateBackendRoles(backendRoles); this.backendRoles = backendRoles; } @@ -114,6 +120,42 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } + /** + * Validates backend roles according to security constraints. + * Each role must: + * - Be at most 128 Unicode characters long + * - Contain only alphanumeric characters and special symbols: :+=,.@-_/ + * - Not be empty or blank + * + * @param backendRoles List of backend role strings to validate (null/empty list is allowed) + * @throws OpenSearchParseException if any role violates constraints + */ + public static void validateBackendRoles(List backendRoles) { + if (backendRoles == null || backendRoles.isEmpty()) { + return; // null or empty list is allowed + } + + // Regex pattern: Unicode letters/digits + allowed special chars: :+=,.@-_/ + Pattern validPattern = Pattern.compile("^[\\p{L}\\p{N}:+=,.@\\-_/]+$"); + + for (String role : backendRoles) { + // Check for null or empty + if (role == null || role.isEmpty() || role.isBlank()) { + throw new OpenSearchParseException(BACKEND_ROLE_EMPTY_ERROR); + } + + // Check length (Unicode character count) + if (role.length() > 128) { + throw new OpenSearchParseException(String.format(BACKEND_ROLE_INVALID_LENGTH_ERROR, role)); + } + + // Check allowed characters + if (!validPattern.matcher(role).matches()) { + throw new OpenSearchParseException(String.format(BACKEND_ROLE_INVALID_CHARACTERS_ERROR, role)); + } + } + } + public static MLCreateMemoryContainerInput parse(XContentParser parser) throws IOException { String name = null; String description = null; diff --git a/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInput.java b/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInput.java index 1b95d3438e..77d7ed0114 100644 --- a/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInput.java +++ b/common/src/main/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInput.java @@ -9,6 +9,7 @@ import static org.opensearch.ml.common.CommonValue.BACKEND_ROLES_FIELD; import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.DESCRIPTION_FIELD; import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.NAME_FIELD; +import static org.opensearch.ml.common.transport.memorycontainer.MLCreateMemoryContainerInput.validateBackendRoles; import java.io.IOException; import java.util.ArrayList; @@ -40,6 +41,7 @@ public class MLUpdateMemoryContainerInput implements ToXContentObject, Writeable public MLUpdateMemoryContainerInput(String name, String description, List backendRoles, MemoryConfiguration configuration) { this.name = name; this.description = description; + validateBackendRoles(backendRoles); this.backendRoles = backendRoles; this.configuration = configuration; } diff --git a/common/src/test/java/org/opensearch/ml/common/memorycontainer/MemoryConfigurationTests.java b/common/src/test/java/org/opensearch/ml/common/memorycontainer/MemoryConfigurationTests.java index 3ee92867e3..236adf0f43 100644 --- a/common/src/test/java/org/opensearch/ml/common/memorycontainer/MemoryConfigurationTests.java +++ b/common/src/test/java/org/opensearch/ml/common/memorycontainer/MemoryConfigurationTests.java @@ -8,6 +8,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -18,6 +19,7 @@ import java.util.Map; import org.junit.Test; +import org.opensearch.OpenSearchParseException; import org.opensearch.ml.common.FunctionName; import org.opensearch.ml.common.memorycontainer.MemoryConfiguration.EmbeddingConfig; @@ -993,4 +995,89 @@ public void testGetMemoryIndexMapping_EmptySettings() { Map result = config.getMemoryIndexMapping("any-index"); assertNull(result); } + + // ==================== buildIndexPrefix CRLF Injection Tests ==================== + + @Test + public void testBuildIndexPrefix_RejectsCarriageReturn() { + OpenSearchParseException exception = assertThrows( + OpenSearchParseException.class, + () -> MemoryConfiguration.builder().indexPrefix("test\rinjection").build() + ); + assertTrue(exception.getMessage().contains("control characters")); + } + + @Test + public void testBuildIndexPrefix_RejectsLineFeed() { + OpenSearchParseException exception = assertThrows( + OpenSearchParseException.class, + () -> MemoryConfiguration.builder().indexPrefix("test\ninjection").build() + ); + assertTrue(exception.getMessage().contains("control characters")); + } + + @Test + public void testBuildIndexPrefix_RejectsCRLF() { + OpenSearchParseException exception = assertThrows( + OpenSearchParseException.class, + () -> MemoryConfiguration.builder().indexPrefix("test\r\ninjection").build() + ); + assertTrue(exception.getMessage().contains("control characters")); + } + + @Test + public void testBuildIndexPrefix_RejectsTab() { + OpenSearchParseException exception = assertThrows( + OpenSearchParseException.class, + () -> MemoryConfiguration.builder().indexPrefix("test\tinjection").build() + ); + assertTrue(exception.getMessage().contains("control characters")); + } + + @Test + public void testBuildIndexPrefix_RejectsNullCharacter() { + OpenSearchParseException exception = assertThrows( + OpenSearchParseException.class, + () -> MemoryConfiguration.builder().indexPrefix("test\u0000injection").build() + ); + assertTrue(exception.getMessage().contains("control characters")); + } + + @Test + public void testBuildIndexPrefix_RejectsOtherControlCharacters() { + // Test ASCII control character 1 (SOH - Start of Header) + OpenSearchParseException exception = assertThrows( + OpenSearchParseException.class, + () -> MemoryConfiguration.builder().indexPrefix("test\u0001injection").build() + ); + assertTrue(exception.getMessage().contains("control characters")); + } + + @Test + public void testBuildIndexPrefix_BackslashRejectedByMetadataCreateIndexService() { + // Verify that backslash is rejected by OpenSearch's built-in validation + // MetadataCreateIndexService.validateIndexOrAliasName handles this + OpenSearchParseException exception = assertThrows( + OpenSearchParseException.class, + () -> MemoryConfiguration.builder().indexPrefix("test\\backslash").build() + ); + // The error message should come from MetadataCreateIndexService + assertTrue( + exception.getMessage().contains("invalid") + || exception.getMessage().contains("index") + || exception.getMessage().contains("prefix") + ); + } + + @Test + public void testBuildIndexPrefix_AcceptsValidPrefix() { + MemoryConfiguration config = MemoryConfiguration.builder().indexPrefix("valid-prefix-123").build(); + assertEquals("valid-prefix-123", config.getIndexPrefix()); + } + + @Test + public void testBuildIndexPrefix_AcceptsHyphenAndUnderscore() { + MemoryConfiguration config = MemoryConfiguration.builder().indexPrefix("valid_prefix-with-chars").build(); + assertEquals("valid_prefix-with-chars", config.getIndexPrefix()); + } } diff --git a/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInputTests.java b/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInputTests.java index 05934db86e..5917d837bd 100644 --- a/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInputTests.java +++ b/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/MLCreateMemoryContainerInputTests.java @@ -11,9 +11,13 @@ import static org.opensearch.core.xcontent.ToXContent.EMPTY_PARAMS; import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import org.junit.Before; import org.junit.Test; +import org.opensearch.OpenSearchParseException; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.XContentType; @@ -400,4 +404,185 @@ private void assertTrue(boolean condition) { private void assertFalse(boolean condition) { org.junit.Assert.assertFalse(condition); } + + // Backend Roles Validation Tests + + @Test + public void testValidateBackendRoles_ValidRoles() { + // Test various valid backend roles + List validRoles = Arrays + .asList( + "admin", + "user123", + "team:developers", + "role+test", + "email@domain.com", + "path/to/resource", + "key=value", + "role-name_test", + "config.property", + "complex:role+name@test.com/path_to-resource=value" + ); + + MLCreateMemoryContainerInput input = MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(validRoles).build(); + + assertNotNull(input); + assertEquals(validRoles, input.getBackendRoles()); + } + + @Test + public void testValidateBackendRoles_UnicodeCharacters() { + // Test Unicode alphanumeric characters + List unicodeRoles = Arrays.asList("用户角色", "роль", "角色123"); + + MLCreateMemoryContainerInput input = MLCreateMemoryContainerInput + .builder() + .name("test-container") + .backendRoles(unicodeRoles) + .build(); + + assertNotNull(input); + assertEquals(unicodeRoles, input.getBackendRoles()); + } + + @Test + public void testValidateBackendRoles_NullList() { + // Null list should be allowed + MLCreateMemoryContainerInput input = MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(null).build(); + + assertNotNull(input); + assertNull(input.getBackendRoles()); + } + + @Test + public void testValidateBackendRoles_EmptyList() { + // Empty list should be allowed + List emptyList = Collections.emptyList(); + + MLCreateMemoryContainerInput input = MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(emptyList).build(); + + assertNotNull(input); + assertEquals(emptyList, input.getBackendRoles()); + } + + @Test + public void testValidateBackendRoles_Exactly128Characters() { + // Edge case: exactly 128 characters should be valid + String exactly128 = "a".repeat(128); + List roles = Arrays.asList(exactly128); + + MLCreateMemoryContainerInput input = MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + + assertNotNull(input); + assertEquals(roles, input.getBackendRoles()); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_TooLong() { + // 129 characters - should fail + String tooLong = "a".repeat(129); + List roles = Arrays.asList(tooLong); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithSpaces() { + // Spaces are not allowed + List roles = Arrays.asList("role with spaces"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithTabs() { + // Tabs are not allowed + List roles = Arrays.asList("role\twith\ttabs"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithSemicolon() { + // Semicolon is not allowed + List roles = Arrays.asList("role;semicolon"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithPipe() { + // Pipe is not allowed + List roles = Arrays.asList("role|pipe"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithBackslash() { + // Backslash is not allowed + List roles = Arrays.asList("role\\backslash"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithHash() { + // Hash is not allowed + List roles = Arrays.asList("role#hash"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_EmptyString() { + // Empty string should be rejected + List roles = Arrays.asList(""); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_BlankString() { + // Blank string (only whitespace) should be rejected + List roles = Arrays.asList(" "); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_NullElement() { + // Null element in list should be rejected + List roles = Arrays.asList("valid-role", null, "another-role"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithAsterisk() { + // Asterisk is not allowed + List roles = Arrays.asList("role*wildcard"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_WithParentheses() { + // Parentheses are not allowed + List roles = Arrays.asList("role(parens)"); + + MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(roles).build(); + } + + @Test + public void testValidateBackendRoles_MixedValidRoles() { + // Test combination of different valid patterns + List mixedRoles = Arrays + .asList("admin", "user:123", "team+dev", "email@test.com", "path/to/role", "key=val", "name-test_role", "config.yml"); + + MLCreateMemoryContainerInput input = MLCreateMemoryContainerInput.builder().name("test-container").backendRoles(mixedRoles).build(); + + assertNotNull(input); + assertEquals(mixedRoles, input.getBackendRoles()); + } } diff --git a/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInputTests.java b/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInputTests.java index 91a0bb73c4..927527c003 100644 --- a/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInputTests.java +++ b/common/src/test/java/org/opensearch/ml/common/transport/memorycontainer/memory/MLUpdateMemoryContainerInputTests.java @@ -13,11 +13,13 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import org.junit.Test; +import org.opensearch.OpenSearchParseException; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.common.io.stream.StreamInput; @@ -524,4 +526,71 @@ public void testRoundTripSerializationWithLlmIdAndStrategies() throws IOExceptio ); } } + + // Backend Roles Validation Tests + + @Test + public void testValidateBackendRoles_ValidRoles() { + // Test various valid backend roles + List validRoles = Arrays + .asList("admin", "user123", "team:developers", "role+test", "email@domain.com", "path/to/resource", "key=value"); + + MLUpdateMemoryContainerInput input = MLUpdateMemoryContainerInput.builder().name("test-update").backendRoles(validRoles).build(); + + assertNotNull(input); + assertEquals(validRoles, input.getBackendRoles()); + } + + @Test + public void testValidateBackendRoles_NullList() { + // Null list should be allowed + MLUpdateMemoryContainerInput input = MLUpdateMemoryContainerInput.builder().name("test-update").backendRoles(null).build(); + + assertNotNull(input); + assertNull(input.getBackendRoles()); + } + + @Test + public void testValidateBackendRoles_EmptyList() { + // Empty list should be allowed + List emptyList = Collections.emptyList(); + + MLUpdateMemoryContainerInput input = MLUpdateMemoryContainerInput.builder().name("test-update").backendRoles(emptyList).build(); + + assertNotNull(input); + assertEquals(emptyList, input.getBackendRoles()); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_TooLong() { + // 129 characters - should fail + String tooLong = "a".repeat(129); + List roles = Arrays.asList(tooLong); + + MLUpdateMemoryContainerInput.builder().name("test-update").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_InvalidCharacters() { + // Spaces are not allowed + List roles = Arrays.asList("role with spaces"); + + MLUpdateMemoryContainerInput.builder().name("test-update").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_EmptyString() { + // Empty string should be rejected + List roles = Arrays.asList(""); + + MLUpdateMemoryContainerInput.builder().name("test-update").backendRoles(roles).build(); + } + + @Test(expected = OpenSearchParseException.class) + public void testValidateBackendRoles_NullElement() { + // Null element in list should be rejected + List roles = Arrays.asList("valid-role", null); + + MLUpdateMemoryContainerInput.builder().name("test-update").backendRoles(roles).build(); + } } diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerAction.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerAction.java index f7fdffda7e..a6118c9261 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerAction.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerAction.java @@ -116,9 +116,12 @@ protected void doExecute(Task task, MLCreateMemoryContainerRequest request, Acti } catch (Exception e) { log.error("Failed to create memory container", e); - listener.onFailure(e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } - }, listener::onFailure); + }, e -> { + log.error("Failed to initialize memory container index", e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); + }); // Initialize memory container index if it doesn't exist mlIndicesHandler.initMemoryContainerIndex(indexCheckListener); @@ -257,7 +260,7 @@ private void indexMemoryContainer(MLMemoryContainer container, ActionListener { log.error("Failed to check for shared index prefix, aborting deletion for safety", e); - actionListener.onFailure(e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } else { // No index deletion requested, proceed with container-only deletion @@ -150,12 +152,13 @@ protected void doExecute(Task task, ActionRequest request, ActionListener { log.error("Failed to retrieve memory container: {} for deletion", memoryContainerId, error); - actionListener.onFailure(error); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } @@ -165,6 +168,7 @@ private void deleteMemoryContainer( String tenantId, boolean deleteAllMemories, Set deleteMemories, + User user, ActionListener listener ) { try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { @@ -184,12 +188,13 @@ private void deleteMemoryContainer( deleteAllMemories, deleteMemories, container, + user, listener ) ); } catch (Exception e) { log.error("Failed to delete Memory Container: {}", memoryContainerId, e); - listener.onFailure(e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } @@ -200,6 +205,7 @@ private void handleDeleteResponse( boolean deleteAllMemories, Set deleteMemories, MLMemoryContainer container, + User user, ActionListener actionListener ) { if (throwable != null) { @@ -209,16 +215,29 @@ private void handleDeleteResponse( } else { try { DeleteResponse deleteResponse = response.deleteResponse(); - log.info("Successfully deleted memory container: {} from index", memoryContainerId); + log + .info( + "Delete memory container - Event: CONTAINER_DELETED, Container ID: {}, User: {}, Timestamp: {}", + memoryContainerId, + user != null ? user.getName() : "unknown", + Instant.now() + ); // Delete memory indices if requested // Note: Shared prefix validation already done BEFORE container deletion if (deleteAllMemories || (deleteMemories != null && !deleteMemories.isEmpty())) { MemoryConfiguration configuration = container.getConfiguration(); if (deleteAllMemories) { - deleteAllMemoryIndices(memoryContainerId, configuration, deleteResponse, actionListener); + deleteAllMemoryIndices(memoryContainerId, configuration, user, deleteResponse, actionListener); } else { - deleteSelectiveMemoryIndices(memoryContainerId, configuration, deleteMemories, deleteResponse, actionListener); + deleteSelectiveMemoryIndices( + memoryContainerId, + configuration, + deleteMemories, + user, + deleteResponse, + actionListener + ); } } else { // No index deletion requested @@ -226,7 +245,7 @@ private void handleDeleteResponse( } } catch (Exception e) { log.error("Failed to process container deletion", e); - actionListener.onFailure(e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } } @@ -234,6 +253,7 @@ private void handleDeleteResponse( private void deleteAllMemoryIndices( String memoryContainerId, MemoryConfiguration configuration, + User user, DeleteResponse deleteResponse, ActionListener actionListener ) { @@ -246,7 +266,7 @@ private void deleteAllMemoryIndices( ); log - .info( + .debug( "Attempting to delete all memory indices for container {}: [{}, {}, {}, {}]", memoryContainerId, configuration.getSessionIndexName(), @@ -255,7 +275,17 @@ private void deleteAllMemoryIndices( configuration.getLongMemoryHistoryIndexName() ); memoryContainerHelper.deleteIndex(configuration, deleteIndexRequest, ActionListener.wrap(r -> { - log.info("Successfully deleted all memory indices for container: {}", memoryContainerId); + log + .info( + "Delete memory container - Event: ALL_INDICES_DELETED, Container ID: {}, Indices: [{}, {}, {}, {}], User: {}, Timestamp: {}", + memoryContainerId, + configuration.getSessionIndexName(), + configuration.getWorkingMemoryIndexName(), + configuration.getLongMemoryIndexName(), + configuration.getLongMemoryHistoryIndexName(), + user != null ? user.getName() : "unknown", + Instant.now() + ); actionListener.onResponse(deleteResponse); }, e -> { log @@ -268,7 +298,7 @@ private void deleteAllMemoryIndices( configuration.getLongMemoryHistoryIndexName(), e ); - actionListener.onFailure(e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } @@ -276,6 +306,7 @@ private void deleteSelectiveMemoryIndices( String memoryContainerId, MemoryConfiguration configuration, Set deleteMemories, + User user, DeleteResponse deleteResponse, ActionListener actionListener ) { @@ -313,11 +344,19 @@ private void deleteSelectiveMemoryIndices( DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(indicesToDelete.toArray(new String[0])); memoryContainerHelper.deleteIndex(configuration, deleteIndexRequest, ActionListener.wrap(r -> { - log.info("Successfully deleted selective memory indices [{}] for container: {}", indicesToDelete, memoryContainerId); + log + .info( + "Delete memory container - Event: SELECTIVE_INDICES_DELETED, Container ID: {}, Indices: {}, Memory Types: {}, User: {}, Timestamp: {}", + memoryContainerId, + indicesToDelete, + deleteMemories, + user != null ? user.getName() : "unknown", + Instant.now() + ); actionListener.onResponse(deleteResponse); }, e -> { log.error("Failed to delete selective memory indices [{}] for container: {}.", memoryContainerId, indicesToDelete, e); - actionListener.onFailure(e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } else { log.info("No valid memory indices to delete for container: {}", memoryContainerId); diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerAction.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerAction.java index 4aea182b79..a87ac9bbd0 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerAction.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerAction.java @@ -117,7 +117,7 @@ protected void doExecute(Task task, ActionRequest request, ActionListener handleResponse(r, throwable, memoryContainerId, tenantId, user, wrappedListener)); } catch (Exception e) { log.error("Failed to get ML memory container {}", memoryContainerId, e); - actionListener.onFailure(e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } @@ -148,7 +148,7 @@ private void handleThrowable( wrappedListener.onFailure(new OpenSearchStatusException("Failed to find memory container index", RestStatus.NOT_FOUND)); } else { log.error("Failed to get ML memory container {}", memoryContainerId, cause); - wrappedListener.onFailure(cause); + wrappedListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerAction.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerAction.java index fcdc763060..72ad3e32de 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerAction.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerAction.java @@ -188,14 +188,17 @@ protected void doExecute(Task task, ActionRequest request, ActionListener { + log.error("Failed to get memory container for update", e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); + })); } /** @@ -263,7 +266,7 @@ private void performUpdate( client.update(updateRequest, ActionListener.runBefore(listener, context::restore)); } catch (Exception e) { log.error("Failed to update memory container {}", memoryContainerId, e); - listener.onFailure(e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } @@ -341,8 +344,8 @@ private void createHistoryIndexOnly( updateFields.put(LAST_UPDATED_TIME_FIELD, Instant.now().toEpochMilli()); performUpdate(ML_MEMORY_CONTAINER_INDEX, memoryContainerId, updateFields, listener); }, e -> { - log.error("Failed to create history index '{}': {}", historyIndexName, e.getMessage(), e); - listener.onFailure(new IllegalStateException("Failed to create history index: " + e.getMessage())); + log.error("Failed to create history index '{}'", historyIndexName, e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } else { updateFields.put(MEMORY_STORAGE_CONFIG_FIELD, config); @@ -373,8 +376,8 @@ private void createLongTermAndHistoryIndices( updateFields.put(LAST_UPDATED_TIME_FIELD, Instant.now().toEpochMilli()); performUpdate(ML_MEMORY_CONTAINER_INDEX, memoryContainerId, updateFields, listener); }, e -> { - log.error("Failed to create history index '{}': {}", historyIndexName, e.getMessage(), e); - listener.onFailure(new IllegalStateException("Failed to create history index: " + e.getMessage())); + log.error("Failed to create history index '{}'", historyIndexName, e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } else { updateFields.put(MEMORY_STORAGE_CONFIG_FIELD, config); @@ -382,8 +385,8 @@ private void createLongTermAndHistoryIndices( performUpdate(ML_MEMORY_CONTAINER_INDEX, memoryContainerId, updateFields, listener); } }, e -> { - log.error("Failed to create long-term index '{}': {}", longTermIndexName, e.getMessage(), e); - listener.onFailure(new IllegalStateException("Failed to create long-term index: " + e.getMessage())); + log.error("Failed to create long-term index '{}'", longTermIndexName, e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingService.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingService.java index 8ce6a2e14a..326d10f46e 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingService.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingService.java @@ -26,9 +26,10 @@ import java.util.List; import java.util.Map; -import org.opensearch.OpenSearchException; +import org.opensearch.OpenSearchStatusException; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.core.action.ActionListener; +import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; @@ -55,6 +56,7 @@ import org.opensearch.transport.client.Client; import com.jayway.jsonpath.JsonPath; +import com.jayway.jsonpath.PathNotFoundException; import lombok.extern.log4j.Log4j2; @@ -177,11 +179,11 @@ public void extractFactsFromConversation( listener.onResponse(facts); } catch (Exception e) { log.error("Failed to parse facts from LLM response", e); - listener.onFailure(new IllegalArgumentException("Failed to parse facts from LLM response", e)); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } }, e -> { log.error("Failed to call LLM for fact extraction", e); - listener.onFailure(new OpenSearchException("Failed to extract facts using LLM model: " + e.getMessage(), e)); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } @@ -262,15 +264,15 @@ public void makeMemoryDecisions( listener.onResponse(decisions); } catch (Exception e) { log.error("Failed to parse memory decisions from LLM response", e); - listener.onFailure(e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } }, e -> { log.error("Failed to get memory decisions from LLM", e); - listener.onFailure(e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } catch (Exception e) { log.error("Failed to build memory decision request", e); - listener.onFailure(e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } @@ -297,31 +299,44 @@ private List parseFactsFromLLMResponse(MemoryStrategy strategy, MemoryCo for (int i = 0; i < modelTensors.getMlModelTensors().size(); i++) { Map dataMap = modelTensors.getMlModelTensors().get(i).getDataAsMap(); String llmResultPath = memoryContainerHelper.getLlmResultPath(strategy, memoryConfig); - Object filterdResult = JsonPath.read(dataMap, llmResultPath); - String llmResult = null; - if (filterdResult != null) { - llmResult = StringUtils.toJson(filterdResult); - } - if (llmResult != null) { - llmResult = StringUtils.toJson(extractJsonProcessorChain.process(llmResult)); - try (XContentParser parser = jsonXContent.createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, llmResult)) { - ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); - while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String fieldName = parser.currentName(); - if ("facts".equals(fieldName)) { - ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.nextToken(), parser); - while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - String fact = parser.text(); - facts.add(fact); + try { + Object filterdResult = JsonPath.read(dataMap, llmResultPath); + String llmResult = null; + if (filterdResult != null) { + llmResult = StringUtils.toJson(filterdResult); + } + if (llmResult != null) { + llmResult = StringUtils.toJson(extractJsonProcessorChain.process(llmResult)); + try ( + XContentParser parser = jsonXContent.createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, llmResult) + ) { + ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); + while (parser.nextToken() != XContentParser.Token.END_OBJECT) { + String fieldName = parser.currentName(); + if ("facts".equals(fieldName)) { + ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.nextToken(), parser); + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { + String fact = parser.text(); + facts.add(fact); + } + } else { + parser.skipChildren(); } - } else { - parser.skipChildren(); } + } catch (IOException e) { + log.error("Failed to extract content from dataMap", e); + throw new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR); } - } catch (IOException e) { - log.error("Failed to extract content from dataMap", e); - throw new IllegalArgumentException("Failed to extract content from LLM response", e); } + } catch (PathNotFoundException e) { + String reason = extractFirstSentence(e.getMessage()); + log.error("Failed to extract LLM result using path {}: {}", llmResultPath, reason); + throw new OpenSearchStatusException( + "LLM predict result cannot be extracted with current llm_result_path with reason: " + + reason + + ". Please check either your llm configuration or your llm_result_path setting in memory container configuration", + RestStatus.NOT_FOUND + ); } } @@ -332,47 +347,64 @@ private List parseMemoryDecisions(String llmResultPath, MLTaskRe try { MLOutput mlOutput = response.getOutput(); if (!(mlOutput instanceof ModelTensorOutput)) { - throw new IllegalStateException("Expected ModelTensorOutput but got: " + mlOutput.getClass().getSimpleName()); + log.error("Expected ModelTensorOutput but got: {}", mlOutput.getClass().getSimpleName()); + throw new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR); } ModelTensorOutput tensorOutput = (ModelTensorOutput) mlOutput; List tensors = tensorOutput.getMlModelOutputs(); if (tensors == null || tensors.isEmpty()) { - throw new IllegalStateException("No model output tensors found"); + log.error("No model output tensors found in LLM response"); + throw new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR); } Map dataAsMap = tensors.get(0).getMlModelTensors().get(0).getDataAsMap(); - Object filterdResult = JsonPath.read(dataAsMap, llmResultPath); - if (filterdResult == null) { - throw new IllegalStateException("No response content found in LLM output"); - } - String responseContent = StringUtils.toJson(filterdResult); + try { + Object filterdResult = JsonPath.read(dataAsMap, llmResultPath); + if (filterdResult == null) { + log.error("No response content found in LLM output"); + throw new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR); + } + String responseContent = StringUtils.toJson(filterdResult); - // Clean response content - responseContent = StringUtils.toJson(extractJsonProcessorChain.process(responseContent)); + // Clean response content + responseContent = StringUtils.toJson(extractJsonProcessorChain.process(responseContent)); - List decisions = new ArrayList<>(); - try (XContentParser parser = jsonXContent.createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, responseContent)) { - ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); + List decisions = new ArrayList<>(); + try ( + XContentParser parser = jsonXContent.createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, responseContent) + ) { + ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); - while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String fieldName = parser.currentName(); - parser.nextToken(); + while (parser.nextToken() != XContentParser.Token.END_OBJECT) { + String fieldName = parser.currentName(); + parser.nextToken(); - if (MEMORY_DECISION_FIELD.equals(fieldName)) { - ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.currentToken(), parser); - while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - decisions.add(MemoryDecision.parse(parser)); + if (MEMORY_DECISION_FIELD.equals(fieldName)) { + ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.currentToken(), parser); + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { + decisions.add(MemoryDecision.parse(parser)); + } + } else { + parser.skipChildren(); } - } else { - parser.skipChildren(); } } - } - return decisions; + return decisions; + } catch (PathNotFoundException e) { + String reason = extractFirstSentence(e.getMessage()); + log.error("Failed to extract LLM result using path {}: {}", llmResultPath, reason); + throw new OpenSearchStatusException( + "LLM predict result cannot be extracted with current llm_result_path with reason: " + + reason + + ". Please check either your llm configuration or your llm_result_path setting in memory container configuration", + RestStatus.NOT_FOUND + ); + } } catch (Exception e) { - throw new RuntimeException("Failed to parse memory decisions", e); + log.error("Failed to parse memory decisions", e); + throw new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR); } } @@ -411,26 +443,38 @@ public void summarizeMessages(MemoryConfiguration configuration, List { log.error("Failed to get memory decisions from LLM", e); - listener.onFailure(e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } catch (Exception e) { - listener.onFailure(e); + log.error("Failed to build summarization request", e); + listener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } } private String parseSessionSummary(ModelTensorOutput output, String llmResultPath) { Map dataAsMap = output.getMlModelOutputs().get(0).getMlModelTensors().get(0).getDataAsMap(); - Object filterdResult = JsonPath.read(dataAsMap, llmResultPath); - String sessionSummary = null; - if (filterdResult != null) { - sessionSummary = StringUtils.toJson(filterdResult); + try { + Object filterdResult = JsonPath.read(dataAsMap, llmResultPath); + String sessionSummary = null; + if (filterdResult != null) { + sessionSummary = StringUtils.toJson(filterdResult); + } + return sessionSummary; + } catch (PathNotFoundException e) { + String reason = extractFirstSentence(e.getMessage()); + log.error("Failed to extract LLM result using path {}: {}", llmResultPath, reason); + throw new OpenSearchStatusException( + "LLM predict result cannot be extracted with current llm_result_path with reason: " + + reason + + ". Please check either your llm configuration or your llm_result_path setting in memory container configuration", + RestStatus.NOT_FOUND + ); } - return sessionSummary; } private boolean validatePromptFormat(String prompt) { @@ -469,4 +513,25 @@ private String getEffectiveLlmId(MemoryStrategy strategy, MemoryConfiguration me // Fall back to memory config return memoryConfig != null ? memoryConfig.getLlmId() : null; } + + /** + * Extracts the first sentence from an exception message without the trailing period. + * Uses ". " (period + space) as the sentence boundary to avoid splitting on periods + * within JSON paths like "$.data.output". + * + * @param message The exception message to extract from + * @return The first sentence without trailing period, or the whole message if no sentence boundary found + */ + private String extractFirstSentence(String message) { + if (message == null) { + return ""; + } + // Look for ". " (period + space) which indicates sentence boundary + // This avoids splitting on periods within JSON paths like "$.data.output" + int sentenceEnd = message.indexOf(". "); + if (sentenceEnd >= 0) { + return message.substring(0, sentenceEnd); // Exclude the period + } + return message; // No sentence boundary found, use whole message + } } diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemorySearchService.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemorySearchService.java index 3b28e7c645..0e7b690a26 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemorySearchService.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemorySearchService.java @@ -77,7 +77,7 @@ private void searchFactsSequentially( QueryBuilder queryBuilder = MemorySearchQueryBuilder .buildFactSearchQuery(strategy, fact, input.getNamespace(), input.getOwnerId(), memoryConfig, input.getMemoryContainerId()); - log.debug("Searching for similar facts with query: {}", queryBuilder.toString()); + log.debug("Searching for similar facts"); SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder(); searchSourceBuilder.query(queryBuilder); @@ -103,16 +103,16 @@ private void searchFactsSequentially( } } - log.debug("Found {} similar facts for: {}", response.getHits().getHits().length, fact); + log.debug("Found {} similar facts", response.getHits().getHits().length); searchFactsSequentially(strategy, input, facts, currentIndex + 1, memoryConfig, maxInferSize, allResults, listener); }, e -> { - log.error("Failed to search for similar facts for: {}", fact, e); + log.error("Failed to search for similar facts"); searchFactsSequentially(strategy, input, facts, currentIndex + 1, memoryConfig, maxInferSize, allResults, listener); }); memoryContainerHelper.searchData(memoryConfig, searchRequest, searchResponseActionListener); } catch (Exception e) { - log.error("Failed to build search query for fact: {}", fact, e); + log.error("Failed to build search query for facts"); searchFactsSequentially(strategy, input, facts, currentIndex + 1, memoryConfig, maxInferSize, allResults, listener); } } diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesAction.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesAction.java index 0b354086d9..f5753a61ce 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesAction.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesAction.java @@ -24,7 +24,6 @@ import java.util.Map; import org.apache.commons.lang3.StringUtils; -import org.opensearch.OpenSearchException; import org.opensearch.OpenSearchStatusException; import org.opensearch.action.index.IndexRequest; import org.opensearch.action.index.IndexResponse; @@ -181,16 +180,23 @@ private void createNewSessionIfAbsent( ActionListener responseActionListener = ActionListener.wrap(r -> { input.getNamespace().put(SESSION_ID_FIELD, r.getId()); processAndIndexMemory(input, container, user, actionListener); - }, e -> actionListener.onFailure(e)); + }, e -> { + log.error("Failed to index session data", e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); + }); memoryContainerHelper.indexData(configuration, indexRequest, responseActionListener); - }, exception -> actionListener.onFailure(exception)); + }, exception -> { + log.error("Failed to summarize messages for session creation", exception); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); + }); memoryProcessingService.summarizeMessages(container.getConfiguration(), messages, summaryListener); } else { processAndIndexMemory(input, container, user, actionListener); } } catch (Exception e) { - actionListener.onFailure(e); + log.error("Failed to create session", e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } @@ -227,14 +233,9 @@ private void processAndIndexMemory( if (infer) { threadPool.executor(AGENTIC_MEMORY_THREAD_POOL).execute(() -> { try { - extractLongTermMemory( - input, - container, - user, - ActionListener.wrap(res -> { log.debug("Long term memory results: {}", res.toString()); }, e -> { - log.error("Failed to extract longTermMemory id from memory container", e); - }) - ); + extractLongTermMemory(input, container, user, ActionListener.wrap(res -> {}, e -> { + log.error("Failed to extract longTermMemory id from memory container", e); + })); } catch (Exception e) { memoryOperationsService.writeErrorToMemoryHistory(memoryConfig, null, input, e); } @@ -244,7 +245,7 @@ private void processAndIndexMemory( memoryContainerHelper.indexData(memoryConfig, indexRequest, responseActionListener); } catch (Exception e) { log.error("Failed to add memory", e); - actionListener.onFailure(e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } @@ -260,8 +261,8 @@ private IndexRequest createWorkingMemoryRequest(String workingMemoryIndex, MLAdd indexRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); return indexRequest; } catch (IOException e) { - logger.error("Failed to build index request source", e); - throw new RuntimeException("Failed to build index request", e); + log.error("Failed to build index request source", e); + throw new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR); } } @@ -288,7 +289,7 @@ private void extractLongTermMemory( }, e -> { log.error("Failed to extract facts with LLM", e); memoryOperationsService.writeErrorToMemoryHistory(memoryConfig, strategyNameSpace, input, e); - actionListener.onFailure(new OpenSearchException("Failed to extract facts: " + e.getMessage(), e)); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } } @@ -343,7 +344,8 @@ private void storeLongTermMemory( ); }, e -> { log.error("Failed to make memory decisions", e); - actionListener.onFailure(new OpenSearchException("Failed to make memory decisions: " + e.getMessage(), e)); + actionListener + .onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } else { List decisions = new ArrayList<>(); @@ -367,7 +369,7 @@ private void storeLongTermMemory( } }, e -> { log.error("Failed to search similar facts", e); - actionListener.onFailure(new OpenSearchException("Failed to search similar facts: " + e.getMessage(), e)); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); })); } else { // No memory decisions needed diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoriesByQueryAction.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoriesByQueryAction.java index d4dd011d26..828652c96f 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoriesByQueryAction.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoriesByQueryAction.java @@ -7,6 +7,8 @@ import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_AGENTIC_MEMORY_DISABLED_MESSAGE; +import java.time.Instant; + import org.opensearch.OpenSearchStatusException; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; @@ -128,7 +130,7 @@ protected void doExecute( deleteByQueryRequest.setRefresh(true); // Step 9: Execute the delete by query - executeDeleteByQuery(container.getConfiguration(), deleteByQueryRequest, actionListener); + executeDeleteByQuery(memoryContainerId, memoryType, user, container.getConfiguration(), deleteByQueryRequest, actionListener); }, error -> { log.error("Failed to get memory container: " + memoryContainerId, error); @@ -164,6 +166,9 @@ private String getMemoryIndexName(MLMemoryContainer container, MemoryType memory * Execute the delete by query request, handling system indices appropriately */ private void executeDeleteByQuery( + String memoryContainerId, + MemoryType memoryType, + User user, MemoryConfiguration configuration, DeleteByQueryRequest deleteByQueryRequest, ActionListener actionListener @@ -181,7 +186,16 @@ private void executeDeleteByQuery( log.warn("Delete by query operation timed out"); } - log.info("Delete by query completed. Deleted {} documents in {} ms", response.getDeleted(), response.getTook().millis()); + log + .info( + "Delete memories by query - Event: MEMORIES_DELETED_BY_QUERY, Container ID: {}, Memory Type: {}, Deleted Count: {}, Duration: {}ms, User: {}, Timestamp: {}", + memoryContainerId, + memoryType, + response.getDeleted(), + response.getTook().millis(), + user != null ? user.getName() : "unknown", + Instant.now() + ); // Wrap the BulkByScrollResponse in our response wrapper actionListener.onResponse(new MLDeleteMemoriesByQueryResponse(response)); }, error -> { diff --git a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoryAction.java b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoryAction.java index 56dc5d4759..304d38ad92 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoryAction.java +++ b/plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/TransportDeleteMemoryAction.java @@ -7,6 +7,8 @@ import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.OWNER_ID_FIELD; +import java.time.Instant; + import org.opensearch.OpenSearchStatusException; import org.opensearch.action.ActionRequest; import org.opensearch.action.delete.DeleteRequest; @@ -121,6 +123,17 @@ protected void doExecute(Task task, ActionRequest request, ActionListener { MLCreateSessionResponse response = MLCreateSessionResponse.builder().sessionId(r.getId()).status("created").build(); actionListener.onResponse(response); - }, e -> { actionListener.onFailure(e); })); + }, e -> { + log.error("Failed to create session in container {}", input.getMemoryContainerId(), e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); + })); } catch (IOException e) { - actionListener.onFailure(e); + log.error("Failed to build XContent for session in container {}", input.getMemoryContainerId(), e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); } } diff --git a/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerPipelineHelper.java b/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerPipelineHelper.java index 3769dfb47d..73765164d4 100644 --- a/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerPipelineHelper.java +++ b/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerPipelineHelper.java @@ -63,14 +63,26 @@ public static void createLongTermMemoryIngestPipeline( indicesHandler.createLongTermMemoryIndex(pipelineName, indexName, config, listener); }, e -> { log.error("Failed to create text embedding pipeline '{}'", pipelineName, e); - listener.onFailure(e); + listener + .onFailure( + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR + ) + ); })); } else { indicesHandler.createLongTermMemoryIndex(null, indexName, config, listener); } } catch (Exception e) { - log.error("Failed to create long-term memory infrastructure for index: {}", indexName, e); - listener.onFailure(e); + log.error("Failed to create text embedding pipeline for long term memory index: {}", indexName, e); + listener + .onFailure( + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR + ) + ); } } @@ -105,7 +117,13 @@ public static void createTextEmbeddingPipeline( createPipelineInternal(pipelineName, config, client, listener); } catch (IOException e) { log.error("Failed to build pipeline configuration for '{}'", pipelineName, e); - listener.onFailure(e); + listener + .onFailure( + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR + ) + ); } }, error -> { // Pipeline doesn't exist (404 error expected) - create it @@ -113,7 +131,13 @@ public static void createTextEmbeddingPipeline( createPipelineInternal(pipelineName, config, client, listener); } catch (IOException e) { log.error("Failed to build pipeline configuration for '{}'", pipelineName, e); - listener.onFailure(e); + listener + .onFailure( + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR + ) + ); } })); } @@ -159,11 +183,23 @@ private static void createPipelineInternal( listener.onResponse(true); } else { log.error("Pipeline creation not acknowledged: {}", pipelineName); - listener.onFailure(new IllegalStateException("Pipeline creation not acknowledged")); + listener + .onFailure( + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR + ) + ); } }, e -> { log.error("Failed to create pipeline '{}'", pipelineName, e); - listener.onFailure(e); + listener + .onFailure( + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR + ) + ); })); } diff --git a/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerSharedIndexValidator.java b/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerSharedIndexValidator.java index 7f8d1effad..79b58a2e81 100644 --- a/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerSharedIndexValidator.java +++ b/plugin/src/main/java/org/opensearch/ml/helper/MemoryContainerSharedIndexValidator.java @@ -111,11 +111,9 @@ private static void validateExistingIndex( log.error("Index '{}' exists but mapping metadata is null", longTermIndexName); listener .onFailure( - new IllegalStateException( - "Cannot validate memory container: Index '" - + longTermIndexName - + "' exists but mapping is unavailable. " - + "This indicates a system issue. Please contact your administrator." + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR ) ); return; @@ -129,10 +127,9 @@ private static void validateExistingIndex( log.error("Index '{}' mapping 'properties' field is null or not a Map", longTermIndexName); listener .onFailure( - new IllegalStateException( - "Cannot validate memory container: Index '" - + longTermIndexName - + "' has malformed mapping structure (properties field is missing or invalid)." + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR ) ); return; @@ -305,11 +302,9 @@ private static void handleMappingError(String indexName, Exception error, Action listener .onFailure( - new IllegalStateException( - "Cannot validate memory container: Failed to retrieve index mapping for '" - + indexName - + "'. Error: " - + error.getMessage() + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR ) ); } @@ -318,15 +313,13 @@ private static void handleMappingError(String indexName, Exception error, Action * Handles errors during pipeline retrieval. */ private static void handlePipelineError(String pipelineName, Exception error, ActionListener listener) { - log.error("Failed to retrieve pipeline '{}' for validation: {}", pipelineName, error.getMessage(), error); + log.error("Failed to retrieve pipeline '{}' for validation", pipelineName, error); listener .onFailure( - new IllegalStateException( - "Cannot validate memory container: Failed to retrieve ingest pipeline '" - + pipelineName - + "' for validation. Error: " - + error.getMessage() + new org.opensearch.OpenSearchStatusException( + "Internal server error", + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR ) ); } diff --git a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerActionTests.java b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerActionTests.java index 9d6c52adb8..01fc0cd3a7 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerActionTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerActionTests.java @@ -417,7 +417,9 @@ public void testDoExecuteWithIndexInitializationFailure() throws InterruptedExce verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); assertNotNull(exception); - assertTrue(exception.getMessage().contains("Index initialization failed")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testAutoGeneratedPrefixPersistence() throws InterruptedException { @@ -497,7 +499,9 @@ public void testDoExecuteWithMemoryContainerIndexingFailure() throws Interrupted verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); assertNotNull(exception); - assertTrue(exception.getMessage().contains("Indexing failed")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testDoExecuteWithMemorySessionIndexCreationFailure() throws InterruptedException { @@ -613,7 +617,9 @@ public void testDoExecuteWithIndexMemoryContainerThrowableFailure() throws Inter verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); assertNotNull(exception); - assertTrue(exception.getMessage().contains("Index throwable failure")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testDoExecuteWithIndexResponseExceptionInTryBlock() throws InterruptedException { @@ -636,7 +642,9 @@ public void testDoExecuteWithIndexResponseExceptionInTryBlock() throws Interrupt verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); assertNotNull(exception); - assertTrue(exception.getMessage().contains("IndexResponse access exception")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testDoExecuteWithInitMemoryContainerIndexCatchException() throws InterruptedException { @@ -1195,8 +1203,9 @@ public void testCreateContainer_PutPipelineFailure() throws InterruptedException ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof RuntimeException); - assertTrue(captor.getValue().getMessage().contains("Pipeline creation failed")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } @Test @@ -1268,8 +1277,9 @@ public void testCreateContainer_IndexResponseNotCreated() throws InterruptedExce ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof RuntimeException); - assertTrue(captor.getValue().getMessage().contains("Failed to create memory container")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } @Test @@ -1630,8 +1640,9 @@ public void testCreateContainer_SharedIndexConfigMismatch() throws InterruptedEx ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(captor.capture()); + // Validation errors for config mismatch should remain as IllegalArgumentException (4XX) assertTrue(captor.getValue() instanceof IllegalArgumentException); - assertTrue(captor.getValue().getMessage().contains("Embedding configuration conflicts")); + assertTrue(captor.getValue().getMessage().toLowerCase().contains("embedding configuration")); } @Test @@ -1662,8 +1673,9 @@ public void testCreateContainer_IndexExistsMappingNull() throws InterruptedExcep ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof IllegalStateException); - assertTrue(captor.getValue().getMessage().contains("mapping is unavailable")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } @Test @@ -1876,8 +1888,9 @@ public void testCreateContainer_GetPipelineValidationError() throws InterruptedE ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof IllegalStateException); - assertTrue(captor.getValue().getMessage().contains("Failed to retrieve ingest pipeline")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } @Test @@ -1901,8 +1914,9 @@ public void testCreateContainer_GetMappingsValidationError() throws InterruptedE ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof IllegalStateException); - assertTrue(captor.getValue().getMessage().contains("Failed to retrieve index mapping")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } // Helper method to mock successful LLM validation (without embedding validation which will be tested) diff --git a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportDeleteMemoryContainerActionTests.java b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportDeleteMemoryContainerActionTests.java index 8fc883f078..f3bcc4984f 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportDeleteMemoryContainerActionTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportDeleteMemoryContainerActionTests.java @@ -245,8 +245,9 @@ public void testDeleteMemoryContainer_IndexNotFoundException() { verify(actionListener).onFailure(argumentCaptor.capture()); Exception exception = argumentCaptor.getValue(); - assertTrue(exception instanceof IndexNotFoundException); - assertTrue(exception.getMessage().contains("Memory container index not found")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testDeleteMemoryContainer_MultiTenancyEnabled_ValidTenantId() { @@ -491,7 +492,9 @@ public void testHandleDeleteResponse_WithDeleteAllMemories_DeleteIndexFailure() ArgumentCaptor exceptionCaptor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); - assertEquals("Failed to delete memory indices", exception.getMessage()); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } @Test @@ -568,7 +571,9 @@ public void testHandleDeleteResponse_WithDeleteAllMemories_NullConfiguration() { ArgumentCaptor exceptionCaptor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); - assertTrue(exception instanceof NullPointerException || exception.getMessage().contains("configuration")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } @Test @@ -855,7 +860,9 @@ public void testDeleteMemoryContainer_CountCheckFailure_AbortsOperation() { ArgumentCaptor exceptionCaptor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); - assertTrue(exception.getMessage().contains("Failed to count containers")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } @Test diff --git a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerActionTests.java b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerActionTests.java index 77894731db..7d8589889d 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerActionTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportGetMemoryContainerActionTests.java @@ -516,9 +516,15 @@ public void testDoExecuteWithGeneralAsyncException() { // Execute action.doExecute(task, getRequest, actionListener); - // Verify failure response with the original exception + // Verify failure response with security-hardened exception verify(actionListener, timeout(1000)) - .onFailure(argThat(exception -> exception instanceof RuntimeException && exception.getMessage().equals("General async error"))); + .onFailure( + argThat( + exception -> exception instanceof OpenSearchStatusException + && ((OpenSearchStatusException) exception).status() == RestStatus.INTERNAL_SERVER_ERROR + && exception.getMessage().contains("Internal server error") + ) + ); } public void testDoExecuteWithNonExistentMemoryContainer() { diff --git a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerActionTests.java b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerActionTests.java index b1bed5435b..6cdaa57f4e 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerActionTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerActionTests.java @@ -223,8 +223,9 @@ public void testDoExecuteWhenGetContainerFails() { verify(listener).onFailure(captor.capture()); Exception exception = captor.getValue(); - assertTrue(exception instanceof RuntimeException); - assertEquals("Container not found", exception.getMessage()); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testDoExecuteWhenAccessDenied() { @@ -566,7 +567,8 @@ public void testUpdateStrategies_InvalidStrategyId() { Exception exception = captor.getValue(); assertTrue(exception instanceof OpenSearchStatusException); - assertEquals(RestStatus.NOT_FOUND, ((OpenSearchStatusException) exception).status()); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testUpdateStrategies_CannotChangeType() { @@ -642,8 +644,9 @@ public void testUpdateStrategies_CannotChangeType() { verify(listener).onFailure(captor.capture()); Exception exception = captor.getValue(); - assertTrue(exception instanceof IllegalArgumentException); - assertTrue(exception.getMessage().contains("Cannot change strategy type")); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testUpdateStrategies_PartialFieldUpdate() { @@ -1003,8 +1006,9 @@ public void testUpdateContainer_AddStrategyWithoutModels_ShouldFail() { ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(listener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof IllegalArgumentException); - assertTrue(captor.getValue().getMessage().contains("Strategies require both an LLM model and embedding model")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } public void testUpdateContainer_AddStrategyWithOnlyLlm_ShouldFail() { @@ -1047,8 +1051,9 @@ public void testUpdateContainer_AddStrategyWithOnlyLlm_ShouldFail() { ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(listener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof IllegalArgumentException); - assertTrue(captor.getValue().getMessage().contains("Missing: embedding model")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } public void testUpdateContainer_ChangeEmbeddingModel_ShouldFail() { @@ -1105,8 +1110,9 @@ public void testUpdateContainer_ChangeEmbeddingModel_ShouldFail() { ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(listener).onFailure(captor.capture()); - assertTrue(captor.getValue() instanceof IllegalArgumentException); - assertTrue(captor.getValue().getMessage().contains("Cannot change embedding configuration once strategies are configured")); + assertTrue(captor.getValue() instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) captor.getValue()).status()); + assertTrue(captor.getValue().getMessage().contains("Internal server error")); } public void testUpdateContainer_SameEmbeddingValues_ShouldSucceed() { diff --git a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceAdditionalTests.java b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceAdditionalTests.java index bfc084cd11..2522d5df97 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceAdditionalTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceAdditionalTests.java @@ -415,4 +415,69 @@ public void testExtractFactsFromConversation_WithProcessorChainComplexJsonStruct // Should extract only the facts array, ignoring other fields verify(factsListener).onResponse(any(List.class)); } + + @Test + public void testJsonPathRead_MissingProperty() { + // Test PathNotFoundException when property doesn't exist - message has no period + Map dataMap = new HashMap<>(); + Map contentItem = new HashMap<>(); + contentItem.put("text", "some value"); + dataMap.put("content", Arrays.asList(contentItem)); + + try { + // Try to read a path that doesn't exist + Object result = com.jayway.jsonpath.JsonPath.read(dataMap, "$.nonexistent"); + throw new AssertionError("Expected PathNotFoundException but got result: " + result); + } catch (com.jayway.jsonpath.PathNotFoundException e) { + // Verify the exception message format + String message = e.getMessage(); + assertEquals("No results for path: $['nonexistent']", message); + + // Verify this is the expected exception type + assertEquals(com.jayway.jsonpath.PathNotFoundException.class, e.getClass()); + } + } + + @Test + public void testJsonPathRead_InvalidFilterOnNonArray() { + // Test PathNotFoundException when filter is applied to non-array - message has period + Map dataMap = new HashMap<>(); + Map id = new HashMap<>(); + id.put("type", "message"); + id.put("role", "assistant"); + Map contentItem = new HashMap<>(); + contentItem.put("type", "text"); + contentItem.put("text", "Alice from California likes running; AI offers well-wishes."); + id.put("content", Arrays.asList(contentItem)); + dataMap.put("id", "msg_bdrk_01NDDutcnm9pEsE2AMQ5PodR"); + dataMap.put("type", "message"); + dataMap.put("role", "assistant"); + dataMap.put("model", "claude-3-7-sonnet-20250219"); + dataMap.put("content", Arrays.asList(contentItem)); + dataMap.put("stop_reason", "end_turn"); + dataMap.put("stop_sequence", null); + Map usage = new HashMap<>(); + usage.put("input_tokens", 86.0); + usage.put("cache_creation_input_tokens", 0.0); + usage.put("cache_read_input_tokens", 0.0); + usage.put("output_tokens", 17.0); + dataMap.put("usage", usage); + + try { + // Try to apply array filter to string - this will fail with detailed error + Object result = com.jayway.jsonpath.JsonPath.read(dataMap, "$.content[0].text"); + // If we successfully got content[0].text, we need a different test case + // Let's try a path that will actually fail + result = com.jayway.jsonpath.JsonPath.read(dataMap, "$.type[0].text"); + throw new AssertionError("Expected PathNotFoundException but got result: " + result); + } catch (com.jayway.jsonpath.PathNotFoundException e) { + // Verify the exception message contains period and has detailed context + String message = e.getMessage(); + // The message should contain a period indicating sentence structure + org.junit.Assert.assertTrue("Message should contain details: " + message, message.length() > 0); + + // Verify this is the expected exception type + assertEquals(com.jayway.jsonpath.PathNotFoundException.class, e.getClass()); + } + } } diff --git a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceTests.java b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceTests.java index 70887b2fbe..add077999e 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryProcessingServiceTests.java @@ -10,6 +10,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -31,7 +32,9 @@ import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.opensearch.OpenSearchStatusException; import org.opensearch.core.action.ActionListener; +import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.ml.common.dataset.remote.RemoteInferenceInputDataSet; import org.opensearch.ml.common.memorycontainer.MemoryConfiguration; @@ -303,7 +306,14 @@ public void testExtractFactsFromConversation_ParseException() { memoryProcessingService.extractFactsFromConversation(messages, memoryStrategy, storageConfig, factsListener); - verify(factsListener).onFailure(any(IllegalArgumentException.class)); + verify(factsListener) + .onFailure( + argThat( + exception -> exception instanceof OpenSearchStatusException + && ((OpenSearchStatusException) exception).status() == RestStatus.INTERNAL_SERVER_ERROR + && exception.getMessage().contains("Internal server error") + ) + ); } @Test diff --git a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesActionTests.java b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesActionTests.java index 93e87cfac1..0ba3524d8b 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesActionTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/TransportAddMemoriesActionTests.java @@ -6,6 +6,7 @@ package org.opensearch.ml.action.memorycontainer.memory; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.doAnswer; @@ -35,6 +36,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.core.action.ActionListener; +import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.ml.common.memorycontainer.MLMemoryContainer; import org.opensearch.ml.common.memorycontainer.MemoryConfiguration; @@ -595,7 +597,9 @@ public void testDoExecute_SessionCreation_SummarizeFailure() { // Verify summarize was called and failure was propagated verify(memoryProcessingService).summarizeMessages(eq(config), eq(messages), any()); - verify(actionListener).onFailure(summarizeException); + verify(actionListener).onFailure(argThat(exception -> exception instanceof OpenSearchStatusException + && ((OpenSearchStatusException)exception).status() == RestStatus.INTERNAL_SERVER_ERROR + && exception.getMessage().contains("Internal server error"))); } @Test diff --git a/plugin/src/test/java/org/opensearch/ml/action/session/TransportCreateSessionActionTests.java b/plugin/src/test/java/org/opensearch/ml/action/session/TransportCreateSessionActionTests.java index 9303ae9178..84236b1996 100644 --- a/plugin/src/test/java/org/opensearch/ml/action/session/TransportCreateSessionActionTests.java +++ b/plugin/src/test/java/org/opensearch/ml/action/session/TransportCreateSessionActionTests.java @@ -380,8 +380,9 @@ public void testDoExecute_IndexingFailure() { verify(actionListener).onFailure(exceptionCaptor.capture()); Exception exception = exceptionCaptor.getValue(); - assertTrue(exception instanceof IOException); - assertEquals("Indexing failed", exception.getMessage()); + assertTrue(exception instanceof OpenSearchStatusException); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, ((OpenSearchStatusException) exception).status()); + assertTrue(exception.getMessage().contains("Internal server error")); } @Test diff --git a/plugin/src/test/java/org/opensearch/ml/helper/MemoryContainerPipelineHelperTests.java b/plugin/src/test/java/org/opensearch/ml/helper/MemoryContainerPipelineHelperTests.java index deda5cffb4..c6a409433d 100644 --- a/plugin/src/test/java/org/opensearch/ml/helper/MemoryContainerPipelineHelperTests.java +++ b/plugin/src/test/java/org/opensearch/ml/helper/MemoryContainerPipelineHelperTests.java @@ -16,12 +16,14 @@ import java.util.concurrent.atomic.AtomicBoolean; import org.junit.Before; +import org.opensearch.OpenSearchStatusException; import org.opensearch.action.ingest.GetPipelineRequest; import org.opensearch.action.ingest.GetPipelineResponse; import org.opensearch.action.ingest.PutPipelineRequest; import org.opensearch.action.support.PlainActionFuture; import org.opensearch.action.support.clustermanager.AcknowledgedResponse; import org.opensearch.core.action.ActionListener; +import org.opensearch.core.rest.RestStatus; import org.opensearch.ml.common.FunctionName; import org.opensearch.ml.common.memorycontainer.MemoryConfiguration; import org.opensearch.ml.engine.indices.MLIndicesHandler; @@ -157,8 +159,9 @@ public void testCreateTextEmbeddingPipelineFailure() { PlainActionFuture future = PlainActionFuture.newFuture(); MemoryContainerPipelineHelper.createTextEmbeddingPipeline("index-embedding", configuration, client, future); - RuntimeException exception = expectThrows(RuntimeException.class, future::actionGet); - assertEquals("put failure", exception.getMessage()); + OpenSearchStatusException exception = expectThrows(OpenSearchStatusException.class, future::actionGet); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, exception.status()); + assertTrue(exception.getMessage().contains("Internal server error")); } public void testCreateHistoryIndexIfEnabled() {