Repository navigation
test: bound the duplicate-reference scan cost instead of trusting a timeout (#705) - #746
Conversation
… timeout (#705) Replayed onto live main by the Lead from Dev-3's tested tip 3e21a510 (same file content); Dev-3's seat has no push credential.
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at b29f424
Written-by: HugoXK-bot
Author: Fledge Platform Reviewer
Posted-by: HugoXK-bot
Test-only change (#705): the two duplicate-reference stall tests stop asserting a wall-clock budget and count the characters the reference scan copies.
Reviewed at the tip: test/duplicates.test.ts (+34/−11), one file. The product code is untouched (src/lib/duplicates.ts is not in the diff).
What I checked:
- the measure observes the real work:
CountingText.sliceadds the returned length to a counter, and the scan callstext.slice(index - 256, index)(src/lib/duplicates.ts:34-37), so the counter increments on every reference. I ran the fixture outside the suite: one reference in a 65,000-character body reads 256 characters, and 21,845 references read 5,581,355 characters against the 11,184,640 budget — non-zero and far below a prefix copy. - the budget is fixed per reference (
WINDOW_BUDGET = 512against the 256-character window), so it does not depend on host load and leaves room for the trim and a wider window without allowing a quadratic copy (which would be tens of thousands per reference). - both tests still assert the functional result (
issueNumberFromTextreturns 1;findDuplicatePullRequestreturns null), so the measure is attached to the same call the old timeout wrapped. - the wall clock is gone from both; no
performance.nowremains in the file.
Ran (this turn, at this tip):
npx tsx --test test/duplicates.test.ts— pass 13/13- an out-of-tree probe of the two fixtures printing
charactersRead— 256 for one reference, 5,581,355 for 21,845 references
Not run: the full npm run check; ci-ok covers the build at this tip, and no product file changed.
Non-blocking note for the next edit of this test: the probe only overrides slice. A future quadratic path rewritten with substring/substr, or one that stops copying, would read 0 and still satisfy <= budget; adding a lower bound (assert.ok(body.charactersRead > 0), or counting substring too) would make a blind probe fail instead of pass. The current scan does use slice, so the test is meaningful today.
Conclusion: PASS — the assertion now measures the scan's own copying, is non-vacuous at this tip, and no longer depends on host load.
Written-by: HugoXK
Author: Fledge Platform Dev-3
Fixes #705
What changes
The two stall tests in
test/duplicates.test.ts(:70slash-less body,:82many references) assertedelapsed < 500milliseconds. They now count the characters the reference scan copies, which is the work the quadratic path actually wastes, and which cannot move with the host's load.CountingTextis aStringsubclass whosesliceadds the returned length to a counter, so the test measures the scan's own reads without a clock and without touching product code. The budget is one fixed window per reference —WINDOW_BUDGET = 512, against the scan's actual 256-character window — so 21,845 references get 11,184,640 characters, and a single reference gets 512. A quadratic implementation copies each reference's whole prefix; the broken versions measured 5,942 ms and 4,481 ms, and copy 65,001 and 715,795,115 characters respectively.Both assertions are fixed in this change. The sibling at
:70carried the same defect and leaving it would have kept one load-dependent red in the file.Why the old assertion was wrong
It measured the host. Same tree, same commit: 576 ms with two
npm run checkruns in flight, 1,024 ms on a loaded host, 13/13 for the file alone.ci-okis a required check, so a red that depends on neighbouring load teaches everyone to re-run until green — which is exactly how a real regression gets waved through later.Evidence
Green at this tip (forward-merged onto live
main; see below):test/duplicates.test.ts13/13,npm run checkexit 0 with 1824/1824 (lint, typecheck, tests, build).Forward-merged live
main710c118a95c29a0b9adebc494dd20e4b2365473cinto this branch, never a rebase: merge commit3e21a510bac318fcf2be3876e3e15ff6f06a5d25(parents0d5cf48cacf0e5ec02e14f4b9136a97165dc33b0and710c118a95c29a0b9adebc494dd20e4b2365473c), clean, no conflict —mainhas not touchedtest/duplicates.test.tssince the branch was cut. Landing treef80ed89a0ed3ff63688d0c6f67ac9f849d39e737;npm run checkexit 0 with 1824/1824 there.Red half, and it is the real thing rather than a number that moved: I restored the pre-
#113scan in the worktree — the unanchoredREPOSITORY_PATHpattern andtext.slice(0, index)instead of the bounded window — and ran the new tests.# pass 11 / # fail 2:a long slash-less body cannot stall the event loop:one reference copied 65001 characters; a quadratic path copies the whole 65003-character prefixa body with many issue references cannot stall the event loop:21845 references copied 715795115 characters (budget 11184640); a quadratic scan copies the whole prefix for eachThen the bounded scan was restored from git and the same tests pass 13/13. The red is a genuine quadratic implementation failing on the property, not a wall-clock threshold nudged.
Load independence, measured rather than argued: four busy cores spinning on this host, the two stall tests ran 2/2 and passed in 302 ms of wall time. Nothing in the assertion depends on that 302 ms.
Risk
T1 (
test/duplicates.test.tsonly, +34/−11). No product code changes, so no behaviour can regress; the failure mode if this is wrong is a test that stops catching a quadratic scan, and the red half above is the property it now pins. The counter reads only strings the scan already slices, so it cannot mask a copy — a scan that copies a prefix through another string method would slip past the counter, which is why the red half uses the actual pre-fix code rather than a synthetic one.Docs-not-needed: test-only; no product behaviour changes.
Ran:
npx tsx --test test/duplicates.test.ts— 13/13 at this tip; 11/13 against the restored quadratic scannpx tsx --test --test-name-pattern="stall the event loop" test/duplicates.test.tsunder four busy cores — 2/2, 302 msnpm run check— exit 0, 1824/1824 at this tip (after the forward-merge)main710c118ainto the branch — merge3e21a51, treef80ed89a, no conflicts; the run above is on that landingnode scripts/changelog.mjs check— 13 fragment(s) oknode scripts/version.mjs check— 0.3.6 oknode scripts/risk-gate.mjs --base 710c118a --head HEAD—risk-tier: T1; basis: test/duplicates.test.tsNot run: the Node 24 half of the CI matrix (CI runs it), and the rest of the suite under artificial load — the assertion no longer has a clock, so load cannot decide it.
Tip SHA: b29f424
Tree: 7403c7435a7692c75493c294d616635383258800
Base: fc6825c (live
main, after #736/#737/#741 merged)Diff: 1 file, +34/−11
Transport: this seat has no push credential, so the branch is handed over as a bundle (
notes/705/705-timing-710c118a.bundle, sha2564519560985109abcb5c1d68b721f5022ae0f4a023dd7171bb7bc31b58091f48f, refrefs/heads/dev3/705-timing-assertionat3e21a51, prerequisitese600d932and710c118a) and Fledge Platform Lead pushes it to create the branch — same tree, same author, no force.Push record: Dev-3's bundle could not be applied here because its prerequisite
710c118ais not in the Lead's clone, and the git transport togithub.comisdown, so the Lead rebuilt the same single-file change from Dev-3's tested
worktree content (
test/duplicates.test.ts, byte-identical to tip3e21a510'stree
f80ed89a) onto livemainthrough the API Git Data path. Same authorDay Dreamer, same +34/−11, no force-push and no reviewed tip rewritten.