http: fix potential FD leak when zombie streams prevent connection close

[wdauchy]

Commit Message:
I am chasing a case where a client sends POST messages on a HTTP1.1 connection, but is being systematically denied. The connection is immediately reused by the client. In some case we seem to trigger a racy case where the number of open file descriptor explodes.

When a stream becomes a zombie (waiting for codec encode completion) and the connection is in DrainState::Closing, the connection is not closed because checkForDeferredClose() is not called after the zombie stream was finally destroyed.

This commonly occurs with HTTP/1.1 connections when:

• ext_authz (or similar filter) denies a request early, sending a 403 response before the full request body is received
• The connection is (potentially) marked DrainState::Closing
• The stream becomes a zombie waiting for the codec to finish encoding
• When onCodecEncodeComplete() fires, doDeferredStreamDestroy() was called but checkForDeferredClose() was not, leaving the connection open

In scenarios where clients try to reuse the same HTTP/1.1 connection and requests are systematically denied (e.g., by ext_authz), this causes rapid FD exhaustion as each denied request leaves an orphaned connection.

The fix adds checkForDeferredClose() calls after zombie stream destruction in both onCodecEncodeComplete() and onCodecLowLevelReset().
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:]

[repokitteh-read-only]

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: #42859 was opened by wdauchy.

see: more, trace.

[botengyao]

@KBaichoo, Kevin, do you want also to take a look for this?

[botengyao]

Thanks for fixing this, and left a high level question.

/wait

[botengyao]

then, should we distinguish all the conditions like in the doEndStream

envoy/source/common/http/conn_manager_impl.cc

Lines 314 to 328 in 25fedad

	if (check_for_deferred_close) {
	// If HTTP/1.0 has no content length, it is framed by close and won't consider
	// the request complete until the FIN is read. Don't delay close in this case.
	const bool http_10_sans_cl =
	(codec_->protocol() == Protocol::Http10) &&
	(!stream.response_headers_ || !stream.response_headers_->ContentLength());
	// We also don't delay-close in the case of HTTP/1.1 where the request is
	// fully read, as there's no race condition to avoid.
	const bool connection_close =
	stream.filter_manager_.streamInfo().shouldDrainConnectionUponCompletion();
	const bool request_complete = stream.filter_manager_.hasLastDownstreamByteReceived();
	// Don't do delay close for HTTP/1.0 or if the request is complete.
	checkForDeferredClose(connection_close && (request_complete || http_10_sans_cl));
	}

to call checkForDeferredClose(false) here as well?

[wdauchy]

Good point. When doEndStream() creates a zombie stream, it does call checkForDeferredClose() with the computed skip_delay_close value, but that call is a no-op because streams_ is possibly not empty yet, so the computed value is effectively lost.

I chose checkForDeferredClose(false) to avoid duplicating that conditional logic, accepting the tradeoff that we always use the delay close path even when it's not strictly necessary.

If you prefer consistency with doEndStream(), I can add the same conditional logic here but I am not sure this is 100% needed in the zombie case. Alternatively, we could store the computed skip_delay_close value in stream state when it becomes a zombie and reuse it later. Let me know which approach you'd prefer.

[botengyao]

Yes, we would need to keep the logic consistent to the existing logic, and maybe refactoring it a bit. There is a long history for the deferClose as these conditions were added.

[wdauchy]

thanks for the feedback. I updated the PR about it

[wdauchy]

/retest transients

[botengyao]

/wait

[botengyao]

Do you mind adding a runtime guard and a release note? I think this will be a high risk change since the release will be next week, does merging it after early next week work for you?