Skip to content

Commit 5a0fec5

Browse files
piehpieh
authored
fix: track revalidate / cdn purge to ensure it finishes execution and is not suspended mid-execution (#2490)
* fix: track revalidate / cdn purge to ensure it finishes execution and is not suspended mid-execution * test: don't destroy lambda env vars until response stream finished * fix: remove 'any' types from next's response proxy * test: add a case for not awaited res.revalidate
1 parent 7e2b9e1 commit 5a0fec5

File tree

7 files changed

+148
-32
lines changed

7 files changed

+148
-32
lines changed

‎src/run/handlers/cache.cts‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -326,19 +326,33 @@ export class NetlifyCacheHandler implements CacheHandler {
326326
if (requestContext?.didPagesRouterOnDemandRevalidate) {
327327
const tag = `_N_T_${key === '/index' ? '/' : key}`
328328
getLogger().debug(`Purging CDN cache for: [${tag}]`)
329-
purgeCache({ tags: [tag] }).catch((error) => {
330-
// TODO: add reporting here
331-
getLogger()
332-
.withError(error)
333-
.error(`[NetlifyCacheHandler]: Purging the cache for tag ${tag} failed`)
334-
})
329+
requestContext.trackBackgroundWork(
330+
purgeCache({ tags: [tag] }).catch((error) => {
331+
// TODO: add reporting here
332+
getLogger()
333+
.withError(error)
334+
.error(`[NetlifyCacheHandler]: Purging the cache for tag ${tag} failed`)
335+
}),
336+
)
335337
}
336338
}
337339
})
338340
}
339341

340342
// eslint-disable-next-line @typescript-eslint/no-explicit-any
341343
async revalidateTag(tagOrTags: string | string[], ...args: any) {
344+
const revalidateTagPromise = this.doRevalidateTag(tagOrTags, ...args)
345+
346+
const requestContext = getRequestContext()
347+
if (requestContext) {
348+
requestContext.trackBackgroundWork(revalidateTagPromise)
349+
}
350+
351+
return revalidateTagPromise
352+
}
353+
354+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
355+
private async doRevalidateTag(tagOrTags: string | string[], ...args: any) {
342356
getLogger().withFields({ tagOrTags, args }).debug('NetlifyCacheHandler.revalidateTag')
343357

344358
const tags = Array.isArray(tagOrTags) ? tagOrTags : [tagOrTags]
@@ -357,7 +371,7 @@ export class NetlifyCacheHandler implements CacheHandler {
357371
}),
358372
)
359373

