fix setting cache synchronization issue (#10661)

* fix cache timing bug

subtle issue where timing of reads and writes could fool a server into thinking it had the latest values when it didn't

src/metabase/models/setting.clj

@@ -262,7 +262,7 @@
     (if obfuscated?
       (log/info (trs "Attempted to set Setting {0} to obfuscated value. Ignoring change." setting-name))
       (do
-        (cache/restore-cache-if-needed!)
+        (cache/restore-cache!)
         ;; write to DB
         (cond
           (nil? new-value)

src/metabase/models/setting/cache.clj

@@ -86,21 +86,22 @@
       to invalidate our cache.)"
   []
   (log/debug (trs "Checking whether settings cache is out of date (requires DB call)..."))
-  (boolean
-   (or
-    ;; is the cache empty?
-    (not @cache*)
-    ;; if not, get the cached value of `settings-last-updated`, and if it exists...
-    (when-let [last-known-update (core/get @cache* settings-last-updated-key)]
-      ;; compare it to the value in the DB. This is done be seeing whether a row exists
-      ;; WHERE value > <local-value>
-      (u/prog1 (db/select-one 'Setting
-                 {:where [:and
-                          [:= :key settings-last-updated-key]
-                          [:> :value last-known-update]]})
-        (when <>
-          (log/info (u/format-color 'red
-                        (trs "Settings have been changed on another instance, and will be reloaded here.")))))))))
+  (let [current-cache (cache)]
+    (boolean
+      (or
+        ;; is the cache empty?
+        (not current-cache)
+        ;; if not, get the cached value of `settings-last-updated`, and if it exists...
+        (when-let [last-known-update (core/get current-cache settings-last-updated-key)]
+          ;; compare it to the value in the DB. This is done be seeing whether a row exists
+          ;; WHERE value > <local-value>
+          (u/prog1 (db/select-one 'Setting
+                                  {:where [:and
+                                           [:= :key settings-last-updated-key]
+                                           [:> :value last-known-update]]})
+                   (when <>
+                     (log/info (u/format-color 'red
+                                               (trs "Settings have been changed on another instance, and will be reloaded here."))))))))))
 
 (def ^:private cache-update-check-interval-ms
   "How often we should check whether the Settings cache is out of date (which requires a DB call)?"
@@ -114,6 +115,12 @@
 
 (def ^:private restore-cache-if-needed-lock (Object.))
 
+(defn restore-cache!
+  "Populate cache with the latest hotness from the db"
+  []
+  (log/debug (trs "Refreshing Settings cache..."))
+  (reset! cache* (db/select-field->field :key :value 'Setting)))
+
 (defn restore-cache-if-needed!
   "Check whether we need to repopulate the cache with fresh values from the DB (because the cache is either empty or
   known to be out-of-date), and do so if needed. This is intended to be called every time a Setting value is
@@ -129,8 +136,7 @@
   ;; certainly quicker than starting the task ourselves from scratch
   (locking restore-cache-if-needed-lock
     (when (should-restore-cache?)
-      (log/debug (trs "Refreshing Settings cache..."))
-      (reset! cache* (db/select-field->field :key :value 'Setting))
+      (restore-cache!)
       ;; Now the cache is up-to-date. That is all good, but if we call `should-restore-cache?` again in a second it
       ;; will still return `true`, because its result is memoized, and we would be on the hook to (again) update the
       ;; cache. So go ahead and clear the memozied results for `should-restore-cache?`. The next time around when

test/metabase/models/setting/cache_test.clj

@@ -30,7 +30,9 @@
                        :postgres "cast((current_timestamp + interval '1 second') AS text)"))))
 
 (defn- simulate-another-instance-updating-setting! [setting-name new-value]
-  (db/update-where! Setting {:key (name setting-name)} :value new-value)
+  (if new-value
+    (db/update-where! Setting {:key (name setting-name)} :value new-value)
+    (db/simple-delete! Setting {:key (name setting-name)}))
   (update-settings-last-updated-value-in-db!))
 
 (defn- flush-memoized-results-for-should-restore-cache!
@@ -122,3 +124,30 @@
     ;; detect a cache out-of-date situation and flush the cache as appropriate, giving us the updated value when we
     ;; call! :wow:
     (setting-test/toucan-name)))
+
+
+;; Simulate experience where:
+;; 1. User writes a setting on Server 1
+;; 2. User reads settings, served from Server 1, memoizing theresult of should-restore-cache?
+;; 3. User writes setting :toucan-name on Server 2
+;; 4. User writes setting on Server 1
+;; 5. User reads setting :toucan-name from Server 1
+;;
+;; This process was causing the updated `:toucan-name` to never be read on Server 1 because Server 1 "thought" it had
+;; the latest values and didn't restore the cache from the db
+(expect
+  "Batman Toucan"
+  (let [internal-cache       @#'metabase.models.setting.cache/cache*
+        external-cache       (atom nil)
+        external-cache-check (memoize/ttl (constantly nil) :ttl/threshold 60000)]
+    (clear-cache!)
+    (flush-memoized-results-for-should-restore-cache!)
+    (setting-test/test-setting-1 "Starfish")
+    ;; Call this to force memoization, simulating process of:
+    ;; 1. User writes
+    (@#'metabase.models.setting.cache/should-restore-cache?)
+    (with-redefs [metabase.models.setting.cache/cache*                external-cache
+                  metabase.models.setting.cache/should-restore-cache? external-cache-check]
+      (setting-test/toucan-name "Batman Toucan"))
+    (setting-test/test-setting-1 "Batman")
+    (setting-test/toucan-name)))