fix(ads): tune the impression dedupe window from 60s to 5s - #193
Merged
Merged
Conversation
#192 shipped a 60s window picked by reasoning about human behaviour rather than by measurement. Backtesting the exact rule against 7 days of real impressions shows that was wrong: the machine bursts it targets are compressed into seconds, so a wide window costs a great deal of real delivery to catch almost nothing extra. window terminal flagged (target) web flagged (cost) 3s 84.5% 12.2% 5s 84.6% 14.2% 10s 84.7% 17.5% 60s 85.4% 36.6% 60s bought +0.9pp on the target and suppressed 36.6% of real web impressions. 5s keeps essentially all of the burst suppression -- the observed burst spans ~2.5s -- with margin for a slower run, since each fetch in the loop can take up to its own timeout. Worth recording why the web number is not noise. Of the 36.6% flagged at 60s, only 0.1pp came from distinct visitors colliding on a shared ip_hash; the rest was the same visitor_id re-fetching the same slot. Broken down by gap, the sub-second repeats are /ad.js clearing data-cp-filled in its .catch() path and SPA callers re-firing scan() -- a real double-count bug, which the 5s window still catches. The 10-60s repeats are human reloads and SPA navigation, which are genuine delivery and should never have been suppressed. No migration: the stored column comment does not name a duration, and existing flagged rows are left as they are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan52 finding(s) HIGH/CRITICAL: 4 | MEDIUM: 45 | LOW: 3
…and 2 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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.
Corrects a number I picked badly in #192.
What was wrong
#192 shipped a 60 second impression dedupe window. I chose that by reasoning about human behaviour — "no human pattern lands inside a minute twice by accident" — not by measuring. Backtesting the exact shipped rule (same slot, same visitor or same ip_hash, within the window) against 7 days of real impressions shows it is wrong:
Terminal is essentially flat — the machine bursts are compressed into ~2.5s, so widening the window catches almost nothing extra. Web triples. 60s bought +0.9pp on the thing it was aimed at and suppressed 36.6% of real web impressions.
This is live right now, so advertisers are currently seeing web impression counts about a third lower than they should be.
Why the web number is not noise
Two checks before concluding:
ip_hashbehind NAT. The rest is the samevisitor_idre-fetching the same slot. (I initially assumed the opposite and was wrong.)/ad.jsclearsdata-cp-filledin its.catch()path, and SPA callers re-firescan(). That is a genuine double-count bug, and the 5s window still catches it.5s sits cleanly between the two, with margin for a slower burst since each
curlin the refresher loop can run up to its own timeout.Not included
The underlying
/ad.jsdouble-fetch bug is left alone. Its effect on counts is already absorbed by the dedupe; what remains is a wasted HTTP request per occurrence. Fixing it properly means deciding whether a slot whose fetch failed should ever retry — an impression may already have been metered server-side, so retrying double-counts while not retrying leaves the slot blank on a transient blip. That trade-off deserves its own change.Testing
npm run typecheck— cleannpm test— 1,426 passed, 1 failed (tracker-geo, pre-existing/environmental, needs the GeoLite2 mmdb; CI installs it)No migration. The stored column comment does not name a duration, and rows already flagged at 60s are left as they are.
🤖 Generated with Claude Code