Stop showing stale cache snapshot on same-page link click

[compumike]

Ran into a similar problem as #780 with a stale cache entry showing when using Turbo Drive and clicking a link to the current page. I've identified and fixed the stale cache bug, and added a new test to prevent regression. This new test fails on main and passes on my PR branch.

Here's a simple example with a page that shows the current timestamp, and has a link to itself:

Before:
before.webm
Notice how on the 2nd-and-beyond click (red flash), Turbo briefly previews an old cache entry, and the timestamp rolls backwards briefly! 😮

Expectation: the timestamp should never roll backwards.

After:
after.webm
The timestamp increases monotonically without rolling back.

Origin of caching bug:

Visit#loadCachedSnapshot had a logic bug because it would snapshot = getCachedSnapshot() before it called this.cacheSnapshot() to preserve the old page. This is fine if you're clicking a link from one page to different page because they operate on different cache keys. However, if you're clicking a link to the current page itself, then the old and new pages have the same cache key (equivalent to the same request URL). The incorrect get-before-set results in getting an older version of the page than is currently being shown!

To fix the bug, there are a few options. One would be to reverse the order of cache operations, and synchronously save a new snapshot to the cache before loading it right back and showing it as a preview. Alternatively, one can simply skip the rendering of the preview in this case. The if (this.isSamePage) was attempting to do something like that, but I'm guessing the behavior of isSamePage conditions changed in a way that no longer makes sense to use it to see if there's this stale cache preview issue. So I've added an isSameCacheKey and use that to determine whether to skip rendering the cached snapshot.

I've also added a page http://localhost:9000/__turbo/timestamp so you can easily test it out.

[domchristie]

The if (this.isSamePage) was attempting to do something like that

isSamePage checks if the visit is a same-page anchor visit. This is a special case so parts of the visit lifecycle are skipped (e.g. this.view.renderPage(…) and this.complete()). I don't think we want to skip these operations when revisiting the same page.

To fix the bug, there are a few options. One would be to reverse the order of cache operations, and synchronously save a new snapshot to the cache before loading it right back and showing it as a preview.

This might be worth exploring although I think there might be some history re. the order of caching/rendering so it might be fragile.

What if getCachedSnapshot didn't return a snapshot if it's a revisit to the same location?

[compumike]

Hi @domchristie I really appreciate the feedback as I've been using Turbo for a while but am fairly new to the internals!

If getCachedSnapshot returned null when it's a click to the same location, that seems fine to me, but it seems that it would similarly skip this.view.renderPage and this.complete which you expressed some concerned about skipping.

Could you help me understand an example or use case where the current solution in this PR produces the incorrect result?

[domchristie]

Could you help me understand an example or use case where the current solution in this PR produces the incorrect result?

Hey @compumike, it may work fine :) But I suppose it's a question of which method should be responsible for performing the check, and due to the complexity of checking of same-page anchor visits, I'm not 100% confident that changing the condition here will work in all cases

[compumike]

Yes I understand, I came across those locationWithActionIsSamePage while I was investigating this issue.

That's why I simplified this check it in terms of being just a pure cache logic bug if the cache key is the same, where it is will obviously show stale data if you read from a cache key just before you write to the same cache key.

[domchristie]

I could be wrong, and it might not matter, but I don't think that check was there to fix a caching issue; I think it's purpose was just to bypass the rendering process specifically for same-page anchor visits, as they are special cases.

where it is will obviously show stale data if you read from a cache key just before you write to the same cache key.

In this case, does it make sense to have a snapshot if it's not valid? and is it necessary to go through the Visit#render flow before chhecking? If not, then it might be preferable to move this check to getCachedSnapshot

[brunoprietog]

With the new page refreshes, there is a similar problem with cache when morphing. In #1098 I'm disabling preview rendering if it's a page refresh and if the render method is morph. Seeing this PR, I think we could take advantage of the same and disable the preview rendering on any page refresh, independent of the method. Could you test if that solves this problem too please?

[brunoprietog]

The caveat of #1098 is that it is only for page refreshes, which means that it only skips the preview rendering if the page is the same and if the action is replace.