Implement concurrency limits plugin

[bpatelcs]

Description

Adds a new concurrency-limit module that provides adaptive per-action concurrency limiting for OpenSearch. Operators can dynamically configure concurrency limits on any transport action (e.g., search, bulk) via cluster settings — no code changes required to protect new actions.

Key features

• Adaptive algorithms: Vegas (TCP-Vegas-based, default), Gradient2, and AIMD — powered by Netflix's concurrency-limits library
• Three operating modes: disabled (default), monitor_only (tracks metrics without rejecting), enforced (actively rejects with HTTP 429)
• Burst capacity: Optional burst headroom above the adaptive base limit, with a configurable close/open state machine to absorb transient spikes
• Vegas baseline poisoning protection: Probe-based RTT baseline resets are gated by a configurable inflight threshold (vegas.baseline_reset_load_threshold), preventing heavy-load latency from corrupting the no-load baseline and causing a feedback loop
• Increase/decrease hysteresis: Configurable consecutive-sample thresholds (vegas.increase_barrier, vegas.decrease_barrier) to dampen limit oscillation
• Request partitioning: Divide the total limit across named sub-pools (e.g., premium/standard) using pluggable resolvers (byHeader, fixed, bySearchType)
• Warmup period: Configurable grace period after (re)configuration during which requests are not rejected, allowing the algorithm to calibrate
• _nodes/stats integration: Limiter state (current limit, in-flight, total rejected, RTT) exposed under concurrency_limiters in the node stats API
• Push telemetry: Observable gauges registered via MetricsRegistry mirror the pull-path stats for monitoring systems

Configuration example

PUT /_cluster/settings
{
  "persistent": {
    "concurrency_limit.action.search.action_name": "indices:data/read/search",
    "concurrency_limit.action.search.mode": "enforced",
    "concurrency_limit.action.search.algorithm": "vegas",
    "concurrency_limit.action.search.limit.initial": 20,
    "concurrency_limit.action.search.limit.max": 200,
    "concurrency_limit.action.search.vegas.baseline_reset_load_threshold": 0.5,
    "concurrency_limit.action.search.burst.capacity": 10,
    "concurrency_limit.action.search.burst.close_after": 5,
    "concurrency_limit.action.search.burst.open_after": 5
  }
}

Files changed

• New module modules/concurrency-limit/
• Server integration — NodeStats, NodeService, Node, NodesStatsRequest, TransportNodesStatsAction, TransportClusterStatsAction updated to plumb concurrency_limiter stats (version-gated to V_3_8_0)
• SPI interface — ConcurrencyLimiterStatsPlugin for decoupled stats collection
• Test framework — MockInternalClusterInfoService, InternalTestCluster updated for new NodeStats constructor parameter

Test plan

• ./gradlew :modules:concurrency-limit:check — precommit, unit tests, spotless, missingJavadoc, thirdPartyAudit all pass
• Manual verification: start a node, configure a limiter via cluster settings, confirm _nodes/stats?metric=concurrency_limiter returns limiter state

Related Issues

Resolves #22295

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 56b241d.

⛔ Hard block: Issues at Medium severity or above will block this PR from merging.

Path	Line	Severity	Description
gradle/libs.versions.toml	23	high	New external dependency version introduced: 'netflix_concurrency_limits = "0.4.0"'. Per mandatory supply chain policy, any new dependency entry must be flagged for maintainer verification of artifact authenticity and integrity regardless of how legitimate the library name appears.
modules/concurrency-limit/build.gradle	17	high	New runtime dependency added: 'com.netflix.concurrency-limits:concurrency-limits-core:0.4.0'. Per mandatory supply chain policy, this external artifact must be independently verified against the SHA1 checksum provided (9bf32c330cad19a6e564b460a65132aa765b6440) and confirmed against the official Netflix GitHub release. Namespace/typosquatting attacks cannot be ruled out without out-of-band verification.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 2 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[philiplhchan]

Thanks for spending the time and effort to upstream this change. This has shown great results internally on our largest clusters, hopefully the community can benefit from it as well.

Left a few small comments.

[msfroh]

Regarding the issues flagged in #22312 (comment):

New external dependency version introduced: 'netflix_concurrency_limits = "0.4.0"'. Per mandatory supply chain policy, any new dependency entry must be flagged for maintainer verification of artifact authenticity and integrity regardless of how legitimate the library name appears.

New runtime dependency added: 'com.netflix.concurrency-limits:concurrency-limits-core:0.4.0'. Per mandatory supply chain policy, this external artifact must be independently verified against the SHA1 checksum provided (9bf32c330cad19a6e564b460a65132aa765b6440) and confirmed against the official Netflix GitHub release. Namespace/typosquatting attacks cannot be ruled out without out-of-band verification.

I've verified the authenticity of the library (https://github.com/Netflix/concurrency-limits) and that 0.4.0 is a legitimate, official version. We might want to upgrade to 0.5.4, since it's the latest release, but the changes since 0.4.0 are fairly minimal.

I also verified the SHA1 checksum matches the one from repo1.maven.org: https://repo1.maven.org/maven2/com/netflix/concurrency-limits/concurrency-limits-core/0.4.0/concurrency-limits-core-0.4.0.jar.sha1, which was last updated in 2023. (So, unless maven.org was hacked with plausible-looking last-modified timestamps, it should be fine.)

