feat: retry invalid/expired refs

[angeloashmore]

Resolves: #333

Description

This PR adds a retry mechanism when a RefNotFoundError or RefExpiredError is thrown in a query method. The client will retry a maximum of 3 times before throwing.

A warning is logged when retrying:

"The ref (xxx) was expired. Now retrying with the latest master ref (yyy). If you were previewing content, the response will not include draft content."

This PR also clears the /api/v2 value cached for 5s when we know a stale value is saved. This prevents a small edge case where calls within 5s would have all failed if the ref was invalid or expired.

Checklist

• If my changes require tests, I added them.
• If my changes affect backward compatibility, it has been discussed.
• If my changes require an update to the CONTRIBUTING.md guide, I updated it.

Preview

There are no changes to existing code. Queries appear as before.

How to QA ¹

See the tests.

Footnotes

1. Please use these labels when submitting a review:
   ❓ #ask: Ask a question.
   💡 #idea: Suggest an idea.
   ⚠️ #issue: Strongly suggest a change.
   🎉 #nice: Share a compliment. ↩

[lihbr]

⚠️ #issue: I'm afraid this could create some concurrency issues when we dedupe requests:

• T0
  • Page Foo fetches nav document
  • Page Bar fetches nav document
• T1
  • Requests get deduped (only one is made) and fail at the same time with the same error
• T2
  • Page Foo retries the request with the latest ref
  • Page Bar cannot retries the request with the latest ref because it's already being retried by Foo
  • Request cannot be deduped

...or when multiple requests (e.g. Promise.all(...)) fail at the same time, which I believe is symptomatic of this kind of error(?)

Maybe we should just retry directly instead of recursively calling the function after that?

- return await this.get({ ...params, ref: latestRef })
+ const url = await this.buildQueryURL({ ...params, ref: latestRef })

+ return await this.fetch<Query<TDocument>>(url, { ...params, ref: latestRef })

Or base the retry off whether or not we had an explicit ref parameter(?)

---

💡 #idea: Maybe we should clear the repository cache when this happens so that other requests (during the cache's 5s lifespan) can fetch a fresh ref?

[angeloashmore]

You're right; it would likely cause some issues when queries are deduplicated. I removed the retriedRefs array and check and simply trust that the provided refs won't cause a loop.

Here's the comment I sent to you in Slack:

I removed the logic that prevents the same ref from being retried more than once. You are right about concurrency issues since the retry logic is in the query method, not in the deduplicated fetch job. I'd like to keep the retry in the method so it is close to its intended usage.

I'm wondering if we should still include some kind of loop prevention. I refactored the implementation you reviewed yesterday such that we now have a private _get() method that preforms the retry. Since we have that now, it's easy for us to track how many retries and halt if we get too deep.

I think we should include some kind of loop protection to be safe, but I wanted to get your thoughts first.

---

Maybe we should just retry directly instead of recursively calling the function after that?

I'd rather not retry directly since it means we can only follow the API's response once. If a website has a very stale cache with multiple chained invalid ref responses, we should be able to use each of them.

We log a warning each time we retry, so devs will still be nudged to fix their caching issue.

---

Or base the retry off whether or not we had an explicit ref parameter(?)

A retry may be necessary whether or not an explicit ref is provided. Websites may have a stale /api/v2, which would result in a stale master ref being used implicitly.

---

💡 #idea: Maybe we should clear the repository cache when this happens so that other requests (during the cache's 5s lifespan) can fetch a fresh ref?

Maybe! This fix is really intended for Next.js' indefinite cache, which works outside the client's internal 5s cache.

I can't think of any downside to clearing the internal cache. It would resolve the tiny chance that a burst of requests from the same client results uses a stale, internally cached ref. If you can't think of any downsides, I'll add it in.

[lihbr]

Thanks for the clarification!

• I agree we should have some kind of loop prevention yes ♻️
• I'm aligned with clearing the cache, it's mostly free and will clear some edge cases 🧹

[lihbr]

LGTM!

Just some kind of loop prevention as discussed would be great, maybe through a triedRefs argument on _get()

And repository cache purging also ☺️

[angeloashmore]

@lihbr I made the following changes:

• Retries are limited to 3 attempts.
• The cached repository value is cleared when we know a stale /api/v2 was used.
• Added tests for those cases.

You can see the changes here: https://github.com/prismicio/prismic-client/pull/356/files/d10ebe6d8fe3f5d5461e3e565d8d4c93370e60ba..f3ae551cd9da9dee493971db58c8c4f003f8ee87?diff=split&w=1

I'll merge once you have a chance to re-review. 🙂

[lihbr]

LGTM, thanks ☺️

[raRaRa]

It would be AWESOME if this could also bypass the cache in the retry if the ref is expired. It has caused so much pain in my NextJS projects where websites suddenly start to "crash" due to this issue.

I was deploying a new version of a site and all of the pages started crashing cause Vercel stores cached data between deployments. I had to delete all the cache manually.