Repository navigation
Fix a quadratic sitemap sampler that blocks the event loop, and stop reporting weight as a percentage - #72
Merged
Conversation
… of the score
Two independent bugs, both found by consumers of the published package.
1. selectRepresentativeSample was quadratic, and it blocks the event loop.
`interleaveBySubValue` walked EVERY bucket on every round, including buckets
long since exhausted. The shape that breaks it is ordinary rather than
adversarial: one dominant sub-value plus a long tail of one-off ones, which is
what a sitemap of /p/<city>/<slug> URLs looks like. Both the round count and the
bucket count become ~n/2, so emitting n values costs ~n^2/4.
`ordered` is already sorted by descending bucket size, so the buckets still
holding a value at round r are exactly a prefix of it. Shrinking that prefix as
rounds advance is linear and emits the IDENTICAL sequence (verified against the
old implementation on 3000 random shapes).
n=40000 6851ms -> 8ms
n=110398 52695ms -> 22ms
Severity is not the wall clock, it is that the work is synchronous and runs
before the first page fetch, so it blocks the event loop and suspends the very
timeouts, AbortSignals and fetch budgets meant to bound a sitemap audit. In a
hosted deployment one 5 MB sitemap (~110k URLs, a single fetch of the budget)
froze the whole process for 13 seconds, taking the health endpoint and every
other tenant's in-flight audit with it.
2. `weight` is not a percentage, and we printed it as one.
The core weights sum to 111, not 100, and `scoreFactors` correctly divides by
that actual total. So a weight-12 factor is 10.8 percent of the score. Both
formatters printed `${factor.weight}%`, overstating all sixteen and producing a
column that never adds up; a consumer dashboard reproduced the same mistake
independently, which is the signal that the payload was missing the number
rather than that the consumers were careless.
Factors now carry `sharePct`, computed from the same denominator the score uses.
It is optional so `compare` still reads reports produced before it existed, and
the exported `factorSharePct` helper derives it from a report's own weight sum in
that case, never from `weight` alone. Both formatters use it, and the markdown
column header is now "% of score" rather than "Weight".
The score itself is unchanged: this fixes what is REPORTED about the weights, not
the weighting.
BREAKING: page scores change. A stored report cannot be compared across this.
An analyzer reporting `applicable: false` still returns score 0, and that 0 was
weighted into overallScore, so a product page lost real points for not being an
FAQ page. The flag existed to say exactly that, and the sitemap rollup already
honored it via `applicableAvgScore` while the page score did not.
Non-applicable factors are now excluded from BOTH sides of the average. Excluding
only the numerator would be worse than the bug, silently capping every such page
below 100. `factorApplies` moves into scoring.ts as the ONE predicate the page
score and the sitemap rollup share; they disagreed before.
example.com: 31 -> 35. Scores rise on pages carrying a page-specific factor that
does not apply to them, and are unchanged everywhere else.
Also: `weight` is not a percentage. The core weights sum to 111, the score divides
by the real total, so a weight-12 factor is ~12.4% of a page's score. Both
formatters printed `${factor.weight}%`, overstating all sixteen into a column that
never added up, and a downstream dashboard reproduced the same mistake
independently, which says the payload was missing the number rather than that
consumers were careless. Factors now carry `sharePct` from the same denominator
the score uses; it sums to 100. Optional, so `compare` still reads older reports,
with `factorSharePct` deriving it from the report's own weights, never from
`weight`. A non-applicable factor reports 0: it moved the score by nothing.
docs/scoring.md and CLAUDE.md no longer claim the weights sum to 100.
Three review findings, all correct.
1. sharePct did not sum to 100, while three places said it did. Rounding each
weight/total independently does not preserve the total: the 16 core factors
over a denominator of 111 come to 99.9. The committed test was the only honest
statement of it ("within rounding"), and the end-to-end check passed on
example.com by luck, because excluding the two non-applicable factors happened
to land on a denominator that rounded cleanly.
Fixed by allocating rather than softening the prose: largest remainder in
integer tenths, ties broken on weight then id so the result is deterministic
for a factor set rather than input order. A consumer can now print a total row.
The test asserts EXACTLY 100 across many subsets, since one lucky denominator
proves nothing, and pins that the naive method gives 99.9 for the core set.
`factorSharePct` uses the same allocation, so a derived column and a recorded
one cannot differ by a tenth depending on report age.
2. `factorApplies` resolves three ways but only the explicit one was recorded. A
page-specific factor judged by the presence fallback came back score 0,
sharePct 0, `applicable` undefined, so a consumer could not tell "did not apply
here" from "applied and scored zero" without reimplementing
PAGE_SPECIFIC_FACTOR_IDS and the threshold. That is this PR's own argument
turned around. The resolved boolean is now always emitted.
3. Tests. The equivalence claim the interleave rewrite rests on ("emits the
identical sequence") was verified against the old implementation but that
verification lived outside the repo, so a later refactor could silently change
which pages a sitemap audit samples. The pre-fix implementation is now
committed as the reference and a seeded property test compares 500 random
shapes against it. Note the sampler returns its picks in DOCUMENT order, so the
interleave decides WHICH pages, not their order; the test compares sets.
Proven by mutation: a linear-but-wrong-order rewrite fails it.
The cost test keeps a wall clock but at a size where the two implementations
are ~3 orders of magnitude apart, so no threshold tuning is possible, and it
gains a ratio check at n and 2n that catches a quadratic regression even on a
slow runner, where an absolute threshold only says "everything is slow".
It failed on its first CI run at 3.53 against a 3x bound, with correct code. The reason is the sizes it used. At n and 2n of 20k, linear work is a few milliseconds, so the ratio measures GC and JIT scheduling rather than the algorithm. I added it reaching for rigor and got the opposite: a test that teaches maintainers to re-run rather than to look, which is worse than not having it. The absolute check already carries the weight, and carries it better. At n=110k the two implementations are ~3 orders of magnitude apart (~22 ms against ~53 s), so there is no threshold to tune and no plausibly-loaded runner closes the gap. Verified it still fails against the quadratic implementation, and that it passes five runs in a row. A genuinely deterministic version would need an operation counter, which means exporting instrumentation from the module purely for a test. Not worth the API surface when the separation is this wide; the equivalence property test is the one carrying the correctness claim anyway.
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
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.
Three fixes, all found by consumers of the published package. Two are breaking, so this is
5.0.0.1.
selectRepresentativeSampleis quadratic, and it blocks the event loopinterleaveBySubValuewalked every sub-value bucket on every round, including buckets long since exhausted. The shape that triggers it is ordinary, not adversarial: one dominant sub-value plus a long tail of one-off ones, which is what a sitemap of/p/<city>/<slug>URLs looks like. Both the round count and the bucket count become ~n/2, so emitting n values cost ~n²/4.orderedis already sorted by descending bucket size, so the buckets still holding a value at round r are exactly a prefix of it. Shrinking that prefix as rounds advance is linear and picks the identical pages.The severity is not the wall clock. The work is synchronous and runs before the first page fetch, so it blocks the event loop and thereby suspends the very timeouts,
AbortSignals and fetch budgets meant to bound a sitemap audit. In a hosted deployment, one 5 MB sitemap — a single fetch of the budget — froze the whole process for 13 seconds, taking the health endpoint and every other tenant's in-flight audit with it.The pre-fix implementation is committed as the reference, and a seeded property test compares 500 random shapes against it, so the equivalence claim is enforced rather than asserted in a commit message. Note the sampler returns picks in document order, so the interleave decides which pages, not their order; the test compares accordingly. A linear-but-wrong-order rewrite fails it.
2. BREAKING: a factor that does not apply no longer counts against the page
An analyzer reporting
applicable: falsestill returns score 0, and that 0 was weighted intooverallScore. A product page lost real points for not being an FAQ page. The flag existed to say exactly that, and the sitemap rollup already honored it throughapplicableAvgScore— the page score did not.Non-applicable factors are now excluded from both the numerator and the denominator. Dropping only the numerator would be worse than the bug, silently capping every such page below 100.
factorAppliesmoves intoscoring.tsas the one predicate both paths share.The resolved applicability is now always emitted, not just what an analyzer volunteered. A page-specific factor judged by the presence fallback used to come back
score: 0,sharePct: 0,applicable: undefined— indistinguishable from one that applied and scored zero unless the consumer reimplementedPAGE_SPECIFIC_FACTOR_IDSand the threshold.example.com goes 31 → 35.
3. BREAKING:
weightis not a percentage, and we printed it as oneThe core weights sum to 111, not 100, and the score correctly divides by the actual total. Both formatters printed
${factor.weight}%, overstating all sixteen and producing a column that never adds to 100. A downstream dashboard reproduced the same mistake independently, which is the signal that the payload was missing the number rather than that consumers were careless.Factors now carry
sharePct, computed from the same denominator the score uses and allocated by largest remainder so the shares sum to exactly 100. Rounding each weight independently does not: the core set comes to99.9, which would reproduce the same never-adds-up column one decimal down. The test asserts exactly 100 across many subsets, since one lucky denominator proves nothing.sharePctis optional socomparestill reads reports produced before it existed, and the exportedfactorSharePcthelper uses the same allocation in that case — neverweightalone. A non-applicable factor reports0.The markdown column header is now "% of score", and
docs/scoring.md/CLAUDE.mdno longer claim the weights sum to 100.Verification
dist: example.com scores 35,sharePctsums to exactly 100, every factor carries a resolvedapplicable, and the two non-applicable factors report 0.