Fix ISR page re-rendering after revalidate expiry (Fork of #24807) #27335

jamsinclair · 2021-07-20T11:33:16Z

Fixes: #24806
Fixes: #26689
Fixes: #27325
Closes: #24807

@tommarshall has done us a huge favor with his PR #24807 which outlines exactly the issue and a pragmatic solution.

I'm not trying to "steal their work", however, the PR seems to have been stuck for some months. I think there's huge value in this for myself and others as essentially ISR is broken for people running Next.js at scale 😱

This PR has cherry-picked @tommarshall's fine fix and added some integrations tests around page revalidation and the edge case when the cache size is exhausted.

✏️ Edits are enabled, so feel free great Vercel staff and other maintainers to improve my bad tests or surrounding code 🙇

Bug

Related issues linked using fixes #number
Integration tests added

* As outlined in vercel#24806 I there's currently a bug with the incremental static regeneration (ISR) logic incorrectly prolonging the life of rendered page content when it's the `LRUCache` is refreshed with potentially stale content from the filesystem cache. * This because if a page is not present in the `LRUCache`, but is present in the filesystem cache then it's re-added to the `LRUCache` with a fresh cache expiry calculated from the current time using the `revalidate` window. * If that page continues to be pushed out of the LRUCache before the next request comes in after the revalidation window expires then this creates an endless loop of the same stale content from the filesystem being served, without it being regenerated. * This fix addresses that issue by adding a `fromTime` argument to the revalidation period calculation, so that when we're falling back to the filesystem we can use the filesystem's `lastModfied` timestamp for that file to anchor the revalidation calculation, rather than restarting from the current time in every case. * That means that if page content from the filesystem is stale it will be marked as so when returned from the incremental cache `get` method (`isStale: true`), which tells next server to regenerate the page in the background. * If however the the page content from the filesystem is still within the revalidate window (i.e. is not yet stale) then it will be added back to the LRUCache with the correct remaining duration for the `revalidateAfter`. Fixes vercel#24806

jamsinclair · 2021-07-20T11:35:14Z

test/integration/prerender-revalidate/test/index.test.js

+    runTests('/', '/')
+    runTests('/named', '/named')
+    runTests('/nested', '/nested')
+    runTests('/nested/named', '/nested/named')


Not sure if it is useful to test against multiple routes? If it's robust enough to test against just one, let me know and I can simplify the suite.

jamsinclair · 2021-07-20T11:36:18Z

packages/next/server/incremental-cache.ts

+    if (process.env.__NEXT_TEST_MAX_ISR_CACHE) {
+      // Allow cache size to be overridden for testing purposes
+      max = parseInt(process.env.__NEXT_TEST_MAX_ISR_CACHE, 10)
+    }


Solution inspired by #24807 (review)

@stefanb2's feature suggestion would also solve this nicely #27337. Those of us pushing the cache to its limit may want to configure this if we have memory to spare.

…idate-period

ijjk · 2021-07-20T16:59:39Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
buildDuration	13.1s	13.1s	⚠️ +12ms
buildDurationCached	3.1s	3s	-116ms
nodeModulesSize	49.5 MB	49.5 MB	⚠️ +894 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.087	2.14	⚠️ +0.05
/ avg req/sec	1197.67	1168.33	⚠️ -29.34
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.125	1.136	⚠️ +0.01
/error-in-render avg req/sec	2223.05	2201.6	⚠️ -21.45

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
359.HASH.js gzip	2.96 kB	2.96 kB	✓
745.HASH.js gzip	180 B	180 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	20.9 kB	20.9 kB	✓
webpack-HASH.js gzip	1.53 kB	1.53 kB	✓
Overall change	67.8 kB	67.8 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
_app-HASH.js gzip	803 B	803 B	✓
_error-HASH.js gzip	3.06 kB	3.06 kB	✓
amp-HASH.js gzip	554 B	554 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.52 kB	2.52 kB	✓
head-HASH.js gzip	2.28 kB	2.28 kB	✓
hooks-HASH.js gzip	903 B	903 B	✓
image-HASH.js gzip	5.61 kB	5.61 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	319 B	319 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	19.1 kB	19.1 kB	✓

Client Build Manifests

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
_buildManifest.js gzip	490 B	490 B	✓
Overall change	490 B	490 B	✓

Rendered Page Sizes

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
index.html gzip	529 B	529 B	✓
link.html gzip	543 B	543 B	✓
withRouter.html gzip	523 B	523 B	✓
Overall change	1.59 kB	1.59 kB	✓

Webpack 4 Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
buildDuration	10.5s	10.4s	-73ms
buildDurationCached	4.1s	4.1s	⚠️ +14ms
nodeModulesSize	49.5 MB	49.5 MB	⚠️ +894 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.137	2.128	-0.01
/ avg req/sec	1169.92	1174.84	+4.92
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.154	1.126	-0.03
/error-in-render avg req/sec	2166.65	2219.83	+53.18

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
17.HASH.js gzip	2.98 kB	2.98 kB	✓
18.HASH.js gzip	185 B	185 B	✓
677f882d2ed8..HASH.js gzip	13.7 kB	13.7 kB	✓
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	8.4 kB	8.4 kB	✓
webpack-HASH.js gzip	1.22 kB	1.22 kB	✓
Overall change	68.4 kB	68.4 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
polyfills-HASH.js gzip	31.3 kB	31.3 kB	✓
Overall change	31.3 kB	31.3 kB	✓

Client Pages

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
_app-HASH.js gzip	791 B	791 B	✓
_error-HASH.js gzip	3.76 kB	3.76 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	2.97 kB	2.97 kB	✓
hooks-HASH.js gzip	911 B	911 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	3.02 kB	3.02 kB	✓
withRouter-HASH.js gzip	294 B	294 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	17.6 kB	17.6 kB	✓

Client Build Manifests

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes

	vercel/next.js canary	jamsinclair/next.js fix-isr-pages-not-re-rendering-after-revalidate-period	Change
index.html gzip	577 B	577 B	✓
link.html gzip	588 B	588 B	✓
withRouter.html gzip	570 B	570 B	✓
Overall change	1.74 kB	1.74 kB	✓

Commit: 404d524

ijjk

Thanks for continuing this on and getting the tests added! 🙌

@tommarshall

…) (vercel#27335) Fixes: vercel#24806 Fixes: vercel#26689 Fixes: vercel#27325 Closes: vercel#24807 @tommarshall has done us a huge favor with his PR vercel#24807 which outlines exactly the issue and a pragmatic solution. I'm not trying to "steal their work", however, the PR seems to have been stuck for some months. I think there's huge value in this for myself and others as essentially **ISR is broken** for people running Next.js at scale 😱 This PR has cherry-picked @tommarshall's fine fix and added some integrations tests around page revalidation and the edge case when the cache size is exhausted. ✏️ Edits are enabled, so feel free great Vercel staff and other maintainers to improve my bad tests or surrounding code 🙇 ## Bug - [x] Related issues linked using `fixes #number` - [x] Integration tests added

tommarshall and others added 3 commits July 20, 2021 15:48

allow cache size to be overridden with env var for testing purposes

107475f

add page revalidation and cache exceeded tests

b5dbf1c

jamsinclair requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners July 20, 2021 11:33

ijjk added the type: next label Jul 20, 2021

jamsinclair commented Jul 20, 2021

View reviewed changes

jamsinclair and others added 2 commits July 20, 2021 20:39

Merge branch 'canary' into fix-isr-pages-not-re-rendering-after-reval…

a91029c

…idate-period

fix typo execeeded => exceeded

3bc902a

This comment has been minimized.

Sign in to view

clarify test comment

3724be2

This comment has been minimized.

Sign in to view

ijjk added 2 commits July 20, 2021 11:03

Update existing test for corrected behavior

140503f

Merge branch 'canary' into fix-isr-pages-not-re-rendering-after-reval…

404d524

…idate-period

ijjk approved these changes Jul 20, 2021

View reviewed changes

kodiakhq bot merged commit 7c408e7 into vercel:canary Jul 20, 2021

ijjk mentioned this pull request Jul 20, 2021

Use timestamp from file when creating LRU entry (#27325) #27336

Closed

10 tasks

vercel locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ISR page re-rendering after revalidate expiry (Fork of #24807) #27335

Fix ISR page re-rendering after revalidate expiry (Fork of #24807) #27335

jamsinclair commented Jul 20, 2021 •

edited by ijjk

Loading

jamsinclair Jul 20, 2021 •

edited

Loading

jamsinclair Jul 20, 2021

jamsinclair Jul 20, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 20, 2021

ijjk left a comment

Fix ISR page re-rendering after revalidate expiry (Fork of #24807) #27335

Fix ISR page re-rendering after revalidate expiry (Fork of #24807) #27335

Conversation

jamsinclair commented Jul 20, 2021 • edited by ijjk Loading

Bug

jamsinclair Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

jamsinclair Jul 20, 2021

Choose a reason for hiding this comment

jamsinclair Jul 20, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 20, 2021

Stats from current PR

ijjk left a comment

Choose a reason for hiding this comment

jamsinclair commented Jul 20, 2021 •

edited by ijjk

Loading

jamsinclair Jul 20, 2021 •

edited

Loading