perf: bound docscheck's code-span scan

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,24 +848,63 @@ 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
 }
 
 // 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

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 "<!--" inside one would open a
+// comment that swallowed the rest of the document if the span were missed
+// -- and the run lengths have to match across the line break exactly as
+// they do within a line.
+func TestCodeSpanClosesOnALaterLine(t *testing.T) {
+	cases := []struct {
+		name  string
+		lines []string
+		close bool
+	}{
+		{"equal runs close across lines", []string{"``x", "x``"}, true},
+		{"longer run on the later line does not", []string{"``x", "x```"}, false},
+		{"shorter run on the later line does not", []string{"``x", "x`"}, false},
+		{"a blank line ends the paragraph first", []string{"``x", "", "x``"}, false},
+	}
+	for _, test := range cases {
+		t.Run(test.name, func(t *testing.T) {
+			_, _, ok := codeSpanEnd(test.lines, 0, 0)
+			if ok != test.close {
+				t.Fatalf("codeSpanEnd(%q) closed = %v, want %v", test.lines, ok, test.close)
+			}
+		})
+	}
+}
+
+// TestCodeSpanBoundLeavesRealSpansIntact is the other half: the ceiling
+// must be unreachable by anything a document would legitimately contain.
+// A span just under the bound still closes; one past it is left as literal
+// text, which is exactly what an unmatched run already becomes.
+func TestCodeSpanBoundLeavesRealSpansIntact(t *testing.T) {
+	t.Run("span just inside the bound closes", func(t *testing.T) {
+		line := "`" + strings.Repeat("x", maxCodeSpanScanBytes-16) + "`"
+		if _, _, ok := codeSpanEnd([]string{line}, 0, 0); !ok {
+			t.Fatal("a span within the bound must still close")
+		}
+	})
+
+	t.Run("span past the bound is literal text", func(t *testing.T) {
+		line := "`" + strings.Repeat("x", maxCodeSpanScanBytes+16) + "`"
+		if _, _, ok := codeSpanEnd([]string{line}, 0, 0); ok {
+			t.Fatal("a span past the bound must be left unclosed")
+		}
+	})
+
+}