FIX: Fallback Redis by checking status on master instead of slave.

lib/discourse_redis.rb

@@ -7,7 +7,9 @@ class DiscourseRedis
   class FallbackHandler
     include Singleton
 
-    MASTER_LINK_STATUS = "master_link_status:up".freeze
+    MASTER_ROLE_STATUS = "role:master".freeze
+    MASTER_LOADING_STATUS = "loading:1".freeze
+    MASTER_LOADED_STATUS = "loading:0".freeze
     CONNECTION_TYPES = %w{normal pubsub}.each(&:freeze)
 
     def initialize
@@ -39,25 +41,31 @@ class DiscourseRedis
       success = false
 
       begin
-        slave_client = ::Redis::Client.new(@slave_config)
+        master_client = ::Redis::Client.new(DiscourseRedis.config)
         logger.warn "#{log_prefix}: Checking connection to master server..."
+        info = master_client.call([:info])
 
-        if slave_client.call([:info]).split("\r\n").include?(MASTER_LINK_STATUS)
-          logger.warn "#{log_prefix}: Master server is active, killing all connections to slave..."
+        if info.include?(MASTER_LOADED_STATUS) && info.include?(MASTER_ROLE_STATUS)
+          begin
+            logger.warn "#{log_prefix}: Master server is active, killing all connections to slave..."
 
-          self.master = true
+            self.master = true
+            slave_client = ::Redis::Client.new(@slave_config)
 
-          CONNECTION_TYPES.each do |connection_type|
-            slave_client.call([:client, [:kill, 'type', connection_type]])
+            CONNECTION_TYPES.each do |connection_type|
+              slave_client.call([:client, [:kill, 'type', connection_type]])
+            end
+
+            MessageBus.keepalive_interval = @message_bus_keepalive_interval
+            Discourse.clear_readonly!
+            Discourse.request_refresh!
+            success = true
+          ensure
+            slave_client&.disconnect
           end
-
-          MessageBus.keepalive_interval = @message_bus_keepalive_interval
-          Discourse.clear_readonly!
-          Discourse.request_refresh!
-          success = true
         end
       ensure
-        slave_client.disconnect
+        master_client&.disconnect
       end
 
       success
@@ -108,7 +116,11 @@ class DiscourseRedis
         options = @options.dup
         options.delete(:connector)
         client ||= Redis::Client.new(options)
-        loading = client.call([:info]).split("\r\n").include?("loading:1")
+
+        loading = client.call([:info, :persistence]).include?(
+          DiscourseRedis::FallbackHandler::MASTER_LOADING_STATUS
+        )
+
         loading ? @slave_options : @options
       rescue Redis::ConnectionError, Redis::CannotConnectError, RuntimeError => ex
         raise ex if ex.class == RuntimeError && ex.message != "Name or service not known"

spec/components/discourse_redis_spec.rb

@@ -165,8 +165,11 @@ describe DiscourseRedis do
     it "should return the slave config when master is still loading data" do
       Redis::Client.any_instance
         .expects(:call)
-        .with([:info])
-        .returns("someconfig:haha\r\nloading:1")
+        .with([:info, :persistence])
+        .returns("
+          someconfig:haha\r
+          #{DiscourseRedis::FallbackHandler::MASTER_LOADING_STATUS}
+        ")
 
       config = connector.resolve
 
@@ -205,16 +208,32 @@ describe DiscourseRedis do
 
       it 'should fallback to the master server once it is up' do
         fallback_handler.master = false
-        redis_connection = mock('test')
-        Redis::Client.expects(:new).with(DiscourseRedis.slave_config).returns(redis_connection)
+        master_conn = mock('master')
+        slave_conn = mock('slave')
 
-        redis_connection.expects(:call).with([:info]).returns(DiscourseRedis::FallbackHandler::MASTER_LINK_STATUS)
+        Redis::Client.expects(:new)
+          .with(DiscourseRedis.config)
+          .returns(master_conn)
+
+        Redis::Client.expects(:new)
+          .with(DiscourseRedis.slave_config)
+          .returns(slave_conn)
+
+        master_conn.expects(:call)
+          .with([:info])
+          .returns("
+            #{DiscourseRedis::FallbackHandler::MASTER_ROLE_STATUS}\r\n
+            #{DiscourseRedis::FallbackHandler::MASTER_LOADED_STATUS}
+          ")
 
         DiscourseRedis::FallbackHandler::CONNECTION_TYPES.each do |connection_type|
-          redis_connection.expects(:call).with([:client, [:kill, 'type', connection_type]])
+          slave_conn.expects(:call).with(
+            [:client, [:kill, 'type', connection_type]]
+          )
         end
 
-        redis_connection.expects(:disconnect)
+        master_conn.expects(:disconnect)
+        slave_conn.expects(:disconnect)
 
         expect(fallback_handler.initiate_fallback_to_master).to eq(true)
         expect(fallback_handler.master).to eq(true)