[serve] Add max_request_retries to bound router retry loops

[HrushiYadav]

Description

The router's while True retry loop in route_and_send_request and _pick_and_reserve_replica retries indefinitely when replicas reject requests. max_queued_requests is only checked once at the entry gate, not during retries. Under sustained load with large payloads (~50MB), requests pile up in the retry loop causing OOM -- num_queued_requests can reach 119 when max_queued_requests=20.

This PR adds max_request_retries to RequestRouterConfig:

• Default -1 (unlimited) preserves backwards compatibility
• When set to a non-negative integer, the router raises BackPressureError (503) after exhausting retries
• Both route_and_send_request (replica rejection retry) and _pick_and_reserve_replica (capacity queue retry) are bounded
• Configurable via RequestRouterConfig(max_request_retries=N) or env var RAY_SERVE_ROUTER_MAX_REQUEST_RETRIES

Related issues

Fixes #61017

Additional information

Files changed:

• constants.py -- new RAY_SERVE_ROUTER_MAX_REQUEST_RETRIES env var (follows existing backoff env var pattern)
• config.py -- new max_request_retries field on RequestRouterConfig with validator, __eq__, __hash__
• router.py -- _check_retry_limit() helper, retry counter in both loops
• test_backpressure.py -- config validation test + integration test with max_request_retries=3

Semantics:

• max_request_retries=-1: unlimited (default, no behavior change)
• max_request_retries=0: no retries (fail on first rejection)
• max_request_retries=N: retry up to N times, then 503

[gemini-code-assist]

Code Review

This pull request introduces a configurable limit on the maximum number of times a request can be retried by the router before being dropped with a BackPressureError. It adds max_request_retries to RequestRouterConfig and implements retry tracking and limit checking in AsyncioRouter. The review feedback suggests avoiding accessing the private _deployment_config attribute of RouterMetricsManager inside _check_retry_limit by instead storing and updating _max_queued_requests directly on AsyncioRouter.

[abrarsheikh]

looks good. left some nits. please update the docs. Also, CI is failing.

[HrushiYadav]

Thanks for the review @abrarsheikh! Addressed all feedback:

• Moved test_max_request_retries_config_validation to tests/unit/test_config.py
• Moved inline imports to top of test_backpressure.py
• Added request_router_config / max_request_retries to deployment configuration docs
• The _max_queued_requests refactor was already done in the previous push

[abrarsheikh]

please resolve comments that are addressed already, include once from bot

[HrushiYadav]

Addressed proto serialization issue: added optional int32 max_request_retries to RequestRouterConfig in serve.proto, plus round-trip tests. All review threads resolved.

[HrushiYadav]

@abrarsheikh All review threads are resolved (yours + bot). The Buildkite failure is in serve: enormous tests [g19_s7] (Windows) and serve: HAProxy tests -- both unrelated to the max_request_retries changes. Could you re-trigger CI or approve when you get a chance?

[abrarsheikh]

The latest CI failure seems related to your changes. Please investigate

[HrushiYadav]

You're right, thanks for pushing back. I dug in and the failure was my integration test, not flakiness.

The test used a single always-busy replica (max_ongoing_requests=1, blocked on a signal) to try to exhaust max_request_retries. But that scenario never reaches the retry counter: when every replica is at capacity, the request router's internal _choose_replicas_with_backoff loop backs off and blocks until a replica frees up, so _pick_and_reserve_replica never iterates. The request just stayed queued and hit the timeout instead of raising BackPressureError.

