Queue WLM requests denied by a throttle limit instead of rejecting by dzane17 · Pull Request #11 · dzane17/OpenSearch

dzane17 · 2026-08-12T21:21:42Z

Summary

Adds request queueing on top of the shipped WLM throttling. When a search exceeds its workload group's throttle limit, instead of an immediate HTTP 429 the coordinator parks it in a bounded queue and admits it once a permit frees. Scope: throttle-limit denials only (resource-limit rejection is unchanged).

Base branch: 3.7-wlm-throttling.

How it works

Config — a new queue object on the workload group, sibling of throttling, with a single setting size_per_bucket (default 0 = queueing disabled; over-limit requests are rejected immediately as before). The cap is per throttle bucket, mirroring how node_limit / shared_limit are themselves per-bucket, so with attribute=username/role one principal's flood cannot consume another principal's queue capacity. For attribute=group there is a single bucket, so it is simply the group's queue depth. Wire-gated at V_3_7_0.
Fixed per-group ceiling — above the per-bucket cap sits a non-configurable ceiling (MAX_GROUP_QUEUE_DEPTH = 10,000) on the total parked requests across all of a group's buckets on one coordinator. It is a footprint backstop, not a fairness knob: username/role bucket keys are derived from the request principal and so are attacker-controlled, and a purely per-bucket cap would let unbounded distinct principals each allocate size_per_bucket slots. size_per_bucket is validated against the same value, so no configured depth is silently unreachable. Note the per-bucket fairness is therefore bounded rather than absolute: at the ceiling, admission reverts to first-come-first-served across buckets.
Coordinator-side parking — a denied request's ActionListener is held (no thread consumed, only the listener + open connection). On the shared tier the request is parked before the owner acquire ("enqueue-first"): the owner registers the coordinator as a waiter inside handleAcquire, before the reply is even sent, so parking only after the denial left a window in which a racing grant found an empty queue, returned the slot as unused, and deregistered the coordinator — stranding the request, with no node-tier drain to recover it on a shared-only group. Because the request is already queued, a granted acquire drains the oldest queued request for the bucket (FIFO) rather than running its own; shared_limit remains gated solely by the owner's tryAcquire, so this cannot over-admit.
Draining — a parked request is admitted by one of three paths:
- node tier — a completing request's permit close() drains the freed bucket (immediate, local). It re-reads the group's live node_limit on each drain, so a dynamic limit change takes effect on the very next drain instead of the chain reusing a value captured when it started.
- cluster tier (owner-push) — the bucket's ring owner pushes a reserved-lease grant to a waiting coordinator; round-robin across coordinators for fairness. This is also driven when the owner's TTL sweep reclaims an expired lease, which is the only free-slot signal available when a lease holder crashed or its release RPC was lost (there is no release RPC to react to).
- backstop sweep — reaps cancelled entries and re-attempts node-tier admission for a bucket the completion drain missed.
No queue timeout — a parked request has no wall-clock deadline. Legitimate queue wait is unbounded (it grows with backlog ÷ throughput), so any fixed cap would eventually cancel healthy, still-connected requests. A client bounds its own wait with cancel_after_time_interval (per-request or the search.cancel_after_time_interval cluster setting) or by disconnecting — either cancels the task, which evicts the parked entry promptly. The sweep only reaps already-cancelled tasks as defense-in-depth.
Disabling throttling releases the backlog — disabling throttling necessarily disables queueing in the same update (a queue with no throttle limit is rejected, and a throttling block whose limits are all unset is rejected), so parked requests would be left with nothing to wait for. An unthrottled group also takes the no-permit fast path and therefore produces no permit completions to drive a drain chain. The cluster-state listener releases the whole backlog immediately, admitting each request with an untracked permit; a deleted group is covered by the same path, and per-request cancellation is re-checked so already-cancelled tasks still fail rather than run.
Stats — GET _wlm/stats gains total_queued, total_queue_rejections, queued_current, queue_peak, and queue-wait aggregates (queue_wait_count, total_queue_wait_millis, max_queue_wait_millis).

Testing

