diff --git a/internal/links/links.go b/internal/links/links.go index 18e439ff..f16e547b 100644 --- a/internal/links/links.go +++ b/internal/links/links.go @@ -244,7 +244,19 @@ func ProjectRewrittenLen(content string, rewrites []BracketRewrite, newTitle, co continue } if bracketUnchanged(content, rw.Position, bracketEnd, newSegment, displaySuffix) { - cursor = bracketEnd + // Deliberately does NOT advance the cursor, mirroring the real pass + // exactly. An earlier version advanced it here, which made the two + // loops disagree about which overlapping rewrites the guard skips: + // on `[[A[[B]]]]` with a no-op at 0 and a change at 3, projection + // reported (10 bytes, 0 applied) while the pass produced 13 bytes + // and applied 1 — so the cascade charged a read it never charged + // the rewrite for, and the bound leaked on exactly the corrupt or + // duplicated offsets the defensive paths exist to handle + // (codex R2). + // + // The pass is the side that must not move: its behaviour is pinned + // to the descending fold the cascade used to perform. So the + // projection follows it, never the reverse. continue } // Length arithmetic only — NOTHING is built here. Concatenating a diff --git a/internal/links/rewrite_brackets_diff_test.go b/internal/links/rewrite_brackets_diff_test.go index e3b96dcd..cad1ede3 100644 --- a/internal/links/rewrite_brackets_diff_test.go +++ b/internal/links/rewrite_brackets_diff_test.go @@ -383,3 +383,105 @@ func TestRewriteBracketsAt_MixedChangedAndUnchangedCountsOnlyTheChanged(t *testi t.Errorf("applied = %d, want 2 — the middle bracket was already [[New]]", applied) } } + +// TestProjectRewrittenLen_IsLockstepWithTheRealPass pins the invariant the cap +// depends on: what the projection PREDICTS must be exactly what the rewrite +// DOES, for every input including the defensive ones. +// +// This is the property codex R2 found broken. The projection advanced its +// cursor past a no-op bracket while the real pass did not, so on corrupt, +// duplicated or overlapping offsets the two disagreed about which rewrites the +// overlap guard skips — projection reporting (unchanged, 0 applied) where the +// pass actually applied one and grew the body. The cascade charges from the +// projection, so the bound's guarantee leaked on precisely the inputs the +// defensive paths exist for. +// +// The corpus deliberately includes NESTED-LOOKING brackets like `[[A[[B]]]]`, +// where two valid `[[` offsets share a closing `]]` and therefore genuinely +// overlap. Random offsets alone rarely produce that shape, which is why it is +// constructed rather than left to chance. +func TestProjectRewrittenLen_IsLockstepWithTheRealPass(t *testing.T) { + rng := rand.New(rand.NewSource(20260902)) + titles := []string{"Old", "old", "A|B", "Other", "A[[B", "B"} + news := []string{"New", "N", "A[[B", strings.Repeat("N", 20), ""} + + overlapping := []string{ + "[[A[[B]]]]", + "[[Old[[Old]]]]", + "x [[A[[B]]]] y", + "[[A[[B]]]] [[Old]]", + } + + check := func(t *testing.T, content string, rewrites []BracketRewrite, newTitle, collSlug string) { + t.Helper() + wantLen, wantApplied := ProjectRewrittenLen(content, rewrites, newTitle, collSlug) + got, gotApplied := RewriteBracketsAt(content, rewrites, newTitle, collSlug) + if len(got) != wantLen || gotApplied != wantApplied { + t.Fatalf("projection and the real pass disagree\n content=%q rewrites=%+v new=%q slug=%q\n"+ + " projected len=%d applied=%d\n actual len=%d applied=%d (result %q)", + content, rewrites, newTitle, collSlug, wantLen, wantApplied, len(got), gotApplied, got) + } + } + + // MIXED target titles per position, not one title for the whole call. + // Cascade rows carry their own target_title (one row may have stored + // "Old" and another "tasks/Old"), and a single shared title is exactly + // what hid this divergence from the first version of this test: the + // reproducing case needs one bracket to be a NO-OP while an overlapping + // one CHANGES, which a single title cannot express. + for _, content := range overlapping { + offs := bracketOffsets(content) + for _, newTitle := range news { + for ti := range titles { + rewrites := make([]BracketRewrite, 0, len(offs)) + for j, p := range offs { + rewrites = append(rewrites, BracketRewrite{ + Position: p, + TargetTitle: titles[(ti+j)%len(titles)], + }) + } + check(t, content, rewrites, newTitle, "") + } + } + } + + // The exact shape codex R2 named, asserted directly so the regression has + // a named home rather than depending on the random search rediscovering it: + // bracket [0,8) is a no-op, bracket [3,8) overlaps it and changes. + check(t, "[[A[[B]]]]", []BracketRewrite{ + {Position: 0, TargetTitle: "A[[B"}, + {Position: 3, TargetTitle: "B"}, + }, "A[[B", "") + + const iterations = 20000 + for i := 0; i < iterations; i++ { + content := randomBracketContent(rng) + if rng.Intn(4) == 0 { + content += overlapping[rng.Intn(len(overlapping))] + } + target := titles[rng.Intn(len(titles))] + newTitle := news[rng.Intn(len(news))] + + positions := bracketOffsets(content) + if rng.Intn(3) == 0 && len(content) > 0 { + positions = append(positions, rng.Intn(len(content))) + } + if rng.Intn(4) == 0 && len(positions) > 0 { + positions = append(positions, positions[rng.Intn(len(positions))]) + } + if len(positions) == 0 { + continue + } + rng.Shuffle(len(positions), func(a, b int) { positions[a], positions[b] = positions[b], positions[a] }) + + rewrites := make([]BracketRewrite, 0, len(positions)) + for j, p := range positions { + tt := target + if rng.Intn(2) == 0 { + tt = titles[(i+j)%len(titles)] + } + rewrites = append(rewrites, BracketRewrite{Position: p, TargetTitle: tt}) + } + check(t, content, rewrites, newTitle, "") + } +} diff --git a/internal/server/handlers_collab.go b/internal/server/handlers_collab.go index 9b2d1c97..3a9c0e37 100644 --- a/internal/server/handlers_collab.go +++ b/internal/server/handlers_collab.go @@ -10,6 +10,7 @@ import ( "github.com/PerpetualSoftware/pad/internal/collab" "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" "github.com/go-chi/chi/v5" "github.com/gorilla/websocket" ) @@ -456,6 +457,28 @@ func (s *Server) applyContentViaCollab(r *http.Request, itemID, markdown string, return collab.ErrRoomActiveDuringPrune } +// isDeterministicWriteFailure reports whether an error from a direct-write +// callback is a settled answer rather than a transient condition. +// +// These three are the errors handleUpdateItem already treats as FINAL at every +// call site: a rejection, a conflict, and a refusal. None can come out +// differently on a retry, so a fallback path that swallows one and retries is +// doing the work twice to reach the same answer — and, worse, may reach it by +// a route that reports it differently. +func isDeterministicWriteFailure(err error) bool { + if err == nil { + return false + } + if _, ok := asOpenChildrenGuardError(err); ok { + return true + } + if _, ok := asUpdateConflictError(err); ok { + return true + } + var tooLarge *store.ItemRenameCascadeTooLargeError + return errors.As(err, &tooLarge) +} + func (s *Server) applyContentViaCollabOnce(r *http.Request, itemID, markdown string, directWrite directWriteFn) error { err := s.collab.ApplyExternalContent(itemID, markdown) switch { @@ -518,6 +541,22 @@ func (s *Server) applyContentViaCollabOnce(r *http.Request, itemID, markdown str // review round 6. return paErr default: + // A DETERMINISTIC failure from directWrite is the caller's answer, + // not a collab routing problem, so it must survive this branch + // (BUG-2804 / codex R2). Returning `err` here discards it and hands + // the caller the original collab error instead, which reads as + // "couldn't route through an applier" — recoverable — so the caller + // falls through to its own direct write and re-derives the identical + // refusal from scratch. Measured: a refused rename ran the whole + // cascade TWICE, 64 rewritten bodies built for one request. + // + // Scoped to errors that cannot come out differently on a second + // attempt. Everything else keeps returning `err`, preserving the + // graceful-degradation contract this branch exists for: a prune + // failure or a transient write fault should still fall through. + if isDeterministicWriteFailure(paErr) { + return paErr + } slog.Warn("collab: failed to prune op-log on direct-write fallback", "item_id", itemID, "error", paErr, diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index 0e87c6dc..fb14d403 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -892,6 +892,37 @@ func (s *Server) handleGetItem(w http.ResponseWriter, r *http.Request) { } // handleUpdateItem updates an existing item (fields, content, or both). + +// writeItemRenameCascadeTooLarge answers a BUG-2804 cascade refusal as 413 and +// reports whether it handled the error. +// +// SHARED because handleUpdateItem reaches store.UpdateItemWithParentLink from +// THREE places — the plain path, the collab-snapshot callback, and the +// collab-edit callback — each with its own error block. The plain path was +// mapped first and the other two were missed, which is a population error, not +// a typo: the sweep that established "bulk and restore never set Title" +// checked other HANDLERS and never asked whether this handler had more than +// one error block (codex R2). +// +// The refusal is DETERMINISTIC and permanent, which is why the collab-edit +// path must treat it as final rather than letting it fall through to the +// direct write: falling through re-runs the whole cascade, re-reads every +// linking body and re-charges the projection, only to refuse identically. The +// caller waits twice for one answer. +func writeItemRenameCascadeTooLarge(w http.ResponseWriter, err error) bool { + var tooLarge *store.ItemRenameCascadeTooLargeError + if !errors.As(err, &tooLarge) { + return false + } + // Composed from TYPED fields, never by splicing err.Error() — every wrapper + // the call path added would otherwise be published to the client. + writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large", + fmt.Sprintf("This rename would rewrite more linked content than the server will process in one "+ + "operation: at least %d bytes, and the limit is %d. Reduce the number of items linking "+ + "this title, or split the rename, and try again.", tooLarge.Processed, tooLarge.Max)) + return true +} + func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { workspaceID, ok := s.getWorkspaceID(w, r) if !ok { @@ -1542,6 +1573,12 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { writeUpdateConflictError(w, itemRefOrSlug(*item), conflict) return } + // BUG-2804 / codex R2: without this the cascade refusal reaches the + // client as a 500 from this path only, while the plain path answers + // 413 for the identical store error. + if writeItemRenameCascadeTooLarge(w, err) { + return + } // Mirror the main UpdateItem path: map UNIQUE constraint / // duplicate key races (e.g. concurrent edits both racing the // invocation_slug partial unique index) to 409 conflict so @@ -1619,6 +1656,12 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { // the same race again. writeUpdateConflictError(w, itemRefOrSlug(*item), conflict) return + } else if writeItemRenameCascadeTooLarge(w, err) { + // FINAL, like the guard and conflict arms above. The refusal is + // deterministic, so the fall-through to the direct write below would + // re-run the entire cascade — every linking body read and projected a + // second time — and refuse identically (codex R2). + return } else if errors.Is(err, collab.ErrApplierAmbiguous) { // BUG-2276 residual 2 (P1, mixed-deploy window): a legacy (non-bracket- // capable) applier was caught by a concurrent version restore and its @@ -1668,12 +1711,7 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { // fields and never by splicing err.Error(), because every wrapper the // call path added on the way up would otherwise be published to the // client. - var cascadeTooLarge *store.ItemRenameCascadeTooLargeError - if errors.As(err, &cascadeTooLarge) { - writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large", - fmt.Sprintf("This rename would rewrite more linked content than the server will process in one "+ - "operation: at least %d bytes, and the limit is %d. Reduce the number of items linking "+ - "this title, or split the rename, and try again.", cascadeTooLarge.Processed, cascadeTooLarge.Max)) + if writeItemRenameCascadeTooLarge(w, err) { return } // Map UNIQUE constraint races (e.g. concurrent updates that both diff --git a/internal/server/handlers_items_rename_cascade_test.go b/internal/server/handlers_items_rename_cascade_test.go index 16cb1774..ea8e6380 100644 --- a/internal/server/handlers_items_rename_cascade_test.go +++ b/internal/server/handlers_items_rename_cascade_test.go @@ -144,3 +144,125 @@ func TestItemRename_OrdinaryCascadeStillSucceeds(t *testing.T) { t.Errorf("found %d rewritten linkers, want 3: the cascade did not run", n) } } + +// TestItemRenameCascadeTooLarge_MappedOnTheCollabSnapshotPath closes codex R2's +// second finding. +// +// handleUpdateItem reaches store.UpdateItemWithParentLink from THREE places, +// each with its own error block: the plain path, the collab-snapshot callback, +// and the collab-edit callback. R1 mapped the first. This pins the second, on +// which the identical store refusal was still answering 500. +// +// Why the miss is worth naming: the CONVE-18 sweep I ran after R1 asked "do +// other HANDLERS reach this?" and correctly answered no. It never asked +// "does THIS handler reach it more than once?" — so the population was scoped +// to the wrong axis, and a grep for the store call inside one function would +// have found all three immediately. +func TestItemRenameCascadeTooLarge_MappedOnTheCollabSnapshotPath(t *testing.T) { + srv := testServerWithCollab(t) + slug := createWSWithCollections(t, srv) + + rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{ + "title": "Old", + }) + if rr.Code != http.StatusCreated { + t.Fatalf("create target: %d: %s", rr.Code, rr.Body.String()) + } + var target models.Item + parseJSON(t, rr, &target) + + const body = 1 << 20 + perLinker := int64(body) * 2 + linkers := int(store.MaxItemRenameCascadeBytes/perLinker) + 2 + linkerBody := "[[Old]]" + strings.Repeat("x", body-len("[[Old]]")) + for i := 0; i < linkers; i++ { + rr = doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{ + "title": fmt.Sprintf("Linker %d", i), + "content": linkerBody, + }) + if rr.Code != http.StatusCreated { + t.Fatalf("create linker %d: %d: %s", i, rr.Code, rr.Body.String()) + } + } + + // ?source=collab-snapshot with content routes through the collab-snapshot + // callback rather than the plain path. + rr = doRequest(srv, "PATCH", + "/api/v1/workspaces/"+slug+"/items/"+target.Slug+"?source=collab-snapshot", + map[string]interface{}{ + "title": "New", + "content": "rewritten by the editor", + "op_log_cursor": 0, + }) + + if rr.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("collab-snapshot path: got %d, want 413 — the same store refusal the plain path "+ + "answers 413 for: %s", rr.Code, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "rename_cascade_too_large") { + t.Errorf("response lacks the error code: %s", rr.Body.String()) + } +} + +// TestItemRenameCascadeTooLarge_CollabEditPathDoesNotRunTheCascadeTwice closes +// the other half of codex R2's second finding. +// +// The collab-edit path treats most callback errors as recoverable and falls +// through to a direct write — graceful degradation for applier timeouts. A +// cascade refusal is NOT recoverable: it is deterministic, so the fall-through +// re-reads every linking body, re-charges the projection, and refuses +// identically. The caller waits twice for one answer. +// +// STATUS CANNOT PIN THIS. Both behaviours end in 413 — the fall-through reaches +// the plain path's arm — so a status assertion passes either way. The +// observable that discriminates is how much WORK the cascade did, counted via +// the store's build observer. +func TestItemRenameCascadeTooLarge_CollabEditPathDoesNotRunTheCascadeTwice(t *testing.T) { + srv := testServerWithCollab(t) + slug := createWSWithCollections(t, srv) + + rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{ + "title": "Old", + }) + if rr.Code != http.StatusCreated { + t.Fatalf("create target: %d: %s", rr.Code, rr.Body.String()) + } + var target models.Item + parseJSON(t, rr, &target) + + const body = 1 << 20 + perLinker := int64(body) * 2 + admits := int(store.MaxItemRenameCascadeBytes / perLinker) + linkers := admits + 2 + linkerBody := "[[Old]]" + strings.Repeat("x", body-len("[[Old]]")) + for i := 0; i < linkers; i++ { + rr = doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{ + "title": fmt.Sprintf("Linker %d", i), + "content": linkerBody, + }) + if rr.Code != http.StatusCreated { + t.Fatalf("create linker %d: %d: %s", i, rr.Code, rr.Body.String()) + } + } + + var built int + srv.store.SetCascadeBuildObserver(func(int) { built++ }) + defer srv.store.SetCascadeBuildObserver(nil) + + // Title + content with collab enabled and no collab-snapshot marker routes + // through the collab-edit callback. + rr = doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/items/"+target.Slug, map[string]interface{}{ + "title": "New", + "content": "rewritten by the editor", + }) + + if rr.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("collab-edit path: got %d, want 413: %s", rr.Code, rr.Body.String()) + } + if built != admits { + t.Errorf("cascade built %d bodies for ONE request, want %d — a second full cascade ran, "+ + "which means the deterministic refusal fell through to the direct write and was "+ + "re-derived from scratch", built, admits) + } + t.Logf("built %d bodies for one refused request (cap admits %d sources)", built, admits) +} diff --git a/internal/store/wiki_links.go b/internal/store/wiki_links.go index 79aac961..7a8fa222 100644 --- a/internal/store/wiki_links.go +++ b/internal/store/wiki_links.go @@ -1575,6 +1575,23 @@ func snippetAround(content string, position int) string { return snippet } +// SetCascadeBuildObserver installs a TEST-SUPPORT observer called once per +// rewritten body the item rename cascade BUILDS, with that body's length. Pass +// nil to clear. +// +// Exported for the same reason SetCollabRoomManager is: the property it makes +// observable — how much work a single request causes the cascade to do — is +// only reachable from the SERVER package, whose tests cannot touch this +// package's unexported fields. It is the instrument behind the collab-edit +// double-work test (codex R2), which asserts that a refused rename runs the +// cascade ONCE rather than falling through and running it again. +// +// Not safe for concurrent use with in-flight renames; tests set it before +// issuing requests and clear it after. +func (s *Store) SetCascadeBuildObserver(fn func(bytes int)) { + s.onItemCascadeBodyBuilt = fn +} + // buildCascadeBody builds one rewritten body and reports the build through the // test seam, as ONE operation. //