Skip to content

Fixes a bug in Redis global cache triggered when TTL is 0#1327

Merged
matsduf merged 1 commit into
zonemaster:developfrom
matsduf:fix-redis-cache-bug
Mar 18, 2024
Merged

Fixes a bug in Redis global cache triggered when TTL is 0#1327
matsduf merged 1 commit into
zonemaster:developfrom
matsduf:fix-redis-cache-bug

Conversation

@matsduf

@matsduf matsduf commented Mar 16, 2024

Copy link
Copy Markdown
Contributor

Purpose

This PR fixes the bug described in #1326 by setting the lowest TTL value to 1.

This should be seen as a temporary solution. There should probably be a shortest time that messages are cached in Redis to make sure that in the normal case the same query should not be sent more than once when running a test. In issue #1326 we can see that the TTL of "iis.se. CDS" is 0, and a query for that would be sent several times if not cached.

That shortest time should be configurable in the profile.

Context

Fixes #1326.

How to test this PR

Find a zone that returns TTL=0 for some record that Zonemaster queries for a verify that there is no fatal error of the type describe in #1326.

@matsduf matsduf added T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version. labels Mar 16, 2024
@matsduf matsduf added this to the v2023.2 milestone Mar 16, 2024
@matsduf matsduf linked an issue Mar 17, 2024 that may be closed by this pull request
@marc-vanderwal

Copy link
Copy Markdown
Contributor

Why don’t we skip caching altogether if the TTL is 0?

@matsduf

matsduf commented Mar 18, 2024

Copy link
Copy Markdown
Contributor Author

Why don’t we skip caching altogether if the TTL is 0?

That is an alternative to work-around the bug, but that will decrease caching in some cases compared to "local cache". With the discovered example, there is more than one test case querying for the CDS record, and with "local cache" that query only goes on the wire once. The global caching should be long enough to survive a test, in the normal case.

Another answer is that I started my investigation by setting the minimum cache value to 1 to see if that solved the issue, and that works as well.

@matsduf matsduf merged commit d7fe804 into zonemaster:develop Mar 18, 2024
@matsduf matsduf deleted the fix-redis-cache-bug branch March 18, 2024 10:48
@hannaeko

hannaeko commented Apr 8, 2024

Copy link
Copy Markdown
Contributor

Why don’t we skip caching altogether if the TTL is 0?

That is an alternative to work-around the bug, but that will decrease caching in some cases compared to "local cache". With the discovered example, there is more than one test case querying for the CDS record, and with "local cache" that query only goes on the wire once. The global caching should be long enough to survive a test, in the normal case.

Another answer is that I started my investigation by setting the minimum cache value to 1 to see if that solved the issue, and that works as well.

You probably should skip inserting the key in Redis if the TTL is 0, the Redis cache still has a "local cache" so the answer will still be cached for the duration of the test anyway, same as the previous cache system.

@matsduf

matsduf commented Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

You probably should skip inserting the key in Redis if the TTL is 0, the Redis cache still has a "local cache" so the answer will still be cached for the duration of the test anyway, same as the previous cache system.

Thanks! I will create an update. I did not realize that the local cache also was used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fatal error in Redis cache

4 participants