fix(cache): compare tag revalidation timestamps - #2916
Conversation
|
/bigbonk |
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dab5b34173
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk |
|
CI follow-up: fixed the integration failure by separating high-resolution mutation ordering (lastModified) from wall-clock TTL policy (writtenAt) in both Memory and KV handlers. Existing KV entries remain compatible via the lastModified fallback. Local verification: exact failed sequence 2/2, full features file 355/355, related 8-file matrix 1,903/1,903, scoped checks/diff-check, and independent Standards + blast-radius reviews pass. Waiting for CI; not requesting re-review yet. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbd28c1de2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@james-elicx I think this is ready for another look now. I fixed the latest ISR review comments, and CI is green. Thanks! |
|
I think we should generate the timestamp and pass it through the adapter, then have it persist and return that instead |
|
Updated in fe528e1. Cache producers now capture the timestamp before the fill starts and pass it through set(); Memory and KV persist and return it unchanged, while writtenAt remains adapter-owned for TTL accounting. I also carried the same boundary through App/Pages ISR, Route Handlers, metadata routes, use cache, unstable_cache, and tagged fetch, with ordering tests for both sides of an invalidation. |
|
@james-elicx I've incorporated Nathan's adapter feedback and fixed the one stale seed-cache assertion caught by CI. The full check suite is green on 729fdb7 now. Thanks for the patience - whenever you have time, this should be ready for another look. |
729fdb7 to
1874f69
Compare
|
@james-elicx, I rebased this onto current main and kept the producer-timestamp approach Nathan suggested. It is conflict-free and CI is green now. Could you take another look when you have a chance? |
1874f69 to
7fab14c
Compare
|
Rebased onto latest Verified locally: |
|
/bigbonk review for issues You have a hard 10-minute workflow time limit. Review the cumulative diff at exact head 4dca1c4, prioritizing correctness, Next.js parity, split response-stage forwarding, ISR/data-cache invalidation ordering, and the merged #3187 KV tag-prime logic. Post an explicit LGTM/no-findings verdict or concise actionable findings before the deadline. |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Hard 10-minute workflow limit. Review exact head 4dca1c4. Be verdict-first and concise: inspect only the cumulative cache invalidation timestamp and authenticated cross-stage forwarding changes, including merged #3187 KV tag-prime interaction. Post explicit LGTM/no findings or actionable correctness findings before the deadline; skip broad summaries. |
|
/bigbonk review for issues Hard 10-minute workflow limit. Review exact head 4dca1c4. Verdict first: inspect only the cumulative cache invalidation timestamp ordering and authenticated cross-stage forwarding changes, including the merged #3187 KV tag-prime interaction. Immediately post explicit LGTM/no findings if clean, or only concise actionable correctness findings; skip all broad summary. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
3 actionable correctness findings posted inline. No additional findings. |
|
/bigbonk review for issues Hard 10-minute workflow limit. Re-review cumulative diff at exact head 77e2f98 after fixes for all three prior findings: Workers zero timeOrigin ordering, forwarded KV tag-cache priming, and non-destructive forwarded stale reads. Verdict first: post explicit LGTM/no findings or only concise actionable correctness findings before the deadline. |
|
2 actionable correctness findings posted inline. |
|
/bigbonk review for issues Hard 10-minute workflow limit. Re-review exact head 0a445c9 after all prior findings. Focus on the new forwarded KV marker provenance: inclusive equality, preservation of stronger markers, detached-prime ordering, TTL replacement, and non-destructive later reads. Verdict first: post explicit LGTM/no findings or only concise actionable correctness findings before the deadline. |
|
LGTM! |
0a445c9 to
c390a56
Compare
|
Rebased onto the latest A few notes for review:
CI should be green on the Linux runners — a handful of build-spawning tests are flaky on my Windows machine, same as on |
…are#2916, rebased) Rebased onto current upstream main. cloudflare#2910 and cloudflare#2915 already landed upstream, so the integration branch is now upstream main plus this change set only.
3 participants
Summary
writtenAtfor TTL accountinguse cache,unstable_cache, tagged fetch, App/Pages ISR, Route Handlers, and metadata routesThis follows the behavior introduced by vercel/next.js#96726 and commit 5da1c1ae03d2ee39c27a2d6d8807c573c46b37f9.
The separate JSON tag header is a deliberate vinext protocol extension: Next.js's comma-delimited wire format cannot represent a legal tag containing a comma. New vinext peers prefer the authenticated JSON header; the standard header remains available to older peers for ordinary tags. Older peers cannot preserve comma-containing tags.
Closes #2819
Tests
isr-cache,fetch-cache, andkv-cache-handlercoveragevp checkpassed for all 26 changed product/test filesgit diff --checkpassedAdversarial coverage includes invalidation during a fill and immediately before a new fill, repeated same-tag invalidation, all three public cache APIs, App/Pages foreground and background ISR writes, Route Handlers, metadata routes, two-read Server Action dedupe, custom-handler old/new timestamps, authenticated redirect forwarding, comma-containing and JSON-looking legacy tags, protocol-header filtering, soft/hard tags, and Memory/KV strict timestamp boundaries.