From 003ff8e7719b92d166abd0c97863ed336bee9b40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero=20Cantero?= Date: Wed, 16 Sep 2026 19:51:14 +0200 Subject: [PATCH] :bug: Audit a branch save and a materialise like every other command The preview's observability criterion is stated as every branch operation, and two of them sat outside it. A save routed to a branch's op log carried only the ordinary save props, and materialise carried none, so the two operations that matter most to an enterprise trial were the two with no duration in the record. Both attach `branch-operation`, `branch-outcome` and `branch-duration-ms` now: `save-branch`/`saved` on the save path, `materialize`/`materialized` on the exit door, including its idempotent no-op. The props travel in `::audit/replace-props` on the save path because that is the key the ordinary save already replaces, so the ordinary save's own props are untouched. The test that asserts the audit props now covers a save and a materialise alongside creation and merge. The three branch namespaces are 40 tests and 338 assertions, green. --- backend/src/app/rpc/commands/files_branch.clj | 114 +++++++++--------- backend/src/app/rpc/commands/files_update.clj | 29 +++-- .../backend_tests/rpc_file_branch_test.clj | 42 +++++++ 3 files changed, 123 insertions(+), 62 deletions(-) diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index 9c26a5faf5..d5653f05b4 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -1230,71 +1230,75 @@ [cfg {:keys [::rpc/profile-id file-id]}] (check-branching-enabled!) (files/check-edition-permissions! cfg profile-id file-id) - (db/tx-run! - cfg - (fn [{:keys [::db/conn] :as cfg}] - ;; the advisory lock every save on this file takes: a concurrent - ;; branch save must not land between the derive and the persist, or - ;; its op would be dropped together with the log - (db/xact-lock! conn file-id) + (let [tpoint (ct/tpoint)] + (db/tx-run! + cfg + (fn [{:keys [::db/conn] :as cfg}] + ;; the advisory lock every save on this file takes: a concurrent + ;; branch save must not land between the derive and the persist, or + ;; its op would be dropped together with the log + (db/xact-lock! conn file-id) - (let [branch (db/get* conn :file-branch {:branch-file-id file-id})] - (if (nil? branch) - {:status :materialized :file-id file-id :changed false} - (let [file (bfc/get-file cfg file-id :realize? true) - team (teams/get-team conn :profile-id profile-id :file-id file-id) - delay (ldel/get-deletion-delay team) - ts (ct/now)] + (let [branch (db/get* conn :file-branch {:branch-file-id file-id})] + (if (nil? branch) + (audited {:status :materialized :file-id file-id :changed false} + tpoint :materialize) + (let [file (bfc/get-file cfg file-id :realize? true) + team (teams/get-team conn :profile-id profile-id :file-id file-id) + delay (ldel/get-deletion-delay team) + ts (ct/now)] - (binding [pmap/*tracked* (pmap/create-tracked) - pmap/*load-fn* (partial fdata/load-pointer cfg file-id) - cfeat/*current* (:features file) - cfeat/*previous* (:features file)] + (binding [pmap/*tracked* (pmap/create-tracked) + pmap/*load-fn* (partial fdata/load-pointer cfg file-id) + cfeat/*current* (:features file) + cfeat/*previous* (:features file)] - (let [libs (bfc/get-resolved-file-libraries cfg file) - errors (not-empty (cfv/validate-file file libs)) - file (if errors - (update file :data cpc/process-changes - (cfr/repair-file file libs errors)) - file)] + (let [libs (bfc/get-resolved-file-libraries cfg file) + errors (not-empty (cfv/validate-file file libs)) + file (if errors + (update file :data cpc/process-changes + (cfr/repair-file file libs errors)) + file)] - ;; the derived state becomes the file's own payload - (fupd/persist-file! (assoc cfg ::fupd/timestamp ts) file) + ;; the derived state becomes the file's own payload + (fupd/persist-file! (assoc cfg ::fupd/timestamp ts) file) - ;; from here the ordinary paths apply - (db/update! conn :file - {:is-branch false} - {:id file-id} - {::db/return-keys false}) + ;; from here the ordinary paths apply + (db/update! conn :file + {:is-branch false} + {:id file-id} + {::db/return-keys false}) - ;; the payload replaced the log - (db/delete! conn :file-branch-change {:branch-id (:id branch)}) + ;; the payload replaced the log + (db/delete! conn :file-branch-change {:branch-id (:id branch)}) - ;; a review sandbox over a file that is no longer a branch - ;; has nothing left to review - (close-branch-pull-requests! cfg (:id branch) - {:profile-id profile-id - :status "closed" - :deleted-at (ct/in-future delay)}) + ;; a review sandbox over a file that is no longer a branch + ;; has nothing left to review + (close-branch-pull-requests! cfg (:id branch) + {:profile-id profile-id + :status "closed" + :deleted-at (ct/in-future delay)}) - ;; nothing derives from the base any more, so the pin goes - (release-base-snapshot! cfg branch (ct/in-future delay)) + ;; nothing derives from the base any more, so the pin goes + (release-base-snapshot! cfg branch (ct/in-future delay)) - ;; The branch row is archived rather than deleted: its pull - ;; requests reference it with ON DELETE CASCADE, so deleting - ;; it would take the review history along, and `archived` is - ;; a status the schema already allows. - (db/update! conn :file-branch - {:status "archived" - :deleted-at (ct/in-future delay) - :updated-at ts} - {:id (:id branch)} - {::db/return-keys false}) + ;; The branch row is archived rather than deleted: its pull + ;; requests reference it with ON DELETE CASCADE, so deleting + ;; it would take the review history along, and `archived` is + ;; a status the schema already allows. + (db/update! conn :file-branch + {:status "archived" + :deleted-at (ct/in-future delay) + :updated-at ts} + {:id (:id branch)} + {::db/return-keys false}) - {:status :materialized - :file-id file-id - :changed true - :revn (:revn file)})))))))) + (audited + {:status :materialized + :file-id file-id + :changed true + :revn (:revn file)} + tpoint :materialize)))))))))) ;; --- COMMAND QUERY: get-file-branch-info diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 438e3dc38d..a6ed200c83 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -210,7 +210,7 @@ (binding [l/*context* (some-> (meta params) (get :app.http/request) (errors/request->context))] - (-> (update-file* cfg params) + (-> (update-file* cfg params tpoint) (rph/with-defer #(let [elapsed (tpoint)] (l/trace :hint "update-file" :time (ct/format-duration elapsed)))))))) @@ -221,9 +221,14 @@ Follow the inner implementation to `update-file-data!` function. + `tpoint` is the request's own timer, threaded in so that a save routed + to a branch's op log can report its duration on the audit event the way + the other branch commands do. + Only intended for internal use on this module." [{:keys [::db/conn ::timestamp] :as cfg} - {:keys [profile-id file team features changes session-id skip-validate] :as params}] + {:keys [profile-id file team features changes session-id skip-validate] :as params} + tpoint] (binding [pmap/*tracked* (pmap/create-tracked) pmap/*load-fn* (partial fdata/load-pointer cfg (:id file))] @@ -286,11 +291,21 @@ (with-meta {:revn revn :lagged (get-lagged-changes conn params)} {::audit/replace-props - {:id (:id file) - :name (:name file) - :features (:features file) - :project-id (:project-id file) - :team-id (:team-id file)}})))) + (cond-> {:id (:id file) + :name (:name file) + :features (:features file) + :project-id (:project-id file) + :team-id (:team-id file)} + + ;; A save routed to a branch's op log is the preview's most + ;; frequent operation, so it reports its duration and outcome on + ;; its own audit event like every other branch command. The + ;; ordinary save path is untouched: the props above are the ones + ;; it has always carried. + (:is-branch file) + (assoc :branch-operation "save-branch" + :branch-outcome "saved" + :branch-duration-ms (inst-ms (tpoint))))})))) (defn get-file "Get not-decoded file, only decodes the features set." diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index 2c34bf7961..c8db98b875 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -1249,4 +1249,46 @@ (t/is (nil? (:error out))) (t/is (= "merge" (:branch-operation props))) (t/is (= "merged" (:branch-outcome props))) + (t/is (int? (:branch-duration-ms props))))) + + (t/testing "a save routed to the branch reports the same three" + ;; the ordinary save path replaces its props, so the branch keys + ;; travel in `::audit/replace-props` and not in `::audit/props` + (let [created (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name "audited-save"}) + bfid (-> created :result :branch-file-id) + file-row (th/db-get :file {:id bfid}) + out (th/command! {::th/type :update-file + ::rpc/profile-id (:id profile) + :id bfid + :session-id (uuid/random) + :revn (:revn file-row) + :vern (:vern file-row) + :features cfeat/supported-features + :changes [{:type :add-color + :color {:id (uuid/random) + :name "audited" + :color "#112233" + :opacity 1}}]}) + props (-> out :result meta :app.loggers.audit/replace-props)] + (t/is (nil? (:error out))) + (t/is (= "save-branch" (:branch-operation props))) + (t/is (= "saved" (:branch-outcome props))) + (t/is (int? (:branch-duration-ms props))) + (t/is (= bfid (:id props))))) + + (t/testing "materialising reports its own operation and outcome" + (let [created (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name "audited-exit"}) + out (th/command! {::th/type :materialize-file-branch + ::rpc/profile-id (:id profile) + :file-id (-> created :result :branch-file-id)}) + props (-> out :result meta :app.loggers.audit/props)] + (t/is (nil? (:error out))) + (t/is (= "materialize" (:branch-operation props))) + (t/is (= "materialized" (:branch-outcome props))) (t/is (int? (:branch-duration-ms props))))))))