fix(store): compare-and-set the wiki-link cascade's writes (BUG-2785)

[xarmian]

Closes BUG-2785.

The defect

A document rename cascades through every document whose content links the old title. Store.updateLinksInTx does that as a read-modify-write across two statements: SELECT each linker's content, rewrite the string in Go, UPDATE the row.

A content edit to a linker that commits between those two statements was silently overwritten — the cascade's UPDATE was unconditional, so it wrote the body it built from the version it read. No error, and no version row for the loss, because the cascade does not create versions for the documents it rewrites.

Same lost-update shape as BUG-2770's activity-metadata merge, one table over.

Dialect scope — the fact that decides what any test here can prove

Reachable on Postgres only.

SQLite's DSN sets _txlock=immediate (see store.go), so UpdateDocument's db.Begin() issues BEGIN IMMEDIATE and takes the write lock at BEGIN, holding it across the cascade's entire read→write window. A concurrent content edit cannot commit inside it — it serializes on busy_timeout. On Postgres under READ COMMITTED each statement takes a fresh snapshot, the edit commits between the two statements, and the stale body wins.

Three things follow, all acted on rather than just noted:

• The new tests SKIP loudly on SQLite rather than passing. A green run there would be a property of the DSN, not evidence about this fix. (Driving the seam on SQLite is worse than useless: the concurrent write would block on the lock the rename already holds, stall for busy_timeout, and then assert about a serialized order that has nothing to do with the defect.)
• The mutation matrix was run under Postgres. Removing the compare-and-set leaves the SQLite suite entirely green — a matrix run there would certify nothing while looking perfect.
• The CAS is a no-op on SQLite by construction: the predicate always matches, because nobody else can have written.

This is the dual-dialect foot-gun the repo warns about, arriving in the form where the SQLite side is the one that looks fine.

The fix

UPDATE documents SET content = ? WHERE id = ? AND deleted_at IS NULL AND content = ?, with the body the cascade read as the compare-and-set token, and a bounded 3-attempt retry.

The retry rewrites the winner's body, not a replay of the original. Replaying the original rewrite would reintroduce exactly the text this bug loses. M4 in the matrix is that mutation.

The zero-row result needs a probe. The UPDATE now carries two predicates that can each refuse it, and RowsAffected cannot say which did:

predicate	meaning	correct response
deleted_at IS NULL	linker archived after the SELECT — a documented normal outcome, guard predates this change	stop; there is no link left to keep consistent
content = ?	a concurrent edit landed	re-read, re-apply the rewrite to the new body, retry

Treating them alike either retries forever against a deleted row or discards a live linker's rewrite. So a refusal is followed by a probe that tells them apart — the same shape BUG-2770 needed for the same reason. The probe reads through tx, never the pool: a pool read from inside a transaction holding row locks is BUG-2409's deadlock.

On exhaustion the rename fails. Considered and rejected: log-and-continue, which trades a loud retryable failure for a silent inconsistency. A rename is atomic in intent — either the title moves and its links follow, or neither — and a user can retry a failed rename, whereas nobody goes looking for a stale wiki-link. Exhausting three attempts needs three consecutive commits to the same linker inside one cascade, which is pathological rather than merely contended.

The seam, and a gap it closes that a previous unit had written off

afterLinkCascadeRead fires after every linker's content is read and before any is written back. No existing seam reaches that gap: afterDocumentPreLockRead fires before the transaction, afterDocumentPreWrite before the renamed document's own update — a different window on a different row.

The deleted_at IS NULL guard carried a note from a previous unit calling itself UNTESTED and deliberately kept, because reaching its window needed a seam nothing could schedule "and which would cost a fifth one". This change adds that fifth seam for its own reasons, so the note is no longer true. It is removed and replaced by a record of the closure — TestUpdateDocument_CascadeTreatsSoftDeletedLinkerAsDone now drives exactly that window, and M2 dies to it. Deleting another unit's deliberate "this is untestable" finding is a claim in its own right, so it is written down rather than quietly dropped.

Tests

Two, both Postgres-only with loud skips. The lost-update test asserts three things, each catching a different wrong fix (CONVE-12 — assert what the WRONG behaviour would DO):

• the concurrent edit's text survives — an unconditional UPDATE erases it; this is the defect
• the link is rewritten to the new title — a "fix" that simply skipped a contended linker would preserve the edit and silently abandon the cascade
• the old title is gone — without this, a rewrite that APPENDED rather than replaced would pass the second assertion

