[codex] expand hit cache and request controls - #12
Merged
Conversation
📝 WalkthroughWalkthroughAdds freshness-aware ChangesMapCache Peek Addition
net/hit request caching and control flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Request
participant Cache
participant Flights as singleflight.Group
participant Server as HTTP server
Caller->>Request: Do(ctx, out)
Request->>Request: build request and cache key
Request->>Cache: Get / GetSWR
alt cache hit
Cache-->>Request: bytes
Request->>Request: decode(bytes, out)
else cache miss
Request->>Flights: DoChan(key, fetch)
Flights->>Request: loadBytes / executeBytesWithRetry
Request->>Server: HTTP request
Server-->>Request: response bytes
Request->>Cache: Set(key, bytes)
Request->>Request: decode(bytes, out)
end
Request-->>Caller: result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Peyton-Spencer
marked this pull request as ready for review
July 1, 2026 04:22
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
net/hit/cache_behavior_test.go (1)
1-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTest may not catch duplicated body-hash key segment.
Per the
cacheKeyValuecontract (net/hit/hit.go:529-547), a hashed"body"key part is auto-appended for non-GET requests with a body, independent of caller-suppliedKeyParts. This test issues a POST with a body and also requestshit.BodyHash()explicitly, so the resulting key likely contains two"|body=sha256:..."segments. The assertion only checksstrings.Contains, so it wouldn't catch this duplication.Consider asserting the exact count of
"|body=sha256:"occurrences (e.g., viastrings.Count) to guard against this, or clarify whether auto-injection should be suppressed when the caller already addedBodyHash().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/hit/cache_behavior_test.go` around lines 1 - 104, The cache key test does not detect duplicate auto-appended body hashes, so it can pass even if cacheKeyValue adds two body segments for POST requests with BodyHash(). Update TestCacheKeyIncludesPathQueryAndConfiguredDimensions to assert the exact count of "|body=sha256:" in the generated key, using the cacheKeyValue behavior as the reference. Use the existing hit.BodyHash() and Cacheable/Key chain in the test to verify whether the auto-injected body hash is duplicated or should be suppressed when already requested by the caller.net/hit/cache.go (1)
111-116: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrefix concatenation without a delimiter risks key collisions.
prefix + keywith no separator meansPrefix("ab")+ key"c"collides withPrefix("a")+ key"bc". Callers currently avoid this by including their own separator (e.g."hit:"in tests), but nothing enforces it.Also applies to: 188-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/hit/cache.go` around lines 111 - 116, The MapCache key construction in key() is vulnerable to collisions because it concatenates c.prefix and the cache key without a separator. Update the key-building logic used by MapCache.key() (and the other matching occurrence mentioned in the review) to insert a consistent delimiter between prefix and key, or otherwise normalize prefixes so callers cannot accidentally create overlapping keys. Keep the change localized to the shared key-generation path so all cache operations use the same collision-safe format.net/hit/example_test.go (1)
14-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCache example doesn't demonstrate an actual cache hit.
The example wires up
hit.Layeredand callsDoonce, but never proves caching is effective (e.g., asserting the handler is invoked only once across twoDocalls). As written it only shows API usage, not behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/hit/example_test.go` around lines 14 - 32, The cache example in ExampleRequest_Cache only shows a single GET call, so it never proves that hit.Layered actually caches results. Update the example to use the existing ExampleRequest_Cache flow with a request counter or similar server-side state, call hit.GET[testItem](server.URL).Cache(cache).Key(hit.Field("provider", "example")).Do(...) twice, and show that the handler is invoked once while both results are returned from the same cached response.net/hit/README.md (2)
1-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting retry / stale-while-revalidate surfaces.
The PR objectives note that existing retry and stale-while-revalidate behavior is retained, but this README only covers caching, keys, and concurrency controls. A short section would round out the docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/hit/README.md` around lines 1 - 116, Add a short documentation section in hit/README.md for retry and stale-while-revalidate behavior, since the current overview only covers request building, cache keys, cache backends, and concurrency limits. Reference the existing hit package APIs that already expose this behavior, and briefly explain how callers should use them without changing code; keep the new section near the Cache/Concurrency docs so readers can find the retention behavior alongside Cache, Layered, Rate, and Gate.
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant "hit:" prefix in Redis example.
baseCacheKeyalready hardcodes a"hit:"literal prefix on every logical cache key (net/hit/key.go:76-95), sohit.Prefix("hit:")in this example would double up ashit:hit:...in the physical Redis key. Consider using a distinct example prefix (e.g."myapp:") to avoid implying redundancy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/hit/README.md` around lines 68 - 72, The Redis cache example in README.md uses hit.Prefix("hit:"), which duplicates the built-in "hit:" prefix already applied by baseCacheKey in hit.Redis/key handling. Update the example to use a distinct application-specific prefix in the Redis snippet so the sample does not imply double-prefixing and still points readers to hit.Redis and hit.Prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@net/hit/cache.go`:
- Around line 235-253: The LayeredCache.Get method currently returns immediately
when any cache layer errors, which prevents fallback to colder caches; update
the loop in LayeredCache.Get so a Get error on one layer is treated as a miss
and the code continues to the next cache instead of short-circuiting, while
still preserving the existing promotion behavior and returned values when a
later layer succeeds.
In `@net/hit/hit.go`:
- Around line 93-96: The Request.Method setter currently changes the verb
without updating the default cacheable/coalesce state inherited from GET, so
Method(http.MethodPost) can still behave like a cached/idempotent request.
Update Request.Method to recompute the defaults for the new method in the
Request[Out] flow, or preserve explicit Cacheable overrides while resetting
cacheable and coalesce appropriately when the method changes. Use the
Request.Method, GET constructor, and any Cacheable/coalesce handling in Request
to keep the behavior consistent.
- Around line 632-639: The caching flow in Request.fetchAndCache currently
stores raw response bytes before decode has validated them, which can poison the
cache with malformed payloads. Update fetchAndCache and the related decode path
around Request.decode so bytes are only written to cache after a successful
decode/parse of the response. If decode falls back or returns an error, skip
caching that payload and keep the existing cache behavior only for successfully
decoded responses.
- Around line 579-586: Route SWR loader calls through loadBytes and propagate
loader errors instead of swallowing them. In Do, when cacheSWR uses
SWRCache.GetSWR, change the loader passed from r.fetchAndCache(fetchCtx, key) to
the coalescing path in r.loadBytes so stale-refresh and miss fetches share the
same request deduplication. Also make sure any error returned by GetSWR’s loader
is handled and returned from Do rather than falling through and triggering
another upstream fetch.
In `@net/hit/limit.go`:
- Around line 16-25: NewLimiter currently computes the rate with
interval/time.Duration(n), which can round down to zero and make rate.Every
return an unlimited limiter. Update NewLimiter to validate the computed interval
before calling rate.NewLimiter, and return an error (or otherwise reject/clamp)
when the derived duration is zero; keep the existing n and interval checks and
use NewLimiter/rate.NewLimiter as the main points to modify.
---
Nitpick comments:
In `@net/hit/cache_behavior_test.go`:
- Around line 1-104: The cache key test does not detect duplicate auto-appended
body hashes, so it can pass even if cacheKeyValue adds two body segments for
POST requests with BodyHash(). Update
TestCacheKeyIncludesPathQueryAndConfiguredDimensions to assert the exact count
of "|body=sha256:" in the generated key, using the cacheKeyValue behavior as the
reference. Use the existing hit.BodyHash() and Cacheable/Key chain in the test
to verify whether the auto-injected body hash is duplicated or should be
suppressed when already requested by the caller.
In `@net/hit/cache.go`:
- Around line 111-116: The MapCache key construction in key() is vulnerable to
collisions because it concatenates c.prefix and the cache key without a
separator. Update the key-building logic used by MapCache.key() (and the other
matching occurrence mentioned in the review) to insert a consistent delimiter
between prefix and key, or otherwise normalize prefixes so callers cannot
accidentally create overlapping keys. Keep the change localized to the shared
key-generation path so all cache operations use the same collision-safe format.
In `@net/hit/example_test.go`:
- Around line 14-32: The cache example in ExampleRequest_Cache only shows a
single GET call, so it never proves that hit.Layered actually caches results.
Update the example to use the existing ExampleRequest_Cache flow with a request
counter or similar server-side state, call
hit.GET[testItem](server.URL).Cache(cache).Key(hit.Field("provider",
"example")).Do(...) twice, and show that the handler is invoked once while both
results are returned from the same cached response.
In `@net/hit/README.md`:
- Around line 1-116: Add a short documentation section in hit/README.md for
retry and stale-while-revalidate behavior, since the current overview only
covers request building, cache keys, cache backends, and concurrency limits.
Reference the existing hit package APIs that already expose this behavior, and
briefly explain how callers should use them without changing code; keep the new
section near the Cache/Concurrency docs so readers can find the retention
behavior alongside Cache, Layered, Rate, and Gate.
- Around line 68-72: The Redis cache example in README.md uses
hit.Prefix("hit:"), which duplicates the built-in "hit:" prefix already applied
by baseCacheKey in hit.Redis/key handling. Update the example to use a distinct
application-specific prefix in the Redis snippet so the sample does not imply
double-prefixing and still points readers to hit.Redis and hit.Prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03a3e7bf-ac2f-40f0-bd2f-98213a785573
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumnet/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
mapcache/mapcache.gonet/go.modnet/hit/README.mdnet/hit/cache.gonet/hit/cache_behavior_test.gonet/hit/example_test.gonet/hit/hit.gonet/hit/hit_test.gonet/hit/key.gonet/hit/limit.go
Peyton-Spencer
force-pushed
the
codex/hit-cache-layers
branch
from
July 4, 2026 13:33
f69dc50 to
8386902
Compare
Peyton-Spencer
force-pushed
the
codex/hit-cache-layers
branch
from
July 4, 2026 14:53
8386902 to
c5d59f1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
net/hit/cache_behavior_test.go (2)
211-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTight TTL margin risks CI flakiness.
A 20ms hot-layer TTL with a 40ms sleep gives only a 2x margin before asserting expiration. Under loaded CI runners, scheduling jitter can plausibly exceed 20ms, causing an intermittent false failure on the "hot layer before backfill" assertion (Line 227-229). Other timing-based tests in this file use larger 50-60ms windows, which are comparatively safer.
Consider widening the TTL/sleep gap (e.g., 50ms TTL / 150ms sleep) to reduce flakiness risk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/hit/cache_behavior_test.go` around lines 211 - 240, The timing in TestLayeredCacheBackfillsHotLayer is too tight and can flake under CI jitter. Widen the gap between the hot cache TTL and the sleep before the first hot-layer Get assertion by increasing the TTL and/or sleep duration, keeping the same expiration/backfill behavior but using a much safer margin. Use the existing TestLayeredCacheBackfillsHotLayer flow with hot, warm, and cache to update the test values consistently.
95-99: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the explicit
BodyHash()here.cacheKeyValuealready hashes the body for non-GET requests with a payload, so this test repeats the same key dimension and doesn’t add coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/hit/cache_behavior_test.go` around lines 95 - 99, The cache key test is redundantly including BodyHash(), since cacheKeyValue already incorporates the request body for non-GET payloads. Update the test around Key() in cache_behavior_test to remove the explicit BodyHash() entry and keep only the dimensions that are not already implied by cacheKeyValue, so the test continues to validate the intended cache behavior without duplicating the body hash key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@net/hit/cache_behavior_test.go`:
- Around line 211-240: The timing in TestLayeredCacheBackfillsHotLayer is too
tight and can flake under CI jitter. Widen the gap between the hot cache TTL and
the sleep before the first hot-layer Get assertion by increasing the TTL and/or
sleep duration, keeping the same expiration/backfill behavior but using a much
safer margin. Use the existing TestLayeredCacheBackfillsHotLayer flow with hot,
warm, and cache to update the test values consistently.
- Around line 95-99: The cache key test is redundantly including BodyHash(),
since cacheKeyValue already incorporates the request body for non-GET payloads.
Update the test around Key() in cache_behavior_test to remove the explicit
BodyHash() entry and keep only the dimensions that are not already implied by
cacheKeyValue, so the test continues to validate the intended cache behavior
without duplicating the body hash key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eee0cf76-b21a-4e8e-9c91-65299dafbd19
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumnet/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
mapcache/mapcache.gonet/go.modnet/hit/README.mdnet/hit/cache.gonet/hit/cache_behavior_test.gonet/hit/example_test.gonet/hit/hit.gonet/hit/hit_test.gonet/hit/key.gonet/hit/limit.go
✅ Files skipped from review due to trivial changes (1)
- net/hit/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- net/go.mod
- mapcache/mapcache.go
- net/hit/example_test.go
- net/hit/hit_test.go
- net/hit/cache.go
- net/hit/limit.go
- net/hit/key.go
- net/hit/hit.go
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
1 participant
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.
Summary
hitcaching around a small key/bytesCacheinterface.Notes
This is intended as the follow-up implementation for PR #11:
#11
It is built on PR #11's current head. GitHub does not currently expose
feat/hit-packageas a branch ref for use as a PR base, so this draft PR targets
main; it should bereviewed/merged after PR #11, at which point the diff narrows to this follow-up.
Validation
go test ./...fromnet/go test ./...frommapcache/./scripts/test-all.shSummary by CodeRabbit
PeekandSetto the cache, including TTL-aware, read-only inspection of fresh entries.