http2: send graceful goaways on close #41284

kanurag94 · 2025-09-30T10:28:09Z

Description

With this change envoy will send one GOAWAY frame with a last_stream_id set to 2^31-1, as described in RFC 9113. After a reasonable interval (defaults to 1s), envoy will send a followup GOAWAY frame with last_stream_id set to the highest received stream ID at that point in time.
This interval is configurable using graceful_goaway_timeout field.

Commit Message: http2: send graceful GoAway on close
Additional Description: According to RFC 9113, envoy should send a goaway with last_stream_id set to 2^31-1 before sending a final goaway with the highest received stream ID.
Risk Level: Low
Testing: Tests added. Manually verified.
Docs Changes: NA
Release Notes: Changelog added
Platform Specific Features:
Runtime guard: envoy.reloadable_features.http2_graceful_goaway
Fixes #39876
[Optional API Considerations:]

Signed-off-by: Anurag Aggarwal <[email protected]>

repokitteh-read-only · 2025-09-30T10:28:15Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #41284 was opened by kanurag94.

see: more, trace.

repokitteh-read-only · 2025-09-30T10:28:21Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #41284 was opened by kanurag94.

see: more, trace.

Signed-off-by: Anurag Aggarwal <[email protected]>

repokitteh-read-only · 2025-09-30T17:46:16Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #41284 was synchronize by kanurag94.

see: more, trace.

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 · 2025-10-01T04:10:21Z

/retest

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 · 2025-10-01T13:39:33Z

/retest

Signed-off-by: Anurag Aggarwal <[email protected]>

antoniovleonti · 2025-10-01T14:32:07Z

My understanding is that a graceful goaway is sent at the beginning of the drain sequence via Connection::shutdownNotice() (check code here) That eventually calls this adapter function which in both nghttp2 and oghttp2 sends the goaway frame with the max stream ID as you have done here.

Can you explain why this is not sufficient?

kanurag94 · 2025-10-01T14:59:52Z

My understanding is that a graceful goaway is sent at the beginning of the drain sequence via Connection::shutdownNotice() (check code here) That eventually calls this adapter function which in both nghttp2 and oghttp2 sends the goaway frame with the max stream ID as you have done here.

Can you explain why this is not sufficient?

On connection drain the shutdown notice in both nghttp2/oghttp2 adapter sends goaways correctly. If you look at the original issue for this PR #39876, an important part is filters triggering GoAways immediately. This PR solves that part.
For nghttp2/oghttp2 connection drain and subsequent shutdown, this goaway will not be called AFAICS.

antoniovleonti

I'm sorry! I missed the issue in the description. Thank you for the explanation.

antoniovleonti · 2025-10-01T15:10:06Z

test/common/http/http2/codec_impl_test.cc

