🐛 Compare in main's frame, and stop decoding the merge base twice

Two defects with one cause, which is that a comparison and a write want
different identifier frames and the code used one frame for both.

Bringing main into a branch compared in the branch's frame. It remapped
main and the base into that frame, which puts the branch's inherited
content where it cannot agree with the base: everything the branch
inherited still names main, so every reference the remap rewrote read as
a difference. On a demo branch that was 364 of 368 reported conflicts,
and none of them was an edit anybody made. The comparison now runs in
main's frame, exactly as the compare view and the listing summary
already do, and the changes it produces are translated into the branch's
frame before they are applied and logged. That translation is the new
`branch_merge.cljc::remap-changes`, which is a separate function rather
than a call to `remap-refs` for a reason its docstring gives: `relink-refs`
finds a reference where the attribute and the value travel together, and
a change vector carries the identifier alone. The write map is renamed to
`apply-map`, so no later reader can reach for it as the comparison map.

The compare also decoded its merge base twice. The derive resolves and
decodes the pinned snapshot in order to fold the op log over it, and the
caller then decoded the same bytes again for the comparison's third
version, which is about 406 ms on the design-system file and is paid by
every uncached branch summary in a listing as well. The derive now
offers the pre-fold value it already holds, behind an explicit opt-in, so
an ordinary read gains neither a key nor a cost, and a branch read that
forgets the flag raises rather than comparing against nothing. The
invariant a comment now states is that the comparison's base is the
pre-fold value, which is sound because the fold is pure: `process-changes`
threads a value, every method builds persistently, and `PointerMap`'s
`assoc` and `without` return new objects, so the pre-fold base shares
structure with the folded result and costs no extra memory for the pages
the log did not touch.

The new test asserts both halves, because either alone passes a wrong
implementation. A branch carrying only inherited content, updated from a
main that changed elsewhere, reports no conflicts and leaves an empty op
log. Main and the branch repainting the same shape still reports exactly
one conflict, naming that shape.

Verified: the whole backend suite, 919 tests and 5,525 assertions with 0
failures.
This commit is contained in:
Álvaro Tejero Cantero 2026-09-17 13:23:58 +02:00
parent b9ff74164e
commit a033a8e1e3
No known key found for this signature in database
4 changed files with 250 additions and 31 deletions

View File

@ -325,8 +325,14 @@
same numbers travel on the returned file's metadata as `::audit/props`,
so the RPC command that paid for the derive reports them on its own
audit event. An ordinary file read never reaches this function, so an
ordinary read neither times nor logs anything."
[{:keys [::db/conn] :as cfg} {:keys [id] :as file} decode?]
ordinary read neither times nor logs anything.
With `include-base-data?` true the returned file also carries the
decoded merge-base document under `::base-data`, which is the value
the replay starts from. A caller that is about to compare the branch
with its base reads the branch file with that flag and takes the
value, and it never decodes the same snapshot a second time."
[{:keys [::db/conn] :as cfg} {:keys [id] :as file} decode? include-base-data?]
(let [tpoint (ct/tpoint)
branch (db/get* conn :file-branch {:branch-file-id id})
@ -367,6 +373,14 @@
rows (db/exec! conn [sql:get-branch-changes id])
;; the decoded merge-base document. It is what the replay starts
;; from, and `process-changes` builds its result with persistent
;; updates and never writes into its input, so this value stays
;; the pre-replay state while `data` becomes the branch. A caller
;; that needs the merge base takes this value instead of decoding
;; the same snapshot a second time.
base-data (:data base)
;; the derive replays the log batch by batch, so each batch is
;; decoded exactly once. `depth` is the flattened op count, the
;; quantity `files_branch.clj::check-oplog-depth-limit!` limits,
@ -376,7 +390,7 @@
(let [changes (blob/decode changes)]
[(cpc/process-changes data changes)
(+ depth (count changes))]))
[(:data base) 0]
[base-data 0]
rows)
data (if decode? data (blob/encode data))
@ -402,6 +416,12 @@
(assoc :version (or (:version base) (:version file)))
(cond-> (some? (:migrations base))
(assoc :migrations (:migrations base)))
(cond-> include-base-data?
;; the comparison's `base` is the value the replay started from,
;; which is the merge base as decoded. The replay returns the
;; branch's own state in `data`, so a comparison handed `data`
;; as its base would compare the branch with itself.
(assoc ::base-data base-data))
;; the numbers travel with the value, because the RPC audit event
;; of the read that paid for this derive reads `::audit/props` off
;; the result metadata
@ -422,7 +442,8 @@
load-data?
throw-if-not-exists?
lock-for-update?
lock-for-share?]
lock-for-share?
include-base-data?]
:or {lock-for-update? false
lock-for-share? false
load-data? true
@ -430,7 +451,8 @@
decode? true
include-deleted? false
throw-if-not-exists? true
realize? false}
realize? false
include-base-data? false}
:as options}]
(assert (db/connection? conn) "expected cfg with valid connection")
@ -475,7 +497,7 @@
file
(if branch?
(branch-file-data cfg file decode?)
(branch-file-data cfg file decode? include-base-data?)
(->> file
(fmigr/resolve-applied-migrations cfg)
(fdata/resolve-file-data cfg)))

