fix: trigger initial body send when downstream already consumed#1
Open
nerdalert wants to merge 1 commit intopraxis-proxy:certificate-publicfrom
Open
fix: trigger initial body send when downstream already consumed#1nerdalert wants to merge 1 commit intopraxis-proxy:certificate-publicfrom
nerdalert wants to merge 1 commit intopraxis-proxy:certificate-publicfrom
Conversation
8 tasks
There was a problem hiding this comment.
Pull request overview
This PR fixes request body forwarding for the case where a ProxyHttp implementation fully consumes (pre-reads) a non-empty downstream request body before the proxy enters duplex streaming to the upstream. It ensures the “initial body send” path is triggered so request_body_filter() receives a terminal callback and can replay its buffered body, without relying on the 64 KiB retry buffer.
Changes:
- Add a
body_already_consumedcondition (downstream already done + body not empty) to enter the initial body-send path for H1, H2, and custom upstream transports. - Refactor retry-buffer handling to pass an
Option<Bytes>directly (instead of destructuring and re-wrapping) where applicable. - Keep existing empty-body handling semantics (H1 uses the initial path; H2/custom continue to handle empty bodies at header send time).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pingora-proxy/src/proxy_h2.rs | Ensures H2 upstream body sending enters the initial send path when downstream body was pre-consumed, enabling terminal request_body_filter() replay. |
| pingora-proxy/src/proxy_h1.rs | Extends H1 initial send conditions to cover pre-consumed (non-empty) downstream bodies so filters can emit buffered bodies. |
| pingora-proxy/src/proxy_custom.rs | Applies the same pre-consumed body handling to custom upstream transports to avoid missing the filter terminal callback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+386
to
+397
| let buffer = session.as_mut().get_retry_buffer(); | ||
| let body_empty = session.as_mut().is_body_empty(); | ||
|
|
||
| // Enter the initial body-send path when: | ||
| // - a retry buffer exists (normal retry replay), or | ||
| // - the downstream body was already consumed before forwarding | ||
| // (e.g. a ProxyHttp implementation that pre-reads the body for | ||
| // inspection). In that case request_body_filter() needs one | ||
| // terminal callback so the implementation can emit its | ||
| // buffered body to upstream. | ||
| let body_already_consumed = downstream_state.is_done() && !body_empty; | ||
| if buffer.is_some() || body_already_consumed { |
Member
Author
There was a problem hiding this comment.
Added a targeted regression test for Copilot’s comment:
- Fully consumes the downstream request body in request_filter().
- Stores the pre-consumed body in the test context.
- Emits that buffered body from request_body_filter() on the terminal callback.
- Sends the request over H2C.
- Forces H2 upstream with x-h2: true.
- Uses a payload larger than BODY_BUF_LIMIT: 128 KiB + 17.
- Asserts the upstream /echo response body exactly matches the full payload.
b8ee4a0 to
85eab8c
Compare
When a ProxyHttp implementation pre-reads the request body for inspection before upstream selection, downstream is already done when duplex mode begins. The initial body-send path was only entered for retry buffers or empty bodies, skipping the case where request_body_filter needs a terminal callback to emit the pre-read body. Add body_already_consumed check across h1, h2, and custom transports so pre-read body replay works without relying on the 64 KiB retry buffer workaround." Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
85eab8c to
1c8d088
Compare
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.
Details and options evaluated are in praxis-proxy/praxis#179
When a ProxyHttp implementation pre-reads the request body for inspection before upstream selection, downstream is already done when duplex mode begins. The initial body-send path was only entered for retry buffers or empty bodies, skipping the case where request_body_filter needs a terminal callback to emit the pre-read body.
Add body_already_consumed check across h1, h2, and custom transports so pre-read body replay works without relying on the 64 KiB retry buffer workaround."
Validation
Sends a ~70 KB JSON-RPC body ("x".repeat(70_000) inside a JSON wrapper) through a json_rpc filter with max_body_bytes: 131072 (128 KiB). The json_rpc filter uses StreamBuffer mode, which pre-reads the body before routing. An echo backend sends the received body back. The test asserts the echoed body length matches the original.
Before the Pingora fix: the 64 KiB retry buffer truncated the body, so the backend received ~65 KB instead of ~70 KB. The assertion failed on the length mismatch.
The test is in a yet to be pushed agentic PR that looks like this. It now passes using that branch along with passing all of the praxis IT suite: