diff --git a/internal/models/document.go b/internal/models/document.go index c5462152..7fde638d 100644 --- a/internal/models/document.go +++ b/internal/models/document.go @@ -1,6 +1,11 @@ package models -import "time" +import ( + "fmt" + "strings" + "time" + "unicode/utf8" +) // Valid document types var ValidDocTypes = []string{ @@ -72,6 +77,113 @@ type DocumentListParams struct { Order string } +// MaxDocumentTitleRunes bounds a document title at write time. +// +// Two reasons, and the second is the load-bearing one: +// +// - A title is emitted into every linking document by the rename cascade +// (`[[newTitle]]` per occurrence), so its length is an amplification +// factor on a body the renamer does not supply. Unbounded, one rename +// could project 10 GB from a 500 KB input — measured, 20,000x (BUG-2798). +// - 255 is the conventional identifier-ish bound and comfortably above any +// real title; the longest document title this codebase seeds is far short +// of it. +// +// RUNES, not bytes, because "255 characters" is what a user and a UI counter +// mean. That makes the byte-level residual up to 4x this number, which is +// precisely why the title bound is the cheap door and NOT the wall: the +// cascade's own guard (store.MaxRenameCascadeRetainedBytes) is byte-accurate +// and is what actually bounds the work. +// +// Enforced at WRITE time only. Titles already over the bound stay valid and +// keep working until their next rename — no retro-breakage of stored data +// (Dave's ruling, day-63). +const MaxDocumentTitleRunes = 255 + +// wikiTitleRoundTripFailure reports why emitting `[[title]]` the way +// links.ReplaceTitle does — plain concatenation, no escaping — would produce a +// bracket that does not read back as this title. Returns "" when the title +// survives the round trip. +// +// This is BUG-2796 stated as a property rather than as a character blacklist, +// and the distinction is not cosmetic: the first version of this function +// banned `]`, `\` and `|` on the reasoning that all three "look like +// wiki-link syntax", and the test below refuted two thirds of that. The rule +// is therefore derived from the two mechanisms that actually consume a stored +// bracket, both in web/src/lib/utils/markdown.ts: +// +// 1. THE GRAMMAR (markdown.ts:327, shared by renderMarkdown and +// wikiLinksToMarkdown since BUG-1744): a bracket body is +// `(?:\\.|[^\]\\])+` — escape pairs, or anything that is neither `]` nor +// `\`. A raw `]` ends the bracket early, which IS BUG-2796's defect: +// renaming to `A]] [[A` emitted `[[A]] [[A]]`, two brackets, neither +// resolving to the renamed document, and the rename reported success. A +// title ending in `\` fails the same way — the trailing backslash pairs +// with the first `]` of the terminator and the bracket never closes. +// +// 2. THE UNESCAPER (markdown.ts:753, `\\(\\|\]|\|)` → `$1`): resolution +// unescapes the body before comparing it to a title, and the editor may +// therefore STORE a link in escaped form. That matters twice over: a +// title containing `\\` or `\|` emitted raw comes back as a different +// string, AND a link stored as `[[Alpha\|Beta]]` is invisible to the +// rename cascade, which searches for the raw `[[Alpha|Beta]]` only. +// +// The second half is why `|` and a lone `\` are refused (codex round 9), +// having been ALLOWED in the first version of this validator. The property +// tested there was "does the renderer read this back as the same title", and +// both characters pass it. That was the wrong property: a title also has to be +// one whose links the cascade can FIND, or a rename silently leaves them +// pointing at a name that no longer exists. The stricter property is the one +// that matters, and it is the same mistake as validating against the renderer +// while the cascade used an unescaped LIKE — checking the layer that displays +// a title instead of the layer that has to maintain it. +// +// Deliberately NOT rejected, because the code these titles pass through +// handles them and refusing them would be a validator inventing a defect: +// +// - `[` — the grammar excludes only `]` and `\`, so `[[A[B]]` carries the +// body `A[B` intact, and the cascade's search term matches it literally. +// BUG-2796's filing proposed rejecting "`[[` or `]]`"; measured against +// the grammar, the `[[` half of that is overreach. +// +// Boundary, stated rather than papered over: this is derived from the SHARED +// stored-syntax path in markdown.ts. The legacy documents surface has no +// renderer of its own that I could locate — every wiki-link consumer found +// routes through these two functions — so the rule is pinned to them. +func wikiTitleRoundTripFailure(title string) string { + if strings.Contains(title, "]") { + return `Title may not contain "]" — it would end the [[wiki-links]] that point at this document early, ` + + `turning them into broken links` + } + if strings.HasSuffix(title, `\`) { + return `Title may not end with "\" — it would escape the closing bracket of the [[wiki-links]] that point ` + + `at this document` + } + if strings.ContainsAny(title, `\|`) { + return `Title may not contain "\" or "|" — a link to a title containing them can be stored in ` + + `escaped form, which the rename cascade would not find, silently leaving those links pointing ` + + `at a title that no longer exists` + } + return "" +} + +// ValidateDocumentTitle checks a document title at write time. Returns a +// message suitable for a 400 response, or "" when the title is acceptable. +// +// Covers BUG-2798 (length, which is an amplification factor on OTHER +// documents' bodies, not merely a field-size preference) and BUG-2796 +// (wiki-link syntax the cascade emits raw). Both are write-time doors on the +// same field, which is why they share one validator and one insertion point. +func ValidateDocumentTitle(title string) string { + if title == "" { + return "Title is required" + } + if n := utf8.RuneCountInString(title); n > MaxDocumentTitleRunes { + return fmt.Sprintf("Title is too long: %d characters, maximum %d", n, MaxDocumentTitleRunes) + } + return wikiTitleRoundTripFailure(title) +} + func IsValidDocType(t string) bool { for _, v := range ValidDocTypes { if v == t { diff --git a/internal/models/document_title_test.go b/internal/models/document_title_test.go new file mode 100644 index 00000000..c72c2db6 --- /dev/null +++ b/internal/models/document_title_test.go @@ -0,0 +1,183 @@ +package models + +import ( + "regexp" + "strings" + "testing" +) + +// The two mechanisms a stored wiki-link bracket passes through, mirrored from +// web/src/lib/utils/markdown.ts so this test can derive the validator's rule +// from them instead of from an opinion about which characters look dangerous. +// +// Duplicated rather than approximated — and duplication is exactly the risk +// here, stated rather than papered over: these are STATIC copies, so a change +// to the TypeScript does not reach them. Nothing in this repository fails when +// the two drift (codex round 6). What this buys is that the Go rule is derived +// from a written-down grammar rather than from an opinion, and that a reader +// can check the two by eye at the cited line numbers. Closing the drift for +// real would need a shared fixture driven from both languages. +var ( + // markdown.ts:327 — shared by renderMarkdown and wikiLinksToMarkdown. + storedWikiLinkBracket = regexp.MustCompile(`\[\[((?:\\.|[^\]\\])+)\]\]`) + // markdown.ts:753 — unescapeWikiBody, applied before title comparison. + wikiBodyEscape = regexp.MustCompile(`\\(\\|\]|\|)`) +) + +// roundTrips reports whether emitting `[[title]]` the way links.ReplaceTitle +// does — plain concatenation, no escaping — produces a bracket that reads back +// as exactly this title. +// +// Both layers, because either alone gives a wrong answer. The grammar alone +// accepts `A\\B` (a valid escape pair) which the unescaper then turns into +// `A\B`, a different title. The unescaper alone accepts `A]B`, which the +// grammar cuts short. +func roundTrips(title string) bool { + emitted := "[[" + title + "]]" + m := storedWikiLinkBracket.FindStringSubmatch(emitted) + if m == nil || m[0] != emitted { + // No match, or a match over a PREFIX of the emission — the latter is + // BUG-2796's early-termination defect, where `[[A]] [[A]]` matched + // twice and neither match was the renamed document. + return false + } + return wikiBodyEscape.ReplaceAllString(m[1], "$1") == title +} + +// TestDocumentTitleValidationMatchesTheRoundTripProperty is the justification +// for the SYNTAX rule, in executable form: of the titles this table covers — +// all of them within the length bound — the validator must reject one if and +// only if its links would not survive being rewritten to it. +// +// Scoped to syntax deliberately. ValidateDocumentTitle also enforces length +// and non-emptiness, and those are NOT round-trip properties: a 300-rune title +// round-trips perfectly and is still refused, for the unrelated reason that it +// is an amplification factor on other documents. Stating the biconditional +// over the whole validator would be false (codex round 6); the length rule has +// its own tests below. +// +// Both directions are asserted, because either alone permits a wrong answer. +// Without the reject leg, a validator that accepted everything passes. +// Without the accept leg, a validator that rejected `[`, `|` or a lone `\` +// passes — and that is not hypothetical: the first version of this fix banned +// `]`, `\` and `|` as a character class, and this test refuted two thirds of +// it. `|` in particular is a title shape resolveWikiBody contains a dedicated +// branch to support. +func TestDocumentTitleValidationMatchesTheRoundTripProperty(t *testing.T) { + for _, tc := range []struct { + name string + title string + }{ + // Must be REJECTED — each breaks the round trip by a different + // mechanism. + {"bug-2796 repro", `A]] [[A`}, + {"single close bracket", `Alpha]Beta`}, + {"trailing backslash", `Alpha\`}, + {"escaped backslash", `Alpha\\Beta`}, + {"escaped pipe", `Alpha\|Beta`}, + + // Must be ACCEPTED — each looks like wiki-link syntax and none of + // them breaks anything. + {"open bracket", `Alpha[Beta`}, + {"double open bracket", `Alpha[[Beta`}, + {"collection-qualified shape", `collection/Title`}, + {"colons", `Ratio 1:2`}, + {"ordinary punctuation", `Title (draft) — v2`}, + {"non-latin", `Ünïcödé título`}, + } { + t.Run(tc.name, func(t *testing.T) { + wantAccept := roundTrips(tc.title) + msg := ValidateDocumentTitle(tc.title) + gotAccept := msg == "" + if gotAccept != wantAccept { + t.Errorf("title %q: round-trips=%v but validator accepts=%v (message: %q)\n"+ + "the validator and the renderers must agree — a rejected title that renders "+ + "correctly is a refusal of valid input; an accepted title that does not is BUG-2796", + tc.title, wantAccept, gotAccept, msg) + } + }) + } +} + +// TestDocumentTitleBoundIsTheRuledNumber pins the VALUE, not just the +// behaviour around it. +// +// Every other length test derives its inputs from MaxDocumentTitleRunes, so +// changing the constant to 512 would leave them all green (codex round 4). +// That is fine for arithmetic but wrong for this number: 255 is a product +// decision Dave ruled on for BUG-2798, and a silent change to it is a silent +// change to how much amplification the cheap door lets through. Changing it +// should require editing this line and noticing why. +// +// Deliberately NOT done for store.MaxRenameCascadeRetainedBytes: that one is +// mine, chosen with a measured receipt in its doc comment, and is expected to +// be tuned if the measurements change. +func TestDocumentTitleBoundIsTheRuledNumber(t *testing.T) { + if MaxDocumentTitleRunes != 255 { + t.Errorf("MaxDocumentTitleRunes = %d, want 255 (Dave's day-63 ruling on BUG-2798). "+ + "If this is a deliberate product change, update the ruling reference too.", + MaxDocumentTitleRunes) + } +} + +// TestDocumentTitleLengthBoundCountsRunesNotBytes pins the bound at 255 +// CHARACTERS, which is what the ruling says and what a UI counter shows. +// +// The multibyte legs are the counterfactual: 255 four-byte runes are 1020 +// bytes, so an implementation that reached for len() would reject them. That +// implementation would be wrong in a user-visible way — an ordinary +// 255-character title in a non-Latin script refused — while every ASCII-only +// test stayed green. +func TestDocumentTitleLengthBoundCountsRunesNotBytes(t *testing.T) { + for _, tc := range []struct { + name string + title string + accept bool + }{ + {"at the bound, ascii", strings.Repeat("a", MaxDocumentTitleRunes), true}, + {"one over, ascii", strings.Repeat("a", MaxDocumentTitleRunes+1), false}, + {"at the bound, 2-byte runes", strings.Repeat("é", MaxDocumentTitleRunes), true}, + {"at the bound, 4-byte runes", strings.Repeat("𝄞", MaxDocumentTitleRunes), true}, + {"one over, 4-byte runes", strings.Repeat("𝄞", MaxDocumentTitleRunes+1), false}, + {"empty", "", false}, + } { + msg := ValidateDocumentTitle(tc.title) + if tc.accept && msg != "" { + t.Errorf("%s: rejected (%s), want accepted", tc.name, msg) + } + if !tc.accept && msg == "" { + t.Errorf("%s: accepted, want rejected", tc.name) + } + } +} + +// TestDocumentTitleRejectsFormsTheCascadeCannotFind covers the two characters +// whose refusal the round-trip property above does NOT explain — and that gap +// is the finding, not an oversight in the table. +// +// `Alpha|Beta` and `Alpha\Beta` both round-trip perfectly: the renderer reads +// each back as exactly the title it started from. The first version of this +// validator accepted them for precisely that reason, and it was wrong (codex +// round 9). A link to such a title can be STORED in escaped form — +// `[[Alpha\|Beta]]` — which the rename cascade, searching for the raw +// `[[Alpha|Beta]]`, will not find. The rename then succeeds and leaves those +// links pointing at a title that no longer exists. +// +// So the property a title must satisfy is stricter than "the renderer reads it +// back": it must also be one whose links the cascade can FIND. Validating +// against the display layer while the maintenance layer disagrees is the same +// mistake as the unescaped LIKE, met from the other side. +func TestDocumentTitleRejectsFormsTheCascadeCannotFind(t *testing.T) { + for _, title := range []string{`Alpha|Beta`, `Alpha\Beta`} { + // The premise: each of these DOES round-trip. If that ever stops being + // true they are rejected for the ordinary syntax reason and this test + // is no longer testing what it says. + if !roundTrips(title) { + t.Fatalf("premise: %q must round-trip, or its rejection needs no separate justification", title) + } + if ValidateDocumentTitle(title) == "" { + t.Errorf("ValidateDocumentTitle(%q) accepted a title whose links the cascade cannot find; "+ + "a rename would leave them stale", title) + } + } +} diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 6fdd8ce1..30b14e85 100644 --- a/internal/server/handlers_documents.go +++ b/internal/server/handlers_documents.go @@ -66,8 +66,8 @@ func (s *Server) handleCreateDocument(w http.ResponseWriter, r *http.Request) { return } - if input.Title == "" { - writeError(w, http.StatusBadRequest, "bad_request", "Title is required") + if msg := models.ValidateDocumentTitle(input.Title); msg != "" { + writeError(w, http.StatusBadRequest, "bad_request", msg) return } if input.DocType != "" && !models.IsValidDocType(input.DocType) { @@ -137,6 +137,27 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { return } + // A title write here is the RENAME door — the one that cascades into every + // linking document — so it carries the same validation as create. Update + // had none at all before BUG-2798/BUG-2796: doc_type and status were + // checked and the field that drives the cascade was not. + // + // Only when the title actually CHANGES, though. Grandfathering existing + // over-limit titles is not a thing you get by validating at write time — + // it is a thing you get by not validating a write that is not a rename + // (codex round 10). A client PATCHing content with the full object, title + // included, is the ordinary shape of an edit; validating the echoed-back + // value would make every document with a legacy title uneditable rather + // than merely un-renameable, which is the opposite of the promise this + // fix's own comments make. The store applies the same test before + // cascading (documents.go, `*input.Title != existing.Title`), so this + // matches where the work actually happens. + if input.Title != nil && *input.Title != doc.Title { + if msg := models.ValidateDocumentTitle(*input.Title); msg != "" { + writeError(w, http.StatusBadRequest, "bad_request", msg) + return + } + } if input.DocType != nil && !models.IsValidDocType(*input.DocType) { writeError(w, http.StatusBadRequest, "bad_request", "Invalid doc_type") return @@ -148,6 +169,49 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { updated, err := s.store.UpdateDocument(doc.ID, input) if err != nil { + // TYPED checks first, prose matching last. The UNIQUE-constraint arm + // below identifies its error by SUBSTRING, and the refusal error + // carries the caller's own title verbatim — so a document renamed to a + // title containing the words "UNIQUE constraint" came back as a 409 + // title collision, telling the caller to pick a different name for a + // rename that was refused for size and would fail identically under + // any name (codex round 3 P2). Any sentinel this handler knows by + // identity is tested before an error is classified by what its text + // happens to contain. + // + // A size refusal is also PERMANENT-shaped, and must not join the + // retryable family below: retrying it unchanged fails identically + // until the workspace's content changes, so it gets a 4xx with no + // Retry-After, carrying the projection because that is the only + // actionable information. 413 follows this codebase's own precedent + // for refusing work whose COST is not in the request body + // (`image_too_large` in handlers_attachments_transform.go, where the + // request is likewise small and what is refused is what handling it + // would take). The precedent's quantity is output size and this + // guard's is retained content; what carries across is the shape — + // a small request refused for what it would cost, not for its size. + // The store re-checks the title under the rename lock, which is the + // authoritative point; this arm surfaces that check for the case the + // handler's pre-lock comparison could not see — a concurrent rename + // turning an echoed legacy title into a real one (codex round 11). + var badTitle *store.InvalidDocumentTitleError + if errors.As(err, &badTitle) { + writeError(w, http.StatusBadRequest, "bad_request", badTitle.Reason) + return + } + var tooLarge *store.RenameCascadeTooLargeError + if errors.As(err, &tooLarge) { + // Composed from TYPED fields, never by splicing err.Error(). The + // two figures are meant to reach the caller — the refusal has to + // be actionable — but the internal call path wrapped around them + // is not, and appending the error text published whatever any + // layer had prefixed (codex round 5). + writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large", + fmt.Sprintf("This rename would rewrite more linked content than the server will process in one "+ + "operation: at least %d bytes, and the limit is %d. Reduce the number of documents linking "+ + "this title, or shorten the new title, and try again.", tooLarge.Retained, tooLarge.Max)) + return + } if strings.Contains(err.Error(), "UNIQUE constraint") { writeError(w, http.StatusConflict, "conflict", "A document with this title already exists in this workspace") return diff --git a/internal/server/handlers_documents_title_test.go b/internal/server/handlers_documents_title_test.go new file mode 100644 index 00000000..177aab89 --- /dev/null +++ b/internal/server/handlers_documents_title_test.go @@ -0,0 +1,263 @@ +package server + +import ( + "fmt" + "net/http" + "regexp" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-2798 / BUG-2796, at the route rather than at the validator. +// +// CONVE-19: models.ValidateDocumentTitle having a test proves the validator +// works, not that anything calls it. Document UPDATE in particular validated +// doc_type and status and did NOT validate title — the one field that drives +// the rename cascade — so the binding is the claim worth testing. These drive +// real requests through srv.ServeHTTP. + +func createDocForTest(t *testing.T, srv *Server, wsSlug, title, content string) string { + t.Helper() + rr := doRequest(srv, "POST", "/api/v1/workspaces/"+wsSlug+"/documents", map[string]interface{}{ + "title": title, + "content": content, + "doc_type": "notes", + "status": "active", + }) + if rr.Code != http.StatusCreated { + t.Fatalf("create document %q: got %d, want 201: %s", title, rr.Code, rr.Body.String()) + } + var doc models.Document + parseJSON(t, rr, &doc) + return doc.ID +} + +// TestDocumentTitleValidation_IsWiredOnBothWriteDoors covers create and update +// with the same table. The update legs are the ones that would have caught the +// original defect: before this fix a PATCH could set any title at all. +func TestDocumentTitleValidation_IsWiredOnBothWriteDoors(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + docID := createDocForTest(t, srv, slug, "Starting Title", "body") + + for _, tc := range []struct { + name string + title string + want int + }{ + {"over the length bound", strings.Repeat("a", models.MaxDocumentTitleRunes+1), http.StatusBadRequest}, + {"bug-2796 bracket repro", `A]] [[A`, http.StatusBadRequest}, + {"empty", "", http.StatusBadRequest}, + + // Controls. Without these a handler that rejected every title — or + // one that rejected any title containing punctuation — passes the + // legs above. (`Alpha|Beta` was a control here until codex round 9: + // it renders correctly but its links can be stored in a form the + // cascade cannot find, so it moved to the refused side. The remaining + // controls carry the load.) + {"at the length bound", strings.Repeat("a", models.MaxDocumentTitleRunes), http.StatusOK}, + {"contains a literal pipe", `Alpha|Beta`, http.StatusBadRequest}, + {"ordinary", "A Renamed Document", http.StatusOK}, + } { + t.Run("update/"+tc.name, func(t *testing.T) { + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/documents/"+docID, map[string]interface{}{ + "title": tc.title, + }) + if rr.Code != tc.want { + t.Errorf("PATCH title=%.40q: got %d, want %d: %s", tc.title, rr.Code, tc.want, rr.Body.String()) + } + }) + } + + for _, tc := range []struct { + name string + title string + want int + }{ + {"over the length bound", strings.Repeat("b", models.MaxDocumentTitleRunes+1), http.StatusBadRequest}, + {"bug-2796 bracket repro", `B]] [[B`, http.StatusBadRequest}, + {"ordinary", "Another Document", http.StatusCreated}, + } { + t.Run("create/"+tc.name, func(t *testing.T) { + rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/documents", map[string]interface{}{ + "title": tc.title, + "doc_type": "notes", + "status": "active", + }) + if rr.Code != tc.want { + t.Errorf("POST title=%.40q: got %d, want %d: %s", tc.title, rr.Code, tc.want, rr.Body.String()) + } + }) + } +} + +// TestRenameCascadeTooLarge_IsPermanentShapedNotRetryable pins the response +// SHAPE, which is the part a client acts on. +// +// A cascade refusal and a cascade CONTENTION failure both abort a rename and +// both come back from the same store call, but they mean opposite things to a +// caller: contention is 503 + Retry-After ("someone got there first"), and +// this is permanent ("this rename cannot be performed as asked"). Answering +// this one from the retryable family would tell a client to retry forever. +func TestRenameCascadeTooLarge_IsPermanentShapedNotRetryable(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + + target := createDocForTest(t, srv, slug, "A", "the document being renamed") + + // Three linking documents, each retaining under the cap, together over it + // — the same total-not-per-document shape the store test pins, driven + // through real requests. Each body is 264,790 bytes, well inside the + // 2 MiB request cap, which is the point: the hostile input is cheap to + // deliver. + const newTitleLen = 255 + const linkers = 3 + perDoc := (store.MaxRenameCascadeRetainedBytes / linkers) + (store.MaxRenameCascadeRetainedBytes / (linkers * 4)) + occurrences := perDoc / (5 + 5 + (newTitleLen - 1)) + body := strings.Repeat("[[A]]", occurrences) + + for i := 0; i < linkers; i++ { + createDocForTest(t, srv, slug, "Linker"+string(rune('a'+i)), body) + } + + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/documents/"+target, map[string]interface{}{ + "title": strings.Repeat("T", newTitleLen), + }) + + if rr.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("got %d, want 413: %s", rr.Code, rr.Body.String()) + } + if got := rr.Header().Get("Retry-After"); got != "" { + t.Errorf("Retry-After = %q; a permanent refusal must not invite a retry", got) + } + respBody := rr.Body.String() + if !strings.Contains(respBody, "rename_cascade_too_large") { + t.Errorf("response lacks the error code a client would switch on: %s", respBody) + } + + // The actual CAP, not the word "maximum" — a message reading "maximum + // exceeded" would satisfy a word check and tell the caller nothing about + // how far over they are (codex round 4). + if !strings.Contains(respBody, fmt.Sprint(store.MaxRenameCascadeRetainedBytes)) { + t.Errorf("response does not state the limit %d: %s", store.MaxRenameCascadeRetainedBytes, respBody) + } + if !regexp.MustCompile(`at least \d+ bytes`).MatchString(respBody) { + t.Errorf("response does not state what the rename would hold: %s", respBody) + } + + // And NOT the store's internal wrapping. The handler composes this message + // from typed fields; splicing err.Error() published whatever prefix any + // layer had added on the way up (codex round 5). These two strings are the + // wrapping that was reaching clients before. + for _, leak := range []string{"update links:", "store:"} { + if strings.Contains(respBody, leak) { + t.Errorf("response carries internal error wrapping %q — the message must be composed "+ + "from typed fields, not spliced from err.Error(): %s", leak, respBody) + } + } +} + +// TestRenameCascadeTooLarge_IsNotMisreportedAsATitleCollision pins codex round +// 3's P2 at the handler. +// +// The UNIQUE-constraint arm identifies its error by SUBSTRING, and the refusal +// error carries the caller's own title verbatim. So a document renamed to a +// title containing the words "UNIQUE constraint" came back as a 409 title +// collision — telling the caller to pick a different name for a rename that +// was actually refused for size, and that would fail identically under any +// name. +// +// The fix is ordering: sentinels this handler knows by identity are tested +// before any error is classified by what its text happens to contain. The test +// is here rather than in the store because the defect is entirely in the +// handler's classification order. +func TestRenameCascadeTooLarge_IsNotMisreportedAsATitleCollision(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + + target := createDocForTest(t, srv, slug, "A", "the document being renamed") + + const linkers = 3 + perDoc := (store.MaxRenameCascadeRetainedBytes / linkers) + (store.MaxRenameCascadeRetainedBytes / (linkers * 4)) + // The title is padded to the bound and CONTAINS the substring the other + // arm matches on. + newTitle := "UNIQUE constraint " + strings.Repeat("t", models.MaxDocumentTitleRunes-len("UNIQUE constraint ")) + occurrences := perDoc / (5 + 5 + (len(newTitle) - 1)) + body := strings.Repeat("[[A]]", occurrences) + + for i := 0; i < linkers; i++ { + createDocForTest(t, srv, slug, "Linker"+string(rune('a'+i)), body) + } + + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/documents/"+target, map[string]interface{}{ + "title": newTitle, + }) + + if rr.Code == http.StatusConflict { + t.Fatalf("got 409 — the size refusal was classified by the title's own text as a name collision: %s", rr.Body.String()) + } + if rr.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("got %d, want 413: %s", rr.Code, rr.Body.String()) + } +} + +// TestLegacyTitleDocumentStaysEditable pins codex round 10, and it is a +// regression this fix introduced rather than one it inherited. +// +// "Enforced at write time, existing titles valid until their next rename" was +// the promise — Dave's ruling, repeated in the constant's doc comment and in +// two commit messages. Validating every supplied title broke it for the most +// ordinary shape of an edit there is: a client that PATCHes the whole object, +// title included, to change the CONTENT. Documents with a legacy title became +// uneditable, not merely un-renameable. +// +// The title used here cannot be created through the API any more, so it is +// written straight to the store — which is exactly the population the +// grandfathering clause is about. +func TestLegacyTitleDocumentStaysEditable(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + + ws, err := srv.store.GetWorkspaceBySlug(slug) + if err != nil { + t.Fatalf("resolve workspace: %v", err) + } + legacyTitle := strings.Repeat("L", models.MaxDocumentTitleRunes+50) + `|legacy\` + doc, err := srv.store.CreateDocument(ws.ID, models.DocumentCreate{ + Title: legacyTitle, Content: "before", DocType: "notes", Status: "active", + }) + if err != nil { + t.Fatalf("seed legacy document: %v", err) + } + + // Content-only edit that echoes the existing title back — the shape a + // full-object client sends. + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/documents/"+doc.ID, map[string]interface{}{ + "title": legacyTitle, + "content": "after", + }) + if rr.Code != http.StatusOK { + t.Fatalf("editing a legacy-titled document returned %d, want 200 — grandfathering means the "+ + "title stays valid until it CHANGES: %s", rr.Code, rr.Body.String()) + } + + // Content-only PATCH with no title at all must also work. + rr = doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/documents/"+doc.ID, map[string]interface{}{ + "content": "after again", + }) + if rr.Code != http.StatusOK { + t.Fatalf("content-only PATCH returned %d, want 200: %s", rr.Code, rr.Body.String()) + } + + // But RENAMING it — the write that actually cascades — is still refused. + // Without this leg, dropping validation entirely would pass the test. + rr = doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/documents/"+doc.ID, map[string]interface{}{ + "title": legacyTitle + "-renamed", + }) + if rr.Code != http.StatusBadRequest { + t.Errorf("renaming to another invalid title returned %d, want 400: %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/store/documents.go b/internal/store/documents.go index 732dd67f..d1036870 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -424,6 +424,20 @@ func (s *Store) UpdateDocument(id string, input models.DocumentUpdate) (*models. // the scan, so a NEW reference requires a title that literally contains a // `pad-attachment:` token. if input.Title != nil && *input.Title != existing.Title { + // Validated HERE, not only at the handler, because here is where the + // rename is decided — under the lock, against the title this + // transaction re-read (codex round 11). + // + // The handler's check compares against a document it read BEFORE the + // lock. That is fine for giving a caller a fast, friendly 400, but it + // is a time-of-check that a concurrent rename can invalidate: echo a + // legacy title back while another request renames the document, and + // the handler sees "unchanged, skip validation" while this branch sees + // a genuine rename and would write the legacy title through. Same + // grandfathering rule, applied where the decision actually happens. + if msg := models.ValidateDocumentTitle(*input.Title); msg != "" { + return nil, &InvalidDocumentTitleError{Reason: msg} + } err = s.updateLinksInTx(tx, existing.WorkspaceID, existing.Title, *input.Title) if err != nil { return nil, fmt.Errorf("update links: %w", err) @@ -564,13 +578,40 @@ func (s *Store) acquireWorkspaceDocumentRenameLock(tx *sql.Tx, workspaceID strin return nil } +// escapeLikePattern escapes the three characters that carry meaning inside a +// LIKE pattern, for use with an explicit `ESCAPE '\'` clause. +// +// Without this the cascade's own search term is interpreted as a pattern, and +// a document TITLE decides how (BUG-2798, codex round 1 P2 — plus the rest of +// the class it was an instance of): +// +// - `_` and `%` are wildcards in BOTH dialects, so a title containing them +// selects documents that do not link it. Those extra rows rewrite to +// themselves, so the damage is not corruption — it is that the guard below +// is computed from this result set, so an over-broad pattern spends a +// caller's budget on rows that were never going to change. +// - `\` is where the two dialects DISAGREE, which is the dangerous half. +// Postgres LIKE treats backslash as the default escape character; SQLite +// LIKE has no default escape character at all. So `[[Alpha\Beta]]` is +// searched for as the literal it is on SQLite and as `[[AlphaBeta]]` on +// Postgres — the linking documents are simply not found, the cascade +// rewrites nothing, and the rename succeeds leaving every link stale. A +// silent, dialect-dependent data defect. +// +// The explicit ESCAPE clause makes both dialects agree, rather than leaving +// SQLite correct by accident and Postgres wrong by default. +func escapeLikePattern(s string) string { + r := strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`) + return r.Replace(s) +} + func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle string) error { // Find all documents in the workspace that contain [[oldTitle]] searchTerm := "[[" + oldTitle + "]]" rows, err := tx.Query(s.q(` SELECT id, content FROM documents - WHERE workspace_id = ? AND deleted_at IS NULL AND content LIKE ? - `), workspaceID, "%"+searchTerm+"%") + WHERE workspace_id = ? AND deleted_at IS NULL AND content LIKE ? ESCAPE '\' + `), workspaceID, "%"+escapeLikePattern(searchTerm)+"%") if err != nil { return err } @@ -583,13 +624,69 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri // the column handed us and never a normalized form of it. read string rewritten string + // retained is what this row contributed to the running total, kept so + // the compare-and-set below can be given ITS share of the budget when + // a concurrent edit forces it to re-read and re-rewrite. + retained int64 } var updates []docUpdate + var retained int64 for rows.Next() { var du docUpdate if err := rows.Scan(&du.id, &du.read); err != nil { return err } + + // Project what this linker will make the cascade HOLD, before building + // it, and refuse on the running TOTAL across the linking set + // (BUG-2798). + // + // The total is the right thing to bound, and a per-document cap would + // not be. Measured: with the title bound in place, one linker holding + // the largest body a 2 MiB request can carry projects 108,632,370 + // bytes of output — 51.8x — and the loop below holds EVERY rewritten + // body in `updates` before it writes any of them, so k linkers hold k + // times that (measured linear at k = 1/2/4). A per-document cap of C + // still admits k * C, which is the same unbounded shape one level up. + // + // RETAINED bytes, not output bytes. An earlier version of this guard + // summed only the projected output, which bounds nothing when the new + // title is SHORTER than the old one: renaming a 255-character title to + // a one-character title makes each 2 MiB linker project about 40 KiB + // while the cascade still retains its 2 MiB read for the + // compare-and-set, so hundreds of linkers exhaust memory while the + // counter reports well under the cap (codex round 1 P1). Both strings + // are alive at once, so both are counted. + // + // Refusing here rather than after the loop is what makes the bound + // real: at the moment of refusal the process holds the linkers already + // counted (under the cap by construction) plus this one row's body, + // and none of the amplified output. + // The SELECT is a LIKE, and LIKE is not the rewriter. On SQLite it is + // ASCII case-INSENSITIVE by default (Postgres's is not), so renaming + // `Alpha` scans every body containing `[[alpha]]` — which + // links.ReplaceTitle, being case-sensitive, will not touch. Charging + // those bodies to the budget lets case-variant content that can never + // be rewritten push a legitimate rename over the cap, on one dialect + // only (codex round 7). + // + // Skipping them is the fix for both halves: no budget is spent, and no + // no-op UPDATE is issued for a row whose content the cascade was never + // going to change. Same class as the `%`/`_` over-matching the ESCAPE + // clause closed — the pattern selects a superset of the linkers, and + // the authority on what is actually a linker is the rewriter's own + // case-sensitive count. + occurrences := int64(strings.Count(du.read, searchTerm)) + if occurrences == 0 { + continue + } + + du.retained = cascadeRetainedBytes(du.read, occurrences, oldTitle, newTitle) + retained += du.retained + if retained > MaxRenameCascadeRetainedBytes { + return newRenameCascadeTooLargeError(newTitle, retained) + } + du.rewritten = links.ReplaceTitle(du.read, oldTitle, newTitle) updates = append(updates, du) } @@ -608,13 +705,96 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri } for _, du := range updates { - if err := s.rewriteLinkerCAS(tx, du.id, du.read, du.rewritten, oldTitle, newTitle); err != nil { + // The compare-and-set is handed the scan's TOTAL, not a pre-computed + // budget, so it can both bound and REPORT correctly: a retry must fit + // in what remains under the cap, and a refusal must name the whole + // operation's size rather than the re-read alone (codex rounds 3, 8). + // + // Everything the scan counted is still held — `updates` references the + // original read and rewritten bodies for every linker, this one + // included — so a re-read is allocated ON TOP of them. An earlier + // version credited this document's share back, on the reasoning that + // the retry replaces it; it does not, and the bound could be exceeded + // by up to one document's share while the arithmetic reported it + // satisfied. + if err := s.rewriteLinkerCAS(tx, du.id, du.read, du.rewritten, oldTitle, newTitle, searchTerm, retained); err != nil { return err } } return nil } +// cascadeRetainedBytes is what one linking document makes the cascade hold: +// the body it read (kept verbatim as the compare-and-set token) plus the body +// it will write. +// +// Exact rather than an estimate. strings.Replace substitutes every +// non-overlapping occurrence, so the rewritten length is +// len(read) + occurrences * (len(new) - len(old)) to the byte, and this +// function is the only place that arithmetic lives — the scan and the retry +// path must not be allowed to drift apart on it. +func cascadeRetainedBytes(read string, occurrences int64, oldTitle, newTitle string) int64 { + if occurrences == 0 { + // No second string exists to charge for: strings.Replace returns its + // input unchanged when there is nothing to replace, so ReplaceTitle + // allocates nothing and `rewritten` aliases `read`. + // + // This is not a micro-optimisation, it is a correctness case on the + // retry path (codex round 3 P2): a concurrent edit that REMOVES the + // link leaves a body with no occurrences, and charging it twice could + // refuse an otherwise valid rename for memory the cascade never + // allocates. + return int64(len(read)) + } + rewritten := int64(len(read)) + occurrences*int64(len(newTitle)-len(oldTitle)) + return int64(len(read)) + rewritten +} + +// RenameCascadeTooLargeError carries the refusal's NUMBERS as typed fields, so +// a caller-facing layer can compose its own sentence instead of splicing this +// error's text into a response. +// +// The distinction matters (codex round 5): the HTTP handler used to append +// err.Error() verbatim, which meant every wrapper any caller added on the way +// up — "update links: " today, anything at all tomorrow — was published to the +// client as part of a public message. The two figures ARE meant to reach the +// caller (Dave's day-63 ruling: the refusal states what it would hold and what +// the cap is, so "split the rename" is actionable advice rather than a shrug); +// the internal call path is not. +// +// This is the round-3 lesson applied in the other direction: there, prose was +// being used to CLASSIFY an error and should have been identity; here, prose +// was being used to REPORT one and should have been data. +type RenameCascadeTooLargeError struct { + // NewTitle is the caller's own requested title. Echoed back deliberately: + // it is theirs, and it is what they need to see to understand which + // rename was refused. + NewTitle string + // Retained is the lower bound on bytes the cascade would have held. A + // lower bound, not a total: the scan stops at the first row that crosses + // the cap, so the true figure is larger. + Retained int64 + // Max is the cap in force. + Max int64 +} + +func (e *RenameCascadeTooLargeError) Error() string { + return fmt.Sprintf("%s: renaming to %q would hold at least %d bytes of linked-document content, maximum %d", + ErrRenameCascadeTooLarge.Error(), e.NewTitle, e.Retained, e.Max) +} + +// Unwrap makes errors.Is(err, ErrRenameCascadeTooLarge) hold, so every existing +// sentinel check keeps working. +func (e *RenameCascadeTooLargeError) Unwrap() error { return ErrRenameCascadeTooLarge } + +func newRenameCascadeTooLargeError(newTitle string, retained int64) error { + return &RenameCascadeTooLargeError{ + NewTitle: newTitle, + Retained: retained, + Max: MaxRenameCascadeRetainedBytes, + } +} + // ErrLinkCascadeContention reports that a rename's link cascade lost its // compare-and-set on the same linking document too many times in a row. // @@ -625,6 +805,98 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri // opposite of the truth here (codex round 2 on BUG-2785). var ErrLinkCascadeContention = errors.New("store: link cascade lost the compare-and-set") +// ErrInvalidDocumentTitle reports a rename to a title the wiki-link machinery +// cannot carry. See InvalidDocumentTitleError for why the store enforces this +// rather than trusting its callers to have done so. +var ErrInvalidDocumentTitle = errors.New("store: invalid document title") + +// InvalidDocumentTitleError carries the human-readable reason a title was +// refused, so the HTTP layer can return it without re-deriving the rule or +// splicing an internal error's text into a response. +type InvalidDocumentTitleError struct{ Reason string } + +func (e *InvalidDocumentTitleError) Error() string { + return ErrInvalidDocumentTitle.Error() + ": " + e.Reason +} + +func (e *InvalidDocumentTitleError) Unwrap() error { return ErrInvalidDocumentTitle } + +// ErrRenameCascadeTooLarge reports that a rename was refused because the +// linked-document content it would hold exceeds MaxRenameCascadeRetainedBytes. +// +// Deliberately NOT in ErrLinkCascadeContention's family, and the distinction +// is the caller-visible one: contention means "someone else got there first, +// try again"; this means "this rename cannot be performed as asked, and +// retrying it unchanged will fail identically until the workspace's content +// changes." One wants 503 + Retry-After, the other a permanent 4xx carrying +// the projection so the caller can see what it asked for. Blurring the two +// vocabularies would tell a client to retry forever (BUG-2798, lead ruling +// day-63). +var ErrRenameCascadeTooLarge = errors.New("store: rename cascade exceeds the retained-content bound") + +// MaxRenameCascadeRetainedBytes bounds the TOTAL linked-document content a +// single rename may hold in memory: for every linking document, the body read +// plus the body written. +// +// RETAINED rather than merely projected-output, because output alone is not +// the resource. A rename to a SHORTER title projects less output than its +// input while still holding every read body for the compare-and-set — so an +// output-only counter reports ~40 KiB per 2 MiB linker and bounds nothing in +// that direction (codex round 1). Counting both strings makes the cap a +// statement about resident memory, which is what actually runs out. +// +// 32 MiB, and both bounds of the gap are measured rather than picked: +// +// - Legitimate ceiling. In this development instance's database — a mature +// workspace set, 206 MB on disk — the ENTIRE corpus of wiki-linking +// content is 2,949 items totalling 10,077,476 bytes (largest single body +// 86,147 bytes). A cascade over all of it would retain read + rewritten, +// so ~20,154,952 bytes. That is the absolute ceiling on any conceivable +// single cascade over that corpus: it assumes every wiki-linking document +// links the one title being renamed, which no real workspace does. 32 MiB +// is ~1.6x that impossible worst case. +// +// The INFERENCE is worth naming rather than hiding, because the guard it +// justifies is on documents and the measurement is not (codex round 6): +// that instance's `documents` table is EMPTY, so there is no direct +// figure to take. `items` is used as the proxy on the grounds that the +// two hold the same kind of prose and cascade the same way — a reasonable +// assumption, not a measurement of the guarded path. What can be said +// without the proxy is narrower and still useful: a workspace whose +// documents linking one title total more than ~16 MB of content will meet +// this cap. If real document corpora ever get that large, this number is +// the thing to re-measure. +// +// - Hostile floor. A single linking document holding the largest body a +// 2 MiB request can carry retains 110,729,520 bytes once the title bound +// is in place — 3.3x this cap — so the attack is refused at k = 1 and +// every k above it, rather than at some threshold count of documents. +// +// The gap is deliberate and wide: a cap has to be far enough above real use +// that nobody meets it by accident, and far enough below the hazard that +// meeting it costs nothing. +// +// The consequence a cap necessarily has, stated because it is user-visible +// and was chosen rather than overlooked: once a workspace's documents linking +// one title exceed this, that title can no longer be renamed, and any EDITOR +// can put it in that state by creating enough linking content. That is a +// denial of one operation by a trusted role — an editor can already delete +// every document in the workspace — and it replaces the previous behaviour, +// where the same input took the server down for everybody. Trading an +// unbounded OOM for a bounded, legible refusal is the whole point of the +// guard, not a gap in it. +// +// Bounding the WORKSPACE's linking content, so the state cannot be reached at +// all, is a quota question rather than a cascade question and is filed +// separately. +// +// What it does NOT cover, stated so the next reader does not over-read it: +// this bounds ONE rename's linked-document content, not concurrent renames (N +// of them may each hold up to this), and not the base cost of a workspace +// whose linking documents are legitimately large — a cascade under the cap +// still allocates whatever it holds. +const MaxRenameCascadeRetainedBytes = 32 << 20 + // cascadeRewriteAttempts bounds rewriteLinkerCAS's retry loop. // // Three, matching debounceMergeAttempts' reasoning rather than copying its @@ -717,9 +989,11 @@ var cascadeRewriteAttempts = 3 // Both are pre-existing and neither is made worse here. They are recorded // because the next reader's question is "is the cascade correct now", and the // honest answer is "for the direction this bug named". -func (s *Store) rewriteLinkerCAS(tx *sql.Tx, id, read, rewritten, oldTitle, newTitle string) error { +func (s *Store) rewriteLinkerCAS(tx *sql.Tx, id, read, rewritten, oldTitle, newTitle, searchTerm string, scanTotal int64) error { expected := read next := rewritten + // Total charged by retries so far — see the accumulation below. + var retriesSpent int64 for attempt := 0; attempt < cascadeRewriteAttempts; attempt++ { res, err := tx.Exec(s.q(` UPDATE documents SET content = ? @@ -749,6 +1023,35 @@ func (s *Store) rewriteLinkerCAS(tx *sql.Tx, id, read, rewritten, oldTitle, newT return err } + // The re-read body is a NEW input, supplied by whoever won the race, + // and it is bounded by nothing this cascade has already checked. Its + // budget is the cap less what the other linkers are holding, so the + // aggregate bound holds across retries too — without this, an editor + // could grow a linker between the scan and the retry and walk the + // rename straight back into the amplification it was refused for + // (BUG-2798, codex round 1 P1). + grownOccurrences := int64(strings.Count(current, searchTerm)) + // ACCUMULATED across attempts, not just this one. Each retry's + // buffers become unreachable when `expected`/`next` are reassigned + // below, but unreachable is not the same as reclaimed — the runtime + // may not have collected them yet, so a run of failures can hold + // several copies at once (codex round 9). Counting every attempt is + // the conservative reading, and it errs toward refusing, which is the + // safe direction for a memory bound. The loop is capped at + // cascadeRewriteAttempts, so this cannot accumulate without end. + retriesSpent += cascadeRetainedBytes(current, grownOccurrences, oldTitle, newTitle) + grown := retriesSpent + if scanTotal+grown > MaxRenameCascadeRetainedBytes { + // Report the AGGREGATE, not this body alone. The bodies the scan + // counted are still held, so the operation's real size is their + // total plus the re-read — and reporting only `grown` produced a + // refusal that contradicted itself, telling the caller it would + // hold 16 MiB against a 32 MiB limit (codex round 8). A refusal + // whose own numbers do not justify it reads as a bug in the + // server, which is the opposite of what an actionable error does. + return newRenameCascadeTooLargeError(newTitle, scanTotal+grown) + } + // A concurrent edit won. Rewrite ITS body rather than ours: replaying // the original rewrite would reintroduce the very content this bug is // about losing. If that edit already removed the link, ReplaceTitle is diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go new file mode 100644 index 00000000..24a97bb2 --- /dev/null +++ b/internal/store/documents_rename_bounds_test.go @@ -0,0 +1,766 @@ +package store + +import ( + "errors" + "fmt" + "regexp" + "runtime" + "strings" + "sync" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-2798. A document rename rewrites `[[oldTitle]]` → `[[newTitle]]` in +// every linking document, and the cascade holds every rewritten body in memory +// before it writes any of them. Neither the title length nor the number of +// linking documents was bounded, so one rename could hold more content than the +// process could carry — measured at 20,000x amplification on the filing, and +// still 51.8x per document after the title bound alone. +// +// The title bound (models.MaxDocumentTitleRunes) is the cheap door. This file +// covers the wall: the cascade refuses when the TOTAL it would retain across +// the linking set — every read body plus every written body — exceeds +// MaxRenameCascadeRetainedBytes. + +// linkerBody returns a body containing exactly n `[[A]]` occurrences, and the +// number of bytes the cascade would RETAIN for it when renaming "A" to a title +// of length newLen: the body it reads plus the body it writes. +// +// Deliberately computed here rather than by calling cascadeRetainedBytes — a +// test that reuses the implementation's arithmetic cannot catch that +// arithmetic being wrong. +func linkerBody(n, newLen int) (string, int) { + body := strings.Repeat("[[A]]", n) + rewritten := len(body) + n*(newLen-1) + return body, len(body) + rewritten +} + +// TestRenameCascade_RefusesOnProjectedTOTAL_NotPerDocument is the load-bearing +// test, and its shape is the finding it encodes: a per-document cap would not +// close this bug. +// +// Every linking document here retains comfortably UNDER the cap on its own. +// Only the total is over. A guard that tested each document in isolation would +// admit all three, allocate the sum, and pass a test that merely asserted "a +// huge single document is refused" — which is why this test asserts the +// per-document figure is under the cap as a PRECONDITION rather than trusting +// the constants to stay where they are. +func TestRenameCascade_RefusesOnProjectedTOTAL_NotPerDocument(t *testing.T) { + const newTitleLen = 255 // the title bound; the worst title that survives it + const linkers = 3 + + // Size each linker so that linkers-1 of them fit under the cap and all of + // them do not. Derived from the cap rather than hardcoded, so the test + // keeps discriminating if the cap moves. + perDocTarget := (MaxRenameCascadeRetainedBytes / linkers) + (MaxRenameCascadeRetainedBytes / (linkers * 4)) + occurrences := perDocTarget / (5 + 5 + (newTitleLen - 1)) + body, perDocRetained := linkerBody(occurrences, newTitleLen) + + // Preconditions — these are what make the test discriminate. If either + // fails the test is no longer testing what its name says. + if perDocRetained >= MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: per-document retention %d must be UNDER the cap %d, "+ + "otherwise a per-document guard would also pass this test", + perDocRetained, MaxRenameCascadeRetainedBytes) + } + if total := perDocRetained * linkers; total <= MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: total retention %d must EXCEED the cap %d", + total, MaxRenameCascadeRetainedBytes) + } + + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeTotalBound") + target := createTestDoc(t, s, ws.ID, "A", "the document being renamed") + + linkerIDs := make([]string, 0, linkers) + for i := 0; i < linkers; i++ { + d := createTestDoc(t, s, ws.ID, "Linker"+string(rune('a'+i)), body) + linkerIDs = append(linkerIDs, d.ID) + } + + newTitle := strings.Repeat("T", newTitleLen) + _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}) + + // 1. Refused, and refused as THIS error. Without the guard this returns + // nil, having allocated the sum. + if !errors.Is(err, ErrRenameCascadeTooLarge) { + t.Fatalf("rename: got %v, want ErrRenameCascadeTooLarge", err) + } + + // 2. Refused with the projection in the message. The only actionable + // information for a caller is what the rename would hold against what + // is allowed; an error that says "too large" and nothing else sends + // them guessing. + // The ACTUAL numbers, not the words around them. Checking only for + // "maximum" and "bytes" would pass a message that says "too large, maximum + // bytes exceeded" and tells the caller nothing (codex round 4). + msg := err.Error() + if !strings.Contains(msg, fmt.Sprint(MaxRenameCascadeRetainedBytes)) { + t.Errorf("error message does not state the cap %d: %q", MaxRenameCascadeRetainedBytes, msg) + } + if !regexp.MustCompile(`hold at least (\d+) bytes`).MatchString(msg) { + t.Errorf("error message does not state what the rename would hold: %q", msg) + } + + // 3. The rename ROLLED BACK. A guard that refused after writing some of + // the linkers would pass assertion 1 and leave the workspace with a + // half-cascaded rename — the exact inconsistency the cascade's + // all-or-nothing transaction exists to prevent. + after, err := s.GetDocument(target.ID) + if err != nil { + t.Fatalf("read back renamed document: %v", err) + } + if after.Title != "A" { + t.Errorf("title = %q after a refused rename, want it unchanged at %q", after.Title, "A") + } + for i, id := range linkerIDs { + got, err := s.GetDocument(id) + if err != nil { + t.Fatalf("read back linker %d: %v", i, err) + } + if got.Content != body { + t.Errorf("linker %d content changed by a refused rename (len %d, want %d)", + i, len(got.Content), len(body)) + } + } +} + +// TestRenameCascade_AllowsAnOrdinaryRename is the control leg. Without it, a +// guard that refused every rename — or a cap accidentally set near zero — +// passes the test above while breaking the feature outright. +// +// The shape is deliberately the same as the refusal case with one fewer +// linker, so the only difference between green and red is the total. +func TestRenameCascade_AllowsAnOrdinaryRename(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeOrdinary") + target := createTestDoc(t, s, ws.ID, "A", "the document being renamed") + linker := createTestDoc(t, s, ws.ID, "Linker", "before [[A]] middle [[A]] after") + + newTitle := "A Perfectly Ordinary Renamed Document" + if _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}); err != nil { + t.Fatalf("ordinary rename refused: %v", err) + } + + got, err := s.GetDocument(linker.ID) + if err != nil { + t.Fatalf("read back linker: %v", err) + } + want := "before [[" + newTitle + "]] middle [[" + newTitle + "]] after" + if got.Content != want { + t.Errorf("cascade did not rewrite the links:\n got: %q\nwant: %q", got.Content, want) + } +} + +// TestRenameCascade_RefusesTheSingleDocumentAttack covers the k=1 case +// directly: a single linking document is refused on its own, so the guard does +// not depend on reaching some threshold COUNT of documents. +// +// The body here is sized from the cap (2x it, 1,271,000 bytes retaining +// 67,108,800) rather than from the request limit, so the test keeps +// discriminating if the cap moves. The full-strength version of this attack — +// the largest body a 2 MiB request can carry, retaining 110,729,520 bytes, +// 3.3x the cap — is exercised by +// TestRenameCascade_RefusesBeforeBuildingTheRewrittenBody, which needs that +// exact shape for its allocation margin. Kept separate rather than described +// here, because a comment naming a shape the test does not build is how +// figures rot (codex round 6). +// +// Kept separate from the total-versus-per-document test because it is the one +// case a per-document guard WOULD catch — asserting both makes it explicit +// that the total guard is a superset, not a replacement of unclear scope. +func TestRenameCascade_RefusesTheSingleDocumentAttack(t *testing.T) { + const newTitleLen = 255 + occurrences := (MaxRenameCascadeRetainedBytes / (5 + 5 + (newTitleLen - 1))) * 2 // 2x the cap + body, retained := linkerBody(occurrences, newTitleLen) + if retained <= MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: single-document retention %d must exceed the cap %d", retained, MaxRenameCascadeRetainedBytes) + } + + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeSingleDoc") + target := createTestDoc(t, s, ws.ID, "A", "the document being renamed") + createTestDoc(t, s, ws.ID, "Linker", body) + + newTitle := strings.Repeat("T", newTitleLen) + _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}) + if !errors.Is(err, ErrRenameCascadeTooLarge) { + t.Fatalf("rename: got %v, want ErrRenameCascadeTooLarge", err) + } +} + +// TestRenameCascade_CountsRetainedBytesNotJustOutput pins codex round 1's P1 +// against the guard that shipped in the first commit, which summed only the +// PROJECTED OUTPUT. +// +// The counterexample is a rename to a SHORTER title. Every linker here holds a +// large body, but the rewrite shrinks it, so an output-only counter reports a +// small number while the cascade still retains every read body for its +// compare-and-set. Under the old guard the total below reported far under the +// cap and the rename proceeded; the retained-bytes guard refuses it. +// +// This is why the direction of the rename matters and why the constant is +// named for retention rather than for projection. +func TestRenameCascade_CountsRetainedBytesNotJustOutput(t *testing.T) { + // Old title long, new title short — the shrinking direction. + oldTitle := strings.Repeat("O", 200) + newTitle := "n" + + // Each linker is ~2 MiB of `[[<200-char title>]]`, the shape a single 2 MiB + // request can deliver. + occurrencesPerDoc := (2 << 20) / (len(oldTitle) + 4) + body := strings.Repeat("[["+oldTitle+"]]", occurrencesPerDoc) + + // Enough linkers that the RETAINED total is over the cap while the + // projected OUTPUT total stays under it. That gap is the finding. + linkers := (MaxRenameCascadeRetainedBytes / len(body)) + 2 + + outputTotal := 0 + retainedTotal := 0 + for i := 0; i < linkers; i++ { + rewritten := len(body) + occurrencesPerDoc*(len(newTitle)-len(oldTitle)) + outputTotal += rewritten + retainedTotal += len(body) + rewritten + } + if outputTotal > MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: projected OUTPUT total %d must stay UNDER the cap %d, "+ + "otherwise an output-only guard would also refuse this and the test proves nothing", + outputTotal, MaxRenameCascadeRetainedBytes) + } + if retainedTotal <= MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: RETAINED total %d must exceed the cap %d", retainedTotal, MaxRenameCascadeRetainedBytes) + } + + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeShrinking") + target := createTestDoc(t, s, ws.ID, oldTitle, "the document being renamed") + for i := 0; i < linkers; i++ { + createTestDoc(t, s, ws.ID, "Linker"+string(rune('a'+i)), body) + } + + if _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}); !errors.Is(err, ErrRenameCascadeTooLarge) { + t.Fatalf("shrinking rename: got %v, want ErrRenameCascadeTooLarge — an output-only guard admits this", err) + } +} + +// TestRenameCascade_FindsLinkersWhoseTitleContainsABackslash pins codex round +// 1's P2. POSTGRES ONLY, and skipped loudly elsewhere rather than passing +// quietly — the defect is a DIALECT DIVERGENCE, and SQLite is the dialect that +// was accidentally right. +// +// The cascade finds linkers with `content LIKE ?`. Postgres LIKE treats +// backslash as the default escape character; SQLite LIKE has no default escape +// character at all. So an unescaped search term for a title containing `\` was +// searched for as the literal it is on SQLite, and as a DIFFERENT literal on +// Postgres — `[[Alpha\Beta]]` became `[[AlphaBeta]]`, the linking documents +// were not found, the cascade rewrote nothing, and the rename reported success +// leaving every link pointing at a title that no longer exists. +// +// A green run on SQLite is therefore a property of the DSN, not evidence about +// this fix, which is why this skips instead. +func TestRenameCascade_FindsLinkersWhoseTitleContainsABackslash(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + t.Skip("asserts a Postgres LIKE-escape property; SQLite LIKE has no default escape character, so the unescaped pattern is accidentally correct there") + } + + ws := createTestWorkspace(t, s, "CascadeLikeBackslash") + title := `Alpha\Beta` + target := createTestDoc(t, s, ws.ID, title, "the document being renamed") + linker := createTestDoc(t, s, ws.ID, "RealLinker", "see [["+title+"]] here") + + newTitle := "Renamed" + if _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}); err != nil { + t.Fatalf("rename: %v", err) + } + + got, err := s.GetDocument(linker.ID) + if err != nil { + t.Fatalf("read back linker: %v", err) + } + if want := "see [[Renamed]] here"; got.Content != want { + t.Errorf("the linker was not rewritten — the cascade's LIKE pattern did not find it:\n got: %q\nwant: %q", + got.Content, want) + } +} + +// TestRenameCascade_DoesNotSpendTheBudgetOnDocumentsThatDoNotLinkTheTitle is +// the rest of the class codex's backslash finding was an instance of +// (CONVE-18): `%` and `_` are LIKE wildcards in BOTH dialects, so an unescaped +// search term for a title containing them selects documents that do not link +// it. +// +// Over-matching cannot be caught by asserting content — the extra rows rewrite +// to themselves, because ReplaceTitle looks for the literal. It is observable +// through the guard, which is computed from this result set: unrelated +// documents inflate the retained total, and a rename that fits the cap is +// refused because of content it was never going to touch. That is the harm, +// and it is what this asserts. +// +// The first version of this test asserted the decoy's content was untouched +// and passed against the unescaped pattern — a vacuous green. Recorded here +// because the fix was to find the observable consequence, not to trust the +// mechanism. +func TestRenameCascade_DoesNotSpendTheBudgetOnDocumentsThatDoNotLinkTheTitle(t *testing.T) { + for _, tc := range []struct { + name string + title string + decoy string // matches the title read as a PATTERN, not as a literal + }{ + {"percent", `Alpha%Beta`, `[[AlphaXYZBeta]]`}, + {"underscore", `Alpha_Beta`, `[[AlphaZBeta]]`}, + } { + t.Run(tc.name, func(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeLike"+tc.name) + target := createTestDoc(t, s, ws.ID, tc.title, "the document being renamed") + linker := createTestDoc(t, s, ws.ID, "RealLinker", "see [["+tc.title+"]] here") + + // Decoys big enough that INCLUDING them blows the cap, while the + // real linker alone is negligible. With the pattern escaped they + // are not selected and the rename is comfortably under budget. + decoyBody := strings.Repeat("x", 1<<20) + " " + tc.decoy + decoys := (MaxRenameCascadeRetainedBytes / len(decoyBody)) + 2 + for i := 0; i < decoys; i++ { + createTestDoc(t, s, ws.ID, "Decoy"+string(rune('a'+i)), decoyBody) + } + + newTitle := "Renamed" + if _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}); err != nil { + t.Fatalf("a rename well under the cap was refused because of documents that do not link it: %v", err) + } + + got, err := s.GetDocument(linker.ID) + if err != nil { + t.Fatalf("read back linker: %v", err) + } + if want := "see [[Renamed]] here"; got.Content != want { + t.Errorf("the real linker was not rewritten:\n got: %q\nwant: %q", got.Content, want) + } + }) + } +} + +// TestRenameCascade_RetryRecheckesTheBudgetAgainstTheGrownBody pins codex round +// 1's other P1: the guard used to cover only the cascade's SCAN, while the +// compare-and-set's retry path re-read a linker's body and called ReplaceTitle +// on it with no bound at all. +// +// The re-read body is a NEW input supplied by whoever won the race, so a +// content edit landing inside the cascade's window could grow a linker from +// harmless to enormous and walk the rename straight back into the +// amplification it would have been refused for. +// +// POSTGRES ONLY, for the same structural reason as +// TestUpdateDocument_CascadeDoesNotOverwriteConcurrentEdit: SQLite's +// `_txlock=immediate` DSN takes the write lock at BEGIN and holds it across the +// cascade's whole read→write window, so a concurrent edit cannot commit inside +// it. A green run there would be a property of the DSN, not evidence about this +// guard. +func TestRenameCascade_RetryRecheckesTheBudgetAgainstTheGrownBody(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + t.Skip("needs a concurrent edit to commit inside the cascade's read→write window; SQLite's BEGIN IMMEDIATE closes it structurally") + } + + ws := createTestWorkspace(t, s, "CascadeRetryBudget") + target := createTestDoc(t, s, ws.ID, "A", "the document being renamed") + + // Small at scan time — the cascade counts a few bytes and proceeds. + linker := createTestDoc(t, s, ws.ID, "Linker", "before [[A]] after") + + // The winner's body is over the cap on its own, so the retry's re-read is + // the first and only place this can be caught. + newTitleLen := 255 + occurrences := (MaxRenameCascadeRetainedBytes / (5 + 5 + (newTitleLen - 1))) * 2 + grownBody, grownRetained := linkerBody(occurrences, newTitleLen) + if grownRetained <= MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: the grown body's retention %d must exceed the cap %d", grownRetained, MaxRenameCascadeRetainedBytes) + } + + var once sync.Once + var editErr error + s.afterLinkCascadeRead = func(string) { + once.Do(func() { + // Content-only, so it takes no rename lock and can commit inside + // the cascade's window (BUG-2785's seam, same mechanism). + _, editErr = s.UpdateDocument(linker.ID, models.DocumentUpdate{Content: &grownBody}) + }) + } + defer func() { s.afterLinkCascadeRead = nil }() + + newTitle := strings.Repeat("T", newTitleLen) + _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}) + + if editErr != nil { + t.Fatalf("the concurrent edit itself failed, so this run never exercised the retry path: %v", editErr) + } + if !errors.Is(err, ErrRenameCascadeTooLarge) { + t.Fatalf("rename: got %v, want ErrRenameCascadeTooLarge — the retry re-read an unbounded body", err) + } +} + +// TestRenameCascade_RetryBudgetExcludesThisDocumentsOwnStrings pins codex round +// 3's P1, and is deliberately separate from the test above: that one catches +// the retry check being ABSENT, this one catches it being too GENEROUS. +// +// The first version credited this document's own contribution back into its +// retry budget, on the reasoning that the retry replaces it. It does not — the +// original read and rewritten bodies stay reachable through `updates` while +// the write loop runs, so the re-read and its rewrite are allocated ON TOP of +// them. The bound could then be exceeded by up to one document's share while +// the arithmetic reported it satisfied. +// +// The grown body here is sized to fall BETWEEN the two budgets: under the +// credited-back budget (which would admit it) and over the true headroom +// (which refuses). A test using a body far over the cap cannot tell the two +// apart, because both refuse it. +// +// POSTGRES ONLY, same structural reason as the test above. +func TestRenameCascade_RetryBudgetExcludesThisDocumentsOwnStrings(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + t.Skip("needs a concurrent edit to commit inside the cascade's read→write window; SQLite's BEGIN IMMEDIATE closes it structurally") + } + + ws := createTestWorkspace(t, s, "CascadeRetryBudgetTight") + target := createTestDoc(t, s, ws.ID, "A", "the document being renamed") + + const newTitleLen = 255 + perOccurrence := 5 + 5 + (newTitleLen - 1) // retained bytes per `[[A]]` + + // The single linker holds ~60% of the cap at scan time. + scanOccurrences := (MaxRenameCascadeRetainedBytes * 6 / 10) / perOccurrence + scanBody, scanRetained := linkerBody(scanOccurrences, newTitleLen) + + // The winner's body retains ~50% of the cap: comfortably under the cap on + // its own, and under the OLD budget (which was the whole cap here, since + // this is the only linker), but over the true headroom of cap - scanned. + grownOccurrences := (MaxRenameCascadeRetainedBytes * 5 / 10) / perOccurrence + grownBody, grownRetained := linkerBody(grownOccurrences, newTitleLen) + + // The credited-back budget was `cap - (retained - this document's share)`. + // With a single linker those two terms are the same number, so it reduced + // to the whole cap — which is exactly how a document could be handed a + // budget that ignored what it was already holding. + oldBudget := int64(MaxRenameCascadeRetainedBytes) + newBudget := int64(MaxRenameCascadeRetainedBytes) - int64(scanRetained) + if int64(grownRetained) > oldBudget { + t.Fatalf("precondition: grown retention %d must fit the credited-back budget %d, or this test "+ + "cannot tell a too-generous budget from an absent one", grownRetained, oldBudget) + } + if int64(grownRetained) <= newBudget { + t.Fatalf("precondition: grown retention %d must exceed the true headroom %d", grownRetained, newBudget) + } + + linker := createTestDoc(t, s, ws.ID, "Linker", scanBody) + + var once sync.Once + var editErr error + s.afterLinkCascadeRead = func(string) { + once.Do(func() { + _, editErr = s.UpdateDocument(linker.ID, models.DocumentUpdate{Content: &grownBody}) + }) + } + defer func() { s.afterLinkCascadeRead = nil }() + + newTitle := strings.Repeat("T", newTitleLen) + _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}) + + if editErr != nil { + t.Fatalf("the concurrent edit itself failed, so this run never exercised the retry path: %v", editErr) + } + if !errors.Is(err, ErrRenameCascadeTooLarge) { + t.Fatalf("rename: got %v, want ErrRenameCascadeTooLarge — the retry budget credited back "+ + "strings the cascade is still holding", err) + } + + // The refusal must JUSTIFY ITSELF: the figure it reports has to exceed the + // cap it cites. Reporting only the re-read body produced a refusal saying + // it would hold less than the limit, which reads as a server bug rather + // than as actionable advice (codex round 8). + var tooLarge *RenameCascadeTooLargeError + if !errors.As(err, &tooLarge) { + t.Fatalf("error does not carry the typed figures: %v", err) + } + if tooLarge.Retained <= tooLarge.Max { + t.Errorf("refusal reports %d bytes against a cap of %d — the reported figure does not "+ + "justify the refusal, so it names the re-read alone rather than the whole operation", + tooLarge.Retained, tooLarge.Max) + } +} + +// TestRenameCascade_ConcurrentEditThatRemovesTheLinkDoesNotRefuseTheRename pins +// codex round 3's P2. +// +// When a concurrent edit removes the link entirely, ReplaceTitle has nothing to +// replace — strings.Replace returns its input unchanged, allocating nothing. +// Charging that body twice (read + a rewritten copy that does not exist) could +// push a legitimate rename over the cap and refuse it for memory the cascade +// never allocates. +// +// The assertion is that the rename SUCCEEDS. POSTGRES ONLY, same reason as its +// neighbours. +func TestRenameCascade_ConcurrentEditThatRemovesTheLinkDoesNotRefuseTheRename(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + t.Skip("needs a concurrent edit to commit inside the cascade's read→write window; SQLite's BEGIN IMMEDIATE closes it structurally") + } + + ws := createTestWorkspace(t, s, "CascadeLinkRemoved") + target := createTestDoc(t, s, ws.ID, "A", "the document being renamed") + + const newTitleLen = 255 + perOccurrence := 5 + 5 + (newTitleLen - 1) + + // Sized so that double-charging the link-free body would exceed the cap + // while charging it once does not — otherwise the test passes either way. + scanOccurrences := (MaxRenameCascadeRetainedBytes * 4 / 10) / perOccurrence + scanBody, scanRetained := linkerBody(scanOccurrences, newTitleLen) + + // The winner's body has NO link left. Sized against the retry's real + // HEADROOM (the cap less what the scan is still holding), not against the + // cap: the first version of this test compared to the cap, and the body it + // chose was legitimately over the headroom, so the refusal it caught was + // correct behaviour rather than the double charge. Charged once it must + // fit; charged twice it must not. + headroom := int64(MaxRenameCascadeRetainedBytes) - int64(scanRetained) + grownBody := strings.Repeat("y", int(headroom*7/10)) + if int64(len(grownBody)) > headroom { + t.Fatalf("precondition: the link-free body %d must fit the retry headroom %d when charged once", + len(grownBody), headroom) + } + if int64(2*len(grownBody)) <= headroom { + t.Fatalf("precondition: the link-free body %d must EXCEED the headroom %d when charged twice, "+ + "or this test cannot detect the double charge", 2*len(grownBody), headroom) + } + + linker := createTestDoc(t, s, ws.ID, "Linker", scanBody) + + var once sync.Once + var editErr error + s.afterLinkCascadeRead = func(string) { + once.Do(func() { + _, editErr = s.UpdateDocument(linker.ID, models.DocumentUpdate{Content: &grownBody}) + }) + } + defer func() { s.afterLinkCascadeRead = nil }() + + newTitle := strings.Repeat("T", newTitleLen) + _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}) + + if editErr != nil { + t.Fatalf("the concurrent edit itself failed, so this run never exercised the retry path: %v", editErr) + } + if err != nil { + t.Fatalf("rename refused after a concurrent edit REMOVED the link: %v — "+ + "the guard charged for a rewritten copy that ReplaceTitle never allocates", err) + } +} + +// TestRenameCascade_CountsBothStringsNotTheLargerOne closes codex round 4's P1: +// every other retained-byte case here exceeds the cap under `max(read, +// rewritten)` as well as under `read + rewritten`, so none of them can tell the +// two arithmetics apart. +// +// This one can. An ordinary same-length rename over content totalling ~60% of +// the cap holds ~120% of it (both strings alive), and ~60% under the max +// reading — so summing refuses and taking the larger admits. +// +// That the refusal is CORRECT here is the point, not a side effect: the +// cascade really does hold both copies, and a workspace with that much linking +// content really is at the bound. The cap's doc comment says so explicitly. +func TestRenameCascade_CountsBothStringsNotTheLargerOne(t *testing.T) { + oldTitle := "Alpha" + newTitle := "Bravo" // same length: the rewrite neither grows nor shrinks + + const linkers = 3 + // ~60% of the cap in total READ bytes, split across the linkers. + perDoc := (MaxRenameCascadeRetainedBytes * 6 / 10) / linkers + body := strings.Repeat("[["+oldTitle+"]]", perDoc/(len(oldTitle)+4)) + + readTotal := int64(len(body)) * linkers + sumTotal := readTotal * 2 // read + an equal-length rewritten copy + if readTotal > MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: under max(read,rewritten) the total %d must stay UNDER the cap %d, "+ + "or this test cannot tell summing from taking the larger", readTotal, MaxRenameCascadeRetainedBytes) + } + if sumTotal <= MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: the summed total %d must EXCEED the cap %d", sumTotal, MaxRenameCascadeRetainedBytes) + } + + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeSumNotMax") + target := createTestDoc(t, s, ws.ID, oldTitle, "the document being renamed") + for i := 0; i < linkers; i++ { + createTestDoc(t, s, ws.ID, "Linker"+string(rune('a'+i)), body) + } + + if _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}); !errors.Is(err, ErrRenameCascadeTooLarge) { + t.Fatalf("rename: got %v, want ErrRenameCascadeTooLarge — a guard that charged only the "+ + "larger of the two strings admits this and then holds twice the cap", err) + } +} + +// TestRenameCascade_RefusesBeforeBuildingTheRewrittenBody closes codex round +// 4's P2: every other oversize test asserts only THAT the rename is refused, +// so moving links.ReplaceTitle above the guard would keep them all green while +// destroying the point of the guard — the refusal exists so the amplified +// string is never built. +// +// The instrument is cumulative allocation (TotalAlloc), not peak memory, and +// both sides of the threshold are measured rather than estimated. Refusing +// allocates 2,114,624 bytes — the one body it had to scan, plus overhead — +// against a ceiling of 52,428,800, so the passing case sits ~24.8x under it. +// Building the rewrite first allocates 110,748,144, which is ~2.1x OVER the +// ceiling; that figure comes from running this test against a mutation with +// links.ReplaceTitle moved above the guard, not from arithmetic. +// +// The two margins are deliberately asymmetric and the smaller one is the +// binding constraint: 2.1x is the headroom that matters, since it is the gap a +// regression has to cross to be caught. +// +// BUG-2798's own filing warned that "a test that tries to prove the ABSENCE of +// amplification by measuring memory would be flaky by construction", and that +// still holds for the thing it described — asserting a peak-RSS floor to show +// nothing blew up. This is the opposite shape: a generous ceiling on a +// deterministic counter, with the two outcomes twenty times apart, whose +// failure names exactly what went wrong. +func TestRenameCascade_RefusesBeforeBuildingTheRewrittenBody(t *testing.T) { + const newTitleLen = 255 + // The single-document attack: one linker holding the largest body a 2 MiB + // request can carry. + occurrences := (2 << 20) / len("[[A]]") + body := strings.Repeat("[[A]]", occurrences) + rewrittenSize := int64(len(body)) + int64(occurrences)*int64(newTitleLen-1) + + // Well above what refusing costs, well below what building the rewrite + // costs. Stated as a ratio so the numbers stay honest if sizes move. + const allocCeiling = 50 << 20 + if rewrittenSize < 2*allocCeiling { + t.Fatalf("precondition: the rewritten body %d must be far above the ceiling %d, "+ + "or this test cannot distinguish refusing from building", rewrittenSize, allocCeiling) + } + + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeNoBuild") + target := createTestDoc(t, s, ws.ID, "A", "the document being renamed") + createTestDoc(t, s, ws.ID, "Linker", body) + + newTitle := strings.Repeat("T", newTitleLen) + + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}) + runtime.ReadMemStats(&after) + + if !errors.Is(err, ErrRenameCascadeTooLarge) { + t.Fatalf("rename: got %v, want ErrRenameCascadeTooLarge", err) + } + allocated := after.TotalAlloc - before.TotalAlloc + t.Logf("refused rename allocated %d bytes; ceiling %d; the rewrite it declined to build is %d", + allocated, allocCeiling, rewrittenSize) + if allocated > allocCeiling { + t.Errorf("the refused rename allocated %d bytes, ceiling %d — the projected rewrite is "+ + "%d bytes, so the guard is running AFTER the body it was supposed to prevent", + allocated, allocCeiling, rewrittenSize) + } +} + +// TestRenameCascade_DoesNotChargeCaseVariantsTheRewriterWillNotTouch closes +// codex round 7, and is the third instance of one class: the LIKE that finds +// candidates is not the rewriter that changes them, so it selects a SUPERSET +// and the difference gets charged to the caller's budget. +// +// The first two instances were `%` and `_` (wildcards on both dialects, closed +// by the ESCAPE clause). This is the case half, and it is DIALECT-SPLIT the +// other way from the backslash bug: SQLite's LIKE is ASCII case-insensitive by +// default and Postgres's is case-sensitive, so renaming `Alpha` scans every +// body containing `[[alpha]]` on SQLite only — bodies links.ReplaceTitle will +// never touch, because it is case-sensitive on both. +// +// Runs on BOTH dialects deliberately, unlike its backslash sibling: on +// Postgres it asserts the behaviour was already correct, which is what makes +// it a regression test rather than a SQLite quirk shim. +func TestRenameCascade_DoesNotChargeCaseVariantsTheRewriterWillNotTouch(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "CascadeCaseVariants") + + target := createTestDoc(t, s, ws.ID, "Alpha", "the document being renamed") + linker := createTestDoc(t, s, ws.ID, "RealLinker", "see [[Alpha]] here") + + // Case-variant bodies, big enough that charging them blows the cap. The + // rewriter cannot change them; only the SELECT thinks they are relevant. + decoyBody := strings.Repeat("z", 1<<20) + " [[alpha]]" + decoys := (MaxRenameCascadeRetainedBytes / len(decoyBody)) + 2 + decoyIDs := make([]string, 0, decoys) + for i := 0; i < decoys; i++ { + d := createTestDoc(t, s, ws.ID, "Decoy"+string(rune('a'+i)), decoyBody) + decoyIDs = append(decoyIDs, d.ID) + } + + newTitle := "Renamed" + if _, err := s.UpdateDocument(target.ID, models.DocumentUpdate{Title: &newTitle}); err != nil { + t.Fatalf("a rename well under the cap was refused because of case-variant content the "+ + "rewriter cannot touch: %v", err) + } + + got, err := s.GetDocument(linker.ID) + if err != nil { + t.Fatalf("read back linker: %v", err) + } + if want := "see [[Renamed]] here"; got.Content != want { + t.Errorf("the real linker was not rewritten:\n got: %q\nwant: %q", got.Content, want) + } + + // And the case variants are still exactly as they were — the cascade is + // case-sensitive, so `[[alpha]]` is a different link, not a missed one. + for i, id := range decoyIDs { + gotDecoy, err := s.GetDocument(id) + if err != nil { + t.Fatalf("read back decoy %d: %v", i, err) + } + if gotDecoy.Content != decoyBody { + t.Errorf("decoy %d was rewritten; the cascade must not treat a case variant as a link", i) + } + } +} + +// TestUpdateDocument_RefusesAnInvalidRenameEvenWhenTheHandlerDidNot pins codex +// round 11: the store validates a rename under the lock, so the guarantee does +// not depend on a caller having checked first. +// +// The handler's own check compares against a document read BEFORE the rename +// lock, which is a time-of-check a concurrent rename can invalidate: echo a +// legacy title back while another request renames the document, and the +// handler sees "unchanged, skip validation" while the store sees a genuine +// rename. Calling the store directly is the honest way to pin its half — +// reproducing the race would test the scheduler, not the guarantee. +// +// The grandfathering leg is the control: without it, validating every title +// here would pass while making legacy documents uneditable, which is the +// regression round 10 fixed. +func TestUpdateDocument_RefusesAnInvalidRenameEvenWhenTheHandlerDidNot(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "StoreSideTitleGuard") + + legacy := strings.Repeat("L", models.MaxDocumentTitleRunes+50) + doc := createTestDoc(t, s, ws.ID, legacy, "body") + + // A rename to an invalid title, reaching the store with no handler in + // front of it. + bad := legacy + "-renamed" + if _, err := s.UpdateDocument(doc.ID, models.DocumentUpdate{Title: &bad}); !errors.Is(err, ErrInvalidDocumentTitle) { + t.Fatalf("rename to an invalid title: got %v, want ErrInvalidDocumentTitle", err) + } + + // Control: the SAME legacy title, unchanged, alongside a content edit. + // Grandfathered, so this must succeed. + content := "edited" + if _, err := s.UpdateDocument(doc.ID, models.DocumentUpdate{Title: &legacy, Content: &content}); err != nil { + t.Fatalf("content edit echoing the unchanged legacy title was refused: %v", err) + } +}