Skip to content

Commit 8d8639c

Browse files
rithinpullelab4sjoo
rithinpullela
and
b4sjoo
authored
fix: wrong error code when deleting memory container (#4723) (#4760)
* fix: wrong error code when deleting memory container * Adress comments --------- (cherry picked from commit 9585180) Signed-off-by: Sicheng Song <sicheng.song@outlook.com> Co-authored-by: Sicheng Song <sicheng.song@outlook.com>
1 parent 19069e7 commit 8d8639c

File tree

8 files changed

+252
-98
lines changed

8 files changed

+252
-98
lines changed

‎common/src/main/java/org/opensearch/ml/common/memorycontainer/MemoryConfiguration.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -352,15 +352,24 @@ public String getFinalMemoryIndexPrefix() {
352352
}
353353
}
354354

355+
/**
356+
* Returns true if long-term memory is disabled, i.e., there is no LLM or no strategies configured.
357+
*/
358+
private boolean isLongTermMemoryDisabled() {
359+
return getLlmId() == null || getStrategies() == null || getStrategies().isEmpty();
360+
}
361+
355362
public String getIndexName(MemoryType memoryType) {
356363
if (memoryType == null) {
357364
return null;
358365
}
359-
// Check if disabled
366+
if (memoryType == MemoryType.LONG_TERM && isLongTermMemoryDisabled()) {
367+
return null;
368+
}
360369
if (memoryType == MemoryType.SESSIONS && isDisableSession()) {
361370
return null;
362371
}
363-
if (memoryType == MemoryType.HISTORY && isDisableHistory()) {
372+
if (memoryType == MemoryType.HISTORY && (isDisableHistory() || isLongTermMemoryDisabled())) {
364373
return null;
365374
}
366375
return getFinalMemoryIndexPrefix() + memoryType.getIndexSuffix();

‎common/src/test/java/org/opensearch/ml/common/memorycontainer/MemoryConfigurationTests.java‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,22 @@
2828
*/
2929
public class MemoryConfigurationTests {
3030

31+
/** Creates a config with long-term memory enabled (llmId + strategies required for LONG_TERM/HISTORY index names). */
32+
private static MemoryConfiguration configWithLongTermMemory(MemoryConfiguration.MemoryConfigurationBuilder builder) {
33+
List<MemoryStrategy> strategies = new ArrayList<>();
34+
strategies
35+
.add(
36+
MemoryStrategy
37+
.builder()
38+
.id("test-id")
39+
.enabled(true)
40+
.type(MemoryStrategyType.SEMANTIC)
41+
.namespace(Arrays.asList("test-namespace"))
42+
.build()
43+
);
44+
return (builder != null ? builder : MemoryConfiguration.builder()).llmId("test-llm").strategies(strategies).build();
45+
}
46+
3147
// ==================== validateStrategiesRequireModels Tests ====================
3248

3349
@Test
@@ -687,7 +703,7 @@ public void testGetIndexName_Working() {
687703

688704
@Test
689705
public void testGetIndexName_LongTerm() {
690-
MemoryConfiguration config = MemoryConfiguration.builder().build();
706+
MemoryConfiguration config = configWithLongTermMemory(null);
691707

692708
String result = config.getIndexName(MemoryType.LONG_TERM);
693709
assertNotNull(result);
@@ -696,7 +712,7 @@ public void testGetIndexName_LongTerm() {
696712

697713
@Test
698714
public void testGetIndexName_HistoryEnabled() {
699-
MemoryConfiguration config = MemoryConfiguration.builder().disableHistory(false).build();
715+
MemoryConfiguration config = configWithLongTermMemory(MemoryConfiguration.builder().disableHistory(false));
700716

701717
String result = config.getIndexName(MemoryType.HISTORY);
702718
assertNotNull(result);
@@ -867,7 +883,7 @@ public void testGetWorkingMemoryIndexName_WithoutSystemIndex() {
867883

868884
@Test
869885
public void testGetLongMemoryIndexName_Default() {
870-
MemoryConfiguration config = MemoryConfiguration.builder().build();
886+
MemoryConfiguration config = configWithLongTermMemory(null);
871887

872888
String result = config.getLongMemoryIndexName();
873889
assertNotNull(result);
@@ -876,7 +892,7 @@ public void testGetLongMemoryIndexName_Default() {
876892

877893
@Test
878894
public void testGetLongMemoryIndexName_WithCustomPrefix() {
879-
MemoryConfiguration config = MemoryConfiguration.builder().indexPrefix("long-term-prefix").build();
895+
MemoryConfiguration config = configWithLongTermMemory(MemoryConfiguration.builder().indexPrefix("long-term-prefix"));
880896

881897
String result = config.getLongMemoryIndexName();
882898
assertNotNull(result);
@@ -886,7 +902,7 @@ public void testGetLongMemoryIndexName_WithCustomPrefix() {
886902

887903
@Test
888904
public void testGetLongMemoryIndexName_WithSystemIndex() {
889-
MemoryConfiguration config = MemoryConfiguration.builder().useSystemIndex(true).build();
905+
MemoryConfiguration config = configWithLongTermMemory(MemoryConfiguration.builder().useSystemIndex(true));
890906

891907
String result = config.getLongMemoryIndexName();
892908
assertNotNull(result);
@@ -898,7 +914,7 @@ public void testGetLongMemoryIndexName_WithSystemIndex() {
898914

899915
@Test
900916
public void testGetLongMemoryHistoryIndexName_Enabled() {
901-
MemoryConfiguration config = MemoryConfiguration.builder().disableHistory(false).build();
917+
MemoryConfiguration config = configWithLongTermMemory(MemoryConfiguration.builder().disableHistory(false));
902918

903919
String result = config.getLongMemoryHistoryIndexName();
904920
assertNotNull(result);
@@ -915,7 +931,9 @@ public void testGetLongMemoryHistoryIndexName_Disabled() {
915931

916932
@Test
917933
public void testGetLongMemoryHistoryIndexName_WithCustomPrefix() {
918-
MemoryConfiguration config = MemoryConfiguration.builder().indexPrefix("history-prefix").disableHistory(false).build();
934+
MemoryConfiguration config = configWithLongTermMemory(
935+
MemoryConfiguration.builder().indexPrefix("history-prefix").disableHistory(false)
936+
);
919937

920938
String result = config.getLongMemoryHistoryIndexName();
921939
assertNotNull(result);

‎plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportDeleteMemoryContainerAction.java‎

Lines changed: 24 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@
1010

1111
import java.time.Instant;
1212
import java.util.ArrayList;
13+
import java.util.EnumSet;
1314
import java.util.List;
1415
import java.util.Set;
1516

17+
import org.opensearch.OpenSearchException;
1618
import org.opensearch.OpenSearchStatusException;
1719
import org.opensearch.action.ActionRequest;
1820
import org.opensearch.action.admin.indices.delete.DeleteIndexRequest;
@@ -157,6 +159,14 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<Delete
157159
);
158160
}
159161
}, error -> {
162+
// Preserve client errors (4XX) with their detailed messages
163+
if (error instanceof OpenSearchException) {
164+
OpenSearchException osException = (OpenSearchException) error;
165+
if (osException.status().getStatus() >= 400 && osException.status().getStatus() < 500) {
166+
actionListener.onFailure(error);
167+
return;
168+
}
169+
}
160170
log.error("Failed to retrieve memory container: {} for deletion", memoryContainerId, error);
161171
actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR));
162172
}));
@@ -227,18 +237,8 @@ private void handleDeleteResponse(
227237
// Note: Shared prefix validation already done BEFORE container deletion
228238
if (deleteAllMemories || (deleteMemories != null && !deleteMemories.isEmpty())) {
229239
MemoryConfiguration configuration = container.getConfiguration();
230-
if (deleteAllMemories) {
231-
deleteAllMemoryIndices(memoryContainerId, configuration, user, deleteResponse, actionListener);
232-
} else {
233-
deleteSelectiveMemoryIndices(
234-
memoryContainerId,
235-
configuration,
236-
deleteMemories,
237-
user,
238-
deleteResponse,
239-
actionListener
240-
);
241-
}
240+
Set<MemoryType> typesToDelete = deleteAllMemories ? EnumSet.allOf(MemoryType.class) : deleteMemories;
241+
deleteSelectiveMemoryIndices(memoryContainerId, configuration, typesToDelete, user, deleteResponse, actionListener);
242242
} else {
243243
// No index deletion requested
244244
actionListener.onResponse(deleteResponse);
@@ -250,58 +250,6 @@ private void handleDeleteResponse(
250250
}
251251
}
252252

253-
private void deleteAllMemoryIndices(
254-
String memoryContainerId,
255-
MemoryConfiguration configuration,
256-
User user,
257-
DeleteResponse deleteResponse,
258-
ActionListener<DeleteResponse> actionListener
259-
) {
260-
// Don't use index pattern here to avoid delete other user's index by mistake
261-
DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(
262-
configuration.getSessionIndexName(),
263-
configuration.getWorkingMemoryIndexName(),
264-
configuration.getLongMemoryIndexName(),
265-
configuration.getLongMemoryHistoryIndexName()
266-
);
267-
268-
log
269-
.debug(
270-
"Attempting to delete all memory indices for container {}: [{}, {}, {}, {}]",
271-
memoryContainerId,
272-
configuration.getSessionIndexName(),
273-
configuration.getWorkingMemoryIndexName(),
274-
configuration.getLongMemoryIndexName(),
275-
configuration.getLongMemoryHistoryIndexName()
276-
);
277-
memoryContainerHelper.deleteIndex(configuration, deleteIndexRequest, ActionListener.wrap(r -> {
278-
log
279-
.info(
280-
"Delete memory container - Event: ALL_INDICES_DELETED, Container ID: {}, Indices: [{}, {}, {}, {}], User: {}, Timestamp: {}",
281-
memoryContainerId,
282-
configuration.getSessionIndexName(),
283-
configuration.getWorkingMemoryIndexName(),
284-
configuration.getLongMemoryIndexName(),
285-
configuration.getLongMemoryHistoryIndexName(),
286-
user != null ? user.getName() : "unknown",
287-
Instant.now()
288-
);
289-
actionListener.onResponse(deleteResponse);
290-
}, e -> {
291-
log
292-
.error(
293-
"Failed to delete memory indices for container: {}. Indices: [{}, {}, {}, {}]",
294-
memoryContainerId,
295-
configuration.getSessionIndexName(),
296-
configuration.getWorkingMemoryIndexName(),
297-
configuration.getLongMemoryIndexName(),
298-
configuration.getLongMemoryHistoryIndexName(),
299-
e
300-
);
301-
actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR));
302-
}));
303-
}
304-
305253
private void deleteSelectiveMemoryIndices(
306254
String memoryContainerId,
307255
MemoryConfiguration configuration,
@@ -334,19 +282,14 @@ private void deleteSelectiveMemoryIndices(
334282
}
335283

336284
if (!indicesToDelete.isEmpty()) {
337-
log
338-
.debug(
339-
"Attempting selective deletion of memory indices for container {}: {}, requested types: {}",
340-
memoryContainerId,
341-
indicesToDelete,
342-
deleteMemories
343-
);
344-
285+
boolean isDeleteAll = deleteMemories.containsAll(EnumSet.allOf(MemoryType.class));
345286
DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(indicesToDelete.toArray(new String[0]));
346287
memoryContainerHelper.deleteIndex(configuration, deleteIndexRequest, ActionListener.wrap(r -> {
288+
String event = isDeleteAll ? "ALL_INDICES_DELETED" : "SELECTIVE_INDICES_DELETED";
347289
log
348290
.info(
349-
"Delete memory container - Event: SELECTIVE_INDICES_DELETED, Container ID: {}, Indices: {}, Memory Types: {}, User: {}, Timestamp: {}",
291+
"Delete memory container - Event: {}, Container ID: {}, Indices: {}, Memory Types: {}, User: {}, Timestamp: {}",
292+
event,
350293
memoryContainerId,
351294
indicesToDelete,
352295
deleteMemories,
@@ -355,6 +298,14 @@ private void deleteSelectiveMemoryIndices(
355298
);
356299
actionListener.onResponse(deleteResponse);
357300
}, e -> {
301+
// Preserve client errors (4XX) with their detailed messages
302+
if (e instanceof OpenSearchException) {
303+
OpenSearchException osException = (OpenSearchException) e;
304+
if (osException.status().getStatus() >= 400 && osException.status().getStatus() < 500) {
305+
actionListener.onFailure(e);
306+
return;
307+
}
308+
}
358309
log.error("Failed to delete selective memory indices [{}] for container: {}.", memoryContainerId, indicesToDelete, e);
359310
actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR));
360311
}));

0 commit comments

Comments
 (0)
⚡