Remove memoization of active user counts setting (#50779) (#50788)

Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>

src/metabase/public_settings/premium_features.clj

@@ -59,34 +59,27 @@
 
 (declare premium-embedding-token)
 
-(def ^:private ^:const active-users-count-cache-ttl
-  "Amount of time before we re-fetch the count of active users."
-  (u/minutes->ms 5))
-
 ;; let's prevent the DB from getting slammed with calls to get the active user count, we only really need one in flight
 ;; at a time.
-(let [f        (fn []
-                 {:post [(integer? %)]}
-                 (log/debug (u/colorize :yellow "GETTING ACTIVE USER COUNT!"))
-                 (assert ((requiring-resolve 'metabase.db/db-is-set-up?)) "Metabase DB is not yet set up")
-                 ;; force this to use a new Connection, it seems to be getting called in situations where the Connection
-                 ;; is from a different thread and is invalid by the time we get to use it
-                 (let [result (binding [t2.conn/*current-connectable* nil]
-                                (t2/count :model/User :is_active true :type :personal))]
-                   (log/debug (u/colorize :green "=>") result)
-                   result))
-      memoized (memoize/ttl
-                f
-                :ttl/threshold (u/minutes->ms active-users-count-cache-ttl))
-      lock     (Object.)]
-  (defn- cached-active-users-count'
-    "Returns a count of users on the system, cached for 5 minutes."
+(let [f    (fn []
+             {:post [(integer? %)]}
+             (log/debug (u/colorize :yellow "GETTING ACTIVE USER COUNT!"))
+             (assert ((requiring-resolve 'metabase.db/db-is-set-up?)) "Metabase DB is not yet set up")
+             ;; force this to use a new Connection, it seems to be getting called in situations where the Connection
+             ;; is from a different thread and is invalid by the time we get to use it
+             (let [result (binding [t2.conn/*current-connectable* nil]
+                            (t2/count :model/User :is_active true :type :personal))]
+               (log/debug (u/colorize :green "=>") result)
+               result))
+      lock (Object.)]
+  (defn- locking-active-user-count
+    "Returns a count of users on the system"
     []
     (locking lock
-      (memoized))))
+      (f))))
 
-(defsetting cached-active-users-count
-  (deferred-tru "Cached number of active users. Refresh every 5 minutes.")
+(defsetting active-users-count
+  (deferred-tru "Number of active users")
   :visibility :admin
   :type       :integer
   :audit      :never
@@ -96,7 +89,7 @@
   :getter     (fn []
                 (if-not ((requiring-resolve 'metabase.db/db-is-set-up?))
                   0
-                  (cached-active-users-count'))))
+                  (locking-active-user-count))))
 
 (defn- token-status-url [token base-url]
   (when (seq token)
@@ -166,25 +159,24 @@
 
 (defn- fetch-token-and-parse-body
   [token base-url site-uuid]
-  (let [active-user-count (cached-active-users-count)]
-    (try
-      (dh/with-circuit-breaker *store-circuit-breaker*
-        (dh/with-timeout {:timeout-ms fetch-token-status-timeout-ms
-                          :interrupt? true}
-          (try (fetch-token-and-parse-body* token base-url site-uuid active-user-count)
-               (catch Exception e
-                 (throw e)))))
-      (catch dev.failsafe.TimeoutExceededException _e
-        {:valid         false
-         :status        (tru "Unable to validate token")
-         :error-details (tru "Token validation timed out.")})
-      (catch dev.failsafe.CircuitBreakerOpenException _e
-        {:valid         false
-         :status        (tru "Unable to validate token")
-         :error-details (tru "Token validation is currently unavailable.")})
-      ;; other exceptions are wrapped by Diehard in a FailsafeException. Unwrap them before rethrowing.
-      (catch dev.failsafe.FailsafeException e
-        (throw (.getCause e))))))
+  (try
+    (dh/with-circuit-breaker *store-circuit-breaker*
+      (dh/with-timeout {:timeout-ms fetch-token-status-timeout-ms
+                        :interrupt? true}
+        (try (fetch-token-and-parse-body* token base-url site-uuid (active-users-count))
+             (catch Exception e
+               (throw e)))))
+    (catch dev.failsafe.TimeoutExceededException _e
+      {:valid         false
+       :status        (tru "Unable to validate token")
+       :error-details (tru "Token validation timed out.")})
+    (catch dev.failsafe.CircuitBreakerOpenException _e
+      {:valid         false
+       :status        (tru "Unable to validate token")
+       :error-details (tru "Token validation is currently unavailable.")})
+    ;; other exceptions are wrapped by Diehard in a FailsafeException. Unwrap them before rethrowing.
+    (catch dev.failsafe.FailsafeException e
+      (throw (.getCause e)))))
 
 ;;;;;;;;;;;;;;;;;;;; Airgap Tokens ;;;;;;;;;;;;;;;;;;;;
 

src/metabase/troubleshooting.clj

@@ -47,5 +47,5 @@
    (when (premium-features/airgap-enabled)
      {:airgap-token       :enabled
       :max-users          (premium-features/max-users-allowed)
-      :current-user-count (premium-features/cached-active-users-count)
+      :current-user-count (premium-features/active-users-count)
       :valid-thru         (some-> (premium-features/premium-embedding-token) premium-features/fetch-token-status :valid-thru)})))

test/metabase/public_settings/premium_features_test.clj

@@ -39,7 +39,7 @@
   [token premium-features-response]
   (http-fake/with-fake-routes-in-isolation
     {{:address      (#'premium-features/token-status-url token @#'premium-features/token-check-url)
-      :query-params {:users      (str (#'premium-features/cached-active-users-count))
+      :query-params {:users      (str (#'premium-features/active-users-count))
                      :site-uuid  (public-settings/site-uuid-for-premium-features-token-checks)
                      :mb-version (:tag config/mb-version-info)}}
      (constantly premium-features-response)}
@@ -275,14 +275,10 @@
 (deftest active-users-count-setting-test
   (t2.with-temp/with-temp
     [User _ {:is_active false}]
-    ;; premium-features/active-users-count is cached so it could be make the test flaky
-    ;; rebinding to avoid caching
     (testing "returns the number of active users"
-      (with-redefs [premium-features/cached-active-users-count (fn []
-                                                                 (t2/count :core_user :is_active true))]
-        (is (= (t2/count :core_user :is_active true)
-               (premium-features/cached-active-users-count)))))
+      (is (= (t2/count :model/User :is_active true :type :personal)
+             (premium-features/active-users-count))))
 
     (testing "Default to 0 if db is not setup yet"
       (binding [mdb.connection/*application-db* {:status (atom nil)}]
-        (is (zero? (premium-features/cached-active-users-count)))))))
+        (is (zero? (premium-features/active-users-count)))))))