From 877b7be579fb01877d99a503d441d3f3028cce67 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Wed, 25 Oct 2017 13:19:43 +1100
Subject: [PATCH] FIX: in readonly mode don't double count pages

---
 app/models/application_request.rb       | 19 +++++++--------
 lib/discourse_redis.rb                  |  9 +++++++
 spec/components/discourse_redis_spec.rb |  5 ++++
 spec/models/application_request_spec.rb | 32 +++++++++++++++++++++++--
 4 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/app/models/application_request.rb b/app/models/application_request.rb
index 1f1f758d956..cb9334ac10b 100644
--- a/app/models/application_request.rb
+++ b/app/models/application_request.rb
@@ -1,3 +1,4 @@
+# frozen_string_literal: true
 class ApplicationRequest < ActiveRecord::Base
   enum req_type: %i(http_total
                     http_2xx
@@ -36,6 +37,12 @@ class ApplicationRequest < ActiveRecord::Base
     end
   end
 
+  GET_AND_RESET = <<~LUA
+    local val = redis.call('get', KEYS[1])
+    redis.call('set', KEYS[1], '0')
+    return val
+  LUA
+
   def self.write_cache!(date = nil)
     if date.nil?
       write_cache!(Time.now.utc)
@@ -51,20 +58,12 @@ class ApplicationRequest < ActiveRecord::Base
     # for concurrent calls without double counting
     req_types.each do |req_type, _|
       key = redis_key(req_type, date)
-      val = $redis.get(key).to_i
 
+      namespaced_key = $redis.namespace_key(key)
+      val = $redis.eval(GET_AND_RESET, keys: [namespaced_key]).to_i
       next if val == 0
 
-      new_val = $redis.incrby(key, -val).to_i
-
-      if new_val < 0
-        # undo and flush next time
-        $redis.incrby(key, val)
-        next
-      end
-
       id = req_id(date, req_type)
-
       where(id: id).update_all(["count = count + ?", val])
     end
   end
diff --git a/lib/discourse_redis.rb b/lib/discourse_redis.rb
index 698294f4763..49f0a3ec1d4 100644
--- a/lib/discourse_redis.rb
+++ b/lib/discourse_redis.rb
@@ -159,6 +159,7 @@ class DiscourseRedis
 
       fallback_handler.verify_master if !fallback_handler.master
       Discourse.received_readonly!
+      nil
     else
       raise ex
     end
@@ -231,6 +232,14 @@ class DiscourseRedis
     @redis.client.reconnect
   end
 
+  def namespace_key(key)
+    if @namespace
+      "#{namespace}:#{key}"
+    else
+      key
+    end
+  end
+
   def namespace
     RailsMultisite::ConnectionManagement.current_db
   end
diff --git a/spec/components/discourse_redis_spec.rb b/spec/components/discourse_redis_spec.rb
index 9067ceedbeb..c1b94859f9e 100644
--- a/spec/components/discourse_redis_spec.rb
+++ b/spec/components/discourse_redis_spec.rb
@@ -10,6 +10,11 @@ describe DiscourseRedis do
 
   let(:fallback_handler) { DiscourseRedis::FallbackHandler.instance }
 
+  it "ignore_readonly returns nil from a pure exception" do
+    result = DiscourseRedis.ignore_readonly { raise Redis::CommandError.new("READONLY") }
+    expect(result).to eq(nil)
+  end
+
   describe 'redis commands' do
     let(:raw_redis) { Redis.new(DiscourseRedis.config) }
 
diff --git a/spec/models/application_request_spec.rb b/spec/models/application_request_spec.rb
index 78488de3dec..3f3e215bf1f 100644
--- a/spec/models/application_request_spec.rb
+++ b/spec/models/application_request_spec.rb
@@ -2,6 +2,7 @@ require 'rails_helper'
 
 describe ApplicationRequest do
   before do
+    ApplicationRequest.last_flush = Time.now.utc
     ApplicationRequest.clear_cache!
   end
 
@@ -13,19 +14,46 @@ describe ApplicationRequest do
     ApplicationRequest.increment!(key, opts)
   end
 
+  def disable_date_flush!
+    freeze_time(Time.now)
+    ApplicationRequest.last_flush = Time.now.utc
+  end
+
+  it 'works even if redis is in readonly' do
+    disable_date_flush!
+
+    inc(:http_total)
+    inc(:http_total)
+
+    $redis.slaveof("127.0.0.1", 666)
+
+    # flush will be deferred no error raised
+    inc(:http_total, autoflush: 3)
+    $redis.slaveof("no", "one")
+
+    inc(:http_total, autoflush: 3)
+    expect(ApplicationRequest.http_total.first.count).to eq(3)
+  end
+
   it 'logs nothing for an unflushed increment' do
     ApplicationRequest.increment!(:anon)
     expect(ApplicationRequest.count).to eq(0)
   end
 
   it 'can automatically flush' do
-    t1 = Time.now.utc.at_midnight
-    freeze_time(t1)
+    disable_date_flush!
+
     inc(:http_total)
     inc(:http_total)
     inc(:http_total, autoflush: 3)
 
     expect(ApplicationRequest.http_total.first.count).to eq(3)
+
+    inc(:http_total)
+    inc(:http_total)
+    inc(:http_total, autoflush: 3)
+
+    expect(ApplicationRequest.http_total.first.count).to eq(6)
   end
 
   it 'can flush based on time' do