update the age header when responding from cache - #15052
Conversation
🦋 Changeset detectedLatest commit: 869414f The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const now = this.timers.now(); | ||
| const age = parseInt(resHeaders.get("age") || "0", 10); | ||
| const cachedDuration = Math.round((now - (cached.metadata.stored || now)) / 1000) | ||
| resHeaders.set("Age", age + cachedDuration); |
There was a problem hiding this comment.
🟡 Behaviour change to cached response headers ships without any test coverage
The change that rewrites the age of responses served from the local cache (resHeaders.set("Age", ...) at packages/miniflare/src/workers/cache/cache.worker.ts:309) has no accompanying test, which the repository requires for new functionality.
Impact: A regression in the reported age of cached responses could ship unnoticed.
Repository testing requirement
CONTRIBUTING.md ("PR Tests") states: "Every PR should include tests for the functionality that's being added", and Miniflare tests live in packages/miniflare/test (e.g. test/plugins/cache/index.spec.ts). The PR modifies cache HIT behaviour and stores a new stored metadata field but adds no spec covering either the incremented age or the fallback for entries stored before this change.
Prompt for agents
Add a Miniflare test (packages/miniflare/test/plugins/cache/index.spec.ts, using the existing miniflareTest harness and the fake timers utilities) verifying that a response served from the cache reports an age equal to the original age plus the elapsed time since it was stored, and that entries whose stored metadata is absent (written by an older version) keep their original age.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| headers: Object.entries(headers), | ||
| status: res.status, | ||
| size, | ||
| stored: this.timers.now(), |
There was a problem hiding this comment.
🟡 Cached responses can report an age that is too young for slow or large uploads
The time a cached item was saved is recorded (this.timers.now() at packages/miniflare/src/workers/cache/cache.worker.ts:366) only after the whole body has finished being written, rather than when the item was received, so slow or large responses are later reported as newer than they are.
Impact: Clients can be told a cached response is fresher than it really is, which can make them hold on to stale content longer.
Why the timestamp lands late: metadata promise resolves after the blob write
metadata is a promise created from sizePromise (packages/miniflare/src/workers/cache/cache.worker.ts:355-367). KeyValueStorage.put() first awaits this.#blob.put(entry.value) and only then awaits entry.metadata (packages/miniflare/src/workers/shared/keyvalue.worker.ts:208-223), so this.timers.now() inside the .then() executes after the entire body has been streamed. By contrast, the entry's expiration is computed synchronously at request time (packages/miniflare/src/workers/cache/cache.worker.ts:372), so stored and expiration use different clocks. Capturing the timestamp once, before starting the stream, and reusing it for both would keep them consistent.
Prompt for agents
In packages/miniflare/src/workers/cache/cache.worker.ts, the `put` handler records `stored: this.timers.now()` inside the metadata promise (`sizePromise.then(...)`). Because `KeyValueStorage.put()` awaits the blob write before awaiting the metadata promise (packages/miniflare/src/workers/shared/keyvalue.worker.ts), this timestamp is taken after the whole response body has been streamed, not when the response was received. The `expiration` passed to `storage.put` is computed synchronously at request time, so the two values are based on different clock readings; for large or slow bodies the entry's recorded store time is later than its effective expiry basis, making the Age header computed in `match` too small. Consider capturing a single `now` value before starting the stream and using it for both `stored` and `expiration`.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Fixes #[insert GH or internal issue link(s)].
When responding from the cache (HIT), update the Age header to be relative to the time the object was stored.
A picture of a cute animal (not mandatory, but encouraged)