From e5131a95839ed4c25e1b15fdc400ab81ba53221f Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 17:23:26 +0000 Subject: [PATCH 01/14] fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) A document rename rewrites [[oldTitle]] into every linking document. Neither factor of the output size was bounded: titles had no length validation, and the cascade holds every rewritten body in memory before writing any of them. One rename could project 10 GB from a 500 KB input -- 20,000x, measured -- and OOM while holding the workspace rename lock. Two walls, per Dave's day-63 ruling. 1. Title length, bounded at write time (models.MaxDocumentTitleRunes = 255). Runes, not bytes: "255 characters" is what a user and a UI counter mean. Existing over-limit titles stay valid until their next rename -- no retro-breakage of stored data. 2. The cascade's projected TOTAL, bounded at 16 MiB (store.MaxRenameCascadeProjectedBytes), accumulated across the linking set and refused before the first rewrite is built. The total is the right quantity and a per-document cap would not have been. Measured, with the title bound already in place: one linker holding the largest body a 2 MiB request can carry projects 108,632,370 bytes -- 51.8x -- and the aggregate is linear in the number of linkers (108.6 / 217.3 / 434.5 MB at k = 1/2/4, allocation tracking output at ~1.02x). A per-document cap of C still admits k * C, which is the same unbounded shape one level up. The 16 MiB figure has a receipt in the constant's doc comment: it sits above the absolute ceiling of any cascade this development instance could produce (its entire wiki-linking corpus is 10,077,476 bytes) and 6.5x below the single-document attack. The refusal is permanent-shaped and deliberately NOT in ErrLinkCascadeContention's family: 413 with the projection in the message and no Retry-After. Contention means "someone got there first, try again"; this means "this rename cannot be performed as asked". Answering it from the retryable family would tell a client to retry forever. BUG-2796 folds in at the same validation point, as ruled -- a title containing wiki-link syntax is emitted raw by links.ReplaceTitle, so renaming to `A]] [[A` produced two broken links and reported success. The rule is derived from the two mechanisms that consume a stored bracket (the grammar at markdown.ts:327 and the unescaper at markdown.ts:753) rather than from a character blacklist: the first version of this fix banned `]`, `\` and `|` because all three "look like wiki-link syntax", and the round-trip test refuted two thirds of that. `|` in particular is a title shape resolveWikiBody contains a dedicated branch to support, and `[` passes the grammar untouched. Doors enumerated rather than assumed (CONVE-24): store.CreateDocument and UpdateDocument have exactly two callers between them, both HTTP handlers. No CLI, import, or seed path writes a document title. Update previously validated doc_type and status and NOT title -- the one field that drives the cascade -- so the handler tests drive real requests through both doors (CONVE-19). BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/models/document.go | 107 ++++++++++- internal/models/document_title_test.go | 121 +++++++++++++ internal/server/handlers_documents.go | 29 ++- .../server/handlers_documents_title_test.go | 136 ++++++++++++++ internal/store/documents.go | 72 ++++++++ .../store/documents_rename_bounds_test.go | 170 ++++++++++++++++++ 6 files changed, 632 insertions(+), 3 deletions(-) create mode 100644 internal/models/document_title_test.go create mode 100644 internal/server/handlers_documents_title_test.go create mode 100644 internal/store/documents_rename_bounds_test.go diff --git a/internal/models/document.go b/internal/models/document.go index c54621529..66e50b8cb 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,106 @@ 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 projection guard (store.MaxRenameCascadeProjectedBytes) 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. A title containing +// `\\` or `\|` is emitted raw, unescaped on the way back in, and the +// result no longer equals the title — so the link resolves to nothing, +// or to a different document. +// +// 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. BUG-2796's filing proposed rejecting "`[[` or `]]`"; +// measured against the grammar, the `[[` half of that is overreach. +// - `|` — resolveWikiBody tries a FULL-BODY title match before the pipe +// split, a branch whose comment says it exists precisely to handle +// "stored legacy titles that contain a literal `|`". Banning it would +// refuse what that branch was written to support. +// - a lone `\` not followed by `\`, `]` or `|` — passes the grammar as an +// escape pair and survives the unescaper unchanged. +// +// 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.Contains(title, `\\`) || strings.Contains(title, `\|`) { + return `Title may not contain "\\" or "\|" — those are escape sequences in [[wiki-link]] syntax, so the ` + + `links that point at this document would resolve to a different title` + } + 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 000000000..7d26ee9e6 --- /dev/null +++ b/internal/models/document_title_test.go @@ -0,0 +1,121 @@ +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. If the TypeScript changes, these should +// start disagreeing with ValidateDocumentTitle — that is the signal wanted, +// not a nuisance. +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 rule, in executable form: the validator must reject a title if and +// only if that title's links would not survive being rewritten to it. +// +// 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`}, + {"literal pipe", `Alpha|Beta`}, + {"lone backslash", `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) + } + }) + } +} + +// 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) + } + } +} diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 6fdd8ce10..0b30f971e 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,16 @@ 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. + if input.Title != nil { + 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 @@ -165,6 +175,21 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { // 500 would be actively misleading — it says the request will never // work. Distinguished by sentinel rather than message text, because // this one is ours to name (codex round 2). + // A projected-output refusal is PERMANENT-shaped, and must not join + // the retryable family below it. Retrying this rename unchanged fails + // identically until the workspace's content changes, so it gets a 4xx + // with no Retry-After — and it carries the projection, because the + // only actionable information is what was projected against what is + // allowed. 413 follows this codebase's own precedent for a bound on + // output rather than on the request body (`image_too_large` in + // handlers_attachments_transform.go, where the request is likewise + // small and the thing refused is what it would produce). + if errors.Is(err, store.ErrRenameCascadeTooLarge) { + writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large", + "This rename would rewrite more linked content than the server will process in one operation. "+ + "Reduce the number of documents linking this title, or shorten the new title, and try again. ("+err.Error()+")") + return + } if isRetryableLockError(err) || errors.Is(err, store.ErrLinkCascadeContention) { w.Header().Set("Retry-After", "1") // Deliberately does NOT name the holder. The previous wording said diff --git a/internal/server/handlers_documents_title_test.go b/internal/server/handlers_documents_title_test.go new file mode 100644 index 000000000..6a001c575 --- /dev/null +++ b/internal/server/handlers_documents_title_test.go @@ -0,0 +1,136 @@ +package server + +import ( + "net/http" + "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. + {"at the length bound", strings.Repeat("a", models.MaxDocumentTitleRunes), http.StatusOK}, + {"contains a literal pipe", `Alpha|Beta`, http.StatusOK}, + {"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 projecting under the cap, together over + // it — the same total-not-per-document shape the store test pins, driven + // through real requests. Each body is ~135 KB, 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.MaxRenameCascadeProjectedBytes / linkers) + (store.MaxRenameCascadeProjectedBytes / (linkers * 4)) + occurrences := perDoc / (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) + } + if b := rr.Body.String(); !strings.Contains(b, "rename_cascade_too_large") { + t.Errorf("response lacks the error code a client would switch on: %s", b) + } + if b := rr.Body.String(); !strings.Contains(b, "maximum") { + t.Errorf("response lacks the projection; the caller cannot tell how far over they are: %s", b) + } +} diff --git a/internal/store/documents.go b/internal/store/documents.go index 732dd67fd..1d2c6801c 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -585,11 +585,39 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri rewritten string } var updates []docUpdate + var projected int64 for rows.Next() { var du docUpdate if err := rows.Scan(&du.id, &du.read); err != nil { return err } + + // Project this linker's rewritten size BEFORE building it, and refuse + // on the running TOTAL across the linking set (BUG-2798). + // + // The quantity is exact rather than an estimate: strings.Replace + // substitutes every non-overlapping occurrence, so the output is + // len(read) + occurrences * (len(new) - len(old)) to the byte. + // + // 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 — 51.8x — and the cascade 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. + // + // Refusing here rather than after the loop is what makes the bound + // real: at the moment of refusal the process holds the linkers already + // projected (under the cap by construction) plus this one row's body, + // and none of the amplified output. + occurrences := int64(strings.Count(du.read, searchTerm)) + projected += int64(len(du.read)) + occurrences*int64(len(newTitle)-len(oldTitle)) + if projected > MaxRenameCascadeProjectedBytes { + return fmt.Errorf("%w: renaming to %q projects at least %d bytes across linked documents, maximum %d", + ErrRenameCascadeTooLarge, newTitle, projected, MaxRenameCascadeProjectedBytes) + } + du.rewritten = links.ReplaceTitle(du.read, oldTitle, newTitle) updates = append(updates, du) } @@ -625,6 +653,50 @@ 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") +// ErrRenameCascadeTooLarge reports that a rename was refused because the work +// it projects across linking documents exceeds +// MaxRenameCascadeProjectedBytes. +// +// 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 projected-output bound") + +// MaxRenameCascadeProjectedBytes bounds the TOTAL bytes a single rename may +// project across every document linking the renamed title. +// +// 16 MiB, and the basis is 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). That is the absolute ceiling on any conceivable single +// cascade there: it assumes every wiki-linking document links the one +// title being renamed, which no real workspace does. 16 MiB is above +// that impossible worst case, so the guard cannot fire on honest use. +// (Measured on `items`, the live surface; the `documents` table in that +// instance is empty, which is why the proxy — the two carry the same kind +// of content through the same kind of cascade.) +// - Hostile floor. A single linking document holding the largest body a +// 2 MiB request can carry projects 108,632,370 bytes once the title bound +// is in place — 6.5x this cap — so the attack is refused at k = 1 and +// every k above it, rather than at some threshold count of documents. +// - Cost of the bound itself. The cascade holds read and rewritten bodies +// concurrently, so the cap is a promise about resident memory: at most +// ~2x this per in-flight rename, which is a bounded, budgetable number +// for a server that previously had none. +// +// The gap between the two figures 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. 16 MiB is ~1.6x the former +// and ~0.15x the latter. +const MaxRenameCascadeProjectedBytes = 16 << 20 + // cascadeRewriteAttempts bounds rewriteLinkerCAS's retry loop. // // Three, matching debounceMergeAttempts' reasoning rather than copying its diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go new file mode 100644 index 000000000..9fe55e5e7 --- /dev/null +++ b/internal/store/documents_rename_bounds_test.go @@ -0,0 +1,170 @@ +package store + +import ( + "errors" + "strings" + "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 project more output than +// the process could hold — measured at 20,000x 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 its projected TOTAL exceeds +// MaxRenameCascadeProjectedBytes. + +// linkerBody returns a body containing exactly n `[[A]]` occurrences, and the +// number of bytes renaming "A" to a title of length newLen would project for +// it: len(content) + occurrences * (len(new) - len(old)). +// +// Exact, not an estimate — strings.Replace substitutes every non-overlapping +// occurrence, so this is the output size to the byte. +func linkerBody(n, newLen int) (string, int) { + body := strings.Repeat("[[A]]", n) + return body, len(body) + n*(newLen-1) +} + +// 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 projects 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 := (MaxRenameCascadeProjectedBytes / linkers) + (MaxRenameCascadeProjectedBytes / (linkers * 4)) + occurrences := perDocTarget / (5 + (newTitleLen - 1)) + body, perDocProjected := 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 perDocProjected >= MaxRenameCascadeProjectedBytes { + t.Fatalf("precondition: per-document projection %d must be UNDER the cap %d, "+ + "otherwise a per-document guard would also pass this test", + perDocProjected, MaxRenameCascadeProjectedBytes) + } + if total := perDocProjected * linkers; total <= MaxRenameCascadeProjectedBytes { + t.Fatalf("precondition: total projection %d must EXCEED the cap %d", + total, MaxRenameCascadeProjectedBytes) + } + + 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 was projected against what is + // allowed; an error that says "too large" and nothing else sends them + // guessing. + if msg := err.Error(); !strings.Contains(msg, "maximum") || !strings.Contains(msg, "bytes") { + t.Errorf("error message lacks the projection: %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: one linking document holding the largest body a 2 MiB request can +// carry still projects 108,632,370 bytes once the title bound is in place +// (measured), which is 6.5x the cap. The attack is refused at every k, not +// only at a threshold count of documents. +// +// 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 := (MaxRenameCascadeProjectedBytes / (5 + (newTitleLen - 1))) * 2 // 2x the cap + body, projected := linkerBody(occurrences, newTitleLen) + if projected <= MaxRenameCascadeProjectedBytes { + t.Fatalf("precondition: single-document projection %d must exceed the cap %d", projected, MaxRenameCascadeProjectedBytes) + } + + 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) + } +} From b09aca12ea2b12d4fb1c97af572a2f4394d94984 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 18:16:06 +0000 Subject: [PATCH 02/14] fix(documents): count retained bytes, bound the retry path, escape the cascade's LIKE pattern (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 1 on #1218. Three findings, all real, all fixed here. 1. The guard bounded 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 ~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. The counter now sums RETAINED bytes — read plus written, both alive at once — so the cap is a statement about resident memory rather than about output. MaxRenameCascadeProjectedBytes becomes MaxRenameCascadeRetainedBytes and moves 16 -> 32 MiB, because the legitimate ceiling it clears doubles under the new metric (that instance's whole wiki-linking corpus retains ~20,154,952 bytes); the single-document attack retains 110,729,522, so it is still refused by 3.3x. 2. The compare-and-set's retry path bypassed the guard entirely. On contention it re-reads the linker and calls ReplaceTitle on whatever the winner wrote — a NEW input, bounded by nothing the scan had checked — so a content edit landing inside the cascade's window could grow a linker from harmless to enormous and walk the rename back into the amplification it would have been refused for. Each document's compare-and-set now carries the cap less what the other linkers hold, and re-checks the grown body against it. 3. The cascade's `content LIKE ?` search term went in unescaped, so a document TITLE decided how the pattern was read. `\` is the default LIKE escape character on Postgres and NOT on SQLite, so `[[Alpha\Beta]]` was searched for as itself on one dialect and as `[[AlphaBeta]]` on the other: linkers not found, cascade rewrites nothing, rename reports success, every link left stale. Silent and dialect-dependent. Codex named the backslash; `%` and `_` are the rest of the class (CONVE-18) — wildcards on both dialects, so a title carrying them selects documents that do not link it. An explicit `ESCAPE '\'` clause plus escapeLikePattern makes both dialects agree, rather than leaving SQLite correct by accident. Finding 3 also constrains finding 3 of the ORIGINAL fix: models' validator allows a lone backslash in a title on the grounds that both renderers handle it, which was true of rendering and false of cascading. That comment now records the dependency — allowing it is only correct while the cascade's pattern stays escaped. Tests, four new, each mutation-verified against the code it guards: - CountsRetainedBytesNotJustOutput — the shrinking rename. Asserts as a PRECONDITION that the projected-output total stays under the cap, so the test cannot pass for the old reason. - RetryRecheckesTheBudgetAgainstTheGrownBody — drives the real race through the afterLinkCascadeRead seam. POSTGRES ONLY and skipped loudly elsewhere: SQLite's BEGIN IMMEDIATE closes the window structurally, so a green run there would be a property of the DSN. - FindsLinkersWhoseTitleContainsABackslash — Postgres only, same reasoning inverted: SQLite is the dialect that was accidentally right. - DoesNotSpendTheBudgetOnDocumentsThatDoNotLinkTheTitle — `%` and `_`. Its first version asserted the decoy's content was untouched and passed against the unescaped pattern, because over-matched rows rewrite to themselves. The observable harm is that they spend the caller's budget, so that is what it now asserts. Mutation matrix for this round: output-only counter -> only the shrinking test fails; retry check removed -> only the retry test fails (PG); LIKE unescaped -> the budget legs fail on SQLite and the backslash test fails on PG. Gates: `go test ./...` under Postgres 17 EXIT=0; SQLite packages EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/models/document.go | 7 +- .../server/handlers_documents_title_test.go | 4 +- internal/store/documents.go | 173 ++++++++--- .../store/documents_rename_bounds_test.go | 275 ++++++++++++++++-- 4 files changed, 381 insertions(+), 78 deletions(-) diff --git a/internal/models/document.go b/internal/models/document.go index 66e50b8cb..048b21743 100644 --- a/internal/models/document.go +++ b/internal/models/document.go @@ -138,7 +138,12 @@ const MaxDocumentTitleRunes = 255 // "stored legacy titles that contain a literal `|`". Banning it would // refuse what that branch was written to support. // - a lone `\` not followed by `\`, `]` or `|` — passes the grammar as an -// escape pair and survives the unescaper unchanged. +// escape pair and survives the unescaper unchanged. Note this one depends +// on store.escapeLikePattern: the rename cascade finds linking documents +// with `content LIKE`, where Postgres reads `\` as an escape character, so +// before that escaping landed a backslash title rendered fine and then +// silently failed to cascade on one dialect. Allowing it here is only +// correct while the cascade's pattern stays escaped. // // Boundary, stated rather than papered over: this is derived from the SHARED // stored-syntax path in markdown.ts. The legacy documents surface has no diff --git a/internal/server/handlers_documents_title_test.go b/internal/server/handlers_documents_title_test.go index 6a001c575..1060bf8b3 100644 --- a/internal/server/handlers_documents_title_test.go +++ b/internal/server/handlers_documents_title_test.go @@ -109,8 +109,8 @@ func TestRenameCascadeTooLarge_IsPermanentShapedNotRetryable(t *testing.T) { // request cap, which is the point: the hostile input is cheap to deliver. const newTitleLen = 255 const linkers = 3 - perDoc := (store.MaxRenameCascadeProjectedBytes / linkers) + (store.MaxRenameCascadeProjectedBytes / (linkers * 4)) - occurrences := perDoc / (5 + (newTitleLen - 1)) + 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++ { diff --git a/internal/store/documents.go b/internal/store/documents.go index 1d2c6801c..d14e4e7b9 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -564,13 +564,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,39 +610,48 @@ 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 projected int64 + var retained int64 for rows.Next() { var du docUpdate if err := rows.Scan(&du.id, &du.read); err != nil { return err } - // Project this linker's rewritten size BEFORE building it, and refuse - // on the running TOTAL across the linking set (BUG-2798). - // - // The quantity is exact rather than an estimate: strings.Replace - // substitutes every non-overlapping occurrence, so the output is - // len(read) + occurrences * (len(new) - len(old)) to the byte. + // 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 — 51.8x — and the cascade 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. + // 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 - // projected (under the cap by construction) plus this one row's body, + // counted (under the cap by construction) plus this one row's body, // and none of the amplified output. - occurrences := int64(strings.Count(du.read, searchTerm)) - projected += int64(len(du.read)) + occurrences*int64(len(newTitle)-len(oldTitle)) - if projected > MaxRenameCascadeProjectedBytes { - return fmt.Errorf("%w: renaming to %q projects at least %d bytes across linked documents, maximum %d", - ErrRenameCascadeTooLarge, newTitle, projected, MaxRenameCascadeProjectedBytes) + du.retained = cascadeRetainedBytes(du.read, searchTerm, oldTitle, newTitle) + retained += du.retained + if retained > MaxRenameCascadeRetainedBytes { + return newRenameCascadeTooLargeError(newTitle, retained) } du.rewritten = links.ReplaceTitle(du.read, oldTitle, newTitle) @@ -636,13 +672,40 @@ 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 { + // Budget handed to this document's compare-and-set: the cap, less + // everything the OTHER documents are holding. A retry that re-reads a + // body grown by a concurrent edit is checked against it, so the + // aggregate bound survives the retry path as well as the scan (codex + // round 1 P1 — the guard used to cover only the scan, and the retry + // called ReplaceTitle on an unbounded re-read). + budget := MaxRenameCascadeRetainedBytes - (retained - du.retained) + if err := s.rewriteLinkerCAS(tx, du.id, du.read, du.rewritten, oldTitle, newTitle, searchTerm, budget); 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, searchTerm, oldTitle, newTitle string) int64 { + occurrences := int64(strings.Count(read, searchTerm)) + rewritten := int64(len(read)) + occurrences*int64(len(newTitle)-len(oldTitle)) + return int64(len(read)) + rewritten +} + +func newRenameCascadeTooLargeError(newTitle string, retained int64) error { + return fmt.Errorf("%w: renaming to %q would hold at least %d bytes of linked-document content, maximum %d", + ErrRenameCascadeTooLarge, newTitle, retained, 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. // @@ -653,9 +716,8 @@ 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") -// ErrRenameCascadeTooLarge reports that a rename was refused because the work -// it projects across linking documents exceeds -// MaxRenameCascadeProjectedBytes. +// 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, @@ -665,37 +727,47 @@ var ErrLinkCascadeContention = errors.New("store: link cascade lost the compare- // 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 projected-output bound") +var ErrRenameCascadeTooLarge = errors.New("store: rename cascade exceeds the retained-content bound") -// MaxRenameCascadeProjectedBytes bounds the TOTAL bytes a single rename may -// project across every document linking the renamed title. +// 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. // -// 16 MiB, and the basis is measured rather than picked: +// 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). That is the absolute ceiling on any conceivable single -// cascade there: it assumes every wiki-linking document links the one -// title being renamed, which no real workspace does. 16 MiB is above +// 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 there: 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, so the guard cannot fire on honest use. -// (Measured on `items`, the live surface; the `documents` table in that -// instance is empty, which is why the proxy — the two carry the same kind -// of content through the same kind of cascade.) +// (Measured on `items`, the live surface; that instance's `documents` +// table is empty, which is why the proxy — the two carry the same kind of +// content through the same kind of cascade.) // - Hostile floor. A single linking document holding the largest body a -// 2 MiB request can carry projects 108,632,370 bytes once the title bound -// is in place — 6.5x this cap — so the attack is refused at k = 1 and +// 2 MiB request can carry retains 110,729,522 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. -// - Cost of the bound itself. The cascade holds read and rewritten bodies -// concurrently, so the cap is a promise about resident memory: at most -// ~2x this per in-flight rename, which is a bounded, budgetable number -// for a server that previously had none. // -// The gap between the two figures 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. 16 MiB is ~1.6x the former -// and ~0.15x the latter. -const MaxRenameCascadeProjectedBytes = 16 << 20 +// 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. +// +// 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. // @@ -789,7 +861,7 @@ 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, budget int64) error { expected := read next := rewritten for attempt := 0; attempt < cascadeRewriteAttempts; attempt++ { @@ -821,6 +893,17 @@ 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). + if grown := cascadeRetainedBytes(current, searchTerm, oldTitle, newTitle); grown > budget { + return newRenameCascadeTooLargeError(newTitle, 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 index 9fe55e5e7..0a83100a0 100644 --- a/internal/store/documents_rename_bounds_test.go +++ b/internal/store/documents_rename_bounds_test.go @@ -3,6 +3,7 @@ package store import ( "errors" "strings" + "sync" "testing" "github.com/PerpetualSoftware/pad/internal/models" @@ -11,30 +12,33 @@ import ( // 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 project more output than -// the process could hold — measured at 20,000x on the filing, and still 51.8x -// per document after the title bound alone. +// 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 its projected TOTAL exceeds -// MaxRenameCascadeProjectedBytes. +// 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 renaming "A" to a title of length newLen would project for -// it: len(content) + occurrences * (len(new) - len(old)). +// 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. // -// Exact, not an estimate — strings.Replace substitutes every non-overlapping -// occurrence, so this is the output size to the byte. +// 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) - return body, len(body) + n*(newLen-1) + 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 projects comfortably UNDER the cap on its own. +// 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 @@ -47,20 +51,20 @@ func TestRenameCascade_RefusesOnProjectedTOTAL_NotPerDocument(t *testing.T) { // 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 := (MaxRenameCascadeProjectedBytes / linkers) + (MaxRenameCascadeProjectedBytes / (linkers * 4)) - occurrences := perDocTarget / (5 + (newTitleLen - 1)) - body, perDocProjected := linkerBody(occurrences, newTitleLen) + 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 perDocProjected >= MaxRenameCascadeProjectedBytes { - t.Fatalf("precondition: per-document projection %d must be UNDER the cap %d, "+ + 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", - perDocProjected, MaxRenameCascadeProjectedBytes) + perDocRetained, MaxRenameCascadeRetainedBytes) } - if total := perDocProjected * linkers; total <= MaxRenameCascadeProjectedBytes { - t.Fatalf("precondition: total projection %d must EXCEED the cap %d", - total, MaxRenameCascadeProjectedBytes) + if total := perDocRetained * linkers; total <= MaxRenameCascadeRetainedBytes { + t.Fatalf("precondition: total retention %d must EXCEED the cap %d", + total, MaxRenameCascadeRetainedBytes) } s := testStore(t) @@ -83,9 +87,9 @@ func TestRenameCascade_RefusesOnProjectedTOTAL_NotPerDocument(t *testing.T) { } // 2. Refused with the projection in the message. The only actionable - // information for a caller is what was projected against what is - // allowed; an error that says "too large" and nothing else sends them - // guessing. + // 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. if msg := err.Error(); !strings.Contains(msg, "maximum") || !strings.Contains(msg, "bytes") { t.Errorf("error message lacks the projection: %q", msg) } @@ -142,19 +146,19 @@ func TestRenameCascade_AllowsAnOrdinaryRename(t *testing.T) { // TestRenameCascade_RefusesTheSingleDocumentAttack covers the k=1 case // directly: one linking document holding the largest body a 2 MiB request can -// carry still projects 108,632,370 bytes once the title bound is in place -// (measured), which is 6.5x the cap. The attack is refused at every k, not -// only at a threshold count of documents. +// carry still retains 110,729,522 bytes once the title bound is in place +// (108,632,370 written plus the 2,097,152 read), which is 3.3x the cap. The +// attack is refused at every k, not only at a threshold count of documents. // // 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 := (MaxRenameCascadeProjectedBytes / (5 + (newTitleLen - 1))) * 2 // 2x the cap - body, projected := linkerBody(occurrences, newTitleLen) - if projected <= MaxRenameCascadeProjectedBytes { - t.Fatalf("precondition: single-document projection %d must exceed the cap %d", projected, MaxRenameCascadeProjectedBytes) + 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) @@ -168,3 +172,214 @@ func TestRenameCascade_RefusesTheSingleDocumentAttack(t *testing.T) { 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) + } +} From f15f99b5fe4a059346fbe8e834ef715de1787c29 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 18:54:06 +0000 Subject: [PATCH 03/14] fix(documents): tighten the retry budget, stop charging no-op rewrites, order the typed check first (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 3 on #1218, an edge-case angle over the new arithmetic and control flow. Three findings fixed, one declined. 1. The retry budget credited back this document's own share, 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 be exceeded by up to one document's share while the arithmetic still reported it satisfied. The budget is now the genuine headroom, `cap - retained`. 2. A concurrent edit that REMOVES the link left a body with no occurrences, which cascadeRetainedBytes still charged twice — once for the read and once for a rewritten copy that does not exist, because strings.Replace returns its input unchanged when there is nothing to replace. That could refuse an otherwise valid rename for memory the cascade never allocates. 3. The handler classified this error by PROSE before testing it by identity. The UNIQUE-constraint arm matches a substring, and the refusal error embeds the caller's title verbatim, so renaming a document to a title containing the words "UNIQUE constraint" came back as a 409 name collision — advice to pick a different name, for a rename that was refused for size and would fail identically under any name. Typed sentinel now tested first. DECLINED: unchecked int64 arithmetic in the projection. The multiplicands are derived from the length of a string already resident in memory, so overflowing int64 needs a single document body of roughly nine exabytes; and the accumulator returns as soon as it passes the cap, so it cannot run away either. Saturating arithmetic here would be guarding a state the machine cannot reach. Tests, three new, each mutation-verified: - RetryBudgetExcludesThisDocumentsOwnStrings — deliberately separate from the existing retry test, because that one catches the check being ABSENT and this one catches it being too GENEROUS. The grown body is sized to fall BETWEEN the two budgets; a body far over the cap cannot tell them apart. - ConcurrentEditThatRemovesTheLinkDoesNotRefuseTheRename — its first version sized the link-free body against the CAP rather than against the retry's real headroom, so the refusal it caught was correct behaviour and the test was wrong, not the code. Re-sized against the headroom: fits when charged once, does not when charged twice. - IsNotMisreportedAsATitleCollision — at the handler, since the defect is entirely in its classification order. Mutation matrix for this round: credit the share back -> only the tight-budget test fails; charge the no-op body twice -> only the link-removed test fails; order the substring arm first -> only the misclassification test fails. Gates: `go test ./...` under Postgres 17 EXIT=0; touched packages re-run after the lint fix EXIT=0; gofmt clean; `make lint` 0 issues. CI green on b09aca12. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/handlers_documents.go | 40 +++-- .../server/handlers_documents_title_test.go | 44 ++++++ internal/store/documents.go | 33 +++- .../store/documents_rename_bounds_test.go | 144 ++++++++++++++++++ 4 files changed, 239 insertions(+), 22 deletions(-) diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 0b30f971e..6df6a7e40 100644 --- a/internal/server/handlers_documents.go +++ b/internal/server/handlers_documents.go @@ -158,6 +158,31 @@ 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 a bound on OUTPUT rather than on the request body + // (`image_too_large` in handlers_attachments_transform.go, where the + // request is likewise small and the thing refused is what it would + // produce). + if errors.Is(err, store.ErrRenameCascadeTooLarge) { + writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large", + "This rename would rewrite more linked content than the server will process in one operation. "+ + "Reduce the number of documents linking this title, or shorten the new title, and try again. ("+err.Error()+")") + return + } if strings.Contains(err.Error(), "UNIQUE constraint") { writeError(w, http.StatusConflict, "conflict", "A document with this title already exists in this workspace") return @@ -175,21 +200,6 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { // 500 would be actively misleading — it says the request will never // work. Distinguished by sentinel rather than message text, because // this one is ours to name (codex round 2). - // A projected-output refusal is PERMANENT-shaped, and must not join - // the retryable family below it. Retrying this rename unchanged fails - // identically until the workspace's content changes, so it gets a 4xx - // with no Retry-After — and it carries the projection, because the - // only actionable information is what was projected against what is - // allowed. 413 follows this codebase's own precedent for a bound on - // output rather than on the request body (`image_too_large` in - // handlers_attachments_transform.go, where the request is likewise - // small and the thing refused is what it would produce). - if errors.Is(err, store.ErrRenameCascadeTooLarge) { - writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large", - "This rename would rewrite more linked content than the server will process in one operation. "+ - "Reduce the number of documents linking this title, or shorten the new title, and try again. ("+err.Error()+")") - return - } if isRetryableLockError(err) || errors.Is(err, store.ErrLinkCascadeContention) { w.Header().Set("Retry-After", "1") // Deliberately does NOT name the holder. The previous wording said diff --git a/internal/server/handlers_documents_title_test.go b/internal/server/handlers_documents_title_test.go index 1060bf8b3..bd7c388de 100644 --- a/internal/server/handlers_documents_title_test.go +++ b/internal/server/handlers_documents_title_test.go @@ -134,3 +134,47 @@ func TestRenameCascadeTooLarge_IsPermanentShapedNotRetryable(t *testing.T) { t.Errorf("response lacks the projection; the caller cannot tell how far over they are: %s", b) } } + +// 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()) + } +} diff --git a/internal/store/documents.go b/internal/store/documents.go index d14e4e7b9..ce061771b 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -672,13 +672,20 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri } for _, du := range updates { - // Budget handed to this document's compare-and-set: the cap, less - // everything the OTHER documents are holding. A retry that re-reads a - // body grown by a concurrent edit is checked against it, so the - // aggregate bound survives the retry path as well as the scan (codex - // round 1 P1 — the guard used to cover only the scan, and the retry - // called ReplaceTitle on an unbounded re-read). - budget := MaxRenameCascadeRetainedBytes - (retained - du.retained) + // Budget handed to this document's compare-and-set: the headroom that + // remains under the cap with everything ALREADY held subtracted — + // including this document's own read and rewritten bodies, which + // `updates` still references while the loop runs. + // + // An earlier version credited back du.retained on the reasoning that + // the retry replaces this document's contribution. It does not: the + // originals stay reachable through the slice, so the re-read and its + // rewrite are allocated ON TOP of them, and the bound could be + // exceeded by up to one document's share while the arithmetic still + // reported it satisfied (codex round 3 P1). A retry that cannot fit in + // the genuine headroom is refused, which is conservative in the rare + // contended case and honest about what the cap means. + budget := MaxRenameCascadeRetainedBytes - retained if err := s.rewriteLinkerCAS(tx, du.id, du.read, du.rewritten, oldTitle, newTitle, searchTerm, budget); err != nil { return err } @@ -697,6 +704,18 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri // path must not be allowed to drift apart on it. func cascadeRetainedBytes(read, searchTerm, oldTitle, newTitle string) int64 { occurrences := int64(strings.Count(read, searchTerm)) + 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 } diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go index 0a83100a0..6cecaebd4 100644 --- a/internal/store/documents_rename_bounds_test.go +++ b/internal/store/documents_rename_bounds_test.go @@ -383,3 +383,147 @@ func TestRenameCascade_RetryRecheckesTheBudgetAgainstTheGrownBody(t *testing.T) 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) + } +} + +// 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) + } +} From 54410066be2f7cd56d9263cca9d4340fd7c7f092 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 19:13:12 +0000 Subject: [PATCH 04/14] test(documents): close four ways the cascade-bound tests could pass for the wrong reason (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4, aimed at the TESTS rather than the code. No production behaviour changes here; four instruments that were weaker than they read. 1. Every retained-byte case exceeded the cap under `max(read, rewritten)` as well as under `read + rewritten`, so none of them could tell the two arithmetics apart — and taking the larger would hold twice the cap. Added CountsBothStringsNotTheLargerOne: an ordinary same-length rename over content totalling ~60% of the cap, which the sum refuses and the max admits. Its preconditions assert both halves of that gap. 2. Nothing pinned the 255 itself. Every length case derived its inputs from MaxDocumentTitleRunes, so changing the constant to 512 left them all green. That is fine for arithmetic and wrong for this number: it is a product decision Dave ruled, and a silent change to it silently changes how much amplification the cheap door lets through. Deliberately NOT done for MaxRenameCascadeRetainedBytes, which is mine and carries a measured receipt that is expected to be re-measured. 3. The oversize tests asserted only THAT a rename is refused, never that it is refused BEFORE the amplified string is built — which is the entire point of the guard. Moving links.ReplaceTitle above it would have kept them green. Added RefusesBeforeBuildingTheRewrittenBody, measuring cumulative allocation with a ~20x margin: refusing costs the one body it had to scan, building first costs ~108 MB. Verified by mutation — with the guard moved after the rewrite it reports 110,748,144 bytes against a 52,428,800 ceiling. This filing warned that measuring memory to prove the ABSENCE of amplification is flaky by construction. That still holds for the shape it described, a peak-RSS floor. This is the opposite: a generous ceiling on a deterministic counter, with the two outcomes twenty times apart. 4. The "reports the projection" assertions checked for the words `maximum` and `bytes`, which a message saying "maximum bytes exceeded" would satisfy while telling a caller nothing. They now require the cap's actual value and a real byte count. Mutation matrix: charge only the larger string -> only CountsBothStrings fails; move the rewrite above the guard -> only RefusesBeforeBuilding fails. Seventeen mutations across four rounds, each detected by the test that should catch it. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/models/document_title_test.go | 21 ++++ .../store/documents_rename_bounds_test.go | 114 +++++++++++++++++- 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/internal/models/document_title_test.go b/internal/models/document_title_test.go index 7d26ee9e6..2d6d4b4cc 100644 --- a/internal/models/document_title_test.go +++ b/internal/models/document_title_test.go @@ -89,6 +89,27 @@ func TestDocumentTitleValidationMatchesTheRoundTripProperty(t *testing.T) { } } +// 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. // diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go index 6cecaebd4..210e63cf2 100644 --- a/internal/store/documents_rename_bounds_test.go +++ b/internal/store/documents_rename_bounds_test.go @@ -2,6 +2,9 @@ package store import ( "errors" + "fmt" + "regexp" + "runtime" "strings" "sync" "testing" @@ -90,8 +93,15 @@ func TestRenameCascade_RefusesOnProjectedTOTAL_NotPerDocument(t *testing.T) { // 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. - if msg := err.Error(); !strings.Contains(msg, "maximum") || !strings.Contains(msg, "bytes") { - t.Errorf("error message lacks the projection: %q", msg) + // 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 @@ -527,3 +537,103 @@ func TestRenameCascade_ConcurrentEditThatRemovesTheLinkDoesNotRefuseTheRename(t "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 +// the margin is ~20x: refusing allocates the one read body it had to scan +// (~2 MiB plus overhead), while building the rewrite first allocates +// ~108 MB. The threshold sits far from both. +// +// 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) + } + if allocated := after.TotalAlloc - before.TotalAlloc; 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) + } +} From f80e58186e948ce1f7c4747309d579b57409c249 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 20:28:34 +0000 Subject: [PATCH 05/14] fix(documents): compose the 413 from typed fields instead of splicing err.Error() (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 5, on the side effects of a REFUSED rename. Side effects were otherwise clean — rollback removes versions, link rewrites, attachment stamps and the title change, and no activity row, SSE event or webhook is emitted — but the response body was built by appending err.Error() to a public sentence. That published whatever any layer had wrapped around the error on its way up. Today that is "update links: store: ", which is a call path clients have no business seeing; tomorrow it is whatever the next wrapper adds, with no decision point in between. The response is now composed from typed fields on a new store.RenameCascadeTooLargeError (NewTitle, Retained, Max), reached with errors.As. Unwrap keeps errors.Is(err, ErrRenameCascadeTooLarge) true, so every existing sentinel check is unaffected. Both FIGURES stay in the message, deliberately: Dave's day-63 ruling asked the refusal to state what it would hold and what the cap is, so "split the rename" is actionable advice rather than a shrug. The reviewer read those numbers as a content-size oracle; that framing does not survive the trust boundary — a rename requires `editor`, documents are readable at `viewer`, so the caller can already read every document the figure summarises and learns nothing from it. What they had no business receiving was the internal call path, and that is what changed. This is round 3's 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. The test now asserts both halves: the real byte counts are present (not merely the word "maximum"), and the strings "update links:" and "store:" are ABSENT. Mutation-verified — splicing err.Error() back in fails on both leaked prefixes. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/handlers_documents.go | 13 ++++-- .../server/handlers_documents_title_test.go | 29 ++++++++++-- internal/store/documents.go | 44 ++++++++++++++++++- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 6df6a7e40..072f0debc 100644 --- a/internal/server/handlers_documents.go +++ b/internal/server/handlers_documents.go @@ -177,10 +177,17 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { // (`image_too_large` in handlers_attachments_transform.go, where the // request is likewise small and the thing refused is what it would // produce). - if errors.Is(err, store.ErrRenameCascadeTooLarge) { + 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", - "This rename would rewrite more linked content than the server will process in one operation. "+ - "Reduce the number of documents linking this title, or shorten the new title, and try again. ("+err.Error()+")") + 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") { diff --git a/internal/server/handlers_documents_title_test.go b/internal/server/handlers_documents_title_test.go index bd7c388de..955aeaa23 100644 --- a/internal/server/handlers_documents_title_test.go +++ b/internal/server/handlers_documents_title_test.go @@ -1,7 +1,9 @@ package server import ( + "fmt" "net/http" + "regexp" "strings" "testing" @@ -127,11 +129,30 @@ func TestRenameCascadeTooLarge_IsPermanentShapedNotRetryable(t *testing.T) { if got := rr.Header().Get("Retry-After"); got != "" { t.Errorf("Retry-After = %q; a permanent refusal must not invite a retry", got) } - if b := rr.Body.String(); !strings.Contains(b, "rename_cascade_too_large") { - t.Errorf("response lacks the error code a client would switch on: %s", b) + 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) } - if b := rr.Body.String(); !strings.Contains(b, "maximum") { - t.Errorf("response lacks the projection; the caller cannot tell how far over they are: %s", b) + + // 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) + } } } diff --git a/internal/store/documents.go b/internal/store/documents.go index ce061771b..bb63eece8 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -720,9 +720,49 @@ func cascadeRetainedBytes(read, searchTerm, oldTitle, newTitle string) int64 { 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 fmt.Errorf("%w: renaming to %q would hold at least %d bytes of linked-document content, maximum %d", - ErrRenameCascadeTooLarge, newTitle, retained, MaxRenameCascadeRetainedBytes) + return &RenameCascadeTooLargeError{ + NewTitle: newTitle, + Retained: retained, + Max: MaxRenameCascadeRetainedBytes, + } } // ErrLinkCascadeContention reports that a rename's link cascade lost its From 0f6f504194b4d9cfe2516803931323f29999267d Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 20:42:30 +0000 Subject: [PATCH 06/14] docs(documents): correct eight claims the prose made that the code does not (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 6, aimed at the comments rather than the code. Eight findings, all mine, all real, no behaviour changed. This is the failure mode my own trail keeps naming — code right, prose broader than the sweep, always in the same direction — so they are corrected individually rather than smoothed over. Stale after the round-1 rename: - models cited store.MaxRenameCascadeProjectedBytes, which no longer exists. - the constant's own hostile figure read 110,729,522; it is 110,729,520. - the HTTP test said each body is ~135 KB; the formula produces 264,790 bytes. Claims wider than what is true: - The round-trip test's header stated a biconditional over the whole validator. False: a 300-rune title round-trips perfectly and is still refused, for the unrelated reason that it is an amplification factor. The property is about the SYNTAX rule, over titles inside the length bound, and now says so. - The mirrored grammar/unescaper comment claimed that a TypeScript change would make this test start disagreeing. It cannot — they are static copies, and nothing in the repository fails when the two drift. Replaced with what the duplication actually buys and what it does not. - The cap's receipt used `items` measurements to conclude the guard "cannot fire on honest use" for DOCUMENTS, having itself noted that instance's documents table is empty. The proxy is reasonable and it is an assumption, not a measurement of the guarded path; the inference is now named, with the narrower claim that survives without it. - The 413's comment cited the image_too_large precedent as a bound on OUTPUT while this guard bounds retained read-plus-write. What carries across is the shape — a small request refused for what handling it would cost — not the quantity. - TestRenameCascade_RefusesTheSingleDocumentAttack described the 2 MiB / 110,729,520-byte shape it does not build; it sizes from the cap (1,271,000 bytes retaining 67,108,800). The full-strength shape is exercised by the allocation test, and the comment now points there instead of describing a body that is not in the function. One figure was replaced by measurement rather than corrected by arithmetic: the allocation test claimed a "~20x margin". Both sides are now measured and stated separately — refusing allocates 2,114,624 bytes, ~24.8x under the 52,428,800 ceiling; building the rewrite first allocates 110,748,144, ~2.1x OVER it, taken from running the test against the mutation rather than computed. The smaller margin is the binding one, since it is the gap a regression must cross to be caught, and saying "~20x" hid that. Gates: `go test` on the three touched packages under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/models/document.go | 4 +-- internal/models/document_title_test.go | 22 +++++++++--- internal/server/handlers_documents.go | 8 +++-- .../server/handlers_documents_title_test.go | 9 ++--- internal/store/documents.go | 23 +++++++++---- .../store/documents_rename_bounds_test.go | 34 ++++++++++++++----- 6 files changed, 71 insertions(+), 29 deletions(-) diff --git a/internal/models/document.go b/internal/models/document.go index 048b21743..dd07f088a 100644 --- a/internal/models/document.go +++ b/internal/models/document.go @@ -92,8 +92,8 @@ type DocumentListParams struct { // 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 projection guard (store.MaxRenameCascadeProjectedBytes) is -// byte-accurate and is what actually bounds the work. +// 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 diff --git a/internal/models/document_title_test.go b/internal/models/document_title_test.go index 2d6d4b4cc..89120d8cd 100644 --- a/internal/models/document_title_test.go +++ b/internal/models/document_title_test.go @@ -10,9 +10,13 @@ import ( // 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. If the TypeScript changes, these should -// start disagreeing with ValidateDocumentTitle — that is the signal wanted, -// not a nuisance. +// 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(`\[\[((?:\\.|[^\]\\])+)\]\]`) @@ -41,8 +45,16 @@ func roundTrips(title string) bool { } // TestDocumentTitleValidationMatchesTheRoundTripProperty is the justification -// for the rule, in executable form: the validator must reject a title if and -// only if that title's links would not survive being rewritten to it. +// 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. diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 072f0debc..3ebd1e624 100644 --- a/internal/server/handlers_documents.go +++ b/internal/server/handlers_documents.go @@ -173,10 +173,12 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { // 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 a bound on OUTPUT rather than on the request body + // 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 the thing refused is what it would - // produce). + // 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. var tooLarge *store.RenameCascadeTooLargeError if errors.As(err, &tooLarge) { // Composed from TYPED fields, never by splicing err.Error(). The diff --git a/internal/server/handlers_documents_title_test.go b/internal/server/handlers_documents_title_test.go index 955aeaa23..7ebdaa6c8 100644 --- a/internal/server/handlers_documents_title_test.go +++ b/internal/server/handlers_documents_title_test.go @@ -105,10 +105,11 @@ func TestRenameCascadeTooLarge_IsPermanentShapedNotRetryable(t *testing.T) { target := createDocForTest(t, srv, slug, "A", "the document being renamed") - // Three linking documents, each projecting under the cap, together over - // it — the same total-not-per-document shape the store test pins, driven - // through real requests. Each body is ~135 KB, well inside the 2 MiB - // request cap, which is the point: the hostile input is cheap to deliver. + // 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)) diff --git a/internal/store/documents.go b/internal/store/documents.go index bb63eece8..6bba91dec 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -806,14 +806,23 @@ var ErrRenameCascadeTooLarge = errors.New("store: rename cascade exceeds the ret // 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 there: 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, so the guard cannot fire on honest use. -// (Measured on `items`, the live surface; that instance's `documents` -// table is empty, which is why the proxy — the two carry the same kind of -// content through the same kind of cascade.) +// 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,522 bytes once the title bound +// 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. // diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go index 210e63cf2..bff77daad 100644 --- a/internal/store/documents_rename_bounds_test.go +++ b/internal/store/documents_rename_bounds_test.go @@ -155,10 +155,18 @@ func TestRenameCascade_AllowsAnOrdinaryRename(t *testing.T) { } // TestRenameCascade_RefusesTheSingleDocumentAttack covers the k=1 case -// directly: one linking document holding the largest body a 2 MiB request can -// carry still retains 110,729,522 bytes once the title bound is in place -// (108,632,370 written plus the 2,097,152 read), which is 3.3x the cap. The -// attack is refused at every k, not only at a threshold count of documents. +// 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 @@ -589,9 +597,16 @@ func TestRenameCascade_CountsBothStringsNotTheLargerOne(t *testing.T) { // string is never built. // // The instrument is cumulative allocation (TotalAlloc), not peak memory, and -// the margin is ~20x: refusing allocates the one read body it had to scan -// (~2 MiB plus overhead), while building the rewrite first allocates -// ~108 MB. The threshold sits far from both. +// 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 @@ -631,7 +646,10 @@ func TestRenameCascade_RefusesBeforeBuildingTheRewrittenBody(t *testing.T) { if !errors.Is(err, ErrRenameCascadeTooLarge) { t.Fatalf("rename: got %v, want ErrRenameCascadeTooLarge", err) } - if allocated := after.TotalAlloc - before.TotalAlloc; allocated > allocCeiling { + 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) From f5d686716730e02f494a925321cc207e7cbe6b73 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 21:06:01 +0000 Subject: [PATCH 07/14] fix(documents): stop charging the cascade budget for rows the rewriter cannot touch (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 7. Third instance of one class, and the one I stopped short of when I extended the previous two. The SELECT that finds candidate linkers is a LIKE; the thing that rewrites them is links.ReplaceTitle. They do not agree on what counts as a match, so the SELECT returns a SUPERSET — and every row in the difference was charged to the caller's retained-byte budget and then handed a no-op UPDATE. Instances one and two were `%` and `_`, wildcards on both dialects, closed by the ESCAPE clause. This is the case half, and it splits the OTHER way from the backslash bug: SQLite's LIKE is ASCII case-insensitive by default while Postgres's is case-sensitive, so renaming `Alpha` scans every body holding `[[alpha]]` on SQLite only. ReplaceTitle is case-sensitive on both and will never touch them, so enough case-variant content could push an otherwise valid rename to a 413 — on one dialect, for content that was never in scope. The fix is to skip a row with no case-sensitive occurrence outright, which closes both halves: no budget is spent, and no pointless UPDATE is issued for a body the cascade was never going to change. The authority on what is a linker is the rewriter's own count, not the pattern that proposed the candidate. The test runs on BOTH dialects deliberately, unlike its Postgres-only 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. It also asserts the case variants are left byte-identical — `[[alpha]]` is a different link, not a missed one. Mutation-verified: restoring the charge fails it with 33,554,790 bytes against the 33,554,432 cap. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. An earlier attempt at that gate died on host disk exhaustion, not on this diff — 373 stale go-tmp directories from crashed runs, cleared, re-run green. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/store/documents.go | 27 +++++++-- .../store/documents_rename_bounds_test.go | 59 +++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/internal/store/documents.go b/internal/store/documents.go index 6bba91dec..36616e13c 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -648,7 +648,26 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri // 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. - du.retained = cascadeRetainedBytes(du.read, searchTerm, oldTitle, newTitle) + // 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) @@ -702,8 +721,7 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri // 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, searchTerm, oldTitle, newTitle string) int64 { - occurrences := int64(strings.Count(read, searchTerm)) +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 @@ -968,7 +986,8 @@ func (s *Store) rewriteLinkerCAS(tx *sql.Tx, id, read, rewritten, oldTitle, newT // 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). - if grown := cascadeRetainedBytes(current, searchTerm, oldTitle, newTitle); grown > budget { + grownOccurrences := int64(strings.Count(current, searchTerm)) + if grown := cascadeRetainedBytes(current, grownOccurrences, oldTitle, newTitle); grown > budget { return newRenameCascadeTooLargeError(newTitle, grown) } diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go index bff77daad..7451d3d42 100644 --- a/internal/store/documents_rename_bounds_test.go +++ b/internal/store/documents_rename_bounds_test.go @@ -655,3 +655,62 @@ func TestRenameCascade_RefusesBeforeBuildingTheRewrittenBody(t *testing.T) { 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) + } + } +} From ed9997b30f88c4c5fdb97786bef17a367d43b84f Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 23:29:10 +0000 Subject: [PATCH 08/14] fix(documents): report the whole operation's size when a retry is refused (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 8, on concurrency and the rest of the rename transaction. The advisory-lock lifetime, the CAS loop's termination, the ordering against attachment stamps and version writes, and the new skip's effect on the transaction's invariants all came back clean. One P2 stood. The retry path bounded correctly and REPORTED wrongly. It compared the re-read body against the headroom — right — and then named only that body in the error. Everything the scan counted is still held, so the operation's real size is the scan total plus the re-read, and a refusal could therefore say it would hold 16,777,200 bytes against a limit of 33,554,432: a refusal whose own figures do not justify it, which reads as a server bug rather than as advice you can act on. The compare-and-set is now handed the scan TOTAL instead of a pre-computed budget, so the same number both bounds and explains: refuse when scanTotal + grown exceeds the cap, and report scanTotal + grown. The test now asserts the refusal justifies itself — the figure reported must exceed the cap it cites — via the typed error added in round 5, which is what makes that property checkable at all rather than a string comparison. Mutation-verified: reporting the re-read alone fails it with exactly the 16,777,200-against-33,554,432 shape. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/store/documents.go | 40 +++++++++++-------- .../store/documents_rename_bounds_test.go | 14 +++++++ 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/internal/store/documents.go b/internal/store/documents.go index 36616e13c..75a86b61f 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -691,21 +691,19 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri } for _, du := range updates { - // Budget handed to this document's compare-and-set: the headroom that - // remains under the cap with everything ALREADY held subtracted — - // including this document's own read and rewritten bodies, which - // `updates` still references while the loop runs. + // 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). // - // An earlier version credited back du.retained on the reasoning that - // the retry replaces this document's contribution. It does not: the - // originals stay reachable through the slice, so the re-read and its - // rewrite are allocated ON TOP of them, and the bound could be - // exceeded by up to one document's share while the arithmetic still - // reported it satisfied (codex round 3 P1). A retry that cannot fit in - // the genuine headroom is refused, which is conservative in the rare - // contended case and honest about what the cap means. - budget := MaxRenameCascadeRetainedBytes - retained - if err := s.rewriteLinkerCAS(tx, du.id, du.read, du.rewritten, oldTitle, newTitle, searchTerm, budget); err != nil { + // 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 } } @@ -947,7 +945,7 @@ 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, searchTerm string, budget int64) error { +func (s *Store) rewriteLinkerCAS(tx *sql.Tx, id, read, rewritten, oldTitle, newTitle, searchTerm string, scanTotal int64) error { expected := read next := rewritten for attempt := 0; attempt < cascadeRewriteAttempts; attempt++ { @@ -987,8 +985,16 @@ func (s *Store) rewriteLinkerCAS(tx *sql.Tx, id, read, rewritten, oldTitle, newT // rename straight back into the amplification it was refused for // (BUG-2798, codex round 1 P1). grownOccurrences := int64(strings.Count(current, searchTerm)) - if grown := cascadeRetainedBytes(current, grownOccurrences, oldTitle, newTitle); grown > budget { - return newRenameCascadeTooLargeError(newTitle, grown) + grown := cascadeRetainedBytes(current, grownOccurrences, oldTitle, newTitle) + 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 diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go index 7451d3d42..ad31b6702 100644 --- a/internal/store/documents_rename_bounds_test.go +++ b/internal/store/documents_rename_bounds_test.go @@ -476,6 +476,20 @@ func TestRenameCascade_RetryBudgetExcludesThisDocumentsOwnStrings(t *testing.T) 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 From 53345e0a429d735a0bdb347707aa885cb90215f9 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 27 Aug 2026 23:52:09 +0000 Subject: [PATCH 09/14] fix(documents): refuse titles whose links the cascade cannot find; count retry buffers (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 9, asking what is MISSING rather than what is wrong. Three findings: two fixed, one already filed. ## Titles validated against the wrong layer The validator ACCEPTED `Alpha|Beta` and `Alpha\Beta`, and I defended that choice with a test. Both round-trip perfectly — the renderer reads each back as exactly the title it started from — and the first version of this fix banned them on vibes, so being shown that was a real correction. It was still the wrong call, for a reason that test could not see. A link to such a title can be STORED escaped, as `[[Alpha\|Beta]]`, and the rename cascade searches for the raw `[[Alpha|Beta]]` only. It does not find those links, so the rename succeeds and leaves them pointing at a title that no longer exists — silently, which is BUG-2796's defect wearing different syntax. So the property a title has to satisfy is stricter than the one I tested: not "the renderer reads it back", but "the renderer reads it back AND the cascade can find its links". Validating against the layer that DISPLAYS a title while the layer that MAINTAINS it disagrees is the same mistake as the unescaped LIKE, met from the other side — twice in one unit, which is the part worth noticing. `[` stays accepted: the cascade's search term matches it literally, so it passes the stricter property too. The two characters get their own test rather than a row in the round-trip table, because the table asserts a biconditional and these are refused for a reason that predicate deliberately does not model. That test asserts its own premise — each title must still round-trip — so if that ever stops being true it fails rather than passing for a new reason. ## Retry buffers were not counted rewriteLinkerCAS bounded each retry against the scan total plus THAT attempt. Earlier attempts' buffers become unreachable when expected/next are reassigned, but unreachable is not reclaimed, so a run of failures could hold several copies while the arithmetic counted one. Now accumulated across attempts, which is conservative — it counts garbage as if live — and errs toward refusing, which is the safe direction for a memory bound. The loop is capped at cascadeRewriteAttempts, so it cannot grow without end. ## Already filed Item renames remaining unbounded, and item titles lacking this validation, are real and out of this unit's scope: BUG-2804 and BUG-2805, filed after round 2 with the code verified rather than taken on report. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/models/document.go | 42 ++++++++++--------- internal/models/document_title_test.go | 33 ++++++++++++++- .../server/handlers_documents_title_test.go | 7 +++- internal/store/documents.go | 13 +++++- 4 files changed, 70 insertions(+), 25 deletions(-) diff --git a/internal/models/document.go b/internal/models/document.go index dd07f088a..7fde638dd 100644 --- a/internal/models/document.go +++ b/internal/models/document.go @@ -122,28 +122,29 @@ const MaxDocumentTitleRunes = 255 // 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. A title containing -// `\\` or `\|` is emitted raw, unescaped on the way back in, and the -// result no longer equals the title — so the link resolves to nothing, -// or to a different document. +// 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. BUG-2796's filing proposed rejecting "`[[` or `]]`"; -// measured against the grammar, the `[[` half of that is overreach. -// - `|` — resolveWikiBody tries a FULL-BODY title match before the pipe -// split, a branch whose comment says it exists precisely to handle -// "stored legacy titles that contain a literal `|`". Banning it would -// refuse what that branch was written to support. -// - a lone `\` not followed by `\`, `]` or `|` — passes the grammar as an -// escape pair and survives the unescaper unchanged. Note this one depends -// on store.escapeLikePattern: the rename cascade finds linking documents -// with `content LIKE`, where Postgres reads `\` as an escape character, so -// before that escaping landed a backslash title rendered fine and then -// silently failed to cascade on one dialect. Allowing it here is only -// correct while the cascade's pattern stays escaped. +// 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 @@ -158,9 +159,10 @@ func wikiTitleRoundTripFailure(title string) string { return `Title may not end with "\" — it would escape the closing bracket of the [[wiki-links]] that point ` + `at this document` } - if strings.Contains(title, `\\`) || strings.Contains(title, `\|`) { - return `Title may not contain "\\" or "\|" — those are escape sequences in [[wiki-link]] syntax, so the ` + - `links that point at this document would resolve to a different title` + 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 "" } diff --git a/internal/models/document_title_test.go b/internal/models/document_title_test.go index 89120d8cd..c72c2db6d 100644 --- a/internal/models/document_title_test.go +++ b/internal/models/document_title_test.go @@ -80,8 +80,6 @@ func TestDocumentTitleValidationMatchesTheRoundTripProperty(t *testing.T) { // them breaks anything. {"open bracket", `Alpha[Beta`}, {"double open bracket", `Alpha[[Beta`}, - {"literal pipe", `Alpha|Beta`}, - {"lone backslash", `Alpha\Beta`}, {"collection-qualified shape", `collection/Title`}, {"colons", `Ratio 1:2`}, {"ordinary punctuation", `Title (draft) — v2`}, @@ -152,3 +150,34 @@ func TestDocumentTitleLengthBoundCountsRunesNotBytes(t *testing.T) { } } } + +// 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_title_test.go b/internal/server/handlers_documents_title_test.go index 7ebdaa6c8..97972b5f4 100644 --- a/internal/server/handlers_documents_title_test.go +++ b/internal/server/handlers_documents_title_test.go @@ -54,9 +54,12 @@ func TestDocumentTitleValidation_IsWiredOnBothWriteDoors(t *testing.T) { // Controls. Without these a handler that rejected every title — or // one that rejected any title containing punctuation — passes the - // legs above. + // 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.StatusOK}, + {"contains a literal pipe", `Alpha|Beta`, http.StatusBadRequest}, {"ordinary", "A Renamed Document", http.StatusOK}, } { t.Run("update/"+tc.name, func(t *testing.T) { diff --git a/internal/store/documents.go b/internal/store/documents.go index 75a86b61f..025012a48 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -948,6 +948,8 @@ var cascadeRewriteAttempts = 3 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 = ? @@ -985,7 +987,16 @@ func (s *Store) rewriteLinkerCAS(tx *sql.Tx, id, read, rewritten, oldTitle, newT // rename straight back into the amplification it was refused for // (BUG-2798, codex round 1 P1). grownOccurrences := int64(strings.Count(current, searchTerm)) - grown := cascadeRetainedBytes(current, grownOccurrences, oldTitle, newTitle) + // 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 From bb3496525cdeb185217e0ab1b1a83f5bd4d8a5eb Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 01:02:34 +0000 Subject: [PATCH 10/14] fix(documents): validate a title only when the rename actually changes it (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 10, on back-compatibility. This one is a regression THIS fix introduced, not one it inherited, and it falsified a promise the fix makes about itself in three places. "Enforced at write time; existing titles stay valid until their next rename" is Dave's ruling, and it is 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. Under that, a document with a legacy title became uneditable rather than merely un-renameable — the opposite of grandfathering. Grandfathering is not something you get by validating at write time. It is something you get by not validating a write that is not a rename. The check now fires only when the supplied title DIFFERS from the stored one, which is also exactly the test the store already applies before cascading, so the validation and the work it guards now agree on what counts as a rename. The regression test seeds its legacy document through the store, because the title it needs can no longer be created through the API — which is precisely the population the grandfathering clause exists for. Three legs: the echoed-title content edit succeeds, a title-less content PATCH succeeds, and renaming to another invalid title is still refused. The last is the control; without it, deleting the validation entirely would pass. Filed rather than folded: BUG-2806, existing documents whose links are stored in escaped form are still orphaned by a rename. Round 9 stopped NEW titles of that shape being created; it did not repair the ones already stored, and the asymmetry — the product refusing to create a shape it still mishandles — belongs in the record rather than in this PR, which is nine commits deep on a bound it has already outgrown. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/handlers_documents.go | 13 ++++- .../server/handlers_documents_title_test.go | 58 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 3ebd1e624..47484dc3d 100644 --- a/internal/server/handlers_documents.go +++ b/internal/server/handlers_documents.go @@ -141,7 +141,18 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { // 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. - if input.Title != nil { + // + // 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 diff --git a/internal/server/handlers_documents_title_test.go b/internal/server/handlers_documents_title_test.go index 97972b5f4..177aab89b 100644 --- a/internal/server/handlers_documents_title_test.go +++ b/internal/server/handlers_documents_title_test.go @@ -203,3 +203,61 @@ func TestRenameCascadeTooLarge_IsNotMisreportedAsATitleCollision(t *testing.T) { 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()) + } +} From 65569bf8b5bdbf2d7aa9095b79786a612680311c Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 01:30:19 +0000 Subject: [PATCH 11/14] fix(documents): validate the rename under the lock, not against a pre-lock read (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 11. Round 10 moved title validation behind "only when the title actually changes", which is the right rule and was applied at the wrong place. The handler compares the supplied title against a document it read BEFORE the rename lock. UpdateDocument re-reads under the lock. Those can disagree: echo a legacy title back on a content edit while another request renames the document, and the handler sees "unchanged, skip validation" while the store sees a genuine rename — and writes the legacy title through with nothing having checked it. The rule is unchanged; the enforcement point moved to where the rename is actually decided. The store validates inside the transaction, on the same branch that triggers the cascade, and returns a typed InvalidDocumentTitleError carrying the reason. The handler keeps its pre-lock check, which is still worth having — it gives the common case a fast 400 without opening a transaction — and gains an arm that surfaces the store's refusal for the case its own check could not see. Grandfathering survives intact, because the store's check sits on the title-actually-changed branch, which is the same condition the handler uses. The test calls the store directly rather than reproducing the race: driving the interleaving would test the scheduler, while the property worth pinning is that the store refuses regardless of what a caller did. Its control leg is the grandfathering case — a content edit echoing the unchanged legacy title must still succeed — so validating everything here would fail it, which is round 10's regression restated as a guard. Mutation-verified: removing the store-side check leaves the handler tests green and fails this one with a nil error. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/handlers_documents.go | 9 +++++ internal/store/documents.go | 30 ++++++++++++++++ .../store/documents_rename_bounds_test.go | 36 +++++++++++++++++++ 3 files changed, 75 insertions(+) diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 47484dc3d..30b14e85e 100644 --- a/internal/server/handlers_documents.go +++ b/internal/server/handlers_documents.go @@ -190,6 +190,15 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) { // 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 diff --git a/internal/store/documents.go b/internal/store/documents.go index 025012a48..6408f4b46 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) @@ -791,6 +805,22 @@ func newRenameCascadeTooLargeError(newTitle string, retained int64) error { // 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. // diff --git a/internal/store/documents_rename_bounds_test.go b/internal/store/documents_rename_bounds_test.go index ad31b6702..24a97bb2e 100644 --- a/internal/store/documents_rename_bounds_test.go +++ b/internal/store/documents_rename_bounds_test.go @@ -728,3 +728,39 @@ func TestRenameCascade_DoesNotChargeCaseVariantsTheRewriterWillNotTouch(t *testi } } } + +// 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) + } +} From c00606b0c16a65a722021b62e6fd3b8ae7ef945b Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 01:52:00 +0000 Subject: [PATCH 12/14] perf(documents): stop counting every linker's occurrences twice (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 13, on what the guard costs the SUCCESS path rather than what it blocks on the failure path. The cascade counts occurrences because the size guard needs that number before it is willing to build anything. It then handed the same content to links.ReplaceTitle, whose strings.Replace with n < 0 counts it again. Every ordinary rename therefore paid a second full pass over every linking document for a number it already had. links.ReplaceTitleN takes the count the caller already computed. It is a separate function rather than an optional parameter because the obligation is real and silent when broken: passing a number that is too small does not error, it leaves later occurrences unrewritten, which on this path means links left pointing at a title that no longer exists. A name at the call site is cheaper than a comment nobody reads. NO measured speedup is claimed, and the doc comment says so. This removes one linear pass from a path that also allocates a full copy of the same content and issues a write per linker, so the saving is real but not obviously significant. It is here because doing the same work twice needs a reason and there was not one — not because a benchmark asked for it. The test asserts equivalence with ReplaceTitle across several shapes, including the new-title-embeds-old case, and its counterfactual leg asserts that an under-count visibly DIVERGES — if it did not, the caller's obligation would be imaginary and the API misleading. Round 13 also confirmed two things worth recording: no quadratic scan across linkers, and the ESCAPE clause does not materially change the query plan because the leading `%` already forced a content scan. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/links/links.go | 24 +++++++++++++ .../links/replace_title_termination_test.go | 35 +++++++++++++++++++ internal/store/documents.go | 2 +- 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/internal/links/links.go b/internal/links/links.go index a8631822d..f37d843f3 100644 --- a/internal/links/links.go +++ b/internal/links/links.go @@ -17,6 +17,30 @@ func ReplaceTitle(content, oldTitle, newTitle string) string { return replaceAll(content, old, new) } +// ReplaceTitleN is ReplaceTitle for a caller that has ALREADY counted the +// occurrences, and it exists to stop that work being done twice. +// +// strings.Replace with n < 0 counts the string itself before building the +// result. The document rename cascade counts first anyway — its size guard +// needs the number before it is willing to build anything — so letting +// Replace re-count adds a full pass over every linking document on the +// success path (codex round 13). +// +// n MUST be the count of `[[oldTitle]]` in this exact content. Passing a +// smaller number silently leaves later occurrences unrewritten, which is why +// this is a separate function rather than an optional parameter on the one +// above: the obligation is visible at the call site. +// +// No measured speedup is claimed. This removes one linear pass from a path +// that also allocates a full copy of the same content and issues a write per +// linker, so the saving is real but not obviously significant; it is here +// because doing the same work twice needs a reason and there was not one. +func ReplaceTitleN(content, oldTitle, newTitle string, n int) string { + old := "[[" + oldTitle + "]]" + new := "[[" + newTitle + "]]" + return strings.Replace(content, old, new, n) +} + // RewriteWikiTitle rewrites the four title-form wiki-link shapes that // resolve to an item titled `oldTitle` in collection `collSlug`, // substituting `newTitle` for the title portion and preserving any diff --git a/internal/links/replace_title_termination_test.go b/internal/links/replace_title_termination_test.go index 81850a0a5..29784b353 100644 --- a/internal/links/replace_title_termination_test.go +++ b/internal/links/replace_title_termination_test.go @@ -71,3 +71,38 @@ func TestReplaceTitle_StillRewritesEveryOccurrence(t *testing.T) { t.Errorf("an occurrence survived: %q", got) } } + +// TestReplaceTitleN_MatchesReplaceTitleWhenGivenTheTrueCount pins the +// obligation ReplaceTitleN puts on its caller: given the real occurrence +// count, it must produce exactly what ReplaceTitle produces. +// +// The under-count leg is the counterfactual, and it is why the two are +// separate functions rather than one with an optional parameter — passing a +// number that is too small does not error, it silently leaves later +// occurrences unrewritten, which on the rename path means links left pointing +// at a title that no longer exists. +func TestReplaceTitleN_MatchesReplaceTitleWhenGivenTheTrueCount(t *testing.T) { + for _, tc := range []struct{ name, content, old, new string }{ + {"several occurrences", "a [[Old]] b [[Old]] c [[Old]] d", "Old", "New"}, + {"none", "nothing to see here", "Old", "New"}, + {"new embeds old", "x [[A]] y", "A", "A]] [[A"}, + {"shrinking", "[[LongOldTitle]] and [[LongOldTitle]]", "LongOldTitle", "n"}, + } { + t.Run(tc.name, func(t *testing.T) { + want := ReplaceTitle(tc.content, tc.old, tc.new) + n := strings.Count(tc.content, "[["+tc.old+"]]") + if got := ReplaceTitleN(tc.content, tc.old, tc.new, n); got != want { + t.Errorf("ReplaceTitleN with the true count %d:\n got: %q\nwant: %q", n, got, want) + } + + // Under-counting must visibly diverge, or the count is not + // load-bearing and this function has no contract worth stating. + if n > 1 { + if got := ReplaceTitleN(tc.content, tc.old, tc.new, n-1); got == want { + t.Errorf("ReplaceTitleN with a count one too low produced the correct result; " + + "the caller's obligation is not real, so the API is misleading") + } + } + }) + } +} diff --git a/internal/store/documents.go b/internal/store/documents.go index 6408f4b46..f7ce316e0 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -687,7 +687,7 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri return newRenameCascadeTooLargeError(newTitle, retained) } - du.rewritten = links.ReplaceTitle(du.read, oldTitle, newTitle) + du.rewritten = links.ReplaceTitleN(du.read, oldTitle, newTitle, int(occurrences)) updates = append(updates, du) } if err := rows.Err(); err != nil { From 3b51e06c8e398f68cdecf56fe0140d054bb5d113 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 02:06:40 +0000 Subject: [PATCH 13/14] docs(store): record the consequence the cascade cap necessarily has (BUG-2798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 14, on authorization and abuse. The refusal paths came back authorization-clean: reachable only after the workspace-access and `editor` checks, with viewers, guests, non-members and cross-workspace document IDs stopped first, and the reported byte figure exposing nothing an editor cannot already read. One consequence stands, and it is a property of having a cap at all rather than a defect in this one: once a workspace's documents linking a title exceed 32 MiB, that title can no longer be renamed, and any editor can put it in that state. Recorded in the constant's doc comment rather than fixed here, because the comparison that matters is with what it replaces. The same input previously took the server down for everyone; it now denies one operation to a role that can already delete every document in the workspace. Trading an unbounded OOM for a bounded, legible refusal is the point of the guard, not a gap in it. What IS missing is that the state has no exit but manual cleanup, with nothing telling an operator which documents to clean. That is a quota-and-recovery question rather than a cascade question, and it is filed as IDEA-2807 with three candidate shapes and an argument for the cheapest one — a state you can get out of is a different severity from one you cannot. The filing also says what is NOT established: no real workspace is known to approach the cap, and this fix's own receipt suggests none does, so it is a trap that exists rather than one anyone has fallen into. The adjacent concern the review raised — repeated near-cap renames contending for the rename lock and the connection pool — is noted there too, with the observation that the pre-existing behaviour was strictly worse, since each attempt was unbounded. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/store/documents.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/store/documents.go b/internal/store/documents.go index f7ce316e0..907113e18 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -876,6 +876,20 @@ var ErrRenameCascadeTooLarge = errors.New("store: rename cascade exceeds the ret // 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 From 9ea31984fbb819d616a01d4f1d0c9bcb9cb524c8 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 02:22:07 +0000 Subject: [PATCH 14/14] =?UTF-8?q?revert(links):=20remove=20ReplaceTitleN?= =?UTF-8?q?=20=E2=80=94=20it=20optimised=20nothing=20(BUG-2798)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 15 caught a claim of mine that was simply false. Reverting the functional half of c00606b0. I added ReplaceTitleN so the cascade could hand strings.Replace the occurrence count it had already computed, and wrote that this "removes one linear pass from a path that also allocates a full copy". It does not. strings.Replace calls Count UNCONDITIONALLY, before it looks at n: func Replace(s, old, new string, n int) string { if old == new || n == 0 { return s } // Compute number of replacements. if m := Count(s, old); m == 0 { return s } else if n < 0 || m < n { n = m } Read from this machine's GOROOT this turn, rather than recalled. Passing n constrains how many replacements are APPLIED; it does not skip the count. So the function bought nothing and cost something: a second way to do the same thing, carrying an obligation that fails SILENTLY when broken — an under-count leaves later occurrences unrewritten, which on this path means links pointing at a title that no longer exists. API surface with a silent failure mode and no payoff is worse than no API, so it goes rather than getting a corrected comment. The failure is the one my own trail keeps naming: I asserted a mechanism without reading it. What makes this instance worse than the earlier ones is that I wrote a careful hedge — "no measured speedup is claimed" — which reads as rigour while the sentence beside it stated the mechanism as fact. Declining to measure a claim is not the same as checking it, and the hedge made the unchecked claim look examined. The occurrence count stays where it is: the guard genuinely needs it before it will build anything, and computing it there is not redundant with anything the guard can avoid. Also declined this round, as already filed: renaming a legacy title with `|`, `\` or `]` leaves escaped links stale — that is BUG-2806, filed at round 10 with the mechanism verified in code. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/links/links.go | 24 ------------- .../links/replace_title_termination_test.go | 35 ------------------- internal/store/documents.go | 2 +- 3 files changed, 1 insertion(+), 60 deletions(-) diff --git a/internal/links/links.go b/internal/links/links.go index f37d843f3..a8631822d 100644 --- a/internal/links/links.go +++ b/internal/links/links.go @@ -17,30 +17,6 @@ func ReplaceTitle(content, oldTitle, newTitle string) string { return replaceAll(content, old, new) } -// ReplaceTitleN is ReplaceTitle for a caller that has ALREADY counted the -// occurrences, and it exists to stop that work being done twice. -// -// strings.Replace with n < 0 counts the string itself before building the -// result. The document rename cascade counts first anyway — its size guard -// needs the number before it is willing to build anything — so letting -// Replace re-count adds a full pass over every linking document on the -// success path (codex round 13). -// -// n MUST be the count of `[[oldTitle]]` in this exact content. Passing a -// smaller number silently leaves later occurrences unrewritten, which is why -// this is a separate function rather than an optional parameter on the one -// above: the obligation is visible at the call site. -// -// No measured speedup is claimed. This removes one linear pass from a path -// that also allocates a full copy of the same content and issues a write per -// linker, so the saving is real but not obviously significant; it is here -// because doing the same work twice needs a reason and there was not one. -func ReplaceTitleN(content, oldTitle, newTitle string, n int) string { - old := "[[" + oldTitle + "]]" - new := "[[" + newTitle + "]]" - return strings.Replace(content, old, new, n) -} - // RewriteWikiTitle rewrites the four title-form wiki-link shapes that // resolve to an item titled `oldTitle` in collection `collSlug`, // substituting `newTitle` for the title portion and preserving any diff --git a/internal/links/replace_title_termination_test.go b/internal/links/replace_title_termination_test.go index 29784b353..81850a0a5 100644 --- a/internal/links/replace_title_termination_test.go +++ b/internal/links/replace_title_termination_test.go @@ -71,38 +71,3 @@ func TestReplaceTitle_StillRewritesEveryOccurrence(t *testing.T) { t.Errorf("an occurrence survived: %q", got) } } - -// TestReplaceTitleN_MatchesReplaceTitleWhenGivenTheTrueCount pins the -// obligation ReplaceTitleN puts on its caller: given the real occurrence -// count, it must produce exactly what ReplaceTitle produces. -// -// The under-count leg is the counterfactual, and it is why the two are -// separate functions rather than one with an optional parameter — passing a -// number that is too small does not error, it silently leaves later -// occurrences unrewritten, which on the rename path means links left pointing -// at a title that no longer exists. -func TestReplaceTitleN_MatchesReplaceTitleWhenGivenTheTrueCount(t *testing.T) { - for _, tc := range []struct{ name, content, old, new string }{ - {"several occurrences", "a [[Old]] b [[Old]] c [[Old]] d", "Old", "New"}, - {"none", "nothing to see here", "Old", "New"}, - {"new embeds old", "x [[A]] y", "A", "A]] [[A"}, - {"shrinking", "[[LongOldTitle]] and [[LongOldTitle]]", "LongOldTitle", "n"}, - } { - t.Run(tc.name, func(t *testing.T) { - want := ReplaceTitle(tc.content, tc.old, tc.new) - n := strings.Count(tc.content, "[["+tc.old+"]]") - if got := ReplaceTitleN(tc.content, tc.old, tc.new, n); got != want { - t.Errorf("ReplaceTitleN with the true count %d:\n got: %q\nwant: %q", n, got, want) - } - - // Under-counting must visibly diverge, or the count is not - // load-bearing and this function has no contract worth stating. - if n > 1 { - if got := ReplaceTitleN(tc.content, tc.old, tc.new, n-1); got == want { - t.Errorf("ReplaceTitleN with a count one too low produced the correct result; " + - "the caller's obligation is not real, so the API is misleading") - } - } - }) - } -} diff --git a/internal/store/documents.go b/internal/store/documents.go index 907113e18..d10368707 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -687,7 +687,7 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri return newRenameCascadeTooLargeError(newTitle, retained) } - du.rewritten = links.ReplaceTitleN(du.read, oldTitle, newTitle, int(occurrences)) + du.rewritten = links.ReplaceTitle(du.read, oldTitle, newTitle) updates = append(updates, du) } if err := rows.Err(); err != nil {