Merge pull request #5019 from tgxworld/more_resiliency_to_readonly_redis

Fix Redis command errors when trying to start app with a readonly Redis.

app/models/global_setting.rb

@@ -48,6 +48,8 @@ class GlobalSetting
       end
       token
     end
+  rescue Redis::CommandError => e
+    @safe_secret_key_base = SecureRandom.hex(64) if e.message =~ /READONLY/
   end
 
   def self.load_defaults

config/initializers/006-mini_profiler.rb

@@ -19,7 +19,9 @@ if defined?(Rack::MiniProfiler)
   #   raw_connection means results are not namespaced
   #
   # namespacing gets complex, cause mini profiler is in the rack chain way before multisite
-  Rack::MiniProfiler.config.storage_instance = Rack::MiniProfiler::RedisStore.new(connection:  DiscourseRedis.raw_connection)
+  Rack::MiniProfiler.config.storage_instance = Rack::MiniProfiler::RedisStore.new(
+    connection:  DiscourseRedis.new(nil, namespace: false)
+  )
 
   skip = [
     /^\/message-bus/,

lib/discourse_redis.rb

@@ -135,9 +135,10 @@ class DiscourseRedis
     options.dup.merge!(host: options[:slave_host], port: options[:slave_port])
   end
 
-  def initialize(config = nil)
+  def initialize(config = nil, namespace: true)
     @config = config || DiscourseRedis.config
     @redis = DiscourseRedis.raw_connection(@config)
+    @namespace = namespace
   end
 
   def self.fallback_handler
@@ -183,29 +184,35 @@ class DiscourseRedis
    :sunion, :ttl, :type, :watch, :zadd, :zcard, :zcount, :zincrby, :zrange, :zrangebyscore, :zrank, :zrem, :zremrangebyrank,
    :zremrangebyscore, :zrevrange, :zrevrangebyscore, :zrevrank, :zrangebyscore].each do |m|
     define_method m do |*args|
-      args[0] = "#{namespace}:#{args[0]}"
+      args[0] = "#{namespace}:#{args[0]}" if @namespace
       DiscourseRedis.ignore_readonly { @redis.send(m, *args) }
     end
   end
 
   def mget(*args)
-    args.map! { |a| "#{namespace}:#{a}" }
+    args.map! { |a| "#{namespace}:#{a}" }  if @namespace
     DiscourseRedis.ignore_readonly { @redis.mget(*args) }
   end
 
   def del(k)
     DiscourseRedis.ignore_readonly do
-      k = "#{namespace}:#{k}"
+      k = "#{namespace}:#{k}"  if @namespace
       @redis.del k
     end
   end
 
   def keys(pattern = nil)
     DiscourseRedis.ignore_readonly do
-      len = namespace.length + 1
-      @redis.keys("#{namespace}:#{pattern || '*'}").map {
-        |k| k[len..-1]
-      }
+      pattern = pattern || '*'
+      pattern = "#{namespace}:#{pattern}" if @namespace
+      keys = @redis.keys(pattern)
+
+      if @namespace
+        len = namespace.length + 1
+        keys.map! { |k| k[len..-1] }
+      end
+
+      keys
     end
   end
 

spec/components/discourse_redis_spec.rb

@@ -10,6 +10,70 @@ describe DiscourseRedis do
 
   let(:fallback_handler) { DiscourseRedis::FallbackHandler.instance }
 
+  describe 'redis commands' do
+    let(:raw_redis) { Redis.new(DiscourseRedis.config) }
+
+    before do
+      raw_redis.flushall
+    end
+
+    after do
+      raw_redis.flushall
+    end
+
+    describe 'when namespace is enabled' do
+      let(:redis) { DiscourseRedis.new }
+
+      it 'should append namespace to the keys' do
+        redis.set('key', 1)
+
+        expect(raw_redis.get('default:key')).to eq('1')
+        expect(redis.keys).to eq(['key'])
+
+        redis.del('key')
+
+        expect(raw_redis.get('default:key')).to eq(nil)
+
+        raw_redis.set('default:key1', '1')
+        raw_redis.set('default:key2', '2')
+
+        expect(redis.mget('key1', 'key2')).to eq(['1', '2'])
+      end
+    end
+
+    describe 'when namespace is disabled' do
+      let(:redis) { DiscourseRedis.new(nil, namespace: false) }
+
+      it 'should not append any namespace to the keys' do
+        redis.set('key', 1)
+
+        expect(raw_redis.get('key')).to eq('1')
+        expect(redis.keys).to eq(['key'])
+
+        redis.del('key')
+
+        expect(raw_redis.get('key')).to eq(nil)
+
+        raw_redis.set('key1', '1')
+        raw_redis.set('key2', '2')
+
+        expect(redis.mget('key1', 'key2')).to eq(['1', '2'])
+      end
+
+      it 'should noop a readonly redis' do
+        expect(Discourse.recently_readonly?).to eq(false)
+
+        redis.without_namespace
+          .expects(:set)
+          .raises(Redis::CommandError.new("READONLY"))
+
+        redis.set('key', 1)
+
+        expect(Discourse.recently_readonly?).to eq(true)
+      end
+    end
+  end
+
   context '.slave_host' do
     it 'should return the right config' do
       slave_config = DiscourseRedis.slave_config(config)
@@ -22,9 +86,9 @@ describe DiscourseRedis do
     it 'should check the status of the master server' do
       begin
         fallback_handler.master = false
-        $redis.without_namespace.expects(:get).raises(Redis::CommandError.new("READONLY"))
+        $redis.without_namespace.expects(:set).raises(Redis::CommandError.new("READONLY"))
         fallback_handler.expects(:verify_master).once
-        $redis.get('test')
+        $redis.set('test', '1')
       ensure
         fallback_handler.master = true
       end