versionedcache: a cache that reloads when a Redis version key moves - #253
Conversation
1883007 to
b8242a5
Compare
lei-wego
left a comment
There was a problem hiding this comment.
Four blocking items, all inline. The one that decides timing rather than content: New has no error return and the six RefreshOutcome strings are already Datadog tag values in flight-cerberus, so the input-validation and enum decisions freeze the moment ./auto_version cuts versionedcache/v0.1.0.
🤖 AI-assisted review via Claude Code
yanyi-wego
left a comment
There was a problem hiding this comment.
Two blockers are inline.
🤖 AI-assisted review via Claude Code
lei-wego
left a comment
There was a problem hiding this comment.
One new blocker plus two non-blocking notes, all inline on cache.go. The four from the last round check out at ad81979: the two recovers, the New guards, the published flag, and the internal test that kills the c.newest mutant 5/5.
🤖 AI-assisted review via Claude Code
yanyi-wego
left a comment
There was a problem hiding this comment.
Blocking: the public commit history includes private ticket identifiers, service names, and session links. Please squash or rewrite the branch to remove them, invalidate or restrict any exposed session links that grant access, and force-push the sanitized history before merge.
🤖 AI-assisted review via Claude Code
Holds one dataset in memory and reloads it only when a short version string published beside that dataset changes. A cycle that finds the version unchanged costs one read of that string instead of reading the whole dataset. - Cache[T] is built on otter v2, which holds the value, times the interval and runs one refresh at a time. The version comparison and what each cycle reports are this package's. - ReadVersion says where the version lives, with VersionInKey and VersionInHashField for the two Redis shapes. A nil one means the dataset carries no version, so the data reloads every interval. - Refreshes run in the background, so a warm cache is answered from memory and Get returns an error only when nothing has ever loaded. - Options.MaxStaleness reads the data again once a version has stopped moving for some reason other than the data being unchanged. Six hours by default. - Six outcomes reported through Options.WhenRefreshed, so a caller can tell an unchanged version from a reload, a staleness read and a failure. - New panics on settings that build a cache which then never refreshes properly, since it has no error to return. - A panicking loader, version read or reporter becomes a failed cycle rather than ending the process, and a source that is failing is read once an interval however much traffic keeps arriving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ee1b62d to
9537199
Compare
lei-wego
left a comment
There was a problem hiding this comment.
report, the ErrNotFound flatten and the godoc all check out at 9537199, and the new interval claim comes with a test that fails without it. One non-blocking README row left, inline.
🤖 AI-assisted review via Claude Code
Co-authored-by: lei-wego <84778813+lei-wego@users.noreply.github.com>
lei-wego
left a comment
There was a problem hiding this comment.
LGTM
🤖 AI-assisted review via Claude Code
yanyi-wego
left a comment
There was a problem hiding this comment.
One blocker, inline on versionedcache/cache.go.
🤖 AI-assisted review via Claude Code
…backwards The interval claim that came with the refresh gate read wall time through UnixNano, which a clock correction moves backwards. The store's own clock counts from a monotonic reading and goes on asking for cycles regardless, so a correction would have held every cycle back until wall time caught up. - keep the last cycle's start as a time.Time and compare with Sub, so the gate counts elapsed time the way the store's clock does - read a negative gap as a clock that was reset, which covers a caller whose Now carries no monotonic reading of its own - add an internal test for a backward step, which fails without the guard - say in the Options.WhenRefreshed docs that calling Get from there never returns, since it runs inside the cycle - note in the README that a call arriving during a refresh waits on a goroutine - shorten the comments added over the last two rounds and drop the jargon Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lei-wego
left a comment
There was a problem hiding this comment.
Two non-blocking notes, inline on versionedcache/cache.go and its internal test.
🤖 AI-assisted review via Claude Code
A caller that read the clock before the loop and then lost the swap compared its own earlier reading against the winner's stamp. That gap comes back negative, which the guard reads as a clock that moved, so the interval was claimed a second time and the stamp went backwards. Sixty-four callers entering together reached two claims; reading the clock inside the loop puts that back to one, because a caller that retries reads a time at or after the winner's, while a clock that really moved still reads negative. Not reachable through Get today, since the store runs one reload per key at a time, but startCycle claims the property for itself and nothing here rests on the store being what holds it up. - build the backward-clock test's fixture with Round(0) so it carries no monotonic reading, which is the case the guard exists for; a fixture built by adding to time.Now keeps one and models a state the real clock cannot reach, so the test passed without exercising the guard - name that test for the clock it uses Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lei-wego
left a comment
There was a problem hiding this comment.
Both of last round's notes are closed. One non-blocking note on test coverage, inline.
🤖 AI-assisted review via Claude Code
…clock The claim loop had no test that entered it twice, which is why both of its regressions landed with the suite green: hoisting the clock read back above the loop still passes every other test in the package. This drives the lost-swap window through the injected clock, so it needs no goroutines and depends on no timing: the first read stamps a later start, as a cycle that won the swap would, and returns an earlier time to the caller that lost it. That caller has to read the clock again rather than compare its own stale sample, which would look like a clock that had moved backwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lei-wego
left a comment
There was a problem hiding this comment.
That closes the last one. Nothing further from me.
🤖 AI-assisted review via Claude Code
yanyi-wego
left a comment
There was a problem hiding this comment.
LGTM
🤖 AI-assisted review via Hermes
yanyi-wego
left a comment
There was a problem hiding this comment.
Four notes inline, none blocking the approval already on this PR; the first two are worth settling before ./auto_version cuts the tag.
🤖 AI-assisted review via Claude Code
lei-wego
left a comment
There was a problem hiding this comment.
Code side is approvable — this HEAD only merges main in, and nothing under versionedcache/ moved.
Holding at COMMENT because the required build is red at this SHA, and it is not this PR: the push-event run tests every module the merge touched, and rand/TestSelectorComprehensive missed its sampling tolerance (32.14% against 35.03% expected over 5000 calls, tolerance 2.58pp). rand's selector draws from runtime.rand through go:linkname, so none of the bumped indirect deps reach it, and rand/selector.go is unchanged since #178. Re-run the job — the pull_request build at this SHA is already green.
🤖 AI-assisted review via Claude Code
lei-wego
left a comment
There was a problem hiding this comment.
LGTM
🤖 AI-assisted review via Claude Code
- narrow the WhenRefreshed godoc: the Get deadlock is the first load only, since a later cycle is answered from memory - reject an Options.MaxStaleness shorter than checkEvery in New — every cycle lands past the limit, so the whole dataset reloads and the version decides nothing - check that limit after the zero default is filled in, so a check interval longer than six hours is caught too - drop the separate negative-MaxStaleness guard — the new check already covers it - say in the README that Get copies a map or slice header only, so callers share the cache's backing store and must treat it as read-only - add two cases to the New guard test for the short limit and the long interval - pin the six RefreshOutcome strings, which are the metric contract nothing else asserts - assert the failed version read names the key it could not read, instead of just any error Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhe5yXCw4Knkz9fEZdqDth
66301ef
lei-wego
left a comment
There was a problem hiding this comment.
LGTM, one note here and five inline. Nothing blocking.
.github/dependabot.yml lists its module directories explicitly and /versionedcache is not among the 35, so go-redis, otter and miniredis here get no weekly minor/patch PRs. /cognito is missing too, so it is a standing gap rather than something this PR introduced, but adding - "/versionedcache" after - "/to" closes it while the module is going in.
🤖 AI-assisted review via Claude Code
- say in the README and on New that loadData gets no deadline, so its timeout has to go inside it — otter strips the deadline along with the cancellation, so nothing else bounds it - fix the README's Use section, which still listed the negative-only MaxStaleness guard that 66301ef replaced - drop "the zero value is fine" from the Options godoc — a zero MaxStaleness fails the floor once checkEvery is longer than six hours - reword the startCycle comment: the store already deduplicates a slow read, so what the guard bounds is a quick one that leaves the data due again straight away - add a failed version read to the versionPlaces table, so the hash shape's error path is covered instead of only the string key's - add a test for deleting a published empty version, the off switch the README advertises - report an outcome in the first-load failure test, so FirstLoadFailed is no longer unasserted on the data path - add a test that a background reload sees neither the caller's cancellation nor a deadline - fail fast when dialling a closed miniredis instead of retrying for 1.7s — suite goes from 10.5s to 5.1s under -race Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhe5yXCw4Knkz9fEZdqDth
lei-wego
left a comment
There was a problem hiding this comment.
Three notes inline on cache.go, none blocking. The first is worth settling before ./auto_version cuts versionedcache/v0.1.0, since the godoc is what callers get.
Separately, 66301ef and b2280d7 reintroduce the Claude-Session: trailer that the 2026-09-18 rewrite took out of this branch. main carries the pattern already, so this is only about the history that had already been cleaned here.
🤖 AI-assisted review via Claude Code
- correct New's godoc: only a background refresh gets a stripped context, the first load runs on the calling goroutine under that caller's deadline - add a test for that first-load half, so the asymmetry the godoc promises is asserted at both ends - reword the startCycle comment again: the store does re-arm the entry when a cycle finishes, failed ones included, so what the guard bounds is the window before that lands - say on the MaxStaleness field that New panics when the value in force is shorter than checkEvery, the six-hour default included Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhe5yXCw4Knkz9fEZdqDth
lei-wego
left a comment
There was a problem hiding this comment.
LGTM, one non-blocking note.
The PR description's otter section still says the client's own timeouts bound the refresh. README.md:98-99 carries the corrected version after last round: the Redis client bounds its own reads, but nothing bounds loadData. Worth the same edit in the description, since that is the record introducing the module ./auto_version tags on merge.
🤖 AI-assisted review via Claude Code
A new module,
github.com/wego/pkg/versionedcache.Holds a dataset in memory and reloads it only when a small version published beside that dataset says the data changed. A cycle that finds the version unchanged costs one read of a short string instead of reading the whole dataset.
It is for config that is read constantly, changes rarely, and is expensive to fetch whole.
Notes for review
ReadVersionfunction, not a client plus a key.VersionInKeyandVersionInHashFieldcover the two Redis shapes; a dataset with no version passesniland reloads every interval. Only those two helpers mention Redis, so a caller could version off a DynamoDB attribute or an ETag without a third constructor.Getreturns an error only when it has never loaded. After a first success, a failed read of either the version or the data keeps the previous value and returns it with a nil error. The failure reaches the caller throughOptions.WhenRefreshed, so an outage does not empty a warm cache.string(outcome)with nothing to branch on:version_unchanged,reloaded_after_change,reloaded_without_version,reloaded_at_max_staleness,refresh_failed,first_load_failed.WhenRefreshedlets the calling service emit its own metric, which is what keeps observability libraries out of this repo.Optionsstruct rather than functional options: Go cannot infer the type parameter for a standalone generic option, soWhenRefreshed[T](...)would make every call site spell the type out.The store underneath is otter
Holding the value, timing the interval and running one refresh at a time are otter v2's. This package adds the version comparison and the outcome. An earlier revision hand-rolled all of it; the library replaced about a hundred lines of atomics and a compare-and-swap election.
Two caller-visible consequences:
Getthat started it does not cut it short — the client's own timeouts bound it.Two otter behaviours this package deliberately overrides, both worth a look:
refreshEveryputs the entry back on the clock after every cycle, failed ones included.Reloadis handed the value the cache held when that call'sGetran. For a cycle queued behind another one that value is out of date, so the cycle decides the version moved, re-reads a dataset the cycle in front just read, and writes its result over the newer one. The cycle therefore compares against the newest entry it produced. This surfaced as a flaky test, not in review; it reproduces 17 times in 40 runs under-race.A floor under how long a version is trusted
Options.MaxStaleness, six hours by default. A version that stops moving is not proof the data is unchanged — a writer can change rows and die before publishing, and a bulk import publishes once at the very end. Data kept on an unchanged version for that long is read again anyway and reportsreloaded_at_max_staleness. Only a read restarts that clock; a skipped cycle does not.Release
Not tagged yet. Once this merges,
./auto_versiononmaintagsversionedcache/v0.1.0.