Mutation matrix — 5 mutants, 5 detected, run under Postgres

mutant	detected by
M1 CAS predicate removed — literally the pre-fix code	lost-update test
M2 soft-delete probe arm removed	archived-linker test
M3 retry budget cut to one attempt	lost-update test
M4 retry replays the original rewrite	lost-update test
M5 seam removed — control: the tests must notice they never raced	lost-update test

M1 being the unfixed behaviour is the receipt that these tests fail against broken code rather than passing for their own reasons. M5 is the control that matters most for a concurrency test: with the seam gone no race is scheduled at all, and a test that still passed would be measuring nothing.

Not fixed here — BUG-2795

cascadeTitleRename, the item-side cascade, has the identical defect, and no lock closes its window either (the pg_advisory_xact_lock calls on that path cover parent-link cycles and parent-children ordering, not title renames).

Filed rather than folded, for a reason rather than convenience: the fix does not transfer. The document cascade rewrites by SEARCHING for [[oldTitle]], so re-applying it to a different body is just running the same search again. The item cascade rewrites by POSITION, using offsets read from item_wiki_links — after a concurrent edit those offsets are stale, so a retry must re-derive them from the winner's body, re-query the index inside the retry loop, and re-run replaceWikiLinks against the body it actually wrote. That is a redesign of the retry unit, not a predicate added to an UPDATE.

Found by this PR's CONVE-23 prose sweep: wiki_links.go claimed the item cascade "matches the document rename behavior — see documents.go::updateLinksInTx", which is what made the population question obvious once one of the two was fixed. That comment is corrected here to say which half no longer matches and to point at BUG-2795.

One review finding declined, with the reasoning recorded

Codex round 6 (dimension: mixed-version rolling deployment) raised as P2 that during a rolling upgrade an old replica still runs the unconditional cascade, so the lost update remains possible until every writer is upgraded — and proposed an explicit drain / blue-green requirement before relying on the fix.

Declined as a blocker, and the "drain requirement" declined outright:

• The interim state is exactly the status quo ante. An old replica behaves as every replica has behaved for as long as this bug has existed. The rolling window makes the fix arrive unevenly; it introduces no failure mode that is not already live.
• A mixed fleet is not worse than a homogeneous old one. New-instance CAS versus old-instance unconditional write ends with the old write winning — identical to two old instances. There is no interaction that is worse than either end state.
• Unlike an authorization change, there is nothing here an actor can retry into. A rename either races a concurrent edit or does not; nobody can aim at an un-upgraded node to reopen the window. (That distinction is what made me correct my reasoning on IDEA-2756's equivalent finding rather than simply reusing the decline — there, a refused caller genuinely could retry until it hit an old node. Here it does not apply.)
• No schema, no migration, so rollback is clean and reverting simply restores the current behaviour.

What is true and worth stating: this is an ordinary behaviour change with no schema component and no sequencing requirement. Deploy it normally; protection appears per-instance as instances update. Demanding a drain or blue-green for a fix whose interim state is "what production already does" would be ceremony, not safety.

Gates

• gofmt clean; make lint 0 issues; make vuln no vulnerabilities affecting this code

• full Go suite on SQLite: 28 packages, 0 failures

• full suite against Postgres 17 on a private container (port 5463, not the shared 5445 — a sibling seat is live): 28 packages, 0 failures. internal/store took 346s vs 79s on SQLite — the positive control that the PG legs ran rather than skipped, which matters more than usual here because this fix is inert on SQLite

• Correction to this line as originally written. It claimed all gates were re-run after the prose edits, on the tree actually pushed. That was false: after the CONVE-23 comment edits I re-ran build and the Postgres suite but NOT lint, and CI caught a gofmt violation in one of those very comments (documents.go:665 — a list item that grew a second paragraph). Fixed in a follow-up commit; lint now 0 issues on the pushed tree, verified after the edit rather than before it.

  Worth stating plainly because it is the second time in one session, with the same cause: the previous unit's CI failure was the identical gofmt-in-a-doc-comment, and its fix commit message says "the gate has to run on the tree being pushed, not on an earlier one that resembles it". Knowing the rule did not prevent it. The reliable form is mechanical — run gofmt/lint as the last action before every push, including a comment-only one — not a rule to remember at the moment of judgement.