Describe the bug

It is implied that the SCRIPT KILL command should be atomic. However, in rare cases, it is possible to demonstrate its synchronicity. Albeit probably not a major concern IMO, it may present testing/timing issues (credit @sazzad16 in https://github.com/redis/jedis/issues/2656) and violates the implied nature of Redis commands.

To reproduce

  1. Spin up a Redis server
  2. Run a slow script, e.g. redis-cli EVAL "while true do end" 0
  3. Kill the script and run another arbitrary command quickly (same querybuf?): echo -e '*1\r\n$4\r\nPING\r\n*2\r\n$6\r\nSCRIPT\r\n$4\r\nKILL\r\n*1\r\n$4\r\nPING\r\n' | nc localhost 6379

Although the script will be killed, the output from step 3 above is:

-BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE.
+OK
-BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE.

Expected behavior

The output should be something like:

-BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE.
+OK
+PONG

Comment From: enjoy-binbin

[root@binblog redis]# echo -e '*1\r\n$4\r\nPING\r\n*2\r\n$6\r\nSCRIPT\r\n$4\r\nKILL\r\n*1\r\n$4\r\nPING\r\n' | nc localhost 6379
-BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE.
+OK
+PONG

looks like we can circumvent it by adding !scriptIsKilled(), but I'm not sure if there is any other effect, @MeirShpilraien https://github.com/redis/redis/blob/56a802057e90ea482702dbd5191bc397caf0f596/src/server.c#L3643-L3665

-    if (scriptIsTimedout() &&
+    if (scriptIsTimedout() && !scriptIsKilled() &&

Comment From: MeirShpilraien

@enjoy-binbin I believe what you are suggesting is dangerous, the command could be, by itself, another eval and then we will basically enter a recursive eval (which is not supported today because all the eval variables are singletons) and I believe bad things will happened. I believe what we should do is to stop processing command after we get script kill, let the script be killed, and then continue processing commands normally.

Comment From: enjoy-binbin

yes, make sence, i thought so too, but currently we only handle script_kill flag in scriptInterrupt, and look like it was called in luaMaskCountHook, i'm not very familiar with that lua part... (i think you can fix it in a easy / right way if needed) i suppose we can kill the script in script|kill command in a sync way? anyway, thank you for the explanation! :)