feat: impl apply_on_stream_done for local rate limit by rudrakhp · Pull Request #48530 · envoyproxy/envoy

rudrakhp · 2026-10-22T06:37:36Z

Commit Message: feat: impl apply_on_stream_done for local rate limit
Additional Description: Implement apply_on_stream_done feature for local rate limit, similar to the existing feature in global rate limit. Note that unlike in global rate limit, apply_on_stream_done does't apply to route/vhost level rate limit descriptors for local rate limit filter. For such behavior typed_per_filter_config can be used instead.
Risk Level: Low (new API)
Testing: Unit and Integration testing
Docs Changes: Yes
Release Notes: Yes
Platform Specific Features: N/A
Fixes #37790

repokitteh-read-only · 2026-10-22T06:37:48Z

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: #48530 was opened by rudrakhp.

see: more, trace.

Copilot

🟡 Changes recommended

Concurrent partial consumption can incorrectly restore tokens in shared buckets, weakening rate-limit enforcement.

1 open issue

Clamp partial token consumption to prevent bucket refunds · New

What changed in this PR

Adds apply_on_stream_done support to HTTP local rate limiting, addressing #37790 by accounting for resource usage available only after a response completes.

Changes:

Updates matching descriptor buckets at stream completion, including usage charges and refunds.
Adds unit and integration coverage for completion accounting and subsequent request enforcement.
Documents the behavior in API comments, configuration guidance, and release notes.

File	Description
`test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc`	Tests completed usage limiting subsequent requests.
`test/extensions/filters/http/local_ratelimit/filter_test.cc`	Tests completion accounting across filter configurations.
`test/extensions/filters/http/local_ratelimit/BUILD`	Adds the simulated-time test dependency.
`test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc`	Tests charges, refunds, wildcard descriptors, and sharing.
`test/extensions/filters/common/local_ratelimit/BUILD`	Adds the simulated-time test dependency.
`source/extensions/filters/http/local_ratelimit/local_ratelimit.h`	Declares completion handling and retained stream context.
`source/extensions/filters/http/local_ratelimit/local_ratelimit.cc`	Applies completion actions using response and stream information.
`source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h`	Declares descriptor accounting and partial consumption.
`source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc`	Implements completed-usage bucket updates.
`docs/root/configuration/http/http_filters/local_rate_limit_filter.rst`	Explains completion accounting with a configuration example.
`changelogs/current/new_features/local_ratelimit__apply-on-stream-done.rst`	Announces local completion-accounting support.
`api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto`	Documents completion actions in local filter configuration.
`api/envoy/config/route/v3/route_components.proto`	Documents local support for `apply_on_stream_done`.

_{Review effort: Balanced}

repokitteh-read-only · 2026-10-22T07:24:17Z

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

🐱

Caused by: #48530 was synchronize by rudrakhp.

see: more, trace.

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

rudrakhp · 2026-10-22T09:22:47Z

/assign @wbpcode

wbpcode

Thanks for this contribution. One high level comment to start it.

/wait-any

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

…eam-done Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> # Conflicts: # source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc # test/extensions/filters/http/local_ratelimit/filter_test.cc

wbpcode

Thanks. The API LGTM now. Thanks for the contribution. And some more comments are added.

/wait

wbpcode · 2026-11-03T13:56:06Z