I'm adding the skip-diff-analyzer label as a confirmation that the new dependencies look legit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit cf8ecf7)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add ActionConcurrencyLimiterStats to NodeStats and wire plugin SPI Relevant files: server/src/main/java/org/opensearch/action/ActionConcurrencyLimiterStats.java server/src/main/java/org/opensearch/action/admin/cluster/node/stats/NodeStats.java server/src/main/java/org/opensearch/node/NodeService.java server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java server/src/test/java/org/opensearch/node/NodeServiceConcurrencyLimiterTests.java server/src/main/java/org/opensearch/plugins/ConcurrencyLimiterStatsPlugin.java test/framework/src/main/java/org/opensearch/cluster/MockInternalClusterInfoService.java Sub-PR theme: Core concurrency-limit module (registry, filter, plugin, algorithms) Relevant files: modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimitFilter.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimitPlugin.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ConcurrencyLimitSettings.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ConcurrencyLimitMode.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ConcurrencyLimitResolverType.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/LimiterRequestContext.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/PartitionResolver.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/OpenSearchVegasLimit.java modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/BurstAwareLimit.java Sub-PR theme: Push-telemetry publisher for concurrency limiters Relevant files: modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ConcurrencyLimiterMetricsPublisher.java modules/concurrency-limit/src/test/java/org/opensearch/concurrency/ConcurrencyLimiterMetricsPublisherTests.java