Unit — queue container (per-bucket FIFO, per-bucket cap vs. the fixed group ceiling, the "no empty bucket left behind" invariant on every rejection path, dynamic size, cancelled-only eviction); queue service (park/drain/cancel, per-bucket capacity, recursion-safe async admit); settings validation (including the removed queue.timeout and the earlier queue.size both rejected as unknown keys, and the per-bucket max pinned to the group ceiling); owner-push drain, round-robin fairness, and TTL-sweep-driven grant; monitor-mode-never-parks on both the node and shared tiers; live node_limit decrease honoured mid-drain; backlog released on throttling disable; stats wire round-trip.
Integration — WlmQueueingIT: node-tier park-then-admit and queue-full → 429 end-to-end. WlmClusterThrottlingIT: a shared-only group parks a request and drains it via cross-node owner-push (the exact strand scenario the enqueue-first change fixes).
Manual (3-node live cluster) — verified across ~22 edge scenarios plus a load test: queue-full backpressure, cancellation (client disconnect / cancel_after_time_interval / _tasks/_cancel) evicting parked requests promptly, cross-node owner-push draining with no stranding, round-robin fairness, mass-cancel recursion safety, and throughput/latency behavior under bursts up to 3000 concurrent.
./gradlew :server:precommit and :plugins:workload-management:precommit pass.

Notes

No client-facing REST changes beyond the queue.size_per_bucket field on the workload group API.
queue.timeout is intentionally not a setting, and neither is the earlier queue.size — both are rejected as unknown keys so a stale config surfaces clearly rather than being silently ignored.
Releasing a deep backlog on throttling-disable is a burst: every parked request starts at once with nothing throttling it, which is what disabling throttling asks for, leaving the search threadpool's own queue as the remaining backpressure.
Pre-merge TODO: ThrottleOwnerSelector.MIN_OWNER_VERSION is a placeholder (V_3_7_0). It must be bumped to whatever release actually first ships these transport handlers, or a rolling upgrade could map buckets to nodes that lack them.

LilyCaroline17 · 2026-08-26T06:22:48Z

        boolean granted = tracker.tryAcquire(request.bucketKey, request.sharedLimit, request.leaseId, request.ttlNanos);
+        if (granted == false && request.wantsQueue && request.requestingNode != null) {
+            // Denied and the coordinator will park the request -> remember it so a freed slot is pushed back as a grant.
+            registerWaiter(request.bucketKey, request.requestingNode);


The coordinator is registered as a waiter here at acquire time, but the request isn't actually put into the queue until later in the async denial callback tryEnqueue. A concurrent release could grant in the gap, find the queue still empty in admitWithPermit, and deregister the coordinator. The request then is added to the queue with no registered waiter, and a shared-only config has no node-tier drain to recover it. Could we register the waiter after tryEnqueue instead?

Fixed, via enqueue-first rather than your exact suggestion: the shared path now parks the request before the owner acquire, so a grant or owner-push always finds a non-empty queue.

LilyCaroline17 · 2026-08-26T06:39:58Z

        final String leaseId;
        final long ttlNanos;
+        // Owner-push: who is asking, and whether they will park the request on denial (so the owner should register a
+        // waiter and later push a grant). requestingNode may be null from an older node that predates queueing.


A message from an older node would fail to deserialize rather than read as null because the fields are read unconditionally.

I think this depends if throttling and queueing are released together. I will add a todo to note version gate may be missing.

LilyCaroline17 · 2026-08-26T06:45:29Z

+        if (deque == null) {
+            return false;
+        }
+        boolean removed = deque.remove(req);


ArrayDeque.remove is O(n), and it runs once per cancelled task through evictCancelled. If a workload group is deleted, every task in it is cancelled through cancelTasksFromDeletedWorkloadGroups, so a group delete on a large single-bucket queue is O(n^2). Could we considered doing a LinkedHashedSet so then it's a O(1) on average remove?

Fixed as suggested — LinkedHashSet per bucket, O(1) average removal, and insertion order still gives the drains their FIFO.

LilyCaroline17 · 2026-08-26T06:48:05Z

+     * node permit if one is free; a no-op otherwise. Wired into {@link WorkloadGroupQueueService#sweep}. This recovers
+     * a request the node-completion chain missed without re-running full admission or re-contacting the shared owner
+     * (the owner recovers its own lost grants and reservations), and — crucially — without dequeuing-and-re-parking,
+     * so a still-waiting request keeps its original {@code queue.timeout} deadline.


I believe queue.timeout is removed now?

LilyCaroline17 · 2026-08-26T06:49:57Z


        // Cross-field checks on the merged throttling config (attribute required with a limit; ceiling must be >= 1).
        WorkloadGroupThrottleSettings.validateMergedConfig(mutableWorkloadGroupFragment.getThrottling());
+        // Cross-field checks on the merged queue config (timeout requires size > 0).


validateMergedConfig is a no-op for WorkloadGroupQueueSettings, should we remove this?

LilyCaroline17 · 2026-08-26T06:53:26Z

+        }
+        return () -> {
+            raw.close();
+            if (qs.totalDepth() > 0) {


drainNode is local to the group, so this guard should be currentDepth(groupId) > 0, not totalDepth() > 0 (totalDepth sums all currentDepths of groups that have been queued) as this would mean a completion drains whenever any group is backlogged.

LilyCaroline17 · 2026-08-26T23:56:46Z

+            return task;
+        }
+
+        public String principal() {


The only time principal is used is for a test in WorkloadGroupQueueTests.testCapturesListenerAndBucket, so is it necessary to pass it into the constructor for QueuedRequest (and thus in this function too)?

LilyCaroline17 · 2026-08-27T00:00:23Z

-            }));
+            // wantsQueue: on a shared-tier denial the owner should register this coordinator as a waiter for owner-push,
+            // but only if this group actually queues (size > 0) and the queue service is wired.
+            boolean wantsQueue = queueService != null && plan.queueSize > 0;


