Fix several bugs on agentic memory #4476

b4sjoo · 2025-12-01T13:32:17Z

Description

Add a default index setting for memory indices to avoid index creating exception in multi-node case
fix not stashing context if index is not system index
fix NPE during search memory container when user is null

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Summary by CodeRabbit

Chores
- Improved memory index configuration handling with consistent default settings across session, working, and long-term history indices, ensuring more reliable fallback behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-01T13:33:11Z

Walkthrough

Replaced null/empty memory index mappings with a safe default and added a new initIndexWithContext path; also guarded admin-role checks with a user nullity check in memory container search action.

Changes

Cohort / File(s)	Summary
Index initialization & defaults `ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLIndicesHandler.java`	Add null/empty checks for memory index mappings and default to `ALL_NODES_REPLICA_INDEX_SETTINGS`; route index initialization via `initIndexIfAbsent` when using system indices or new `initIndexWithContext(...)` otherwise; add public `initIndexWithContext(...)` method and `Objects` import.
Access check null-safety `plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportSearchMemoryContainerAction.java`	Guard admin-role check with `user != null` to avoid NPE (change to `if (user != null && !memoryContainerHelper.isAdminUser(user))`), preserving backend-role filtering behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Review initialization branching between initIndexIfAbsent and the new initIndexWithContext for correctness and concurrent indexing behavior.
Verify public API/visibility of the new initIndexWithContext and its listener semantics.
Confirm ALL_NODES_REPLICA_INDEX_SETTINGS is appropriate default merging behavior in createLongTermMemoryIndex.
Check the null-safe admin check does not change intended authorization semantics or logging.

Poem

🐇 I hopped through code at break of day,

Defaults in place to keep wild NPEs away,
A new pathway found for indices to grow,
And a cautious check keeps the roles in tow,
Hooray for safer starts — now off I go! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Fix several bugs on agentic memory' is vague and doesn't specifically describe the main change of adding default index settings for memory indices.	Use a more specific title that reflects the core change, such as 'Add default index settings for memory indices to prevent multi-node creation exceptions'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description includes the required template structure with Description and Related Issues sections, but the Related Issues section uses an unfilled placeholder without an actual issue number.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLIndicesHandler.java (2)

175-200: Memory index defaults align with PR goal; consider de-duplicating and caching lookups

Using ALL_NODES_REPLICA_INDEX_SETTINGS when configuration.getMemoryIndexMapping(...) is null or empty for session, working, and long‑term history indices matches the intent to have safe multi‑node defaults and avoids relying on initIndexIfAbsent’s generic fallback.

Two small, optional cleanups:

Cache the result of configuration.getMemoryIndexMapping(...) in a local variable per method to avoid calling it twice and to make the null/empty handling clearer.

The three methods now share essentially the same pattern; consider a small helper (e.g., resolveMemoryIndexSettings(MemoryConfiguration config, String logicalIndex)) to centralize this logic and reduce future drift between indices.

These are non-blocking and can be done later if you prefer to keep this PR minimal.

277-282: Long‑term memory index: new replica default looks correct; verify config access pattern

Adding ALL_NODES_REPLICA_INDEX_SETTINGS in the else branch ensures long‑term memory indices get a sane default when no per‑index settings are configured, which is consistent with the other memory indices and should help with multi‑node index creation.

One thing to double‑check:

The guard uses memoryConfig.getIndexSettings().containsKey(LONG_TERM_MEMORY_INDEX) but then pulls the map via memoryConfig.getMemoryIndexMapping(LONG_TERM_MEMORY_INDEX). If these two accessors are backed by the same underlying structure, this is fine; if not, there’s a small risk of divergence (e.g., the key present in one but not the other). A slightly clearer pattern would be to retrieve the map once and check for null/isEmpty() directly, similar to the other memory index methods.

Functionally this looks good; I’d just confirm the MemoryConfiguration API contract or simplify it in a follow‑up.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87e742e and 01e0c40.