+  EXPECT_EQ(1, server_stats_store_.counter("http2.goaway_sent").value());
+}
+
+TEST_P(Http2CodecImplTest, GracefulGoAwayCausesOutboundFlood) {


I think it would be prudent to add some tests showing how this new feature and the existing http connection manager drain phase interact.

Could you also add a couple integration tests?

Thanks!, I added some integration tests. Please let me know if they are okay.

TY for the added testing!

I think what worries me the most is if it's possible for the drain timer to fire after the graceful shutdown timer or vice versa.

Will that crash the envoy? (e.g. the second timer fires and tries calling a function on a now null object).

Could that potentially cause us to send two final goaways (with the second having a higher stream_id-- which is not allowed according to the spec)?

As far as I understand the code (which is little to be honest) -- the destructor on ConnectionImpl should destroy the timers -> cancelling any pending callbacks -> preventing null object access.

Actually the behavior hasn't changed much. I reverted the change on calling gracefulGoAway on onDrainTimeout. Now only filter or load shedding invoked goaways are changed to graceful. Both goaway paths are separate and non overlapping now. It just changes the manual goaway to be more RFC compliant.

However, I will keep attempting adding test.

1. only call custom gracefulGoAway when triggered from LoadSheddding or Filter 2. Add UTs 3. Add integration tests showing interaction of connection drain and filter invoked Signed-off-by: Anurag Aggarwal <[email protected]>

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 · 2025-10-02T06:35:25Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540.
Please correct me if my understanding is wrong.

cc @birenroy, @antoniovleonti, @yanavlasov

Signed-off-by: Anurag Aggarwal <[email protected]>

antoniovleonti · 2025-10-02T20:16:18Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540. Please correct me if my understanding is wrong.

cc @birenroy, @antoniovleonti, @yanavlasov

https://github.com/envoyproxy/envoy/blob/main/envoy/http/codec.h#L599 This comment seems to imply this is working as intended, but I agree the RFC seems to say this shouldn't be allowed.

kanurag94 · 2025-10-03T05:03:07Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540. Please correct me if my understanding is wrong.
cc @birenroy, @antoniovleonti, @yanavlasov

https://github.com/envoyproxy/envoy/blob/main/envoy/http/codec.h#L599 This comment seems to imply this is working as intended, but I agree the RFC seems to say this shouldn't be allowed.

Thanks for the reference. I see the behavior with nghttp2 vs oghttp2 different.

nghttp2 resets any new streams as soon as it goaway.
oghttp2 seems to accept new streams.

I think this makes the behaviour already inconsistent irrespective what we expect according to the comment, orz.

yanavlasov · 2025-10-07T01:40:49Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540. Please correct me if my understanding is wrong.

cc @birenroy, @antoniovleonti, @yanavlasov

Yes, Envoy does not enforce GOAWAY semantics as it should. We should fix this and make it configurable with default to RFC correct behavior.

yanavlasov · 2025-10-07T01:41:23Z

@kanurag94 please resolve merge conflicts. Will wait for @antoniovleonti LGTM.

/wait

antoniovleonti

LGTM, thank you for the improved testing!

birenroy · 2025-10-07T18:54:46Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540. Please correct me if my understanding is wrong.
cc @birenroy, @antoniovleonti, @yanavlasov

https://github.com/envoyproxy/envoy/blob/main/envoy/http/codec.h#L599 This comment seems to imply this is working as intended, but I agree the RFC seems to say this shouldn't be allowed.

Thanks for the reference. I see the behavior with nghttp2 vs oghttp2 different.

nghttp2 resets any new streams as soon as it goaway.

oghttp2 seems to accept new streams.

I think this makes the behaviour already inconsistent irrespective what we expect according to the comment, orz.

If nghttp2 resets new streams after Envoy sends the initial GOAWAY with max stream ID, then that is a bug in nghttp2.

A server should continue to accept new streams that arrive until it sends the final GOAWAY.

birenroy · 2025-10-07T19:03:41Z

I appreciate the work you've done here to fix the issue in question. Would it be possible to offer two options for the inter-GOAWAY timeout?

time duration specified in config (already done in the current version of the PR)
a round trip time, as measured by a PING frame initiated by the Envoy

wbpcode

Thanks for this great contribution. I added some comments. But I think the core problems are here:

sendGoAwayAndClose() will reset all pending streams directly. It's make no sense to delay the final goaway. That's say, the sendGoAwayAndClose() should not be used if graceful shutdown is necessary. I think it's OK to submit a shutdown notice first for RFC. But it makes no much sense in actual practice.
If we need a graceful shutdown, we should enhance the sendGoAwayAndClose() to provide a graceful mode, then we can redirect the call to the startDrainSequence(). Then the shutdown notice (initial goaway) will be sent immediately. And the final goway will be sent after the drain time is end.

cc @yanavlasov cc @birenroy

/wait

wbpcode · 2025-10-10T14:24:19Z

envoy/http/codec.h

+  /**
+   * Indicate graceful "go away" to the remote. For HTTP/2, this implements RFC-compliant
+   * graceful shutdown. For other protocols, this falls back to regular goAway().
+   */
+  virtual void goAwayGraceful() { goAway(); }


I think you should reuse the shutdownNotice().

wbpcode · 2025-10-10T14:39:46Z

api/envoy/config/core/v3/protocol.proto

+
+  // Timeout for graceful HTTP/2 GOAWAY shutdown sequence.
+  // When graceful shutdown is enabled, Envoy will send an initial GOAWAY frame with
+  // last_stream_id set to 2^31-1, wait for this timeout period, then send a final
+  // GOAWAY frame with the actual highest received stream ID. This allows in-flight
+  // requests to complete gracefully. If set to 0, graceful shutdown is disabled
+  // and the standard immediate GOAWAY behavior is used. Defaults to 1000ms (1 second).
+  google.protobuf.Duration graceful_goaway_timeout = 18;


The connection will be closed directly after the delayed_close_timeout, the that will finally effect the result of the graceful_goaway_timeout.

http2: send graceful goaways on close

5cfd1fd

Signed-off-by: Anurag Aggarwal <[email protected]>

repokitteh-read-only bot added the api label Sep 30, 2025

repokitteh-read-only bot assigned markdroth Sep 30, 2025

run format; add changelog; update tests

2b15c51

Signed-off-by: Anurag Aggarwal <[email protected]>

correct alphabetical ordering of runtime guard

284c938

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 marked this pull request as ready for review September 30, 2025 18:23

correct alphabetical ordering of runtime guard - 2

fe44aad

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 added 5 commits October 1, 2025 10:55

Merge remote-tracking branch 'upstream/main' into graceful-http2-goaway

7006b22

add missing flood test

7d6ea68

Signed-off-by: Anurag Aggarwal <[email protected]>

add runtime guard disabled test

32a19ca

Signed-off-by: Anurag Aggarwal <[email protected]>

run format

5c85dd4

Signed-off-by: Anurag Aggarwal <[email protected]>

Merge remote-tracking branch 'upstream/main' into graceful-http2-goaway

8b65795

kick ci

3641061

Signed-off-by: Anurag Aggarwal <[email protected]>

antoniovleonti suggested changes Oct 1, 2025

View reviewed changes

address review comments:

10c24ff

1. only call custom gracefulGoAway when triggered from LoadSheddding or Filter 2. Add UTs 3. Add integration tests showing interaction of connection drain and filter invoked Signed-off-by: Anurag Aggarwal <[email protected]>

KBaichoo assigned birenroy Oct 1, 2025

kanurag94 added 2 commits October 2, 2025 00:17

try fixing failint uts

5a8404b

Signed-off-by: Anurag Aggarwal <[email protected]>

fix test and point out issue

d26710a

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 added 2 commits October 2, 2025 20:05

fix comments

5a24ddc

Signed-off-by: Anurag Aggarwal <[email protected]>

fix spelling

e39a5d5

Signed-off-by: Anurag Aggarwal <[email protected]>

repokitteh-read-only bot added the waiting label Oct 7, 2025

antoniovleonti approved these changes Oct 7, 2025

View reviewed changes

wbpcode reviewed Oct 10, 2025

View reviewed changes

wbpcode assigned yanavlasov and wbpcode and unassigned markdroth Oct 10, 2025

http2: send graceful goaways on close #41284

Are you sure you want to change the base?

http2: send graceful goaways on close #41284

Uh oh!

Conversation

kanurag94 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

antoniovleonti commented Oct 1, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

antoniovleonti left a comment

Choose a reason for hiding this comment

Uh oh!

antoniovleonti Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kanurag94 Oct 1, 2025 • 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!

antoniovleonti Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kanurag94 Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kanurag94 commented Oct 2, 2025

Uh oh!

antoniovleonti commented Oct 2, 2025

Uh oh!

kanurag94 commented Oct 3, 2025

Uh oh!

yanavlasov commented Oct 7, 2025

Uh oh!

yanavlasov commented Oct 7, 2025

Uh oh!

antoniovleonti left a comment

Choose a reason for hiding this comment

Uh oh!

birenroy commented Oct 7, 2025

Uh oh!

birenroy commented Oct 7, 2025

Uh oh!

wbpcode left a comment • 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 Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

wbpcode Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kanurag94 commented Sep 30, 2025 •

edited

Loading

kanurag94 Oct 1, 2025 •

edited

Loading

wbpcode left a comment •

edited

Loading