refactor(responsecache): remove legacy Middleware() path#476
Conversation
Production traffic has gone exclusively through HandleRequest (translated inference service) and HandleInternalRequest (guardrail executor) since the exchange seam landed; the echo middleware wrapper was test-only. Remove ResponseCacheMiddleware.Middleware(), simpleCacheMiddleware.Middleware(), and the three helpers only that path used (requestBodyForCache, shouldSkipCacheForWorkflow, shouldSkipCache) — their production equivalents live in handle() or at the call sites, which pass the patched body explicitly and gate on workflow.CacheEnabled(). Test migration (possible-refactoring #4): middleware_test.go becomes exact_cache_test.go. Hit/miss, key separation, streaming-vs-JSON replay, Cache-Control skip, close-drains-writes, and write-concurrency tests now drive the production HandleRequest entry. Dropped with the legacy path, as their behavior no longer exists in this package: workflow-gating bypass tests (caller-gated since the HandleRequest migration), snapshot-body and body-read-error tests (callers supply the body), and the non-cacheable-path test (exact-layer path gating was legacy-only; the semantic layer still enforces cacheablePaths and callers only invoke the cache for inference endpoints). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughRemoves the legacy ChangesLegacy Middleware Removal and Test Coverage Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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.
Actionable comments posted: 1
🤖 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 `@internal/responsecache/exact_cache_test.go`:
- Around line 106-142: The response cache tests are leaking the middleware’s
background write workers because they only close the store and never shut down
the ResponseCacheMiddleware. Update TestHandleRequest_ExactCacheHit and the
other affected tests to defer mw.Close() after creating the middleware from
NewResponseCacheMiddlewareWithStore, so the worker pool and any in-flight writes
are drained before the store is closed. Keep the existing store.Close() defer,
but ensure mw.Close() runs first to terminate the cache-write goroutines
cleanly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fb4171c-9cc5-4ca5-927c-21d0f2328fbf
📒 Files selected for processing (5)
docs/dev/possible-refactoring.mdinternal/responsecache/exact_cache_test.gointernal/responsecache/middleware_test.gointernal/responsecache/responsecache.gointernal/responsecache/simple.go
💤 Files with no reviewable changes (3)
- internal/responsecache/middleware_test.go
- internal/responsecache/responsecache.go
- internal/responsecache/simple.go
| func TestHandleRequest_ExactCacheHit(t *testing.T) { | ||
| store := cache.NewMapStore() | ||
| defer store.Close() | ||
| mw := NewResponseCacheMiddlewareWithStore(store, time.Hour) | ||
| workflow := resolvedWorkflow("openai", "gpt-4") | ||
| body := []byte(`{"model":"gpt-4","messages":[{"role":"user","content":"hi"}]}`) | ||
| callCount := 0 | ||
| next := func(c *echo.Context) error { | ||
| callCount++ | ||
| return c.JSON(http.StatusOK, map[string]string{"result": "cached"}) | ||
| } | ||
|
|
||
| rec := driveHandleRequest(t, mw, workflow, body, nil, next) | ||
| if rec.Code != http.StatusOK { | ||
| t.Fatalf("first request: got status %d", rec.Code) | ||
| } | ||
| if rec.Header().Get("X-Cache") != "" { | ||
| t.Fatalf("first request should not have X-Cache: %s", rec.Header().Get("X-Cache")) | ||
| } | ||
|
|
||
| // Wait for the tracked background write to complete before the second request. | ||
| mw.simple.wg.Wait() | ||
|
|
||
| rec2 := driveHandleRequest(t, mw, workflow, body, nil, next) | ||
| if rec2.Code != http.StatusOK { | ||
| t.Fatalf("second request: got status %d", rec2.Code) | ||
| } | ||
| if rec2.Header().Get("X-Cache") != "HIT (exact)" { | ||
| t.Fatalf("second request should have X-Cache=HIT (exact), got %s", rec2.Header().Get("X-Cache")) | ||
| } | ||
| if !bytes.Contains(rec2.Body.Bytes(), []byte("cached")) { | ||
| t.Fatalf("cached response body missing expected content: %s", rec2.Body.String()) | ||
| } | ||
| if callCount != 1 { | ||
| t.Fatalf("exact hit should not call next again, callCount=%d", callCount) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Missing mw.Close() leaks the cache-write worker pool across tests.
TestHandleRequest_ExactCacheHit, TestHandleRequest_DifferentBodyDifferentKey, TestHandleRequest_SeparatesStreamingAndNonStreamingEntries, and TestHandleRequest_SkipsNoCache only defer store.Close() and never call mw.Close(). Given TestClose_WaitsForPendingWrites's own comment that "Close must drain any in-flight write before closing the store" and that Close is needed to avoid racing the store's Set against its Close, this implies ResponseCacheMiddleware spawns a persistent cacheWriteWorkerCount-sized worker pool that only terminates via Close(). Without it, each of these tests leaks worker goroutines that outlive the test.
♻️ Proposed fix
func TestHandleRequest_ExactCacheHit(t *testing.T) {
store := cache.NewMapStore()
defer store.Close()
mw := NewResponseCacheMiddlewareWithStore(store, time.Hour)
+ defer mw.Close()
workflow := resolvedWorkflow("openai", "gpt-4")(apply similarly to the other three tests)
Also applies to: 144-166, 242-319, 382-404
🤖 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/exact_cache_test.go` around lines 106 - 142, The
response cache tests are leaking the middleware’s background write workers
because they only close the store and never shut down the
ResponseCacheMiddleware. Update TestHandleRequest_ExactCacheHit and the other
affected tests to defer mw.Close() after creating the middleware from
NewResponseCacheMiddlewareWithStore, so the worker pool and any in-flight writes
are drained before the store is closed. Keep the existing store.Close() defer,
but ensure mw.Close() runs first to terminate the cache-write goroutines
cleanly.
2 participants
Summary
Completes possible-refactoring item #4, the follow-up deferred from #474. Production traffic has gone exclusively through
HandleRequest/HandleInternalRequestsince the exchange seam landed; the echo middleware wrapper was referenced only by its own tests.Removed:
ResponseCacheMiddleware.Middleware()andsimpleCacheMiddleware.Middleware()requestBodyForCache,shouldSkipCacheForWorkflow,shouldSkipCache— production equivalents live inhandle()(Cache-Control skip viashouldSkipCacheControl) or at the call sites, which pass the patched body explicitly and gate onworkflow.CacheEnabled()(translated_inference_service.go,internal_chat_completion_executor.go)Test migration (per that doc's guidance)
middleware_test.go→exact_cache_test.go. Preserved by converting to productionHandleRequestdrivers: exact hit/miss + X-Cache header/status/body assertions, different-body keying, streaming-vs-JSON separate entries with verbatim SSE replay,Cache-Control: no-cachebypass, close-drains-pending-writes, and bounded write concurrency. Pure-helper tests (hashRequest×4,isStreamingRequest+ benchmarks) kept verbatim.Dropped, with reasons: workflow-gating bypass tests (that gate was legacy-middleware-only; callers gate before invoking the cache), snapshot-body precedence and body-read-error tests (callers supply the body; the helper is gone), and the non-cacheable-path test (exact-layer path gating was legacy-only; the semantic layer still enforces
cacheablePathsinternally).User-visible impact
None. The removed API had no production callers.
Testing
Full
go build ./...andgo test ./...pass;go test -race -count=2on responsecache;golangci-lint0 issues; pre-commitmake test-raceon the commit.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Documentation
Tests