⚡ Recommended focus areas for review Warmup Bypass Rejection Counting In tryAcquire, when in enforced mode and the underlying limiter rejects the acquire during warmup (token is empty), the code returns a NOOP listener without incrementing totalRejected. This is intentional pass-through behavior, but subsequently on the acquired branch inFlight is not incremented for these pass-through requests either. However, because the underlying limiter refused the acquire, the counters remain consistent. The concern: the returned NOOP_LISTENER means when the request completes there is no inFlight decrement — but there was no increment either, so this is fine. Confirm the semantics are as intended: warmup pass-throughs are invisible to metrics and don't contribute to inflight tracking, which may under-report load during warmup. if (this.mode == ConcurrencyLimitMode.MONITOR_ONLY) { totalRejected.incrementAndGet(); return Optional.of(NOOP_LISTENER); } // enforced: reject after warmup, pass through during warmup if (!s.isWarmedUp()) return Optional.of(NOOP_LISTENER); totalRejected.incrementAndGet(); return Optional.empty(); } Publish Order During Reconfigure In reconfigure, state.set(newState) is written before this.mode = cfg.mode. The comment claims the opposite ordering ("Write state BEFORE mode so that if another thread reads mode and sees a non-disabled value, state is guaranteed to be non-null"), but a reader on the hot path in tryAcquire reads state first (not mode), so if a transition goes disabled→enforced, a concurrent reader could see the new state via the AtomicReference while mode still reads DISABLED. The mode field is then read inside tryAcquire only for the MONITOR_ONLY branch after rejection, so the actual practical impact is limited, but the stated invariant does not match the code and may confuse future maintainers. // All construction succeeded — now commit. Write state BEFORE mode so that if // another thread reads mode (volatile) and sees a non-disabled value, state is // guaranteed to be non-null. this.actionName = cfg.actionName; this.algorithm = cfg.algorithm; this.partitioned = newPartitioned; if (newState != null) { LimiterState prev = state.get(); if (prev != null && prev.isWarmedUp()) { newState.warmedUp = true; } state.set(newState); } else { state.set(null); } this.mode = cfg.mode; } Reconfigure Failure Leaves Partial State onAliasReconfigured catches RuntimeException from inst.reconfigure(cfg) and logs a message about "keeping previous configuration". However reconfigure mutates instance state after constructing newState; the ordering claim in the comment is that construction happens first so a throw leaves the instance untouched. But buildLimiter (called during construction) validates partition percentages and can throw — which is safe. If a different validation is added later that runs after state.set(...), previous-configuration invariant would silently break. Consider isolating all validation strictly before any field mutation to preserve the documented guarantee. private synchronized void onAliasReconfigured(String alias, AliasConfig cfg) { boolean reconfigured = true; if (cfg.actionName.isEmpty()) { limiters.remove(alias); } else { ActionLimiterInstance inst = limiters.computeIfAbsent(alias, ActionLimiterInstance::new); try { inst.reconfigure(cfg); } catch (RuntimeException e) { reconfigured = false; LOG.error( "Failed to apply concurrency limit config for alias [{}] action [{}]; " + "keeping previous configuration. Error: {}", alias, cfg.actionName, e.getMessage() ); } } rebuildReverseMap(); if (reconfigured) { notifyMetricsListener(alias); } } Version Gate Ordering The read path for concurrencyLimiterStats uses onOrAfter(Version.V_3_8_0) while the surrounding V_3_7_0 branch reads a discarded AnalyticsBackendNativeMemoryStats. Confirm that on the write path, the corresponding V_3_8_0 write is placed in the same relative wire position on both sides (writeTo and constructor). The diff shows writeOptionalWriteable(concurrencyLimiterStats) after the V_3_7_0 null write and before the totalEstimatedNativeBytes write — this matches the read order, but any future field insertion between these gates must preserve this ordering or the wire format will silently corrupt. if (in.getVersion().onOrAfter(Version.V_3_8_0)) { concurrencyLimiterStats = in.readOptionalWriteable(ActionConcurrencyLimiterStats::new); } else { concurrencyLimiterStats = null; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to cf8ecf7

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Release token on Errors too Catching only Exception misses Error (e.g. OutOfMemoryError, AssertionError) thrown by chain.proceed, which would leak the token and permanently reduce capacity. Catch Throwable here to guarantee token release in all failure modes, then rethrow. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimitFilter.java [109-112] -} catch (Exception e) { +} catch (Throwable e) { if (released.compareAndSet(false, true)) limitToken.onIgnore(); throw e; } Suggestion importance[1-10]: 6 __ Why: Catching Throwable ensures token release for Error cases like OutOfMemoryError, preventing capacity leaks. This is a legitimate robustness improvement, though Error conditions typically indicate unrecoverable states.	Low
	Snapshot volatile config for coherent reads cfg.partitions is a volatile field updated concurrently by cluster-settings consumers; reading it multiple times inside reconfigure (here and again in buildLimiter) can produce inconsistent snapshots. Snapshot both cfg.partitions and cfg.partitionConfig once at method entry and use those local copies throughout to ensure a coherent view. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [203-208] synchronized void reconfigure(AliasConfig cfg) { - // Build the new state into a local FIRST. If construction throws (e.g. invalid - // partition percentages), no instance field is mutated and the caller's try/catch - // leaves this instance fully on its previous configuration. + // Snapshot volatile fields so all downstream logic sees a coherent view. + List<String> partitionsSnapshot = cfg.partitions; LimiterState newState = null; - boolean newPartitioned = !cfg.partitions.isEmpty(); + boolean newPartitioned = !partitionsSnapshot.isEmpty(); Suggestion importance[1-10]: 3 __ Why: reconfigure is called under synchronization, and reconfigure calls are serialized via the outer onAliasReconfigured synchronized method, so the coherence concern is less critical. The suggestion is only partial (does not snapshot partitionConfig).	Low
	Make drift check arithmetic explicit The drift-prevention check inflight * (upDriftFactor + 1) < estimatedLimit uses integer multiplication on the left side, but upDriftFactor is a double. Java will perform floating-point multiplication here, which is fine, but inflight * (upDriftFactor + 1) may not exactly equal the arithmetic in tests/docs which describe it as inflight * 2. Confirm precedence: inflight * (upDriftFactor + 1) should be evaluated as inflight * (upDriftFactor + 1.0) — parenthesize explicitly to make the double promotion unambiguous, and cast the comparison to avoid subtle rounding at large values. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/OpenSearchVegasLimit.java [283-286] -} else if (inflight * (upDriftFactor + 1) < estimatedLimit) { +} else if ((double) inflight * (upDriftFactor + 1.0) < estimatedLimit) { increaseConsecutive = 0; decreaseConsecutive = 0; return (int) estimatedLimit; } else { alpha = (int) (upDriftFactor * alphaFunc.applyAsInt((int) estimatedLimit)); Suggestion importance[1-10]: 2 __ Why: Java already promotes inflight to double when multiplied with upDriftFactor + 1 (a double), so the existing code is semantically correct. The suggested change is a readability tweak rather than a fix.	Low
Possible issue	Avoid NPE from race in consumer lookups There is a race between computeIfAbsent and aliasConfigs.get(alias): another thread could remove the alias between the two calls, causing a NullPointerException in onAliasReconfigured. Capture the returned reference from computeIfAbsent in a local variable and pass it directly to onAliasReconfigured, as already done in the LIMIT_CONFIG, AIMD_CONFIG, and other consumers. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [444-455] clusterSettings.addAffixUpdateConsumer(ConcurrencyLimitSettings.ACTION_NAME, (alias, val) -> { - aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()).actionName = val; - onAliasReconfigured(alias, aliasConfigs.get(alias)); + AliasConfig cfg = aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()); + cfg.actionName = val; + onAliasReconfigured(alias, cfg); }, (alias, val) -> {}); clusterSettings.addAffixUpdateConsumer(ConcurrencyLimitSettings.MODE, (alias, val) -> { - aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()).mode = ConcurrencyLimitMode.fromName(val); - onAliasReconfigured(alias, aliasConfigs.get(alias)); + AliasConfig cfg = aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()); + cfg.mode = ConcurrencyLimitMode.fromName(val); + onAliasReconfigured(alias, cfg); }, (alias, val) -> {}); Suggestion importance[1-10]: 5 __ Why: The suggestion improves consistency with other consumers by capturing the returned reference from computeIfAbsent. However, since aliasConfigs is never removed from (only put into), the NPE risk described is largely theoretical. Still, it's a minor code quality improvement.	Low
	Version-gate new stats metric for BWC Adding a new enum constant to Metric is a wire-protocol/serialization concern. Ensure the new CONCURRENCY_LIMITER value is only requested/serialized when all nodes in the cluster support it (version-gated), otherwise older nodes receiving this metric name will fail to parse the request and break mixed-version clusters during rolling upgrades. server/src/main/java/org/opensearch/action/admin/cluster/node/stats/NodesStatsRequest.java [232-233] NATIVE_MEMORY("native_memory"), -CONCURRENCY_LIMITER("concurrency_limiter"); +CONCURRENCY_LIMITER("concurrency_limiter"); // TODO: version-gate on wire before enabling in mixed clusters Suggestion importance[1-10]: 5 __ Why: Raises a legitimate BWC concern about adding a new Metric enum value that could affect mixed-version clusters during rolling upgrades, but the improved_code only adds a TODO comment rather than implementing a real version gate, limiting its concrete impact.	Low