I replaced it with a deterministic unit test in test_router.py that drives the actual path this PR bounds (the one causing the OOM in #61017): a replica that repeatedly rejects reservations (_reject_reservation=True on both the initial and retry pick). With max_request_retries set, the router raises BackPressureError once the budget is exhausted. It follows the existing test_choose_replica_retries_when_reservation_rejected harness, so it's fast and not timing-dependent.

Pushed in 2164206.

[HrushiYadav]

@abrarsheikh Addressed the failing tests. Ready for another look whenever you have time, thanks!

[HrushiYadav]

@abrarsheikh gentle nudge on this one whenever you get a chance, it's green and ready to go. no rush, thanks!

[github-actions]

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[HrushiYadav]

Still green and ready, not stale from my side. This bounds the router retry loop with max_request_retries, and the earlier test feedback from @abrarsheikh is addressed. @abrarsheikh @eicherseiji would either of you have a moment to take a look before the stale bot closes it? Happy to rebase if needed.

[abrarsheikh]

please add a e2e test

[HrushiYadav]

Thanks. I dug into how a reservation rejection actually gets triggered so the e2e exercises the retry bound rather than the queue-full or capacity-backoff paths.

The catch: a rejection only happens in the optimistic-cache race window, and the cache self-corrects to the true queue length right after each rejection (on_new_queue_len_info in _pick_and_reserve_replica). So a single request accrues at most one rejection before it falls into the backoff/block path. That means real routing can only deterministically trip max_request_retries=0 (first rejection gives num_retries=1), and even producing that one rejection depends on concurrent routing tasks racing the semaphore, so it is timing sensitive.

For thresholds >= 1 I don't see a way to drive repeated rejections through real routing without a fake replica, which is why the deterministic coverage sits in the unit tests (FakeRunningReplica rejecting on reserve).

I can add a best-effort e2e that saturates a single replica (max_ongoing_requests=1, no max_queued_requests) with a SignalActor and fires a burst of concurrent requests, asserting at least one BackPressureError from the retry limit at max_request_retries=0. Happy to add it, just flagging it will be inherently timing dependent. Or if you had a specific e2e scenario in mind that avoids the race, point me at it and I will build it. What would you prefer?

[abrarsheikh]

with max ongoing = 1 and unlimited queue request and max_request_retries=1 + using a signal actor to block requests.

Send 2 requests; 1st will be blocked by the signal actor and the 2nd request will sit in the queue, incurring a backoff. The second request will deterministically fail with a backpressure error, right?

[HrushiYadav]

Pushed changes addressing the review:

• Added an e2e test (test_max_request_retries_bounds_retries in test_backpressure.py) using your recipe: a single replica at max_ongoing_requests=1, unlimited queue, max_request_retries=1, a signal actor blocking the first request; the second request deterministically fails with BackPressureError.
• max_request_retries now only counts replica-capacity rejections. Transient ActorDiedError/ActorUnavailableError retries no longer consume the budget, per your preference on the actor-failures thread.
• BackPressureError now reports retry-budget exhaustion (num_retries, max_request_retries) instead of the queue-limit fields, so the 503 body reflects the real cause.

Also resolved the bot threads that were already addressed. Ready for another look, and happy to re-trigger premerge whenever.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit a7237ba. Configure here.}

[HrushiYadav]

@abrarsheikh I reverted my last push after digging into the new Bugbot finding ("Retry bound skips saturated replicas"), which turns out to be correct.

Tracing it: _choose_replica_for_request does replica = await pending_request.future, and that future is only fulfilled by a routing task once a replica actually has capacity. When the sole replica is saturated (the recipe from your earlier comment), the routing task just keeps backing off and never fulfills the future, so _route_and_send_request_once never reaches the capacity-rejection branch. num_retries never increments, BackPressureError is never raised, the request parks indefinitely, and the e2e test hangs. So the retry bound as written does not fire in the saturated case, which is the OOM scenario this PR is meant to bound.

This also runs into the "only capacity rejections count" preference: in the saturated case there is no capacity rejection to count, the request is stuck waiting inside the router backoff.

How would you like to bound it? Two options I see:

1. Bound the wait in the choose-replica path itself (a timeout or a max-attempts cap on the backoff loop) and have that count toward max_request_retries.
2. Keep capacity-rejection counting, but add a separate bound on time spent parked with no available replica.

Happy to implement whichever you prefer, then add an e2e test that actually terminates.

[HrushiYadav]

Rebased on latest master, merge conflicts resolved. All 6 commits cherry-picked cleanly. CI should re-trigger now. @abrarsheikh

[HrushiYadav]

Branch is up to date with master. @abrarsheikh — want to pick this back up? Happy to address any remaining feedback.

[HrushiYadav]

Pushed a new commit (84ff199) that fixes the root cause and makes your recipe work.

Root cause: max_request_retries was only checked in _pick_and_reserve_replica (after reserve_slot returns accepted=False). When all replicas are saturated, the routing task backs off inside _fulfill_pending_requests / _choose_replicas_with_backoff and never reaches reserve_slot, so the retry counter never fired.

Fix:

• Added _max_request_retries to RequestRouter base class, propagated from AsyncioRouter via update_max_request_retries() on both lazy init and config update.
• In _fulfill_pending_requests, after each failed backoff attempt, check backoff_index > _max_request_retries and fail pending_request.future with BackPressureError.
• In _choose_replica_for_request, catch BackPressureError from the future and clean up indices before re-raising.

Integration test uses your exact recipe: num_replicas=1, max_ongoing_requests=1, max_queued_requests=-1, max_request_retries=1, signal blocks request 1 → request 2 exhausts the backoff budget and raises BackPressureError deterministically.

The existing unit test (test_choose_replica_bounds_retries_on_repeated_rejection) still covers the reserve_slot rejection path. Happy to add a dedicated unit test for the backoff path if you'd like it.

[HrushiYadav]

@abrarsheikh Rebased against master. Force-push: d67fe68f → 9d91f26d7f.

Only conflict was doc/source/serve/configure-serve-deployment.md — kept upstream's improved max_queued_requests wording (mentions backpressure_config) and added the request_router_config bullet for max_request_retries. All other commits applied cleanly via auto-merge.

9 files, +130/-1. CI should re-trigger now.