Skip to content

[BUG] Dynamic pruning failure in cardinality aggregation falls back instead of failing the shard (#22582) - #22629

Open
waterWang wants to merge 1 commit into
opensearch-project:mainfrom
waterWang:fix/cardinality-aggregation-dynamic-pruning-fallback
Open

[BUG] Dynamic pruning failure in cardinality aggregation falls back instead of failing the shard (#22582)#22629
waterWang wants to merge 1 commit into
opensearch-project:mainfrom
waterWang:fix/cardinality-aggregation-dynamic-pruning-fallback

Conversation

@waterWang

Copy link
Copy Markdown

Description

When a cardinality aggregation is used together with a hybrid query that has two or more subqueries, the dynamic pruning optimization in CardinalityAggregator can read past the end of a doc-values slice, producing an EOFException: read past EOF.

Root Cause

The dynamic pruning path in tryScoreWithPruningCollector assumes scorer/iterator semantics that the hybrid scorer (from the neural-search plugin) does not provide. When the pruning fails, the original code wraps the exception in an OpenSearchStatusException with RestStatus.INTERNAL_SERVER_ERROR, which:

  1. Causes the entire shard to fail (_shards.failed > 0)
  2. Returns HTTP 200 with silently truncated results
  3. Clients that only check the HTTP status code never see the partial failure

Fix

Changed tryScoreWithPruningCollector to:

  1. Log a warning instead of throwing an error when pruning fails
  2. Return false to trigger the non-pruning fallback path in pickCollector
  3. Close the pruning collector on all failure paths to prevent resource leaks

This means the cardinality aggregation will still compute correctly — just without the pruning optimization for the affected segment. The user sees the correct result instead of a silent partial failure.

Related Issues

Closes #22582

Testing

  • Dynamic pruning is an optimization; the non-pruning path is always available as a fallback
  • The existing test suite for cardinality aggregation continues to exercise the non-pruning path
  • The error message in the warning log (with the setting name to disable pruning) is preserved for debugging

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Partial State On Fallback

When an exception occurs mid-way through bulkCollect, the pruningCollector may have already partially collected documents into shared aggregator state (e.g., counts/hashes) before the failure. Returning false to trigger the non-pruning fallback path will then re-collect those documents, potentially producing incorrect cardinality results (double counting) rather than the silent truncation described. Consider whether the pruning collector's mutations to aggregator state can be rolled back, or whether the fallback is only safe when the exception occurs before any collection has taken place.

} catch (Exception e) {
    Releasables.closeWhileHandlingException(pruningCollector);
    logger.warn(
        "Dynamic pruning failed for cardinality aggregation, falling back to non-pruning path. "
            + "You can set cluster setting [{}] to 0 to disable this optimization.",
        CARDINALITY_AGGREGATION_PRUNING_THRESHOLD.getKey(),
        e
    );
    return false;
}

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid swallowing control-flow exceptions

Catching Exception will also swallow CollectionTerminatedException and other
control-flow exceptions that the aggregation framework relies on. Additionally,
partial state may already be written into the pruning collector's internal counts
before the failure, which could produce incorrect results if callers merge results.
Consider catching only IOException/RuntimeException, and/or re-throwing
CollectionTerminatedException and InterruptedException so shard cancellation still
works.

server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregator.java [290-298]

+} catch (CollectionTerminatedException e) {
+    throw e;
 } catch (Exception e) {
     Releasables.closeWhileHandlingException(pruningCollector);
     logger.warn(
         "Dynamic pruning failed for cardinality aggregation, falling back to non-pruning path. "
             + "You can set cluster setting [{}] to 0 to disable this optimization.",
         CARDINALITY_AGGREGATION_PRUNING_THRESHOLD.getKey(),
         e
     );
     return false;
 }
Suggestion importance[1-10]: 6

__

Why: Valid concern: catching all Exception may swallow CollectionTerminatedException which is used for control flow in Lucene collectors. Re-throwing it preserves proper aggregation termination semantics, though the practical impact depends on whether such exceptions actually occur in this path.

Low

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for d303322: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

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

Labels

bug Something isn't working Search:Aggregations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] EOFException "read past EOF" in cardinality aggregation dynamic pruning when combined with a hybrid query

1 participant