router: fix a bug where internal redirect will hang up request or unexpected redirect by wbpcode · Pull Request #44154 · envoyproxy/envoy

wbpcode · 2026-03-28T14:39:15Z

Commit Message: router: fix a bug where internal redirect will hang up request or unexpected redirect
Additional Description:

To close #44114

The PR fixed some bugs that introduced in the #40254 and also other old bug existed for a while.

In previous implementation:

It's possible the size of first chunk of request is larger than the buffer limit and if retry and internal redirect are enabled at same time (if version <= 1.35, only need the internal redirect be enabled), no any data will be added to filter chain buffer and the decodingBuffer() will always return nullptr. This will finally result in unexpected redirection because decodingBuffer() == nullptr be used as a flag that could redirect.
If version > 1.35, if only internal redirect is enabled, the request body will always be added to filter chain buffer (buffering flag is not set correctly) and will hang up the request (because the readDisable(true) and TCP flow control) if the buffer is overflowed and the request is still not completed.
If version > 1.35 and both internal redirect and retry policy are enabled, then the chunk X that trigger the buffer overflow will be skipped, but all other chunks (the chunks before chunk X and chunks after chunk X) will be added to filter chain buffer and finally may hang up the request. This is a little more complex:
- If chunk X triggered overflow, then the retry state will be reset and buffering flag will be false, so chunk X will be skipped correctly.
- After chunk X, if new chunks coming, because the retry state was reset, then the request will enter the internal redirect only's logic and the buffering flag will not be set correctly and result in problem.

Risk Level: mid, core logic change.
Testing: unit.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

wbpcode · 2026-03-28T14:40:33Z

This at least need to be back ported to 1.36 and 1.37.

wbpcode · 2026-03-28T15:13:00Z

