Fix IndexStatsIT#testThrottleStats

[arteam]

Engine.PauseLock#throttle can be called when the lock is being throttled, so we can't guarantee that all permits are available before throttling.

Resolve #126359
See #127173

[elasticsearchmachine]

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

[ankikuma]

Hi Artem, sorry for taking so long to get to this. I am not convinced this is the right fix. We do not call throttle and unthrottle directly on the lock. We go through InternalEngine#activateThrottling and InternalEngine#deactivateThrottling instead, which update a counter to make sure that the throttle and unthrottle calls are matched.
Basically, the callers are expected to follow the semantics that for each activateThrottling call there should be a matching deactivateThrottling call and we throw asserts in the code if this is not the case.
I think the problem might be in the way the ThreadPoolMergeScheduler is calling activateThrottling and deactivateThrottling.

But I am not sure yet where the mismatch might be caused.

[ankikuma]

I believe there is a race condition between IndexThrottle#activate() and IndexThrottle#deactivate() because both can be called by different threads.

[henningandersen]

I believe there is a race condition between IndexThrottle#activate() and IndexThrottle#deactivate() because both can be called by different threads.

This makes sense to me too, I think the test could be rewritten to use those handles and provoke the issue.

I agree the fix needs to be done differently, we might need to have a lock inside activeate/deactivateThrottling (though other solutions can be done too, but seems more complex and this should not be a heavily called mechanism).

[albertzaharovits]

I believe there is a race condition between IndexThrottle#activate() and IndexThrottle#deactivate() because both can be called by different threads.

But the ThreadPoolMergeScheduler calls the throttling methods under a lock:

elasticsearch/server/src/main/java/org/elasticsearch/index/engine/ThreadPoolMergeScheduler.java

Line 251 in b909a50

synchronized (shouldThrottleIncomingMerges) {

. What I think is going is a cycle of IndexThrottle#activate -> IndexThrottle#deactivate -> IndexThrottle#activate where some IndexThrottle#acquireThrottle from the first activate haven't released yet, before the second one. If that's the case, I believe Artem's fix proposal, to remove the assert, should fix this test failure (but I don't think that the added test exercises the fault here).

[ankikuma]

This has been fixed in #128405. We can close this PR.