Don't cache conditional/bodyless responses; serve conditional requests from cached 200 - #204
Don't cache conditional/bodyless responses; serve conditional requests from cached 200#204kbukum1 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Prevents replaying bodyless conditional responses and enables conditional requests to reuse cached responses.
Changes:
- Excludes 1xx, 204, 205, and 304 responses from new cache writes.
- Collapses conditional and unconditional request cache keys.
- Adds unit and integration regression coverage.
Show a summary per file
| File | Description |
|---|---|
internal/cache/handlers.go |
Updates cache eligibility and conditional keys. |
internal/cache/handlers_test.go |
Tests bodyless response caching policy. |
proxy_test.go |
Tests conditional requests through the proxy. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
internal/cache/handlers.go:347
- This check only prevents new writes.
Newstill loads every entry from a pre-existingdb.yaml, andOnRequestserves those entries before this path runs, so 204/205 (and any validator-less 304) cached by an older proxy continue to be replayed after upgrade. ApplycacheableStatuswhile loading or serving persisted entries so the new policy also covers existing caches.
if !cacheableStatus(resp.StatusCode) {
// Never cache conditional / no-content responses (1xx/204/205/304).
// A cached 304 would replay a stale "Not Modified" and hide upstream
// package updates from the updater; there is no body to reuse anyway.
// Pass it straight through, correctly framed.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
…isted bodyless entries Revert the header-collapsing change (adding If-None-Match/If-Modified-Since to ignoreHeaders): collapsing a conditional request onto the unconditional key could serve a cached representation older than the one the client already holds, bypassing revalidation and masking upstream updates. Conditional validators now stay in the key, so a conditional request is never short-circuited to a stale cached 200. Also apply cacheableStatus when loading db.yaml so conditional/no-content entries (1xx/204/205/304) persisted by an older proxy are dropped on load and never replayed after upgrade — cacheableStatus previously only gated new writes. Update tests accordingly and add coverage for the persisted-entry filter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f21dba67-58ce-4aa3-a3ac-f33fcb5d086d
There was a problem hiding this comment.
Review details
Suppressed comments (1)
proxy_test.go:230
- The PR title says conditional requests are served from a cached 200, but this comment, the implementation, and the detailed description say validators remain in the key and conditional requests are not short-circuited. Please update the PR title so reviewers and release history describe the behavior accurately.
// Conditional validators stay in the cache key, so a conditional request is
// never short-circuited to a cached 200 (which could serve content older than
// the client already holds).
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| // desyncing the keep-alive MITM tunnel. This normalization runs whether | ||
| // or not we go on to cache the response. |
|
Closing this PR — the reported Why it fails CIThe smoke jobs download a pre-recorded proxy cache and expect ~100% cache hits (no real registry calls). Several ecosystems — bundler (compact-index), cargo, composer, hex, pip-compile — rely on conditional requests whose recorded responses are Evidence from this run:
TakeawayIn this proxy, caching and replaying Leaving the branch |
|
I am re-evaluating this option and will reopen if necessary |
What are you trying to accomplish?
Dependabot pnpm jobs were failing with:
The proxy cache was storing and replaying
304 Not Modified(and other bodyless) responses. Replaying a stale304tells a client "use your local copy" when, in a fresh job, it may not have that copy — breaking pnpm. There is also no body to reuse, so caching a304saves nothing while risking that the cache masks upstream package updates from the updater.Anything you want to highlight for special attention from reviewers?
Never cache
1xx/204/205/304(cacheableStatus). These carry no reusable body, and a replayed304is actively harmful for a dependency updater. They are passed straight through, correctly framed (the bodyless-body normalization from #201 still runs).The policy is enforced on both paths:
OnResponse) — non-cacheable statuses are never stored.New) — conditional/no-content entries persisted in adb.yamlby an older proxy are dropped on load, so they are never replayed after upgrade.Conditional-request validators (
If-None-Match/If-Modified-Since) are deliberately kept in the cache key. An earlier revision collapsed conditional requests onto the unconditional key to serve them from a cached200; that was dropped because it could serve a client a representation older than the one it already holds, bypassing revalidation and masking upstream updates. Keeping validators in the key means a conditional request is never short-circuited — it either misses and revalidates upstream, or matches its own cached entry.How will you know you've accomplished your goal?
New and updated tests, all passing (
go test ./...,-race,go vetclean):Test_cacheableStatus— status-code policy table.TestCache_PersistedNonCacheableEntriesAreNotLoaded— adb.yamlcontaining304/204entries loads only the cacheable200.TestProxyHTTPSConditionalNotModifiedIsNotCached— every conditional request reaches upstream (no stale304replay) over a single, healthy reused MITM tunnel.TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse— a passthrough304does not corrupt the tunnel and leaves the cached unconditional200intact.TestCache_BodylessResponses/TestCache_BodylessResponseWithWrappedBodyIsRestored— bodyless bodies are still normalized tohttp.NoBody, but conditional/no-content statuses are no longer cached (only HEAD for a cacheable status is stored, headers-only).Checklist