Fix ConcurrencyLimitTest flakiness on threeNodes (#435)#436
Merged
Conversation
The test counted the downstream `Blocker/%/run` invocations as a proxy for how many scoped invocations are running. That count is cumulative, not concurrent: each BlockingProxy calls a distinct Blocker key, and once a Blocker/run is spawned it blocks on an awakeable and stays running until resolved. Under a leadership change a running BlockingProxy yields its slot to a held one, but the Blocker/run it already spawned keeps running, so the Blocker count can exceed `limit` even though the limit is respected. The runtime only guarantees no more than `limit` scoped invocations run concurrently, not that it is always the same set. Count the scoped BlockingProxy invocations with status `running` instead, which settles to exactly `limit` and tolerates leadership-change churn. Additionally track, via in-process atomics on BlockingProxy.block, the maximum number of handlers that ever ran concurrently, and assert it never exceeds `limit`. This strict bound is only asserted on single-node clusters: on multi-node clusters a leadership change can momentarily run more than `limit` handlers because the old leader's in-flight invocations are not guaranteed to be torn down before the new leader dispatches replacements. Also rename the test to drop "action" from "action concurrency limit". This fixes restatedev#435. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Address review: make ConcurrencyLimitTest @isolated and single-node only The strict in-process concurrency bound is the point of the test and can only be asserted on single-node clusters, so remove the test from the threeNodes suite. Mark the class @isolated so it stays isolated if more tests are added to it later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The test counted the downstream
Blocker/%/runinvocations as a proxy for how many scoped invocations are running. That count is cumulative, not concurrent: each BlockingProxy calls a distinct Blocker key, and once a Blocker/run is spawned it blocks on an awakeable and stays running until resolved. Under a leadership change a running BlockingProxy yields its slot to a held one, but the Blocker/run it already spawned keeps running, so the Blocker count can exceedlimiteven though the limit is respected. The runtime only guarantees no more thanlimitscoped invocations run concurrently, not that it is always the same set.Count the scoped BlockingProxy invocations with status
runninginstead, which settles to exactlylimitand tolerates leadership-change churn.Additionally track, via in-process atomics on BlockingProxy.block, the maximum number of handlers that ever ran concurrently, and assert it never exceeds
limit. This strict bound is only asserted on single-node clusters: on multi-node clusters a leadership change can momentarily run more thanlimithandlers because the old leader's in-flight invocations are not guaranteed to be torn down before the new leader dispatches replacements.Also rename the test to drop "action" from "action concurrency limit".
This fixes #435