fix(python): do not replay POST requests after a server error - #1353
fix(python): do not replay POST requests after a server error#1353pbxqdown wants to merge 1 commit into
Conversation
kubernetes-prow
Bot
added
do-not-merge/work-in-progress
✅ Deploy Preview for agent-sandbox canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pbxqdown The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Welcome @pbxqdown! |
kubernetes-prow
Bot
added
the
needs-ok-to-test
|
Hi @pbxqdown. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
the
size/M
POST /execute runs a shell command inside the sandbox, so a 5xx response can arrive after the runtime already ran the command. Both connectors retried it, executing the command up to six times while the caller saw a single error. Limit transport-level retries to GET, PUT and DELETE.
4f367fd to
6a1bb64
Compare
kubernetes-prow
Bot
removed
the
do-not-merge/invalid-commit-message
There was a problem hiding this comment.
Pull request overview
This PR updates the Python SDK’s retry behavior to avoid replaying side-effecting POST requests (notably command execution) after transient 5xx responses, aligning the SDK with the project’s “don’t replay commands” reliability expectations.
Changes:
- Sync connector: removes
POSTfrom the urllib3Retry(allowed_methods=...)set. - Async connector: retries 5xx responses only for
GET/PUT/DELETE, notPOST. - Adds regression tests covering both sync and async connectors to ensure
POSTis not retried on 5xx responses.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/connector.py | Narrows urllib3 retryable HTTP methods to avoid replaying POST after 5xx. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/async_connector.py | Adds method check so 5xx retries only happen for non-POST methods. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_connector.py | Adds sync regression test asserting POST is not in retryable methods. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_async_sandboxclient.py | Adds async regression test asserting POST is not retried on 5xx. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| delay = BACKOFF_FACTOR * (2 ** attempt) | ||
| logger.warning( | ||
| f"Retryable status {response.status_code} from {url}, " |
There was a problem hiding this comment.
Thanks for looking at this. I don't think the response is left open here.
AsyncClient.request() calls send(..., stream=False), which awaits response.aread(). Reading a response to completion runs aiter_raw() to the end, whose last statement is await self.aclose(); Response.aclose() is documented as "Automatically called if the response body is read to completion". So the connection is already released back to the pool before the loop continues, and an explicit close would early-return on is_closed.
This loop is also unchanged by this PR apart from the added method check. Happy to add an explicit close anyway if you would rather see it stated.
| retries = Retry( | ||
| total=5, | ||
| backoff_factor=0.5, | ||
| status_forcelist=[500, 502, 503, 504], | ||
| allowed_methods=["GET", "POST", "PUT", "DELETE"] | ||
| allowed_methods=RETRYABLE_METHODS, |
There was a problem hiding this comment.
Good thing to check, but allowed_methods does not gate connect-time retries.
In urllib3 2.7 Retry.increment(), the connection-error branch decrements connect without consulting _is_method_retryable(). Only the read-error branch and is_retry() (status retries) consult it.
Measured with this connector's own Retry policy, counting POST attempts that reach the transport:
| POST failure mode | POST in allowed_methods |
POST removed |
|---|---|---|
| nothing listening (dial) | 6 | 6 |
| server reads the request, then closes (read) | 6 | 1 |
So dial-time failures behave exactly as before, which is what the description meant.
The read case does change, and that is intended: the body reached the server, so the command may already be running — the same risk as the 5xx case this PR is about. The description only mentioned 5xx, so I have made that explicit.
What this PR does / why we need it:
commands.run()sendsPOST /execute, which runs a shell command inside thesandbox. Both Python connectors retried that request on 500/502/503/504, so a
command whose response failed after the runtime had already run it was executed
again — six attempts in total. The caller saw a single
SandboxRequestErrorwith no indication that the command had run more than once.
A retryable status code means the connection succeeded and the request body
reached the server, so the work it describes may already be done. This change
limits transport-level replay to
GET,PUTandDELETE: the sync connectordrops
POSTfrom the urllib3Retry(allowed_methods=...), and the asyncconnector checks the method alongside the status code.
Concretely, for a
POST:The read-error row changes for the same reason as the 5xx row: the body reached
the server, so the command may already be running. Dial-time failures are
untouched — urllib3's
allowed_methodsdoes not gate connect-error retries, andthe async connector never retried transport errors to begin with.
This matches how the rest of the project draws the line.
Run()in the Go SDKdefaults to a single attempt, and the router retries dial-class failures only
because replaying a body that may already have been sent could duplicate side
effects.
Testing: two regression tests were added, both failing before this change and
passing after —
test_connector.py::TestSandboxConnectorStrategySelection::test_post_requests_are_not_retriedand
test_async_sandboxclient.py::TestAsyncConnector::test_post_requests_are_not_retried_on_server_error.make test-unitpasses. The behaviour was also verified end to end against astub runtime that performs a side effect and then answers 503: it records the
command executed 6 times on
mainand once with this change, for both the syncand async connectors.
Which issue(s) this PR is related to:
Fixes #1352
Release Note