diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index 6cae36c2d9ed9c387eefcd424441941e453ed553..f0172c5159dd81062721d9fb50a0f6743c58106a 100644
--- a/src/metabase/models/setting.clj
+++ b/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)
diff --git a/src/metabase/models/setting/cache.clj b/src/metabase/models/setting/cache.clj
index a3578389a59415e70d1ecf1566f24f780d7d426e..68daac579057cc8121adf259d84c2b968da1c191 100644
--- a/src/metabase/models/setting/cache.clj
+++ b/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
diff --git a/test/metabase/models/setting/cache_test.clj b/test/metabase/models/setting/cache_test.clj
index 1cc922cd229fc47255d985ebe037ef1b75e2d32d..0aa585844e80a12fcd3bcd985d7a23fb0263cbe0 100644
--- a/test/metabase/models/setting/cache_test.clj
+++ b/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)))