Skip to content

fix: correct inverted condition in DocLevelMonitorQueries causing query index churn - #2154

Open
thecodingshrimp wants to merge 3 commits into
opensearch-project:mainfrom
thecodingshrimp:fix/doc-level-query-index-inverted-condition
Open

thecodingshrimp wants to merge 3 commits into
opensearch-project:mainfrom
thecodingshrimp:fix/doc-level-query-index-inverted-condition

Conversation

@thecodingshrimp

Copy link
Copy Markdown

Summary

Fixes #2153

The condition in DocLevelMonitorQueries.updateQueryIndexMappings that decides when to re-fetch the write index from the query index alias was inverted (!= instead of ==). This caused the re-fetch path to fire on every monitor execution rather than only in the backwards-compatibility case it was designed to handle.

Root Cause

monitorMetadata.sourceToQueryIndexMapping stores the concrete backing index name (e.g. .opensearch-sap-pre-packaged-rules-queries-000001) after the first successful lookup. The condition was intended to re-resolve the alias only when the stored value is the alias name itself — a legacy situation where the metadata was written before this logic existed.

With !=, the condition evaluates to true on every subsequent run because the stored concrete index name is always different from the alias name. This triggers getWriteIndexNameForAlias and a metadata rewrite on every run, which cascades into a delete+recreate of the backing query index, generating 6–10 MergeSchedulerConfig log lines per node per cycle.

With ==, the condition correctly fires only when the alias name itself is stored, which is the backwards-compat path it was designed for.

Fix

// Before (broken)
targetQueryIndex != monitor.dataSources.queryIndex && monitor.deleteQueryIndexInEveryRun == true

// After (correct)
targetQueryIndex == monitor.dataSources.queryIndex && monitor.deleteQueryIndexInEveryRun == true

The comment block was also updated to document the correct invariant.

Impact

At scale (3 master nodes, many detectors), the broken condition produced 23,000+ MergeSchedulerConfig log lines per minute. This fix eliminates that churn.

Related

This bug is amplified by a separate issue in opensearch-project/security-analytics where chained_findings monitors are created with deleteQueryIndexInEveryRun=true, activating the broken code path on every detector run. Both fixes are needed to fully eliminate the log storm. The security-analytics fix prevents the flag from being set unnecessarily; this fix corrects the inverted logic that acts on it.

@cwperks

cwperks commented Oct 27, 2026

Copy link
Copy Markdown
Member

@thecodingshrimp same comment here as the other PR. Can we add a test for this?

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 86eadb7)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Logic Correctness

The fixed condition targetQueryIndex == monitor.dataSources.queryIndex && monitor.deleteQueryIndexInEveryRun == true only re-resolves the alias when the stored value happens to equal the alias name AND the flag is true. Verify this matches the intended semantics: when deleteQueryIndexInEveryRun==true is set to force index recreation on every run, this change means the stored concrete index (which no longer exists after deletion) will be used as targetQueryIndex on subsequent runs, since it will never equal the alias name. If the deletion path elsewhere is supposed to clear the metadata entry or reset it to the alias, confirm that path still runs; otherwise the "delete+recreate every run" feature may be silently disabled for monitors that have already populated the mapping with a concrete name.

): Pair<AcknowledgedResponse, String> {
    var targetQueryIndex = monitorMetadata.sourceToQueryIndexMapping[sourceIndex + monitor.id]
    if (
        targetQueryIndex == null || (
            targetQueryIndex == monitor.dataSources.queryIndex &&
                monitor.deleteQueryIndexInEveryRun == true
            )
    ) {
        // Fix: old != caused churn — a concrete index always differs from the alias, so the branch
        // fired every run. == also catches the rare case where the alias name itself was persisted.
        targetQueryIndex = getWriteIndexNameForAlias(monitor.dataSources.queryIndex)

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Always re-resolve when stored value is alias

The fix only re-resolves when the stored value equals the alias AND
deleteQueryIndexInEveryRun is true. If a stale alias value was persisted but
deleteQueryIndexInEveryRun is false/null, the alias string will be used as the
concrete index for mapping updates, which will fail or write to the alias instead of
a concrete index. Consider re-resolving whenever the stored value equals the alias
name, regardless of the delete flag.

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt [497-503]

 var targetQueryIndex = monitorMetadata.sourceToQueryIndexMapping[sourceIndex + monitor.id]
 if (
-    targetQueryIndex == null || (
-        targetQueryIndex == monitor.dataSources.queryIndex &&
-            monitor.deleteQueryIndexInEveryRun == true
-        )
+    targetQueryIndex == null ||
+    targetQueryIndex == monitor.dataSources.queryIndex
 ) {
Suggestion importance[1-10]: 6

__

Why: The suggestion raises a valid edge case: if the alias name was previously persisted in metadata and deleteQueryIndexInEveryRun is false/null, the alias would be used as a concrete index. However, the PR author intentionally scoped the fix to the deleteQueryIndexInEveryRun == true case, and the test test updateQueryIndexMappings when deleteQueryIndexInEveryRun false and targetQueryIndex equals alias does not re-resolve explicitly validates the current behavior, so the suggestion contradicts intended behavior but highlights a legitimate concern.

Low

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9394ff7

…ry index churn (opensearch-project#2153)

Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
…-project#2153)

Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
…eries (opensearch-project#2153)

- Clarify updateQueryIndexMappings comment (churn fix is primary, alias-persistence is secondary)
- Open class and key methods for test isolation; document extension-point intent
- Add log.warn when updateSettings is not acknowledged
- Add lastPutConcreteQueryIndex assertions across all happy-path tests
- Add tests: null+flag=false resolves; alias with no write index; field-limit rollover; non-field-limit exception

Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
@thecodingshrimp
thecodingshrimp force-pushed the fix/doc-level-query-index-inverted-condition branch from 9394ff7 to 86eadb7 Compare October 28, 2026 09:19
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 86eadb7

@thecodingshrimp

Copy link
Copy Markdown
Author

@thecodingshrimp same comment here as the other PR. Can we add a test for this?

@cwperks added some tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] DocLevelMonitorQueries: inverted condition causes query index to be deleted and recreated on every monitor execution

2 participants