-  if (downstream_end_stream_ && (!request_buffer_overflowed_ || !callbacks_->decodingBuffer()) &&
-      location != nullptr &&
+  if (downstream_end_stream_ && (!request_buffer_overflowed_) && location != nullptr &&


The !request_buffer_overflowed_ || !callbacks_->decodingBuffer() will result in unexpected redirection because it's possible the the request has body but first chunk overflow and the decodingBuffer() still be nullptr.

wbpcode · 2026-03-28T15:14:46Z

-  // Check if buffer overflow occurred and override error details accordingly
-  if (request_buffer_overflowed_) {
-    code = Http::Code::InsufficientStorage;
-    body = "exceeded request buffer limit while retrying upstream";
-    details = StreamInfo::ResponseCodeDetails::get().RequestPayloadExceededRetryBufferLimit;
-  }


The buffer overflow will affect retry and redirection. But if upstream abort (reset, connection failure), we should expose the actual error from upstream to client. This is also introduced at #40254

wbpcode · 2026-03-28T15:15:26Z

-  EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillOnce(Return(10));
-
-  NiceMock<Http::MockRequestEncoder> encoder1;
-  Http::ResponseDecoder* response_decoder = nullptr;
-  expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10);
-
-  Http::TestRequestHeaderMapImpl headers{
-      {"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}, {"myheader", "present"}};
-  HttpTestUtility::addDefaultHeaders(headers);
-  router_->decodeHeaders(headers, false);


This PR also removed some repeated tests for buffer/retry.

wbpcode · 2026-03-30T02:16:53Z

/retest

agrawroh

LGTM, Thank You for fixing it.

agrawroh · 2026-03-30T08:48:47Z

+
+  // In production, the HCM recreateStream would have called this.
+  router_->onDestroy();
+  EXPECT_FALSE(callbacks_.streamInfo().filterState()->hasDataWithName("num_internal_redirects"));


Should we also verify that retry_or_shadow_abandoned gets incremented as we would increment that now per the new flow?

EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("retry_or_shadow_abandoned").value()); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("upstream_internal_redirect_failed_total").value());

agrawroh · 2026-03-30T08:49:57Z

-  // request.
-  if (would_exceed_buffer && retry_enabled && !is_redirect_only && !request_buffer_overflowed_) {
+  // Handle buffer overflow.
+  if (buffering && would_exceed_buffer && !request_buffer_overflowed_) {


I think we could just remove !request_buffer_overflowed_ from here as it's already covered as part of buffering?

good point. 👍

agrawroh · 2026-03-30T08:50:17Z

+  Http::ResponseDecoder* response_decoder = nullptr;
+  expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10);
+
+  // Enable redirects also. This feature will not be used but will affects buffer logic.


Suggested change

// Enable redirects also. This feature will not be used but will affects buffer logic.

// Enable redirects also. This feature will not be used but will affect buffer logic.

agrawroh · 2026-03-30T08:51:29Z

  Buffer::OwnedImpl body("t");
  router_->decodeData(body, false);
+  Buffer::OwnedImpl body2("t");
+  // Ensure the second chunk also isn't buffered and triggers the retry logic again.


Suggested change

// Ensure the second chunk also isn't buffered and triggers the retry logic again.

// Ensure the second chunk also isn't buffered and triggers the retry logic again.

// Ensure subsequent chunks after overflow are also not buffered.

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

wbpcode · 2026-04-01T01:36:06Z

gently ping @yanavlasov

wbpcode · 2026-04-01T01:42:57Z

Hah, I found @yanavlasov recently no activity on github. Let me ping @ggreenway

phlax · 2026-04-02T18:06:32Z

/backport

…xpected redirect (envoyproxy#44154)

…xpected redirect (envoyproxy#44154) Signed-off-by: wbpcode <wbphub@gmail.com>

…xpected redirect (#44154) (#44304) Commit Message: router: fix a bug where internal redirect will hang up request or unexpected redirect (#44154) Additional Description: Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] Signed-off-by: wbpcode <wbphub@gmail.com>

…xpected redirect (#44154) (#44303) Commit Message: router: fix a bug where internal redirect will hang up request or unexpected redirect (#44154) Additional Description: Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] Signed-off-by: wbpcode <wbphub@gmail.com>

…xpected redirect (envoyproxy#44154) Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>

…xpected redirect (envoyproxy#44154)

wbpcode added 3 commits March 28, 2026 13:16

add tests first

345d346

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

add more tests

0d0d940

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

router: fix a bug where internal redirect will hang up request

7f21afc

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

wbpcode assigned yanavlasov, ggreenway and agrawroh Mar 28, 2026

wbpcode added the backport/review Request to backport to stable releases label Mar 28, 2026

wbpcode commented Mar 28, 2026

View reviewed changes

agrawroh previously approved these changes Mar 30, 2026

View reviewed changes

address comments

4c019dd

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

wbpcode dismissed agrawroh’s stale review via 4c019dd March 30, 2026 12:03

agrawroh approved these changes Mar 30, 2026

View reviewed changes

ggreenway removed their assignment Apr 1, 2026

mattklein123 approved these changes Apr 1, 2026

View reviewed changes

wbpcode merged commit 4f56daf into envoyproxy:main Apr 1, 2026
29 checks passed

wbpcode deleted the dev-fix-router branch April 2, 2026 10:12

wbpcode added a commit to wbpcode/envoy that referenced this pull request Apr 7, 2026

router: fix a bug where internal redirect will hang up request or une…

ab0a304

…xpected redirect (envoyproxy#44154)

wbpcode added a commit to wbpcode/envoy that referenced this pull request Apr 7, 2026

router: fix a bug where internal redirect will hang up request or une…

5b63102

…xpected redirect (envoyproxy#44154)

wbpcode added a commit to wbpcode/envoy that referenced this pull request Apr 7, 2026

router: fix a bug where internal redirect will hang up request or une…

9273c1a

…xpected redirect (envoyproxy#44154) Signed-off-by: wbpcode <wbphub@gmail.com>

wbpcode added a commit to wbpcode/envoy that referenced this pull request Apr 7, 2026

router: fix a bug where internal redirect will hang up request or une…

a6c333e

…xpected redirect (envoyproxy#44154) Signed-off-by: wbpcode <wbphub@gmail.com>

This was referenced Apr 7, 2026

[bp/1.36] router: fix a bug where internal redirect will hang up request or unexpected redirect (#44154) #44303

Merged

[bp/1.37] router: fix a bug where internal redirect will hang up request or unexpected redirect (#44154) #44304

Merged

phlax removed the backport/review Request to backport to stable releases label Apr 9, 2026

nshipilov pushed a commit to nshipilov/envoy that referenced this pull request Apr 13, 2026

router: fix a bug where internal redirect will hang up request or une…

2cf075c

…xpected redirect (envoyproxy#44154) Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>

krinkinmu pushed a commit to grnmeira/envoy that referenced this pull request Apr 20, 2026

router: fix a bug where internal redirect will hang up request or une…

611129a

…xpected redirect (envoyproxy#44154)

	// Enable redirects also. This feature will not be used but will affects buffer logic.
	// Enable redirects also. This feature will not be used but will affect buffer logic.

	// Ensure the second chunk also isn't buffered and triggers the retry logic again.
	// Ensure the second chunk also isn't buffered and triggers the retry logic again.
	// Ensure subsequent chunks after overflow are also not buffered.

Conversation

wbpcode commented Mar 28, 2026

Uh oh!

wbpcode commented Mar 28, 2026

Uh oh!

wbpcode Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

wbpcode Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 30, 2026

Uh oh!

agrawroh left a comment

Choose a reason for hiding this comment

Uh oh!

agrawroh Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

agrawroh Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

wbpcode Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

agrawroh Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

agrawroh Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Apr 1, 2026

Uh oh!

wbpcode commented Apr 1, 2026

Uh oh!

Uh oh!

phlax commented Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wbpcode Mar 28, 2026 •

edited

Loading