From b4627d785778b80f474b8cded49910f60dc25cab Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Tue, 8 Sep 2026 23:19:10 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=9A=80=EF=BC=9Aread?= =?UTF-8?q?=20a=20message=20without=20segmenting=20every=20line?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commit queue reads a message assembled from whatever a pull request's commits say, while holding a token that can merge, so how long that can be made to take is a security property. Issue #906 reported the reading as super-linear. Measured again one call to a process, which is how the queue does it, it is linear at every size -- but 25 to 30 times dearer than it needs to be, and it allocates enough to be killed for it: 800,000 lines took 674 MB, and on a smaller heap the process simply died. That is the denial of service the issue is about, reached by weight rather than by exponent. Four things were paying for it, all of them per line: - Every line was segmented into graphemes to measure its width. No grapheme is fewer than one code unit, so a line of 72 code units or fewer is within the limit whatever it holds and the segmenter need never see it. This was the bulk of the cost. - Seven regexes were compiled for each line of the last paragraph, which a message can make the whole message. Built once now. - The trailer block was found by rebuilding the message from its lines and splitting it all over again, with the paragraphs already in hand. - Splitting into paragraphs joined the lines and split the result twice more. It walks the lines it is given instead. Reading 200,000 lines now costs 2 to 6 times looking at every line once, against 33 to 124 before, and 800,000 lines fit in 183 MB. The tests are rebuilt, because the pair that claimed to guard this could not. They asserted a wall-clock budget at a single size, which cannot tell linear from quadratic, and which went red twice on a loaded runner over code that was fine. Each is now a ratio between two measurements taken moments apart on the same machine, one of them work known to be linear, so a slow or busy machine raises both and leaves the ratio where it was. The floor is written out in the test rather than borrowed from the module, since sharing it would let a change that made splitting quadratic raise the floor by as much as the measurement and go unseen. They were checked against the code they have to reject: the reading this replaces fails three of them, restoring the regex that strips the trailing newlines fails two at over a thousand times the floor, and dropping the `^` from the Assisted-by pattern fails the fourth at three thousand times its control. Ten runs on a machine carrying four times as many spinning processes as it has cores produced no failures. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 Fixes: https://github.com/OpenINF/.github/issues/906 --- build/shared/commit-message.mts | 88 +++++++++---- build/shared/commit-message.test.mts | 186 ++++++++++++++++++++++++--- 2 files changed, 229 insertions(+), 45 deletions(-) diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts index b35f5d00..1ee7b90e 100644 --- a/build/shared/commit-message.mts +++ b/build/shared/commit-message.mts @@ -122,6 +122,16 @@ const ASSISTED_BY_VALUE = /^[^\s:]+:\S+( \S+)*$/; /** git folds a trailer whose value runs onto an indented line beneath it. */ const CONTINUATION_LINE = /^\s/; +/** + * Each known token with the pattern that also matches it misspelt with a + * space where a hyphen belongs. Built here because the check that uses them + * runs for every line of the last paragraph. + */ +const SPACED_TRAILER_TOKENS = TRAILER_ORDER.map((token) => ({ + token, + spaced: new RegExp(`^${token.replaceAll('-', '[ -]')}:`, 'i'), +})); + /** * Splits a commit message into its lines, without the blank ones git leaves * at the end. Written as a scan rather than as `/[\r\n]+$/`, which takes time @@ -146,29 +156,42 @@ export function linesOf(message: string) { } /** - * Splits a message body into paragraphs of non-empty lines. + * Splits a message body into paragraphs of non-empty lines. Walks the lines + * it is given, so nothing copies the message. * @param {string[]} lines Every line after the subject. * @returns {string[][]} The paragraphs, in order. */ -const paragraphsOf = (lines: string[]) => - lines - .join('\n') - .split(/\n{2,}/) - .map((paragraph) => paragraph.split('\n').filter(Boolean)) - .filter((paragraph) => paragraph.length > 0); +const paragraphsOf = (lines: string[]) => { + const paragraphs: string[][] = []; + let paragraph: string[] = []; + + for (const line of lines) { + if (line === '') { + if (paragraph.length > 0) { + paragraphs.push(paragraph); + paragraph = []; + } + } else { + paragraph.push(line); + } + } + + if (paragraph.length > 0) paragraphs.push(paragraph); + + return paragraphs; +}; /** - * Reads the trailer block out of a message, agreeing with git about whether - * there is one: the last paragraph, every line of it either a trailer or a - * continuation of the one above, and the first of them a trailer. A paragraph - * that merely contains a colon somewhere is prose, and git reads no trailers - * in it -- so neither does this. - * @param {string} message The whole commit message. + * Reads the trailer block out of a message's paragraphs, agreeing with git + * about whether there is one: the last paragraph, every line of it either a + * trailer or a continuation of the one above, and the first of them a + * trailer. A paragraph that merely contains a colon somewhere is prose, and + * git reads no trailers in it -- so neither does this. + * @param {string[][]} paragraphs The paragraphs after the subject. * @returns {string[]} The trailer lines, one per trailer, empty if there is no block. */ -export function readTrailers(message: string) { - const [, ...rest] = linesOf(message); - const last = paragraphsOf(rest).at(-1) ?? []; +const trailerBlockOf = (paragraphs: string[][]) => { + const last = paragraphs.at(-1) ?? []; const isBlock = last.length > 0 && TRAILER_LINE.test(last[0] ?? '') && @@ -177,6 +200,17 @@ export function readTrailers(message: string) { ); return isBlock ? last.filter((line) => !CONTINUATION_LINE.test(line)) : []; +}; + +/** + * Reads the trailer block out of a whole message. + * @param {string} message The whole commit message. + * @returns {string[]} The trailer lines, one per trailer, empty if there is no block. + */ +export function readTrailers(message: string) { + const [, ...rest] = linesOf(message); + + return trailerBlockOf(paragraphsOf(rest)); } /** @@ -276,9 +310,7 @@ const checkTrailers = (lines: string[]) => { // known tokens with their hyphens loosened, since a looser test than that // flags any body sentence containing a colon. for (const line of last) { - for (const token of TRAILER_ORDER) { - const spaced = new RegExp(`^${token.replaceAll('-', '[ -]')}:`, 'i'); - + for (const { token, spaced } of SPACED_TRAILER_TOKENS) { if ( spaced.test(line) && !line.toLowerCase().startsWith(`${token.toLowerCase()}:`) @@ -306,7 +338,7 @@ const checkTrailers = (lines: string[]) => { } const tokens: string[] = []; - const block = readTrailers(['', ...lines].join('\n')); + const block = trailerBlockOf(paragraphs); // A last paragraph that is not a clean block is prose, and git reads no // trailers in it. Saying so is only worth doing for a line that was plainly @@ -403,10 +435,18 @@ export function validateCommitMessage(message: string) { // An unbreakable line -- a URL, near enough always -- cannot be wrapped, // and reflowing one to fit would break it. - if (countGraphemes(line) > BODY_MAX && /\s/.test(line.trim())) { - problems.push( - `line is ${countGraphemes(line)} characters; the limit is ${BODY_MAX}: “${line.slice(0, 40)}…”` - ); + // + // No grapheme is fewer than one code unit, so a line of BODY_MAX code + // units or fewer is within the limit whatever it is made of, and the + // segmenter need not see it. + if (line.length > BODY_MAX && /\s/.test(line.trim())) { + const width = countGraphemes(line); + + if (width > BODY_MAX) { + problems.push( + `line is ${width} characters; the limit is ${BODY_MAX}: “${line.slice(0, 40)}…”` + ); + } } } diff --git a/build/shared/commit-message.test.mts b/build/shared/commit-message.test.mts index 552ceb74..374332d0 100644 --- a/build/shared/commit-message.test.mts +++ b/build/shared/commit-message.test.mts @@ -368,35 +368,179 @@ describe('the vocabulary', () => { describe('a message written to be slow', () => { // A commit message comes from whoever opened the pull request, and the - // commit queue reads it holding credentials that can write here. Taking - // time proportional to the square of its length is a way to stop the queue - // working, so these hold the rules to reading it in linear time. - // Generous, because the work itself is linear and CI is slow. What it - // separates is linear from quadratic: at this size the regexes these - // replaced took twenty seconds and ninety seconds respectively. - const budget = 3000; + // commit queue reads it holding credentials that can write here. How long + // it can be made to take is therefore a security property, and these hold + // the rules to it. + // + // Each is a ratio between two measurements taken on the same machine + // moments apart, one of them work known to be linear. A machine that is + // slow, or busy, raises both and leaves the ratio alone, where a figure in + // milliseconds would measure the machine as much as the code. + + /** Timings to take for each measurement, of which the fastest is kept. */ + const runs = 5; + + /** Times to measure both sides of a ratio, alternating between them. */ + const rounds = 3; + + /** The size of the messages measured, in lines or in repetitions. */ + const size = 100_000; + + /** + * How long something takes, at its fastest out of a few goes. + * + * The fastest, because everything that happens to a timing on a shared + * machine -- losing the processor, waiting on a collection, sharing a core + * -- adds to it and nothing takes away. The smallest of several is the + * closest to what the work itself costs. + * @param {() => unknown} work What to measure. + * @returns {number} The smallest of `runs` timings, in milliseconds. + */ + const spentOn = (work: () => unknown) => { + let fastest = Infinity; + + for (let run = 0; run < runs; run += 1) { + const started = performance.now(); + + work(); + + fastest = Math.min(fastest, performance.now() - started); + } - test('reads a long run of newlines quickly', () => { - const message = `🏗️🔧：fix it\n\nA body.${'\n'.repeat(200_000)}x`; - const started = performance.now(); + return fastest; + }; + + /** + * What one piece of work costs in multiples of another. + * @param {() => unknown} measured The work being judged. + * @param {() => unknown} baseline The work it is judged against. + * @returns {number} The multiple. + */ + const ratioOf = (measured: () => unknown, baseline: () => unknown) => { + // Leaves both compiled before either is measured. + measured(); + baseline(); + + let cost = Infinity; + let floor = Infinity; + + // Alternated, so that a machine getting busier partway through raises + // both sides. + for (let round = 0; round < rounds; round += 1) { + floor = Math.min(floor, spentOn(baseline)); + cost = Math.min(cost, spentOn(measured)); + } - validateCommitMessage(message); + return cost / floor; + }; + + /** + * The floor: split a message into lines and look at each one once. Reading + * a message cannot cost less than this, so it stands in for how fast this + * machine is at the work in question. + * + * Written out here rather than taken from the module, so that a change to + * the module's own splitting moves the measurement without moving the + * floor. + * @param {string} message The message to read. + * @returns {number} How many lines were a run of dashes, which is beside the point. + */ + const readEveryLine = (message: string) => { + let seen = 0; + + for (const line of message.split(/\r?\n/)) { + if (/^-{3,}$/.test(line)) seen += 1; + } - const spent = performance.now() - started; + return seen; + }; + + /** + * What reading a message costs against looking at every line of it once. + * @param {string} message The message to read. + * @returns {number} The multiple. + */ + const costOfReading = (message: string) => + ratioOf( + () => validateCommitMessage(message), + () => readEveryLine(message) + ); - ok(spent < budget, `took ${spent.toFixed(0)}ms, budget ${budget}ms`); + /** + * A message of blank lines gives the reading almost nothing to do beyond + * splitting, so its cost sits just above the floor and anything spent per + * line shows at once. Costs about 1, and 9 with a stray second pass over + * the lines. + */ + const blankLineCeiling = 5; + + /** + * A message that is all one paragraph is trailer candidates from top to + * bottom, so the reading has real work to do and the ratio is both higher + * and looser under load. A bound on the whole rather than a fine measure. + */ + const paragraphCeiling = 30; + + test('reads a body of many lines without looking at each one twice', () => { + const message = [ + '🏗️🔧：fix it', + '', + ...Array.from({ length: size }, () => 'x'), + ].join('\n'); + const factor = costOfReading(message); + + ok( + factor < paragraphCeiling, + `reading cost ${factor.toFixed(1)}x looking at every line, ceiling ${paragraphCeiling}x` + ); }); - test('reads a long Assisted-by value quickly', () => { - // `\S` matches a colon, so the obvious spelling of agent:model lets the - // engine try every colon as the split point. - const message = `🏗️🔧：fix it\n\nAssisted-by: ${'a:'.repeat(100_000)} `; - const started = performance.now(); + test('reads a long run of blank lines without segmenting each one', () => { + const message = `🏗️🔧：fix it\n\nA body.${'\n'.repeat(size)}x`; + const factor = costOfReading(message); - validateCommitMessage(message); + ok( + factor < blankLineCeiling, + `reading cost ${factor.toFixed(1)}x looking at every line, ceiling ${blankLineCeiling}x` + ); + }); - const spent = performance.now() - started; + test('strips a trailing run of newlines without backtracking', () => { + // A run of newlines ending on something else is what a regex written to + // strip it backtracks over; `linesOf` scans. The floor splits the same + // message without that cost, so such a change shows here. + const message = `🏗️🔧：fix it\n\nA body.${'\n'.repeat(size)}\nend`; + const factor = costOfReading(message); - ok(spent < budget, `took ${spent.toFixed(0)}ms, budget ${budget}ms`); + ok( + factor < blankLineCeiling, + `reading cost ${factor.toFixed(1)}x looking at every line, ceiling ${blankLineCeiling}x` + ); + }); + + test('reads an Assisted-by value that does not match, quickly', () => { + // `\S` matches a colon, so the obvious spelling of agent:model lets the + // engine try every colon as the split point. This value is built to fail, + // which is when a pattern that can backtrack does so. + // + // Judged against a value of the same length that matches at once: the + // message is one line, so splitting it measures nothing. Both sides do + // the same work but for the pattern. + const failing = `🏗️🔧：fix it\n\nAssisted-by: ${'a:'.repeat(size)} `; + const matching = `🏗️🔧：fix it\n\nAssisted-by: Claude-Code:${'a'.repeat( + 2 * size - 12 + )}`; + const factor = ratioOf( + () => validateCommitMessage(failing), + () => validateCommitMessage(matching) + ); + + // The two sit within a few percent of each other while the pattern holds. + const patternCeiling = 10; + + ok( + factor < patternCeiling, + `the failing value cost ${factor.toFixed(1)}x the matching one, ceiling ${patternCeiling}x` + ); }); });