test(china-policy): interleave the superlinear-rescan measurements (#6985) - #7020
Open
Yigtwxx wants to merge 1 commit into
Open
test(china-policy): interleave the superlinear-rescan measurements (#6985)#7020Yigtwxx wants to merge 1 commit into
Yigtwxx wants to merge 1 commit into
Conversation
…oala73#6985) The 8x gate is a ratio between two timings, so it is only trustworthy when both of them saw the same machine. Best-of-3 sat inside a single size, and the two sizes were timed as two contiguous blocks -- which makes a stall correlate across every sample of whichever block it overlaps. A ~20ms scheduling gap covers a whole block, and that is what test:data's 16-way concurrency produces. Interleaving spreads each size's samples across the full window, so the same gap has to recur on every attempt of the larger size to survive the Math.min. Math.min stays per size rather than becoming the minimum of the per-attempt ratios: timing noise is one-sided, so the fastest sample of each size is the best estimate of its true cost, while the smallest ratio preferentially selects the attempt whose denominator was stalled. Adds a positive control -- a known-quadratic scan run through the same harness and required to trip the same gate. It earned its place immediately by refuting the ratio form above, which let it through at 7.7x. The parser, the threshold, the 4x size step and the hostile inputs are unchanged.
|
@Yigtwxx is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
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.
Summary
Fixes #6985.
The diagnosis in the issue holds: the 8x threshold is fine and the measurement method is what flakes. On
main,tests/china-policy-events.test.mts:191-215:Best-of-N inside one size only absorbs a stall that lands inside that size. Because the two sizes are timed as two contiguous blocks, a stall correlates across every sample of whichever block it overlaps — a single ~20 ms scheduling gap covers a whole block, and the ratio inflates with no change in the parser. Interleaving spreads each size's samples across the full window, so the same gap has to recur on every attempt of the larger size to survive the
Math.min. That is what this PR implements.Two things came out of trying to verify it that are worth more than the diff.
Math.minper size is the right form, and the alternative is measurably wrongMy first implementation went further than the issue asked. Interleaving alone still lets the base come from a quiet attempt and the quadrupled from a busy one, so I made each attempt keep its own pair and let the attempt with the best pair decide. Across 44 rounds under load that looked like a clear win: p90 fell from 5.6x to 3.9x and the worst observation from 7.2x to 4.6x.
It was wrong, and the positive control below is what caught it.
Timing noise is one-sided — a measurement can only be slower than the work, never faster — so the fastest sample of each size is the best available estimate of its true cost, and
Math.minper size is an unbiased estimator. Taking the smallest ratio is not: it preferentially selects the attempt whose denominator was stalled. The tighter distribution was bias, not precision. Two observations make that concrete:Math.minper size clears it by 92.2 ms. The bias was eating more than half the guard's teeth.And it was not theoretical: across 22 isolation runs the ratio form let the known-quadratic control through twice, once at 7.7x against the 8x gate.
So this PR ships the form the issue specifies. The rejected alternative is recorded here because "the ratio is quieter" is a tempting and wrong reason to prefer it.
Positive control
Acceptance criterion (c) asked that a deliberately backtracking implementation still fail the guard, and there is no seam in the production parser for injecting one. Rather than add one, the new
keeps its teethtest runs a known-quadratic scan through the samemeasureScalingharness and asserts it trips the same gate. It walks the sameunbalancedinput with the unbounded prefix rescan thatparsePolicyHtmlFields' closing-tag unwind degenerates into onceopenTagCountsstops bounding it. It runs atbaseRepeat: 250because a quadratic scan at 16k would dominate the file's runtime.Its margin is not marginal: over 20 samples the control scales 14.8–19.4x against an 8x gate, with a minimum margin of 14.9 ms above the ceiling.
What I could not show
I could not reproduce the failure itself. Three load models on a 20-core machine — steady oversubscription, a load ramp, and bursts pulsed at the same timescale as one measurement — produced 0 readings above 8x in ~220 measured rounds for either the sequential or the interleaved shape, and no separation between them (worst observations 5.9x vs 6.0x at n=60, 6.6x vs 7.8x under bursts).
The most likely reason is machine size: 16-way concurrency on 20 cores is not the loaded 2–4 core box where you saw it. The first two load models also failed for a reason worth recording — a full six-measurement round takes ~25 ms, and both varied load on a scale of hundreds of milliseconds, so the load was effectively constant across the round and could not produce a between-size mismatch at all.
So the argument for interleaving here is structural rather than measured: it is the same number of samples per size, at the same cost, drawn from the whole window instead of from one contiguous block, and it cannot be worse under one-sided noise. If you would rather see a reproduction before taking it, say so and I will chase it on a constrained-CPU runner instead.
Verification
Isolation.
npx tsx --test tests/china-policy-events.test.mts— 27 pass / 0 fail (26 onmain; the control is the new one). 25 consecutive runs, 25 green.Acceptance criterion (b) — loaded and concurrent. Ten consecutive runs of
npx tsx --test --test-concurrency=16over a batch including this file, with 40 CPU spinners contending throughout: 10 pass / 0 fail.Mutation proofs. Each applied, observed, then restored from a file copy.
A real, semantics-preserving quadratic regression in the parser. Replaced the constant-time early-out in
scripts/china-policy/adapters.mjs,if (!openTagCounts.has(tag.name)) return;, with the equivalent-but-linearif (!stack.includes(tag.name)) return;— the kind of simplification a future change could plausibly make. Every correctness test in the file still passes,parses bounded hostile markup correctlyincluded; the timing guard is the only thing that fires:(Deleting that early-out outright is not a usable mutant: it throws
Invalid array lengthand takes a correctness test down with it, so it proves nothing about the timing gate.)Linearise the positive control (inner bound
index→1). The control fails, as it must —scaled only 4.0x ... -2.2ms against the ceiling.Loosen the gate (
base * 8 + 2→base * 800 + 2). The control fails at 17.3x. So the control guards the threshold too: a future "just relax the gate" fix cannot land quietly.Cost.
+180 mson this file — 616 ms onmainversus 796 ms on the branch, mean of three runs each. Effectively all of it is the positive control.Neighbouring gates.
npx biome lint tests/china-policy-events.test.mtsclean, identical tomain. No tsconfig project includes this file (tsconfig.jsonhas"include": ["src"];tsconfig.contract-tests.jsonnames eight files, none of them this one), sonpm run typecheckis unaffected; a standalonetsc --strictover the file reports 39 pre-existing errors, none inside the new block.Full suite.
npm run test:dataon the branch and on a cleanorigin/mainat02840e574. Test count 25210 → 25211, the new control. Comparing failure sets rather than counts, the branch carries exactly one namemaindoes not:pre-push heavy-phase admission(tests/prepush-admission.test.mjs). It is a pre-existing flake and not a regression — the suite builds its own throwaway git repo with five linked worktrees and races workers for two heavy-phase slots, so it does not read this repository's state at all, and on a cleanorigin/maincheckout it fails 4 of 6 isolated runs (3 of 4 on the branch). Every other name matches in both directions.Design decisions for maintainer review
SCALING_ATTEMPTSstays 3, unchanged frommain. More attempts lower the odds of a correlated stall at a linear cost in runtime, but I have no measurement showing 3 is insufficient, so raising it would be guessing.No third size. The issue floats an optional 64k measurement asserting the ratio does not grow. A 64k input is four times the largest allocation this file makes today, and its ratio would carry exactly the same fragility as the first one unless it were interleaved too — at which point it is a second copy of the assertion already made, for roughly a second of runtime. It would add cost without adding a distinct claim.
The failure message now carries the margin in milliseconds alongside the ratio, because the ratio alone does not say how close the run came to the
+2 msabsolute term. That is what made the biased-estimator problem visible.Out of scope
tests/cross-strait-activity.test.mts:251,269,287,312,630carry five absolute wall-clock gates of the formassert.ok(elapsedMs < 1_500, ...). That is precisely the anti-pattern the comment attests/china-policy-events.test.mts:181-183says this test was written to avoid, and those five will flake under the same concurrency for a simpler reason — one measurement, no ratio, no best-of-N. Different fix, different file; they deserve their own issue rather than a place in this diff.Type of change
Affected areas
/api/*)tests/china-policy-events.test.mtsonly — the China policy adapter regression suite. No production code changes.Checklist
api/rss-proxy.jsallowlist (if adding feeds) — N/A, no feeds added.npm run typecheck) — unaffected by this PR; this file is not in any tsc project. Verified separately with a standalonetsc --strictpass: no errors inside the new block.Documentation Alignment Checklist
N/A for this section: this PR publishes and changes no documentation claim. It touches one test file and no doc, proto, OpenAPI, example, Redis key, or methodology surface.
Screenshots
N/A — no UI surface.