📒 Files selected for processing (1)

ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLIndicesHandler.java (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Build and Test MLCommons Plugin on linux (21)
GitHub Check: Build and Test MLCommons Plugin on Windows (21)
GitHub Check: Build and Test MLCommons Plugin on Windows (24)
GitHub Check: Build and Test MLCommons Plugin on linux (24)

Signed-off-by: Sicheng Song <sicheng.song@outlook.com>

…#4407) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

…project#4427)

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (3)

plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportSearchMemoryContainerAction.java (1)

91-94: Null-guard on user correctly prevents potential NPE

The updated condition if (user != null && !memoryContainerHelper.isAdminUser(user)) safely avoids calling isAdminUser and addUserBackendRolesFilter with a null User, eliminating a possible NPE while preserving intended behavior (filter only for non-admin, non-null users). If helpful for future readers, consider a brief comment clarifying what a null user represents here (e.g., system or security-disabled context) and why no filter is applied in that case.

ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLIndicesHandler.java (2)

289-295: Default long‑term memory index settings behavior looks good; consider future upgrade story

Using ALL_NODES_REPLICA_INDEX_SETTINGS as a fallback when LONG_TERM_MEMORY_INDEX has no explicit settings is consistent with the other memory indices and should help with multi‑node robustness. The conditional routing to initIndexIfAbsent (system index) vs initIndexWithContext (non‑system) also makes sense.

One thing to be aware of: the non‑system path via initIndexWithContext does not perform any mapping/version checks or settings updates if the index already exists, unlike initIndexIfAbsent. If you ever need schema upgrades for non‑system long‑term memory indices, you’ll likely need a follow‑up to add an upgrade path similar to the system index flow.

Also applies to: 298-302

318-351: initIndexWithContext behavior differences and unused version parameter

The new initIndexWithContext method cleanly handles parallel index creation and applies a default via Objects.requireNonNullElse(indexSettings, DEFAULT_INDEX_SETTINGS), which matches the use cases in this PR.

Two minor points to consider:

The version argument is currently unused; keeping it in the signature may be confusing since initIndexIfAbsent uses version to drive upgrade logic. Either document that it’s intentionally unused here or remove it if you don’t expect upgrades on this path.

Unlike initIndexIfAbsent, this method does not check doesMultiTenantIndexExist and does not perform mapping/settings upgrades for existing indices. That seems intentional for context‑specific/non‑system indices, but it’s worth making sure all call sites understand this semantic difference.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 01e0c40 and fcace1b.

📒 Files selected for processing (2)

ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLIndicesHandler.java (4 hunks)
plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportSearchMemoryContainerAction.java (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: spotless

coderabbitai · 2025-12-02T11:14:41Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLIndicesHandler.java

    public void createSessionMemoryDataIndex(String indexName, MemoryConfiguration configuration, ActionListener<Boolean> listener) {
        String indexMappings = getMapping(ML_MEMORY_SESSION_INDEX_MAPPING_PATH);
-        Map<String, Object> indexSettings = configuration.getMemoryIndexMapping(SESSION_INDEX);
-        initIndexIfAbsent(indexName, StringUtils.toJson(indexMappings), indexSettings, 1, listener);
+        Map<String, Object> indexSettings = configuration.getMemoryIndexMapping(SESSION_INDEX) == null
+            || configuration.getMemoryIndexMapping(SESSION_INDEX).isEmpty()
+                ? ALL_NODES_REPLICA_INDEX_SETTINGS
+                : configuration.getMemoryIndexMapping(SESSION_INDEX);
+        if (configuration.isUseSystemIndex()) {
+            initIndexIfAbsent(indexName, StringUtils.toJson(indexMappings), indexSettings, 1, listener);
+        } else {
+            initIndexWithContext(indexName, StringUtils.toJson(indexMappings), indexSettings, 1, listener);
+        }
    }


⚠️ Potential issue | 🔴 Critical

Avoid double-encoding mapping JSON in memory index creators

getMapping(...) already returns a JSON string for the mapping. Wrapping that string with StringUtils.toJson(indexMappings) will typically serialize it again as a JSON string literal (e.g., "\"{...}\""), which will not be a valid mapping document when passed to CreateIndexRequest.mapping(..., XContentType.JSON). That would cause index creation to fail for session, working, and long-term memory history indices.

At the same time, the new defaulting to ALL_NODES_REPLICA_INDEX_SETTINGS when getMemoryIndexMapping(...) is null/empty looks correct and aligned with the PR’s goal.

Suggested fix (applies to all three methods):

- if (configuration.isUseSystemIndex()) { - initIndexIfAbsent(indexName, StringUtils.toJson(indexMappings), indexSettings, 1, listener); - } else { - initIndexWithContext(indexName, StringUtils.toJson(indexMappings), indexSettings, 1, listener); - } + if (configuration.isUseSystemIndex()) { + initIndexIfAbsent(indexName, indexMappings, indexSettings, 1, listener); + } else { + initIndexWithContext(indexName, indexMappings, indexSettings, 1, listener); + }

Also applies to: 189-200, 202-213

🤖 Prompt for AI Agents

In ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLIndicesHandler.java around lines 176-187 (and similarly apply to 189-200 and 202-213), the code double-encodes the mapping JSON by calling StringUtils.toJson(indexMappings) even though getMapping(...) already returns a JSON string; remove the StringUtils.toJson(...) wrapper and pass the indexMappings string directly to initIndexIfAbsent/initIndexWithContext so the mapping is provided as raw JSON (ensure the calls still use the JSON content type where required).

dhrubo-os · 2025-12-02T16:15:46Z

Execution failed for task ':opensearch-ml-plugin:jacocoTestCoverageVerification'.
> A failure occurred while executing org.gradle.internal.jacoco.JacocoCoverageAction
   > Rule violated for class org.opensearch.ml.action.memorycontainer.TransportSearchMemoryContainerAction: branches covered ratio is 0.6, but expected minimum is 0.7

dhrubo-os · 2025-12-02T16:17:43Z

...main/java/org/opensearch/ml/action/memorycontainer/TransportSearchMemoryContainerAction.java

                .wrap(wrappedListener::onResponse, e -> wrapListenerToHandleSearchIndexNotFound(e, wrappedListener));

-            if (!memoryContainerHelper.isAdminUser(user)) {
+            if (user != null && !memoryContainerHelper.isAdminUser(user)) {


what happens for security disabled cluster.

All user will have admin permission then

b4sjoo · 2025-12-02T22:10:05Z

Execution failed for task ':opensearch-ml-plugin:jacocoTestCoverageVerification'.
> A failure occurred while executing org.gradle.internal.jacoco.JacocoCoverageAction
   > Rule violated for class org.opensearch.ml.action.memorycontainer.TransportSearchMemoryContainerAction: branches covered ratio is 0.6, but expected minimum is 0.7

Will add UTs

b4sjoo requested review from HenryL27, Zhangxunmt, austintlee, dhrubo-os, jngz-es, mingshl, model-collapse, pyek-bot, rbhavna, sam-herman, xinyual, ylwu-amzn and zane-neo as code owners December 1, 2025 13:32

b4sjoo had a problem deploying to ml-commons-cicd-env December 1, 2025 13:34 — with GitHub Actions Failure

b4sjoo had a problem deploying to ml-commons-cicd-env December 1, 2025 13:34 — with GitHub Actions Error

b4sjoo had a problem deploying to ml-commons-cicd-env December 1, 2025 13:34 — with GitHub Actions Failure

coderabbitai bot reviewed Dec 1, 2025

View reviewed changes

b4sjoo had a problem deploying to ml-commons-cicd-env December 1, 2025 21:08 — with GitHub Actions Error

b4sjoo had a problem deploying to ml-commons-cicd-env December 1, 2025 21:08 — with GitHub Actions Failure

b4sjoo and others added 3 commits December 2, 2025 10:54

Add a default index setting for memory indices

a127acb

Signed-off-by: Sicheng Song <sicheng.song@outlook.com>

not stashing context if index is not system index (opensearch-project…

a39100f

…#4407) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

fix NPE during search memory container when user is null (opensearch-…

fcace1b

…project#4427)

b4sjoo force-pushed the feature/mem-container branch from 01e0c40 to fcace1b Compare December 2, 2025 11:11

b4sjoo changed the title ~~Add a default index setting for memory indices~~ Fix several bugs on agentic memory Dec 2, 2025

b4sjoo had a problem deploying to ml-commons-cicd-env December 2, 2025 11:13 — with GitHub Actions Error

b4sjoo had a problem deploying to ml-commons-cicd-env December 2, 2025 11:13 — with GitHub Actions Failure

coderabbitai bot reviewed Dec 2, 2025

View reviewed changes

dhrubo-os reviewed Dec 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix several bugs on agentic memory #4476

Fix several bugs on agentic memory #4476

b4sjoo commented Dec 1, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 1, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Dec 2, 2025

Uh oh!

dhrubo-os commented Dec 2, 2025

Uh oh!

dhrubo-os Dec 2, 2025

Uh oh!

b4sjoo Dec 2, 2025

Uh oh!

b4sjoo commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix several bugs on agentic memory #4476

Are you sure you want to change the base?

Fix several bugs on agentic memory #4476

Conversation

b4sjoo commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

dhrubo-os commented Dec 2, 2025

Uh oh!

dhrubo-os Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

b4sjoo Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

b4sjoo commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

b4sjoo commented Dec 1, 2025 •

edited

Loading

coderabbitai bot commented Dec 1, 2025 •

edited

Loading