feat: cache raw/diff responses with 30s TTL#13
Merged
Conversation
Add a separate raw_store (String-keyed) alongside the JSON cache. Raw responses are cached with key = path + Accept header, 30s TTL. This means burst reads (multiple bots reviewing the same PR diff) hit cache instead of consuming rate limit on every request. Closes #12
…ection, byte cap, configurable TTL, tests Fixes raised in self-review (5 perspectives): Security (🔴): - Cache key now includes identity.id so a response fetched under one PAT's access scope is never served to a request that would resolve to a different identity (cross-identity leakage fix) - Identity is selected BEFORE the cache check, closing the gap where cache hits bypassed identity resolution entirely Correctness (🟡): - Cache key now includes sorted query params (was previously dropped, causing collisions on paginated raw requests) - Accept header normalized to lowercase before keying (avoids duplicate entries for equivalent headers) - Stampede protection via moka's try_get_with — concurrent misses on the same key now coalesce into a single upstream request instead of N simultaneous GitHub calls Maintainability (🔴/🟡): - raw_ttl_secs, raw_max_entries, raw_max_bytes added to CacheConfig (was hardcoded, inconsistent with existing TTL config pattern) - Added byte-size weigher (raw_max_bytes, default 256MiB) to bound memory for large diffs — entry count alone was an insufficient cap - Removed duplicated get/insert pair in favor of a single get_or_insert_raw combinator - Added 8 new unit tests for cache key construction and behavior (previously zero coverage for the new code) Scope note (per spec-verification review): This does NOT implement SHA-aware invalidation as originally requested in #12. It's a bounded, safer version of the flat-TTL approach — documented as such rather than claiming full parity with octopool's design.
- Replace try_get_with with entry_by_ref().or_try_insert_with() to get is_fresh() — correctly distinguishes cache hits from first-insert misses (previously every Ok was counted as a hit, inflating stats) - Include key.len() in weigher so byte budget accounts for key memory - Update tests to verify correct hit/miss accounting
This comment has been minimized.
This comment has been minimized.
chaodu-agent
commented
Jul 8, 2026
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Remove raw_max_entries field from CacheConfig — it was never enforced
(raw_store uses raw_max_bytes via weigher only). Removes misleading
config that gave operators false confidence in entry-count limiting.
- Replace hardcoded User-Agent 'ghpool/0.1.0' with
concat!("ghpool/", env!("CARGO_PKG_VERSION")) across all 4 call sites.
Ensures GitHub API logs reflect the actual deployed version.
- Clean up stale comments and config.example.toml references.
Contributor
Author
Consolidated Review — PR #13 (
|
| # | Finding | Status |
|---|---|---|
| 1 | raw_max_entries dead config — misleading to operators |
✅ Fixed — field removed from struct, Default impl, config.example.toml, and all comments |
| 2 | User-Agent hardcoded as ghpool/0.1.0 |
✅ Fixed — replaced with concat!("ghpool/", env!("CARGO_PKG_VERSION")) at all 4 call sites |
Non-blocking Follow-ups (documented for future)
| # | Finding | Priority |
|---|---|---|
| 1 | Cache key separator : theoretically ambiguous (low practical risk with controlled inputs) — use \x1f or || |
Low |
| 2 | No per-store metrics in /stats — cannot distinguish raw vs JSON cache performance |
Medium |
| 3 | Log attribution on cache hits shows selected identity, not fetching identity | Low |
| 4 | String-encoded error type — typed enum would be compile-time safe | Low |
| 5 | select() inflates request_count on cache hits |
Low |
| 6 | No negative caching for 4xx responses | Medium |
| 7 | Missing tests: capacity eviction and error-not-cached | Medium |
What's Good (🟢)
- ✅ Cross-identity cache isolation correctly enforced (identity selected before lookup, key includes
identity.id) - ✅ Byte-bounded weigher prevents large diffs from exhausting memory
- ✅ Stampede protection via
entry_by_ref().or_try_insert_with()correctly implemented - ✅
is_fresh()hit/miss accounting — correct semantics - ✅ Error handling preserves upstream status without information leakage
- ✅ Rate limit tracking correctly skipped on cache hits
- ✅
env!("CARGO_PKG_VERSION")for User-Agent — compile-time, zero-cost, always in sync - ✅ Clean config surface —
raw_max_bytesis the sole raw cache capacity control - ✅ 8 unit tests covering key construction, roundtrip, stampede dedup, identity isolation
- ✅ CI passing
Reviewed across Architecture, Correctness, Testing, Docs/UX, Security/CI, Operability, and General perspectives. Unanimous LGTM after fixes.
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
2 participants
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
Add caching for raw (non-JSON) responses like PR diffs. Previously every
gh pr diffcall hit GitHub API directly.Scope note: this implements a bounded flat-TTL cache, not the SHA-aware invalidation described in #12. It's a safer stopgap — see "Not done" below for the gap to full SHA-aware caching.
Implementation
raw_store(moka cache, String values) alongside the existing JSONstoreraw:{path}?{sorted_query}:{accept_header}:{identity_id}— varies on query params, Accept header, and selected identity (fixes cross-identity leakage: a response fetched under one PAT's access scope can never be served to a request resolving to a different identity)raw_ttl_secs)raw_max_bytes, default 256MiB, enforced via moka weigher) — prevents a few large monorepo diffs from exhausting memorymoka::try_get_withcoalesces concurrent cache misses on the same key into a single upstream requestAddressed from self-review (5 perspectives: correctness, security, spec-verification, maintainability, skeptical)
identity.id; identity selected before cache lookupraw_max_bytesconfig (default 256MiB)build_keytry_get_withcoalescingCacheConfig(raw_ttl_secs,raw_max_entries,raw_max_bytes)Not done (deliberately out of scope)
head.sha, auto-invalidated on push. This PR's 30s TTL means a diff fetched within 30s of a push could be stale. Tracked as a possible follow-up, not blocking this PR.If-None-Match): would let GitHub confirm freshness without consuming rate limit on a 304. Also a possible follow-up.Before/After