Expose script flags to processCommand for better handling (#10744)

The important part is that read-only scripts (not just EVAL_RO
and FCALL_RO, but also ones with `no-writes` executed by normal EVAL or
FCALL), will now be permitted to run during CLIENT PAUSE WRITE (unlike
before where only the _RO commands would be processed).

Other than that, some errors like OOM, READONLY, MASTERDOWN are now
handled by processCommand, rather than the command itself affects the
error string (and even error code in some cases), and command stats.

Besides that, now the `may-replicate` commands, PFCOUNT and PUBLISH, will
be considered `write` commands in scripts and will be blocked in all
read-only scripts just like other write commands.
They'll also be blocked in EVAL_RO (i.e. even for scripts without the
`no-writes` shebang flag.

This commit also hides the `may_replicate` flag from the COMMAND command
output. this is a **breaking change**.

background about may_replicate:
We don't want to expose a no-may-replicate flag or alike to scripts, since we
consider the may-replicate thing an internal concern of redis, that we may
some day get rid of.
In fact, the may-replicate flag was initially introduced to flag EVAL: since
we didn't know what it's gonna do ahead of execution, before function-flags
existed). PUBLISH and PFCOUNT, both of which because they have side effects
which may some day be fixed differently.

code changes:
The changes in eval.c are mostly code re-ordering:
- evalCalcFunctionName is extracted out of evalGenericCommand
- evalExtractShebangFlags is extracted luaCreateFunction
- evalGetCommandFlags is new code

src/aof.c

