Skip to content

fix: [#2373] ResponseCache must not evict responses revalidatable via Last-Modified - #2266

Open
marciomazza wants to merge 1 commit into
capricorn86:masterfrom
marciomazza:fix/response-cache-lastmodified-eviction
Open

fix: [#2373] ResponseCache must not evict responses revalidatable via Last-Modified#2266
marciomazza wants to merge 1 commit into
capricorn86:masterfrom
marciomazza:fix/response-cache-lastmodified-eviction

Conversation

@marciomazza

@marciomazza marciomazza commented Jul 31, 2026

Copy link
Copy Markdown

Description

Resolves #2373

ResponseCache.get() keeps an expired entry that has a Last-Modified date as
stale so the next request can revalidate it with If-Modified-Since.
ResponseCache.add() didn't: its eviction check only looked at ETag, so a
response with Cache-Control: max-age=0 + Last-Modified and no ETag (a
common "always revalidate" setup) was purged immediately, turning every
follow-up request into an uncached GET instead of a conditional one.

import { Window } from 'happy-dom';

const window = new Window({ url: 'https://example.com/' });
await window.fetch('/data');   // Last-Modified, no ETag, max-age=0
await window.fetch('/data');   // happy-dom: plain GET 200 — browsers: 304 revalidation

Root cause: add()'s expiry cleanup dropped any expired entry without an
ETag, ignoring lastModified — out of sync with the check in get().

Fix: add the same !cachedResponse.lastModified term to add()'s eviction
condition, so an entry is only dropped when it has neither validator.

Tests

Added 2 tests (test/fetch/Fetch.test.ts, test/fetch/SyncFetch.test.ts): an
already-expired Last-Modified response with no ETag and no Cache-Control
must stay cached so the next request is a conditional If-Modified-Since GET,
not a plain refetch. Both fail on master (the second request goes out
unconditional) and pass with this change. Full npm test passes.

Verification

The new tests use an Expires date in the past rather than a sub-millisecond
max-age, so they fail deterministically on master instead of only under
scheduling latency — this same eviction is the cause of the intermittent
SyncFetch.test.ts / Fetch.test.ts cache-revalidation failures on master.
npm test (happy-dom package) — 302 files, 7721 tests green.
Behavior checked against HTTP caching semantics (RFC 9111 §4.3): a stored
response with a validator must be revalidated on expiry, not discarded.

AI

I used Claude Code to write this, over multiple iterations. I guided and reviewed
it myself at every step.

Before submitting the PR, please make sure you do the following:

  • Read the contributing guidelines.
  • It's really useful if your PR references an issue where it is discussed ahead of time.
  • Please check Allow edits by maintainers to make review process faster. Note that this option is not available for repositories that are owned by Github organizations.

Tests

  • Make sure to add tests for your changes. Run your test in a real browser to make sure that the test tests what a real browser would do (e.g. by running the code in the browser console).
    • Added 2 deterministic regression tests (Fetch.test.ts, SyncFetch.test.ts). Not runnable in a browser console (server-side response cache); verified against RFC 9111 §4.3 conditional-request semantics instead.
  • Run the tests with npm test locally to make sure that all tests pass before submitting the PR.

Title

  • The title of the pull request should be in the format of "type: [#issue] description". The type can be feat, fix, chore or BREAKING CHANGE. The issue is optional and can be omitted if the pull request does not relate to an issue.
  • The title should be concise and descriptive. The title will be used when generating release notes. Make sure that the title is easily understood by users of the library.

AI tools

  • Please disclose in the PR description that you used AI tools to generate code. This is important for transparency and to ensure that the generated code meets the quality standards of the project.

…t-Modified

add()'s expiry cleanup deleted any cache entry without an ETag as soon as it
was expired, ignoring Last-Modified entirely - unlike get(), which keeps an
expired entry with a Last-Modified date around as "stale" so it can be
revalidated with a conditional If-Modified-Since request.

This mismatch meant a response with a very small/zero max-age plus a
Last-Modified header (a common "always revalidate" pattern) was purged from
the cache immediately instead of being kept for revalidation, causing the
next request to be an ordinary uncached GET instead of a conditional one.
This also caused the intermittent SyncFetch.test.ts cache-revalidation
failures on master.

Adds deterministic regression tests in Fetch.test.ts and SyncFetch.test.ts:
an already-expired Last-Modified response with no ETag or Cache-Control
must stay cached so the next request is a conditional If-Modified-Since GET.
@marciomazza
marciomazza force-pushed the fix/response-cache-lastmodified-eviction branch from 4e560b9 to 4f36c23 Compare September 8, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cached responses with only Last-Modified are evicted on expiry instead of being revalidated

1 participant