Conversation
serveAd diverts HOUSE_AD_ROTATION_RATE (~10%) of otherwise-fillable requests to the house ad, which is unmetered: no impression row, so no short code and no /a/ click URL. Four of the five tests in this file read fields off whatever came back, so each failed on ~10% of runs — about one CI run in three went red, on whichever test happened to draw the house ad. Measured on master before this change: 6 of 15 runs failed, across four different tests. That is what took down the builds on #179 and #183, neither of which touches ads. These tests are about the click URL of a *paid* fill, so the draw is pinned above the threshold. Fixing Math.random is safe precisely here: the auction has a single candidate either way, and short codes come from crypto.randomBytes, so "issues a distinct code per paid fill" still exercises real entropy — and now checks all 60 fills instead of a subset that silently shrank whenever the house ad turned up. Pinning the draw would have deleted the only coverage of the rotation, so it is asserted directly rather than left to chance: one test for a draw under the rate, one for a draw exactly on it. The boundary is the interesting part, and sampling the rate would just be a slower way to reintroduce a flaky test. 40 consecutive runs of the file now pass, against 9 of 15 before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan45 finding(s) HIGH/CRITICAL: 10 | MEDIUM: 35
Snippets are redacted; ThreatCrush never prints matched credential material. |
Contributor
Author
|
Superseded. #183 landed an equivalent de-flake (mocking The one piece worth keeping is the rotation coverage that pinning the rate removes; that's split out into its own PR against current master, which touches no shared lines. |
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
1 participant
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.
Neither #179 nor #183 is broken. Both were red on the same file —
tests/contract/ads-short-code-serving.test.ts— but on different tests, and neither PR touches ads. The file is flaky onmaster.Cause
serveAddivertsHOUSE_AD_ROTATION_RATE(~10%) of otherwise-fillable requests to the house ad (lib/ads/serve.ts:213). A house fill is unmetered: no impression row, so no short code and no/a/click URL.Four of the five tests read fields off whatever came back, so each failed ~10% of the time:
.toMatch() expects a string, but got undefinedCannot read properties of undefined (reading 'src')Measured on clean
master: 6 of 15 runs failed, across four different tests — roughly one CI run in three.Fix
These tests are about the click URL of a paid fill, so the draw is pinned above the threshold in
beforeEach.Fixing
Math.randomis safe precisely here, and the reasons are worth stating because they wouldn't hold elsewhere:crypto.randomBytes, notMath.random— so "issues a distinct code per paid fill" still exercises real entropy, and now checks all 60 fills instead of a subset that silently shrank whenever the house ad turned up.Pinning the draw would have deleted the only coverage of the rotation, so it's now asserted directly instead of statistically — one test for a draw under the rate, one for a draw exactly on it (the comparison is
<, so the rate itself must stay paid). The boundary is the interesting part, and sampling the rate would just be a slower way to reintroduce a flaky test.Verification
tsc --noEmitclean.serveAd(ads-campaign-status,ads-self-deal) — stable across 30 runs, so this was the only one exposed.The same commit is cherry-picked onto
feature/careers-widgetandfix/docs-index-stats-trackerso both builds go green now; it merges cleanly either way, since both sides carry an identical change.🤖 Generated with Claude Code