---

Previous suggestions

Suggestions up to commit 965ec8f

Category	Suggestion	Impact
General	Validate probeMultiplier is at least 1 When probeMultiplier is set to 0 via the builder, shouldProbe() returns true on every sample (0 <= probeCount), causing the algorithm to reset every probeCount and never call updateEstimatedLimit. Add validation in the builder to require probeMultiplier >= 1, matching the intent of the existing probeInflightThreshold validation. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/OpenSearchVegasLimit.java [111-114] -private boolean shouldProbe() { - return probeJitter * probeMultiplier * estimatedLimit <= probeCount; +public Builder probeMultiplier(int probeMultiplier) { + if (probeMultiplier < 1) { + throw new IllegalArgumentException("probeMultiplier must be >= 1 but got " + probeMultiplier); + } + this.probeMultiplier = probeMultiplier; + return this; } Suggestion importance[1-10]: 5 __ Why: Adding validation for probeMultiplier >= 1 prevents pathological configuration where every sample triggers a probe reset, which is a valid input-validation improvement consistent with other builder checks.	Low
	Fail fast on invalid initial config Initial config load calls onAliasReconfigured which invokes inst.reconfigure(cfg), but if cfg contains invalid partition percentages or other invalid values in the initial settings (e.g. from a node config file), the exception is caught and logged but the node still starts with a silently-broken alias. Consider validating initial settings up-front and failing node startup on invalid concurrency-limit configuration, consistent with how other setting validators reject bad values at update time. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [433-441] // Load all aliases present in initial settings Set<String> aliases = new HashSet<>(); for (Setting<?> s : ConcurrencyLimitSettings.ALL_SETTINGS) { aliases.addAll(((Setting.AffixSetting<?>) s).getNamespaces(initialSettings)); } for (String alias : aliases) { AliasConfig cfg = buildConfig(alias, initialSettings); + // Validate up-front so bad node-level settings fail startup rather than + // silently landing in a "previous config retained" state with no previous config. + ConcurrencyLimitSettings.validatePartitionConfig(cfg.partitions, cfg.partitionConfig); aliasConfigs.put(alias, cfg); onAliasReconfigured(alias, cfg); } Suggestion importance[1-10]: 5 __ Why: Failing startup on invalid initial concurrency-limit config is a reasonable robustness improvement, aligning behavior with the dynamic-update validation path, though it changes semantics and may need broader consideration.	Low
Possible issue	Guard against state/mode visibility race The comment claims state is written before mode so a reader seeing a non-disabled mode is guaranteed to see non-null state, but tryAcquire reads state first and then checks mode, and there is no memory barrier ordering here. Under concurrent reads, a thread could observe mode != DISABLED while state is still null (or vice versa). Consider using a single atomic/volatile holder that carries both state and mode together, or read mode before state and handle null state explicitly. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [222-234] this.actionName = cfg.actionName; this.algorithm = cfg.algorithm; this.partitioned = newPartitioned; if (newState != null) { LimiterState prev = state.get(); if (prev != null && prev.isWarmedUp()) { newState.warmedUp = true; } state.set(newState); } else { state.set(null); } +// mode is written last; readers must tolerate a transient (mode != DISABLED, state == null) +// window — tryAcquire() already guards against null state. this.mode = cfg.mode; Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential ordering concern but the improved_code only adds a comment without a real fix; tryAcquire already handles null state gracefully, so the practical impact is low.	Low
	Verify inFlight accounting on NOOP paths inst.tryAcquire may return Optional.of(NOOP_LISTENER) during warmup or in monitor_only mode. The caller in ActionConcurrencyLimitFilter then invokes limitToken.onSuccess()/onDropped() on this NOOP listener, which is safe, but it also increments inFlight inside ActionLimiterInstance.tryAcquire unconditionally on token acquisition. For the monitor_only rejection path (line returning NOOP_LISTENER after totalRejected.incrementAndGet()), inFlight is NOT incremented, but the caller will still eventually invoke the listener's onSuccess/onDropped, which are NOOPs — however the wrapping listener in tryAcquire that decrements inFlight is only created when token is non-empty from the inner limiter. Verify that the monitor_only NOOP path does not run through the inFlight-decrementing wrapper, otherwise inFlight will go negative. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [554-560] +public AcquireResult tryAcquire(String action, Task task, ActionRequest request) { + ActionLimiterInstance inst = actionToInstance.get(action); + if (inst == null) return AcquireResult.NO_LIMITER; + Optional<Limiter.Listener> token = inst.tryAcquire(task, request); + if (token.isEmpty()) return AcquireResult.REJECTED; + return AcquireResult.acquired(token.get()); +} - Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify behavior without proposing a concrete change (improved_code equals existing_code), and the concern appears unfounded since NOOP listeners are returned before inFlight.incrementAndGet().	Low

Suggestions up to commit bf67dd5

Category	Suggestion	Impact
Possible issue	Fix mode/state visibility race in reconfigure There is a race in tryAcquire: it reads state first and then mode for the MONITOR_ONLY check. During reconfigure, state may be set to null while mode is still non-DISABLED (from a prior config), causing tryAcquire to return Optional.empty() and produce spurious rejections. Read mode before state, or set mode to DISABLED before nulling state, to preserve the ordering invariant described in the comment. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [219-234] -// All construction succeeded — now commit. Write state BEFORE mode so that if -// another thread reads mode (volatile) and sees a non-disabled value, state is -// guaranteed to be non-null. +// All construction succeeded — now commit. If we're clearing state (disabled), +// set mode to DISABLED first so readers never see (mode!=DISABLED, state==null). this.actionName = cfg.actionName; this.algorithm = cfg.algorithm; this.partitioned = newPartitioned; if (newState != null) { LimiterState prev = state.get(); if (prev != null && prev.isWarmedUp()) { newState.warmedUp = true; } state.set(newState); + this.mode = cfg.mode; } else { + this.mode = cfg.mode; state.set(null); } -this.mode = cfg.mode; Suggestion importance[1-10]: 7 __ Why: Valid concern about a race condition where tryAcquire could observe state==null with mode!=DISABLED during reconfigure, potentially causing spurious behavior. The ordering fix is reasonable, though the actual impact depends on how tryAcquire handles null state (it returns Optional.empty() which the caller treats as rejection).	Medium
General	Avoid truncation of fractional drift factor The drift check inflight * (upDriftFactor + 1) < estimatedLimit multiplies an int by a double and then compares to a double, which is fine, but the surrounding math is fragile: alpha, beta, and threshold are cast to int via (int) (upDriftFactor * …) which truncates fractional values. With upDriftFactor=1.5 and alphaFunc returning 3, alpha becomes 4 instead of 4.5, and small values can round to 0. Use Math.round or keep them as double to avoid silent truncation that undermines the fractional-factor feature the builder advertises. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/OpenSearchVegasLimit.java [288-290] } else if (inflight * (upDriftFactor + 1) < estimatedLimit) { increaseConsecutive = 0; decreaseConsecutive = 0; return (int) estimatedLimit; } else { - alpha = (int) (upDriftFactor * alphaFunc.applyAsInt((int) estimatedLimit)); + alpha = (int) Math.round(upDriftFactor * alphaFunc.applyAsInt((int) estimatedLimit)); + beta = (int) Math.round(upDriftFactor * betaFunc.applyAsInt((int) estimatedLimit)); + threshold = (int) Math.round(upDriftFactor * thresholdFunc.applyAsInt((int) estimatedLimit)); Suggestion importance[1-10]: 6 __ Why: Legitimate concern about truncation of fractional upDriftFactor values when computing alpha/beta/threshold. Using Math.round would better preserve the intent of fractional factors, though the existing test testFractionalUpDriftFactorNotTruncated shows the current implementation still produces divergent behavior.	Low
	Clean up alias config on removal When action_name is cleared (set to empty), onAliasReconfigured removes the limiter from limiters but the stale AliasConfig remains in aliasConfigs. Subsequent affix updates for the same alias will resurrect a partially-configured limiter with defaults for fields other than the one being changed. Remove the alias config entry when action_name becomes empty to keep the two maps consistent. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [444-447] clusterSettings.addAffixUpdateConsumer(ConcurrencyLimitSettings.ACTION_NAME, (alias, val) -> { - aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()).actionName = val; - onAliasReconfigured(alias, aliasConfigs.get(alias)); + AliasConfig cfg = aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()); + cfg.actionName = val; + onAliasReconfigured(alias, cfg); + if (val == null || val.isEmpty()) { + aliasConfigs.remove(alias); + } }, (alias, val) -> {}); Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that stale AliasConfig entries remain in aliasConfigs after clearing action_name. However, the impact is minor since re-enabling explicitly re-populates fields, as demonstrated by the passing testClearingActionNameRemovesLimiterAndAllowsReactivation test.	Low
	Guard against uninitialized registry registry is assigned in the non-telemetry createComponents overload and read here without any memory-visibility guarantee if the two overloads were ever invoked from different threads. It is declared volatile which is good, but there is no null-check: if for any reason the telemetry-aware overload runs first or the non-telemetry one is skipped, this will NPE. Add a defensive null-check and log/skip rather than crash node startup. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimitPlugin.java [134-136] ConcurrencyLimiterMetricsPublisher publisher = new ConcurrencyLimiterMetricsPublisher(metricsRegistry); -registry.setMetricsListener(publisher); +if (registry != null) { + registry.setMetricsListener(publisher); +} return List.of(publisher); Suggestion importance[1-10]: 3 __ Why: The defensive null check is minor since the node lifecycle guarantees the non-telemetry createComponents runs before the telemetry-aware one, as documented in the code. The suggestion masks a real bug (registry not initialized) rather than fixing it.	Low
	Verify new metric registration completeness The new CONCURRENCY_LIMITER enum constant introduces a new metric name that must be kept in sync with client-side parsing and REST API contracts. Verify that the metric name "concurrency_limiter" is also registered in any allow-list or documentation that enumerates valid _nodes/stats metrics to avoid IllegalArgumentException when users request it. server/src/main/java/org/opensearch/action/admin/cluster/node/stats/NodesStatsRequest.java [233] +NATIVE_MEMORY("native_memory"), +CONCURRENCY_LIMITER("concurrency_limiter"); - Suggestion importance[1-10]: 3 __ Why: The suggestion only asks to verify registration and does not propose a concrete change; existing_code and improved_code are identical, giving minimal actionable value.	Low

