mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
cade263fe5
* fix(store): compare-and-set the wiki-link cascade's writes (BUG-2785)
A document rename cascades through every document linking the old title
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 committing between those two statements was silently overwritten
— the cascade wrote the body it built from the version it read, with no
error and no version row for the loss.
Same lost-update shape as BUG-2770's activity-metadata merge, one table
over, and fixed the same way: the UPDATE now carries `content = ?` with
the body the cascade read, plus a bounded retry that re-reads the row
and re-applies the rewrite.
DIALECT SCOPE, which decides what the tests can prove. Reachable on
POSTGRES only. SQLite's DSN sets `_txlock=immediate`, so UpdateDocument's
db.Begin() takes the write lock at BEGIN and holds it across the whole
read→write window; a concurrent edit cannot commit inside it and
serializes on busy_timeout instead. On Postgres under READ COMMITTED each
statement takes a fresh snapshot and the stale body wins. The CAS is
therefore a no-op on SQLite by construction — the predicate always
matches, because nobody else can have written.
Two consequences, both acted on rather than noted: the new tests SKIP
loudly on SQLite instead of passing for a reason unrelated to this fix,
and the mutation matrix was run under Postgres, where removing the CAS
leaves a SQLite suite entirely green.
The retry rewrites the WINNER's body rather than replaying the original
rewrite — replaying it would reintroduce exactly the text this bug loses.
A mutation that replays instead is in the matrix.
THE ZERO-ROW RESULT NEEDS A PROBE. The UPDATE now has two predicates that
can each refuse it, and RowsAffected cannot say which did: `deleted_at IS
NULL` (the linker was archived — a documented normal outcome, stop) or
`content = ?` (a concurrent edit landed — re-read and retry). Treating
them alike either retries forever against a deleted row or discards a
live linker's rewrite, so a probe distinguishes them, as BUG-2770 needed
for the same reason. The probe reads through tx, never the pool
(BUG-2409).
On retry exhaustion the RENAME fails rather than leaving one linker
holding a title that no longer exists. The alternative — log and continue
— was considered and rejected: it trades a loud retryable failure for a
silent inconsistency, and a rename is atomic in intent. Exhausting three
attempts needs three consecutive commits to the same linker inside one
cascade.
Adds afterLinkCascadeRead, the seam between the cascade's read and its
writes. No existing seam reaches that gap: afterDocumentPreLockRead fires
before the transaction, afterDocumentPreWrite before the renamed
document's own update.
That new seam also closes a gap a previous unit recorded as permanently
open: the `deleted_at IS NULL` guard carried a note calling itself
UNTESTED because reaching its window "would cost a fifth seam". This is
that fifth seam, so the note is removed and replaced by a record of the
closure, and the archived-linker test now drives exactly that window.
Mutation matrix, run under Postgres — 5 mutants, 5 detected, including
one that is literally the pre-fix code:
M1 CAS predicate removed (the unfixed behaviour) → 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: tests must notice they never raced)
→ lost-update test
Gates: gofmt clean, lint 0 issues, govulncheck clean, full suite green on
SQLite (28 pkgs) and on Postgres 17 (28 pkgs, internal/store 346s vs 79s
— the positive control that the PG legs ran rather than skipped).
Not fixed here, filed as BUG-2795: cascadeTitleRename, the ITEM-side
cascade, has the identical defect and no lock closes its window either.
Its fix does not transfer — it rewrites by POSITION from item_wiki_links
offsets, so a retry must re-derive positions from the winner's body and
re-run replaceWikiLinks, which is a redesign of the retry unit rather
than a predicate on an UPDATE. wiki_links.go's claim that it "matches the
document rename behavior" is corrected to say which half no longer
matches.
* style(store): gofmt the CONVE-23 sweep comment (BUG-2785)
Same failure as the previous unit, same cause, and worth naming rather
than quietly fixing: gofmt wants a blank line between list items once one
item grows a second paragraph, which the note about the previously-
untested deleted_at guard made true.
I re-ran build and the full Postgres suite after those comment edits but
not lint, because the change was 'only a comment' — the exact reasoning
the previous unit's fix commit warned about in writing, one unit earlier.
The PR body's claim that all gates were re-run after the prose edits was
false and has been corrected there too.
The rule as remembered does not work. The mechanical form does: gofmt and
lint are the last action before a push, comment-only changes included.
* fix(links,server): stop a rename hanging the server, and report cascade contention honestly (BUG-2785)
Three findings from Codex round 2 on this PR. The first is a server hang.
1. ReplaceTitle could never terminate. replaceAll looped "find old in
result, splice new in" — re-searching the string it was building,
including the text it had just inserted. When the NEW title contains
the OLD link token it grows without bound.
Measured, not argued: ReplaceTitle("x [[A]] y", "A", "A]] [[A")
builds `[[A]] [[A]]`, which still contains `[[A]]`; a probe against
the old implementation ran 3s without terminating before being
killed. Document titles have no validation, so this is reachable from
user input — and the caller is inside the rename transaction holding
the workspace rename advisory lock (BUG-2778), so the hang would take
every other rename in that workspace down with it while exhausting
memory.
strings.Replace with n = -1 has the semantics that were wanted:
non-overlapping, left-to-right, over the input. Three-case regression
test, all three of which fail against the old implementation, plus a
control that catches a "fix" which terminates by doing nothing.
Pre-existing, and folded in rather than filed: three lines against a
server hang, and this PR's retry calls the helper again per attempt,
which makes it reachable more often than before.
2. Retry exhaustion surfaced as an opaque 500. The rename rolls back
cleanly and retrying can succeed, so "an internal error occurred"
tells the caller the opposite of the truth. Adds the exported
ErrLinkCascadeContention sentinel; the handler now answers 503
lock_contention with Retry-After, reusing the disposition BUG-2778
already established for 55P03/40P01.
3. That 503's message claimed the workspace was "busy with another
rename". 55P03 there is just as likely to be an ordinary content edit
holding the row, and the new arm is definitely one. It no longer
names a cause the server has not established.
Also closes the coverage gap round 2 named around this unit's own
decision: exhaustion now has a test asserting the rename ROLLS BACK
(target keeps its title) and that the concurrent editor's text survives
that rollback. cascadeRewriteAttempts becomes a var so the test can
force exhaustion at 1 rather than arranging three consecutive commits,
which would need a per-attempt hook in production code — the divergence
from its const sibling is noted where it lives.
Records two limitations in the code rather than leaving "the cascade is
safe now" to rot: the MIRROR direction is still open (a content writer
that read before this transaction can commit afterwards and reinstate
the old title — fixing it means giving ordinary content writes a CAS
too), and delete-then-restore of a linker mid-rename brings back the old
title. Both pre-existing, neither worsened here.
Mutation matrix now 7 mutants, 7 detected, run under Postgres:
M6 (%w -> %v, sentinel lost) and M7 (exhaustion swallowed) cover the new
mechanisms.
Gates re-run on the tree being pushed, tests included, after the final
comment edit rather than before it: gofmt clean, lint 0 issues, SQLite
28/28, Postgres 17 28/28 (internal/store 345s vs 79s). gofmt caught an
unformatted test file locally this time, which is the point.
* fix(store,links): correct three prose claims and close the SQLite coverage gap (BUG-2785)
Codex round 4, on SQLite semantics and prose accuracy. One finding says a
bug I FILED is wrong; that is the important one.
1. BUG-2795's premise was false, and this PR repeated it in a comment.
I filed that item claiming the item-side cascade has "the identical
defect" and that "no lock closes its window either", on the strength
of a grep for pg_advisory_xact_lock in items.go that turned up only
the parent-link locks.
It missed acquireWorkspaceSeqLock (items.go:2136), taken
UNCONDITIONALLY by every UpdateItem — content-only edits included —
immediately after Begin and long before cascadeTitleRename, and held
to COMMIT. So two item updates in a workspace fully serialize on
Postgres and an ordinary content edit CANNOT commit inside that
cascade's window. The scenario I filed is not reachable.
Not fully invalid: sweeping every `SET content` writer in
internal/store finds RemapAttachmentReferencesInWorkspace, which
rewrites items.content in its own transaction without that lock. So a
real but far narrower window survives — attachment remap versus
cascade, not user-edit versus cascade. BUG-2795 corrected on its
trail and dropped to low; the comment here now states the lock, the
one surviving writer, and stops claiming parity with the document
cascade.
I searched for the locks I expected rather than for what serializes
that path, then wrote a sentence broader than the search. Same
failure this PR's review has produced repeatedly.
2. "A title nobody should be able to write" was false — the document API
validates doc_type and status, never the title. Correcting it
surfaced a real second defect: renaming to `A]] [[A` now terminates
(round 2's fix) but writes `[[A]] [[A]]`, two links to nothing. Filed
as BUG-2796. The termination test deliberately still asserts the
COUNT rather than the output, so it does not freeze today's broken
rendering as intended behaviour.
3. The hang's blast-radius claim named the workspace rename advisory
lock without qualifying the dialect. That lock is a no-op on SQLite,
where the equivalent damage is the database-wide write lock the
transaction already holds under BEGIN IMMEDIATE. Different mechanism,
same outcome for everyone else.
Also closes the last coverage gap round 2 named. The CAS predicate runs
on SQLite in production and every concurrency test skips there, so
TestUpdateDocument_CascadeRewritesEveryLinkOnBothDialects does not skip:
two linkers, multiple links per body, plus a document that merely
contains the word and must be left alone. Verified it earns its place —
a mutant comparing against the rewritten body instead of the body that
was read compiles, and dies to this test ON SQLITE, where nothing else
would have caught it.
Documents the parallel-test constraint on the now-mutable
cascadeRewriteAttempts, with its boundary: no current t.Parallel test in
internal/store reaches this cascade, but that is a fact about today's
corpus rather than an invariant.
Gates on the pushed tree: gofmt clean, lint 0 issues, govulncheck clean,
SQLite 28/28, Postgres 17 28/28 (internal/store 349s vs 79s).
* docs(store,links): point the cascade comment at the root cause, qualify a claim in its second location (BUG-2785)
Codex round 5, probing cross-connection visibility and transaction
boundaries. Two findings, both mine, neither changing behaviour.
1. The "one surviving writer" framing was too comfortable, twice over.
RemapAttachmentReferencesInWorkspace is not a rare non-interactive
writer: bundle import reaches it on an ordinary user-triggered
import, and the workspace is ALREADY VISIBLE to its owner while it
runs — store.ImportWorkspace commits the workspace row (with
owner_id) in its own transaction before opening the one that inserts
items, and the bundle handler runs the remap as Phase 3 afterwards.
Both verified in code.
So that writer races ordinary item edits, not just the rename
cascade, and it is missing a guard outright rather than being an
exotic pairing. Filed as BUG-2797, which covers the remap itself;
BUG-2795 is now a consequence of it and says so on its trail. The
comment here points at the root cause instead of implying the
cascade's pairing is the whole story.
2. The dialect-unqualified advisory-lock claim survived in a SECOND
location. Round 4 caught it in links.go and I fixed it there; the
same sentence sat in the termination test's comment, and I never
enumerated the sites. That is CONVE-23's verify half failing exactly
as it warns: I fixed the instance I was shown rather than the
population. Both now qualified, and a sweep for remaining unqualified
copies leaves only BUG-2778's own comment, which already carries its
no-op-on-SQLite note.
Worth recording that this is the second time on BUG-2795 that a scope
sentence of mine ran ahead of the sweep supporting it — first "no lock
closes its window either" (acquireWorkspaceSeqLock did), now "not user
triggered" (import is). Both found by a review round rather than by me.
Gates on the pushed tree: gofmt clean, lint 0 issues, SQLite 28/28,
Postgres 17 28/28 (internal/store 348s vs 79s).
74 lines
3.1 KiB
Go
74 lines
3.1 KiB
Go
package links
|
|
|
|
import (
|
|
"strings"
|
|
"testing"
|
|
"time"
|
|
)
|
|
|
|
// ReplaceTitle used to re-scan its own output: it looped "find old in result,
|
|
// splice new in" until no match remained, so the text it had just inserted was
|
|
// searched again. When the NEW title contains the OLD link token that never
|
|
// terminates and the string grows without bound.
|
|
//
|
|
// The caller runs inside the rename transaction, so a hang here does not merely
|
|
// wedge one request — it holds that transaction open while exhausting memory.
|
|
// On Postgres it also holds the workspace rename advisory lock (BUG-2778) and
|
|
// blocks every other rename in the workspace; on SQLite that lock is a no-op
|
|
// and the transaction's own BEGIN IMMEDIATE write lock does the equivalent
|
|
// damage.
|
|
//
|
|
// Found by Codex round 2 on BUG-2785, while enumerating the ways the cascade's
|
|
// retry loop could fail to terminate.
|
|
func TestReplaceTitle_TerminatesWhenNewTitleEmbedsOldToken(t *testing.T) {
|
|
// `[[A]]` -> `[[A]] [[A]]`, whose output still contains `[[A]]`. Against the
|
|
// old implementation this grew until it was killed; a probe ran 3s without
|
|
// finishing.
|
|
cases := []struct{ name, old, new string }{
|
|
{"new title re-embeds the whole old token", "A", "A]] [[A"},
|
|
{"new title re-embeds it twice", "A", "A]] [[A]] [[A"},
|
|
{"old token embedded mid-title", "X", "pre A]] [[X]] [[post"},
|
|
}
|
|
|
|
for _, tc := range cases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
done := make(chan string, 1)
|
|
go func() { done <- ReplaceTitle("x [["+tc.old+"]] y", tc.old, tc.new) }()
|
|
|
|
select {
|
|
case got := <-done:
|
|
// The property under test is TERMINATION, and specifically that
|
|
// the substitution happens once per occurrence in the INPUT.
|
|
//
|
|
// Asserting the exact output would pin escape semantics this
|
|
// codebase does not have: the document API accepts these titles
|
|
// (handlers_documents.go validates doc_type and status, never the
|
|
// title), and ReplaceTitle emits them raw, so renaming to
|
|
// `A]] [[A` yields `[[A]] [[A]]` — two links to nothing rather
|
|
// than one link to the new title. That is a REAL second defect,
|
|
// filed as BUG-2796; it is not what this test is about, and
|
|
// pinning the output here would freeze the broken rendering as
|
|
// though it were intended.
|
|
if n := strings.Count(got, "[["+tc.new+"]]"); n != 1 {
|
|
t.Errorf("substituted %d times, want exactly 1 (one occurrence in the input)\n got: %q", n, got)
|
|
}
|
|
case <-time.After(5 * time.Second):
|
|
t.Fatal("ReplaceTitle did not terminate: the replacement re-scanned its own output, " +
|
|
"which grows without bound when the new title contains the old link token")
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// The ordinary path has to keep working, and this is the control that would
|
|
// catch a "fix" that terminated by doing nothing.
|
|
func TestReplaceTitle_StillRewritesEveryOccurrence(t *testing.T) {
|
|
got := ReplaceTitle("[[Old]] middle [[Old]] end", "Old", "New")
|
|
if want := "[[New]] middle [[New]] end"; got != want {
|
|
t.Errorf("got %q, want %q", got, want)
|
|
}
|
|
if strings.Contains(got, "[[Old]]") {
|
|
t.Errorf("an occurrence survived: %q", got)
|
|
}
|
|
}
|