Skip to content

request_body_filter not called when downstream body consumed before forwarding #179

Description

@nerdalert

Problem

This is lengthy but it's not an obvious solution and will need to manifest into an upstream fix so worth some optionality. For the unblocking of dev I'm going with option 1 with our fork to get unblocked for agentic > 64KiB sessions.

When a ProxyHttp implementation reads the downstream request body before upstream forwarding begins, the downstream body can be fully consumed by the time Pingora enters the proxy forwarding loop.

This can happen when an implementation needs to inspect the request body before choosing an upstream, for example body-based routing, JSON field extraction, request validation, or request normalization.

In that state:

  • is_body_done() returns true
  • is_body_empty() returns false because the request did have a body
  • get_retry_buffer() returns None unless retry buffering was explicitly enabled

The forwarding loop currently only enters the initial body-send path when it has a retry buffer, or in the h1 path when the body is empty:

if buffer.is_some() || session.as_mut().is_body_empty() {
    send_body_to_pipe(...)
}

For a non-empty body that was already consumed before forwarding, both conditions are false. Pingora skips the initial body-send path and then enters the normal duplex loop. The downstream side is already done, so read_body_or_idle(done = true) idles instead of producing another body chunk. The upstream waits for the request body, the client waits for a response, and the request can hang until timeout.

Downstream: body already consumed, is_body_done() = true
Upstream:   waiting for request body
Proxy:      idle(), waiting for downstream close
Result:     request stalls until timeout

The missing behavior is a terminal request_body_filter() callback after a non-empty body has already been consumed by the implementation. Without that callback, the implementation has no chance to emit a transformed or replayed body to upstream.

Impact

Any ProxyHttp implementation that pre-reads a request body and expects to provide the upstream body through request_body_filter() can stall on non-empty requests.

The current workaround is to call enable_retry_buffering() before the pre-read and then rely on get_retry_buffer() to trigger the initial body-send path. That workaround is not ideal:

  • Retry buffering is meant for upstream retry/reuse decisions, not pre-read body replay.
  • The retry buffer is capped by BODY_BUF_LIMIT at 64 KiB.
  • Bodies larger than the retry buffer limit can be truncated or become unavailable for retry-buffer replay.
  • Increasing the retry buffer limit does not fix the lifecycle issue; it only stretches a workaround.

Options Considered

Option 1: Trigger the initial body-send path when a non-empty downstream body is already done

Add a small condition to the initial body-send gate in the h1, h2, and custom forwarding paths:

let downstream_done = downstream_state.is_done();
let body_empty = session.as_mut().is_body_empty();
let body_already_consumed = downstream_done && !body_empty;

if buffer.is_some() || body_empty || body_already_consumed {
    send_body_to_pipe(session, buffer, downstream_done, ...).await?;
}

For h2 and custom transports, preserve existing empty-body behavior and add only the non-empty already-consumed case:

let downstream_done = downstream_state.is_done();
let body_empty = session.as_mut().is_body_empty();
let body_already_consumed = downstream_done && !body_empty;

if buffer.is_some() || body_already_consumed {
    send_body_to2(...).await?;
}

This gives request_body_filter() one terminal callback with:

body = None
end_of_stream = true

The implementation can then emit its buffered/transformed body during that callback.

Pros:

  • Smallest practical bug fix.
  • No new public API.
  • No new fields on Session.
  • No new allocations.
  • Does not change normal streaming/duplex body forwarding.
  • Does not rely on retry buffering or BODY_BUF_LIMIT.
  • Keeps retry-buffer behavior unchanged because the retry-buffer condition still wins.

Cons:

  • Intent is implicit unless comments/tests make the lifecycle clear.
  • Pingora still relies on the ProxyHttp implementation to emit the replay/transformed body from request_body_filter().
  • If an implementation pre-reads a body and forgets to emit a body on the terminal callback, upstream still receives an empty body. This is already true for implementations that drop body data in request_body_filter().

Option 2: Add an explicit pre-read replay API such as set_preread_body()

Add a new API on Session that lets a ProxyHttp implementation hand Pingora the pre-read body, for example:

session.set_preread_body(chunks);

Pingora would then own replaying those chunks to upstream before entering the normal duplex loop.

Pros:

  • Intent is explicit: the implementation tells Pingora it has pre-read body data to send upstream.
  • Pingora owns replay mechanics instead of relying on the implementation to emit data from request_body_filter().
  • Could support multi-chunk replay more naturally.
  • Easier to document as a first-class pre-read body feature.

