Repository navigation
Conversation
RoeRetryTransport retried every httpx.TransportError for every method. A read timeout, a dropped connection, or a protocol error can happen after the server has received the request, so a POST was replayed up to max_retries times. With workflows.run(wait=True), the server holds the request for 60 seconds and the client times out at 60, so a long run reached a local test server four times: one call would start four runs. POST and PATCH now retry only ConnectError, ConnectTimeout and PoolTimeout, which are raised before any bytes are sent. Idempotent methods and the status-code retries are unchanged. This matches the other SDKs: the TypeScript retry middleware only retries on response status codes, and Go does not retry a request that hit its deadline. The module docstring's claim of alignment is corrected. The fix lives in src/roe/utils, outside the paths the roe-main fan-out regenerates, so later release PRs keep it. The test file is force-added like the rest of tests/, which .gitignore lists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The PR appears safe to merge; no actionable issues were found. Summary
Chen-ROE explicitly acknowledged that POST requests still retry after 502 or 504 responses. The PR leaves this unchanged because the gateway's behavior was not checked. No actionable issues were found. Tests were reviewed but not run. Reviews (1) · Last reviewed commit: "Don't resend a POST or PATCH that may ha..." · Reviewed by Greptile |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RoeRetryTransportretried everyhttpx.TransportErrorfor every HTTP method. Read timeouts, dropped connections and protocol errors can all happen after the server has received the request, so a POST could be replayed up tomax_retriestimes.workflows.run(wait=True), new in #99, makes this likely. The server holds the request for up to 60 seconds, and the client's default timeout is also 60 seconds. Against a local server slower than the timeout, one POST arrived 4 times. On the real API, that means one call starts four workflow runs. Greptile flagged the timeout on #99.Change
ConnectError,ConnectTimeoutandPoolTimeout. These are raised before any bytes are sent, so resending cannot duplicate work.This matches the other SDKs:
retryMiddlewareonly retries on response status codes.shouldRetryrefuses to retry a request that hit its deadline.The module docstring claimed alignment with them; I've corrected it.
Why this survives later release PRs
The roe-main SDK fan-out regenerates only
src/roe/_generated,src/roe/api,openapi/,README.mdandSDK_EXAMPLES.md(seeroe-sdk/targets.yml). Each release PR also branches frommain. A fix insrc/roe/utils/transport.pyis therefore kept by every future generated PR.Merge this before #99. #99 doesn't touch this file, and merging #99 publishes 1.1.20 from
main, so the release will include the fix.Test Plan
New
tests/unit/test_retry_transport.pycovers six cases:The two non-retry cases fail without the fix. The file is force-added like the rest of
tests/, which.gitignorelists.uv run pytest: 68 passed.ruff checkandruff format --checkare clean.Against a real local server slower than the client timeout: POST reached the server once with the fix (4 times before); GET still retried 4 times.
Not changed
All three SDKs still retry a POST that gets a 502 or 504. Whether that can duplicate a run depends on the gateway in front of the API, which I didn't check.
🤖 Generated with Claude Code