ext_proc: closing the gRPC stream ASAP once no more external processing needed #41425

yanjunxiang-google · 2025-10-08T16:33:26Z

This is to address a portion of the issue: #37088, i.e, close the gRPC stream once no further external processing needed.

Currently the ext_proc gRPC stream is opened when the 1st ProcessingRequest is sent to the ext_proc server. And it is closed during ext_proc filter destruction. This is wasting resource on both Envoy and ext_proc server side. For example, if envoy is configured to only send request headers, the gRPC stream is left open until all the way to the response is processed.

This PR is trying to close the ext_proc gRPC stream once Envoy detects no more external processing needed.

needed Signed-off-by: Yanjun Xiang <[email protected]>

Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google · 2025-10-08T18:32:06Z

/retest

yanjunxiang-google · 2025-10-08T20:05:49Z

/assign @yanavlasov @tyxia @stevenzzzz

Signed-off-by: Yanjun Xiang <[email protected]>

yanavlasov

/wait

yanavlasov · 2025-10-09T13:01:03Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+
 } // namespace

+bool Filter::noExternalProcessInEncoding() const {


Why not add this method to the EncodingProcessorState ?

yanavlasov · 2025-10-09T13:02:39Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+// Close the gRPC stream if the last ProcessingResponse is received.
+void Filter::closeGrpcStreamIfLastRespReceived(
+    const std::unique_ptr<envoy::service::ext_proc::v3::ProcessingResponse>& response) {
+  bool last_response = false;


This is something that should part of the ProcessorState. After processing a response message it should return a flag indicating that a terminal state has been reached and gRPC stream can be closed.

hmm, the idea is to decide whether to terminate the gRPC stream once the ext_proc filter receives the ProcessingResponse, i.e, at the center place inside Filter::onReceiveMessage(), as Envoy has enough information here to decide whether to terminate it then. Basically, this is determined by 1)whether the end-of-stream is received, 2) and whether Envoy needs to send more data to the ext_proc server based on filter configuration. Hooking the logic to ProcessorState will unavoidably spread the logic to each of the handling header response, handling body response(different modes), and handling trailer response, which will make it error prone and hard to maintain.

Signed-off-by: Yanjun Xiang <[email protected]>

stevenzzzz

Hooah. You are fighting a beast here. :)

high level, since this is a behavior change that impacts prod traffic, could you guard this change with a feature flag?

stevenzzzz

left some comments, pls consider some common cases like(there for sure are more):

CONTINUE_AND_REPLACE finishes one direction's all events.
EOS from trailers implicitly terminates body.
only check if-last-reponse after response is "processed", on error cases, stream will already be closed.

On a high level, if we have gone this far already, let's also consider to wait-for-trailers after the half close been sent. but that could be in another PR I assume.

