Repository navigation
Add stale reads, entry bounds and per-entry TTL - #2
Conversation
Adds the pieces a caller needs to prefer availability over freshness when its data source goes away, and to run the cache against an open-ended key space. GetEntry reports where a value came from and, with WithMaxStaleAge, returns an expired value when the UpdateFunc fails. Get previously dropped the cached value on the floor in that case and returned the zero value with the error, so a temporary outage of the source became an outage for the caller. Get keeps that behaviour and never returns a stale value, so the option cannot affect existing callers. WithMaxEntries bounds the cache and evicts the least recently stored entry. Without it the cache grows with the key space, which is fine for a closed set of keys and fatal for a hash of arbitrary requests. EntryUpdate.TTL lets the UpdateFunc override the ttl for the entry it just produced, for sources that say how long their answer is valid. GetEntryWith takes the UpdateFunc per call, for callers whose key cannot carry the update's inputs, such as a hash of a request. The keyed mutexes are now reference counted and removed once released. They previously leaked one mutex per key seen, and Prune deleted them while callers could still hold them, which let a second caller create a fresh mutex for the same key and defeat the single-flight guarantee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Negative stale ages can remove fresh entries, and the public struct changes introduce unresolved source-compatibility breaks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds bounded caching, stale reads, per-entry TTLs, per-call updates, and leak-free keyed locking.
Changes:
- Adds entry bounds and stale-value support.
- Adds per-entry TTL and per-call update functions.
- Refactors keyed mutex lifecycle and expands tests/documentation.
File summaries
| File | Description |
|---|---|
cache.go |
Implements new cache behavior and eviction. |
entry.go |
Adds TTL and entry states. |
options.go |
Defines cache options. |
mutex.go |
Reference-counts keyed locks. |
README.md |
Documents new APIs. |
bounds_test.go |
Tests bounds, TTL, and per-call updates. |
stale_test.go |
Tests stale reads. |
cache_test.go |
Updates existing tests and checks lock cleanup. |
helpers_test.go |
Adds cache test helpers. |
Review details
Suppressed comments (1)
entry.go:23
- Adding
TTLto this exported struct makes every existing positionalEntryUpdate[T]{value, err}literal fail to compile with “too few values.” If source compatibility is required, this needs a non-struct-field extension mechanism; otherwise the release should be explicitly versioned and documented as breaking.
// TTL overrides the cache's default ttl for this entry only. Zero means the default is used.
//
// This lets the source of the data decide how long its answer stays valid, which is useful when
// that varies per key, for example when an upstream tells the caller how long it is caching a
// response itself.
TTL time.Duration
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collapse the lookup API to Get and GetWith, both returning the EntryState instead of a hit bool. Stale reads now flow through Get whenever WithMaxStaleAge is set, so there is no separate GetEntry. Clamp negative WithMaxStaleAge and WithMaxEntries to zero so a negative stale age can no longer prune fresh entries. Correct the single-flight wording to say that concurrent calls are serialized and a failed update is retried by the next waiter, and update the type doc for the new methods. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Zero stale age permits a stale read at the exact expiry boundary, and the implemented public API contradicts the PR description.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| func (c *UpdateCache[T]) Get(ctx context.Context, key string) (res T, state EntryState, err error) { | ||
| return c.get(ctx, key, c.updateFunc) |
| // isWithinStaleWindow reports whether an expired entry may still be served. With no stale age | ||
| // configured this is simply "not expired". | ||
| func (c *UpdateCache[T]) isWithinStaleWindow(entry *Entry[T]) bool { | ||
| return !c.now().After(entry.ExpiresAt.Add(c.maxStaleAge)) |
Adds the pieces a caller needs to prefer availability over freshness when its data source goes away, and to run the cache against an open-ended key space.
GetEntry reports where a value came from and, with WithMaxStaleAge, returns an expired value when the UpdateFunc fails. Get previously dropped the cached value on the floor in that case and returned the zero value with the error, so a temporary outage of the source became an outage for the caller. Get keeps that behaviour and never returns a stale value, so the option cannot affect existing callers.
WithMaxEntries bounds the cache and evicts the least recently stored entry. Without it the cache grows with the key space, which is fine for a closed set of keys and fatal for a hash of arbitrary requests.
EntryUpdate.TTL lets the UpdateFunc override the ttl for the entry it just produced, for sources that say how long their answer is valid.
GetEntryWith takes the UpdateFunc per call, for callers whose key cannot carry the update's inputs, such as a hash of a request.
The keyed mutexes are now reference counted and removed once released. They previously leaked one mutex per key seen, and Prune deleted them while callers could still hold them, which let a second caller create a fresh mutex for the same key and defeat the single-flight guarantee.