Handle unread streaming request bodies during 402 retries#6
Handle unread streaming request bodies during 402 retries#6shuofengzhang wants to merge 2 commits intoqntx:mainfrom
Conversation
gitctrlx
left a comment
There was a problem hiding this comment.
Thanks for the fix — the root cause analysis is solid and the streaming-body edge case is a real problem worth addressing.
The main blocker: the original 402 response is never closed before the retry request is sent, which leaks connections. See inline comments for the suggested one-liner fix on both sync and async paths.
A couple of minor suggestions inline as well (clone helper safety, test coverage gaps) — non-blocking, happy to discuss.
Also: the second commit message (Handle unread streaming bodies...) doesn't follow the project's Conventional Commits convention — a quick git rebase -i to reword it to something like fix(transport): handle unread streaming bodies on 402 retry would be appreciated before merge.
| body = request.read() | ||
|
|
||
| retry = _clone_request_with_headers(request, payment_headers, content=body) | ||
| return self._inner.handle_request(retry) |
There was a problem hiding this comment.
The original 402 response is never closed before issuing the retry. Since response.read() was called above (L79), the body is consumed but the underlying connection is still held open. This will leak connections under sustained 402 traffic.
| return self._inner.handle_request(retry) | |
| retry = _clone_request_with_headers(request, payment_headers, content=body) | |
| response.close() | |
| return self._inner.handle_request(retry) |
| body = await request.aread() | ||
|
|
||
| retry = _clone_request_with_headers(request, payment_headers, content=body) | ||
| return await self._inner.handle_async_request(retry) |
There was a problem hiding this comment.
Same issue on the async path — the 402 response is never closed before the retry.
| return await self._inner.handle_async_request(retry) | |
| retry = _clone_request_with_headers(request, payment_headers, content=body) | |
| await response.aclose() | |
| return await self._inner.handle_async_request(retry) |
| url=original.url, | ||
| headers=headers, | ||
| content=original.content, | ||
| content=original.content if content is None else content, |
There was a problem hiding this comment.
Nit: when content is None, this falls back to original.content, which will raise RequestNotRead if the body hasn't been consumed. Since every current call site passes content= explicitly this is fine in practice, but it's a footgun for future callers.
Consider either making content required, or documenting the precondition more prominently (e.g. "caller must ensure body is materialized when content is omitted").
| assert response.status_code == 200 | ||
| assert inner.calls == 2 | ||
| assert inner.retry_headers["x-payment"] == "signed" | ||
| assert inner.retry_body == b'{"prompt":"hi"}' |
There was a problem hiding this comment.
Good coverage for the streaming-body edge case. A few more scenarios would round out the test suite:
- Happy path: non-402 response passes through untouched (ensures the transport doesn't accidentally mutate normal traffic).
- Exception fallback:
handle_402_responseraises → original 402 is returned as-is. close()/aclose()delegation: verifies the lifecycle methods forward to the inner transport.
There was a problem hiding this comment.
Pull request overview
Improves x402 retry behavior in the custom httpx transports when a 402 Payment Required response arrives before a streaming request body has been consumed, ensuring retries can safely replay the original payload.
Changes:
- Extend
_clone_request_with_headers(...)to optionally accept explicit body bytes for deterministic retry replay. - Update sync + async x402
402retry paths to materialize request bodies whenhttpx.RequestNotReadis raised. - Add regression tests covering short-circuiting
402responses with unread streaming request bodies for both transports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/x402_openai/_transport.py |
Materializes unread request bodies on the 402 retry path and passes explicit bytes into the cloned retry request. |
tests/test_transport.py |
Adds sync/async regression tests simulating a 402 short-circuit that never consumes the request stream. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_transport.py
Outdated
| yield b'"hi"}' | ||
|
|
||
|
|
||
| @pytest.mark.anyio |
|
Addressed the review feedback with a follow-up commit:\n\n- Close the original 402 response before issuing sync/async retries ( / ) to avoid leaking held connections.\n- Clarified docs about the body-materialization precondition when is omitted.\n- Expanded transport tests for:\n - non-402 passthrough behavior,\n - signing-failure fallback returning original 402 response,\n - close/aclose delegation to inner transports,\n - explicit verification that the original 402 response is closed before retry.\n- Switched async test marker to (repo uses with ).\n\nLocal validation run:\n- \n- \n- |
|
Correction to my previous summary (formatting was mangled by the CLI shell): Addressed review feedback with commit 98f6712:
Local validation:
|
What changed
402 Payment Requiredresponse is returned before the request body is consumed.request.contentis not yet readable (httpx.RequestNotRead)._clone_request_with_headers(...)to accept explicitcontentbytes for deterministic retry body replay.402response with an unread streaming request body.Why
request.contentwas always available.402without first reading the body.httpx.RequestNotRead, breaking transparent x402 retries.Insight / Why this matters
402path where replay is required.Testing
scripts/clone_and_test.sh qntx/x402-openai-python(28 passed)ruff check src/x402_openai/_transport.py tests/test_transport.pypytest -q(28 passed)