feat!: Add Redis Cache store

[ElijahAhianyo]

This Pr builds on #399 and adds support for a Redis cache backend

[codecov]

Codecov Report

❌ Patch coverage is 91.51786% with 38 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cot/src/test.rs	85.29%	1 Missing and 19 partials ⚠️
cot/src/cache/store/redis.rs	92.67%	0 Missing and 17 partials ⚠️
cot/src/cache.rs	97.22%	0 Missing and 1 partial ⚠️

Flag	Coverage Δ
rust	90.66% <91.51%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cot-macros/src/cache.rs	100.00% <100.00%> (ø)
cot-macros/src/lib.rs	97.53% <100.00%> (+0.12%)	⬆️
cot/src/cache/store.rs	100.00% <ø> (ø)
cot/src/config.rs	96.57% <100.00%> (+0.03%)	⬆️
cot/src/cache.rs	84.06% <97.22%> (+0.51%)	⬆️
cot/src/cache/store/redis.rs	92.67% <92.67%> (ø)
cot/src/test.rs	88.69% <85.29%> (-1.36%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[m4tx]

Suggested change

	#[cfg(feature = "redis")]
	#[ignore = "Tests that use Redis are ignored by default"]
	#[cfg(feature = "redis")]

We probably want to add this to avoid broken tests when a Redis server is not available.

[m4tx]

Can't we just hide the entire function behind #[cfg(feature = "cache")] and get rid of this #[allow]?

[m4tx]

I'm not exactly happy about this flushing the entire database, but there doesn't seem to be any sensible way to only remove the keys with a specific prefix, and other framework don't solve this either, so I guess this is fine.

[ElijahAhianyo]

Yeah, that makes sense. Should we support it(probably not in this PR unless it's critical)? I'm thinking of a usecase for this and it only makes sense if users are able to dynamically change the prefix or set different prefixes when inserting into the cache. Currently, we only allow setting the prefix system-wide in the config and not per insertion or on the fly without completely recreating the cache instance again. What if we have an API that allows you to set the key with a prefix just like we do for expiry? Or instead of another API, we can modify the set API to include options:

struct InsertOption{
    timeout:  Option<Timeout>,
    prefix: Option<String>,
    ...
}

...
fn insert<K, V>(&self, key: K, value: V: options: Option<InsertOption>){...}

Then in this case, providing an API that deletes or flushes the DB based on a prefix will have a better use

[m4tx]

Should we support it(probably not in this PR unless it's critical)?

I don't think so, especially because I'm not sure if there's any way to implement this efficiently.

I'm thinking of a usecase for this and it only makes sense if users are able to dynamically change the prefix or set different prefixes when inserting into the cache.

Agreed, this is the only concern of mine. Redis instances are also cheap to set up, so it's not even a problem with sharing the instances with other services or something similar.

Or instead of another API, we can modify the set API to include options:

Yes, I think we should have this kind of API. I'm a bit afraid of the API verbosity, though—for instance, should we keep the insert_expiring method in the user-facing API? If not, we should make sure InsertOptions can be easily created with some sort of builder-like APIs. But this is likely a discussion for the future.

[m4tx]

I'm thinking whether it makes sense. Maybe it's best for the user to decide which database should be used in the tests, so that the user can run a single Redis instance for multiple purposes? The database number could be just passed in REDIS_URL.

On the other hand, ideally, each test should run in a separate database, similarly to what happens with PostgreSQL/MySQL tests. This guarantees that the tests do not interfere with each other and can be run fully parallelized (which they do by default with nextest!). This would also allow us to just call flushdb at the end of each test. With Redis, however, this is somewhat challenging:

1. The default total database number is 16
2. We don't have database names; only numbers

The first problem is easy to solve—we can just increase the number of databases in our Redis config in compose.yml. The second one is tough—we can take the test name and use it as our DB name—if there are no equally called tests, we're fine. With Redis we could probably hash the test name, get the remainder of the division with the total number of databases, use it, and hope for the best. Or select a random database index and check if it contains any keys.

I'm not sure what would suit us the best here. Do you have any thoughts?

[ElijahAhianyo]

Ah yeah that makes sense. I actually took this approach because I needed a way for a test to specify what db the test wants to run in, to avoid race conditions. I love the approach of running each test in a completely isolated DB.

For the second problem, I have a somewhat silly suggestion as well, it might not be the best, and I agree if it feels over-engineered, but hear me out:

What if we can have a simple redis database ID allocator instead? So in our API contract we can reserve the last DB for allocations. For eg, If we set the max number of DB to 100, then the 99th would be a reserved one. (I chose the last as the default is 0 and we typically wouldnt need to force users to increase their DB max value as well). We can run a script once the redis server is up to push the numbers 0 - N-2(98) into the allocator DB. At runtime, the tests will pop values from the allocator and that will be their db number.
The downside to this is having to make two connections: one to get their actual DB number from the reserved DB and one to connect to the actual DB.
Another similar case is one where a test connects to redis without using the macro, in that case, they wouldnt really know what DB is free to use, however, they can manually connect to the reserved DB and request for one, or better still, use a random one ONLY if they are sure theyre sure there would be no side effects, which is risky since the DBs will cleanup after the tests are done. Alternatively, we can further have a contract that reserves a range of DBs for internal use(used by the macro) which gurantees that any DB used out of the range will indeed be free to use.
I havent tested this so I'm unsure of the feasibility

What do you think?

[m4tx]

The database ID allocator sounds smart! And yes, with the correct Redis configuration, I can't see a reason why this couldn't work. I would implement it slightly differently, though:

1. Each test would try to create a key with a random database ID, and the test name as the value. If a key already exists, it tries a different one (with some timeout/max retry number).
2. When the test exits, it removes the key it used, so it can be reused with other tests.

By using this approach, we don't need to run a separate script (and we don't need to make sure the script is run before the tests), so I think this is a bit more robust.

(by the way, we could probably use Redis lock system for this)

---

In addition to that, I've got other options in mind:

1. Have a file-based locking mechanism to ensure that only one Redis test can be run at given time - might be feasible here, since we probably won't have that many long-running Redis tests.
2. Using testcontainers-rs - this is definitely the most robust and modern way to achieve true test separation, as we spin up a separate Docker container for each test. The downside is that it's probably slower (although I haven't done any actual measurements), and if we use it here, we would probably want to use it for the database tests, too.

Which of these do I think is the best? It feels like testcontainers might be the easiest to scale in the long run—when we add another cache backends (or basically whatever that needs any sort of resource sharing), we won't need to reimplement any sharing logic—just spinning up a new container in each test will do. I'm a bit concerned about the performance hit, though, incurred by constantly re-running the containers. I'll be happy with each of the solutions, though, (unless we find out they're not suitable for some reason), so I'll leave the decision to you.

