Count failed msearch sub-search responses against the circuit breaker - #156683

Nit: Does randomness adds anything here? Predictable tests are generally better.

Good catch - unlike the success-path test, this one doesn't sample a timing-sensitive peak-usage window, so the random scale wasn't adding coverage. Switched to a fixed numSearches

I find using the ternary operator for side-effect while returning the same object confusing.
I think it would be clearer as:

if (item.isFailure()) {
   item = accountFailureItem(breakerAccounting, item);
}
responses.set(responseSlot, item);

Agreed, applied as suggested.

Shouldn't this flow first de-alloc item?

This code path runs when item.isFailure() is true, and a failure item do not carry a SearchResponse — both places in this file that construct one pass response=null for the failure case.

This not only accounts, but can replace. The name can be misleading.

Fair point! Renamed to accountOrSubstituteFailureItem to reflect that it can replace the item, not just account for it.

Just for my own knowledge, when would we expect the breaker to be less than zero here?

That should never happen. I picked up that pattern from a previously declared test. I've updated all occurrences in the test class.

Should this be equalTo()? If the breaker returns to a value lower than the baseline then wouldn't that mean that something has been miscounted?

Fair point. Updated in the previous commit.

Is it that it's safe, or just that we have no alternative at this point? It's possible that we could still OOM due to the CircuitBreakingException if there are enough of them, I think?

This is the core of that PR, safe was too strong; updated the wording.

A failed sub-search still has to produce a response item — it can't be dropped or retried — so when reserving breaker bytes for it trips the limit, rejecting it outright isn't an option.

This isn't just theoretical either. Estimating a failure's size means walking its whole cause chain, and for a SearchPhaseExecutionException we also have to sum up every ShardSearchFailure's cause and reason string. A search that fails across a lot of shards ends up with a genuinely large exception. On top of that, _msearch buffers every item — successes and failures — until the whole batch finishes, and all of that is reserved against the same shared REQUEST breaker as everything else running on the node. So a big failure can trip it on its own, and it's also competing for headroom with whatever else happens to be using that breaker at the time. (This is what happened in a real-world scenario.)

What we do instead is substitute a small, fixed-size CircuitBreakingException and account for that rather than the original, which has no upper bound. That doesn't remove the risk, it just bounds it: if the breaker's already so full that even the small substitute won't fit, we still force it through with addWithoutBreaking, since refusing to record it at all would leave the request stuck forever. The important part is that what gets force-added is now capped and no longer tied to how bad the original failure was, so the worst case is a small, bounded overshoot instead of one that scales with the failure.

Either way, all of it — original or substitute — gets released back to the breaker once the _msearch finishes, same as successful responses.

safe was too strong; updated the wording.

Not a major issue, but the wording still says "safe", just checking that this wasn't something that was intended to be changed but got missed

That fix wasn't committed yet, only staged locally. Pushing it now.

Possibly overkill, but could the byteLimit be some fraction of originalFailureBytes so that it's guaranteed to always be below the failure bytes? The current value of 2048 seems arbitrary if you don't know what a reasonable expected value for originalFailureBytes is.

Good idea, the byteLimit is now originalFailureBytes / 2, so it's always guaranteed to be below the failure size, regardless of the estimation constants.

This comment seems slightly misleading since the byte limit is exhausted immediately by the first failure. Would the test be better if the first few failures did not trip the breaker, but then subsequent ones did?

+1 Set the limit to fit exactly the first 2 original failures, and everything after is substituted

Is this test doing anything meaningfully different than the one above it? Both cause the breaker to trip immediately due to a too-large SearchPhaseExecutionException and then assert that the failure we store is a CircuitBreakingException. This test just uses a single failure rather than several, so it seems like the behaviour is already covered in the above test.

They test different outcomes: this one shows the substitute always fits (maxWithoutBreaking() == 0), the one above shows it can also get force-added when the breaker's already full (maxWithoutBreaking() > 0). Prefer keeping them separate so a failure points to the exact broken behavior.

Ah, I see, thanks for the clarification!

