Fetchers 2.0: Account fetchers - #4895
Conversation
| // contributes nothing to bulk cache preloading and reports no error. Accounts still | ||
| // load lazily on demand via FetchAccount. | ||
| func (fetcher *HttpFetcher) FetchAllAccounts(ctx context.Context) (map[string]json.RawMessage, []error) { | ||
| logger.Warnf("http_fetcher: bulk account preload is not supported by the by-id HTTP endpoint; accounts will load lazily on demand") |
There was a problem hiding this comment.
Will implement this in a follow up PR.
| var preload cachekit.BulkSource[string] | ||
| switch cfg.Refresh { | ||
| case "", config.RefreshTTL: | ||
| // serve-stale via ttl; nothing to preload. |
There was a problem hiding this comment.
serve-stale default is documented inconsistently and contradicts the actual behavior.
In stored_requests.go line 93-95 (the Refresh field) it is stated that serve-stale is on by default — "ttl" is described as "(serve-stale + background refresh, the default)" — and line 89-91 (the TTLSeconds field) reinforces it: "the value is still served (stale) while it refreshes, so reads never block on the backend." The same claim is repeated in cachekit.go line 85: "ttl (serve-stale + background revalidation)".
But in stored_requests.go line 100-104 (the ServeStale field) it says serve-stale is off by default — "Opt-in; defaults to off, which expires the entry and reloads it synchronously on the next read (classic TTL cache)."
The code confirms the "off" version is what actually runs: serve-stale is gated solely by f.serveStale in cachekit.go line 128-140, and cachekit.go line 86-109 never sets ServeStale for refresh: ttl — it only adjusts effectiveTTL/preload. The test TestGetExpiresAndReloadsByDefault in cachekit_test.go line 109-126 proves it: "Past TTL with serve-stale off: the read reloads synchronously."
So we need to fix this because the two field comments describe opposite defaults, and the one that operators will read to enable refresh: ttl (lines 89-95) promises a non-blocking stale read that the default does not deliver — under load, past TTL the read blocks on a synchronous backend reload, which is a surprising latency cliff. Either make RefreshTTL actually enable serve-stale (set serveStale = true for the ttl/preload cases in NewCacheKitAccountFetcher), or correct the comments at lines 89-95 and cachekit.go line 85 to state that ttl is a classic blocking-reload cache and serve-stale requires serve_stale: true.
Suggested fix (align behavior to the docs) in NewCacheKitAccountFetcher:
effectiveTTL := cfg.TTL()
serveStale := cfg.ServeStale
switch cfg.Refresh {
case "", config.RefreshTTL:
serveStale = true // ttl mode == stale-while-revalidate, per its documented contract
case config.RefreshNone:
effectiveTTL = 0
case config.RefreshPreload:
serveStale = true
// ...preload wiring...
}
// pass ServeStale: serveStale into cachekit.New(...)
There was a problem hiding this comment.
thanks for catching this, serveStale enabled by default
| // negative-cached); on any error (transient or a newly-malformed value) the last | ||
| // good value keeps being served and a backoff is recorded. | ||
| func (f *Fetcher[K, V]) revalidate(ctx context.Context, key K) { | ||
| start := f.clock.Now() |
There was a problem hiding this comment.
Background revalidation runs with context.Background() and no deadline. If source.Fetch hangs, finish is never called, the per-key inFlight slot stays claimed forever, and that key can never revalidate again (it keeps serving stale under serve-stale). We're implicitly relying on the HTTP client having its own timeout.
Suggested fix — add a bounded context (ideally operator-configurable via CacheKitConfig):
func (f *Fetcher[K, V]) revalidate(ctx context.Context, key K) {
ctx, cancel := context.WithTimeout(ctx, f.revalTimeout) // e.g. default 10s
defer cancel()
...
}
With a deadline, a wedged backend hits the timeout → finish(key, true) → the slot releases after revalidateBackoff, so the key recovers instead of being pinned.
There was a problem hiding this comment.
Thanks for catching this. Added a timeout post which the marker is cleared so that it can be retried again later
| st.inFlight = true | ||
| r.state[key] = st | ||
| return true | ||
| } |
There was a problem hiding this comment.
revalidator.state grows with one-off failures:
On failure the key is retained (revalidate.go:39-47); it's only removed on a later successful finish. A key that fails once and is never requested again lingers. Bounded for accounts, but the package targets larger key spaces (GVL/stored data). Minimal opportunistic cleanup — drop an expired-backoff entry when we next see it, and note the intent:
func (r *revalidator[K]) begin(key K) bool {
r.mu.Lock()
defer r.mu.Unlock()
st, ok := r.state[key]
if ok && !st.inFlight && !st.failedAt.IsZero() &&
!r.clock.Now().Before(st.failedAt.Add(r.backoff)) {
// Backoff elapsed; forget the stale failure record before re-claiming so
// one-off failures for keys that are never revisited don't accumulate.
delete(r.state, key)
st = revalState{}
}
if st.inFlight || (!st.failedAt.IsZero() && r.clock.Now().Before(st.failedAt.Add(r.backoff))) {
return false
}
st.inFlight = true
r.state[key] = st
return true
}
There was a problem hiding this comment.
Addressed with opportunistic pruning in begin(): before admitting a new revalidation, we remove any non-in-flight failure records whose backoff has elapsed, so one-off failed keys don’t linger indefinitely.
bsardo
changed the title
| @@ -0,0 +1,74 @@ | |||
| package cachekit | |||
There was a problem hiding this comment.
Curious why you've chosen the name cachekit. I recommend the name fetcher instead for better discoverability.
| @@ -0,0 +1,213 @@ | |||
| // Package cachekit is a small, generic read-through fetching engine shared by | |||
| // Prebid Server subsystems (accounts today; GVL / stored data / currency later). | |||
There was a problem hiding this comment.
This doesn't read cleanly. We don't typically refer to different parts of the app as subsystems - perhaps packages. This also references a current state of implementation (accounts today, .. later) which will quickly become stale.
Recommend adjusting the wording and moving this out to a readme.md file in the package instead of a comment here for easier discoverability.
| // NoCache is a pass-through cache: every Get is a miss and Save is a no-op. Paired | ||
| // with the engine's single-flight coalescing it yields "always fetch, still | ||
| // deduplicate" behaviour for direct-source / live tenants. | ||
| type NoCache[K comparable, V any] struct{} |
There was a problem hiding this comment.
Nitpick: Please name NilCache to keep with current naming structure.
There was a problem hiding this comment.
Please break out each cache (LRUCache an NilCache) into it's own file. A cache folder like we have for Fetcher 1.0 (stored_requests) could work nicely.
| } | ||
|
|
||
| // NewLRUCache builds an LRU cache holding up to maxEntries values. | ||
| func NewLRUCache[K comparable, V any](maxEntries int, clk clock.Clock) (*LRUCache[K, V], error) { |
There was a problem hiding this comment.
Can you use the timeutil.Time interface used throughout this repo instead of clock.Clock?
| group singleflight.Group | ||
| reval *revalidator[K] | ||
| revalTimeout time.Duration | ||
| } |
There was a problem hiding this comment.
This is very close to the ideal Fetcher definition I envisioned. I'm ok with two mutually exclusive sources depending on type configuration, but is there a way to combine them into a single Source which can either be preload/refresh or per item?
I think ttl belongs to the Cache and NegativeStore is either a detail of the Cache or a wrap layer around a Cache. I don't understand what serveStale is doing. Clock (please use timeutil instead) is a detail of the Cache and Refresh job on top of the BulkSource.
Consider renaming reval -> validator as reval can be confused with reveal. Why is there a revalTimeout?
What is a group?
| p.Clock = clock.New() | ||
| } | ||
| if p.Metrics == nil { | ||
| p.Metrics = noopRecorder{} |
There was a problem hiding this comment.
Why does it default to real clock but a no-op metrics? Consider making these required.
| if p.Metrics == nil { | ||
| p.Metrics = noopRecorder{} | ||
| } | ||
| if p.RevalidateTimeout <= 0 { |
There was a problem hiding this comment.
I don't understand based on the name what a RevalidateTimeout controls.
| } | ||
| f.cache.Save(key, v, f.ttl) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add context to the metrics that is operation is for the Start operation.
| for key, bytes := range raw { | ||
| v, err := f.transform(key, bytes) | ||
| if err != nil { | ||
| continue // skip malformed entries; they surface on demand |
There was a problem hiding this comment.
How do they surface? A transform failure is a big problem.
4 participants
Adding support for new fetchers 2.0 Account fetchers
Design - #4860
Validation Summary
Validation was performed in a non-production Kubernetes canary environment using both filesystem-backed and HTTP-backed account sources.
Design Reference
#4860
Fetchers 2.0 caches the final typed
*config.Accountinstead of caching raw JSON bytes. This avoids repeated account defaults merge, JSON unmarshal, DSA unpacking, derived GDPR map creation, and IP masking defaults on account cache hits.Metrics Used
prebid_server_cachekit_cache_result{result="miss",subsystem="account"}prebid_server_cachekit_cache_result{result="hit",subsystem="account"}prebid_server_cachekit_cache_result{result="negative",subsystem="account"}prebid_server_cachekit_backend_fetch{result="ok",subsystem="account"}prebid_server_cachekit_backend_fetch{result="notfound",subsystem="account"}prebid_server_cachekit_backend_fetch{result="error",subsystem="account"}prebid_server_account_cache_performance{cache_result="hit/miss"}Compatibility and Filesystem Source Tests
/cookie_syncaccounts.v2_enabled=false/cookie_synclru,refresh=ttl, negative offbackend ok +1,miss +1,hit +1serve_stale=falsenotfound +2, no negative metricrefresh=nonecache.type=none, negative offcache.type=none, negative onserve_stale=truerefresh=preloadmax_entries=1HTTP Source Tests
refresh=ttlrefresh=preload, HTTP source onlyEndpoint Coverage
/cookie_sync/cookie_sync/openrtb2/auctionseatbid/openrtb2/auctionseatbidStress and Thundering Herd Tests
MOCK_SLOW_DELTA=1, backend ok +1MOCK_ABSENT_DELTA=1, negative +997serve_stale=trueNotes
serve_stale=trueprebid_server_account_cache_performancestayed zero