Repository navigation
fix(cache): stop serving a value after the source reports it gone - #232
Merged
Merged
Conversation
The cache keeps serving a cached value after the fetcher returns ErrDoesNotExist for it: only the short negative sentinel is written, so each time the sentinel expires the next Get is a soft miss served from the old data. These tests fail on that behavior for a single backend with and without negative caching, for multiple backends, and for the shadow client, and pin that the fetcher's error is still returned when Redis rejects the eviction.
…gone When a fetcher returned ErrDoesNotExist, the cache only wrote the short negative sentinel and left the data key with its full stale TTL. Each time the sentinel expired, the next Get was a soft miss and served the old value again, so a revoked item kept coming back until the stale TTL ran out. fill now also removes the data and fresh keys from every backend and the shadow client in one transaction per client, even when negative caching is off, and still returns the fetcher's error if that eviction fails.
There was a problem hiding this comment.
🟡 Changes recommended
Non-atomic deletion and generation increments allow late fills to restore revoked data, while permanent generation entries cause unbounded Redis storage growth.
4 open issues
What changed in this PR
This PR updates the shared cache used by API authorization to evict values when their source reports ErrDoesNotExist and prevent older fills from restoring them. It does not update the API dependency or deploy the change.
Changes:
- Delete cached data before optionally writing a negative marker.
- Add per-key generations and conditional writes for cache fills, including shadow writes.
- Add regression tests for revocation, negative-marker expiry, multiple backends, late fills, and eviction-error handling.
| File | Description |
|---|---|
cache/cache.go |
Adds eviction and generation-checked fill paths; clarifies ErrDoesNotExist behavior. |
cache/cache_test.go |
Updates command expectations and adds revocation regression tests. |
Review effort: Balanced
| if err := client.Del(ctx, keys.data, keys.fresh).Err(); err != nil { | ||
| return err | ||
| } | ||
| if err := client.Incr(ctx, keys.gen).Err(); err != nil { |
Comment on lines
+405
to
+410
| if err := client.Del(ctx, keys.data, keys.fresh).Err(); err != nil { | ||
| return err | ||
| } | ||
| if err := client.Incr(ctx, keys.gen).Err(); err != nil { | ||
| return err | ||
| } |
| errs = append(errs, c.writeIfCurrent(ctx, client, keys, observed[i], data)) | ||
| } | ||
| if c.opts.ShadowWriteClient != nil { | ||
| shadowObserved := observed[0] |
|
|
||
| func (*failDeletes) DialHook(next redis.DialHook) redis.DialHook { return next } | ||
|
|
||
| func (*failDeletes) ProcessHook(next redis.ProcessHook) redis.ProcessHook { return next } |
Member
Author
|
The four review notes are valid, and the branch now addresses them.
|
Eviction ran DEL and the negative-marker SET in one MULTI. Redis aborts the whole transaction when it rejects a queued command, such as a SET at maxmemory, so the value survived. The delete now runs first on its own. The eviction-failure hook now also rejects a DEL sent outside a pipeline, and the test requires it to fire.
lucataco
force-pushed
the
lucataco/cache-evict-missing
branch
from
November 3, 2026 15:31
91b7447 to
e4e6326
Compare
Member
Author
|
I reduced this PR to the eviction fix, plus one ordering change that deletes the value before writing the negative marker. The generation compare-and-set work, including the Copilot follow-ups, moved to the stacked draft #233. It changes every fill, so it should be reviewed and measured separately. The earlier review threads about generations apply to #233. |
meatballhat-cf
approved these changes
Nov 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
When a fetcher returned
ErrDoesNotExist, the cache wrote a negative marker and kept the old value. After the marker expired,Getserved the old value again until the stale TTL ran out.What changed
ErrDoesNotExist. The cache deletes the data and freshness keys, then writes the negative marker if negative caching is on. Before this change, it only wrote the marker.MULTIas the marker write. Redis aborts a whole transaction when it rejects a queued command, such as aSETat maxmemory, and that would have kept the value.ErrDoesNotExist. Eviction errors are recorded and logged but not returned.Blast radius
DELthenSETinstead of a singleSET, which adds one round trip. With more than one backend, eviction takes the cross-backend lock thatSetalready takes.Not in scope
Verification
go test -race -shuffle=on -count=5 ./cachepasses, andgolangci-lintv2.1.5 reports no issues.go.mod, the API stopped serving a key once the fresh window ended after the source reported it gone, and it did not serve it again.