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.

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

fix: track revalidate / cdn purge to ensure it finishes execution and is not suspended mid-execution #2490

Merged
merged 12 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/run/handlers/cache.cts
Original file line number Diff line number Diff line change
Expand Up @@ -283,19 +283,33 @@ export class NetlifyCacheHandler implements CacheHandler {
if (requestContext?.didPagesRouterOnDemandRevalidate) {
const tag = `_N_T_${key === '/index' ? '/' : key}`
getLogger().debug(`Purging CDN cache for: [${tag}]`)
purgeCache({ tags: [tag] }).catch((error) => {
// TODO: add reporting here
getLogger()
.withError(error)
.error(`[NetlifyCacheHandler]: Purging the cache for tag ${tag} failed`)
})
requestContext.trackBackgroundWork(
purgeCache({ tags: [tag] }).catch((error) => {
// TODO: add reporting here
getLogger()
.withError(error)
.error(`[NetlifyCacheHandler]: Purging the cache for tag ${tag} failed`)
}),
)
Comment on lines +329 to +336
Copy link
Contributor Author

Choose a reason for hiding this comment

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

making sure we track purgeCache part of the pages router on-demand revalidation so it completes before function execution gets suspended

}
}
})
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
async revalidateTag(tagOrTags: string | string[], ...args: any) {
const revalidateTagPromise = this.doRevalidateTag(tagOrTags, ...args)

const requestContext = getRequestContext()
if (requestContext) {
requestContext.trackBackgroundWork(revalidateTagPromise)
}

return revalidateTagPromise
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
private async doRevalidateTag(tagOrTags: string | string[], ...args: any) {
Comment on lines 343 to +355
Copy link
Contributor Author

Choose a reason for hiding this comment

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

App Router revalidateTag and revalidatePath don't return a promise so user can't even await on the revalidation work in their code. Instead we will track it internally (should not affect timing of the response, just that we should keep execution running and not suspend before this is completed)

getLogger().withFields({ tagOrTags, args }).debug('NetlifyCacheHandler.revalidateTag')

const tags = Array.isArray(tagOrTags) ? tagOrTags : [tagOrTags]
Expand All @@ -314,7 +328,7 @@ export class NetlifyCacheHandler implements CacheHandler {
}),
)

purgeCache({ tags }).catch((error) => {
await purgeCache({ tags }).catch((error) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't change much, because .revalidateTag overall was not awaited on and all of this is ~background work that won't block response

// TODO: add reporting here
getLogger()
.withError(error)
Expand Down
30 changes: 23 additions & 7 deletions src/run/revalidate.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,34 @@
import type { ServerResponse } from 'node:http'
import { isPromise } from 'node:util/types'
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL


import type { NextApiResponse } from 'next'

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

type ResRevalidateMethod = NextApiResponse['revalidate']

function isRevalidateMethod(
key: string,
nextResponseField: unknown,
): nextResponseField is ResRevalidateMethod {
return key === 'revalidate' && typeof nextResponseField === 'function'
}

// Needing to proxy the response object to intercept the revalidate call for on-demand revalidation on page routes
export const nextResponseProxy = (res: ServerResponse, requestContext: RequestContext) => {
return new Proxy(res, {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
get(target: any[string], key: string) {
const originalValue = target[key]
if (key === 'revalidate') {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return async function newRevalidate(...args: any[]) {
get(target: ServerResponse, key: string) {
const originalValue = Reflect.get(target, key)
if (isRevalidateMethod(key, originalValue)) {
return function newRevalidate(...args: Parameters<ResRevalidateMethod>) {
requestContext.didPagesRouterOnDemandRevalidate = true
return originalValue?.apply(target, args)

const result = originalValue.apply(target, args)
if (result && isPromise(result)) {
requestContext.trackBackgroundWork(result)
}

return result
}
}
return originalValue
Expand Down
5 changes: 4 additions & 1 deletion tests/fixtures/page-router/pages/api/revalidate.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
export default async function handler(req, res) {
try {
await res.revalidate('/static/revalidate-manual')
// res.revalidate returns a promise that can be awaited to wait for the revalidation to complete
// if user doesn't await it, we still want to ensure the revalidation is completed, so we internally track
// this as "background work" to ensure it completes before function suspends execution
res.revalidate('/static/revalidate-manual')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might make our page router on-demand revalidate tests flaky, as we do use to trigger on-demand revalidation, then sleep for a bit and then check revalidated page, but because we don't await here, the response from revalidate api endpoint would happen before revalidation completed (at least Next.js side of it)

on the other hand it would be good to verify that background tracking actually works so I would prefer to have this and potentially increase sleep timeouts in tests (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would be better to explicitly not await this call in a specific fixture/test for this behavior, and await it in all other tests. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to have 2 cases here so in case those e2e tests fail it's easier to tell wether there is overall problem with revalidation or specifically with not awaited revalidation. Adding variants here would be much smoother after #2495 is in as it does add ~test.each kind of setup to on-demand revalidation tests where it's super easy to add another case, so I'll get this one in first and then revert this change and instead add another API endpoint that doesn't await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dd43ce8 added separate case for not awaiting res.revalidate on top of existing ones

return res.json({ code: 200, message: 'success' })
} catch (err) {
return res.status(500).send({ code: 500, message: err.message })
Expand Down
22 changes: 20 additions & 2 deletions tests/utils/fixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,14 +364,24 @@ export async function invokeFunction(
NETLIFY_BLOBS_CONTEXT: createBlobContext(ctx),
...(env || {}),
}

const envVarsToRestore = {}

// We are not using lambda-local's environment variable setting because it cleans up
// environment vars to early (before stream is closed)
Object.keys(environment).forEach(function (key) {
if (typeof process.env[key] !== 'undefined') {
envVarsToRestore[key] = process.env[key]
}
process.env[key] = environment[key]
})

const response = (await execute({
event: {
headers: headers || {},
httpMethod: httpMethod || 'GET',
rawUrl: new URL(url || '/', 'https://example.netlify').href,
},
environment,
envdestroy: true,
lambdaFunc: { handler },
timeoutMs: 4_000,
})) as LambdaResponse
Expand All @@ -386,6 +396,14 @@ export async function invokeFunction(

const bodyBuffer = await streamToBuffer(response.body)

Object.keys(environment).forEach(function (key) {
if (typeof envVarsToRestore[key] !== 'undefined') {
process.env[key] = envVarsToRestore[key]
} else {
delete process.env[key]
}
})

return {
statusCode: response.statusCode,
bodyBuffer,
Expand Down
21 changes: 19 additions & 2 deletions tests/utils/sandbox-child.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,23 @@ process.on('message', async (msg) => {
...(env || {}),
}

const envVarsToRestore = {}

// We are not using lambda-local's environment variable setting because it cleans up
// environment vars to early (before stream is closed)
Object.keys(environment).forEach(function (key) {
if (typeof process.env[key] !== 'undefined') {
envVarsToRestore[key] = process.env[key]
}
process.env[key] = environment[key]
})

const response = await execute({
event: {
headers: headers || {},
httpMethod: httpMethod || 'GET',
rawUrl: new URL(url || '/', 'https://example.netlify').href,
},
environment,
envdestroy: true,
lambdaFunc: { handler },
timeoutMs: 4_000,
})
Expand All @@ -70,6 +79,14 @@ process.on('message', async (msg) => {

const bodyBuffer = await streamToBuffer(response.body)

Object.keys(environment).forEach(function (key) {
if (typeof envVarsToRestore[key] !== 'undefined') {
process.env[key] = envVarsToRestore[key]
} else {
delete process.env[key]
}
})

const result = {
statusCode: response.statusCode,
bodyBuffer,
Expand Down
Loading