Handle long running sync in connection pool hash check (#18664)

* Handle long running sync in connection pool hash check

Modify the logic in `db->pooled-connection-spec` to check for the latest `DatabaseInstance` from the app DB in case of a hash mismatch, before deciding that the pool needs to be invalidated (based on changing hash of db details)

This is to handle the case where a long running sync keeps passing in a stale version of the `DatabaseInstance` over a period of many minutes/hours, long after the app DB has received an updated version that was saved through the UI

In this case, the connection pool code will get the latest `DatabaseInstance` from the app DB, on a hash mismatch, then compare that against the in-memory has for the given DB ID.  And if it still doesn't match, then the invalidation kicks in

Updating the test accordingly

src/metabase/driver/sql_jdbc/connection.clj

@@ -172,18 +172,23 @@
     (let [database-id (u/the-id db-or-id-or-spec)
           ;; we need the Database instance no matter what (in order to compare details hash with cached value)
           db          (or (and (instance? (type Database) db-or-id-or-spec) db-or-id-or-spec) ; passed in
-                        (db/select-one [Database :id :engine :details] :id database-id)       ; look up by ID
-                        (throw (ex-info (tru "Database {0} does not exist." database-id)
-                                 {:status-code 404
-                                  :type        qp.error-type/invalid-query
-                                  :database-id database-id})))
+                          (db/select-one [Database :id :engine :details] :id database-id)     ; look up by ID
+                          (throw (ex-info (tru "Database {0} does not exist." database-id)
+                                   {:status-code 404
+                                    :type        qp.error-type/invalid-query
+                                    :database-id database-id})))
           get-fn      (fn [db-id log-invalidation?]
                         (when-let [details (get @database-id->connection-pool db-id)]
                           (cond
                             ;; details hash changed from what is cached; invalid
                             (let [curr-hash (get @database-id->db-details-hashes db-id)
                                   new-hash  (db-details-hash db)]
-                              (and (some? curr-hash) (not= curr-hash new-hash)))
+                              (when (and (some? curr-hash) (not= curr-hash new-hash))
+                                ;; the hash didn't match, but it's possible that a stale instance of `DatabaseInstance`
+                                ;; was passed in (ex: from a long-running sync operation); fetch the latest one from
+                                ;; our app DB, and see if it STILL doesn't match
+                                (not= curr-hash (-> (db/select-one [Database :id :engine :details] :id database-id)
+                                                    db-details-hash))))
                             (if log-invalidation?
                               (log-db-details-hash-change-msg! db-id)
                               nil)

test/metabase/driver/sql_jdbc/connection_test.clj

@@ -7,10 +7,12 @@
             [metabase.driver.sql-jdbc.test-util :as sql-jdbc.tu]
             [metabase.driver.util :as driver.u]
             [metabase.models.database :refer [Database]]
+            [metabase.sync :as sync]
             [metabase.test :as mt]
             [metabase.test.data :as data]
             [metabase.test.fixtures :as fixtures]
-            [metabase.util :as u]))
+            [metabase.util :as u]
+            [toucan.db :as db]))
 
 (use-fixtures :once (fixtures/initialize :db))
 