stevenzzzz · 2025-10-10T15:02:03Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+                        const envoy::service::ext_proc::v3::ProcessingResponse& response) {
+  switch (state.bodyMode()) {
+  case ProcessingMode::BUFFERED:
+  case ProcessingMode::BUFFERED_PARTIAL:


much appreciate if you could add a method comment talking about what this method is about, and comment heavily on which modes are not supported yet.

stevenzzzz · 2025-10-10T15:03:01Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+    break;
+  case ProcessingMode::STREAMED:
+    if (!state.chunkQueue().empty()) {
+      return state.chunkQueue().queue().front()->end_stream;


do we need to consider the corner case that body is done, because we received trailers already here?

There are other corner cases:
If CONTINUE_AND_REPLACE is set, it also means we can safely half-close.

Moved the stream optimization logic to the end of the onReceiveMessage, i.e, after the ProcessingResponse is already processed. This will automatically count in CONTINUE_AN_REPLACE, as such message will modify the filter processing modes configuration, thus be counted in the check.

do we need to consider the corner case that body is done, because we received trailers already here?

This is a little bit tricky. Can we skip this corner case? If we missed closing the stream in the decoding path, the stream closing added at the encodeHeaders() will catch it and cleanup any way.

stevenzzzz · 2025-10-10T15:10:08Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+  case ProcessingMode::FULL_DUPLEX_STREAMED: {
+    const envoy::service::ext_proc::v3::BodyResponse* body_response = nullptr;
+    if (response.has_request_body()) {
+      body_response = &response.request_body();


nit, there should not be a if-else here.
instead, use the state's type to get the real body response field?

I think the corner cases above apply here as well: trailers received, CONTINUE_AND_REPLACE set.

nit, there should not be a if-else here. instead, use the state's type to get the real body response field?

done

stevenzzzz · 2025-10-10T15:16:26Z

source/extensions/filters/http/ext_proc/ext_proc.h

+  void closeStreamMaybeGraceful();
+
+  // Close the gRPC stream if the last ProcessingResponse is received.
+  void closeGrpcStreamIfLastRespReceived(


nit, you could use "const ProcessingResponse&" here to save some dereferences.

stevenzzzz · 2025-10-10T15:19:10Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+  bool last_response = false;
+
+  switch (response->response_case()) {
+  case ProcessingResponse::ResponseCase::kRequestHeaders:


if the ext_proc sends invalid response, e.g. requestHeaders on Body events, it's a bad state that would cause stream close already.
Shall we evaluate all the cases after the response is processed?

Moving the stream closing logic to the end of onReceiveMessage() automatically count in this as well as stream_ become nullptr in these conditions.

stevenzzzz · 2025-10-10T15:20:37Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+  switch (response->response_case()) {
+  case ProcessingResponse::ResponseCase::kRequestHeaders:
+    if ((decoding_state_.hasNoBody() ||
+         (decoding_state_.bodyMode() == ProcessingMode::NONE && !decoding_state_.sendTrailers())) &&


other case: body mode confgiured, but trailers received.

trailer mode set, but EoS seen in headers or Body already

I added the cases you mentioned in the TBD list in the PR description, please take a look. The goal of this PR is to get the normal case optimization working, then we can deal with the corner cases in the future enhancement. BTW, if some corner cases are missed in the decoding path, the stream closing logic added in encodeHeaders() will catch them all.

stevenzzzz · 2025-10-10T15:24:03Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+    }
+    break;
+  case ProcessingResponse::ResponseCase::kRequestBody:
+    if (isLastBodyResponse(decoding_state_, *response) && encoding_state_.noExternalProcess()) {


what if trailers are configured?

Here we detect EoS is received with body, so no trailers in this request. Thus even filter is configured to send trailers, the external processing in this direction is completed.

Signed-off-by: Yanjun Xiang <[email protected]>

botengyao · 2025-10-14T12:21:30Z

Waiting for addressing comments.

/wait

Signed-off-by: Yanjun Xiang <[email protected]>

repokitteh-read-only · 2025-10-14T21:36:03Z

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

🐱

Caused by: #41425 was synchronize by yanjunxiang-google.

see: more, trace.

yanjunxiang-google · 2025-10-15T17:26:08Z

left some comments, pls consider some common cases like(there for sure are more):

CONTINUE_AND_REPLACE finishes one direction's all events.

EOS from trailers implicitly terminates body.

only check if-last-reponse after response is "processed", on error cases, stream will already be closed.

On a high level, if we have gone this far already, let's also consider to wait-for-trailers after the half close been sent. but that could be in another PR I assume.

is considered by moving the stream closing check at the end of the onReceiveMessage().
meaning: if trailers is received, if the body response is received, and if no more body chunks in the state chunk queue, that's the last body response. On top of that, if the trailer processing mode is SKIP, then external processing in the current direction is done. I am leaving this case optimization to the future enhancement. Note, we also have a catch all optimization added in encodeHeaders() which will help this situation a little bit.
is considered by moving the stream closing check at the end of the onReceiveMessage(), as in this case, stream_ is a nullptr.

Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google · 2025-10-15T20:36:07Z

Hooah. You are fighting a beast here. :)

high level, since this is a behavior change that impacts prod traffic, could you guard this change with a feature flag?

Thanks for the comments! done

Signed-off-by: Yanjun Xiang <[email protected]>

tyxia

Thanks for working on this!

+1 on runtime guard to protect this change,. The processing mode/ext_proc is getting more and more complicated )

tyxia · 2025-10-16T01:24:21Z

source/extensions/filters/http/ext_proc/ext_proc.cc

    processing_status = encoding_state_.handleHeadersResponse(response->response_headers());
    break;
  case ProcessingResponse::ResponseCase::kRequestBody:
+    if (response->has_request_body()) {


qq; why we need this response->has_request_body . Do we receive empty response when response case is set to kRequestBody?

That's a good point! done

tyxia · 2025-10-16T01:50:59Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+  // If there is no external processing configured in the encoding path,
+  // closing the gRPC stream if it is still open.
+  if (encoding_state_.noExternalProcess()) {
+    closeStreamMaybeGraceful();


if noExternalProcess can you simply return FilterHeadersStatus::Continue here and avoids the rest of handling in this function?

That's a good point. However, I prefer if noExternalProcess, just close the stream and do not change the other logic, like still set below flag:
if (end_stream) {
encoding_state_.setCompleteBodyAvailable(true);
}
Also prefer to still have the trace logs if response_header sending is SKIP: ENVOY_STREAM_LOG(trace, "encodeHeaders: Skipped header processing", *decoder_callbacks_);

I am curious if encoding_state_.setCompleteBodyAvailable(true); and all other logics are still needed when noExternalProcess . Basically there is no callout at encode path at all. ext_proc becomes a pass-through filter, no?

But if there are any corner cases you can think of, yes I agree going through rest of function is safer

tyxia · 2025-10-16T01:52:36Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+void Filter::closeGrpcStreamIfLastRespReceived(const ProcessingResponse& response,
+                                               const bool is_last_body_resp) {
+
+  if (stream_ == nullptr || !Runtime::runtimeFeatureEnabled(


checking if (stream_ == nullptr looks a bit strange to me. I assume it means the stream has already been closed?
Should this case already been handled by other code path?

Yeah, there are some cases in onReceiveMessage() which already close the stream. For example, ImmediateResponse handling, or a header response contains invalid mutation. For these cases the stream is already closed, so no need to go through this logic again. For other normal cases, the stream is not closed.

My point is closed stream should be handled by closeStream() below. i.e., this check is not needed.

But I understand your point of avoiding going through the rest of the function, which is fair. Maybe consider adding comment to clarify the check

Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google · 2025-10-17T14:23:42Z

The initial optimization does not apply for a few scenarios listed below, which is TBD in the future:

For BUFFERED and BUFFERED_PARTIAL body mode
During header response processing, if request does not have body but has trailer, and trailer is received, at same time if body sending mode is not NONE, and trailer sending mode is SKIP(unlikely request pattern, corner case)
During header response processing, if request has body and end_of_stream is received with body, at same time if body sending mode is NONE, and trailer sending mode is SEND(unlikely processing mode configuration, corner case).

ext_proc: closing the gRPC stream ASAP once no more external processing

755e111

needed Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google requested review from mattklein123, tyxia and yanavlasov as code owners October 8, 2025 16:33

fix format

f6b536b

Signed-off-by: Yanjun Xiang <[email protected]>

repokitteh-read-only bot assigned stevenzzzz, tyxia and yanavlasov Oct 8, 2025

yanjunxiang-google added 5 commits October 8, 2025 21:55

set processing_complete_ to true

f8eba26

Signed-off-by: Yanjun Xiang <[email protected]>

adding server close test

dcf5c6c

Signed-off-by: Yanjun Xiang <[email protected]>

skip onGrpcClose() if processing_complete_ true

07be305

Signed-off-by: Yanjun Xiang <[email protected]>

fix test

49fbe96

Signed-off-by: Yanjun Xiang <[email protected]>

fix format

33a1d74

Signed-off-by: Yanjun Xiang <[email protected]>

yanavlasov requested changes Oct 9, 2025

View reviewed changes

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

address comments

35ef112

Signed-off-by: Yanjun Xiang <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 9, 2025

stevenzzzz reviewed Oct 10, 2025

View reviewed changes

fix test

6d4aa09

Signed-off-by: Yanjun Xiang <[email protected]>

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

moving stream close to the end of onReceiveMessage()

921c40b

Signed-off-by: Yanjun Xiang <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 14, 2025

yanjunxiang-google added 3 commits October 14, 2025 21:16

addressing comments

bf71363

Signed-off-by: Yanjun Xiang <[email protected]>

adding runtime protection

2e8ab46

Signed-off-by: Yanjun Xiang <[email protected]>

fix format

b8934d1

Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google added 2 commits October 15, 2025 18:02

adding release notes

4f6542b

Signed-off-by: Yanjun Xiang <[email protected]>

merge upstream main

8986c15

Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google added 2 commits October 15, 2025 21:19

fix format

e713075

Signed-off-by: Yanjun Xiang <[email protected]>

fix format

1a55c23

Signed-off-by: Yanjun Xiang <[email protected]>

tyxia reviewed Oct 16, 2025

View reviewed changes

address comments

826a6cc

Signed-off-by: Yanjun Xiang <[email protected]>


		} // namespace

		bool Filter::noExternalProcessInEncoding() const {

ext_proc: closing the gRPC stream ASAP once no more external processing needed #41425

Are you sure you want to change the base?

ext_proc: closing the gRPC stream ASAP once no more external processing needed #41425

Uh oh!

Conversation

yanjunxiang-google commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Oct 8, 2025

Uh oh!

yanjunxiang-google commented Oct 8, 2025

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Oct 9, 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!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Oct 14, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botengyao commented Oct 14, 2025

Uh oh!

repokitteh-read-only bot commented Oct 14, 2025

Uh oh!

yanjunxiang-google commented Oct 15, 2025

Uh oh!

yanjunxiang-google commented Oct 15, 2025

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Oct 8, 2025 •

edited

Loading

yanjunxiang-google Oct 9, 2025 •

edited

Loading

yanjunxiang-google Oct 14, 2025 •

edited

Loading

yanjunxiang-google Oct 16, 2025 •

edited

Loading

yanjunxiang-google Oct 16, 2025 •

edited

Loading

yanjunxiang-google Oct 16, 2025 •

edited

Loading

yanjunxiang-google commented Oct 17, 2025 •

edited

Loading