Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revalidate expiration check is incorrect for paths not in LRU cache #27325

Closed
stefanb2 opened this issue Jul 20, 2021 · 3 comments · Fixed by #27335
Closed

Revalidate expiration check is incorrect for paths not in LRU cache #27325

stefanb2 opened this issue Jul 20, 2021 · 3 comments · Fixed by #27335
Labels
bug Issue was opened via the bug report template.

Comments

@stefanb2
Copy link
Contributor

What version of Next.js are you using?

10.2.3

What version of Node.js are you using?

14.17.1

What browser are you using?

Chrome

What operating system are you using?

Linux

How are you deploying your application?

next start in container on Google Cloud

Describe the Bug

The revalidation expiration check is based on a timestamp kept in the LRU cache data structure (data.revalidateAfter). The timestamp based on the current time when the LRU cache entry is created. As long as the LRU cache entry exists the revalidation expiration check on cache fetch works correctly.

But as soon as the LRU cache is filled entries get purged. When a path is requested again then the timestamp is not based on the file in the page cache on disk. In worst case a path always gets purged from LRU cache before it is requested again, hence the file in the page cache on disk never gets updated.

Side note: the size of the LRU cache is fixed to 50MB. There is no configuration option to control its size. For our use case we would like to increase it, because it can only hold 25 to 200 of our pages out of ~8000.

Expected Behavior

Revalidation timestamp should be based on the file timestamp in the page cache on disk. I.e. when an overdue page is requested and therefore added to the LRU cache, the revalidation check should expire immediately.

To Reproduce

This problem can be observed f.ex. on the index page of our production site. All of our pages set revalidate to 86400 seconds, i.e. one day. The HTML code includes a timestamp from the getStaticProps() call that generated it. At the time of this writing it is 2021-06-18T11:33:01.814Z, i.e. the page has never been regenerated although it is already over a month old.

I do know that our production site is behind a CDN, but I have verified the issue by talking straight to the server in the production container and a local production build.
@stefanb2 stefanb2 added the bug Issue was opened via the bug report template. label Jul 20, 2021
stefanb2 added a commit to stefanb2/next.js that referenced this issue Jul 20, 2021
@stefanb2
Use timestamp from file when creating LRU entry (vercel#27325)
29bf809 
This makes sure that revalidation expiration check in IncrementalCache
get() is based on when the file in page cache has been changed and not
on the time when the LRU entry has been created.

- calculateRevalidate(): add optional timestamp to use instead of current
  time
- get(): use HTML file mtime to calculate revalidateAfter value

Fixes vercel#27325
@stefanb2 stefanb2 mentioned this issue Jul 20, 2021
Closed
10 tasks
@jamsinclair jamsinclair mentioned this issue Jul 20, 2021
Merged
2 tasks
stefanb2 added a commit to stefanb2/next.js that referenced this issue Jul 20, 2021
@stefanb2
Add configuration for LRU Cache Size (vercel#21535)
e243103 
This allows the user to adjust the LRU cache size according to the
application need:

- optimize the cache size based on average page HTML & JSON size
- remove the cache limit and let it grow to cache all pages
- make it so small to practically disable it

The hard-coded default of 50MB has been moved from the code to the
default configuration object.

Resolves vercel#21535
See also vercel#27325
@stefanb2 stefanb2 mentioned this issue Jul 20, 2021
Merged
10 tasks
stefanb2 added a commit to stefanb2/next.js that referenced this issue Jul 20, 2021
@stefanb2
Add configuration for LRU Cache Size (vercel#21535)
a83e419 
This allows the user to adjust the LRU cache size according to the
application need:

- optimize the cache size based on average page HTML & JSON size
- remove the cache limit and let it grow to cache all pages
- make it so small to practically disable it

The hard-coded default of 50MB has been moved from the code to the
default configuration object.

Fixes vercel#21535
See also vercel#27325
@stefanb2
Copy link
Contributor Author

I guess this could be closed as duplicate of #24806

@kodiakhq kodiakhq bot closed this as completed in #27335 Jul 20, 2021
kodiakhq bot pushed a commit that referenced this issue Jul 20, 2021
@jamsinclair
Fix ISR page re-rendering after revalidate expiry (Fork of #24807) (#…
7c408e7 
…27335)

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

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
@ijjk
Copy link
Member

ijjk commented Jul 20, 2021

Hi, this has been updated in v11.0.2-canary.18 of Next.js, please update and give it a try!

kodiakhq bot pushed a commit that referenced this issue Jul 21, 2021
@stefanb2
Add configuration for ISR Memory Cache Size (#21535) (#27337)
dbfce34 
This allows the user to adjust the LRU cache size according to the
application need:

- optimize the cache size based on average page HTML & JSON size
- disable the memory cache by setting the size to 0

The hard-coded default of 50MB has been moved from the code to the
default configuration object.

Fixes #21535
See also #27325



## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [X] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [X] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [X] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes
flybayer pushed a commit to blitz-js/next.js that referenced this issue Aug 19, 2021
@jamsinclair
Fix ISR page re-rendering after revalidate expiry (Fork of vercel#24807…
a8b071a 
…) (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
flybayer pushed a commit to blitz-js/next.js that referenced this issue Aug 19, 2021
@stefanb2
Add configuration for ISR Memory Cache Size (vercel#21535) (vercel#27337
5ae27b5 
)

This allows the user to adjust the LRU cache size according to the
application need:

- optimize the cache size based on average page HTML & JSON size
- disable the memory cache by setting the size to 0

The hard-coded default of 50MB has been moved from the code to the
default configuration object.

Fixes vercel#21535
See also vercel#27325



## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [X] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [X] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [X] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Milestone
No milestone
3 participants
@stefanb2 @balazsorban44 @ijjk