@@ -1502,7 +1502,10 @@ int loadSingleAppendOnlyFile(char *filename) {
         if (fakeClient->flags & CLIENT_MULTI &&
             fakeClient->cmd->proc != execCommand)
         {
-            queueMultiCommand(fakeClient);
+            /* Note: we don't have to attempt calling evalGetCommandFlags,
+             * since this is AOF, the checks in processCommand are not made
+             * anyway.*/
+            queueMultiCommand(fakeClient, cmd->flags);
         } else {
             cmd->proc(fakeClient);
         }

src/eval.c

@@ -285,6 +285,124 @@ void scriptingReset(int async) {
  * EVAL and SCRIPT commands implementation
  * ------------------------------------------------------------------------- */
 
+static void evalCalcFunctionName(int evalsha, sds script, char *out_funcname) {
+    /* We obtain the script SHA1, then check if this function is already
+     * defined into the Lua state */
+    out_funcname[0] = 'f';
+    out_funcname[1] = '_';
+    if (!evalsha) {
+        /* Hash the code if this is an EVAL call */
+        sha1hex(out_funcname+2,script,sdslen(script));
+    } else {
+        /* We already have the SHA if it is an EVALSHA */
+        int j;
+        char *sha = script;
+
+        /* Convert to lowercase. We don't use tolower since the function
+         * managed to always show up in the profiler output consuming
+         * a non trivial amount of time. */
+        for (j = 0; j < 40; j++)
+            out_funcname[j+2] = (sha[j] >= 'A' && sha[j] <= 'Z') ?
+                sha[j]+('a'-'A') : sha[j];
+        out_funcname[42] = '\0';
+    }
+}
+
+/* Helper function to try and extract shebang flags from the script body.
+ * If no shebang is found, return with success and COMPAT mode flag.
+ * The err arg is optional, can be used to get a detailed error string.
+ * The out_shebang_len arg is optional, can be used to trim the shebang from the script.
+ * Returns C_OK on success, and C_ERR on error. */
+int evalExtractShebangFlags(sds body, uint64_t *out_flags, ssize_t *out_shebang_len, sds *err) {
+    ssize_t shebang_len = 0;
+    uint64_t script_flags = SCRIPT_FLAG_EVAL_COMPAT_MODE;
+    if (!strncmp(body, "#!", 2)) {
+        int numparts,j;
+        char *shebang_end = strchr(body, '\n');
+        if (shebang_end == NULL) {
+            if (err)
+                *err = sdsnew("Invalid script shebang");
+            return C_ERR;
+        }
+        shebang_len = shebang_end - body;
+        sds shebang = sdsnewlen(body, shebang_len);
+        sds *parts = sdssplitargs(shebang, &numparts);
+        sdsfree(shebang);
+        if (!parts || numparts == 0) {
+            if (err)
+                *err = sdsnew("Invalid engine in script shebang");
+            sdsfreesplitres(parts, numparts);
+            return C_ERR;
+        }
+        /* Verify lua interpreter was specified */
+        if (strcmp(parts[0], "#!lua")) {
+            if (err)
+                *err = sdscatfmt(sdsempty(), "Unexpected engine in script shebang: %s", parts[0]);
+            sdsfreesplitres(parts, numparts);
+            return C_ERR;
+        }
+        script_flags &= ~SCRIPT_FLAG_EVAL_COMPAT_MODE;
+        for (j = 1; j < numparts; j++) {
+            if (!strncmp(parts[j], "flags=", 6)) {
+                sdsrange(parts[j], 6, -1);
+                int numflags, jj;
+                sds *flags = sdssplitlen(parts[j], sdslen(parts[j]), ",", 1, &numflags);
+                for (jj = 0; jj < numflags; jj++) {
+                    scriptFlag *sf;
+                    for (sf = scripts_flags_def; sf->flag; sf++) {
+                        if (!strcmp(flags[jj], sf->str)) break;
+                    }
+                    if (!sf->flag) {
+                        if (err)
+                            *err = sdscatfmt(sdsempty(), "Unexpected flag in script shebang: %s", flags[jj]);
+                        sdsfreesplitres(flags, numflags);
+                        sdsfreesplitres(parts, numparts);
+                        return C_ERR;
+                    }
+                    script_flags |= sf->flag;
+                }
+                sdsfreesplitres(flags, numflags);
+            } else {
+                /* We only support function flags options for lua scripts */
+                if (err)
+                    *err = sdscatfmt(sdsempty(), "Unknown lua shebang option: %s", parts[j]);
+                sdsfreesplitres(parts, numparts);
+                return C_ERR;
+            }
+        }
+        sdsfreesplitres(parts, numparts);
+    }
+    if (out_shebang_len)
+        *out_shebang_len = shebang_len;
+    *out_flags = script_flags;
+    return C_OK;
+}
+
+/* Try to extract command flags if we can, returns the modified flags.
+ * Note that it does not guarantee the command arguments are right. */
+uint64_t evalGetCommandFlags(client *c, uint64_t cmd_flags) {
+    char funcname[43];
+    int evalsha = c->cmd->proc == evalShaCommand || c->cmd->proc == evalShaRoCommand;
+    if (evalsha && sdslen(c->argv[1]->ptr) != 40)
+        return cmd_flags;
+    evalCalcFunctionName(evalsha, c->argv[1]->ptr, funcname);
+    char *lua_cur_script = funcname + 2;
+    dictEntry *de = dictFind(lctx.lua_scripts, lua_cur_script);
+    uint64_t script_flags;
+    if (!de) {
+        if (evalsha)
+            return cmd_flags;
+        if (evalExtractShebangFlags(c->argv[1]->ptr, &script_flags, NULL, NULL) == C_ERR)
+            return cmd_flags;
+    } else {
+        luaScript *l = dictGetVal(de);
+        script_flags = l->flags;
+    }
+    if (script_flags & SCRIPT_FLAG_EVAL_COMPAT_MODE)
+        return cmd_flags;
+    return scriptFlagsToCmdFlags(cmd_flags, script_flags);
+}
+
 /* Define a Lua function with the specified body.
  * The function name will be generated in the following form:
  *
@@ -305,7 +423,7 @@ void scriptingReset(int async) {
 sds luaCreateFunction(client *c, robj *body) {
     char funcname[43];
     dictEntry *de;
-    uint64_t script_flags = SCRIPT_FLAG_EVAL_COMPAT_MODE;
+    uint64_t script_flags;
 
     funcname[0] = 'f';
     funcname[1] = '_';
@@ -317,56 +435,10 @@ sds luaCreateFunction(client *c, robj *body) {
 
     /* Handle shebang header in script code */
     ssize_t shebang_len = 0;
-    if (!strncmp(body->ptr, "#!", 2)) {
-        int numparts,j;
-        char *shebang_end = strchr(body->ptr, '\n');
-        if (shebang_end == NULL) {
-            addReplyError(c,"Invalid script shebang");
-            return NULL;
-        }
-        shebang_len = shebang_end - (char*)body->ptr;
-        sds shebang = sdsnewlen(body->ptr, shebang_len);
-        sds *parts = sdssplitargs(shebang, &numparts);
-        sdsfree(shebang);
-        if (!parts || numparts == 0) {
-            addReplyError(c,"Invalid engine in script shebang");
-            sdsfreesplitres(parts, numparts);
-            return NULL;
-        }
-        /* Verify lua interpreter was specified */
-        if (strcmp(parts[0], "#!lua")) {
-            addReplyErrorFormat(c,"Unexpected engine in script shebang: %s", parts[0]);
-            sdsfreesplitres(parts, numparts);
-            return NULL;
-        }
-        script_flags &= ~SCRIPT_FLAG_EVAL_COMPAT_MODE;
-        for (j = 1; j < numparts; j++) {
-            if (!strncmp(parts[j], "flags=", 6)) {
-                sdsrange(parts[j], 6, -1);
-                int numflags, jj;
-                sds *flags = sdssplitlen(parts[j], sdslen(parts[j]), ",", 1, &numflags);
-                for (jj = 0; jj < numflags; jj++) {
-                    scriptFlag *sf;
-                    for (sf = scripts_flags_def; sf->flag; sf++) {
-                        if (!strcmp(flags[jj], sf->str)) break;
-                    }
-                    if (!sf->flag) {
-                        addReplyErrorFormat(c,"Unexpected flag in script shebang: %s", flags[jj]);
-                        sdsfreesplitres(flags, numflags);
-                        sdsfreesplitres(parts, numparts);
-                        return NULL;
-                    }
-                    script_flags |= sf->flag;
-                }
-                sdsfreesplitres(flags, numflags);
-            } else {
-                /* We only support function flags options for lua scripts */
-                addReplyErrorFormat(c,"Unknown lua shebang option: %s", parts[j]);
-                sdsfreesplitres(parts, numparts);
-                return NULL;
-            }
-        }
-        sdsfreesplitres(parts, numparts);
+    sds err = NULL;
+    if (evalExtractShebangFlags(body->ptr, &script_flags, &shebang_len, &err) == C_ERR) {
+        addReplyErrorSds(c, err);
+        return NULL;
     }
 
     /* Note that in case of a shebang line we skip it but keep the line feed to conserve the user's line numbers */
@@ -430,26 +502,7 @@ void evalGenericCommand(client *c, int evalsha) {
         return;
     }
 
-    /* We obtain the script SHA1, then check if this function is already
-     * defined into the Lua state */
-    funcname[0] = 'f';
-    funcname[1] = '_';
-    if (!evalsha) {
-        /* Hash the code if this is an EVAL call */
-        sha1hex(funcname+2,c->argv[1]->ptr,sdslen(c->argv[1]->ptr));
-    } else {
-        /* We already have the SHA if it is an EVALSHA */
-        int j;
-        char *sha = c->argv[1]->ptr;
-
-        /* Convert to lowercase. We don't use tolower since the function
-         * managed to always show up in the profiler output consuming
-         * a non trivial amount of time. */
-        for (j = 0; j < 40; j++)
-            funcname[j+2] = (sha[j] >= 'A' && sha[j] <= 'Z') ?
-                sha[j]+('a'-'A') : sha[j];
-        funcname[42] = '\0';
-    }
+    evalCalcFunctionName(evalsha, c->argv[1]->ptr, funcname);
 
     /* Push the pcall error handler function on the stack. */
     lua_getglobal(lua, "__redis__err__handler");

src/functions.c

@@ -604,6 +604,17 @@ void functionKillCommand(client *c) {
     scriptKill(c, 0);
 }
 
+/* Try to extract command flags if we can, returns the modified flags.
+ * Note that it does not guarantee the command arguments are right. */
+uint64_t fcallGetCommandFlags(client *c, uint64_t cmd_flags) {
+    robj *function_name = c->argv[1];
+    functionInfo *fi = dictFetchValue(curr_functions_lib_ctx->functions, function_name->ptr);
+    if (!fi)
+        return cmd_flags;
+    uint64_t script_flags = fi->f_flags;
+    return scriptFlagsToCmdFlags(cmd_flags, script_flags);
+}
+
 static void fcallCommandGeneric(client *c, int ro) {
     robj *function_name = c->argv[1];
     functionInfo *fi = dictFetchValue(curr_functions_lib_ctx->functions, function_name->ptr);

src/multi.c

@@ -56,7 +56,7 @@ void freeClientMultiState(client *c) {
 }
 
 /* Add a new command into the MULTI commands queue */
-void queueMultiCommand(client *c) {
+void queueMultiCommand(client *c, uint64_t cmd_flags) {
     multiCmd *mc;
 
     /* No sense to waste memory if the transaction is already aborted.
@@ -75,8 +75,8 @@ void queueMultiCommand(client *c) {
     mc->argv_len = c->argv_len;
 
     c->mstate.count++;
-    c->mstate.cmd_flags |= c->cmd->flags;
-    c->mstate.cmd_inv_flags |= ~c->cmd->flags;
+    c->mstate.cmd_flags |= cmd_flags;
+    c->mstate.cmd_inv_flags |= ~cmd_flags;
     c->mstate.argv_len_sums += c->argv_len_sum + sizeof(robj*)*c->argc;
 
     /* Reset the client's args since we copied them into the mstate and shouldn't

src/script.c

@@ -108,6 +108,25 @@ int scriptInterrupt(scriptRunCtx *run_ctx) {
     return (run_ctx->flags & SCRIPT_KILLED) ? SCRIPT_KILL : SCRIPT_CONTINUE;
 }
 
+uint64_t scriptFlagsToCmdFlags(uint64_t cmd_flags, uint64_t script_flags) {
+    /* If the script declared flags, clear the ones from the command and use the ones it declared.*/
+    cmd_flags &= ~(CMD_STALE | CMD_DENYOOM | CMD_WRITE);
+
+    /* NO_WRITES implies ALLOW_OOM */
+    if (!(script_flags & (SCRIPT_FLAG_ALLOW_OOM | SCRIPT_FLAG_NO_WRITES)))
+        cmd_flags |= CMD_DENYOOM;
+    if (!(script_flags & SCRIPT_FLAG_NO_WRITES))
+        cmd_flags |= CMD_WRITE;
+    if (script_flags & SCRIPT_FLAG_ALLOW_STALE)
+        cmd_flags |= CMD_STALE;
+
+    /* In addition the MAY_REPLICATE flag is set for these commands, but
+     * if we have flags we know if it's gonna do any writes or not. */
+    cmd_flags &= ~CMD_MAY_REPLICATE;
+
+    return cmd_flags;
+}
+
 /* Prepare the given run ctx for execution */
 int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *caller, const char *funcname, uint64_t script_flags, int ro) {
     serverAssert(!curr_run_ctx);
@@ -136,7 +155,7 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca
              * 2. no disk error detected
              * 3. command is not `fcall_ro`/`eval[sha]_ro` */
             if (server.masterhost && server.repl_slave_ro && !obey_client) {
-                addReplyError(caller, "Can not run script with write flag on readonly replica");
+                addReplyError(caller, "-READONLY Can not run script with write flag on readonly replica");
                 return C_ERR;
             }
 
@@ -327,16 +346,23 @@ static int scriptVerifyACL(client *c, sds *err) {
 }
 
 static int scriptVerifyWriteCommandAllow(scriptRunCtx *run_ctx, char **err) {
-    if (!(run_ctx->c->cmd->flags & CMD_WRITE)) {
-        return C_OK;
-    }
 
-    if (run_ctx->flags & SCRIPT_READ_ONLY) {
-        /* We know its a write command, on a read only run we do not allow it. */
+    /* A write command, on an RO command or an RO script is rejected ASAP.
+     * Note: For scripts, we consider may-replicate commands as write commands.
+     * This also makes it possible to allow read-only scripts to be run during
+     * CLIENT PAUSE WRITE. */
+    if (run_ctx->flags & SCRIPT_READ_ONLY &&
+        (run_ctx->c->cmd->flags & (CMD_WRITE|CMD_MAY_REPLICATE)))
+    {
         *err = sdsnew("Write commands are not allowed from read-only scripts.");
         return C_ERR;
     }
 
+    /* The other checks below are on the server state and are only relevant for
+     *  write commands, return if this is not a write command. */
+    if (!(run_ctx->c->cmd->flags & CMD_WRITE))
+        return C_OK;
+
     /* Write commands are forbidden against read-only slaves, or if a
      * command marked as non-deterministic was already called in the context
      * of this script. */
@@ -366,16 +392,6 @@ static int scriptVerifyWriteCommandAllow(scriptRunCtx *run_ctx, char **err) {
     return C_OK;
 }
 
-static int scriptVerifyMayReplicate(scriptRunCtx *run_ctx, char **err) {
-    if (run_ctx->c->cmd->flags & CMD_MAY_REPLICATE &&
-        server.client_pause_type == CLIENT_PAUSE_WRITE) {
-        *err = sdsnew("May-replicate commands are not allowed when client pause write.");
-        return C_ERR;
-    }
-
-    return C_OK;
-}
-
 static int scriptVerifyOOM(scriptRunCtx *run_ctx, char **err) {
     if (run_ctx->flags & SCRIPT_ALLOW_OOM) {
         /* Allow running any command even if OOM reached */
@@ -530,10 +546,6 @@ void scriptCall(scriptRunCtx *run_ctx, robj* *argv, int argc, sds *err) {
         goto error;
     }
 
-    if (scriptVerifyMayReplicate(run_ctx, err) != C_OK) {
-        goto error;
-    }
-
     if (scriptVerifyOOM(run_ctx, err) != C_OK) {
         goto error;
     }

src/script.h

@@ -93,6 +93,7 @@ typedef struct scriptFlag {
 
 extern scriptFlag scripts_flags_def[];
 
+uint64_t scriptFlagsToCmdFlags(uint64_t cmd_flags, uint64_t script_flags);
 int scriptPrepareForRun(scriptRunCtx *r_ctx, client *engine_client, client *caller, const char *funcname, uint64_t script_flags, int ro);
 void scriptResetRun(scriptRunCtx *r_ctx);
 int scriptSetResp(scriptRunCtx *r_ctx, int resp);

src/server.c

@@ -3633,19 +3633,33 @@ int processCommand(client *c) {
         }
     }
 
-    int is_read_command = (c->cmd->flags & CMD_READONLY) ||
+    /* If we're executing a script, try to extract a set of command flags from
+     * it, in case it declared them. Note this is just an attempt, we don't yet
+     * know the script command is well formed.*/
+    uint64_t cmd_flags = c->cmd->flags;
+    if (c->cmd->proc == evalCommand || c->cmd->proc == evalShaCommand ||
+        c->cmd->proc == evalRoCommand || c->cmd->proc == evalShaRoCommand ||
+        c->cmd->proc == fcallCommand || c->cmd->proc == fcallroCommand)
+    {
+        if (c->cmd->proc == fcallCommand || c->cmd->proc == fcallroCommand)
+            cmd_flags = fcallGetCommandFlags(c, cmd_flags);
+        else
+            cmd_flags = evalGetCommandFlags(c, cmd_flags);
+    }
+
+    int is_read_command = (cmd_flags & CMD_READONLY) ||
                            (c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_READONLY));
-    int is_write_command = (c->cmd->flags & CMD_WRITE) ||
+    int is_write_command = (cmd_flags & CMD_WRITE) ||
                            (c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_WRITE));
-    int is_denyoom_command = (c->cmd->flags & CMD_DENYOOM) ||
+    int is_denyoom_command = (cmd_flags & CMD_DENYOOM) ||
                              (c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_DENYOOM));
-    int is_denystale_command = !(c->cmd->flags & CMD_STALE) ||
+    int is_denystale_command = !(cmd_flags & CMD_STALE) ||
                                (c->cmd->proc == execCommand && (c->mstate.cmd_inv_flags & CMD_STALE));
-    int is_denyloading_command = !(c->cmd->flags & CMD_LOADING) ||
+    int is_denyloading_command = !(cmd_flags & CMD_LOADING) ||
                                  (c->cmd->proc == execCommand && (c->mstate.cmd_inv_flags & CMD_LOADING));
-    int is_may_replicate_command = (c->cmd->flags & (CMD_WRITE | CMD_MAY_REPLICATE)) ||
+    int is_may_replicate_command = (cmd_flags & (CMD_WRITE | CMD_MAY_REPLICATE)) ||
                                    (c->cmd->proc == execCommand && (c->mstate.cmd_flags & (CMD_WRITE | CMD_MAY_REPLICATE)));
-    int is_deny_async_loading_command = (c->cmd->flags & CMD_NO_ASYNC_LOADING) ||
+    int is_deny_async_loading_command = (cmd_flags & CMD_NO_ASYNC_LOADING) ||
                                         (c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_NO_ASYNC_LOADING));
     int obey_client = mustObeyClient(c);
 
@@ -3913,7 +3927,7 @@ int processCommand(client *c) {
         c->cmd->proc != quitCommand &&
         c->cmd->proc != resetCommand)
     {
-        queueMultiCommand(c);
+        queueMultiCommand(c, cmd_flags);
         addReply(c,shared.queued);
     } else {
         call(c,CMD_CALL_FULL);
@@ -4325,7 +4339,7 @@ void addReplyFlagsForCommand(client *c, struct redisCommand *cmd) {
         {CMD_ASKING,            "asking"},
         {CMD_FAST,              "fast"},
         {CMD_NO_AUTH,           "no_auth"},
-        {CMD_MAY_REPLICATE,     "may_replicate"},
+        /* {CMD_MAY_REPLICATE,     "may_replicate"},, Hidden on purpose */
         /* {CMD_SENTINEL,          "sentinel"}, Hidden on purpose */
         /* {CMD_ONLY_SENTINEL,     "only_sentinel"}, Hidden on purpose */
         {CMD_NO_MANDATORY_KEYS, "no_mandatory_keys"},

src/server.h

@@ -2592,7 +2592,7 @@ void listElementsRemoved(client *c, robj *key, int where, robj *o, long count, i
 void unwatchAllKeys(client *c);
 void initClientMultiState(client *c);
 void freeClientMultiState(client *c);
-void queueMultiCommand(client *c);
+void queueMultiCommand(client *c, uint64_t cmd_flags);
 size_t multiStateMemOverhead(client *c);
 void touchWatchedKey(redisDb *db, robj *key);
 int isWatchedKeyExpired(client *c);
@@ -3201,6 +3201,8 @@ void sha1hex(char *digest, char *script, size_t len);
 unsigned long evalMemory();
 dict* evalScriptsDict();
 unsigned long evalScriptsMemory();
+uint64_t evalGetCommandFlags(client *c, uint64_t orig_flags);
+uint64_t fcallGetCommandFlags(client *c, uint64_t orig_flags);
 int isInsideYieldingLongCommand();
 
 typedef struct luaScript {

tests/unit/functions.tcl

@@ -424,7 +424,7 @@ start_server {tags {"scripting repl external:skip"}} {
                 r -1 fcall test 0
             } e
             set _ $e
-        } {*Can not run script with write flag on readonly replica*}
+        } {READONLY You can't write against a read only replica.}
     }
 }
 
@@ -1052,7 +1052,7 @@ start_server {tags {"scripting"}} {
         r config set maxmemory 1
 
         catch {[r fcall f1 1 k]} e
-        assert_match {*can not run it when used memory > 'maxmemory'*} $e
+        assert_match {OOM *when used memory > 'maxmemory'*} $e
 
         r config set maxmemory 0
     } {OK} {needs:config-maxmemory}
@@ -1082,12 +1082,12 @@ start_server {tags {"scripting"}} {
         r replicaof 127.0.0.1 1
 
         catch {[r fcall f1 0]} e
-        assert_match {*'allow-stale' flag is not set on the script*} $e
+        assert_match {MASTERDOWN *} $e
 
         assert_equal {hello} [r fcall f2 0]
 
         catch {[r fcall f3 0]} e
-        assert_match {*Can not execute the command on a stale replica*} $e
+        assert_match {ERR *Can not execute the command on a stale replica*} $e
 
         assert_match {*redis_version*} [r fcall f4 0]
 

tests/unit/moduleapi/misc.tcl

@@ -262,7 +262,7 @@ start_server {tags {"modules"}} {
             return 2
         } 1 x
 
-        assert_error {ERR Can not run script with write flag on readonly replica} {
+        assert_error {READONLY Can not run script with write flag on readonly replica*} {
             r test.rm_call eval {#!lua
                 redis.call('get','x')
                 return 3

tests/unit/pause.tcl

@@ -47,18 +47,14 @@ start_server {tags {"pause network"}} {
     test "Test read/admin mutli-execs are not blocked by pause RO" {
         r SET FOO BAR
         r client PAUSE 100000 WRITE
-        set rd [redis_deferring_client]
-        $rd MULTI
-        assert_equal [$rd read] "OK"
-        $rd PING
-        assert_equal [$rd read] "QUEUED"
-        $rd GET FOO
-        assert_equal [$rd read] "QUEUED"
-        $rd EXEC
+        set rr [redis_client]
+        assert_equal [$rr MULTI] "OK"
+        assert_equal [$rr PING] "QUEUED"
+        assert_equal [$rr GET FOO] "QUEUED"
+        assert_match "PONG BAR" [$rr EXEC]
         assert_equal [s 0 blocked_clients] 0
         r client unpause 
-        assert_match "PONG BAR" [$rd read]
-        $rd close
+        $rr close
     }
 
     test "Test write mutli-execs are blocked by pause RO" {
@@ -78,20 +74,129 @@ start_server {tags {"pause network"}} {
     test "Test scripts are blocked by pause RO" {
         r client PAUSE 60000 WRITE
         set rd [redis_deferring_client]
+        set rd2 [redis_deferring_client]
         $rd EVAL "return 1" 0
 
-        wait_for_blocked_clients_count 1 50 100
+        # test a script with a shebang and no flags for coverage
+        $rd2 EVAL {#!lua
+            return 1
+        } 0
+
+        wait_for_blocked_clients_count 2 50 100
         r client unpause 
         assert_match "1" [$rd read]
+        assert_match "1" [$rd2 read]
         $rd close
+        $rd2 close
     }
 
-    test "Test may-replicate commands are rejected in ro script by pause RO" {
+    test "Test RO scripts are not blocked by pause RO" {
+        r set x y
+        # create a function for later
+        r FUNCTION load replace {#!lua name=f1
+            redis.register_function{
+                function_name='f1',
+                callback=function() return "hello" end,
+                flags={'no-writes'}
+            }
+        }
+
+        r client PAUSE 6000000 WRITE
+        set rr [redis_client]
+
+        # test an eval that's for sure not in the script cache
+        assert_equal [$rr EVAL {#!lua flags=no-writes
+                return 'unique script'
+            } 0
+        ] "unique script"
+
+        # for sanity, repeat that EVAL on a script that's already cached
+        assert_equal [$rr EVAL {#!lua flags=no-writes
+                return 'unique script'
+            } 0
+        ] "unique script"
+
+        # test EVAL_RO on a unique script that's for sure not in the cache
+        assert_equal [$rr EVAL_RO {
+            return redis.call('GeT', 'x')..' unique script'
+            } 1 x
+        ] "y unique script"
+
+        # test with evalsha
+        set sha [$rr script load {#!lua flags=no-writes
+                return 2
+            }]
+        assert_equal [$rr EVALSHA $sha 0] 2
+
+        # test with function
+        assert_equal [$rr fcall f1 0] hello
+
+        r client unpause
+        $rr close
+    }
+
+    test "Test read-only scripts in mutli-exec are not blocked by pause RO" {
+        r SET FOO BAR
+        r client PAUSE 100000 WRITE
+        set rr [redis_client]
+        assert_equal [$rr MULTI] "OK"
+        assert_equal [$rr EVAL {#!lua flags=no-writes
+                return 12
+            } 0
+        ] QUEUED
+        assert_equal [$rr EVAL {#!lua flags=no-writes
+                return 13
+            } 0
+        ] QUEUED
+        assert_match "12 13" [$rr EXEC]
+        assert_equal [s 0 blocked_clients] 0
+        r client unpause
+        $rr close
+    }
+
+    test "Test write scripts in mutli-exec are blocked by pause RO" {
+        set rd [redis_deferring_client]
+        set rd2 [redis_deferring_client]
+
+        # one with a shebang
+        $rd MULTI
+        assert_equal [$rd read] "OK"
+        $rd EVAL {#!lua
+                return 12
+            } 0
+        assert_equal [$rd read] "QUEUED"
+
+        # one without a shebang
+        $rd2 MULTI
+        assert_equal [$rd2 read] "OK"
+        $rd2 EVAL {#!lua
+                return 13
+            } 0
+        assert_equal [$rd2 read] "QUEUED"
+
         r client PAUSE 60000 WRITE
-        assert_error {ERR May-replicate commands are not allowed when client pause write*} {
+        $rd EXEC
+        $rd2 EXEC
+        wait_for_blocked_clients_count 2 50 100
+        r client unpause
+        assert_match "12" [$rd read]
+        assert_match "13" [$rd2 read]
+        $rd close
+        $rd2 close
+    }
+
+    test "Test may-replicate commands are rejected in RO scripts" {
+        # that's specifically important for CLIENT PAUSE WRITE
+        assert_error {ERR Write commands are not allowed from read-only scripts. script:*} {
             r EVAL_RO "return redis.call('publish','ch','msg')" 0
         }
-        r client unpause
+        assert_error {ERR Write commands are not allowed from read-only scripts. script:*} {
+            r EVAL {#!lua flags=no-writes
+                return redis.call('publish','ch','msg')
+            } 0
+        }
+        # make sure that publish isn't blocked from a non-RO script
+        assert_equal [r EVAL "return redis.call('publish','ch','msg')" 0] 0
     }
 
     test "Test multiple clients can be queued up and unblocked" {

tests/unit/scripting.tcl

@@ -1416,7 +1416,7 @@ start_server {tags {"scripting"}} {
         ] {123}
 
         # Fail to execute regardless of script content when we use default flags in OOM condition
-        assert_error {OOM allow-oom flag is not set on the script, can not run it when used memory > 'maxmemory'} {
+        assert_error {OOM *} {
             r eval {#!lua flags=
                 return 1
             } 0
@@ -1489,14 +1489,53 @@ start_server {tags {"scripting"}} {
                 } 1 x
             ] "some value"
 
-            assert_error {ERR Can not run script with write flag on readonly replica} {
+            assert_error {READONLY You can't write against a read only replica.} {
                 r eval {#!lua
                     return redis.call('get','x')
                 } 1 x
             }
 
-            # sanity check on protocol after error reply
-            assert_equal [r ping] PONG
+            # test no-write inside multi-exec
+            r multi
+            r eval {#!lua flags=no-writes
+                redis.call('get','x')
+                return 1
+            } 1 x
+            assert_equal [r exec] 1
+
+            # test no shebang without write inside multi-exec
+            r multi
+            r eval {
+                redis.call('get','x')
+                return 1
+            } 1 x
+            assert_equal [r exec] 1
+
+            # temporarily set the server to master, so it doesn't block the queuing
+            # and we can test the evaluation of the flags on exec
+            r replicaof no one
+            set rr [redis_client]
+            set rr2 [redis_client]
+            $rr multi
+            $rr2 multi
+
+            # test write inside multi-exec
+            # we don't need to do any actual write
+            $rr eval {#!lua
+                return 1
+            } 0
+
+            # test no shebang with write inside multi-exec
+            $rr2 eval {
+                redis.call('set','x',1)
+                return 1
+            } 1 x
+
+            r replicaof [srv -1 host] [srv -1 port]
+            assert_error {EXECABORT Transaction discarded because of: READONLY *} {$rr exec}
+            assert_error {READONLY You can't write against a read only replica. script: *} {$rr2 exec}
+            $rr close
+            $rr2 close
         }
     }
 
@@ -1541,7 +1580,7 @@ start_server {tags {"scripting"}} {
             } 1 x
         }
 
-        assert_error {*'allow-stale' flag is not set on the script*} {
+        assert_error {MASTERDOWN Link with MASTER is down and replica-serve-stale-data is set to 'no'.} {
             r eval {#!lua flags=no-writes
                 return 1
             } 0
@@ -1582,7 +1621,7 @@ start_server {tags {"scripting"}} {
     test "reject script do not cause a Lua stack leak" {
         r config set maxmemory 1
         for {set i 0} {$i < 50} {incr i} {
-            assert_error {OOM allow-oom flag is not set on the script, can not run it when used memory > 'maxmemory'} {r eval {#!lua
+            assert_error {OOM *} {r eval {#!lua
                 return 1
             } 0}
         }