+  auto cb = [tokens, threshold, allow_partial](double total) {
+    if (allow_partial) {
+      return std::max(0.0, std::min(total, tokens));
+    }


see, we actually could consumer future tokens. So, rather than allow_partial, should we support a overdraft here?

Wanted to keep budget decisions within rate limit window, but makes sense if we can allow overdraft till max_tokens, basically one extra bucket.

wbpcode · 2026-11-03T14:04:15Z

+      if (used_config_->hasStreamDoneActions()) {
+        // Keep the request headers and the route that owns per-route config for onDestroy().
+        request_headers_ = &headers;
+        route_ = decoder_callbacks_->routeSharedPtr();


I think it's unnecessary to store the request_headers_ and route_ now?

wbpcode · 2026-11-03T14:07:45Z

+  // Stream-owned headers outlive onDestroy(), as in the global rate limit filter.
+  response_headers_ = &headers;


we also needn't this? because we always could access the request headers and response headers from the stream callbacks if necessary in the onDestory(). And it's unsafe to keep reference of response headers.

Agree, done.

wbpcode · 2026-11-03T14:13:05Z

+  // Only set when the used config's embedded rate limits have apply_on_stream_done entries.
+  if (request_headers_ == nullptr) {
+    return;
+  }
+  std::vector<RateLimit::Descriptor> descriptors;
+  used_config_->populateDescriptors(*request_headers_, response_headers_,
+                                    decoder_callbacks_->streamInfo(), descriptors,
+                                    /*on_stream_done=*/true);


we also need to respect the enabled decision here:

we could add a bool reatelimit_eanbled_{false}; to indicate it, then:

# At the decodeHeaders(): ratelimit_enabled_ = used_config_->enabled(); if (!ratelimit_enabled_) { return Http::FilterHeadersStatus::Continue; } # At the onDestory() if (!ratelimit_enabled_ || !used_config_->hasStreamDoneActions()) { return; } ..... ``

This was already handled, request_headers_ was only stored after the enabled() check in decodeHeaders(), so onDestroy() returned early for disabled streams (covered by test DisabledOrNotDecodedDoesNotAccount).
Now that #48530 (comment) is addressed, it needs to be explicit like you said, done.

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

rudrakhp requested review from mattklein123 and wbpcode as code owners October 22, 2026 06:37

repokitteh-read-only Bot added the api label Oct 22, 2026

github-actions Bot requested a balanced review from Copilot October 22, 2026 06:37

repokitteh-read-only Bot assigned markdroth Oct 22, 2026

Copilot started reviewing on behalf of rudrakhp October 22, 2026 06:38

View session

Copilot AI reviewed Oct 22, 2026

View reviewed changes

Comment thread source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc Resolved

rudrakhp force-pushed the local-ratelimit-stream-done branch from a636a95 to c38a5ae Compare October 22, 2026 07:24

rudrakhp force-pushed the local-ratelimit-stream-done branch 2 times, most recently from 462f9d9 to e0a0404 Compare October 22, 2026 08:11

feat: impl apply_on_stream_done for local rate limit

87d1e5e

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

rudrakhp force-pushed the local-ratelimit-stream-done branch from e0a0404 to 87d1e5e Compare October 22, 2026 08:26

repokitteh-read-only Bot assigned wbpcode and unassigned markdroth Oct 22, 2026

wbpcode reviewed Oct 25, 2026

View reviewed changes

Comment thread source/common/runtime/runtime_features.cc Resolved

repokitteh-read-only Bot added the waiting:any label Oct 25, 2026

remove the field from route/vhost level descriptors

3cdf5a0

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

repokitteh-read-only Bot removed the waiting:any label Oct 26, 2026

rudrakhp added 2 commits October 26, 2026 10:20

add test to show request header changes by other filters apply

3d0cbdd

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

Merge branch 'main' into local-ratelimit-stream-done

f9d2871

rudrakhp requested a review from wbpcode October 26, 2026 04:51

wbpcode reviewed Nov 3, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Nov 3, 2026

address comments

294fc89

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

repokitteh-read-only Bot removed the waiting label Nov 4, 2026

remove redundant early return

0a158de

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

Merge branch 'main' into local-ratelimit-stream-done

27297e4

rudrakhp requested a review from wbpcode November 4, 2026 18:22

		// Stream-owned headers outlive onDestroy(), as in the global rate limit filter.
		response_headers_ = &headers;

Conversation

rudrakhp commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Oct 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Oct 22, 2026

Uh oh!

rudrakhp commented Oct 22, 2026

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wbpcode 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!

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!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rudrakhp commented Oct 22, 2026 •

edited

Loading