fix(store): migrate relation fields when their target collection is renamed (BUG-2873) - #1243
Merged
Conversation
…enamed (BUG-2873) `models.FieldDef.Collection` holds the target's SLUG, and it is the ONLY pointer a relation field carries — there is no id beside it to fall back on. `UpdateCollection` re-slugifies on rename and nothing migrated the definitions aimed at the renamed collection, so every relation field pointing at it was stranded: the picker filters on a slug that resolves to nothing and the field silently stops being fillable. `retargetRelationFieldsTx` re-points them in the SAME transaction as the rename, for the reason the field-value migrations already run there: a failure must roll the rename back rather than commit collections pointing at a slug that no longer exists. **It parses instead of string-replacing.** A schema's JSON contains the old slug in places that must not move — a text field's `default`, a select's `options`, a label. Only `FieldDef.Collection` on a `relation` field is a reference. Export's `remapFieldIDs` gets away with a blind replace because it substitutes UUIDs, which cannot collide with prose; a slug is a word. A control test pins that. **The renamed collection is included deliberately** — a relation targeting ITSELF needs the same rewrite — and the rewrite lands after the caller's own `schema` write in the transaction, so a simultaneous schema edit composes rather than being reverted. Both have tests. ## The deadlock hazard, and why the existing comment does not cover it The lock-order comment above this transaction is a Codex P1 fix that orders the workspace lock against ONE collection row lock, because until now nothing took more than one. This change writes SIBLING collection rows, so two concurrent renames of mutually-referencing collections take those locks in opposite orders. **Reproduced, not theorised:** with the serialization removed, the test fails in 1.4s with `ERROR: deadlock detected (SQLSTATE 40P01)` on Postgres. Renames now take the workspace lock — previously acquired only when `len(input.Migrations) > 0` — BEFORE the row lock, which closes it without inventing a second ordering rule to keep in sync with the first. **The first version of that test was not an instrument.** Two unsynchronised goroutines passed against the same mutant in 0.44s, having simply never collided. It takes a start barrier and 40 rounds to be evidence. ## Scope The out-of-tx slug allocation (`uniqueSlugExcluding(s.db, …)` at :420, before `s.db.Begin()` at :503) is deliberately NOT touched — filed as IDEA-2874. Its dependents are every collection rename in every workspace, not the relation feature, so it does not belong in a change whose reviewability rests on affecting zero live rows. `UNIQUE(workspace_id, slug)` makes today's behaviour loud rather than lossy, so it can wait. A census found ZERO relation fields across all 11 accessible workspaces on this instance, and no shipped template declares one — this repairs the rename path before PLAN-2857 creates the population, which is why a migration is not needed. Gates: `gofmt` clean, `go vet ./...` ok, `go build ./...` ok, full `go test ./...` green on SQLite, and the full `internal/store` suite green on **Postgres** (private container on 127.0.0.1:5473, never the shared 5445 a sibling seat may tear down). Pin written and run BEFORE the fix per team CONVE-29: 2 propagation tests failed, the control passed.
Codex round 1: four findings, three P1, all real. **The migrated siblings' concurrency token was not advanced.** `collections.updated_at` doubles as the OCC token (BUG-2265), so rewriting a sibling's schema without touching it left a client holding the PRE-rename schema — and a token that still matched — able to write it straight back and undo the migration. Every rewritten row now takes the rename's own token, so the whole rename shares one instant. Pinned by asserting the stale token now 409s. **The scan did not lock the rows it rewrites.** A concurrent schema update to a sibling could commit between the SELECT and the UPDATE, and this transaction would then overwrite the newer schema with its stale copy. `FOR UPDATE` on Postgres, ordered by id so the multi-row acquisition is deterministic; SQLite is covered by its BEGIN IMMEDIATE write lock. **The old slug came from the pre-transaction snapshot.** Two tokenless concurrent renames of the same collection both read the ORIGINAL slug outside the lock; the loser would migrate `original -> its own new slug` while the relations already said the WINNER's, matching nothing and stranding them at a name no collection holds. The slug is now re-read alongside the token under the row lock. This is the READ — the ALLOCATION of the new slug is still outside the transaction and still IDEA-2874's, deliberately. **Re-marshaling through `models.CollectionSchema` dropped unknown properties.** That struct has fixed fields, so unmarshal+marshal silently erased anything it does not declare — a rename would quietly strip forward-compatible metadata from every relation-bearing schema in the workspace. It now edits the raw decoded JSON, touching only `fields[i].collection`. ## Two instruments that were not instruments Both found by mutation, not by reading: - **The deadlock test passed against its own mutant in 0.44s.** Two unsynchronised goroutines never collided. With a start barrier and 40 rounds it now fails in 1.4s with `ERROR: deadlock detected (SQLSTATE 40P01)` — so Rook's hazard was reproducible, not theoretical. - **The pre-tx-slug mutant survived the first matrix**, because nothing forced the interleaving. Rather than call it untestable, it is pinned by an end-state invariant that holds under ANY interleaving — whatever slug the collection ends up with, every relation aimed at it points there — over 40 concurrent rounds. It fails at round 1 under the mutant. Mutation matrix 4 of 4 killed. Gates: `gofmt` clean, `go vet ./...` ok, full `go test ./...` green on SQLite, and the full `internal/store` suite green on **Postgres** — 448.6s on a private container at 127.0.0.1:5473, never the shared 5445 a sibling seat may tear down. Run detached with a sentinel after the first attempt was killed at a turn boundary; the harness kills backgrounded tasks, it does not kill disowned ones.
…t (BUG-2873)
Codex round 2: four findings, two fixed here and two filed as their own items.
**Migrated siblings' OCC tokens could REGRESS.** A sibling updated between this
rename's timestamp and the scan already holds a newer `updated_at`; stamping the
rename's value on it moved the token BACKWARDS — breaking the strictly-increasing
invariant the transaction above exists to maintain, and re-validating a token the
client should have lost. Each rewritten row now takes `max(current + 1ns,
renameToken)`, computed in Go from the value read under the row lock rather than
by comparing timestamp TEXT, which the existing comment warns is never safe.
**Large integers in unknown properties were corrupted.** Round 1 fixed the typed
round-trip dropping unknown keys, but decoding into `interface{}` turns every
JSON number into float64, so `9007199254740993` came back CHANGED. A rename would
silently damage a property it exists only to carry through. `UseNumber` keeps the
literal text.
## Filed, not absorbed — both because their dependents are not the relation feature
- **BUG-2875** — a collection CREATED during a rename escapes the scan's
`FOR UPDATE` and keeps a relation aimed at the old slug. Closing it means
`CreateCollection` takes the workspace lock, which changes the concurrency
behaviour of every collection creation on the instance. Same reasoning that
split IDEA-2874 out; this unit's reviewability rests on affecting zero live rows.
- **IDEA-2876** — migrated siblings emit no `collection_updated` event, so an open
page keeps the pre-rename schema until reload. Handler/event layer; the store
publishes nothing.
## The mutation harness was reporting a false survivor
Counting `--- FAIL` lines treats a mutant that FAILS TO COMPILE as one that
survived — zero failures either way. Removing the token guard leaves
`rowUpdatedAt` and `renameToken` unused, so that is exactly what happened, and it
read as "the guard is untested". With a compiling mutant (`_ = rowUpdatedAt`) it
dies immediately. Worth stating because the failure mode is silent and points the
wrong way: it invents doubt about code that is fine, and would equally hide a
real survivor behind an unrelated build break.
Mutation matrix 6 of 6 killed.
Gates: `gofmt` clean, `go vet ./...` ok, full `go test ./...` green on SQLite,
full `internal/store` green on Postgres — 460.9s, private container at
127.0.0.1:5473, run detached with a sentinel.
…ng it (BUG-2873) Codex round 3, one P2. `json.Decoder.Decode` stops at the end of the FIRST value and ignores whatever follows, where `json.Unmarshal` refuses it — so a stored schema with junk after the object would be silently truncated by the rewrite. It is now treated as unparseable and left alone, the same posture as any other schema this migration cannot faithfully reproduce. **The Postgres gate then failed the new test, and the failure is the finding.** It failed at the SEED, not the assertion: `ERROR: invalid input syntax for type json (SQLSTATE 22P02)`. `collections.schema` is TEXT on SQLite (`005_collections.sql:10`) and JSONB on Postgres (`pgmigrations/001_initial.sql:114`), so a value with trailing content cannot be STORED on Postgres at all. The state this guard defends against is reachable on one dialect and forbidden by the column type on the other. So the test skips on Postgres with that reason recorded. Asserting there would be asserting about a state that cannot exist — and reading WHICH LINE failed is what separated "my test is not portable" from "the product is broken on PG". **Second instance of the mutation harness reporting a false survivor**, same cause as the last: deleting the guard leaves `io` unused, the mutant fails to compile, and counting `--- FAIL` lines sees zero. With `_ = err` in place of the return it dies immediately. Twice in one unit makes it a harness defect, not bad luck: a runner that counts test failures must check the BUILD separately, or every non-compiling mutant reads as a hole in the tests. Mutation matrix 7 of 7 killed. Gates: `gofmt` clean, `go vet ./...` ok, full `go test ./...` green on SQLite, full `internal/store` green on Postgres (502.2s, private container at 127.0.0.1:5473, detached with a sentinel).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
models.FieldDef.Collectionholds the target's slug, and it is the only pointer a relation field carries — there is no id beside it.UpdateCollectionre-slugifies on rename and nothing migrated the definitions aimed at it, so every relation field pointing at a renamed collection was stranded: the picker filters on a slug that resolves to nothing and the field silently stops being fillable.retargetRelationFieldsTxre-points them in the same transaction as the rename, for the reason the field-value migrations already run there — a failure must roll the rename back rather than commit collections pointing at a name that no longer exists.Zero affected rows, deliberately
A census across every workspace on this instance found no relation fields anywhere, and no shipped template declares one. The feature has been unusable, which is what PLAN-2857 exists to change. So this repairs the rename path before the population exists, which is why there is no data migration — and the "zero live rows" property is what makes the change cheap to review. Three findings were split out rather than folded in to preserve it (below).
What it does that an obvious version does not
default, a select'soptions, a label. Onlycollectionon arelationfield is a reference. Export'sremapFieldIDsgets away with a blind replace because it substitutes UUIDs, which cannot collide with prose; a slug is a word. Pinned by a control test.FOR UPDATE, ordered by id, on Postgres). Otherwise a concurrent sibling schema update commits between the scan and the rewrite and gets clobbered by a stale copy.collections.updated_atdoubles as the concurrency token (BUG-2265): leaving it unchanged lets a client holding the pre-rename schema write it straight back; stamping the rename's value on a sibling that was updated later moves the token backwards. Each row takesmax(current + 1ns, renameToken), computed in Go rather than by comparing timestamp text, which the surrounding comment warns is never safe.models.CollectionSchemadrops unknown keys; decoding intointerface{}turns numbers into float64, so9007199254740993came back changed. Raw decode withUseNumber.Decodeignores what follows the first value whereUnmarshalrefuses it.schemawrite, so a simultaneous schema edit composes rather than being reverted.The deadlock hazard, and why the existing comment does not cover it
The lock-order comment above this transaction is a Codex P1 fix ordering the workspace lock against one collection row lock, because until now nothing took more than one. This change writes sibling rows, so two concurrent renames of mutually-referencing collections take them in opposite orders.
Reproduced, not theorised: with the serialization removed the test fails in 1.4s with
ERROR: deadlock detected (SQLSTATE 40P01)on Postgres. Renames now take the workspace lock — previously acquired only whenlen(input.Migrations) > 0— before the row lock, closing it without inventing a second ordering rule to keep in sync with the first.Raised in review by a teammate who read the function on being asked, and flagged that a reviewer would hit that comment and assume the question was settled. It was not.
Split out rather than folded in
Each has dependents that are not the relation feature, so none belongs in a change whose reviewability rests on affecting zero live rows:
uniqueSlugExcluding(s.db, …)at :420, befores.db.Begin()at :503). Its dependents are every collection rename in every workspace.UNIQUE(workspace_id, slug)makes today's behaviour loud rather than lossy, so it can wait. (The read of the old slug is fixed here — see below.)FOR UPDATE. Closing it meansCreateCollectiontakes the workspace lock, changing every collection creation on the instance.collection_updatedevent, so an open page keeps the pre-rename schema until reload. Handler layer; the store publishes nothing.The read of the old slug was in scope and is fixed: it came from the pre-transaction snapshot, so two tokenless concurrent renames both saw the original, and the loser migrated
original → its own new slugwhile the relations already said the winner's — matching nothing. It is now re-read alongside the token under the row lock.Three instruments that were not instruments
Every one found by mutation rather than by reading, and each would have shipped a false green:
--- FAILlines, so a mutant that fails to COMPILE reads as one that survived. Deleting the token guard leavesrowUpdatedAt/renameTokenunused; deleting the trailing-content guard leavesiounused. Both died immediately once made to compile. Twice in one unit is a harness defect, not bad luck.One dialect asymmetry the Postgres gate caught
TestRenameLeavesASchemaWithTrailingJunkUntouchedfailed on Postgres at the seed, not the assertion:invalid input syntax for type json (22P02).collections.schemais TEXT on SQLite and JSONB on Postgres, so the state that guard defends against cannot be stored there at all. The test skips on Postgres with the reason recorded. Reading which line failed is what separated "my test is not portable" from "the product is broken on PG".Verification
gofmtclean,go vet ./...ok.go test ./...green on SQLite.internal/storegreen on Postgres — 502.2s, private container at127.0.0.1:5473, never the shared 5445 a sibling seat may tear down. Run detached with a sentinel after the first attempt was killed at a turn boundary.