@@ -76,39 +78,65 @@
 (deftest connection-pool-invalidated-on-details-change
   (mt/test-drivers (sql-jdbc.tu/sql-jdbc-drivers)
     (testing "db->pooled-connection-spec marks a connection pool invalid if the db details map changes"
-      (let [db                 (mt/db)
-            hash-change-called (atom false)
-            hash-change-fn     (fn [db-id]
-                                 (is (= (u/the-id db) db-id))
-                                 (reset! hash-change-called true)
-                                 nil)
-            perturb-db-details (fn [db]
-                                 (update db :details (fn [details]
-                                                       (cond-> details
-                                                         ;; swap localhost and 127.0.0.1
-                                                         (= "localhost" (:host details))
-                                                         (assoc :host "127.0.0.1")
+      (let [db                       (mt/db)
+            hash-change-called-times (atom 0)
+            hash-change-fn           (fn [db-id]
+                                       (is (= (u/the-id db) db-id))
+                                       (swap! hash-change-called-times inc)
+                                       nil)
+            perturb-db-details       (fn [db]
+                                       (update db
+                                               :details
+                                               (fn [details]
+                                                 (case driver/*driver*
+                                                   :redshift
+                                                   (assoc details :additional-options "defaultRowFetchSize=1000")
 
-                                                         (= "127.0.0.1" (:host details))
-                                                         (assoc :host "localhost")
+                                                   (cond-> details
+                                                     ;; swap localhost and 127.0.0.1
+                                                     (= "localhost" (:host details))
+                                                     (assoc :host "127.0.0.1")
 
-                                                         :else
-                                                         (assoc :new-config "something")))))]
-        (sql-jdbc.conn/invalidate-pool-for-db! db)
-        ;; a little bit hacky to redefine the log fn, but it's the most direct way to test
-        (with-redefs [sql-jdbc.conn/log-db-details-hash-change-msg! hash-change-fn]
-          (let [pool-spec-1 (sql-jdbc.conn/db->pooled-connection-spec db)
-                db-hash-1   (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
-            (testing "hash value calculated correctly for new pooled conn"
-              (is (some? pool-spec-1))
-              (is (integer? db-hash-1))
-              (is (not= db-hash-1 0)))
-            (testing "changing DB details results in hash value changing and connection being invalidated"
-              (let [db-perturbed (perturb-db-details db)
-                    pool-spec-2  (sql-jdbc.conn/db->pooled-connection-spec db-perturbed)
-                    db-hash-2    (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
-                (is (some? pool-spec-2))
-                (is (true? @hash-change-called))
-                (is (integer? db-hash-2))
-                (is (not= db-hash-2 0))
-                (is (not= db-hash-1 db-hash-2))))))))))
+                                                     (= "127.0.0.1" (:host details))
+                                                     (assoc :host "localhost")
+
+                                                     :else
+                                                     (assoc :new-config "something"))))))]
+        (try
+          (sql-jdbc.conn/invalidate-pool-for-db! db)
+          ;; a little bit hacky to redefine the log fn, but it's the most direct way to test
+          (with-redefs [sql-jdbc.conn/log-db-details-hash-change-msg! hash-change-fn]
+            (let [pool-spec-1 (sql-jdbc.conn/db->pooled-connection-spec db)
+                  db-hash-1   (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
+              (testing "hash value calculated correctly for new pooled conn"
+                (is (some? pool-spec-1))
+                (is (integer? db-hash-1))
+                (is (not= db-hash-1 0)))
+              (testing "changing DB details results in hash value changing and connection being invalidated"
+                (let [db-perturbed (perturb-db-details db)]
+                  (db/update! Database (mt/id) :details (:details db-perturbed))
+                  (let [;; this call should result in the connection pool becoming invalidated, and the new hash value
+                        ;; being stored based upon these updated details
+                        pool-spec-2  (sql-jdbc.conn/db->pooled-connection-spec db-perturbed)
+                        db-hash-2    (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
+                    ;; to throw a wrench into things, kick off a sync of the original db (unperturbed); this
+                    ;; simulates a long running sync that began before the perturbed details were saved to the app DB
+                    ;; the sync steps SHOULD NOT invalidate the connection pool, because doing so could cause a seesaw
+                    ;; effect that continuously invalidates the connection pool on every sync step and query, which
+                    ;; wreaks havoc (#18499)
+                    ;; instead, the connection pool code will simply fetch the newest DatabaseInstance it
+                    ;; can find in the app DB, in the case of a hash mismatch, and check AGAIN to see whether the hash
+                    ;; still doesn't match (in this test case, it should actually match this time, because we updated
+                    ;; the app DB with the perturbed DatabaseInstance above here)
+                    ;; this should still see a hash mismatch in the case that the DB details were updated external to
+                    ;; this process (i.e. by a different instance), since our in-memory hash value still wouldn't match
+                    ;; even after getting the latest `DatabaseInstance`
+                    (sync/sync-database! db {:scan :schema})
+                    (is (some? pool-spec-2))
+                    (is (= 1 @hash-change-called-times))
+                    (is (integer? db-hash-2))
+                    (is (not= db-hash-2 0))
+                    (is (not= db-hash-1 db-hash-2)))))))
+          (finally
+            ;; restore the original test DB details, no matter what just happened
+            (db/update! Database (mt/id) :details (:details db))))))))