Cons:

  • Larger API and transport-loop change.
  • Requires new storage on Session or another per-request structure.
  • Needs design decisions around whether replayed chunks should pass through request_body_filter() again or bypass it.
  • Needs explicit interaction rules with retry buffering.
  • Needs careful header/content-length semantics if replayed body size changes.
  • More review surface than a narrow lifecycle bug fix.

Option 3: Make BODY_BUF_LIMIT configurable or growable

Increase, configure, or dynamically grow the retry buffer so the existing retry-buffer workaround can hold larger pre-read bodies.

Pros:

  • No new replay API.
  • Uses an existing code path.
  • May help applications that already rely on retry buffering.

Cons:

  • Semantically wrong for this issue: retry buffering is for retries, not pre-read body replay.
  • Does not fix the fact that Pingora skips the terminal body-filter callback when a body is already consumed.
  • A fixed larger limit can still truncate bodies above the configured cap.
  • A growable retry buffer changes memory pressure for retryable requests, not just pre-read use cases.
  • Changing retry-buffer truncation behavior risks affecting existing retry/reuse logic.

Option 4: Add an explicit flag to request a terminal body-filter callback

Add a smaller API than set_preread_body(), for example:

session.enable_preconsumed_body_filter_callback();

Then the forwarding loop would enter the initial body-send path when that flag is set.

Pros:

  • More explicit than Option 1.
  • Much smaller than set_preread_body().
  • No body storage added to Pingora.
  • No retry-buffer abuse.
  • Behavior changes only when the implementation opts in.

Cons:

  • Adds a new public API.
  • The implementation can forget to call it.
  • Pingora still does not own replay; it only guarantees the terminal callback.
  • Slightly harder to justify as a generic lifecycle fix than Option 1.

Recommendation

Recommend Option 1 as the first upstream fix, with the refined condition:

let downstream_done = downstream_state.is_done();
let body_empty = session.as_mut().is_body_empty();
let body_already_consumed = downstream_done && !body_empty;

Then:

  • h1 should enter initial body-send when buffer.is_some() || body_empty || body_already_consumed.
  • h2/custom should enter initial body-send when buffer.is_some() || body_already_consumed, preserving existing empty-body behavior.

This is the most likely upstream-acceptable patch because it is a narrow lifecycle bug fix. It avoids new API surface, avoids changing retry-buffer semantics, and avoids adding body storage to Session.

If maintainers prefer explicit opt-in behavior, Option 4 is the best fallback. Option 2 is a reasonable longer-term design if Pingora wants first-class pre-read body replay ownership. Option 3 should not be used as the fix for this issue.

Implementation Notes

The terminal callback produced by Option 1 is a single callback with body = None and end_of_stream = true.

A ProxyHttp implementation that pre-reads the body must emit the complete buffered or transformed body during that callback. For multi-chunk pre-read bodies, the implementation should coalesce the body or otherwise ensure the full upstream body is emitted in that one callback.

The implementation should include comments near the condition explaining that this path exists for request bodies consumed before the forwarding loop starts, not for normal empty-body requests.

Suggested Tests

Minimum coverage should prove the generic Pingora behavior rather than a specific downstream project behavior.

Test Purpose
h1 pre-consumed non-empty body receives terminal request_body_filter() callback Proves the h1 lifecycle bug is fixed.
h2 pre-consumed non-empty body receives terminal request_body_filter() callback Proves protocol parity.
custom transport pre-consumed non-empty body receives terminal request_body_filter() callback Proves custom transport parity if practical in existing tests.
Body emitted by request_body_filter() reaches upstream Proves the fix forwards a replayed/transformed body, not only that the callback fires.
Body larger than 64 KiB works without retry buffering Proves the fix does not depend on BODY_BUF_LIMIT.
Empty-body h2/custom behavior remains unchanged Prevents accidental new callbacks for naturally empty requests.
Retry-buffer path still works when get_retry_buffer() returns data Prevents retry behavior regression.

What This Does Not Change

  • Normal request streaming is unchanged when the downstream body has not already been consumed.
  • Retry-buffer behavior is unchanged.
  • Empty-body h1 behavior is unchanged.
  • h2/custom empty-body behavior should remain unchanged if the refined condition is used.
  • No new body buffering is introduced by Pingora.
  • No new public API is required for the recommended fix.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Fields

No fields configured for Bug.

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions