update the age header when responding from cache - #15052
Conversation
🦋 Changeset detectedLatest commit: f3dcb37 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.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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.
NuroDev
left a comment
NuroDev
left a comment
There was a problem hiding this comment.
Can you add some tests to this PR so we can validate this change actually works as intended.
Additionally can you update the PR description to fill out the missing details.
| "miniflare": patch | ||
| --- | ||
|
|
||
| Increment the Age response header when responding from cache. |
There was a problem hiding this comment.
Can we add some more details to this changeset so it makes it a bit clearer on the change and what it actually fixes.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
869414f to
f3dcb37
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
We recently landed #14994, a large change to Miniflare's configuration internals that touched ~177 files across the repo. Leaving this PR on its old base was likely to cause conflicts, so we've rebased it onto the latest Your local copy of this branch is now out of date. Before you push again, please reset to the new version: git fetch origin
git checkout cache-hit-age
git reset --hard origin/cache-hit-ageBecause the base moved a long way, it's also worth reinstalling before you carry on — the lockfile changed: pnpm installSorry for the interruption. If the rebase looks wrong, or CI now fails in a way that seems related to the Miniflare config change rather than your own work, comment here and we'll help get it sorted. |
4 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)