Replication Script Cache Bug

[minus-infinity]

Step 1:

eval "if tonumber(KEYS[1]) > 0 then redis.call('incr', 'x') end" 1 0

Step 2:

evalsha <sha1 step 1 script> 1 0

At this step sha1 of the script is added to the replication script cache (the script is marked as known to the slaves) and EVALSHA command is transformed to EVAL.
However it is not dirty (there is no changes to db), so it is not propagated to the slaves.

Step 3:

evalsha <sha1 step 1 script> 1 1

At this step master checks that the script already exists in the replication script cache and doesn't transform it to EVAL command. It is dirty and propagated to the slaves, but they fail to evaluate the script as they don't have it in the script cache.

This issue affects AOF rewrite and replication.

The issue can be fixed by setting REDIS_FORCE_REPL and REDIS_FORCE_AOF flags when adding sha1 to the replication script cache:

scripting.c:

if (!replicationScriptCacheExists(c->argv[1]->ptr)) {
     /* This script is not in our script cache, replicate it as
      * EVAL, then add it into the script cache, as from now on
      * slaves and AOF know about it. */

       robj *script = dictFetchValue(server.lua_scripts,c->argv[1]->ptr);

       replicationScriptCacheAdd(c->argv[1]->ptr);
       redisAssertWithInfo(c,NULL,script != NULL);
       rewriteClientCommandArgument(c,0,
               resetRefCount(createStringObject("EVAL",4)));
       rewriteClientCommandArgument(c,1,script);
       c->flags |= (REDIS_FORCE_REPL | REDIS_FORCE_AOF);
}

[antirez]

This is outstanding, thanks! And you joined Github just to report the bug 👍

[minus-infinity]

It was quite challenging to reproduce and debug it on 100GB dataset in production environment

[antirez]

And we were looking to hunt this bug as well... I believe it is an issue that affected Hulu as well. So I guess that your fix was also verified in production that is definitely a bonus point even if the solution looks obvious and sane per se.

[minus-infinity]

I have updated our production servers and it works fine for few hours already. The slaves are in sync (did not check AOF). The slaves couldn't make successful initial sync at all before the patch.

[antirez]

Thanks for the ack, merging tomorrow morning + unit tests.

[minus-infinity]

I would like to thank you for such great in-memory data structure database with good persistence options, we use it as primary db, not just like caching key-value solution.

[antirez]

Thank you for using it @minus-infinity 😃

Fix pushed into all branches, writing a regression test now. I'll release 2.8.6 ASAP (there is another issue to fix). Closing.