Stop updating client cache when revalidated and still expired

[mairatma]

What is the purpose of this pull request?

This is removing a bunch of unnecessary disk writes, which just waste CPU.

What problem is this solving?

When a cached response is expired, we try sending a request using the cached etag to revalidate it. If this revalidation is successful, we use the cache contents. In that case, we also update the cache again with the contents it already has. That's just a waste of CPU.

This is only ever useful if the cache max age has increased, to avoid further revalidations, since we also write the new max age to the disk. But at least for pages-graphql calls it's very frequent for the content to still be expired at this point.

This PR is making sure that we only ever update the cache if either the content has changed (meaning that we didn't use the cached contents) or the expiration time has changed.

This should free a lot of disk writes from pages-graphql (at least ~60% of disk writes for VBase calls, plus others in the same situation).

How should this be manually tested?

I've tested this by vtex linking pages-graphql with @vtex/api locally linked, and adding console.log to show when we skip writing to cache. The skips were done at the right time (after revalidating VBase requests, which are always expired).

I've also tested by looking at Honeycomb traces. On production you'll always see the local-cache-saved event for requests to VBase, even for STALE requests. Now you'll only ever see this event for MISS requests to VBase (example trace with STALE, example trace with MISS).

Screenshots or example usage

Types of changes

• [x] Bug fix (a non-breaking change which fixes an issue)
• [ ] New feature (a non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Requires change to documentation, which has been updated accordingly.

[mairatma]

For VBase requests, this implementation makes the remove of await from storage.set unnecessary. Should we merge the changes on PR and test it all together? We are trying to create a test env to evaluate the changes before going live with it. WDYT?

Do you mean this test environment? I like the idea of having that but it seems unnecessary for this change, since I was able to test it very well by just linking, with console.log + tracing. What's the main purpose of using this new test environment? What kind of changes would require using it?

As for merging with the PR for running the saves on the background, I feel like it's better to deploy them separately. This one is ready and it has some extra logic related to checking expiration. It would already help VBase's case, like you said. It would be better to observe it on its own before deploying writes on the background, since those will affect other clients besides VBase. These 2 PRs affect the apps in different ways, so the things we need to test for each are different as well.

[filafb]

Not necessary that one, but any test env that allow us a close to production test. The main idea is to avoid having side effects we cannot map testing only linking the projects and having more data to back up the changes. But since we will be moving this change by itself, I think we are safe.