[m4tx]

I'm not sure if we should have this method here. I'd like to avoid referencing specific backends inside config.rs mostly so that we can more easily modularize subsystems such as Cache in the future.

[ElijahAhianyo]

Yeah, I know, sleep here looks really suspicious, and I'm not even proud of it, but I couldn't find a better way to sync the processes so only one process runs the initialization script. Here are a few approaches I tried:

1. Synchronizing on Redis. This did not work. The idea was to set a key an init key on the allocator DB, which should be done only by one process and visible to the others, but since the Redis connections are created per process and DBs accessed logically, it wasn't atomic.
2. The redis redlock (via rslock) didn't work here. Per my understanding, you need this in a distributed setting to coordinate access to multiple Redis DBs. I felt this was a good candidate for use with testcontainers-rs(even though we won't even need it since each test will be isolated by default, avoiding all the shenanigans here anyway)
3. File lock. The other option I had was to use a file lock to coordinate the processes. To make the file visible to all processes, I decided to create the file in the system's tempdir. I couldn't really use a conventional file lock, since I needed a way to tell whether the initialization script had already been run without creating too many special cases or side effects. To get around this, I relied on the file creation itself as the lock, which is atomic across processes. The first process to create the file "has" the lock and runs the initialization script; the rest receive an error and do nothing. This works, but there's another problem. Subsequent test runs may see the file since the file isn't cleared automatically. Deleting a file isn't atomic, so there's another headache of ensuring that the file doesn't exist for a fresh run. This might be fine in the CI but non-deterministic locally. To "guarantee" that the file is cleared, the process that runs the script removes it after running the script, but it delays a bit so the other processes see the file before deleting it. If there are better ways to go about this, I'd love to hear them

[seqre]

I think this could be solved with transactions, can you give them a try?

[seqre]

I think we no longer need that, look here: #412

[seqre]

I think this could be solved with transactions, can you give them a try?

[m4tx]

Please use expect here instead of unwrap to provide some meaningful message when the connection fails (and yes, I know we have unwraps in the #[cot_macros::dbtest] macro - they shouldn't be there either).

[m4tx]

Do we need 10 connections in the pool for the initializer?

[ElijahAhianyo]

actually, I tried 1 and the redis tests were too way too slow, 2 was also nondeterminstic; on some runs the tests failed in weird areas, 3 seemed to have been the sweetspot. I left it at 10 for redanduncy

[m4tx]

This is really good, thank you a lot!