invalidate local cache on is_negative_hits by oclaw · Pull Request #1230 · envoyproxy/ratelimit

oclaw · 2026-09-01T11:31:53Z

Problem

is_negative_hits (#1140) lets a descriptor decrement previously consumed quota, but refunds are invisible to the local over-limit cache: once a replica cached a key as over limit, a refund that freed quota in Redis kept being answered OVER_LIMIT from every replica's local cache until the window ended which degrades the feature on long windows

Suggested design

Introduce feature flag ENABLE_NEGATIVE_HITS (default off). While off, a request containing a negative-hit descriptor is rejected with UNIMPLEMENTED
Refuse to boot the one unfixable combination: negative hits + memcached + local cache (this PR does not add any support for the invalidations with memcached as a backend). Memcached refunds without a local cache enabled keep working.
Add Redis pub-sub backed invalidation routine: at-most-once guarantee, single added connection per replica + local cache gen-based guard for eliminating races between invalidation and stale insert

P.S. Agent also added some missing tests for touched components, look fine

oclaw · 2026-09-01T13:12:30Z

-// DecrementScript atomically decrements a rate limit counter, floored at 0.
-// If the key does not exist there is nothing to refund, so it returns 0 without
-// creating a phantom key.
-const DecrementScript = `


This is not really a common limiter part, moved it closer to Redis backend impl

oclaw · 2026-09-01T13:12:42Z

Reused driver.go as much as possible but this routine has quite independent connection cycle so decided not to bother too much

oclaw · 2026-09-01T13:13:05Z

 		if hitsAddends[i].IsNegative {
-			pipelineAppendDecrement(client, p, cacheKey.Key, hitsAddends[i].Value, &results[i], expirationSeconds)
+			// The subscriber listens only on the main Redis.
+			publishInvalidation := this.publishInvalidations && !onPerSecondRedis


dedicated per second redis assume high load and redis-backed refunds are barely effective here, not doing them (and the invalidator only listens main redis at the moment)

oclaw · 2026-09-06T11:33:57Z

@collin-lee as a first-time contributor I do not have an opportunity to request for the review it looks like, so pinging you here :)

rudrakhp

Thanks for the bug fix. Can we keep it focused on fixing the cache invalidation part though, any reason why we need the ENABLE_NEGATIVE_HITS flag?

oclaw · 2026-09-23T14:18:23Z

@rudrakhp The reason is that the bug is still present - I did not support the same invalidation for the memcached backend case as I am not sure how to deliver it properly. So without ENABLE_NEGATIVE_HITS and with local overlimit cache enabled memcached-backed workload will silently keep working in a broken way

rudrakhp · 2026-09-27T13:36:58Z

@oclaw ok on second thought I think preventing RLS to come up with known unsupported combinations like Memcached + local cache + enable_negative_hits combination makes sense. SGTM.

rudrakhp · 2026-10-04T12:30:18Z

+	)
+	if withLocalCache {
+		localCacheGuard = limiter.NewLocalCacheGuard(localCache)
+		publishInvalidations = s.EnableNegativeHits && localCacheGuard != nil


Shouldn't publishInvalidations be set without local cache as well? We seem to be setting it only if withLocalCache.

Yeah, the condition need to be a bit different. Gated it on the fact if the negative hits are enabled at all, not on the local cache

rudrakhp · 2026-10-04T12:38:33Z

+if ARGV[3] == '1' then
+  local limit = tonumber(ARGV[4])
+  if old > limit and new_val <= limit then
+    redis.call('PUBLISH', ARGV[5], KEYS[1])


Since we can override limits in requests, I think it is safer to refund regardless of old>limit?

Suggested change

if ARGV[3] == '1' then

local limit = tonumber(ARGV[4])

if old > limit and new_val <= limit then

redis.call('PUBLISH', ARGV[5], KEYS[1])

if ARGV[3] == '1' and new_val < old then

redis.call('PUBLISH', ARGV[5], KEYS[1])

Makes sense. Switched to publish of every decrement, should be good

rudrakhp · 2026-10-04T12:53:45Z

+// It owns one raw connection instead of reusing the driver's clients: radix
+// pub/sub wraps a single Conn (pooled clients cannot subscribe), and losing
+// it must never take the process down — the loop reconnects forever.


Can you explain a bit further? Don't think this warrants entire connection lifecycle management from scratch. I was expecting existing machinery to be good enough while we provide the invalidation logic only.

It is still better to use an independent connection management here (this pipeline does not need a pool and is intended to operate on long-living connections), that's why I've implemented this machinery. But you're right that it is possible to reuse pretty much all of the dialing stuff from the driver, so baked it into the custom dialer function returned by the driver

…cal cache on refunds Signed-off-by: Daniil Kogtev <oclaw@icloud.com>

- reuse dial from driver - simplify Lua script to publish on every decrement - publish always on negative hits enabled Signed-off-by: Daniil Kogtev <oclaw@icloud.com>

oclaw · 2026-10-22T18:20:51Z

@rudrakhp any chance you can review it again?

oclaw commented Sep 1, 2026

View reviewed changes

oclaw force-pushed the negative-hits-flag-invalidation branch 3 times, most recently from 0602802 to ef92df1 Compare September 1, 2026 14:51

rudrakhp reviewed Sep 23, 2026

View reviewed changes

oclaw force-pushed the negative-hits-flag-invalidation branch from ef92df1 to 0de2a9c Compare October 3, 2026 11:10

oclaw requested a review from rudrakhp October 3, 2026 11:11

rudrakhp reviewed Oct 4, 2026

View reviewed changes

oclaw requested a review from rudrakhp October 5, 2026 10:01

oclaw force-pushed the negative-hits-flag-invalidation branch 2 times, most recently from 2453e8c to 86ea20c Compare October 7, 2026 12:41

oclaw added 2 commits October 22, 2026 17:59

Gate negative hits behind ENABLE_NEGATIVE_HITS flag and invalidate lo…

0cb144f

…cal cache on refunds Signed-off-by: Daniil Kogtev <oclaw@icloud.com>

review fixes

07387a3

- reuse dial from driver - simplify Lua script to publish on every decrement - publish always on negative hits enabled Signed-off-by: Daniil Kogtev <oclaw@icloud.com>

oclaw force-pushed the negative-hits-flag-invalidation branch from 86ea20c to 07387a3 Compare October 22, 2026 17:04

Conversation

oclaw commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

oclaw commented Sep 6, 2026

Uh oh!

rudrakhp left a comment

Choose a reason for hiding this comment

Uh oh!

oclaw commented Sep 23, 2026

Uh oh!

rudrakhp commented Sep 27, 2026

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!

oclaw commented Oct 22, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oclaw commented Sep 1, 2026 •

edited

Loading