The value of totalWithoutBreaking is never used, so could it be removed? Also, would maxWithoutBreaking be clearer as largestAddWithoutBreaking? I wasn't initially sure what maxWithoutBreaking meant before looking at the implementation of addWithoutBreaking().

Good catch, totalWithoutBreaking was dead and removed it. Renamed maxWithoutBreaking to largestAddWithoutBreaking for clarity as proposed

safe was too strong; updated the wording.

Not a major issue, but the wording still says "safe", just checking that this wasn't something that was intended to be changed but got missed

Ah, I see, thanks for the clarification!

Do we need to use the same approach as in testAllSubSearchesFailStillTripsBreaker() to determine the appropriate limit for the breaker here? If I add some logging to TrackingCircuitBreaker.addEstimateBytesAndMaybeBreak() I see byteLimit = 937996 and bytes = 1162917, which is a difference of significantly more than 1. I don't think it affects the test in a particularly meaningful way, but the fact that largeFailureBytes is ~20% smaller than the actual size of the failure that gets passed to the circuit breaker could cause some confusion. For example, it might be expected that the test would start failing if we used new TrackingCircuitBreaker(largeFailureBytes + 1) because the first failure wouldn't trip the breaker, but it still passes.

Maybe it would be enough to just use new TrackingCircuitBreaker(largeFailureBytes) and add a comment saying that the actual size of the failure that makes it to accountOrSubstituteFailureItem() is bigger than the SearchPhaseExecutionException the test creates because the stack is slightly different. The one we use to calculate largeFailureBytes has:

java.lang.RuntimeException: simulated shard failure 0
	at org.elasticsearch.action.search.TransportMultiSearchActionTests.searchPhaseExecutionExceptionWithShardFailures(TransportMultiSearchActionTests.java:1001)
	at org.elasticsearch.action.search.TransportMultiSearchActionTests.testAccountFailureItemSubstitutesBoundedExceptionWhenTripped(TransportMultiSearchActionTests.java:976)
...

but the one that makes it to accountOrSubstituteFailureItem() has:

java.lang.RuntimeException: simulated shard failure 0
	at org.elasticsearch.action.search.TransportMultiSearchActionTests.searchPhaseExecutionExceptionWithShardFailures(TransportMultiSearchActionTests.java:1001)
	at org.elasticsearch.action.search.TransportMultiSearchActionTests.lambda$testAccountFailureItemSubstitutesBoundedExceptionWhenTripped$31(TransportMultiSearchActionTests.java:987)
	at org.elasticsearch.action.search.TransportMultiSearchActionTests$3.search(TransportMultiSearchActionTests.java:1064)
	at org.elasticsearch.action.search.TransportMultiSearchAction.doExecuteSearch(TransportMultiSearchAction.java:603)
	at org.elasticsearch.action.search.TransportMultiSearchAction.executeSearch(TransportMultiSearchAction.java:586)
	at org.elasticsearch.action.search.TransportMultiSearchAction.doExecute(TransportMultiSearchAction.java:248)
	at org.elasticsearch.action.search.TransportMultiSearchAction.doExecute(TransportMultiSearchAction.java:59)
	at org.elasticsearch.action.support.TransportAction$RequestFilterChain.proceed(TransportAction.java:135)
	at org.elasticsearch.action.support.TransportAction.handleExecution(TransportAction.java:96)
	at org.elasticsearch.action.support.TransportAction.execute(TransportAction.java:59)
	at org.elasticsearch.action.search.TransportMultiSearchActionTests.runMsearchWithBreaker(TransportMultiSearchActionTests.java:1092)
	at org.elasticsearch.action.search.TransportMultiSearchActionTests.testAccountFailureItemSubstitutesBoundedExceptionWhenTripped(TransportMultiSearchActionTests.java:982)
...

(with everything after the ... being the same in both stacks)

You are right, the -1 implied more precision than it actually has. Went with your suggestion: dropped it and added a comment explaining the real failure ends up bigger due to the deeper stack trace it picks up going through the actual execution chain.