refactor(responsecache): transport-free cache core, drop httptest from production#474
Conversation
…m production The cache decision logic (skip checks, hit lookup, replay, miss capture) was written against *echo.Context, so the internal guardrail LLM path had to synthesize a fake HTTP request and recorder via net/http/httptest to pass through it. Introduce an exchange seam with two implementations: echoExchange (the HTTP adapter, sole owner of framework types) and internalExchange (a buffered transport-free call). HandleRequest and HandleInternalRequest are now thin adapters over one shared handle() core. - httptest and the synthetic echo.Echo instance are gone from production code; HandleInternalRequest's callback returns a buffered InternalResponse instead of writing through a fake echo context. - Audit/usage hit side effects move behind the seam: audit enrichment stays on the HTTP path (it was already a no-op on the synthetic internal context) and the usage hit recorder is transport-free. - Cache behavior is unchanged: same keys, same skip headers, same capture rules (200-only, JSON/SSE validation, failover responses never stored). - New coverage: internal exact miss->hit round trip incl. failover-not-stored, and zero-value middleware acting as a no-op cache. - Remove dead exported IoReadAllBody (no callers). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a transport-agnostic ChangesExchange-based response cache refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Executor as internal_chat_completion_executor
participant Middleware as ResponseCacheMiddleware
participant Exchange as internalExchange
participant Cache as simple/semanticCacheMiddleware
Executor->>Middleware: HandleInternalRequest(ctx, method, path, body, next)
Middleware->>Exchange: newInternalExchange(ctx, method, path, next)
Middleware->>Cache: handle(exchange, body, next)
Cache->>Exchange: RequestHeader / Path / Method
alt cache hit
Cache->>Exchange: ReplayHit(cachedBytes)
else cache miss
Exchange->>Executor: runNext(ctx)
Executor-->>Exchange: InternalResponse
Cache->>Exchange: Capture(response)
end
Middleware-->>Executor: InternalHandleResult
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/responsecache/semantic.go (1)
644-659: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate Cache-Control parsing to
shouldSkipCacheControlto remove duplication.
shouldSkipAllCacheHeadersreimplements the exactCache-Controldirective loop already provided byshouldSkipCacheControl(simple.go, lines 237-249). Beyond the DRY smell, the two copies have drifted: this one usesstrings.SplitwhileshouldSkipCacheControlusesstrings.SplitSeq. Collapsing them keeps the skip semantics in one place.As per coding guidelines: "Keep files small and follow KISS" and "make the smallest change... avoid... duplication."
♻️ Proposed refactor
func shouldSkipAllCacheHeaders(header func(string) string) bool { if strings.EqualFold(header("X-Cache-Control"), "no-store") { return true } - cc := header("Cache-Control") - if cc == "" { - return false - } - directives := strings.Split(strings.ToLower(cc), ",") - for _, d := range directives { - d = strings.TrimSpace(d) - if d == "no-cache" || d == "no-store" { - return true - } - } - return false + return shouldSkipCacheControl(header("Cache-Control")) }🤖 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 `@internal/responsecache/semantic.go` around lines 644 - 659, `shouldSkipAllCacheHeaders` is duplicating the `Cache-Control` directive parsing already handled by `shouldSkipCacheControl`; update this helper to delegate the `Cache-Control` check to `shouldSkipCacheControl` instead of reimplementing the loop. Keep the existing `X-Cache-Control` handling in `shouldSkipAllCacheHeaders`, and use the shared helper in `semantic.go`/`simple.go` so the skip logic stays in one place and remains consistent.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@internal/responsecache/semantic.go`:
- Around line 644-659: `shouldSkipAllCacheHeaders` is duplicating the
`Cache-Control` directive parsing already handled by `shouldSkipCacheControl`;
update this helper to delegate the `Cache-Control` check to
`shouldSkipCacheControl` instead of reimplementing the loop. Keep the existing
`X-Cache-Control` handling in `shouldSkipAllCacheHeaders`, and use the shared
helper in `semantic.go`/`simple.go` so the skip logic stays in one place and
remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f172ec03-d712-451d-8d2f-bede8c7c3b4f
📒 Files selected for processing (8)
internal/responsecache/exchange.gointernal/responsecache/handle_request_test.gointernal/responsecache/responsecache.gointernal/responsecache/semantic.gointernal/responsecache/semantic_test.gointernal/responsecache/simple.gointernal/responsecache/usage_hit.gointernal/server/internal_chat_completion_executor.go
… helper shouldSkipAllCacheHeaders reimplemented the no-cache/no-store directive loop that shouldSkipCacheControl already owns; delegate to it so the skip logic lives in one place. X-Cache-Control handling is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Third PR from the architecture review (doc in #472; follows #473). The response cache's decision logic was modeled as HTTP middleware, so the internal guardrail LLM path had to fabricate a fake HTTP request/recorder with
net/http/httptestin production code to pass through it.This introduces an
exchangeseam — request info, hit replay, miss capture, hit side effects — with two implementations:echoExchange: the HTTP adapter; now the only place cache logic touches the web framework (replay/capture keep their existing echo implementations, including SSE streaming replay).internalExchange: a buffered transport-free call for the guardrail executor; request headers still come from the originating request's snapshot allowlist.HandleRequest(HTTP) andHandleInternalRequest(internal) are thin adapters over one sharedhandle()core, so the two paths can no longer drift.What's gone
httptestand the syntheticecho.Echoinstance in production code.IoReadAllBody(zero callers).m.echo == nilfailure mode: a zero-value middleware is now a working no-op cache (pinned by a test).Behavior
Unchanged: same cache keys, same skip-header semantics, same capture rules (200-only, JSON/SSE validation, failover responses never stored), same usage/audit hit recording. Audit enrichment stays HTTP-only — on the old synthetic internal context it was already a silent no-op since the echo store was empty.
New coverage: internal exact miss→hit round trip (incl. failover-responses-never-stored) and the zero-value no-op case.
Follow-up (intentionally not here)
Deleting the legacy
ResponseCacheMiddleware.Middleware()path (possible-refactoring #4) — it now shims through the same exchange seam, but its removal requires migrating ~15 middleware-driven tests per that doc's guidance, so it stays a separate focused change.Testing
Full
go build ./...andgo test ./...pass;go test -raceon responsecache + server;golangci-lint0 issues; pre-commitmake test-raceon the commit.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes