Skip to content

fix: clear per-key metadata (hashmapStats) on Reset - #432

Merged
janisz merged 1 commit into
allegro:mainfrom
jaideeppyne:fix/reset-clear-hashmapstats
Aug 31, 2026
Merged

janisz merged 1 commit into
allegro:mainfrom
jaideeppyne:fix/reset-clear-hashmapstats

Conversation

@jaideeppyne

Copy link
Copy Markdown
Contributor

cacheShard.reset rebuilds hashmap, entryBuffer and the entries queue but leaves hashmapStats untouched.

Everywhere else the invariant holds that hashmapStats only contains counts for currently-present entries — del and removeOldestEntry both delete(s.hashmapStats, hash) when an entry leaves the cache. Reset() removes every entry yet keeps their request counts, so:

  1. after Reset(), KeyMetadata(key).RequestCount still reports a stale, non-zero value for keys that are no longer cached, and
  2. the map grows without bound across Reset cycles when the key set churns (a leak).

This re-creates hashmapStats in reset(), mirroring initNewShard (zero capacity when StatsEnabled is false, so nothing is allocated in the disabled case).

Adds TestResetClearsKeyMetadata, which fails before this change (RequestCount == 10 after Reset) and passes after (== 0). go test ./..., -race, gofmt, and go vet all pass.

Related: #322 (Reset not clearing stats), #414 (adds a separate ResetKeyMetadata API — orthogonal to this fix).

cacheShard.reset rebuilds hashmap, entryBuffer and the entries queue but
leaves hashmapStats untouched. Everywhere else the invariant holds that
hashmapStats only contains counts for currently-present entries: del and
removeOldestEntry both delete(s.hashmapStats, hash) when an entry leaves
the cache. Reset removes every entry yet keeps their request counts, so
after Reset() KeyMetadata still reports stale, non-zero RequestCount for
keys that are no longer cached, and the map grows without bound across
Reset cycles when the key set churns.

Re-create hashmapStats in reset(), mirroring initNewShard (zero capacity
when StatsEnabled is false so nothing is allocated for the disabled case).

Adds TestResetClearsKeyMetadata, which fails before this change
(RequestCount == 10 after Reset) and passes after (== 0).

Signed-off-by: jaideeppyne <jaideeppyne1997@gmail.com>
Comment thread shard.go
@jaideeppyne

Copy link
Copy Markdown
Contributor Author

The red build here is TestCacheLen, which I don't think is related to this change.

It builds a cache with LifeWindow: time.Second and no CleanWindow, then writes 1337 keys and asserts Len() is 1337. It's also t.Parallel(), so on a loaded runner the write loop can outlast the one second life window, and the oldest entries are then expired and get evicted as later Sets grow the queue. That matches the failure, 933 instead of 1337.

This PR only touches hashmapStats in reset(), which isn't on that path. Locally the full suite passes on both main and this branch, and TestCacheLen on its own passes 12 out of 12 on each.

Happy to push an empty commit if you want CI re-run.

@jaideeppyne

Copy link
Copy Markdown
Contributor Author

The red Build (stable) here is TestCacheLen, which I think is a pre-existing timing flake rather than anything in this change. I opened #433 with a deterministic repro on master.

This PR only touches cacheShard.reset, and TestCacheLen never calls Reset. Build (oldstable) passed on the same commit.

@janisz
janisz merged commit c761d1a into allegro:main Aug 31, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants