fix: track revalidate / cdn purge to ensure it finishes execution and is not suspended mid-execution #2490
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… is not suspended mid-execution
pieh
commented
Jun 17, 2024
Comment on lines
+247
to
+254
| requestContext.trackBackgroundWork( | ||
| purgeCache({ tags: [tag] }).catch((error) => { | ||
| // TODO: add reporting here | ||
| getLogger() | ||
| .withError(error) | ||
| .error(`[NetlifyCacheHandler]: Purging the cache for tag ${tag} failed`) | ||
| }), | ||
| ) |
There was a problem hiding this comment.
making sure we track purgeCache part of the pages router on-demand revalidation so it completes before function execution gets suspended
📊 Package size report 0.01%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
commented
Jun 17, 2024
Comment on lines
261
to
+273
| 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) { |
There was a problem hiding this comment.
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)
pieh
commented
Jun 17, 2024
| @@ -275,7 +289,7 @@ export class NetlifyCacheHandler implements CacheHandler { | |||
| }), | |||
| ) | |||
|
|
|||
| purgeCache({ tags }).catch((error) => { | |||
| await purgeCache({ tags }).catch((error) => { | |||
There was a problem hiding this comment.
this doesn't change much, because .revalidateTag overall was not awaited on and all of this is ~background work that won't block response
pieh
commented
Jun 17, 2024
src/run/revalidate.ts
Outdated
Comment on lines
17
to
22
| const result = originalValue?.apply(target, args) | ||
| if (result && isPromise(result)) { | ||
| requestContext.trackBackgroundWork(result) | ||
| } | ||
|
|
||
| return result |
There was a problem hiding this comment.
User's CAN await pages router res.revalidate(), so if user is doing that, this is not changing anything, but it's quite easy to miss await in user code (especially that the promise would resolve to void anyway, so user would not be doing anything with res.revalidate() return value (other than hopefully await it).
This just make sure that if user doesn't await it, we will (not blocking a response, just tracking this as background work)
pieh
commented
Jun 17, 2024
| // 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') |
There was a problem hiding this comment.
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 (?)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
dd43ce8 added separate case for not awaiting res.revalidate on top of existing ones
pieh
added
the
test all versions
pieh
force-pushed
the
fix/track-background-revalidation
branch
from
6d66d8f to
bd9771b
Compare
serhalp
previously approved these changes
Jun 21, 2024
| // 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') |
There was a problem hiding this comment.
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?
src/run/revalidate.ts
Outdated
Comment on lines
13
to
14
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return async function newRevalidate(...args: any[]) { |
There was a problem hiding this comment.
Do you know why we're using all these dangerous anys here? We should be able to reference the type of the original function here instead, shouldn't we?
There was a problem hiding this comment.
Not sure why, but at least with code as is we ourselves don't use args directly and only forward them to actual res.revalidate method so in this case (at least this specific line, as there are some other any in this module) we don't necessarily care about types, but there is no reason not to add proper types here I think. I'll take a look and see if I can get rid of all those any in this module.
There was a problem hiding this comment.
b475810 got rid of any usage. Added type guard there is pretty rudimentary, but I didn't really see a way to make it better/narrower (it only check key - which we already were doing + started checking if value is at least a function)
| @@ -1,4 +1,5 @@ | |||
| import type { ServerResponse } from 'node:http' | |||
| import { isPromise } from 'node:util/types' | |||
serhalp
previously approved these changes
Jun 24, 2024
orinokai
approved these changes
Jun 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
With the changes some time ago to streaming functions to not await untracked background work we might have not handled all cases of background work we should be tracking. We do track background regeneration (~SWR), but on-demand revalidation code paths were not tracked possibly resulting in flaky on-demand revalidate behaviors
Documentation
Tests
Not clear how to exactly test this without adding some TEST specific timeous to runtime, if anyone has ideas on what kind of tests could assert that we indeed are awaiting/tracking relevant on-demand revalidation work, happy to add or modify tests.
We do have tests for on-demand revalidation which are a bit flaky. Maybe this will un-flake them (or at least decrease flakiness)?
Relevant links (GitHub issues, etc.) or a picture of cute animal
https://linear.app/netlify/issue/FRA-566/nextjs-routes-revalidating-when-not-required#comment-1bf42469 (+ my next comment)
// notes:
my hunch in here it looks like Next cache is being correctly invalidated, but then CDN purge is not going through resulting in CDN nodes still serving whatever it had cached prior to purge - optionally CDN nodes that didn't have response cached would serve fresh leading to undeterministic response (depending on CDN node that handled request)
https://linear.app/netlify/issue/FRA-568/help-understanding-cache-tag-usage#comment-169cc400
// notes:
App Router
revalidateTag(orrevalidatePathfor that matter) don't return promises that user can await on, so execution can be suspended without going through <- I'm not exactly the most confident that changes here will fully resolve what this comment talks about, but from my own testing those changes seem required