Suggestions up to commit 72b0aee

Category	Suggestion	Impact
Possible issue	Remove new instance on reconfigure failure When computeIfAbsent creates a brand-new ActionLimiterInstance and inst.reconfigure(cfg) throws, the newly-created instance is left in the limiters map in an uninitialized state (no state, null mode/actionName). Subsequent hasLimiterFor/tryAcquire calls would see a broken entry. Track whether the instance was newly created and remove it on failure to preserve the "keep previous configuration" invariant. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [673-695] private synchronized void onAliasReconfigured(String alias, AliasConfig cfg) { boolean reconfigured = true; if (cfg.actionName.isEmpty()) { limiters.remove(alias); } else { + boolean created = !limiters.containsKey(alias); ActionLimiterInstance inst = limiters.computeIfAbsent(alias, ActionLimiterInstance::new); try { inst.reconfigure(cfg); } catch (RuntimeException e) { reconfigured = false; + if (created) { + limiters.remove(alias); + } LOG.error( "Failed to apply concurrency limit config for alias [{}] action [{}]; " + "keeping previous configuration. Error: {}", alias, cfg.actionName, e.getMessage() ); } } rebuildReverseMap(); if (reconfigured) { notifyMetricsListener(alias); } } Suggestion importance[1-10]: 7 __ Why: Valid concern: if reconfigure throws on the first configuration attempt, an empty ActionLimiterInstance remains in the map with null mode/actionName, potentially causing NPEs in rebuildReverseMap (which calls inst.getActionName().isEmpty()). The fix is correct and prevents leaking a broken entry.	Medium
General	Ensure visibility of probe jitter field resetProbeJitter is called from _update which is invoked concurrently by the limiter on RTT samples. Randomness.get() returns a thread-local Random in tests but a shared one in production; more importantly, probeJitter itself is a non-volatile field mutated without synchronization while shouldProbe() reads it. Since _update in AbstractLimit is typically synchronized, verify or make this state's mutation/visibility explicit to avoid stale reads across threads. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/OpenSearchVegasLimit.java [227-230] +private volatile double probeJitter; + private void resetProbeJitter() { Random rng = Randomness.get(); probeJitter = 0.5 + rng.nextDouble() * 0.5; } Suggestion importance[1-10]: 3 __ Why: AbstractLimit._update is typically invoked under synchronization by the framework, so the concern about visibility may be moot. The suggestion asks the author to verify, which is a low-impact improvement.	Low
	Preserve monitor_only pass-through when state null When state is null but mode is MONITOR_ONLY (e.g. transient state between reconfigure steps or if disabled path is taken while mode still reads MONITOR_ONLY), the method returns Optional.empty() which the caller interprets as a hard rejection. In monitor_only mode, rejections should never propagate as HTTP 429. Guard by checking mode before returning empty so monitor_only pass-through is preserved even when state is transiently null. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [249-257] Optional<Limiter.Listener> tryAcquire(Task task, ActionRequest request) { LimiterState s = state.get(); - if (s == null) return Optional.empty(); + if (s == null) { + return this.mode == ConcurrencyLimitMode.MONITOR_ONLY ? Optional.of(NOOP_LISTENER) : Optional.empty(); + } // Only allocate the context object when the limiter is partitioned and actually // needs it to resolve a partition. The non-partitioned SimpleLimiter ignores it. LimiterRequestContext ctx = partitioned ? new LimiterRequestContext(task, actionName, request) : null; Optional<Limiter.Listener> token = s.limiter.acquire(ctx); Suggestion importance[1-10]: 2 __ Why: In reconfigure, state is written before mode, and state is only null when mode is DISABLED. The transient-state concern described is unlikely given the write ordering in the code. The suggestion is speculative.	Low

Suggestions up to commit 42bcef8

Category	Suggestion	Impact
Possible issue	Always propagate drop signal to limit update When didDrop == true but rtt < rtt_noload (or rtt_noload == 0), the current code falls into the else-branch, updates rtt_noload to the new minimum, and returns without applying the drop-triggered decrease. A drop signal should always propagate to updateEstimatedLimit regardless of RTT comparison, otherwise the algorithm ignores a hard congestion signal. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/OpenSearchVegasLimit.java [263-270] -} else if (this.rtt_noload != 0L && rtt >= this.rtt_noload) { +} else if ((this.rtt_noload != 0L && rtt >= this.rtt_noload) || didDrop) { this.rttSampleListener.addSample(this.rtt_noload); return this.updateEstimatedLimit(rtt, inflight, didDrop); } else { LOG.debug("New MinRTT {}", (double) TimeUnit.NANOSECONDS.toMicros(rtt) / (double) 1000.0F); this.rtt_noload = rtt; return (int) this.estimatedLimit; } Suggestion importance[1-10]: 7 __ Why: Valid concern — a drop signal with a low RTT could bypass the decrease logic in updateEstimatedLimit, potentially missing a congestion signal. This affects algorithm correctness in edge cases, though matches original Netflix Vegas behavior.	Medium
General	Prevent token leak on Throwable The catch only handles Exception, but if chain.proceed throws a Throwable (e.g. Error or an AssertionError in tests), the token will leak (never released) because the finally-like release logic is bypassed. Consider catching Throwable to ensure the token is always released on synchronous failure of chain.proceed. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimitFilter.java [109-112] -} catch (Exception e) { +} catch (Throwable e) { if (released.compareAndSet(false, true)) limitToken.onIgnore(); throw e; } Suggestion importance[1-10]: 5 __ Why: Catching Throwable would prevent token leaks on Error cases, though such scenarios are rare in production. This is a defensive coding improvement of moderate value.	Low
	Clean up alias config on clear When action_name is cleared (via putNull), the consumer receives the default empty string and onAliasReconfigured removes the limiter from limiters, but the AliasConfig entry remains in aliasConfigs. Subsequent consumer callbacks for the same alias (e.g. mode default firing) will re-computeIfAbsent and could resurrect a stale config. Remove the aliasConfigs entry when actionName becomes empty to keep both maps consistent. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [444-447] clusterSettings.addAffixUpdateConsumer(ConcurrencyLimitSettings.ACTION_NAME, (alias, val) -> { - aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()).actionName = val; - onAliasReconfigured(alias, aliasConfigs.get(alias)); + AliasConfig cfg = aliasConfigs.computeIfAbsent(alias, k -> new AliasConfig()); + cfg.actionName = val; + onAliasReconfigured(alias, cfg); + if (val.isEmpty()) { + aliasConfigs.remove(alias); + } }, (alias, val) -> {}); Suggestion importance[1-10]: 4 __ Why: Minor cleanup — the stale AliasConfig entry doesn't cause functional issues since onAliasReconfigured correctly removes the limiter when actionName is empty, and subsequent consumer firings would rebuild the config properly. The test testClearingActionNameRemovesLimiterAndAllowsReactivation confirms current behavior works.	Low
	Detect multiple stats providers If multiple plugins implement ConcurrencyLimiterStatsPlugin, silently using only the first one hides configuration errors. Consider logging a warning or throwing an IllegalStateException when more than one provider is registered, to make the misconfiguration visible. server/src/main/java/org/opensearch/node/Node.java [2481-2482] +if (providers.size() > 1) { + throw new IllegalStateException("Multiple ConcurrencyLimiterStatsPlugin providers registered: " + providers); +} ConcurrencyLimiterStatsPlugin provider = providers.getFirst(); return provider::getConcurrencyLimiterStats; Suggestion importance[1-10]: 3 __ Why: Minor defensive improvement — logging or failing on multiple providers would surface misconfiguration, but in practice only one module implements this interface, so the impact is low.	Low

