feat(ads): flag repeat impressions so prefetch stops reading as delivery - #192
Merged
Merged
Conversation
Clicks have had a 6h dedupe window since the ad network shipped. Impressions
had none at all, and they are metered server-side at fill time on all three
serving paths -- so anything fetching a slot on a schedule books an advertiser
impression per fetch, with no human involved. The observed case is a pool
refresher firing 12 fetches in ~3 seconds every 10 minutes: ~1,700 impressions
a day from one machine, against ~39 real web impressions in the same window.
Two deliberate differences from the click rules:
* Keyed on the SLOT, not the campaign. Each fetch in a burst draws a
different campaign at random, so campaign-keyed dedupe -- the shape used
for clicks -- collapses none of it.
* 60 seconds, not 6 hours. A repeat view an hour later is real delivery and
has to keep counting; only a machine lands twice inside a minute.
Flagged, never dropped. The impression row is what /a/<short_code> resolves a
terminal click back to, so skipping the insert would serve a real advertiser's
creative with a click link pointing at nothing -- unbilled click, unpaid
publisher, which is strictly worse than an inflated count. Reporting excludes
flagged rows instead, across all five surfaces that read impressions:
ad_account_series, ad_campaign_totals, ad_campaign_daily_series,
ad_campaign_stats, and ad_slot_stats.
The probe is best-effort and wrapped: it runs on the hot path of every fill,
and if it throws the right answer is "count it" rather than losing the
impression and the click that may follow.
Existing rows default to duplicate=false, so no historical figure moves.
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. |
ralyodio
marked this pull request as ready for review
August 10, 2026 12:09
ralyodio
added a commit
that referenced
this pull request
Aug 10, 2026
#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>
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.
Closes the impression-side half of the ad metering work started in #191.
The problem
Clicks have had a 6h dedupe window since the ad network shipped (
lib/ads/fraud.ts). Impressions had none at all — and they're metered server-side inserveAdat fill time, on all three serving paths (/api/ads/serve,/api/ads/frame,/api/ads/motd). None of that requires a browser, and the terminal path deliberately treatscurlas a real client.So anything fetching a slot on a schedule books an advertiser impression per fetch, with no human involved. Measured on prod today:
src=userdirsThat's a pool refresher firing 12 fetches in ~3 seconds every 10 minutes — roughly 1,700 impressions/day from one machine.
Two deliberate differences from the click rules
Flagged, never dropped
The impression row is what
/a/<short_code>resolves a terminal click back to. Skipping the insert would serve a real advertiser's creative with a click link pointing at nothing — unbilled click, unpaid publisher, which is strictly worse than an inflated count. And since each fetch renders a different campaign, there's no single earlier row that could stand in for the rest without misattributing every later click.So the row is always written, with a new
duplicateboolean, and reporting excludes flagged rows. All five surfaces that read impressions are updated, or the spike just reappears on a different screen:ad_account_series,ad_campaign_totals,ad_campaign_daily_series,ad_campaign_stats,ad_slot_stats.The probe is best-effort and wrapped in try/catch — it runs on the hot path of every fill, and if it throws the right answer is "count it" rather than losing the impression and the click that may follow. (This also matters because existing tests mock
ad_impressionswith only aninsertmethod.)Existing rows default to
duplicate=false, so no historical figure moves.Deploy order
The migration must be applied before this code ships, since the insert writes the new column. Per the repo convention it's applied by hand via psql/MCP, not
db push. The code does retry the insert without the new columns if they're missing, so a wrong-order deploy degrades rather than breaks — but the retry also dropsshort_code, so terminal click URLs would fall back to the long UUID form until the migration lands.Testing
npm run typecheck— cleannpm test— 1,426 passed, 1 failedtracker-geo, which needs the GeoLite2 mmdb. Pre-existing and environmental; verified against the baseline in feat(ads): share the visitor id with /ad.js, salt + rotate IP hashes #191. CI installs it vianpm ci.Note on the alternative fix
Marking the pool refresher as a crawler (
-A 'sponsor-ad-monitor/1.0') was considered and rejected:serveAdreturnshouseFill(format)for bot devices, so the per-user pages would fill with CrawlProof house ads and stop showing real advertisers entirely. This PR fixes the counting without touching what gets served.🤖 Generated with Claude Code