A group in monitor mode with a shared_limit > 0 and queue.size > 0 still sends wantsQueue=true so handleAcquire registers it as a waiter, and since it never gets added to the queue, the next freed slot pushes a grant that finds nothing and deregisters it. We could add && !plan.monitorMode check here to remove that extra work.

LilyCaroline17 · 2026-08-27T00:02:23Z

Also, I think some of the javadoc is behind the current code, may want to check and re-update the comments after the code updates.

LilyCaroline17 · 2026-08-27T00:37:57Z

+                qs.drainNode(
+                    groupId,
+                    bucketKey,
+                    key -> wrapNodePermit(throttleTracker.tryAcquire(key, nodeLimit), groupId, key, nodeLimit)


This uses the existing nodeLimit when the drain chain started, but if the nodeLimit is lowered or increased while a bucket is busy with a queue, that value is ignored here. Could the completion drain re-read the current nodeLimit so then we get the live updated value while draining.

Fixed — the drain re-reads the live node_limit per hop via currentNodeLimit(groupId). sweepDrainNode already re-read it, so the two drain paths had disagreed.

LilyCaroline17 · 2026-08-27T00:47:14Z

The new onSharedSlotFreed calls ensure that if a slot is freed, we know, but here we call tracker.sweepExpired without onSharedSlotFreed so we could have a situation where a slot never goes to a registered waiter so should we add that to this function?

sweepExpired() now returns the buckets that freed capacity and runs the next queued request (if present)

Signed-off-by: David Zane <davizane@amazon.com>

LilyCaroline17 reviewed Aug 26, 2026

View reviewed changes

LilyCaroline17 reviewed Aug 27, 2026

View reviewed changes

dzane17 force-pushed the 3.7-wlm-queue branch 2 times, most recently from 9292d59 to 8b6c534 Compare August 27, 2026 21:01

dzane17 added 2 commits August 28, 2026 16:42

Queue WLM requests denied by a throttle limit

41c972c

Signed-off-by: David Zane <davizane@amazon.com>

Let the permit owner decide when to drive owner-push

b278658

Signed-off-by: David Zane <davizane@amazon.com>

dzane17 force-pushed the 3.7-wlm-queue branch from 8b6c534 to b278658 Compare August 29, 2026 00:05

Conversation

dzane17 commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How it works

Testing

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LilyCaroline17 commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dzane17 commented Aug 12, 2026 •

edited

Loading