fix: handle non-existent keys properly in Store.SMembers

[volmedo]

I saw some unexpected traces when debugging some custom instrumentation I'm adding. Basically, the service was not looking for claims in IPNI or legacy systems even though there were no results in the cache.

Turns out that the SMEMBERS command doesn't return (nil) when the key doesn't exist, as others commands do. Instead, it returns an empty set (see the docs for SMEMBERS and then SINTER). Therefore, the store was returning an empty slice instead of ErrKeyNotFound.

I added an explicit existence check in SMembers when the command returns an empty set, assuming that it may be interesting to differentiate between a non-existent and an empty set.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
pkg/redis/redisstore.go	86.36% <100.00%> (+0.20%)	⬆️

... and 2 files with indirect coverage changes

[alanshaw]

Hmm, Exists will be another round trip to the redis instance right? In the interests of speed and cost I'd probably either:

1. Check the number of items in the result and return not found error if 0
2. Make a note that this function does not return a not found error and check for 0 members in the calling code

...also bearing in mind that we do not have an API to remove from the set, so 0 members is effectively not exists

[volmedo]

According to the traces, Exists takes around 0.8ms to complete, so it's not that much of a performance penalty. Not sure about the cost, though.

My first implementation was 1. I then thought that an empty set could have a different meaning. We don't currently do it, but we could store an empty set for hashes we have already tried to find claims for in the different systems but we couldn't find any. That would speed up future queries where we know we won't find any claims. Of course, that means that no claims will be returned as long as the key doesn't expire.

I'm not sure how the upload-service queries the indexer or what behavior it expects, so please let me know whether the above makes any sense or not. If it doesn't make sense, I'll simplify and implement 1.

[alanshaw]

Wow that is fast! Still, my gut instinct is to not cache failures so I'm not sure I'd agree to the possibility of an empty set anyway. I've approved anyway, it's not worth blocking the fix.

[volmedo]

Let's assume empty sets are not useful for now. We can iterate on this if we find a use case for them in the future.

[alanshaw]

ACK ok! Still LGTM