[Serve] Fix P99 latency regression: decrement queue length cache on request completion, reduce replica update overhead

[abrarsheikh]

Fixes elevated P99 latency observed when scaling Ray Serve deployments with max_ongoing_requests=1. The root cause is that the queue length cache is incremented when a request is sent (on_send_request) but never decremented when the request completes, causing cache entries to get "stuck" at values >= max_ongoing_requests. This forces every subsequent routing decision to fall back to blocking probe RPCs instead of using cached values.

This regression was introduced when on_send_request was added in the router refactor (commit de1494e, Aug 2025). Prior to that (Ray <= 2.10), the cache was only updated from replica-reported values (probes and rejection protocol responses), so there was no increment-without-matching-decrement problem.

Changes

1. Decrement queue length cache on request completion (primary fix)

Implements decrement_queue_len_cache in RequestRouter to decrement the cache entry by 1 when a request finishes. This restores the increment/decrement symmetry that was missing since on_send_request was introduced.

With max_ongoing_requests=1, the routing algorithm in _select_from_candidate_replicas treats any cache entry >= max_ongoing_requests as "needs probing". Before this fix, every routed request would bump the cache to 1, and it would stay there until either the 10s TTL expired or a probe happened to refresh it. This meant the cache was nearly useless, and most routing decisions required a blocking probe RPC (~20-40ms round trip), directly explaining the observed P99 increase.

2. Reuse existing replica wrappers in _update_running_replicas

Previously, every replica update created new RunningReplica wrappers for all replicas, even those that hadn't changed. During scaling storms (100+ updates with 250+ replicas each), this caused O(n) synchronous work per update on the router's event loop.

Now reuses existing wrappers for known replicas, only creating wrappers for genuinely new ones. This reduces per-update work from O(all_replicas) to O(new_replicas).

3. Reduce replica update log noise

The "Got updated replicas" log line previously serialized every replica ID (250+) into a string on every update. Changed to log only the total count and the added/removed counts, reducing both log volume and the synchronous formatting cost on the event loop.

Load Test Results

Scale	Client	Master QPS	Master P99 Latency	Optimized QPS	Optimized P99 Latency
Up to 100 Users
Up to 200 Users

[gemini-code-assist]

Code Review

This pull request effectively addresses a P99 latency regression by ensuring the replica queue length cache is decremented upon request completion. The implementation is robust, handling edge cases like expired cache entries and preventing negative queue lengths. The accompanying unit tests are comprehensive and validate the fix thoroughly. Additionally, the pull request includes valuable optimizations that reduce replica update overhead and log noise, improving overall system performance and observability during scaling events. The changes are well-structured and clear. Excellent work.

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

[alvinttang]

Code Review

This is a well-analyzed fix with clear root cause identification and load test validation. The increment-without-decrement asymmetry in the queue length cache is a classic bug pattern.

Issues found

1. Thread safety of decrement_queue_len_cache called from add_done_callback

The result.add_done_callback(lambda _: self.request_router.decrement_queue_len_cache(...)) in router.py runs the callback from whatever thread/context completes the future. If the ReplicaResult future is resolved by a different asyncio task or thread (e.g., a gRPC callback thread), decrement_queue_len_cache accesses _replica_queue_len_cache without synchronization. The cache's get() and update() methods don't appear to be thread-safe (they're dict operations). This could cause rare data races under high concurrency.

Consider wrapping the decrement in loop.call_soon_threadsafe() or ensuring the callback always runs on the router's event loop.

2. Decrement on failed/cancelled requests

The add_done_callback fires regardless of whether the request succeeded, failed, or was cancelled. The docstring for decrement_queue_len_cache says "Should NOT be called for rejected requests" but doesn't mention failed/cancelled ones. If a request fails after being sent (e.g., replica crashes mid-request), the decrement still fires. This is probably correct (the request did leave the queue), but worth documenting explicitly.

3. The update_replica_info optimization assumes immutable fields stay immutable

update_replica_info only updates _replica_info and _multiplexed_model_ids. If other fields on RunningReplicaInfo change (e.g., max_ongoing_requests changes during a config update), the wrapper won't pick them up. The current RunningReplica base class caches max_ongoing_requests from replica_info in __init__ but update_replica_info doesn't refresh it. This is probably fine today if max_ongoing_requests truly never changes for a live replica, but it's a subtle contract.

4. Lambda closure captures replica by reference

result.add_done_callback(
    lambda _: self.request_router.decrement_queue_len_cache(
        replica.replica_id
    )
)

This is inside a loop (async for replica, result in ...), but since there's a return result immediately after, replica won't be re-bound before the lambda executes. So this is safe in practice. Just noting it since lambda-in-loop is a common Python gotcha.

Positive notes

• The replica wrapper reuse optimization (_update_running_replicas) is a clean O(n) -> O(new) improvement
• The log noise reduction (count-based instead of serializing all IDs) is a nice touch
• Comprehensive test coverage including edge cases (expired cache, negative clamp, multiple decrements)
• The FakeRunningReplica.update_replica_info override in tests shows good attention to the test infrastructure

Solid PR overall. The thread safety concern (point 1) is the main thing I'd want verified before merging.

[abrarsheikh]

Thanks @alvinttang for the review, i have added the thread safety, i am a little concerned about the performance overhead from this. I will run a full set of benchmarks to confirm before merging.

[akyang-anyscale]

can we move this logic to _process_finished_request?

[abrarsheikh]

no because _process_finished_request doesn't have access to if the request was rejected or not.

[abrarsheikh]

• Throughput metrics (http_avg_rps, grpc_avg_rps, handle_avg_rps) are within ~1–3%
• Most latency metrics are within ±5%
• A few p90/p95/p99 outliers (~10–25%) appear in both directions and are consistent with normal benchmark variance.

Conclusion:
There is no meaningful performance regression from the thread-safe implementation. Throughput remains effectively unchanged and latency differences are within expected run-to-run noise.

[abrarsheikh]

link to release test https://buildkite.com/ray-project/release/builds/84297
rerunning master release tests again to eliminate noise https://buildkite.com/ray-project/release/builds/84431

[abrarsheikh]

master_vs_thread_vs_nonthread_sorted.csv