fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) - #1218
Merged
Merged
Conversation
…ected 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
…e cascade's LIKE pattern (BUG-2798) 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
…s, order the typed check first (BUG-2798) 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 b09aca1. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…or the wrong reason (BUG-2798) 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
… err.Error() (BUG-2798) 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
…es not (BUG-2798) 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
…r cannot touch (BUG-2798) 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
…used (BUG-2798) 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
…unt retry buffers (BUG-2798) 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
…s it (BUG-2798) 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
…-lock read (BUG-2798) 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
…2798) 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
…BUG-2798) 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
Codex round 15 caught a claim of mine that was simply false. Reverting the functional half of c00606b. 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes BUG-2798 and BUG-2796.
A document rename rewrites
[[oldTitle]]into every linking document, and the cascade holds every read and rewritten body in memory before writing any of them. Neither factor was bounded: titles had no length validation, and nothing bounded the cascade. One rename could project 10 GB from a 500 KB input — 20,000x, measured — and OOM while holding the workspace rename lock.The fix
1. Title length, at write time —
models.MaxDocumentTitleRunes = 255, on create and rename. Runes, not bytes. Grandfathered: a title only has to be valid when it changes, so existing documents stay editable.2. The cascade's retained total —
store.MaxRenameCascadeRetainedBytes = 32 MiB, accumulated across the linking set and refused before the first rewrite is built. Refused as a permanent 413 with the projection, deliberately outside the retryableErrLinkCascadeContentionfamily.3. BUG-2796 folds in at the same door — a title containing wiki-link syntax was emitted raw, so renaming to
A]] [[Aproduced two broken links and reported success.Why retained, and why the total
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 aggregate is linear in the number of linkers (108.6 / 217.3 / 434.5 MB at k = 1/2/4). A per-document cap of C still admits k × C — the same unbounded shape one level up.
Output alone is also the wrong quantity: a rename to a shorter title projects ~40 KiB per 2 MiB linker while still holding every read body for the compare-and-set. The guard counts both strings.
The 32 MiB figure has a receipt in the constant's doc comment, including the inference it rests on: this instance's
documentstable is empty, so the ceiling is measured onitems(2,949 wiki-linking rows, 10,077,476 bytes) and used as a proxy.Review
Eleven Codex rounds; every one found something, which is why convergence was not declared early. Rounds found, and this PR fixes:
LIKEterm was unescaped (\is Postgres's escape character and not SQLite's — a backslash title silently failed to cascade on one dialect)err.Error(), publishing internal wrappingLIKEis case-insensitive, so case variants were charged to the caller's budgetFiled rather than folded (verified in code first): BUG-2804 and BUG-2805 (the item-side cascade carries the same unbounded shape and escaping gap, on the live surface), BUG-2806 (existing documents' escaped links are still orphaned by a rename).
Verification
Twenty-two mutations, each detected by exactly the test that should catch it — including the two that discriminate design decisions rather than code: a per-document guard fails only the total test, and
max(read, rewritten)fails only the sum test.Four store tests are Postgres-only and skip loudly rather than passing on a DSN property; the case-variant test runs on both dialects on purpose, asserting Postgres was already correct.
Gates on every round:
go test ./...under Postgres 17 EXIT=0,gofmtclean,make lint0 issues.https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN