Skip to content

Commit

Permalink
fix: revalidation with file-system-cache (#58508)
Browse files Browse the repository at this point in the history
### What?

When using the file system cache with `isrMemoryCacheSize: 0`, time-based revalidation is not working, and the file is constantly updated. I have also added some debug logging to mirror that in the `fetch-cache` handler

Detailed explanation in #58507

### Why?

The cached object's tags are incorrectly accessed, causing the cache to be rewritten every hit. This is catastrophic for a caching system that relies on file modification timestamps. The tags are one level up in the object from where [they are currently being accessed](https://github.com/vercel/next.js/blob/9ab8828f72e96f5de86a7c50b67a1c5abe6e146c/packages/next/src/server/lib/incremental-cache/file-system-cache.ts#L178).

Below shows a cached fetch representation on disk. When written, the tags reside at `obj.tags` instead of `obj.data.tags`

```json
{
  "kind": "FETCH",
  "data": {
    "headers": {
      "connection": "keep-alive",
      "content-encoding": "br",
      "content-type": "application/json; charset=utf-8",
      "date": "Wed, 15 Nov 2023 21:17:42 GMT",
      "server": "nginx/1.18.0 (Ubuntu)",
      "transfer-encoding": "chunked",
      "vary": "Accept-Encoding"
    },
    "body": "[SNIP]",
    "status": 200,
    "url": "https://timeapi.io/api/Time/current/zone?timeZone=UTC"
    # this is where the current code is trying to pull the tags
    # "tags": [ "time-with-fetch" ]
  },
  "revalidate": 20,
  # tags actually live here
  "tags": [
    "time-with-fetch"
  ]
}
```

Fixes #58507
  • Loading branch information
gtjamesa authored Nov 16, 2023
1 parent 82beef9 commit df4c2aa
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export default class FileSystemCache implements CacheHandler {
private tagsManifestPath?: string
private revalidatedTags: string[]
private readonly experimental: { ppr: boolean }
private debug: boolean

constructor(ctx: FileSystemCacheContext) {
this.fs = ctx.fs
Expand All @@ -47,8 +48,13 @@ export default class FileSystemCache implements CacheHandler {
this.pagesDir = !!ctx._pagesDir
this.revalidatedTags = ctx.revalidatedTags
this.experimental = ctx.experimental
this.debug = !!process.env.NEXT_PRIVATE_DEBUG_CACHE

if (ctx.maxMemoryCacheSize && !memoryCache) {
if (this.debug) {
console.log('using memory store for fetch cache')
}

memoryCache = new LRUCache({
max: ctx.maxMemoryCacheSize,
length({ value }) {
Expand All @@ -69,7 +75,10 @@ export default class FileSystemCache implements CacheHandler {
)
},
})
} else if (this.debug) {
console.log('not using memory store for fetch cache')
}

if (this.serverDistDir && this.fs) {
this.tagsManifestPath = path.join(
this.serverDistDir,
Expand All @@ -91,9 +100,14 @@ export default class FileSystemCache implements CacheHandler {
} catch (err: any) {
tagsManifest = { version: 1, items: {} }
}
if (this.debug) console.log('loadTagsManifest', tagsManifest)
}

public async revalidateTag(tag: string) {
if (this.debug) {
console.log('revalidateTag', tag)
}

// we need to ensure the tagsManifest is refreshed
// since separate workers can be updating it at the same
// time and we can't flush out of sync data
Expand All @@ -112,6 +126,9 @@ export default class FileSystemCache implements CacheHandler {
this.tagsManifestPath,
JSON.stringify(tagsManifest || {})
)
if (this.debug) {
console.log('Updated tags manifest', tagsManifest)
}
} catch (err: any) {
console.warn('Failed to update tags manifest.', err)
}
Expand All @@ -122,6 +139,10 @@ export default class FileSystemCache implements CacheHandler {
const { tags, softTags, kindHint } = ctx
let data = memoryCache?.get(key)

if (this.debug) {
console.log('get', key, tags, kindHint, !!data)
}

// let's check the disk for seed data
if (!data && process.env.NEXT_RUNTIME !== 'edge') {
try {
Expand Down Expand Up @@ -175,12 +196,15 @@ export default class FileSystemCache implements CacheHandler {
}

if (data.value?.kind === 'FETCH') {
const storedTags = data.value?.data?.tags
const storedTags = data.value?.tags

// update stored tags if a new one is being added
// TODO: remove this when we can send the tags
// via header on GET same as SET
if (!tags?.every((tag) => storedTags?.includes(tag))) {
if (this.debug) {
console.log('tags vs storedTags mismatch', tags, storedTags)
}
await this.set(key, data.value, { tags })
}
}
Expand Down Expand Up @@ -296,6 +320,10 @@ export default class FileSystemCache implements CacheHandler {
value: data,
lastModified: Date.now(),
})
if (this.debug) {
console.log('set', key)
}

if (!this.flushToDisk) return

if (data?.kind === 'ROUTE') {
Expand Down
6 changes: 3 additions & 3 deletions packages/next/src/server/response-cache/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ export interface CachedFetchValue {
body: string
url: string
status?: number
// tags are only present with file-system-cache
// fetch cache stores tags outside of cache entry
tags?: string[]
}
// tags are only present with file-system-cache
// fetch cache stores tags outside of cache entry
tags?: string[]
revalidate: number
}

Expand Down
80 changes: 80 additions & 0 deletions test/unit/incremental-cache/file-system-cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,83 @@ describe('FileSystemCache', () => {
})
})
})

describe('FileSystemCache (isrMemory 0)', () => {
const fsCache = new FileSystemCache({
_appDir: true,
_pagesDir: true,
_requestHeaders: {},
flushToDisk: true,
fs: nodeFs,
serverDistDir: cacheDir,
revalidatedTags: [],
experimental: {
ppr: false,
},
maxMemoryCacheSize: 0, // disable memory cache
})

it('should cache fetch', async () => {
await fsCache.set(
'fetch-cache',
{
kind: 'FETCH',
data: {
headers: {},
body: 'MTcwMDA1NjM4MQ==',
status: 200,
url: 'http://my-api.local',
},
revalidate: 30,
},
{
fetchCache: true,
revalidate: 30,
fetchUrl: 'http://my-api.local',
fetchIdx: 5,
tags: ['server-time'],
}
)

const res = await fsCache.get('fetch-cache', {
tags: ['server-time'],
kindHint: 'fetch',
})

expect(res.value).toEqual({
kind: 'FETCH',
data: {
headers: {},
body: 'MTcwMDA1NjM4MQ==',
status: 200,
url: 'http://my-api.local',
},
revalidate: 30,
tags: ['server-time'],
})
})

it('should cache unstable_cache', async () => {
await fsCache.set(
'unstable-cache',
{
kind: 'FETCH',
data: { headers: {}, body: '1700056381', status: 200, url: '' },
revalidate: 30,
},
{ revalidate: 30, fetchCache: true, tags: ['server-time2'] }
)

const res = await fsCache.get('unstable-cache', {
tags: ['server-time'],
kindHint: 'fetch',
})

expect(res.value).toEqual({
kind: 'FETCH',
data: { headers: {}, body: '1700056381', status: 200, url: '' },
revalidate: 30,
tags: ['server-time2'],
})
})
})

1 comment on commit df4c2aa

@ijjk
Copy link
Member

@ijjk ijjk commented on df4c2aa Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stats from current release

Default Build
General Overall increase ⚠️
vercel/next.js canary v14.0.3 vercel/next.js canary Change
buildDuration 10s 12.3s ⚠️ +2.4s
buildDurationCached 6s 6s N/A
nodeModulesSize 199 MB 199 MB ⚠️ +23 kB
nextStartRea..uration (ms) 426ms 428ms N/A
Client Bundles (main, webpack)
vercel/next.js canary v14.0.3 vercel/next.js canary Change
199-HASH.js gzip 28.7 kB 28.8 kB N/A
3f784ff6-HASH.js gzip 53.3 kB 53.3 kB
494.HASH.js gzip 180 B 181 B N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 242 B 239 B N/A
main-HASH.js gzip 31.7 kB 31.8 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB
Overall change 100 kB 100 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary v14.0.3 vercel/next.js canary Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary v14.0.3 vercel/next.js canary Change
_app-HASH.js gzip 194 B 195 B N/A
_error-HASH.js gzip 182 B 181 B N/A
amp-HASH.js gzip 504 B 506 B N/A
css-HASH.js gzip 322 B 323 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB
edge-ssr-HASH.js gzip 253 B 255 B N/A
head-HASH.js gzip 348 B 347 B N/A
hooks-HASH.js gzip 369 B 368 B N/A
image-HASH.js gzip 4.3 kB 4.3 kB N/A
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.63 kB 2.63 kB N/A
routerDirect..HASH.js gzip 311 B 311 B
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 307 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.17 kB 3.17 kB
Client Build Manifests
vercel/next.js canary v14.0.3 vercel/next.js canary Change
_buildManifest.js gzip 484 B 483 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary v14.0.3 vercel/next.js canary Change
index.html gzip 528 B 527 B N/A
link.html gzip 539 B 540 B N/A
withRouter.html gzip 524 B 522 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary v14.0.3 vercel/next.js canary Change
edge-ssr.js gzip 92.6 kB 92.6 kB N/A
page.js gzip 145 kB 145 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary v14.0.3 vercel/next.js canary Change
middleware-b..fest.js gzip 622 B 626 B N/A
middleware-r..fest.js gzip 150 B 151 B N/A
middleware.js gzip 24.8 kB 24.8 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 1.92 kB 1.92 kB
Next Runtimes
vercel/next.js canary v14.0.3 vercel/next.js canary Change
app-page-exp...dev.js gzip 167 kB 167 kB N/A
app-page-exp..prod.js gzip 93.3 kB 93.4 kB N/A
app-page-tur..prod.js gzip 94.1 kB 94.1 kB N/A
app-page-tur..prod.js gzip 88.7 kB 88.7 kB N/A
app-page.run...dev.js gzip 137 kB 137 kB N/A
app-page.run..prod.js gzip 88 kB 88 kB N/A
app-route-ex...dev.js gzip 23.8 kB 23.8 kB N/A
app-route-ex..prod.js gzip 16.4 kB 16.4 kB N/A
app-route-tu..prod.js gzip 16.4 kB 16.4 kB N/A
app-route-tu..prod.js gzip 16 kB 16 kB
app-route.ru...dev.js gzip 23.2 kB 23.2 kB N/A
app-route.ru..prod.js gzip 16 kB 16 kB
pages-api-tu..prod.js gzip 9.37 kB 9.37 kB
pages-api.ru...dev.js gzip 9.64 kB 9.64 kB
pages-api.ru..prod.js gzip 9.37 kB 9.37 kB
pages-turbo...prod.js gzip 21.8 kB 21.8 kB
pages.runtim...dev.js gzip 22.5 kB 22.5 kB
pages.runtim..prod.js gzip 21.8 kB 21.8 kB
server.runti..prod.js gzip 49 kB 49.1 kB N/A
Overall change 127 kB 127 kB
Diff details
Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for 199-HASH.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js
failed to diff
Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js
failed to diff
Diff for app-page.runtime.prod.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Please # to comment.