From d5cd669464097bb172ea73855cd94f21a20d8404 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Tue, 30 Jul 2024 09:08:12 +0200
Subject: [PATCH] FIX: race condition in Discourse.cache.fetch (#28124)

When using `Discourse.cache.fetch` with an expiry, there's a potential for a race condition due to how we read the data from redis.

The code used to be

```ruby
raw = redis.get(key) if !force
entry = read_entry(key) if raw
return entry if raw && !(entry == :__corrupt_cache__)
```

with `read_entry` defined as follow

```ruby
def read_entry(key)
  if data = redis.get(key)
    Marshal.load(data)
  end
rescue => e
  :__corrupt_cache__
end
```

If the value at "key" expired in redis between `raw = redis.get` and `entry = read_entry`, the `entry` variable would be `nil` despite `raw` having a value.

We would then proceed to return `entry` (which is `nil`) thinking it had a value, when it didn't.

The first `redis.get` can be skipped altogether and we can rely only on `read_entry` to read the data from redis. Thus avoiding the race condition and removing the double read operations.

Internal ref - t/132507
---
 lib/cache.rb           | 22 +++++++++++++++-------
 spec/lib/cache_spec.rb | 10 ++++++++++
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/lib/cache.rb b/lib/cache.rb
index 313fc418a83..15cd79e6047 100644
--- a/lib/cache.rb
+++ b/lib/cache.rb
@@ -82,9 +82,13 @@ class Cache
     end
 
     key = normalize_key(name)
-    raw = redis.get(key) if !force
-    entry = read_entry(key) if raw
-    return entry if raw && !(entry == :__corrupt_cache__)
+
+    if !force
+      if raw = redis.get(key)
+        entry = decode_entry(raw, key)
+        return entry if entry != :__corrupt_cache__
+      end
+    end
 
     val = blk.call
     write_entry(key, val, expires_in: expires_in)
@@ -99,10 +103,8 @@ class Cache
     Discourse.warn_exception(e, message: "Corrupt cache... skipping entry for key #{key}")
   end
 
-  def read_entry(key)
-    if data = redis.get(key)
-      Marshal.load(data) # rubocop:disable Security/MarshalLoad
-    end
+  def decode_entry(raw, key)
+    Marshal.load(raw) # rubocop:disable Security/MarshalLoad
   rescue => e
     # corrupt cache, this can happen if Marshal version
     # changes. Log it once so we can tell it is happening.
@@ -112,6 +114,12 @@ class Cache
     :__corrupt_cache__
   end
 
+  def read_entry(key)
+    if data = redis.get(key)
+      decode_entry(data, key)
+    end
+  end
+
   def write_entry(key, value, expires_in: nil)
     dumped = Marshal.dump(value)
     expiry = expires_in || MAX_CACHE_AGE
diff --git a/spec/lib/cache_spec.rb b/spec/lib/cache_spec.rb
index 96d497ebeb4..4f08048a64d 100644
--- a/spec/lib/cache_spec.rb
+++ b/spec/lib/cache_spec.rb
@@ -121,5 +121,15 @@ RSpec.describe Cache do
         expect(cache.read("my_key")).to eq("bob")
       end
     end
+
+    it "isn't prone to a race condition due to key expiring between GET calls" do
+      key = cache.normalize_key("my_key")
+
+      # while this is not technically testing the race condition, it's
+      # ensuring we're only calling redis.get once, which is a good enough proxy
+      Discourse.redis.stubs(:get).with(key).returns(Marshal.dump("bob")).once
+
+      expect(fetch_value).to eq("bob")
+    end
   end
 end