360-
purgeCache({ tags }).catch((error) => {
374+
await purgeCache({ tags }).catch((error) => {
361375
// TODO: add reporting here
362376
getLogger()
363377
.withError(error)

‎src/run/revalidate.ts‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,34 @@
11
import type { ServerResponse } from 'node:http'
2+
import { isPromise } from 'node:util/types'
3+
4+
import type { NextApiResponse } from 'next'
25

36
import type { RequestContext } from './handlers/request-context.cjs'
47

8+
type ResRevalidateMethod = NextApiResponse['revalidate']
9+
10+
function isRevalidateMethod(
11+
key: string,
12+
nextResponseField: unknown,
13+
): nextResponseField is ResRevalidateMethod {
14+
return key === 'revalidate' && typeof nextResponseField === 'function'
15+
}
16+
517
// Needing to proxy the response object to intercept the revalidate call for on-demand revalidation on page routes
618
export const nextResponseProxy = (res: ServerResponse, requestContext: RequestContext) => {
719
return new Proxy(res, {
8-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
9-
get(target: any[string], key: string) {
10-
const originalValue = target[key]
11-
if (key === 'revalidate') {
12-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
13-
return async function newRevalidate(...args: any[]) {
20+
get(target: ServerResponse, key: string) {
21+
const originalValue = Reflect.get(target, key)
22+
if (isRevalidateMethod(key, originalValue)) {
23+
return function newRevalidate(...args: Parameters<ResRevalidateMethod>) {
1424
requestContext.didPagesRouterOnDemandRevalidate = true
15-
return originalValue?.apply(target, args)
25+
26+
const result = originalValue.apply(target, args)
27+
if (result && isPromise(result)) {
28+
requestContext.trackBackgroundWork(result)
29+
}
30+
31+
return result
1632
}
1733
}
1834
return originalValue

‎tests/e2e/page-router.test.ts‎

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -50,25 +50,35 @@ export async function check(
5050

5151
test.describe('Simple Page Router (no basePath, no i18n)', () => {
5252
test.describe('On-demand revalidate works correctly', () => {
53-
for (const { label, prerendered, pagePath, expectedH1Content } of [
53+
for (const { label, prerendered, pagePath, revalidateApiBasePath, expectedH1Content } of [
5454
{
55-
label: 'prerendered page with static path',
55+
label: 'prerendered page with static path and awaited res.revalidate()',
5656
prerendered: true,
5757
pagePath: '/static/revalidate-manual',
58+
revalidateApiBasePath: '/api/revalidate',
5859
expectedH1Content: 'Show #71',
5960
},
6061
{
61-
label: 'prerendered page with dynamic path',
62+
label: 'prerendered page with dynamic path and awaited res.revalidate()',
6263
prerendered: true,
6364
pagePath: '/products/prerendered',
65+
revalidateApiBasePath: '/api/revalidate',
6466
expectedH1Content: 'Product prerendered',
6567
},
6668
{
67-
label: 'not prerendered page with dynamic path',
69+
label: 'not prerendered page with dynamic path and awaited res.revalidate()',
6870
prerendered: false,
6971
pagePath: '/products/not-prerendered',
72+
revalidateApiBasePath: '/api/revalidate',
7073
expectedH1Content: 'Product not-prerendered',
7174
},
75+
{
76+
label: 'not prerendered page with dynamic path and not awaited res.revalidate()',
77+
prerendered: false,
78+
pagePath: '/products/not-prerendered-and-not-awaited-revalidation',
79+
revalidateApiBasePath: '/api/revalidate-no-await',
80+
expectedH1Content: 'Product not-prerendered-and-not-awaited-revalidation',
81+
},
7282
]) {
7383
test(label, async ({ page, pollUntilHeadersMatch, pageRouter }) => {
7484
// in case there is retry or some other test did hit that path before
@@ -192,7 +202,7 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
192202
expect(data2?.pageProps?.time).toBe(date1)
193203

194204
const revalidate = await page.goto(
195-
new URL(`/api/revalidate?path=${pagePath}`, pageRouter.url).href,
205+
new URL(`${revalidateApiBasePath}?path=${pagePath}`, pageRouter.url).href,
196206
)
197207
expect(revalidate?.status()).toBe(200)
198208

@@ -411,25 +421,35 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
411421

412422
test.describe('Page Router with basePath and i18n', () => {
413423
test.describe('Static revalidate works correctly', () => {
414-
for (const { label, prerendered, pagePath, expectedH1Content } of [
424+
for (const { label, prerendered, pagePath, revalidateApiBasePath, expectedH1Content } of [
415425
{
416-
label: 'prerendered page with static path',
426+
label: 'prerendered page with static path and awaited res.revalidate()',
417427
prerendered: true,
418428
pagePath: '/static/revalidate-manual',
429+
revalidateApiBasePath: '/api/revalidate',
419430
expectedH1Content: 'Show #71',
420431
},
421432
{
422-
label: 'prerendered page with dynamic path',
433+
label: 'prerendered page with dynamic path and awaited res.revalidate()',
423434
prerendered: true,
424435
pagePath: '/products/prerendered',
436+
revalidateApiBasePath: '/api/revalidate',
425437
expectedH1Content: 'Product prerendered',
426438
},
427439
{
428-
label: 'not prerendered page with dynamic path',
440+
label: 'not prerendered page with dynamic path and awaited res.revalidate()',
429441
prerendered: false,
430442
pagePath: '/products/not-prerendered',
443+
revalidateApiBasePath: '/api/revalidate',
431444
expectedH1Content: 'Product not-prerendered',
432445
},
446+
{
447+
label: 'not prerendered page with dynamic path and not awaited res.revalidate()',
448+
prerendered: false,
449+
pagePath: '/products/not-prerendered-and-not-awaited-revalidation',
450+
revalidateApiBasePath: '/api/revalidate-no-await',
451+
expectedH1Content: 'Product not-prerendered-and-not-awaited-revalidation',
452+
},
433453
]) {
434454
test.describe(label, () => {
435455
test(`default locale`, async ({ page, pollUntilHeadersMatch, pageRouterBasePathI18n }) => {
@@ -622,7 +642,10 @@ test.describe('Page Router with basePath and i18n', () => {
622642

623643
// revalidate implicit locale path
624644
const revalidateImplicit = await page.goto(
625-
new URL(`/base/path/api/revalidate?path=${pagePath}`, pageRouterBasePathI18n.url).href,
645+
new URL(
646+
`/base/path${revalidateApiBasePath}?path=${pagePath}`,
647+
pageRouterBasePathI18n.url,
648+
).href,
626649
)
627650
expect(revalidateImplicit?.status()).toBe(200)
628651

@@ -713,8 +736,10 @@ test.describe('Page Router with basePath and i18n', () => {
713736

714737
// revalidate implicit locale path
715738
const revalidateExplicit = await page.goto(
716-
new URL(`/base/path/api/revalidate?path=/en${pagePath}`, pageRouterBasePathI18n.url)
717-
.href,
739+
new URL(
740+
`/base/path${revalidateApiBasePath}?path=/en${pagePath}`,
741+
pageRouterBasePathI18n.url,
742+
).href,
718743
)
719744
expect(revalidateExplicit?.status()).toBe(200)
720745

@@ -934,8 +959,10 @@ test.describe('Page Router with basePath and i18n', () => {
934959
expect(data2?.pageProps?.time).toBe(date1)
935960

936961
const revalidate = await page.goto(
937-
new URL(`/base/path/api/revalidate?path=/de${pagePath}`, pageRouterBasePathI18n.url)
938-
.href,
962+
new URL(
963+
`/base/path${revalidateApiBasePath}?path=/de${pagePath}`,
964+
pageRouterBasePathI18n.url,
965+
).href,
939966
)
940967
expect(revalidate?.status()).toBe(200)
941968

‎tests/fixtures/page-router-base-path-i18n/pages/api/revalidate-no-await.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
export default async function handler(req, res) {
2+
try {
3+
const pathToPurge = req.query.path ?? '/static/revalidate-manual'
4+
// res.revalidate returns a promise that can be awaited to wait for the revalidation to complete
5+
// if user doesn't await it, we still want to ensure the revalidation is completed, so we internally track
6+
// this as "background work" to ensure it completes before function suspends execution
7+
res.revalidate(pathToPurge)
8+
return res.json({ code: 200, message: 'success' })
9+
} catch (err) {
10+
return res.status(500).send({ code: 500, message: err.message })
11+
}
12+
}

‎tests/fixtures/page-router/pages/api/revalidate-no-await.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
export default async function handler(req, res) {
2+
try {
3+
const pathToPurge = req.query.path ?? '/static/revalidate-manual'
4+
// res.revalidate returns a promise that can be awaited to wait for the revalidation to complete
5+
// if user doesn't await it, we still want to ensure the revalidation is completed, so we internally track
6+
// this as "background work" to ensure it completes before function suspends execution
7+
res.revalidate(pathToPurge)
8+
return res.json({ code: 200, message: 'success' })
9+
} catch (err) {
10+
return res.status(500).send({ code: 500, message: err.message })
11+
}
12+
}

‎tests/utils/fixture.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -364,14 +364,24 @@ export async function invokeFunction(
364364
NETLIFY_BLOBS_CONTEXT: createBlobContext(ctx),
365365
...(env || {}),
366366
}
367+
368+
const envVarsToRestore = {}
369+
370+
// We are not using lambda-local's environment variable setting because it cleans up
371+
// environment vars to early (before stream is closed)
372+
Object.keys(environment).forEach(function (key) {
373+
if (typeof process.env[key] !== 'undefined') {
374+
envVarsToRestore[key] = process.env[key]
375+
}
376+
process.env[key] = environment[key]
377+
})
378+
367379
const response = (await execute({
368380
event: {
369381
headers: headers || {},
370382
httpMethod: httpMethod || 'GET',
371383
rawUrl: new URL(url || '/', 'https://example.netlify').href,
372384
},
373-
environment,
374-
envdestroy: true,
375385
lambdaFunc: { handler },
376386
timeoutMs: 4_000,
377387
})) as LambdaResponse
@@ -386,6 +396,14 @@ export async function invokeFunction(
386396

387397
const bodyBuffer = await streamToBuffer(response.body)
388398

399+
Object.keys(environment).forEach(function (key) {
400+
if (typeof envVarsToRestore[key] !== 'undefined') {
401+
process.env[key] = envVarsToRestore[key]
402+
} else {
403+
delete process.env[key]
404+
}
405+
})
406+
389407
return {
390408
statusCode: response.statusCode,
391409
bodyBuffer,

‎tests/utils/sandbox-child.mjs‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,23 @@ process.on('message', async (msg) => {
4848
...(env || {}),
4949
}
5050

51+
const envVarsToRestore = {}
52+
53+
// We are not using lambda-local's environment variable setting because it cleans up
54+
// environment vars to early (before stream is closed)
55+
Object.keys(environment).forEach(function (key) {
56+
if (typeof process.env[key] !== 'undefined') {
57+
envVarsToRestore[key] = process.env[key]
58+
}
59+
process.env[key] = environment[key]
60+
})
61+
5162
const response = await execute({
5263
event: {
5364
headers: headers || {},
5465
httpMethod: httpMethod || 'GET',
5566
rawUrl: new URL(url || '/', 'https://example.netlify').href,
5667
},
57-
environment,
58-
envdestroy: true,
5968
lambdaFunc: { handler },
6069
timeoutMs: 4_000,
6170
})
@@ -70,6 +79,14 @@ process.on('message', async (msg) => {
7079

7180
const bodyBuffer = await streamToBuffer(response.body)
7281

82+
Object.keys(environment).forEach(function (key) {
83+
if (typeof envVarsToRestore[key] !== 'undefined') {
84+
process.env[key] = envVarsToRestore[key]
85+
} else {
86+
delete process.env[key]
87+
}
88+
})
89+
7390
const result = {
7491
statusCode: response.statusCode,
7592
bodyBuffer,

0 commit comments

Comments
 (0)