[9.5] ES|QL:Fix Tdigest percentiles circuit breaker (#155586) - #156858
Merged
elasticsearchmachine merged 1 commit intoAug 15, 2026
Conversation
* Integrate circuit breaker into TDigest percentiles agg `AbstractTDigestPercentilesAggregator` was creating every per-bucket `HistogramUnionState` with `NOOP_BREAKER`, making all TDigest memory invisible to the request circuit breaker. Under high-cardinality `TERMS` + `PERCENTILES` queries the heap could fill and OOM the node instead of the breaker throwing `CircuitBreakingException` and rejecting the query gracefully. Pass `context.breaker()` so the full tracking chain activates: `HistogramUnionState` -> `TDigestState` -> `MemoryTrackingTDigestArrays`, which charges every centroid array allocation against the REQUEST breaker. Also fix `doClose()` to close each `HistogramUnionState` individually before releasing the backing array, so the bytes charged at creation are returned to the breaker rather than leaked in its accounting. Closes elastic#99815 (_search agg path) * Revert unsafe doClose; add circuit breaker test Remove element-level close from doClose: HistogramUnionState is aliased by the InternalAggregation built in buildAggregation, and AggregatorCollector calls releaseAggregations() immediately after buildTopLevel(), making element-level close unsafe. Bytes from collection states are not explicitly returned; that requires making InternalTDigestPercentiles Releasable (deferred, elastic#99815). Add testCircuitBreakerTripsOnHighCardinality to verify the REQUEST breaker trips during collection when centroid arrays exceed the configured limit, confirming the context.breaker() fix works. * fix reduce-path crash: use NOOP_BREAKER in getLeaderReducer The HistogramUnionState built during collection holds a reference to the PreallocatedCircuitBreaker from the aggregation context. That breaker is closed when the context closes (after buildTopLevel()), before the reduce phase runs on the coordinator. When getLeaderReducer() called createUsingParamsFrom(percentiles.state) it inherited the now-closed breaker, causing IllegalStateException: already closed at addEstimateBytesAndMaybeBreak inside TDigestState.createUsingParamsFrom. Fix: add a breaker-explicit overload to both TDigestState and HistogramUnionState createUsingParamsFrom, and pass NOOP_BREAKER when creating the reduce-phase accumulator. Wire-deserialized states already used NOOP_BREAKER; this brings local-reduce into parity. The merged accumulator is never closed, so NOOP_BREAKER is correct here. * fix forbidden API: replace MatchAllDocsQuery with Queries.ALL_DOCS_INSTANCE testCircuitBreakerTripsOnHighCardinality used new MatchAllDocsQuery() directly; ES policy requires Queries.ALL_DOCS_INSTANCE instead. * fix doClose: null-out states after buildAggregation, close elements buildAggregation() previously returned a live HistogramUnionState reference to InternalTDigestPercentiles while doClose() would also attempt to close it (via Releasables.close(states) which only closes the ObjectArray container, not its elements -- but the element-level close was missing entirely). Add takeState() to the base class: atomically reads and nulls out a slot, transferring ownership to the caller. Both buildAggregation() overrides now use takeState() instead of getState() so doClose() can no longer double-close a state that was handed off successfully. doClose() now iterates over the remaining (non-null) slots and closes each one individually before closing the container. This ensures circuit-breaker bytes are returned on the failure path -- i.e. when a CircuitBreakingException aborts collection before buildAggregation is ever called, leaked states are released rather than left charged against the breaker until the parent breaker resets. Also guard against states == null in doClose(): a cranky circuit breaker can trip inside the constructor before the field is assigned. * fix takeState byte release and reducer swap Release breaker bytes in takeState() while the aggregation context is still open — the only safe window before the PreallocatedCircuitBreaker closes. Replace merge() swap logic with merged.add() to prevent the accumulator from inheriting a shard-side breaker that may already be closed. Use closeWhileHandlingException in doClose() so a failure on one slot does not prevent the remaining slots and the container from being released. * add breaker-balance assert; null-check doClose slots Verify that doClose() returns all partial breaker bytes after CircuitBreakingException by asserting the REQUEST breaker is at zero after expectThrows. Add explicit null check inside the doClose loop so taken slots are visibly skipped rather than relying on Releasables null-tolerance. * clean up comments and simplify test boilerplate Remove diff-anchored and redundant comments; fix em dashes in Javadoc; trim doClose block comment to two lines; extract requestBreakerService() helper to reduce test setup boilerplate. * add cranky breaker test; extract withSequentialIndex/collectWithBreaker helpers * restore merge() helper; only the NOOP_BREAKER initialisation needed to change * Update docs/changelog/155586.yaml * Update docs/changelog/155586.yaml * update comments. * address review feedback - @nullable + final on takeState(); document null cases - doClose(): early exit, drop redundant null guard in loop, remove try/finally (closeWhileHandlingException never rethrows) - NOOP_BREAKER comment: honest about coordinator gap + follow-up - remove redundant @param/@return from createUsingParamsFrom javadoc - rename tests; add terms+percentiles high-cardinality trip test; drop cranky test (coverage held by deterministic trip tests) * spotless: remove final from takeState() * [CI] Auto commit changes from spotless * simplify NOOP_BREAKER comment in getLeaderReducer * fix NOOP_BREAKER comment: describe use-after-close problem * update comments. * fix comment: NOOP_BREAKER is for unreleased accumulator * fix comment: explain both reasons for NOOP_BREAKER * simplify comments in AbstractTDigestPercentilesAggregator * simplify NOOP_BREAKER comment in getLeaderReducer * address review: static helper, drop redundant javadoc tags * mark takeState() final to prevent subclass breaker accounting bypass * add multi-bucket success and PercentileRanks circuit breaker tests * revert PercentileRanks breaker tests: fix is in the abstract class, already covered * remove duplicate multi-bucket success breaker test * remove redundant comments above breaker assertions * update comments, explain more clearly * address review feedback, add more clear comments. * make withSequentialIndex method static * Mute LookupJoinExpression CurrentCoordinator BWC tests * Revert LookupJoinExpression CurrentCoordinator BWC test mutes --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
hawkhuang-collab
added
:Analytics/ES|QL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports the following commits to 9.5: