diff --git a/internal/docscheck/docscheck.go b/internal/docscheck/docscheck.go index 9fa1f2a..dd79963 100644 --- a/internal/docscheck/docscheck.go +++ b/internal/docscheck/docscheck.go @@ -32,12 +32,14 @@ // live Markdown. A repository path or a link quoted inside such a // span is therefore never checked. No multi-line span appears in // this repository's documentation today. -// - A backtick run that never closes is scanned to the end of its -// paragraph, so a paragraph built entirely of such runs costs more -// than linear time. Checking this repository's real tree takes a few -// tens of milliseconds, and the growth starts to matter only for a -// crafted document orders of magnitude larger than the tree itself. -// Bounding the scan is tracked separately. +// - A backtick run longer than maxCodeSpanScanBytes, or whose search +// for its closing run runs that far, is left as literal text. That is +// what an unmatched run already becomes, so the ceiling changes no +// behavior a real document can observe: it takes a span longer than +// the bound to reach one, and the longest in this repository is two +// orders of magnitude below it. Without the ceiling, a paragraph +// built entirely of never-closing runs costs superlinear time, +// because each run rescans the rest of the paragraph. // - Indented (non-fenced) code blocks are not treated as examples. Every // example here uses a fence. // - The checks trust the working tree's file identity, not only its path @@ -75,6 +77,34 @@ const ( releaseWorkflowFile = ".github/workflows/release.yml" ) +// maxCodeSpanScanBytes bounds both how long a backtick run may be and how +// far it may search for the run that closes it. Beyond the bound the run +// is left as literal text — the same outcome an unmatched run already +// produces, so nothing a real document can express changes: reaching the +// ceiling needs a single code span longer than 8 KiB, where the longest +// in this repository is under a hundred bytes. +// +// The bound is a ceiling on each of the three reads one call makes rather +// than on their sum: the opening run, the search for a closing one, and +// the measurement of the candidate that ends that search. A call reads +// three times the ceiling in the worst case — delimiters at the ceiling +// with the search between them running its full length — which is a +// constant, not the unbounded quantity the ceiling was added to remove. +// +// That last measurement is not charged before it is accepted, because a +// closing run can only be told from a longer one by reading one byte past +// the opening's length. Refusing to spend a byte the budget no longer +// covers would leave the two indistinguishable, and would turn a closing +// run landing on the last of the budget into literal text. +// +// The ceiling exists because the search is per run: a paragraph of runs +// that never close makes each one rescan everything after it. Measured +// before the bound, a paragraph of runs of increasing length cost 163 ms +// at 20 KB, 1.8 s at 81 KB, and 26.7 s at 321 KB — a shape that would +// spend a CI job's whole budget inside one paragraph and surface as a +// timeout rather than as a named failure. +const maxCodeSpanScanBytes = 8 << 10 + // selfCheckStep is the workflow step whose command list AGENTS.md declares // authoritative. const selfCheckStep = "Self-check fixture documents" @@ -797,8 +827,18 @@ func codeSpanEnd(lines []string, index, position int) (endIndex, endPosition int if position >= len(lines[index]) || lines[index][position] != '`' { return 0, 0, false } - opening := leadingRun(lines[index][position:], '`') + // The opening run is measured under the same ceiling as the search + // that follows it, and a run past the ceiling is left as literal + // text. Without that, a pair of oversized delimiters would close a + // span the ceiling is meant to have given up on -- two runs of 8 KiB + // and one byte between them inspect 16 KiB -- and every measurement + // below would be free to read a run that long. + opening := leadingRunAtMost(lines[index][position:], '`', maxCodeSpanScanBytes+1) + if opening > maxCodeSpanScanBytes { + return 0, 0, false + } scan := position + opening + budget := maxCodeSpanScanBytes for cursor := index; cursor < len(lines); cursor++ { if cursor > index { if strings.TrimSpace(lines[cursor]) == "" { @@ -808,15 +848,44 @@ func codeSpanEnd(lines []string, index, position int) (endIndex, endPosition int } line := lines[cursor] for scan < len(line) { + if budget <= 0 { + return 0, 0, false + } if line[scan] != '`' { scan++ + budget-- continue } - run := leadingRun(line[scan:], '`') + // Measuring only far enough to tell an exact match from a + // longer run is what keeps one oversized run inside the + // budget: reading the run whole would spend its full length + // before the check above ever saw the cost, and every run + // that precedes it repeats that read. + // + // The limit is not clamped to the remaining budget. Doing so + // would leave a run of exactly opening backticks + // indistinguishable from a longer one, and resolving that + // ambiguity as a non-match turns a closing run landing on + // the last of the budget into literal text. Reading one past + // opening costs at most the opening run's own length, once + // per call, against a budget the next iteration is about to + // find spent. + run := leadingRunAtMost(line[scan:], '`', opening+1) if run == opening { return cursor, scan + opening, true } scan += run + budget -= run + // A measurement that stopped at the limit may have stopped + // inside the run rather than at its end, so the remainder is + // stepped over here. Resuming the search at that offset + // instead would read what is left as a run of its own and + // could take its tail for the closing one: ``x````` would + // close on the last two backticks of a run of five. + for budget > 0 && scan < len(line) && line[scan] == '`' { + scan++ + budget-- + } } } return 0, 0, false @@ -824,8 +893,18 @@ func codeSpanEnd(lines []string, index, position int) (endIndex, endPosition int // leadingRun counts the leading repetitions of char in value. func leadingRun(value string, char byte) int { + return leadingRunAtMost(value, char, len(value)) +} + +// leadingRunAtMost counts the leading repetitions of char in value, +// stopping once it has counted limit of them. A caller that only needs to +// tell one exact length from every greater one then pays for the length +// it asked about rather than for the run it was handed, but it gets back +// a count that may sit inside a run rather than at its end, and has to +// step over the remainder itself. +func leadingRunAtMost(value string, char byte, limit int) int { run := 0 - for run < len(value) && value[run] == char { + for run < limit && run < len(value) && value[run] == char { run++ } return run diff --git a/internal/docscheck/docscheck_test.go b/internal/docscheck/docscheck_test.go index dc9cfbe..353f739 100644 --- a/internal/docscheck/docscheck_test.go +++ b/internal/docscheck/docscheck_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" "testing/fstest" + "time" ) // The fixture repository is a miniature of this one: the same file @@ -849,3 +850,235 @@ func TestDocumentFilesSkipsFixturesAndToolDirectories(t *testing.T) { t.Fatal("document set is empty") } } + +// TestCodeSpanScanIsBounded is the regression test for the cost curve that +// motivated maxCodeSpanScanBytes. A paragraph of backtick runs that never +// close makes each run search everything after it, so the work grows with +// the cube of the run count. +// +// A wall-clock assertion is ordinarily a poor shape for a test, and is +// used here because the defect was a growth rate rather than a wrong +// answer. The size was chosen by measuring both regimes rather than +// guessing: at 3000 runs the unbounded scan takes 4.4 s, which a five +// second ceiling does not reliably separate from anything — an earlier +// version of this test passed with the bound removed. At 6000 runs the +// same input costs 61 ms bounded and 35 s unbounded, so the ceiling below +// sits eighty times above the passing time and seven times beneath the +// failing one. +// +// Scanning starts at each run rather than at each backtick, which is how +// readCodeSpan actually calls it. +func TestCodeSpanScanIsBounded(t *testing.T) { + var builder strings.Builder + for run := 1; run <= 6000; run++ { + builder.WriteString(strings.Repeat("`", run)) + builder.WriteString("x") + } + line := builder.String() + lines := []string{line} + + start := time.Now() + for index := 0; index < len(line); { + if line[index] != '`' { + index++ + continue + } + codeSpanEnd(lines, 0, index) + index += leadingRun(line[index:], '`') + } + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("scanning %d bytes of unclosed runs took %v", len(line), elapsed) + } +} + +// TestCodeSpanScanBoundsOversizedCandidateRun covers the way a single run +// used to escape maxCodeSpanScanBytes. The budget was spent per byte +// stepped over, but the candidate run a scan landed on was measured whole +// before that accounting ran, so one enormous run cost its full length no +// matter how little budget was left. +// +// Runs of strictly increasing length never match each other, so every run +// in the prefix below travels the whole way to the oversized run and pays +// for it again. The prefix is sized to sit just under the budget, which +// is what keeps all of it in play: 125 runs reach the 64 MiB run, and the +// unbounded measurement reads it 125 times. +// +// The wall-clock assertion carries the same caveat as +// TestCodeSpanScanIsBounded, and the same measurement behind it. Bounded, +// this input takes 79 ms — one unavoidable pass, made when splitCodeSpans +// reaches the oversized run and measures it as an opening run of its own. +// Unbounded it takes 4.5 s, so the ceiling below sits twelve times above +// the passing time and four times beneath the failing one. The race +// detector does not move the passing time, which is a single tight loop +// over bytes nothing else touches. +func TestCodeSpanScanBoundsOversizedCandidateRun(t *testing.T) { + var builder strings.Builder + for run := 1; run <= 125; run++ { + builder.WriteString(strings.Repeat("`", run)) + builder.WriteString("x") + } + if builder.Len() >= maxCodeSpanScanBytes { + t.Fatalf("prefix of %d bytes must stay under the %d byte budget", + builder.Len(), maxCodeSpanScanBytes) + } + builder.WriteString(strings.Repeat("`", 1<<26)) + line := builder.String() + lines := []string{line} + + start := time.Now() + for index := 0; index < len(line); { + if line[index] != '`' { + index++ + continue + } + codeSpanEnd(lines, 0, index) + index += leadingRun(line[index:], '`') + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("scanning %d bytes ending in an oversized run took %v", len(line), elapsed) + } +} + +// TestCodeSpanRunLengthsMustMatchExactly pins the distinction the bounded +// measurement has to preserve. Measuring a candidate run only far enough +// to tell it from an exact match means the closing rule is enforced by a +// length limit rather than by a full count, so a run one backtick too +// long has to stay a non-match. +// +// The last three cases guard the trap that shape sets. A measurement that +// stops at the limit stops inside the run, and a search that resumed +// there would read the remainder as a run of its own: a run of five +// backticks would close a two-backtick opening on its last two, silently +// pulling whatever lay between into a code span and out of the checks. +func TestCodeSpanRunLengthsMustMatchExactly(t *testing.T) { + cases := []struct { + name string + line string + close bool + }{ + {"equal runs close", "``x``", true}, + {"longer candidate does not close", "``x```", false}, + {"shorter candidate does not close", "``x`", false}, + {"longer candidate before an equal one", "`x``x`", true}, + {"run far longer than the opening", "``x`````", false}, + {"run three times the opening", "`x```", false}, + {"remainder of a long run is not a new run", "```x```````", false}, + } + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + _, _, ok := codeSpanEnd([]string{test.line}, 0, 0) + if ok != test.close { + t.Fatalf("codeSpanEnd(%q) closed = %v, want %v", test.line, ok, test.close) + } + }) + } +} + +// TestCodeSpanClosesOnTheLastOfTheBudget pins the other edge of the +// bounded measurement. A closing run that lands exactly where the budget +// runs out is still a closing run, so the measurement reads one backtick +// past the opening length even when the budget cannot cover it: a +// candidate capped at the budget would be indistinguishable from a longer +// run, and calling that unmatched would turn a legitimate span into +// literal text a few bytes short of the documented ceiling. +// +// The second case is why the first cannot simply be waved through. At the +// same offset, a run longer than the opening must still fail to close. +func TestCodeSpanClosesOnTheLastOfTheBudget(t *testing.T) { + const opening = 3 + filler := strings.Repeat("x", maxCodeSpanScanBytes-opening) + run := strings.Repeat("`", opening) + + t.Run("an exact run on the last of the budget closes", func(t *testing.T) { + line := run + filler + run + if _, _, ok := codeSpanEnd([]string{line}, 0, 0); !ok { + t.Fatal("a closing run reached within the budget must still close") + } + }) + + t.Run("a longer run on the last of the budget does not", func(t *testing.T) { + line := run + filler + run + "``" + if _, _, ok := codeSpanEnd([]string{line}, 0, 0); ok { + t.Fatal("a run longer than the opening must never close it") + } + }) +} + +// TestCodeSpanDelimitersObeyTheBound pins the ceiling against the runs +// themselves rather than against the distance between them. Two runs of +// equal length with a byte between them close a span whatever their +// length, so measuring the opening run without a ceiling would let a pair +// of 8 KiB delimiters inspect 16 KiB -- a span the bound is meant to have +// given up on. The opening run is measured under the ceiling too, and one +// past it is literal text. +func TestCodeSpanDelimitersObeyTheBound(t *testing.T) { + closes := func(length int) bool { + run := strings.Repeat("`", length) + _, _, ok := codeSpanEnd([]string{run + "x" + run}, 0, 0) + return ok + } + if !closes(maxCodeSpanScanBytes) { + t.Error("delimiters at the ceiling must still close") + } + if closes(maxCodeSpanScanBytes + 1) { + t.Error("delimiters past the ceiling must be literal text") + } + + // The worst case the const comment names: delimiters at the ceiling + // with the search between them running its full length, so the call + // reads three times the ceiling. It closes, which is what makes the + // figure reachable rather than hypothetical. + widest := strings.Repeat("`", maxCodeSpanScanBytes) + line := widest + strings.Repeat("x", maxCodeSpanScanBytes-1) + widest + if _, _, ok := codeSpanEnd([]string{line}, 0, 0); !ok { + t.Error("delimiters at the ceiling a full search apart must close") + } +} + +// TestCodeSpanClosesOnALaterLine covers the multi-line path through the +// same measurement. A span whose closing run sits on a later line is the +// case codeSpanEnd exists for -- a quoted "