Suggestions up to commit 66a8a3b

Category	Suggestion	Impact
Possible issue	Remove half-initialized limiter on reconfigure failure When computeIfAbsent creates a brand-new ActionLimiterInstance and reconfigure throws, the empty instance is left in the limiters map with null/default fields. Subsequent getStats() calls or reverse-map rebuilds may observe this half-initialized instance. Track whether the instance was newly created and remove it from the map on reconfigure failure so no stale, uninitialized entry remains. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [678-689] +boolean created = !limiters.containsKey(alias); ActionLimiterInstance inst = limiters.computeIfAbsent(alias, ActionLimiterInstance::new); try { inst.reconfigure(cfg); } catch (RuntimeException e) { reconfigured = false; + if (created) { + limiters.remove(alias); + } LOG.error( "Failed to apply concurrency limit config for alias [{}] action [{}]; " + "keeping previous configuration. Error: {}", alias, cfg.actionName, e.getMessage() ); } Suggestion importance[1-10]: 7 __ Why: Valid concern: computeIfAbsent inserts a new empty ActionLimiterInstance before reconfigure runs, and if reconfigure throws, an uninitialized instance remains in the map with default null fields, which could affect subsequent stats or reverse-map builds. However, the note about containsKey before computeIfAbsent has a race, but the enclosing method is synchronized, so it's safe.	Medium
	Synchronize delegate change callback to prevent race The delegate.notifyOnChange callback reads/writes burstOpen and calls updateLimit without holding the synchronized lock used by onSample. If the delegate fires the callback from another thread concurrently with onSample, burstOpen reads and limit writes race. Synchronize the callback path on the same monitor to keep state consistent. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/BurstAwareLimit.java [51-61] public BurstAwareLimit(AbstractLimit delegate, int burstCapacity, int closeThreshold, int recoveryThreshold) { this.delegate = delegate; this.burstCapacity = burstCapacity; this.closeThreshold = closeThreshold; this.recoveryThreshold = recoveryThreshold; this.limit = delegate.getLimit() + burstCapacity; delegate.notifyOnChange(newBase -> { - int newEffective = newBase + (burstOpen ? burstCapacity : 0); - updateLimit(newEffective); + synchronized (this) { + int newEffective = newBase + (burstOpen ? burstCapacity : 0); + updateLimit(newEffective); + } }); } Suggestion importance[1-10]: 7 __ Why: Legitimate concurrency concern: the notifyOnChange callback modifies limit and reads burstOpen without holding the same monitor used by onSample. If the delegate ever fires the callback from a thread concurrent with onSample, state could race. Synchronizing the callback is a low-cost safety improvement.	Medium
