From acf111f69ea5611c0b911fa57e8d73200e957bd4 Mon Sep 17 00:00:00 2001 From: Adam Vartanian Date: Mon, 17 Feb 2025 20:44:13 +0000 Subject: [PATCH] Fix time parsing for limit cache Fix a bug in the code that handles determining the timestamp for values in the cache that's used to implement removing the oldest key when the limit is reached. The bug is in parsing the time returned by Redis, making it produce incorrect results when the leading portion of the microseconds moves from 09 to 10. This tended to reveal itself as flakiness in the `test_limit` unit test, which we were seeing around 1 in 200 runs. The `TIME` operation in Redis returns the time as an array of two strings, the first holding the number of seconds and the second holding the number of microseconds. Importantly, the second string has no leading zeroes. The Lua code parses this by concatenating these strings together separated by a decimal point and then parsing that as a number, which produces incorrectly ordered results when faced with values like `("1234567", "999")` and `("1234567", "1000")`, parsing them as 1234567.999000 and 1234567.100000, respectively, meaning the second value will be expunged from the cache first despite being inserted second. Instead, parse each returned string as a number and then combine them numerically, which produces the correct number in this situation. I can't easily produce a test for this, because to test this deterministically you have to control the timestamps Redis produces, and the current testing setup uses a standalone Redis process. We have a internal unit test that uses `freezegun` and an in-process Redis fake to test it, but I didn't think you would want that level of change to the testing configuration just to make the test for this very narrow change work. --- redis_cache/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/redis_cache/__init__.py b/redis_cache/__init__.py index 899d6de..ec9ac0b 100644 --- a/redis_cache/__init__.py +++ b/redis_cache/__init__.py @@ -64,7 +64,9 @@ def get_cache_lua_fn(client): local limit = tonumber(ARGV[3]) if limit > 0 then local time_parts = redis.call('TIME') - local time = tonumber(time_parts[1] .. '.' .. time_parts[2]) + -- TIME returns [seconds, microseconds] (as strings), so parse each + -- and add together to get the full timestamp + local time = tonumber(time_parts[1]) + (tonumber(time_parts[2]) / 1000000) redis.call('ZADD', KEYS[2], time, KEYS[1]) local count = tonumber(redis.call('ZCOUNT', KEYS[2], '-inf', '+inf')) local over = count - limit