From 2c00644336fb1bf3bd330f66260e5a970c014298 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 29 Oct 2026 16:53:59 +0000 Subject: [PATCH] :bug: Decode snapshot migrations as an ordered set `file-snapshots/decode-snapshot` handed the `file_change.migrations` pgarray as a vector. `set/difference` falls back to index lookups when the first collection is shorter than the second, so the pending set was right only by accident: a stored subset happens to work, while a stored set larger than `available-migrations` (a snapshot taken by a newer build) marks every available migration as pending and re-applies it to already migrated data. Decode it as a `d/ordered-set`, the type the rest of the code and `schema:file` expect. This also covers `restore!` and the snapshot list, and it lets `get-file-snapshot` drop the `(into #{} ...)` normalization it was carrying. Re-wrap the union in `fmg/migrate` so the invariant holds even when a caller hands a plain set. A snapshot with NULL `migrations` predates SQL migration 0140 and keeps the format version of its time. Pin that legacy path: the preview regenerates the applied set from that version, migrates in memory and leaves the stored row untouched. Update the file-data memory with the invariant and the legacy path. AI-assisted-by: deepseek-v4.1-flash --- .serena/memories/backend/subtleties.md | 3 +- backend/src/app/features/file_snapshots.clj | 10 +++- .../src/app/rpc/commands/files_snapshot.clj | 10 ++-- .../backend_tests/rpc_file_snapshot_test.clj | 53 +++++++++++++++++++ common/src/app/common/files/migrations.cljc | 4 +- 5 files changed, 71 insertions(+), 9 deletions(-) diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index c4f797e66a..ebc200f586 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -94,7 +94,8 @@ - Pointer-map fragments are persisted separately as type `fragment`, and only modified pointer maps are written. A pointer keeps its id while nothing changes; content (`assoc`/`without`) and metadata (`with-meta`) changes produce a fresh id, so a fragment row is never rewritten after it is written. Re-assoc with an identical value returns the same map in array and hash maps alike, so a save keeps the fragment id of every page it does not change. - `enable-objects-map` skips unloaded pointer pages: their fragment was already wrapped when it was written, and loading it only to wrap it again costs one page read per untouched page on every save. A page with a plain fragment is wrapped when it is first loaded or edited; a data migration that enables `fdata/objects-map` must rewrite the data itself. - Snapshots share fragment rows with the live file (they are stored with pointers, not materialized). `file-gc` marks every fragment referenced neither by the current main file nor by an unpurged snapshot; the file is read unrealized and unchanged pages keep their ids via reattach, so a pass over an unchanged file writes no new fragments. -- A save attaches its automatic snapshot before applying pending file migrations: the snapshot keeps the pre-migration pointers, data and migration set, and restoring it leaves the next read to migrate the file. This also holds for files without the feature. The preview (`get-file-snapshot`) applies the pending migrations in memory with the live file's libraries and never persists them: the stored version keeps its data, the payload carries the snapshot's `:migrations` (normalized to a set; the decoder keeps the pgarray as a vector and `set/difference` would compare indexes) and `fdata/objects-map` is realized for clients that do not declare it. The historical feature set is deliberately not checked against the enabled one: a version may predate a feature change. The window is narrow: the `get-file` RPC reads with `migrate?` on and its ETag hashes `available-migrations`, so a post-deploy read cannot be a 304 from the pre-deploy state and migrates before any save in the normal flow; the pre-migration snapshot only appears when the save is the first request to touch the file after the deploy (an open tab that saves without re-reading, or a non-UI writer). `create-file-snapshot` and the restore backup read with `migrate?` on, so those versions are stored already migrated. +- A save attaches its automatic snapshot before applying pending file migrations: the snapshot keeps the pre-migration pointers, data and migration set, and restoring it leaves the next read to migrate the file. This also holds for files without the feature. The preview (`get-file-snapshot`) applies the pending migrations in memory with the live file's libraries and never persists them: the stored version keeps its data, the payload carries the snapshot's `:migrations` (an ordered set) and `fdata/objects-map` is realized for clients that do not declare it. The historical feature set is deliberately not checked against the enabled one: a version may predate a feature change. The window is narrow: the `get-file` RPC reads with `migrate?` on and its ETag hashes `available-migrations`, so a post-deploy read cannot be a 304 from the pre-deploy state and migrates before any save in the normal flow; the pre-migration snapshot only appears when the save is the first request to touch the file after the deploy (an open tab that saves without re-reading, or a non-UI writer). `create-file-snapshot` and the restore backup read with `migrate?` on, so those versions are stored already migrated. +- `:migrations` is always a `d/ordered-set`: `file-migrations/resolve-applied-migrations` builds one, `fmg/migrate` re-wraps the union after applying, and `file-snapshots/decode-snapshot` converts the `file_change` pgarray (as a plain vector, `set/difference` and `contains?` would compare indexes instead of names). A snapshot with NULL `:migrations` and an old `:version` predates the `file_change.migrations` column (SQL migration 0140): the preview regenerates the set from the stored version and applies that full chain, while `restore!` uses a fixed 67 baseline, so preview and restore can disagree for very old snapshots. - The migration read path (`binfile.common/migrate-file`) captures `fdata/original-pointers` before realizing, reattaches unchanged positions after applying the migrations, and persists the reattached file; it returns the realized file because reads expose plain data. A migration that adds `fdata/objects-map` skips the reattach so the pages get wrapped. - `fdata/load-pointer` reads rows that a previous `file-gc` pass marked on purpose (explicit `::db/remove-deleted false`): the fragment is only gone once `objects-gc` purges it. The `get-file-fragment` RPC keeps rejecting marked rows. - Bind the fragment loader through `fdata/with-pointers` (reads), `fdata/with-tracked-pointers` (writes with a loader) or `fdata/with-tracked` (writes with nothing to load yet, e.g. file creation), and `fdata/with-untracked` (validation passes that must track nothing) instead of hand-rolled `binding` forms. Two direct bindings remain on purpose: the fine-grained tracked toggles of `app.rpc.commands.files-update` (validation/repair turn tracking off, restore the existing atom and read it before repairing), and `binfile.common/encode-file` (a fresh tracked set inheriting the current one). Converge them only if atom-scoped macros (`with-tracking`, `with-inherited-tracking`) are added; today the extra indirection buys nothing. diff --git a/backend/src/app/features/file_snapshots.clj b/backend/src/app/features/file_snapshots.clj index e9d8429d63..2370758d28 100644 --- a/backend/src/app/features/file_snapshots.clj +++ b/backend/src/app/features/file_snapshots.clj @@ -48,11 +48,19 @@ AND fd.type = 'snapshot') WHERE c.label IS NOT NULL") +;; `:migrations` is an ordered set everywhere (see `common.types.file`), +;; and `need-migration?`/`migrate` use set semantics on it: the pgarray +;; must never surface as a vector, where `contains?`/`set/difference` +;; would compare indexes instead of names. +(defn- decode-migrations + [value] + (into (d/ordered-set) (db/decode-pgarray value []))) + (defn- decode-snapshot [snapshot] (some-> snapshot (-> (d/update-when :metadata fdata/decode-metadata) - (d/update-when :migrations db/decode-pgarray []) + (d/update-when :migrations decode-migrations) (d/update-when :features db/decode-pgarray #{})))) (def ^:private sql:get-minimal-file diff --git a/backend/src/app/rpc/commands/files_snapshot.clj b/backend/src/app/rpc/commands/files_snapshot.clj index 80a867bcee..ba27fb5186 100644 --- a/backend/src/app/rpc/commands/files_snapshot.clj +++ b/backend/src/app/rpc/commands/files_snapshot.clj @@ -77,13 +77,11 @@ ;; format. Apply the pending migrations in memory, the same pure ;; step a normal read runs, and never persist them: the stored ;; version keeps its data and restoring it re-migrates on the - ;; next read. `:migrations` is normalized to a set because the - ;; snapshot decoder keeps the pgarray as a vector and - ;; `set/difference` would compare indexes. + ;; next read. `decode-snapshot` hands an ordered set, which is + ;; what `need-migration?`/`migrate` require; a stored NULL is + ;; regenerated from the snapshot's version. (let [base-file (bfc/get-file cfg file-id :load-data? false) - snapshot (-> snapshot - (assoc :id file-id) - (update :migrations #(into #{} %))) + snapshot (assoc snapshot :id file-id) file (if (fmg/need-migration? snapshot) (let [file (fdata/realize cfg snapshot)] (fmg/migrate-file diff --git a/backend/test/backend_tests/rpc_file_snapshot_test.clj b/backend/test/backend_tests/rpc_file_snapshot_test.clj index 9f6e20b6b1..9238a25b79 100644 --- a/backend/test/backend_tests/rpc_file_snapshot_test.clj +++ b/backend/test/backend_tests/rpc_file_snapshot_test.clj @@ -7,7 +7,10 @@ (ns backend-tests.rpc-file-snapshot-test (:require [app.binfile.common :as bfc] + [app.common.data :as d] [app.common.features :as cfeat] + [app.common.files.defaults :as cfd] + [app.common.files.migrations :as fmg] [app.common.pprint :as pp] [app.common.thumbnails :as thc] [app.common.time :as ct] @@ -686,3 +689,53 @@ (get-in with-feature [:result :data :pages-index page-id :objects]))) (t/is (not (omap/objects-map? (get-in without-feature [:result :data :pages-index page-id :objects])))))) + +(t/deftest snapshot-preview-migrates-legacy-snapshots-without-migrations + ;; Snapshots written before the `migrations` column existed (SQL + ;; migration 0140) store NULL and the format version of their time. + ;; The decoder must hand an ordered set (a vector would make + ;; `need-migration?` compare indexes), and the preview must + ;; regenerate the applied set from the stored version, migrate the + ;; data in memory and leave the row untouched. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + file-id (:id file) + snap-id (:id (:result (th/command! {::th/type :create-file-snapshot + ::rpc/profile-id (:id profile) + :file-id file-id + :label "legacy"})))] + + (t/is (d/ordered-set? (:migrations (fsnap/get-snapshot th/*system* file-id snap-id)))) + + ;; Turn the version into a pre-0140 snapshot: no applied set and an + ;; old format version. + (th/db-exec! ["UPDATE file_change SET migrations = NULL, version = 60 WHERE id = ?" + snap-id]) + + (let [stored (fsnap/get-snapshot th/*system* file-id snap-id) + out (th/command! {::th/type :get-file-snapshot + ::rpc/profile-id (:id profile) + :file-id file-id + :id snap-id + :features cfeat/supported-features}) + result (:result out)] + + (t/is (nil? (:migrations stored))) + (t/is (= 60 (:version stored))) + + (t/is (nil? (:error out))) + (t/is (d/ordered-set? (:migrations result))) + ;; The preview regenerates everything the stored version lacks + ;; instead of trusting the absent applied set. + (t/is (= cfd/version (:version result))) + (t/is (= (into #{} fmg/available-migrations) + (set (:migrations result)))) + (t/is (set/subset? (into #{} (fmg/generate-migrations-from-version cfd/version)) + (set (:migrations result)))) + + ;; Nothing is persisted. + (let [stored (fsnap/get-snapshot th/*system* file-id snap-id)] + (t/is (nil? (:migrations stored))) + (t/is (= 60 (:version stored))))))) diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index de82f16f9e..676cc6291a 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -88,7 +88,9 @@ (-> file (assoc :data data) - (update :migrations set/union diff) + ;; `:migrations` is an ordered set everywhere; keep it that way + ;; even if a caller hands a plain set. + (update :migrations #(into (d/ordered-set) (set/union % diff))) (vary-meta assoc ::migrated (not-empty diff))))) (defn generate-migrations-from-version