General	Validate all builder parameters for range Other builder parameters (e.g. upDriftFactor, increaseHysteresis, decreaseHysteresis, smoothing, probeMultiplier) lack any input validation, so invalid values (negative, zero, or fractional < 1.0) silently produce incorrect algorithm behavior. Add range checks in each setter to fail fast at configuration time, matching the pattern used for probeInflightThreshold. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/limit/OpenSearchVegasLimit.java [125-131] public Builder probeInflightThreshold(double probeInflightThreshold) { if (probeInflightThreshold < 0.0 || probeInflightThreshold > 1.0) { throw new IllegalArgumentException("probeInflightThreshold must be in [0.0, 1.0] but got " + probeInflightThreshold); } this.probeInflightThreshold = probeInflightThreshold; return this; } +public Builder upDriftFactor(double upDriftFactor) { + if (upDriftFactor < 1.0) { + throw new IllegalArgumentException("upDriftFactor must be >= 1.0 but got " + upDriftFactor); + } + this.upDriftFactor = upDriftFactor; + return this; +} + Suggestion importance[1-10]: 4 __ Why: Adding validation for other builder parameters is a reasonable defensive improvement, but the settings validators in ConcurrencyLimitSettings already validate these values before they reach the builder in the normal configuration path. The impact is limited.	Low
	Improve error message for invalid algorithm value LimitAlgorithm.valueOf(cfg.algorithm.toUpperCase(...)) throws IllegalArgumentException if the algorithm value comes from an unvalidated source (settings validator only checks at update time, but initial settings load bypasses validateXxx paths). Wrap with a more descriptive error so a bad configured value produces a clear message rather than a raw enum valueOf error. modules/concurrency-limit/src/main/java/org/opensearch/concurrency/ActionConcurrencyLimiterRegistry.java [209-210] synchronized void reconfigure(AliasConfig cfg) { - // Build the new state into a local FIRST. If construction throws (e.g. invalid - // partition percentages), no instance field is mutated and the caller's try/catch - // leaves this instance fully on its previous configuration. LimiterState newState = null; boolean newPartitioned = !cfg.partitions.isEmpty(); if (cfg.mode != ConcurrencyLimitMode.DISABLED) { - LimitAlgorithm algo = LimitAlgorithm.valueOf(cfg.algorithm.toUpperCase(Locale.ROOT)); + LimitAlgorithm algo; + try { + algo = LimitAlgorithm.valueOf(cfg.algorithm.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + "Unknown algorithm [" + cfg.algorithm + "] for alias; must be one of: vegas, gradient2, aimd" + ); + } Suggestion importance[1-10]: 3 __ Why: The ALGORITHM setting already validates via the setter's parser (checks against known values in the Setting definition), so this is largely a cosmetic error-message improvement rather than a correctness fix.	Low

[github-actions]

❌ Gradle check result for 56b241d: 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?

[msfroh]

@bpatelcs -- Can you please check the items in #22312 (comment) and #22312 (comment)?

Also, please run ./gradlew spotlessApply to format your code, since the build is currently failing the formatter checks. You can run ./gradlew precommit to run all "quick" checks before pushing.

[github-actions]

Persistent review updated to latest commit fde61ad

[github-actions]

Persistent review updated to latest commit ff2e64c

[github-actions]

Persistent review updated to latest commit 2ecb5b1

[github-actions]

Persistent review updated to latest commit d42b28c

[bpatelcs]

@bpatelcs -- Can you please check the items in #22312 (comment) and #22312 (comment)?

Also, please run ./gradlew spotlessApply to format your code, since the build is currently failing the formatter checks. You can run ./gradlew precommit to run all "quick" checks before pushing.

I went through the few iterations and all the new suggestions are either false positives or as design.

[github-actions]

❌ Gradle check result for d42b28c: TIMEOUT

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?

[github-actions]

Persistent review updated to latest commit 66a8a3b

[github-actions]

❌ Gradle check result for 66a8a3b: 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?

[github-actions]

Persistent review updated to latest commit 42bcef8

[github-actions]

✅ Gradle check result for 42bcef8: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.43%. Comparing base (599785a) to head (cf8ecf7).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
server/src/main/java/org/opensearch/node/Node.java	50.00%	2 Missing and 1 partial ⚠️
...rch/action/admin/cluster/node/stats/NodeStats.java	77.77%	1 Missing and 1 partial ⚠️
.../cluster/node/stats/TransportNodesStatsAction.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22312      +/-   ##
============================================
- Coverage     71.54%   71.43%   -0.12%     
+ Complexity    77023    76951      -72     
============================================
  Files          6153     6156       +3     
  Lines        358354   358430      +76     
  Branches      52237    52248      +11     
============================================
- Hits         256399   256032     -367     
- Misses        81586    82008     +422     
- Partials      20369    20390      +21     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 72b0aee

[github-actions]

Persistent review updated to latest commit bf67dd5

[github-actions]

Persistent review updated to latest commit 965ec8f

[github-actions]

❌ Gradle check result for 965ec8f: 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?

[github-actions]

Persistent review updated to latest commit cf8ecf7

[github-actions]

❌ Gradle check result for cf8ecf7: 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?

[github-actions]

✅ Gradle check result for cf8ecf7: SUCCESS