fix: concurrency correctness#515
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses correctness issues in HTTP request handling under concurrency (serverless result posting) and improves endpoint client behavior around retries and final-state handling.
Changes:
- Avoids mutating shared
aiohttpsession headers by passingX-Request-IDper request when posting serverless results (with matching test updates). - Improves endpoint
requests.Sessionretry configuration by mounting retry adapters for bothhttps://andhttp://and expanding retryable status codes. - Fixes endpoint final-state logic by adding
CANCELLEDtoFINAL_STATESand makingis_completed()reference that constant.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_serverless/test_modules/test_http.py | Updates assertions to require X-Request-ID header in result/stream POSTs. |
| runpod/serverless/modules/rp_http.py | Passes request ID per request to avoid shared-session header races. |
| runpod/endpoint/runner.py | Updates retry/backoff policy and mounts adapter for HTTPS as well as HTTP. |
| runpod/endpoint/helpers.py | Aligns final-state constants with is_completed() logic (includes CANCELLED). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| retries = Retry( | ||
| total=5, | ||
| backoff_factor=1, | ||
| status_forcelist=[408, 429, 500, 502, 503, 504], | ||
| allowed_methods=frozenset(["GET", "POST"]), | ||
| ) |
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
deanq
left a comment
deanq
left a comment
There was a problem hiding this comment.
Review notes from a PR walkthrough. Two inline comments below.
| total=5, | ||
| backoff_factor=1, | ||
| status_forcelist=[408, 429, 500, 502, 503, 504], | ||
| allowed_methods=frozenset(["GET"]), |
There was a problem hiding this comment.
Two things on this retry policy:
- Behavior change: POSTs are no longer retried on 429. Previously all methods retried on 408/429; now
/run,/runsync,/cancelretry on nothing. That's the right call for duplicate-job safety given there's no idempotency key, but a 429 is a rejected request that is safe to retry — worth documenting the tradeoff rather than leaving it implicit in theallowed_methodsblanket. - No test coverage: this is the headline "don't create duplicate jobs" fix and nothing guards it. A future refactor could silently reintroduce POST retries. Please add a regression test asserting the mounted adapter's
Retry.allowed_methods == {"GET"}and that bothhttp://andhttps://are mounted.
| """ Helper functions for the Runpod Endpoint API. """ | ||
|
|
||
| FINAL_STATES = ["COMPLETED", "FAILED", "TIMED_OUT"] | ||
| FINAL_STATES = ["COMPLETED", "FAILED", "TIMED_OUT", "CANCELLED"] |
There was a problem hiding this comment.
Good catch aligning FINAL_STATES with is_completed. This is broader than a cleanup, though: FINAL_STATES is consumed directly in runner.py stream() (and the asyncio runner). Before this change, streaming a CANCELLED job never hit the break — CANCELLED not in FINAL_STATES stayed true, so the loop spun forever. Adding CANCELLED fixes that latent infinite loop. Please add a test that stream() terminates on CANCELLED (or at minimum assert "CANCELLED" in FINAL_STATES) so the fix is locked in.
No description provided.