1. 19 Mar, 2023 1 commit
    • Viktor Söderqvist's avatar
      redis-cli: Accept commands in subscribed mode (#11873) · bbf364a4
      Viktor Söderqvist authored
      The message "Reading messages... (press Ctrl-C to quit)" is replaced by
      "Reading messages... (press Ctrl-C to quit or any key to type command)".
      
      This allows users to subscribe to more channels, to try out UNSUBSCRIBE and to
      combine pubsub with other features such as push messages from client tracking.
      
      The "Reading messages" info message is displayed in the bottom of the output in a
      distinct style and moves downward as more messages appear. When any key is pressed,
      the info message is replaced by the prompt with for entering commands.
      After entering a command and the reply is displayed, the "Reading messages" info
      messages appears again. This is added to the repl loop in redis-cli and in the
      corresponding place for non-interactive mode.
      
      An indication "(subscribed mode)" is included in the prompt when entering commands
      in subscribed mode.
      
      Also:
      * Fixes a problem that UNSUBSCRIBE hanged when used with RESP3 and push callback,
        without first entering subscribe mode. It hanged because UNSUBSCRIBE gets one or
        more push replies but no in-band reply.
      * Exit subscribed mode after RESET.
      bbf364a4
  2. 16 Mar, 2023 3 commits
    • Meir Shpilraien (Spielrein)'s avatar
      Support for RM_Call on blocking commands (#11568) · d0da0a6a
      Meir Shpilraien (Spielrein) authored
      Allow running blocking commands from within a module using `RM_Call`.
      
      Today, when `RM_Call` is used, the fake client that is used to run command
      is marked with `CLIENT_DENY_BLOCKING` flag. This flag tells the command
      that it is not allowed to block the client and in case it needs to block, it must
      fallback to some alternative (either return error or perform some default behavior).
      For example, `BLPOP` fallback to simple `LPOP` if it is not allowed to block.
      
      All the commands must respect the `CLIENT_DENY_BLOCKING` flag (including
      module commands). When the command invocation finished, Redis asserts that
      the client was not blocked.
      
      This PR introduces the ability to call blocking command using `RM_Call` by
      passing a callback that will be called when the client will get unblocked.
      In order to do that, the user must explicitly say that he allow to perform blocking
      command by passing a new format specifier argument, `K`, to the `RM_Call`
      function. This new flag will tell Redis that it is allow to run blocking command
      and block the client. In case the command got blocked, Redis will return a new
      type of call reply (`REDISMODULE_REPLY_PROMISE`). This call reply indicates
      that the command got blocked and the user can set the on_unblocked handler using
      `RM_CallReplyPromiseSetUnblockHandler`.
      
      When clients gets unblocked, it eventually reaches `processUnblockedClients` function.
      This is where we check if the client is a fake module client and if it is, we call the unblock
      callback instead of performing the usual unblock operations.
      
      **Notice**: `RM_CallReplyPromiseSetUnblockHandler` must be called atomically
      along side the command invocation (without releasing the Redis lock in between).
      In addition, unlike other CallReply types, the promise call reply must be released
      by the module when the Redis GIL is acquired.
      
      The module can abort the execution on the blocking command (if it was not yet
      executed) using `RM_CallReplyPromiseAbort`. the API will return `REDISMODULE_OK`
      on success and `REDISMODULE_ERR` if the operation is already executed.
      **Notice** that in case of misbehave module, Abort might finished successfully but the
      operation will not really be aborted. This can only happened if the module do not respect
      the disconnect callback of the blocked client. 
      For pure Redis commands this can not happened.
      
      ### Atomicity Guarantees
      
      The API promise that the unblock handler will run atomically as an execution unit.
      This means that all the operation performed on the unblock handler will be wrapped
      with a multi exec transaction when replicated to the replica and AOF.
      The API **do not** grantee any other atomicity properties such as when the unblock
      handler will be called. This gives us the flexibility to strengthen the grantees (or not)
      in the future if we will decide that we need a better guarantees.
      
      That said, the implementation **does** provide a better guarantees when performing
      pure Redis blocking command like `BLPOP`. In this case the unblock handler will run
      atomically with the operation that got unblocked (for example, in case of `BLPOP`, the
      unblock handler will run atomically with the `LPOP` operation that run when the command
      got unblocked). This is an implementation detail that might be change in the future and the
      module writer should not count on that.
      
      ### Calling blocking commands while running on script mode (`S`)
      
      `RM_Call` script mode (`S`) was introduced on #0372. It is used for usecases where the
      command that was invoked on `RM_Call` comes from a user input and we want to make
      sure the user will not run dangerous commands like `shutdown`. Some command, such
      as `BLPOP`, are marked with `NO_SCRIPT` flag, which means they will not be allowed on
      script mode. Those commands are marked with  `NO_SCRIPT` just because they are
      blocking commands and not because they are dangerous. Now that we can run blocking
      commands on RM_Call, there is no real reason not to allow such commands on script mode.
      
      The underline problem is that the `NO_SCRIPT` flag is abused to also mark some of the
      blocking commands (notice that those commands know not to block the client if it is not
      allowed to do so, and have a fallback logic to such cases. So even if those commands
      were not marked with `NO_SCRIPT` flag, it would not harm Redis, and today we can
      already run those commands within multi exec).
      
      In addition, not all blocking commands are marked with `NO_SCRIPT` flag, for example
      `blmpop` are not marked and can run from within a script.
      
      Those facts shows that there are some ambiguity about the meaning of the `NO_SCRIPT`
      flag, and its not fully clear where it should be use.
      
      The PR suggest that blocking commands should not be marked with `NO_SCRIPT` flag,
      those commands should handle `CLIENT_DENY_BLOCKING` flag and only block when
      it's safe (like they already does today). To achieve that, the PR removes the `NO_SCRIPT`
      flag from the following commands:
      * `blmove`
      * `blpop`
      * `brpop`
      * `brpoplpush`
      * `bzpopmax`
      * `bzpopmin`
      * `wait`
      
      This might be considered a breaking change as now, on scripts, instead of getting
      `command is not allowed from script` error, the user will get some fallback behavior
      base on the command implementation. That said, the change matches the behavior
      of scripts and multi exec with respect to those commands and allow running them on
      `RM_Call` even when script mode is used.
      
      ### Additional RedisModule API and changes
      
      * `RM_BlockClientSetPrivateData` - Set private data on the blocked client without the
        need to unblock the client. This allows up to set the promise CallReply as the private
        data of the blocked client and abort it if the client gets disconnected.
      * `RM_BlockClientGetPrivateData` - Return the current private data set on a blocked client.
        We need it so we will have access to this private data on the disconnect callback.
      * On RM_Call, the returned reply will be added to the auto memory context only if auto
        memory is enabled, this allows us to keep the call reply for longer time then the context
        lifetime and does not force an unneeded borrow relationship between the CallReply and
        the RedisModuleContext.
      d0da0a6a
    • Binbin's avatar
      Fix usleep compilation warning in auth.c (#11925) · 484b73a8
      Binbin authored
      There is a -Wimplicit-function-declaration warning in here:
      ```
      auth.c: In function ‘AuthBlock_ThreadMain’:
      auth.c:116:5: warning: implicit declaration of function ‘usleep’; did you mean ‘sleep’? [-Wimplicit-function-declaration]
        116 |     usleep(500000);
            |     ^~~~~~
            |     sleep
      ```
      484b73a8
    • Binbin's avatar
      Bump codespell to 2.2.4, fix typos and outupdated comments (#11911) · 0b159b34
      Binbin authored
      Fix some seen typos and wrong comments.
      0b159b34
  3. 15 Mar, 2023 3 commits
    • KarthikSubbarao's avatar
      Custom authentication for Modules (#11659) · f8a5a4f7
      KarthikSubbarao authored
      
      
      This change adds new module callbacks that can override the default password based authentication associated with ACLs. With this, Modules can register auth callbacks through which they can implement their own Authentication logic. When `AUTH` and `HELLO AUTH ...` commands are used, Module based authentication is attempted and then normal password based authentication is attempted if needed.
      The new Module APIs added in this PR are - `RM_RegisterCustomAuthCallback` and `RM_BlockClientOnAuth` and `RedisModule_ACLAddLogEntryByUserName `.
      
      Module based authentication will be attempted for all Redis users (created through the ACL SETUSER cmd or through Module APIs) even if the Redis user does not exist at the time of the command. This gives a chance for the Module to create the RedisModule user and then authenticate via the RedisModule API - from the custom auth callback.
      
      For the AUTH command, we will support both variations - `AUTH <username> <password>` and `AUTH <password>`. In case of the `AUTH <password>` variation, the custom auth callbacks are triggered with “default” as the username and password as what is provided.
      
      
      ### RedisModule_RegisterCustomAuthCallback
      ```
      void RM_RegisterCustomAuthCallback(RedisModuleCtx *ctx, RedisModuleCustomAuthCallback cb) {
      ```
      This API registers a callback to execute to prior to normal password based authentication. Multiple callbacks can be registered across different modules. These callbacks are responsible for either handling the authentication, each authenticating the user or explicitly denying, or deferring it to other authentication mechanisms. Callbacks are triggered in the order they were registered. When a Module is unloaded, all the auth callbacks registered by it are unregistered. The callbacks are attempted, in the order of most recently registered callbacks, when the AUTH/HELLO (with AUTH field is provided) commands are called. The callbacks will be called with a module context along with a username and a password, and are expected to take one of the following actions:
      
       (1) Authenticate - Use the RM_Authenticate* API successfully and return `REDISMODULE_AUTH_HANDLED`. This will immediately end the auth chain as successful and add the OK reply.
      (2) Block a client on authentication - Use the `RM_BlockClientOnAuth` API and return `REDISMODULE_AUTH_HANDLED`. Here, the client will be blocked until the `RM_UnblockClient `API is used which will trigger the auth reply callback (provided earlier through the `RM_BlockClientOnAuth`). In this reply callback, the Module should authenticate, deny or skip handling authentication.
      (3) Deny Authentication - Return `REDISMODULE_AUTH_HANDLED` without authenticating or blocking the client. Optionally, `err` can be set to a custom error message. This will immediately end the auth chain as unsuccessful and add the ERR reply.
      (4) Skip handling Authentication - Return `REDISMODULE_AUTH_NOT_HANDLED` without blocking the client. This will allow the engine to attempt the next custom auth callback.
      
      If none of the callbacks authenticate or deny auth, then password based auth is attempted and will authenticate or add failure logs and reply to the clients accordingly.
      
      ### RedisModule_BlockClientOnAuth
      ```
      RedisModuleBlockedClient *RM_BlockClientOnAuth(RedisModuleCtx *ctx, RedisModuleCustomAuthCallback reply_callback,
                                                     void (*free_privdata)(RedisModuleCtx*,void*))
      ```
      This API can only be used from a Module from the custom auth callback. If a client is not in the middle of custom module based authentication, ERROR is returned. Otherwise, the client is blocked and the `RedisModule_BlockedClient` is returned similar to the `RedisModule_BlockClient` API.
      
      ### RedisModule_ACLAddLogEntryByUserName
      ```
      int RM_ACLAddLogEntryByUserName(RedisModuleCtx *ctx, RedisModuleString *username, RedisModuleString *object, RedisModuleACLLogEntryReason reason)
      ```
      Adds a new entry in the ACL log with the `username` RedisModuleString provided. This simplifies the Module usage because now, developers do not need to create a Module User just to add an error ACL Log entry. Aside from accepting username (RedisModuleString) instead of a RedisModuleUser, it is the same as the existing `RedisModule_ACLAddLogEntry` API.
      
      
      ### Breaking changes
      - HELLO command - Clients can now only set the client name and RESP protocol from the `HELLO` command if they are authenticated. Also, we now finish command arg validation first and return early with a ERR reply if any arg is invalid. This is to avoid mutating the client name / RESP from a command that would have failed on invalid arguments.
      
      ### Notable behaviors
      - Module unblocking - Now, we will not allow Modules to block the client from inside the context of a reply callback (triggered from the Module unblock flow `moduleHandleBlockedClients`).
      
      ---------
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      f8a5a4f7
    • Binbin's avatar
      Fix WAITAOF mix-use last_offset and last_numreplicas (#11922) · 58285a6e
      Binbin authored
      There be a situation that satisfies WAIT, and then wrongly unblock
      WAITAOF because we mix-use last_offset and last_numreplicas.
      
      We update last_offset and last_numreplicas only when the condition
      matches. i.e. output of either replicationCountAOFAcksByOffset or
      replicationCountAcksByOffset is right.
      
      In this case, we need to have separate last_ variables for each of
      them. Added a last_aof_offset and last_aof_numreplicas for WAITAOF.
      
      WAITAOF was added in #11713. Found while coding #11917.
      A Test was added to validate that case.
      58285a6e
    • Binbin's avatar
      Fix WAITAOF reply when using last_offset and last_numreplicas (#11917) · 70b2c4f5
      Binbin authored
      WAITAOF wad added in #11713, its return is an array.
      But forget to handle WAITAOF in last_offset and last_numreplicas,
      causing WAITAOF to return a WAIT like reply.
      
      Tests was added to validate that case (both WAIT and WAITAOF).
      This PR also refactored processClientsWaitingReplicas a bit for better
      maintainability and readability.
      70b2c4f5
  4. 14 Mar, 2023 1 commit
    • Slava Koyfman's avatar
      Implementing the WAITAOF command (issue #10505) (#11713) · 9344f654
      Slava Koyfman authored
      
      
      Implementing the WAITAOF functionality which would allow the user to
      block until a specified number of Redises have fsynced all previous write
      commands to the AOF.
      
      Syntax: `WAITAOF <num_local> <num_replicas> <timeout>`
      Response: Array containing two elements: num_local, num_replicas
      num_local is always either 0 or 1 representing the local AOF on the master.
      num_replicas is the number of replicas that acknowledged the a replication
      offset of the last write being fsynced to the AOF.
      
      Returns an error when called on replicas, or when called with non-zero
      num_local on a master with AOF disabled, in all other cases the response
      just contains number of fsync copies.
      
      Main changes:
      * Added code to keep track of replication offsets that are confirmed to have
        been fsynced to disk.
      * Keep advancing master_repl_offset even when replication is disabled (and
        there's no replication backlog, only if there's an AOF enabled).
        This way we can use this command and it's mechanisms even when replication
        is disabled.
      * Extend REPLCONF ACK to `REPLCONF ACK <ofs> FACK <ofs>`, the FACK
        will be appended only if there's an AOF on the replica, and already ignored on
        old masters (thus backwards compatible)
      * WAIT now no longer wait for the replication offset after your last command, but
        rather the replication offset after your last write (or read command that caused
        propagation, e.g. lazy expiry).
      
      Unrelated changes:
      * WAIT command respects CLIENT_DENY_BLOCKING (not just CLIENT_MULTI)
      
      Implementation details:
      * Add an atomic var named `fsynced_reploff_pending` that's updated
        (usually by the bio thread) and later copied to the main `fsynced_reploff`
        variable (only if the AOF base file exists).
        I.e. during the initial AOF rewrite it will not be used as the fsynced offset
        since the AOF base is still missing.
      * Replace close+fsync bio job with new BIO_CLOSE_AOF (AOF specific)
        job that will also update fsync offset the field.
      * Handle all AOF jobs (BIO_CLOSE_AOF, BIO_AOF_FSYNC) in the same bio
        worker thread, to impose ordering on their execution. This solves a
        race condition where a job could set `fsynced_reploff_pending` to a higher
        value than another pending fsync job, resulting in indicating an offset
        for which parts of the data have not yet actually been fsynced.
        Imposing an ordering on the jobs guarantees that fsync jobs are executed
        in increasing order of replication offset.
      * Drain bio jobs when switching `appendfsync` to "always"
        This should prevent a write race between updates to `fsynced_reploff_pending`
        in the main thread (`flushAppendOnlyFile` when set to ALWAYS fsync), and
        those done in the bio thread.
      * Drain the pending fsync when starting over a new AOF to avoid race conditions
        with the previous AOF offsets overriding the new one (e.g. after switching to
        replicate from a new master).
      * Make sure to update the fsynced offset at the end of the initial AOF rewrite.
        a must in case there are no additional writes that trigger a periodic fsync,
        specifically for a replica that does a full sync.
      
      Limitations:
      It is possible to write a module and a Lua script that propagate to the AOF and doesn't
      propagate to the replication stream. see REDISMODULE_ARGV_NO_REPLICAS and luaRedisSetReplCommand.
      These features are incompatible with the WAITAOF command, and can result
      in two bad cases. The scenario is that the user executes command that only
      propagates to AOF, and then immediately
      issues a WAITAOF, and there's no further writes on the replication stream after that.
      1. if the the last thing that happened on the replication stream is a PING
        (which increased the replication offset but won't trigger an fsync on the replica),
        then the client would hang forever (will wait for an fack that the replica will never
        send sine it doesn't trigger any fsyncs).
      2. if the last thing that happened is a write command that got propagated properly,
        then WAITAOF will be released immediately, without waiting for an fsync (since
        the offset didn't change)
      
      Refactoring:
      * Plumbing to allow bio worker to handle multiple job types
        This introduces infrastructure necessary to allow BIO workers to
        not have a 1-1 mapping of worker to job-type. This allows in the
        future to assign multiple job types to a single worker, either as
        a performance/resource optimization, or as a way of enforcing
        ordering between specific classes of jobs.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      9344f654
  5. 13 Mar, 2023 1 commit
    • Binbin's avatar
      Fix tail->repl_offset update in feedReplicationBuffer (#11905) · 7997874f
      Binbin authored
      
      
      In #11666, we added a while loop and will split a big reply
      node to multiple nodes. The update of tail->repl_offset may
      be wrong. Like before #11666, we would have created at most
      one new reply node, and now we will create multiple nodes if
      it is a big reply node.
      
      Now we are creating more than one node, and the tail->repl_offset
      of all the nodes except the last one are incorrect. Because we
      update master_repl_offset at the beginning, and then use it to
      update the tail->repl_offset. This would have lead to an assertion
      during PSYNC, a test was added to validate that case.
      
      Besides that, the calculation of size was adjusted to fix
      tests that failed due to a combination of a very low backlog size,
      and some thresholds of that get violated because of the relatively
      high overhead of replBufBlock. So now if the backlog size / 16 is too
      small, we'll take PROTO_REPLY_CHUNK_BYTES instead.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      7997874f
  6. 12 Mar, 2023 3 commits
    • xbasel's avatar
      Large blocks of replica client output buffer could lead to psync loops and... · 7be7834e
      xbasel authored
      
      Large blocks of replica client output buffer could lead to psync loops and unnecessary memory usage (#11666)
      
      This can happen when a key almost equal or larger than the
      client output buffer limit of the replica is written.
      
      Example:
      1. DB is empty
      2. Backlog size is 1 MB
      3. Client out put buffer limit is 2 MB
      4. Client writes a 3 MB key
      5. The shared replication buffer will have a single node which contains
      the key written above, and it exceeds the backlog size.
      
      At this point the client output buffer usage calculation will report the
      replica buffer to be 3 MB (or more) even after sending all the data to
      the replica.
      The primary drops the replica connection for exceeding the limits,
      the replica reconnects and successfully executes partial sync but the
      primary will drop the connection again because the buffer usage is still
      3 MB. This happens over and over.
      
      To mitigate the problem, this fix limits the maximum size of a single
      backlog node to be (repl_backlog_size/16). This way a single node can't
      exceed the limits of the COB (the COB has to be larger than the
      backlog).
      It also means that if the backlog has some excessive data it can't trim,
      it would be at most about 6% overuse.
      
      other notes:
      1. a loop was added in feedReplicationBuffer which caused a massive LOC
        change due to indentation, the actual changes are just the `min(max` and the loop.
      3. an unrelated change in an existing test to speed up a server termination which took 10 seconds.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      7be7834e
    • Binbin's avatar
      Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notifications (#11875) · 416842e6
      Binbin authored
      This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
      mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
      With these flags, prepareClientToWrite called by addReply* will
      return C_ERR directly. So the client can't receive the Pub/Sub
      messages and any other push notifications, e.g client side tracking.
      
      In this PR, we adding a CLIENT_PUSHING flag, disables the reply
      silencing flags. When adding push replies, set the flag, after the reply,
      clear the flag. Then add the flag check in prepareClientToWrite.
      
      Fixes #11874
      
      Note, the SUBSCRIBE command response is a bit awkward,
      see https://github.com/redis/redis-doc/pull/2327
      
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      416842e6
    • Binbin's avatar
      Fix race in sentinel manual failover test (#11900) · 4e7eb16a
      Binbin authored
      In #9408, we added some SENTINEL DEBUG to reduce default
      timeouts and allow tests to execute faster. The change
      in 05-manual.tcl may cause a race that SENTINEL FAILOVER
      response with a NOGOODSLAVE:
      ```
      Manual failover works: FAILED: Expected NOGOODSLAVE No suitable replica to promote eq "OK" (context: type eval line 6 cmd {assert {$reply eq "OK"}} proc ::test)
      (Jumping to next unit after error)
      FAILED: caught an error in the test
      assertion:Expected NOGOODSLAVE No suitable replica to promote eq "OK" (context: type eval line 6 cmd {assert {$reply eq "OK"}} proc ::test)
      ```
      
      The reason is that the info-period value was reduced in #9408
      (the default value is 10000), and then manual failover was
      performed immediately, but the INFO may not exchanged between
      the sentinel and replicas, causing the sentinel to skip all
      the replicas in sentinelSelectSlave (Because replica's info_refresh
      is not updated, see the code snippet below), then return a NOGOODSLAVE,
      break the test.
      
      Code snippet from sentinelSelectSlave:
      ```
      while((de = dictNext(di)) != NULL) {
          sentinelRedisInstance *slave = dictGetVal(de);
          mstime_t info_validity_time;
          if (master->flags & SRI_S_DOWN)
              info_validity_time = sentinel_ping_period*5;
          else
              info_validity_time = sentinel_info_period*3;
          if (mstime() - slave->info_refresh > info_validity_time) continue;
      }
      ```
      
      By adding a wait_for_condition, we have the opportunity to
      let sentinel update the info_period of the replicas.
      4e7eb16a
  7. 11 Mar, 2023 1 commit
    • guybe7's avatar
      Add reply_schema to command json files (internal for now) (#10273) · 4ba47d2d
      guybe7 authored
      Work in progress towards implementing a reply schema as part of COMMAND DOCS, see #9845
      Since ironing the details of the reply schema of each and every command can take a long time, we
      would like to merge this PR when the infrastructure is ready, and let this mature in the unstable branch.
      Meanwhile the changes of this PR are internal, they are part of the repo, but do not affect the produced build.
      
      ### Background
      In #9656 we add a lot of information about Redis commands, but we are missing information about the replies
      
      ### Motivation
      1. Documentation. This is the primary goal.
      2. It should be possible, based on the output of COMMAND, to be able to generate client code in typed
        languages. In order to do that, we need Redis to tell us, in detail, what each reply looks like.
      3. We would like to build a fuzzer that verifies the reply structure (for now we use the existing
        testsuite, see the "Testing" section)
      
      ### Schema
      The idea is to supply some sort of schema for the various replies of each command.
      The schema will describe the conceptual structure of the reply (for generated clients), as defined in RESP3.
      Note that the reply structure itself may change, depending on the arguments (e.g. `XINFO STREAM`, with
      and without the `FULL` modifier)
      We decided to use the standard json-schema (see https://json-schema.org/) as the reply-schema.
      
      Example for `BZPOPMIN`:
      ```
      "reply_schema": {
          "oneOf": [
              {
                  "description": "Timeout reached and no elements were popped.",
                  "type": "null"
              },
              {
                  "description": "The keyname, popped member, and its score.",
                  "type": "array",
                  "minItems": 3,
                  "maxItems": 3,
                  "items": [
                      {
                          "description": "Keyname",
                          "type": "string"
                      },
                      {
                          "description": "Member",
                          "type": "string"
                      },
                      {
                          "description": "Score",
                          "type": "number"
                      }
                  ]
              }
          ]
      }
      ```
      
      #### Notes
      1.  It is ok that some commands' reply structure depends on the arguments and it's the caller's responsibility
        to know which is the relevant one. this comes after looking at other request-reply systems like OpenAPI,
        where the reply schema can also be oneOf and the caller is responsible to know which schema is the relevant one.
      2. The reply schemas will describe RESP3 replies only. even though RESP3 is structured, we want to use reply
        schema for documentation (and possibly to create a fuzzer that validates the replies)
      3. For documentation, the description field will include an explanation of the scenario in which the reply is sent,
        including any relation to arguments. for example, for `ZRANGE`'s two schemas we will need to state that one
        is with `WITHSCORES` and the other is without.
      4. For documentation, there will be another optional field "notes" in which we will add a short description of
        the representation in RESP2, in case it's not trivial (RESP3's `ZRANGE`'s nested array vs. RESP2's flat
        array, for example)
      
      Given the above:
      1. We can generate the "return" section of all commands in [redis-doc](https://redis.io/commands/)
        (given that "description" and "notes" are comprehensive enough)
      2. We can generate a client in a strongly typed language (but the return type could be a conceptual
        `union` and the caller needs to know which schema is relevant). see the section below for RESP2 support.
      3. We can create a fuzzer for RESP3.
      
      ### Limitations (because we are using the standard json-schema)
      The problem is that Redis' replies are more diverse than what the json format allows. This means that,
      when we convert the reply to a json (in order to validate the schema against it), we lose information (see
      the "Testing" section below).
      The other option would have been to extend the standard json-schema (and json format) to include stuff
      like sets, bulk-strings, error-string, etc. but that would mean also extending the schema-validator - and that
      seemed like too much work, so we decided to compromise.
      
      Examples:
      1. We cannot tell the difference between an "array" and a "set"
      2. We cannot tell the difference between simple-string and bulk-string
      3. we cannot verify true uniqueness of items in commands like ZRANGE: json-schema doesn't cover the
        case of two identical members with different scores (e.g. `[["m1",6],["m1",7]]`) because `uniqueItems`
        compares (member,score) tuples and not just the member name. 
      
      ### Testing
      This commit includes some changes inside Redis in order to verify the schemas (existing and future ones)
      are indeed correct (i.e. describe the actual response of Redis).
      To do that, we added a debugging feature to Redis that causes it to produce a log of all the commands
      it executed and their replies.
      For that, Redis needs to be compiled with `-DLOG_REQ_RES` and run with
      `--reg-res-logfile <file> --client-default-resp 3` (the testsuite already does that if you run it with
      `--log-req-res --force-resp3`)
      You should run the testsuite with the above args (and `--dont-clean`) in order to make Redis generate
      `.reqres` files (same dir as the `stdout` files) which contain request-response pairs.
      These files are later on processed by `./utils/req-res-log-validator.py` which does:
      1. Goes over req-res files, generated by redis-servers, spawned by the testsuite (see logreqres.c)
      2. For each request-response pair, it validates the response against the request's reply_schema
        (obtained from the extended COMMAND DOCS)
      5. In order to get good coverage of the Redis commands, and all their different replies, we chose to use
        the existing redis test suite, rather than attempt to write a fuzzer.
      
      #### Notes about RESP2
      1. We will not be able to use the testing tool to verify RESP2 replies (we are ok with that, it's time to
        accept RESP3 as the future RESP)
      2. Since the majority of the test suite is using RESP2, and we want the server to reply with RESP3
        so that we can validate it, we will need to know how to convert the actual reply to the one expected.
         - number and boolean are always strings in RESP2 so the conversion is easy
         - objects (maps) are always a flat array in RESP2
         - others (nested array in RESP3's `ZRANGE` and others) will need some special per-command
           handling (so the client will not be totally auto-generated)
      
      Example for ZRANGE:
      ```
      "reply_schema": {
          "anyOf": [
              {
                  "description": "A list of member elements",
                  "type": "array",
                  "uniqueItems": true,
                  "items": {
                      "type": "string"
                  }
              },
              {
                  "description": "Members and their scores. Returned in case `WITHSCORES` was used.",
                  "notes": "In RESP2 this is returned as a flat array",
                  "type": "array",
                  "uniqueItems": true,
                  "items": {
                      "type": "array",
                      "minItems": 2,
                      "maxItems": 2,
                      "items": [
                          {
                              "description": "Member",
                              "type": "string"
                          },
                          {
                              "description": "Score",
                              "type": "number"
                          }
                      ]
                  }
              }
          ]
      }
      ```
      
      ### Other changes
      1. Some tests that behave differently depending on the RESP are now being tested for both RESP,
        regardless of the special log-req-res mode ("Pub/Sub PING" for example)
      2. Update the history field of CLIENT LIST
      3. Added basic tests for commands that were not covered at all by the testsuite
      
      ### TODO
      
      - [x] (maybe a different PR) add a "condition" field to anyOf/oneOf schemas that refers to args. e.g.
        when `SET` return NULL, the condition is `arguments.get||arguments.condition`, for `OK` the condition
        is `!arguments.get`, and for `string` the condition is `arguments.get` - https://github.com/redis/redis/issues/11896
      - [x] (maybe a different PR) also run `runtest-cluster` in the req-res logging mode
      - [x] add the new tests to GH actions (i.e. compile with `-DLOG_REQ_RES`, run the tests, and run the validator)
      - [x] (maybe a different PR) figure out a way to warn about (sub)schemas that are uncovered by the output
        of the tests - https://github.com/redis/redis/issues/11897
      - [x] (probably a separate PR) add all missing schemas
      - [x] check why "SDOWN is triggered by misconfigured instance replying with errors" fails with --log-req-res
      - [x] move the response transformers to their own file (run both regular, cluster, and sentinel tests - need to
        fight with the tcl including mechanism a bit)
      - [x] issue: module API - https://github.com/redis/redis/issues/11898
      - [x] (probably a separate PR): improve schemas: add `required` to `object`s - https://github.com/redis/redis/issues/11899
      
      Co-authored-by: default avatarOzan Tezcan <ozantezcan@gmail.com>
      Co-authored-by: default avatarHanna Fadida <hanna.fadida@redislabs.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarShaya Potter <shaya@redislabs.com>
      4ba47d2d
  8. 09 Mar, 2023 1 commit
  9. 08 Mar, 2023 2 commits
    • Binbin's avatar
      Fix test and improve assert_replication_stream print the whole stream (#11793) · a7c9e505
      Binbin authored
      This PR has two parts:
      
      1. Fix flaky test case, the previous tests set a lot of volatile keys,
      it injects an unexpected DEL command into the replication stream during
      the later test, causing it to fail. Add a flushall to avoid it.
      
      2. Improve assert_replication_stream, now it can print the whole stream
      rather than just the failing line.
      a7c9e505
    • ranshid's avatar
      Fix an issue when module decides to unblock a client which is blocked on keys (#11832) · 4988b928
      ranshid authored
      Currently (starting at #11012) When a module is blocked on keys it sets the
      CLIENT_PENDING_COMMAND flag.
      However in case the module decides to unblock the client not via the regular flow
      (eg timeout, key signal or CLIENT UNBLOCK command) it will attempt to reprocess the
      module command and potentially blocked again.
      
      This fix remove the CLIENT_PENDING_COMMAND flag in case blockedForKeys is
      issued from module context.
      4988b928
  10. 07 Mar, 2023 2 commits
    • Binbin's avatar
      Solve race in CLIENT NO-TOUCH lru test (#11883) · 9958ab8b
      Binbin authored
      I've seen it fail here (test-centos7-tls-module-no-tls and test-freebsd):
      ```
      *** [err]: Operations in no-touch mode do not alter the last access time of a key in tests/unit/introspection-2.tcl
      Expected '244296' to be more than '244296' (context: type eval line 12 cmd {assert_morethan $newlru $oldlru} proc ::test)
      ```
      
      Our LRU_CLOCK_RESOLUTION value is 1000ms, and default hz is 10, so if the
      test is really fast, or the timing is just right, newlru will be the same
      as oldlru. We fixed this by changing `after 1000` to `after 1100`.
      9958ab8b
    • sundb's avatar
      Skip test for sdsRemoveFreeSpace when mem_allocator is not jemalloc (#11878) · 3fba3ccd
      sundb authored
      Test `trim on SET with big value` (introduced from #11817) fails under mac m1 with libc mem_allocator.
      The reason is that malloc(33000) will allocate 65536 bytes(>42000).
      This test still passes under ubuntu with libc mem_allocator.
      
      ```
      *** [err]: trim on SET with big value in tests/unit/type/string.tcl
      Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test)
      ```
      
      simple test under mac m1 with libc mem_allocator:
      ```c
      void *p = zmalloc(33000);
      printf("malloc size: %zu\n", zmalloc_size(p));
      
      # output
      malloc size: 65536
      ```
      3fba3ccd
  11. 04 Mar, 2023 1 commit
    • Binbin's avatar
      Increase the threshold of the AOF loading defrag test (#11871) · bfe50a30
      Binbin authored
      This test is very sensitive and fragile. It often fails in Daily,
      in most cases, it failed in test-ubuntu-32bit (the AOF loading one),
      with the range in (31, 40):
      ```
      [err]: Active defrag in tests/unit/memefficiency.tcl
      Expected 38 <= 30 (context: type eval line 113 cmd {assert {$max_latency <= 30}} proc ::test)
      ```
      
      The AOF loading part isn't tightly fixed to the cron hz. It calls
      processEventsWhileBlocked once in every 1024 command calls.
      ```
              /* Serve the clients from time to time */
              if (!(loops++ % 1024)) {
                  off_t progress_delta = ftello(fp) - last_progress_report_size;
                  loadingIncrProgress(progress_delta);
                  last_progress_report_size += progress_delta;
                  processEventsWhileBlocked();
                  processModuleLoadingProgressEvent(1);
              }
      ```
      
      In this case, we can either decrease the 1024 or increase the
      threshold of just the AOF part of that test. Considering the test
      machines are sometimes slow, and all sort of quirks could happen
      (which do not indicate a bug), and we've already set to 30, we suppose
      we can set it a little bit higher, set it to 40. We can have this instead of
      adding another testing config (we can add it when we really need it).
      
      Fixes #11868
      bfe50a30
  12. 28 Feb, 2023 4 commits
  13. 23 Feb, 2023 2 commits
  14. 21 Feb, 2023 2 commits
    • Binbin's avatar
      Speed up test: client evicted due to client tracking prefixes (#11823) · cd58af4d
      Binbin authored
      We noticed that `client evicted due to client tracking prefixes`
      takes over 200 seconds with valgrind.
      
      We combine three prefixes in each command, this will probably
      save us half the testing time.
      
      Before: normal: 3508ms, valgrind: 289503ms -> 290s
      With three prefixes, normal: 1500ms, valgrind: 135742ms -> 136s
      
      Since we did not actually count the memory usage of all prefixes, see
      getClientMemoryUsage, so we can not use larger prefixes to speed up the
      test here. Also this PR cleaned up some spaces (IDE jobs) and typos.
      cd58af4d
    • Madelyn Olson's avatar
      Prevent Redis from crashing from key tracking invalidations (#11814) · dca5927a
      Madelyn Olson authored
      There is a built in limit to client side tracking keys, which when exceeded will invalidate keys. This occurs in two places, one in the server cron and other before executing a command. If it happens in the second scenario, the invalidations will be queued for later since current client is set. This queue is never drained if a command is not executed (through call) such as a multi-exec command getting queued. This results in a later server assert crashing.
      dca5927a
  15. 20 Feb, 2023 1 commit
  16. 16 Feb, 2023 1 commit
    • Oran Agra's avatar
      Cleanup around script_caller, fix tracking of scripts and ACL logging for RM_Call (#11770) · 233abbbe
      Oran Agra authored
      * Make it clear that current_client is the root client that was called by
        external connection
      * add executing_client which is the client that runs the current command
        (can be a module or a script)
      * Remove script_caller that was used for commands that have CLIENT_SCRIPT
        to get the client that called the script. in most cases, that's the current_client,
        and in others (when being called from a module), it could be an intermediate
        client when we actually want the original one used by the external connection.
      
      bugfixes:
      * RM_Call with C flag should log ACL errors with the requested user rather than
        the one used by the original client, this also solves a crash when RM_Call is used
        with C flag from a detached thread safe context.
      * addACLLogEntry would have logged info about the script_caller, but in case the
        script was issued by a module command we actually want the current_client. the
        exception is when RM_Call is called from a timer event, in which case we don't
        have a current_client.
      
      behavior changes:
      * client side tracking for scripts now tracks the keys that are read by the script
        instead of the keys that are declared by the caller for EVAL
      
      other changes:
      * Log both current_client and executing_client in the crash log.
      * remove prepareLuaClient and resetLuaClient, being dead code that was forgotten.
      * remove scriptTimeSnapshot and snapshot_time and instead add cmd_time_snapshot
        that serves all commands and is reset only when execution nesting starts.
      * remove code to propagate CLIENT_FORCE_REPL from the executed command
        to the script caller since scripts aren't propagated anyway these days and anyway
        this flag wouldn't have had an effect since CLIENT_PREVENT_PROP is added by scriptResetRun.
      * fix a module GIL violation issue in afterSleep that was introduced in #10300 (unreleased)
      233abbbe
  17. 15 Feb, 2023 1 commit
    • Binbin's avatar
      Remove wrong code in list pot timeout test (#11805) · 7d5382c0
      Binbin authored
      In #9373, actually need to replace `$rd $pop blist1{t} blist2{t} 1`
      with `bpop_command_two_key $rd $pop blist1{t} blist2{t} 1` but forgot
      to delete the latter.
      
      This doesn't affect the test, because the later assert_error "WRONGTYPE"
      is expected (and right). And if we read $rd again, it will get the
      wrong result, like 'ERR unknown command 'BLMPOP_LEFT' | 'BLMPOP_RIGHT'
      7d5382c0
  18. 14 Feb, 2023 2 commits
    • guybe7's avatar
      Minor changes around the blockonkeys test module (#11803) · 9483ab0b
      guybe7 authored
      All of the POP commands must not decr length below 0.
      So, get_fsl will delete the key if the length is 0 (unless
      the caller wished to create if doesn't exist)
      
      Other:
      1. Use REDISMODULE_WRITE where needed (POP commands)
      2. Use wait_for_blokced_clients in test
      
      Unrelated:
      Use quotes instead of curly braces in zset.tcl, for variable expansion
      9483ab0b
    • guybe7's avatar
      SCAN/RANDOMKEY and lazy-expire (#11788) · fd82bccd
      guybe7 authored
      Starting from Redis 7.0 (#9890) we started wrapping everything a command
       propagates with MULTI/EXEC. The problem is that both SCAN and RANDOMKEY can
      lazy-expire arbitrary keys (similar behavior to active-expire), and put DELs in a transaction.
      
      Fix: When these commands are called without a parent exec-unit (e.g. not in EVAL or
      MULTI) we avoid wrapping their DELs in a transaction (for the same reasons active-expire
      and eviction avoids a transaction)
      
      This PR adds a per-command flag that indicates that the command may touch arbitrary
      keys (not the ones in the arguments), and uses that flag to avoid the MULTI-EXEC.
      For now, this flag is internal, since we're considering other solutions for the future.
      
      Note for cluster mode: if SCAN/RANDOMKEY is inside EVAL/MULTI it can still cause the
      same situation (as it always did), but it won't cause a CROSSSLOT because replicas and AOF
      do not perform slot checks.
      The problem with the above is mainly for 3rd party ecosystem tools that propagate commands
      from master to master, or feed an AOF file with redis-cli into a master.
      This PR aims to fix the regression in redis 7.0, and we opened #11792 to try to handle the
      bigger problem with lazy expire better for another release.
      fd82bccd
  19. 09 Feb, 2023 2 commits
    • Meir Shpilraien (Spielrein)'s avatar
      Match REDISMODULE_OPEN_KEY_* flags to LOOKUP_* flags (#11772) · 5c3938d5
      Meir Shpilraien (Spielrein) authored
      The PR adds support for the following flags on RedisModule_OpenKey:
      
      * REDISMODULE_OPEN_KEY_NONOTIFY - Don't trigger keyspace event on key misses.
      * REDISMODULE_OPEN_KEY_NOSTATS - Don't update keyspace hits/misses counters.
      * REDISMODULE_OPEN_KEY_NOEXPIRE - Avoid deleting lazy expired keys.
      * REDISMODULE_OPEN_KEY_NOEFFECTS - Avoid any effects from fetching the key
      
      In addition, added `RM_GetOpenKeyModesAll`, which returns the mask of all
      supported OpenKey modes. This allows the module to check, in runtime, which
      OpenKey modes are supported by the current Redis instance.
      5c3938d5
    • Binbin's avatar
      When DEBUG LOADAOF fails, return an error instead of exiting (#11790) · 66bed3f2
      Binbin authored
      Return an error when loadAppendOnlyFiles fails instead of
      exiting. DEBUF LOADAOF command is only meant to be used by
      the test suite, and only by tests that generated an AOF file
      first. So this change is ok (considering that the caller is
      likely to catch this error and die).
      
      This actually revert part of the code in #9012, and now
      DEBUG LOADAOF behaves the same as DEBUG RELOAD (returns an
      error when the load fails).
      
      Plus remove a `after 2000` in a test, which can save times (looks like copy paste error).
      66bed3f2
  20. 06 Feb, 2023 1 commit
  21. 03 Feb, 2023 2 commits
    • Binbin's avatar
      Fix unstable test: replication with parallel clients writing in different DBs (#11782) · 03347d04
      Binbin authored
      Failure happens in FreeBSD daily:
      ```
      *** [err]: Test replication with parallel clients writing in different DBs in tests/integration/replication-4.tcl
      Expected [::redis::redisHandle2 dbsize] > 0 (context: type eval line 19 cmd {assert {[$master dbsize] > 0}} proc ::test)
      ```
      
      The test is failing because db 9 has no data (default db), and
      according to the log, we can see that db 9 does not have a key:
      ```
       ### Starting test Test replication with parallel clients writing in different DBs in tests/integration/replication-4.tcl
      3338:S 03 Feb 2023 00:15:18.723 - DB 11: 1 keys (0 volatile) in 4 slots HT.
      3338:S 03 Feb 2023 00:15:18.723 - DB 12: 141 keys (0 volatile) in 256 slots HT.
      ```
      
      We use `wait_for_condition` to ensure that parallel clients have
      written data before calling stop_bg_complex_data. At the same time,
      `wait_for_condition` is also used to remove the above `after 1000`,
      which can save time in most cases.
      03347d04
    • Binbin's avatar
      Fix timing issue in new ACL log test (#11781) · 5a3cdddd
      Binbin authored
      There is a timing issue in the new ACL log test:
      ```
      *** [err]: ACL LOG aggregates similar errors together and assigns unique entry-id to new errors in tests/unit/acl.tcl
      Expected 1675382873989 < 1675382873989 (context: type eval line 15 cmd {assert {$timestamp_last_update_original < $timestamp_last_updated_after_update}} proc ::test)
      ```
      
      Looking at the test code, we will check the `timestamp-last-updated` before
      and after a new ACL error occurs. Actually `WRONGPASS` errors can be executed
      very quickly on fast machines. For example, in the this case, the execution is
      completed within one millisecond.
      
      The error is easy to reproduce, if we reduce the number of the for loops, for
      example set to 2, and using --loop and --stop. Avoid this timing issue by adding
      an `after 1` before the new errors.
      
      The test was introduced in #11477.
      5a3cdddd
  22. 02 Feb, 2023 2 commits
    • Roshan Khatri's avatar
      Added fields to ACL LOG error entries for precise time logging (#11477) · ac312954
      Roshan Khatri authored
      
      
      Added 3 fields to the ACL LOG - adds entry_id, timestamp_created and timestamp_last_updated, which updates similar existing log error entries. The pair - entry_id, timestamp_created is a unique identifier of this entry, in case the node dies and is restarted, it can detect that if it's a new series.
      
      The primary use case of Unique id is to uniquely identify the error messages and not to detect if the server has restarted.
      
      entry-id is the sequence number of the entry (starting at 0) since the server process started. Can also be used to check if items were "lost" if they fell between periods.
      timestamp-created is the unix-time in ms at the time the entry was first created.
      timestamp-last-updated is the unix-time in ms at the time the entry was last updated
      Time_created gives the absolute time which better accounts for network time as compared to time since. It can also be older than 60 secs and presently there is no field that can display the original time of creation once the error entry is updated.
      The reason of timestamp_last_updated field is that it provides a more precise value for the “last time” an error was seen where as, presently it is only in the 60 second period.
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      ac312954
    • Harkrishn Patro's avatar
      Propagate message to a node only if the cluster link is healthy. (#11752) · fd397568
      Harkrishn Patro authored
      Currently while a sharded pubsub message publish tries to propagate the message across the cluster, a NULL check is missing for clusterLink. clusterLink could be NULL if the link is causing memory beyond the set threshold cluster-link-sendbuf-limit and server terminates the link.
      
      This change introduces two things:
      
      Avoids the engine crashes on the publishing node if a message is tried to be sent to a node and the link is NULL.
      Adds a debugging tool CLUSTERLINK KILL to terminate the clusterLink between two nodes.
      fd397568
  23. 01 Feb, 2023 1 commit
    • Binbin's avatar
      Fix handshake timeout replication test race (#11773) · ffb691f6
      Binbin authored
      Test on x86 + TLS fail with this error:
      ```
      *** [err]: Slave is able to detect timeout during handshake in tests/integration/replication.tcl
      Replica is not able to detect timeout
      ```
      
      The replica logs is:
      ```
       ### Starting test Slave is able to detect timeout during handshake in tests/integration/replication.tcl
      7681:S 05 Jan 2023 00:21:56.635 * Non blocking connect for SYNC fired the event.
      7681:S 05 Jan 2023 00:21:56.638 * Master replied to PING, replication can continue...
      7681:S 05 Jan 2023 00:21:56.638 * Trying a partial resynchronization (request ef70638885500aad12dd673c68ca1541116a59fe:1).
      7681:S 05 Jan 2023 00:22:56.894 # Failed to read response from the server: error:0A000126:SSL routines::unexpected eof while reading
      7681:S 05 Jan 2023 00:22:56.894 # Master did not reply to PSYNC, will try later
      ```
      
      This is another issue that appeared after #11640 was merged. This PR try to fix it.
      The idea is to make it stable in `wait_bgsave`, for example, it may wait until the
      next psync retry in the following situation: `Master did not reply to PSYNC, will try later`
      
      Other than that, the change will make the test more consistent / predictable since
      it'll mean the master is always frozen in the desired state (waiting for repl-diskless-sync-delay
      to happen, rather than earlier stages of the handshake).
      ffb691f6