♻️ Refactor storage transaction management

backend/test/app/services_files_test.clj

@@ -174,6 +174,14 @@
                             :type :image
                             :metadata {:id (:id fmo1)}}}]})]
 
+
+
+      ;; If we launch gc-touched-task, we should have 4 items to freeze.
+      (let [task (:app.storage/gc-touched-task th/*system*)
+            res  (task {})]
+        (t/is (= 4 (:freeze res)))
+        (t/is (= 0 (:delete res))))
+
       ;; run the task immediately
       (let [task  (:app.tasks.file-media-gc/handler th/*system*)
             res   (task {})]
@@ -202,16 +210,22 @@
       (t/is (some? (sto/get-object storage (:media-id fmo1))))
       (t/is (some? (sto/get-object storage (:thumbnail-id fmo1))))
 
-      ;; but if we pass the touched gc task two of them should disappear
+      ;; now, we have deleted the unused file-media-object, if we
+      ;; execute the touched-gc task, we should see that two of them
+      ;; are marked to be deleted.
       (let [task (:app.storage/gc-touched-task th/*system*)
             res  (task {})]
         (t/is (= 0 (:freeze res)))
-        (t/is (= 2 (:delete res)))
+        (t/is (= 2 (:delete res))))
 
-        (t/is (nil? (sto/get-object storage (:media-id fmo2))))
-        (t/is (nil? (sto/get-object storage (:thumbnail-id fmo2))))
-        (t/is (some? (sto/get-object storage (:media-id fmo1))))
-        (t/is (some? (sto/get-object storage (:thumbnail-id fmo1)))))
+
+      ;; Finally, check that some of the objects that are marked as
+      ;; deleted we are unable to retrieve them using standard storage
+      ;; public api.
+      (t/is (nil? (sto/get-object storage (:media-id fmo2))))
+      (t/is (nil? (sto/get-object storage (:thumbnail-id fmo2))))
+      (t/is (some? (sto/get-object storage (:media-id fmo1))))
+      (t/is (some? (sto/get-object storage (:thumbnail-id fmo1))))
 
       )))
 

backend/test/app/services_management_test.clj

@@ -11,6 +11,7 @@
    [app.http :as http]
    [app.storage :as sto]
    [app.test-helpers :as th]
+   [app.storage-test :refer [configure-storage-backend]]
    [clojure.test :as t]
    [buddy.core.bytes :as b]
    [datoteka.core :as fs]))
@@ -19,7 +20,9 @@
 (t/use-fixtures :each th/database-reset)
 
 (t/deftest duplicate-file
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
+
         sobject (sto/put-object storage {:content (sto/content "content")
                                          :content-type "text/plain"
                                          :other "data"})
@@ -90,7 +93,8 @@
         ))))
 
 (t/deftest duplicate-file-with-deleted-rels
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         sobject (sto/put-object storage {:content (sto/content "content")
                                          :content-type "text/plain"
                                          :other "data"})
@@ -151,7 +155,9 @@
         ))))
 
 (t/deftest duplicate-project
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
+
         sobject (sto/put-object storage {:content (sto/content "content")
                                          :content-type "text/plain"
                                          :other "data"})
@@ -221,7 +227,8 @@
           )))))
 
 (t/deftest duplicate-project-with-deleted-files
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         sobject (sto/put-object storage {:content (sto/content "content")
                                          :content-type "text/plain"
                                          :other "data"})

backend/test/app/storage_test.clj

@@ -7,6 +7,7 @@
 (ns app.storage-test
   (:require
    [app.common.exceptions :as ex]
+   [app.common.uuid :as uuid]
    [app.db :as db]
    [app.storage :as sto]
    [app.test-helpers :as th]
@@ -22,9 +23,19 @@
                        th/database-reset
                        th/clean-storage))
 
+(defn configure-storage-backend
+  "Given storage map, returns a storage configured with the appropriate
+  backend for assets."
+  ([storage]
+   (assoc storage :backend :tmp))
+  ([storage conn]
+   (-> storage
+       (assoc :conn conn)
+       (assoc :backend :tmp))))
 
 (t/deftest put-and-retrieve-object
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         content (sto/content "content")
         object  (sto/put-object storage {:content content
                                          :content-type "text/plain"
@@ -39,9 +50,9 @@
     (t/is (= "content" (slurp (sto/get-object-path storage object))))
     ))
 
-
 (t/deftest put-and-retrieve-expired-object
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         content (sto/content "content")
         object  (sto/put-object storage {:content content
                                          :content-type "text/plain"
@@ -59,7 +70,8 @@
     ))
 
 (t/deftest put-and-delete-object
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         content (sto/content "content")
         object  (sto/put-object storage {:content content
                                          :content-type "text/plain"
@@ -79,7 +91,8 @@
     ))
 
 (t/deftest test-deleted-gc-task
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         content (sto/content "content")
         object1 (sto/put-object storage {:content content
                                          :content-type "text/plain"
@@ -96,14 +109,17 @@
     (let [res (db/exec-one! th/*pool* ["select count(*) from storage_object;"])]
       (t/is (= 1 (:count res))))))
 
-(t/deftest test-touched-gc-task
-  (let [storage (:app.storage/storage th/*system*)
+(t/deftest test-touched-gc-task-1
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         prof    (th/create-profile* 1)
         proj    (th/create-project* 1 {:profile-id (:id prof)
                                        :team-id (:default-team-id prof)})
+
         file    (th/create-file* 1 {:profile-id (:id prof)
                                     :project-id (:default-project-id prof)
                                     :is-shared false})
+
         mfile   {:filename "sample.jpg"
                  :tempfile (th/tempfile "app/test_files/sample.jpg")
                  :content-type "image/jpeg"
@@ -140,12 +156,12 @@
 
       ;; now check if the storage objects are touched
       (let [res (db/exec-one! th/*pool* ["select count(*) from storage_object where touched_at is not null"])]
-        (t/is (= 2 (:count res))))
+        (t/is (= 4 (:count res))))
 
       ;; run the touched gc task
       (let [task (:app.storage/gc-touched-task th/*system*)
             res  (task {})]
-        (t/is (= 0 (:freeze res)))
+        (t/is (= 2 (:freeze res)))
         (t/is (= 2 (:delete res))))
 
       ;; now check that there are no touched objects
@@ -157,8 +173,85 @@
         (t/is (= 2 (:count res))))
     )))
 
+
+(t/deftest test-touched-gc-task-2
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
+        prof    (th/create-profile* 1 {:is-active true})
+        team-id (:default-team-id prof)
+        proj-id (:default-project-id prof)
+        font-id (uuid/custom 10 1)
+
+        proj    (th/create-project* 1 {:profile-id (:id prof)
+                                       :team-id team-id})
+
+        file    (th/create-file* 1 {:profile-id (:id prof)
+                                    :project-id proj-id
+                                    :is-shared false})
+
+        ttfdata (-> (io/resource "app/test_files/font-1.ttf")
+                    (fs/slurp-bytes))
+
+        mfile   {:filename "sample.jpg"
+                 :tempfile (th/tempfile "app/test_files/sample.jpg")
+                 :content-type "image/jpeg"
+                 :size 312043}
+
+        params1 {::th/type :upload-file-media-object
+                 :profile-id (:id prof)
+                 :file-id (:id file)
+                 :is-local true
+                 :name "testfile"
+                 :content mfile}
+
+        params2 {::th/type :create-font-variant
+                 :profile-id (:id prof)
+                 :team-id team-id
+                 :font-id font-id
+                 :font-family "somefont"
+                 :font-weight 400
+                 :font-style "normal"
+                 :data {"font/ttf" ttfdata}}
+
+        out1     (th/mutation! params1)
+        out2     (th/mutation! params2)]
+
+    ;; (th/print-result! out)
+
+    (t/is (nil? (:error out1)))
+    (t/is (nil? (:error out2)))
+
+    ;; run the touched gc task
+    (let [task (:app.storage/gc-touched-task th/*system*)
+          res  (task {})]
+      (t/is (= 6 (:freeze res)))
+      (t/is (= 0 (:delete res)))
+
+      (let [result-1 (:result out1)
+            result-2 (:result out2)]
+
+        ;; now we proceed to manually delete one team-font-variant
+        (db/exec-one! th/*pool* ["delete from team_font_variant where id = ?" (:id result-2)])
+
+        ;; revert touched state to all storage objects
+        (db/exec-one! th/*pool* ["update storage_object set touched_at=now()"])
+
+        ;; Run the task again
+        (let [res  (task {})]
+          (t/is (= 2 (:freeze res)))
+          (t/is (= 4 (:delete res))))
+
+        ;; now check that there are no touched objects
+        (let [res (db/exec-one! th/*pool* ["select count(*) from storage_object where touched_at is not null"])]
+          (t/is (= 0 (:count res))))
+
+        ;; now check that all objects are marked to be deleted
+        (let [res (db/exec-one! th/*pool* ["select count(*) from storage_object where deleted_at is not null"])]
+          (t/is (= 4 (:count res))))))))
+
 (t/deftest test-touched-gc-task-without-delete
-  (let [storage (:app.storage/storage th/*system*)
+  (let [storage (-> (:app.storage/storage th/*system*)
+                    (configure-storage-backend))
         prof    (th/create-profile* 1)
         proj    (th/create-project* 1 {:profile-id (:id prof)
                                        :team-id (:default-team-id prof)})
@@ -198,72 +291,3 @@
       ;; check that we have all object in the db
       (let [res (db/exec-one! th/*pool* ["select count(*) from storage_object where deleted_at is null"])]
         (t/is (= 4 (:count res)))))))
-
-
-;; Recheck is the mechanism for delete leaked resources on
-;; transaction failure.
-
-(t/deftest test-recheck
-  (let [storage (:app.storage/storage th/*system*)
-        content (sto/content "content")
-        object  (sto/put-object storage {:content content
-                                         :content-type "text/plain"})]
-    ;; Sleep fo 50ms
-    (th/sleep 50)
-
-    (let [rows (db/exec! th/*pool* ["select * from storage_pending"])]
-      (t/is (= 1 (count rows)))
-      (t/is (= (:id object) (:id (first rows)))))
-
-    ;; Artificially make all storage_pending object 1 hour older.
-    (db/exec-one! th/*pool* ["update storage_pending set created_at = created_at - '1 hour'::interval"])
-
-    ;; Sleep fo 50ms
-    (th/sleep 50)
-
-    ;; Run recheck task
-    (let [task (:app.storage/recheck-task th/*system*)
-          res  (task {})]
-      (t/is (= 1 (:processed res)))
-      (t/is (= 0 (:deleted res))))
-
-    ;; After recheck task, storage-pending table should be empty
-    (let [rows (db/exec! th/*pool* ["select * from storage_pending"])]
-      (t/is (= 0 (count rows))))))
-
-(t/deftest test-recheck-with-rollback
-  (let [storage (:app.storage/storage th/*system*)
-        content (sto/content "content")]
-
-    ;; check with aborted transaction
-    (ex/ignoring
-     (db/with-atomic [conn th/*pool*]
-       (let [storage (assoc storage :conn conn)] ; make participate storage in the transaction
-         (sto/put-object storage {:content content
-                                  :content-type "text/plain"})
-         (throw (ex-info "expected" {})))))
-
-    ;; let a 200ms window for recheck registration thread
-    ;; completion before proceed.
-    (th/sleep 200)
-
-    ;; storage_pending table should have the object
-    ;; registered independently of the aborted transaction.
-    (let [rows (db/exec! th/*pool* ["select * from storage_pending"])]
-      (t/is (= 1 (count rows))))
-
-    ;; Artificially make all storage_pending object 1 hour older.
-    (db/exec-one! th/*pool* ["update storage_pending set created_at = created_at - '1 hour'::interval"])
-
-    ;; Sleep fo 50ms
-    (th/sleep 50)
-
-    ;; Run recheck task
-    (let [task (:app.storage/recheck-task th/*system*)
-          res  (task {})]
-      (t/is (= 1 (:processed res)))
-      (t/is (= 1 (:deleted res))))
-
-    ;; After recheck task, storage-pending table should be empty
-    (let [rows (db/exec! th/*pool* ["select * from storage_pending"])]
-      (t/is (= 0 (count rows))))))

backend/test/app/test_helpers.clj

@@ -52,7 +52,6 @@
                    (assoc-in [:app.db/pool :uri] (:database-uri config))
                    (assoc-in [:app.db/pool :username] (:database-username config))
                    (assoc-in [:app.db/pool :password] (:database-password config))
-                   (assoc-in [[:app.main/main :app.storage.fs/backend] :directory] "/tmp/app/storage")
                    (dissoc :app.srepl/server
                            :app.http/server
                            :app.http/router
@@ -65,8 +64,7 @@
                            :app.worker/scheduler
                            :app.worker/worker)
                    (d/deep-merge
-                    {:app.storage/storage {:backend :tmp}
-                     :app.tasks.file-media-gc/handler {:max-age (dt/duration 300)}}))
+                    {:app.tasks.file-media-gc/handler {:max-age (dt/duration 300)}}))
         _      (ig/load-namespaces config)
         system (-> (ig/prep config)
                    (ig/init))]