Verified Commit e523484f authored by Bob Van Landuyt's avatar Bob Van Landuyt 💬 Committed by GitLab
Browse files

Merge branch 'fix/rate-limit-noscript-rescue' into 'master' 

fix(redis): catch RedisClient::CommandError in Script EVALSHA rescue

Closes gitlab-com/gl-infra/production-engineering#29097

See merge request !299

Merged-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
Approved-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
Reviewed-by: default avatarGitLab Duo <gitlab-duo@gitlab.com>
Co-authored-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
parents b94e1223 5f810066

lib/labkit/redis/script.rb

+7 −1
Original line number Diff line number Diff line
@@ -33,7 +33,13 @@ module Labkit 
      # @return the script's return value

      def eval(conn, keys:, argv:)

        conn.evalsha(@sha, keys: keys, argv: argv)

      rescue ::Redis::CommandError => e

      # Redis::CommandError and RedisClient::CommandError are sibling class trees

      # with no common ancestor above StandardError -- neither inherits from the

      # other. A NOSCRIPT response arrives as Redis::* on single-instance

      # connections (the redis gem's translator runs) and as RedisClient::* on

      # cluster connections when the cluster translator skips the conversion.

      # Both shapes must reach the EVAL fallback, so we list both classes.

      rescue ::Redis::CommandError, ::RedisClient::CommandError => e

        raise unless e.message.start_with?("NOSCRIPT")



        conn.eval(@body, keys: keys, argv: argv)

spec/labkit/redis/script_spec.rb

+30 −0
Original line number Diff line number Diff line
@@ -114,6 +114,25 @@ RSpec.describe Labkit::Redis::Script do 
      expect(conn.eval_calls).to eq(1)

    end



    # When the connection is a Redis Cluster client, the underlying RedisClient::NoScriptError

    # can surface without being translated to Redis::NoScriptError. The rescue must cover both

    # exception hierarchies. See https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/issues/29097.

    it "falls through to EVAL when EVALSHA raises a RedisClient NOSCRIPT error" do

      conn = spy_conn_class.new(

        evalsha_responses: [

          -> { raise RedisClient::CommandError, "NOSCRIPT No matching script. Please use EVAL." },

          1

        ],

        eval_responses: [1]

      )



      script.eval(conn, keys: [], argv: [])

      script.eval(conn, keys: [], argv: [])



      expect(conn.evalsha_calls).to eq(2)

      expect(conn.eval_calls).to eq(1)

    end



    it "propagates non-NOSCRIPT Redis errors instead of swallowing them" do

      conn = spy_conn_class.new(

        evalsha_responses: [
@@ -124,5 +143,16 @@ RSpec.describe Labkit::Redis::Script do 
      expect { script.eval(conn, keys: [], argv: []) }.to raise_error(Redis::CommandError, /WRONGTYPE/)

      expect(conn.eval_calls).to eq(0)

    end



    it "propagates non-NOSCRIPT RedisClient errors instead of swallowing them" do

      conn = spy_conn_class.new(

        evalsha_responses: [

          -> { raise RedisClient::CommandError, "WRONGTYPE Operation against a key holding the wrong kind of value" }

        ]

      )



      expect { script.eval(conn, keys: [], argv: []) }.to raise_error(RedisClient::CommandError, /WRONGTYPE/)

      expect(conn.eval_calls).to eq(0)

    end

  end

end