View File

@ -243,7 +243,12 @@
(defn- get-base-data
"The merge-base `:data` from the branch's base snapshot. Raises when the
snapshot cannot be resolved: silently falling back to main would degrade
the three-way merge into \"branch overwrites main\" without conflicts."
the three-way merge into \"branch overwrites main\" without conflicts.
Prefer `branch-base-data` where the branch file was read with
`:include-base-data? true`: the derive decoded the merge base to replay
the op log over it, so this function would decode the same snapshot a
second time."
[cfg {:keys [source-file-id base-snapshot-id] :as branch}]
(when-not base-snapshot-id
(ex/raise :type :not-found
@ -252,6 +257,20 @@
:branch-id (:id branch)))
(:data (fsnap/get-snapshot cfg source-file-id base-snapshot-id)))
(defn- branch-base-data
"The comparison's `base`: the merge-base document that a branch file
read with `:include-base-data? true` carries under `::bfc/base-data`.
That document is the state the branch's op log was replayed over, that
is the merge base before the replay. The file's `:data` holds the
branch's own state, so a comparison against `:data` would compare the
branch with itself. Raises when the key is absent, so a read that
forgot the flag fails loudly instead of comparing against nil."
[branch-file]
(or (::bfc/base-data branch-file)
(ex/raise :type :assertion
:code :branch-base-data-missing
:hint "the branch file was read without `:include-base-data? true`")))
(defn- release-base-snapshot!
"Reschedule a branch's base snapshot for normal deletion: it is pinned
~10 years while the branch is open, and must be released when the branch
@ -593,14 +612,19 @@
Raises on failure instead of degrading: the caller decides whether the
result is worth caching, and a value computed from an error must never
enter the summary cache."
[cfg main-data {:keys [source-file-id branch-file-id ahead-revn behind-revn] :as branch}]
[cfg main-data {:keys [source-file-id branch-file-id ahead-revn behind-revn]}]
(if (and (zero? ahead-revn) (zero? behind-revn))
[0 0 0]
(let [main-data (or main-data (:data (bfc/get-file cfg source-file-id :realize? true)))
;; the derive hands back the merge base it replayed the log over,
;; so the comparison's `base` costs one decode of the snapshot
;; rather than two
branch-file (bfc/get-file cfg branch-file-id :realize? true
:include-base-data? true)
branch-data (bm/remap-refs
(:data (bfc/get-file cfg branch-file-id :realize? true))
(:data branch-file)
(branch-id-map cfg branch-file-id source-file-id))
base-data (get-base-data cfg branch)
base-data (branch-base-data branch-file)
clean-count (fn [m] (let [s (:stats m)]
(+ (:added s) (:modified s) (:deleted s))))
fwd (when (pos? ahead-revn)
@ -646,8 +670,9 @@
(defn cached-diff-counts
"`branch-diff-counts!` behind the summary cache, degrading to zeros with
a warning when the base snapshot cannot be resolved: a listing is
read-only, and merge/update DO refuse loudly in that situation
(`get-base-data`). A value computed from an error never enters the cache.
read-only, and merge/update DO refuse loudly in that situation (reading
the branch file raises `:base-snapshot-missing`). A value computed from
an error never enters the cache.
`branch` carries the key tuple and the pair being compared, with
`source-file-id` naming main and `branch-file-id` the branch, whichever
@ -759,14 +784,15 @@
dir (or direction :branch->main)
main-file (-> (bfc/get-file cfg (:source-file-id branch) :realize? true)
(check-file-size-limits! :compare))
branch-file (bfc/get-file cfg (:branch-file-id branch) :realize? true)
branch-file (bfc/get-file cfg (:branch-file-id branch)
:realize? true :include-base-data? true)
log (-> (branch-log cfg (:branch-file-id branch))
(check-oplog-depth-limit! (:branch-file-id branch) :compare))
main-data (:data main-file)
branch-data (bm/remap-refs
(:data branch-file)
(branch-id-map cfg (:branch-file-id branch) (:source-file-id branch)))
base-data (get-base-data cfg branch)
base-data (branch-base-data branch-file)
;; only the branch->main direction may be bounded to the pages
;; the branch touched: the other direction is main's changes and
;; nothing here knows which pages those are
@ -849,8 +875,9 @@
(check-file-size-limits! :merge))
_ (-> (branch-log cfg branch-file-id)
(check-oplog-depth-limit! branch-file-id :merge))
branch-file (bfc/get-file cfg branch-file-id :realize? true)
base-data (get-base-data cfg branch)
branch-file (bfc/get-file cfg branch-file-id
:realize? true :include-base-data? true)
base-data (branch-base-data branch-file)
;; canonicalize the branch's local refs to main's ids: the
;; branch file id (components, library color/typography
@ -1070,19 +1097,39 @@
team (teams/get-team conn :profile-id profile-id :file-id branch-file-id)
delay (ldel/get-deletion-delay team)
;; update applies main's changes INTO the branch, so the
;; canonical local ids are the branch's: re-point main/base
;; local refs (file id + paired media) to the branch's ids;
;; media ADDED on main gets fresh ids pre-allocated here and
;; its rows copied into the branch only if the update applies.
;; Two id maps, one per direction of the crossing.
;;
;; The COMPARISON runs in main's frame, the frame the merge
;; base was captured in, the frame the compare view uses
;; (`files_branch.clj::branch-id-map` in
;; `files_branch.clj::get-branch-diff`), and the only frame
;; the branch's inherited content agrees with. A branch stores
;; no data, so `binfile.clj::branch-file-data` replays its op
;; log over that base and everything it inherited carries the
;; base's ids. Comparing in the branch's frame instead
;; rewrites main's and the base's local references to the
;; branch file id, and a self reference in main is then
;; unequal to the branch's own copy of it, so every shape
;; main touched that carries one reads as an edit on both
;; sides.
base-data base-raw
main-raw (:data main-file)
branch-cmp (bm/remap-refs (:data branch-file)
(branch-id-map cfg branch-file-id main-id))
;; The WRITE runs in the branch's frame: a reference is only a
;; reference in the file it names, so main's file id and
;; main's media rows are re-pointed at the branch's ids before
;; the change reaches the branch. Media ADDED on main has no
;; branch row yet, so it gets a fresh id here and its row is
;; copied below only if the update applies.
pairs (media-pairs cfg main-id branch-file-id)
new-media (unpaired-media-rows cfg main-id (:data main-file) pairs)
new-media (unpaired-media-rows cfg main-id main-raw pairs)
fresh-map (into {} (map (fn [row] [(:id row) (uuid/next)])) new-media)
id-map (-> pairs
apply-map (-> pairs
(merge fresh-map)
(assoc main-id branch-file-id))
base-data (bm/remap-refs base-raw id-map)
main-data (bm/remap-refs (:data main-file) id-map)
main-data (bm/remap-refs main-raw apply-map)
;; did the branch carry its OWN changes before this update?
;; (revn moved since the base was last positioned)
@ -1116,8 +1163,7 @@
{::db/return-keys false})))
merge-summary
(bm/compute-merge base-data main-data
(:data branch-file) :main->branch)
(bm/compute-merge base-data main-raw branch-cmp :main->branch)
conflicts (:conflicts merge-summary)
resolved? (fn [c] (bm/conflict-resolved? c (get resolutions (:id c))))
@ -1145,7 +1191,7 @@
;; assignment compute-changes uses internally here, so it can
;; be reused for the unsupported-kinds gate without re-diffing.
(let [{:keys [changes unsupported]}
(bm/compute-changes base-data (:data branch-file) main-data
(bm/compute-changes base-data branch-cmp main-raw
(or inverted {})
merge-summary)]
(cond
@ -1178,9 +1224,10 @@
;; under the pre-allocated ids the changes reference
(copy-media-rows! conn branch-file-id fresh-map new-media)
(let [updated (-> branch-file
(let [applied (bm/remap-changes changes apply-map)
updated (-> branch-file
(update :revn inc)
(update :data #(cpc/process-changes % changes)))
(update :data #(cpc/process-changes % applied)))
libs (bfc/get-resolved-file-libraries cfg updated)
errors (not-empty (cfv/validate-file updated libs))
updated (if errors
@ -1199,7 +1246,7 @@
:revn (:revn updated)
:version (:version updated)
:features (into-array (:features updated))
:changes (blob/encode (vec changes))}
:changes (blob/encode (vec applied))}
{::db/return-keys false})
;; SQUASH: the new base (main's current state)

View File

@ -14,6 +14,7 @@
(:require
[app.common.features :as cfeat]
[app.common.time :as ct]
[app.common.types.shape :as cts]
[app.common.uuid :as uuid]
[app.config :as cf]
[app.rpc :as-alias rpc]
@ -208,3 +209,101 @@
(or (nil? deleted-at)
(ct/is-after? deleted-at (ct/in-future {:days 400}))))
rows)))))))
(t/deftest update-from-main-compares-in-main-frame
;; A branch stores no data: its document is the base snapshot plus the op
;; log, so every reference it inherited names MAIN. A local reference is a
;; self reference in both files, because a branch keeps main's colours and
;; typographies under the same entity ids, so the comparison must run in
;; main's frame and the change that lands in the branch must still name the
;; branch.
(with-redefs [cf/flags (conj cf/flags :branching)]
(let [profile (th/create-profile* 1 {:is-active true})
proj-id (:default-project-id profile)
file (th/create-file* 1 {:profile-id (:id profile)
:project-id proj-id
:is-shared false})
main-id (:id file)
page-id (-> (th/command! {::th/type :get-file
::rpc/profile-id (:id profile)
:id main-id})
:result :data :pages first)
color-id (uuid/random)
shape-id (uuid/random)
fills (fn [ref-file color]
[{:fill-color color
:fill-opacity 1
:fill-color-ref-id color-id
:fill-color-ref-file ref-file}])]
;; main publishes a colour of its own and one of its shapes uses it, so
;; the shape carries main's file id in a self reference
(apply-change* profile main-id
{:type :add-color
:color {:id color-id :name "Brand" :color "#ff0000" :opacity 1}})
(apply-change* profile main-id
{:type :add-obj
:page-id page-id
:id shape-id
:parent-id uuid/zero
:frame-id uuid/zero
:obj (-> (cts/setup-shape
{:id shape-id :name "Referenced" :type :rect
:parent-id uuid/zero :frame-id uuid/zero})
(assoc :fills (fills main-id "#ff0000")))})
(let [create (create-branch* profile main-id "main-frame")
branch-id (:id create)
branch-file-id (:branch-file-id create)]
(t/testing "main's repaint is not a conflict, and is not logged as a branch change"
(apply-change* profile main-id
{:type :mod-obj
:page-id page-id
:id shape-id
:operations [{:type :set :attr :fills :val (fills main-id "#00ff00")}]})
(let [out (th/command! {::th/type :update-branch-from-main
::rpc/profile-id (:id profile)
:branch-id branch-id})]
(t/is (nil? (:error out)))
(t/is (= :updated (-> out :result :status))))
(let [out (th/command! {::th/type :get-file
::rpc/profile-id (:id profile)
:id branch-file-id})]
(t/is (= "#00ff00" (get-in out [:result :data :pages-index page-id
:objects shape-id :fills 0 :fill-color]))))
;; main's own change arrives through the repositioned base, so the
;; net the update writes is empty. A ref left in main's frame would
;; make this shape differ from the new base and put it in the log.
(t/is (empty? (oplog-rows branch-file-id))))
(t/testing "a genuine conflict still reports"
(apply-change* profile main-id
{:type :mod-obj
:page-id page-id
:id shape-id
:operations [{:type :set :attr :fills :val (fills main-id "#0000ff")}]})
(apply-change* profile branch-file-id
{:type :mod-obj
:page-id page-id
:id shape-id
:operations [{:type :set :attr :fills :val (fills branch-file-id "#ffffff")}]})
(let [out (th/command! {::th/type :update-branch-from-main
::rpc/profile-id (:id profile)
:branch-id branch-id})]
(t/is (= :conflicts (-> out :result :status)))
(t/is (= 1 (count (-> out :result :conflicts))))
(t/is (= shape-id (-> out :result :conflicts first :id)))))
(t/testing "taking main keeps the branch writeable"
(let [out (th/command! {::th/type :update-branch-from-main
::rpc/profile-id (:id profile)
:branch-id branch-id
:resolutions {shape-id :main}})]
(t/is (nil? (:error out)))
(t/is (= :updated (-> out :result :status))))
(let [out (th/command! {::th/type :get-file
::rpc/profile-id (:id profile)
:id branch-file-id})]
(t/is (= "#0000ff" (get-in out [:result :data :pages-index page-id
:objects shape-id :fills 0 :fill-color])))))))))

View File

@ -644,6 +644,57 @@
colors
colors)))))))
(def ^:private reference-attrs
"Shape attrs whose value IS a reference to the file that owns the entity.
`relink-refs` finds them inside a shape map, where the attr and the value
travel together. An operation that sets one of them carries the id alone."
#{:component-file :fill-color-ref-file :stroke-color-ref-file :typography-ref-file})
(defn- remap-op
"Rewrite the reference of one `:mod-obj` operation through `id-map`.
`shape-set-ops` emits one `:set` per differing attr, so a reference can
arrive either as the attr's own value or, for an image shape, as the media
map under `:metadata`."
[op id-map]
(let [attr (:attr op)
val (:val op)]
(cond
(and (contains? reference-attrs attr) (uuid? val))
(assoc op :val (get id-map val val))
(and (= :metadata attr) (uuid? (:id val)))
(assoc op :val (update val :id #(get id-map % %)))
:else op)))
(defn remap-changes
"Rewrite the cross-file references of a change vector through `id-map`,
the counterpart of `remap-refs` for the changes `compute-changes` emits.
A change computed in the source file's frame has to be applied in the
target's, because a reference is only a reference in the file it names: a
fill whose `:fill-color-ref-file` names another file loses its link, and so
does an instance whose `:component-file` does.
Every payload `compute-changes` can emit is covered. `relink-refs` walks a
whole map, so `:obj`, `:page`, `:params`, and a component's `:objects` are
complete on their own. The two payloads it cannot see are the values that
ARE the reference: a media object's `:id` (its `:media-id` is a storage key
and must not be touched), and an operation value that is the reference
itself. Token changes are left alone: no id in `id-map` is a token id."
[changes id-map]
(if (empty? id-map)
changes
(let [lookup #(get id-map % %)
relink #(cfh/relink-refs % lookup)]
(mapv (fn [change]
(cond-> (relink change)
(uuid? (:id (:object change)))
(update-in [:object :id] lookup)
(seq (:operations change))
(update :operations #(mapv (fn [op] (remap-op op id-map)) %))))
changes))))
(defn- strip-modified-at
"Remove the `:modified-at` bookkeeping timestamp from every entry of an
indexed collection (components, colors, typographies).