Cache the version_id for download requests

[jtgeibel]

This adds a caching layer in the app backend to reduce the number of
database queries. While this query is extremely fast, a massive number
of concurrent download requests can still result in a request backlog.

In this implementation cached values are never dropped. This means
that memory usage will grow over time, however the memory usage is
expected to be well within our instance size and the application is
automatically rebooted at least once every 24 hours. Additionally, on
the rare occasion that a crate version is deleted, the cache will
continue to serve redirects and attempt to record download counts for
that version.

If a version is deleted and continues to see download requests, it is
expected that the entire shard of downloads containing that version_id
will fail to persist. Even if we expired cached entries after a certain
time period, there would still be a window where a deleted version
could cause download counts for other versions to not be persisted. It
is possible we could make the download counter logic robust against this
scenario, but given that deleting versions is rare we can mitigate this
by restarting the app after deleting crates or versions.

r? @pietroalbini

[pietroalbini]

We can support serving responses for non-canonical spellings with little effort:

struct CachedVersion {
    canonical_crate_name: String,
    version_id: i32,
}

version_id_cacher: DashMap<String, CachedVersion>

Then the cache key could be normalized with crate_name.lower().replace('_', '-'), while still having access to the canonical crate name to perform the redirect.

[jtgeibel]

I was originally storing the canonical crate name as well, but ended up dropping that to minimize memory usage, especially since there are so few non-canonical requests.

I'm not actually that worried about memory usage, so this approach does sound reasonable to me as well. Maybe it's worth looking at memory usage in production first, and then adding this if we're sure we have plenty of room to spare.

[pietroalbini]

DashMap::entry acquires a write lock, even if the entry is present in the map. This would cause every request to do a write lock on the assigned shard, which we should try to avoid. We should do something similar to DownloadsCounter, where we first call .get to try and use the read lock and only if it fails call .entry.

[jtgeibel]

Excellent point, I'll switch to .get.

[pietroalbini]

I did some experiments and this should use at most 10-20 MB with the current set of crates, so this should be fine for the foreseeable future for memory usage.

Regarding cache expiration, I'd prefer to actually have some cache expiration built in with a relatively short TTL (1-5 minutes?), to limit the effect of deleted crates. We could just add a fetched_at timestamp in the cache value, and if it's older than the TTL then ignore the cached value and fetch from the database again. This shouldn't make the code too more complex and shouldn't use too much memory either.

There should also eventually be some automatic cleanup in place to ensure the map doesn't grow too much, but that can be done in a later PR. Deleting a random shard whenever we reach more than N items in the cache should be enough, without adding LRU logic.

It would also be interesting to have a "cache hit" metric, especially if we implement some TTL.

[bors]

☔ The latest upstream changes (presumably #3858) made this pull request unmergeable. Please resolve the merge conflicts.

[Turbo87]

I just discovered https://github.com/moka-rs/moka, which sounds like it might be useful for this use case. Would it make sense to use that instead of building our own cache?

[Turbo87]

@jtgeibel FYI I've rebased the branch and extracted the test code refactoring into a second (or rather: first) commit

[bors]

☔ The latest upstream changes (presumably #3884) made this pull request unmergeable. Please resolve the merge conflicts.

[Turbo87]

rebased once more and resolved the conflict with #3884