Repository navigation
Conversation
There was a problem hiding this comment.
馃煛 Changes recommended
Quota reconciliation can incorrectly block available capacity, and retry-delay calculation and reporting need correction.
4 open findings
What changed in this PR
Adds coordinated rate-limit handling to the management REST transport for synchronous and asynchronous callers.
Changes:
- Shares quota reservations across routes by origin and API key.
- Adds bounded retries, Retry-After handling, and jittered backoff.
- Introduces
RateLimitError, documentation, and regression tests.
| File | Description |
|---|---|
| tests/鈥媡est_api/鈥媡est_rate_limit.py | Tests quota coordination, retries, budgets, and cancellation. |
| runpod/鈥媏rror.py | Adds RateLimitError with retry-delay details. |
| runpod/鈥媋pi/鈥媟est.py | Integrates shared quotas and bounded retries. |
| runpod/鈥媋pi/鈥媉rate_limit.py | Implements parsing, reservations, cooldowns, and registry management. |
| docs/鈥媋pi/鈥媓andling_errors.md | Documents rate-limit behavior and configuration. |
馃 Review effort: Balanced
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if now >= reset_at: | ||
| self.windows[name] = (None, reset_at) | ||
| if self.in_flight: | ||
| delay = max(delay, 0.05) |
There was a problem hiding this comment.
After a window passes reset_at and remaining is None, every acquire() with something in flight takes the now >= reset_at branch again and returns 0.05. The check doesn't look at whether remaining was already cleared.
Result: after each minute reset, concurrent callers drop to one-at-a-time with 50ms polling until a response refreshes the window (and if the server stops sending that window, until the next update deletes it). Combined with the budget accounting above, this can raise RateLimitError with quota available.
There was a problem hiding this comment.
Still present in 3d1c1a2 (_rate_limit.py:92-96). The now >= reset_at branch rewrites the window to (None, reset_at) and sets probing = True whenever in_flight, without checking whether remaining is already None. So an already-reset window keeps forcing the 50ms probe path on every acquire() until a response refreshes it. Guarding with remaining is not None (or dropping the window once reset) should fix it. A test with two concurrent requests after a reset, asserting the second is not serialized, would cover it.
| if self.in_flight and ( | ||
| not self.discovered or (self.blocked_until and not delay) | ||
| ): | ||
| delay = 0.05 |
There was a problem hiding this comment.
Waiting on in-flight requests is a fixed 50ms busy-poll. N waiters each wake 20x/s and take the bucket lock, and the async path takes a threading.Lock on the event loop. A threading.Condition notified from release()/update() (or for async, a computed sleep until the next release) removes the wakeups and up to 50ms of added latency.
There was a problem hiding this comment.
Still present in 3d1c1a2 (_rate_limit.py:104, return 0.05). The budget accounting part is fixed, since the probe delay no longer goes through budget.wait(), but waiters still busy-poll at 20 Hz under the bucket lock, and the async path still takes a threading.Lock on the event loop. A threading.Condition notified from release()/update() would remove the wakeups and the added latency.


Honor
Retry-Afterand use jittered exponential backoff when no valid hint is available. Retry and wait budgets surfaceRateLimitErrorwhen exhausted.