Source: merge commit d9aff3cf2264311e06a9ebd23f49c0f16132503e — review-heavy (4 substantive review rounds, including a prior misdiagnosis that was corrected).
Proposed learning: when a performance test goes flaky under CI contention, separate the measurement basis from the timeout basis before touching either. The reviewer reproduced the failure locally rather than judging from the description. Isolated on a 4-core runner the test completed in ~2.4s; under 12 spinning processes on the same 4 cores it took ~7.1s, with a file duration of ~13s. The mechanism: the helper samples process.cpuUsage(), so the assertions are on CPU-time ratios that stay stable under contention, while the wall clock that the test runner enforces its timeout against inflates roughly threefold. The ratio assertions were never the flaky part — the clock was. Raising the wall-clock timeout was therefore the correct fix, not a mask.
The generalizable check for whether raising a timeout hides a real regression. The reviewer answered it structurally rather than by vibes: the calibration loop bounds work by CPU-milliseconds, both operations exceed the floor on the first repetition, so the repetition count stays at 1 and the sample count is constant. Nothing about a genuine superlinear regression could route through the timeout instead of the assertion. Combined with the fact that the test exercises synthetic closures and carries no production surface, the raised timeout is a wall-clock-only knob on a deterministic workload. That chain — what does the assertion measure, what does the timeout measure, can a real regression escape through the timeout path — is the reusable template.
A second learning worth capturing separately: paired assertions that look redundant often aren't. The change restored both a separation check and an absolute bound. The reviewer's argument for keeping both: the separation check survives contention because inflation cancels across a ratio of ratios, while the absolute bound catches uniform inflation that the separation check is structurally blind to. Deleting the bound would have left the estimator free to lie in one direction with nothing watching. Before removing one of two assertions as duplicative, name the failure mode each one uniquely catches.
Suggested capture: docs/solutions/testing/. Scope: diagnosing CPU-time-versus-wall-clock flakiness in performance guards, the regression-escape analysis that licenses a timeout increase, and the non-redundant-paired-assertions rule. The prior misdiagnosis on this same change is itself an argument for reproducing the failure mode before judging it.
Source: merge commit
d9aff3cf2264311e06a9ebd23f49c0f16132503e— review-heavy (4 substantive review rounds, including a prior misdiagnosis that was corrected).Proposed learning: when a performance test goes flaky under CI contention, separate the measurement basis from the timeout basis before touching either. The reviewer reproduced the failure locally rather than judging from the description. Isolated on a 4-core runner the test completed in ~2.4s; under 12 spinning processes on the same 4 cores it took ~7.1s, with a file duration of ~13s. The mechanism: the helper samples
process.cpuUsage(), so the assertions are on CPU-time ratios that stay stable under contention, while the wall clock that the test runner enforces its timeout against inflates roughly threefold. The ratio assertions were never the flaky part — the clock was. Raising the wall-clock timeout was therefore the correct fix, not a mask.The generalizable check for whether raising a timeout hides a real regression. The reviewer answered it structurally rather than by vibes: the calibration loop bounds work by CPU-milliseconds, both operations exceed the floor on the first repetition, so the repetition count stays at 1 and the sample count is constant. Nothing about a genuine superlinear regression could route through the timeout instead of the assertion. Combined with the fact that the test exercises synthetic closures and carries no production surface, the raised timeout is a wall-clock-only knob on a deterministic workload. That chain — what does the assertion measure, what does the timeout measure, can a real regression escape through the timeout path — is the reusable template.
A second learning worth capturing separately: paired assertions that look redundant often aren't. The change restored both a separation check and an absolute bound. The reviewer's argument for keeping both: the separation check survives contention because inflation cancels across a ratio of ratios, while the absolute bound catches uniform inflation that the separation check is structurally blind to. Deleting the bound would have left the estimator free to lie in one direction with nothing watching. Before removing one of two assertions as duplicative, name the failure mode each one uniquely catches.
Suggested capture:
docs/solutions/testing/. Scope: diagnosing CPU-time-versus-wall-clock flakiness in performance guards, the regression-escape analysis that licenses a timeout increase, and the non-redundant-paired-assertions rule. The prior misdiagnosis on this same change is itself an argument for reproducing the failure mode before judging it.