fix(caching): Eagerly cleanup old versions

[loewenheim]

Currently, when we recompute a cache entry for which a fallback version exists, we just add the new version and leave the fallback in place. The idea was that the fallback version would fall out of use and eventually be cleaned up by the cleanup task.

While this is true, "eventually" is doing a lot of work here. In the immediate term, it's massively wasteful. Therefore, we now delete the fallback versions in spawn_refresh immediately after the new cache entry has been computed.

Fixes #1577.

ETA: Changed this to always deleting old versions on compute, not just on lazy recomputes. That way it also covers incompatible cache version bumps.

[Dav1dde]

What happens if there is an old cache file version which is not part of the fallbacks?

[jjbayer]

Awesome!

[loewenheim]

What happens if there is an old cache file version which is not part of the fallbacks?

The same thing that currently happens, it would eventually be cleaned up by the cleanup job. Might be worth it to immediately clean up such files as well. I'll think about how that could be done.

[loewenheim]

@jjbayer @Swatinem @Dav1dde I moved the deletion of old files into compute immediately after the new cache file is persisted. Also, it now iterates over all lower version numbers and attempts to delete them. This means that now all old versions get cleaned up, fallback or no. I also updated the test to confirm this.

[Dav1dde]

Nice!

[loewenheim]

It was definitely a good catch; for incompatible cache version bumps the original version of this change would've made no difference.

[Swatinem]

I believe we intentionally did not clean these up, just in case one wants to revert a deploy.

[jjbayer]

I believe we intentionally did not clean these up, just in case one wants to revert a deploy.

Since the refresh process is gradual & throttled, what's the worst case behavior after a revert? The old files would still be in the shared cache, right?

[loewenheim]

I believe we intentionally did not clean these up, just in case one wants to revert a deploy.

Ah. Hm. That's also a fair point. It applies equally regardless of whether we're doing a compatible or incompatible bump, though.

This seems like a genuine dilemma to me—do we want to keep old caches around for rollbacks or delete theme eagerly to limit storage consumption during updates?

[Dav1dde]

This seems like a genuine dilemma to me—do we want to keep old caches around for rollbacks or delete theme eagerly to limit storage consumption during updates?

The upgrade case is gonna happen more often than the rollback case, in a normal update we already have to deal with missing caches, so that's no different from (down-)grading imo.

[jjbayer]

This seems like a genuine dilemma to me—do we want to keep old caches around for rollbacks or delete theme eagerly to limit storage consumption during updates?

A middle ground could be to give the old files a shorter TTL, e.g. 1 hour.

To me running out of disk space seems like a worse failure mode than having a cold(er) cache on rollback, but I'm biased.