fix(filter): create filter chain before calling onLocalReply

Project: envoyproxy/envoy | Language: C++ | Concepts: error-handling, testing-strategy, refactoring | Difficulty: mid
Link: https://github.com/envoyproxy/envoy/pull/45988

The problem

When Envoy needs to send a local reply (an error response generated internally), it may do so before the HTTP filter chain is constructed. The previous implementation would call onLocalReply on filters before the chain existed, so filters would miss this notification. This meant that features relying on onLocalReply, such as access logging or cleanup, could be skipped in early error cases.

Why it's non-trivial

A naive fix would just call onLocalReply when needed, but if the filter chain hasn't been constructed yet, there are no filters to notify. Simply moving code around risks breaking the order of operations, or causing side effects if the chain is created too late or too early. Ensuring the filter chain is always present before any local reply logic is invoked is necessary for correctness, but must be done carefully to avoid double-creation or skipping filters.

The approach

1. Move filter chain creation before onLocalReply is called

The main fix is in DownstreamFilterManager::sendLocalReply. The filter chain is now created before any local reply handling, ensuring filters always exist to receive the onLocalReply call.

 void DownstreamFilterManager::sendLocalReply(
   if (filter_manager_callbacks_.isHalfCloseEnabled()) {
     state_.decoder_filter_chain_aborted_ = true;
   }
+  // For early error handling, do a best-effort attempt to create a filter chain
+  // to ensure access logging. If the filter chain already exists this will be
+  // a no-op.
+  createDownstreamFilterChain();

   streamInfo().setResponseCodeDetails(details);
   StreamFilterBase::LocalReplyData data{code, grpc_status, details, false};

This ensures that even for early errors, the filter chain is present before onLocalReply is invoked.

2. Remove redundant filter chain creation from other local reply helpers

Previously, both prepareLocalReplyViaFilterChain and sendLocalReplyViaFilterChain also called createDownstreamFilterChain. Now that the main entry point always creates the chain first, these redundant calls are removed to avoid unnecessary or repeated creation.

-  // For early error handling, do a best-effort attempt to create a filter chain
-  // to ensure access logging. If the filter chain already exists this will be
-  // a no-op.
-  createDownstreamFilterChain();

This cleans up the logic so the chain is created exactly once, in the right place.

3. Update and expand tests to verify the new behavior

Several tests are updated to expect onLocalReply to be called on filters, even in early error cases:

+  EXPECT_CALL(*filter, onLocalReply(_));

This is added to various tests to assert that filters now always see local replies.

Integration tests are also improved to verify that filters can observe local replies even when the error happens before the chain is normally constructed:

+  // The illegal Content-Length value will cause a local reply to be sent before the
+  // filter chain is constructed. In that case the filter manager will construct the filter chain
+  // just before calling onLocalReply.
+  // HTTP2 just resets the stream instead, so we don't test it.
+  if (downstreamProtocol() == Http::CodecType::HTTP2) {
+    return;
+  }
+  {
+    default_request_headers_.addCopy("Content-Length", "-1");
+    auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+    ASSERT_TRUE(response->waitForEndStream());
+    ASSERT_TRUE(response->complete());
+    EXPECT_FALSE(response->headers().get(Http::LowerCaseString{"saw-on-local-reply"}).empty());
+  }

This block verifies that a filter can observe a local reply even when the error is detected before the filter chain would usually be constructed.

From the review thread

abeyad:
What this seems to mean is that right now, a change was made such that when we previously would invoke the on_data_ callback, instead now we invoke the on_error_ callback. That's a behavior change for API consumers of Envoy Mobile.

Exactly what are the circumstances under which an on_data_ call now becomes an on_error_ call? I think we will need to proceed with caution and make sure the downstream API consumers are not effected by the behavior change.

cc @danzh2010 any thoughts on this change?

syntax-case:
The behavior changes in the case where a local reply is generated early during request processing, before the HTTP filter chain has been instantiated.
One example is an invalid Content-Length header in HTTP/1.1.
In Envoy mobile it can apparently also happen when a cleartext request is attempted when it's not permitted.

Previously this would have resulted in the client getting a response from envoy. Now, since the filter chain is always informed about the local reply, the mobile filters will instead reset the stream.

I'm don't know much about mobile, so I don't know which other envoy features could be configured here that could also cause a behavior change, or how much freedom the clients have to produce invalid requests.

abeyad:
I think what you described sounds like the right approach and shouldn't break our existing users, so I'm approving. Thanks for the explanation!

Takeaways

• Always ensure dependencies are initialized before invoking dependent logic. If a callback or handler expects an object (like a filter chain) to exist, create it first—even in error or early-exit cases.
• Centralize side-effectful initialization to avoid redundant or inconsistent state. By moving filter chain creation to a single, well-defined place, the code avoids accidental double-creation and makes the call sequence clearer.
• Update tests to cover edge cases introduced by error handling changes. When error handling order changes, tests should verify that all relevant callbacks (like onLocalReply) are invoked in all scenarios, including early errors.