Fix DNS cache invalidation race in tracker attribution

[kasnder]

What's fixed

Closes #757.

dnsResolved() (ServiceSinkhole.java) does, in order: insertDns(rr) → prepareUidIPFilters(rr.QName) → ipToHost.remove(rr.Resource) / ipToTracker.remove(rr.Resource). blockKnownTracker() reads DNS evidence via dh.getQAName(uid, daddr, ...) under DatabaseHelper's read lock, then does ipToHost.put(...) / ipToTracker.put(...) outside any lock.

insertDns/getQAName serialise on DatabaseHelper's ReentrantReadWriteLock, but that lock does nothing for the cache write. A packet thread that reads the DB before a concurrent insert can still write its now-stale answer after the clear, pinning pre-insert attribution in the cache. prepareUidIPFilters() sitting between the insert and the removes widens the window further. This is reachable at the worst possible moment: a DNS answer arrives and the app connects to that IP microseconds later, so the racing threads run concurrently by construction on the first connection after resolution.

Corrected impact (vs. the issue's claims)

The issue frames this as multi-day stale attribution. Both TTL claims don't hold up:

• Common case is capped at 60s, not 3 days. If the racing read finds no row (the IP was never resolved before), the result is dname == NO_DNAME, which uses NEGATIVE_TRACKER_CACHE_TTL_MS = 60_000 specifically so an unconfident miss re-checks soon.
• Ceiling is 12h, not 3 days, for any entry. householding() runs every 12h and does a wholesale ipToHost.clear() / ipToTracker.clear(), so chosenTime + chosenTtl is never the real bound.

What actually bites is narrower: the IP already has a live DNS row, and the new row would have flipped the verdict — either gaining tracker evidence where the cache locked in non-tracker (missed blocks), or gaining non-tracker evidence that makes it ambiguous (sawTrackerEvidence && sawNonTrackerEvidence && !blockAmbiguousTrackers → NO_TRACKER, i.e. over-blocking persists). Bounded at ≤12h. This is the shared-CDN-IP case, so it's not exotic, but it's a good deal rarer and shorter-lived than the issue implies.

The fix

A static final AtomicLong trackerCacheGeneration, bumped at all three sites that invalidate ipToHost/ipToTracker, and checked in the one place that writes to them after an unlocked DB read:

• dnsResolved() bumps it after both ipToHost.remove/ipToTracker.remove calls, inside the same Util.isNumericAddress(rr.Resource) block, itself inside the insertDns(rr) succeeded branch. Bumping before the removes would reopen the identical window (a racing writer could still land after the bump but before the actual clear). If insertDns returns false, no new row was inserted and no cache entry became stale, so no bump is needed. If the resource isn't a numeric address, it was never a cache key, so again nothing to invalidate.
• clearTrackerCaches() — called from BlockingMode.applyMode() (net/kollnig/missioncontrol/data/BlockingMode.java) whenever the user changes blocking mode — bumps it right after the two clear() calls. This one matters: blockKnownTracker() reads blockAmbiguousTrackers from the current mode at the very top of the method, before any cache or DB read, and uses it to decide whether mixed tracker/non-tracker evidence resolves to NO_TRACKER or stays blocked. If the mode changes mid-computation, clearTrackerCaches() wipes the caches, and without this bump an in-flight put would re-pin a verdict computed under the old mode — e.g. the user switches to Strict and one IP silently keeps its Standard-mode answer until that entry's TTL (or the next 12h householding()) catches up.
• householding() — the 12-hourly wholesale clear — bumps it right after its ipToHost.clear()/ipToTracker.clear(), for the same reason and same fix shape, though the impact here is much smaller since the next 12h cycle would clear it again regardless.
• blockKnownTracker() snapshots the counter before the getQAName DB read, and performs the two puts only if the counter is unchanged after the read. Skipping is correct and cheap: the next packet to that IP simply re-reads the DB.

Why a global counter, not per-IP (or per-invalidation-site)

A single global counter means any cache invalidation — a DNS answer for any IP, a mode change, or the 12h housekeeping sweep — can cause a spurious skip on an unrelated in-flight blockKnownTracker() read. The cost of that false-skip is one extra DB read on the next packet to the affected IP; it's not a correctness cost. The window an in-flight read is exposed for is one getQAName cursor scan (sub-millisecond to low-millisecond), and the trigger is any other invalidation arriving in that window — not exotic on a busy device, but the miss just means the read repeats, so the amortised cost of the false-positive rate stays small relative to the packet path. A per-IP scheme (e.g. a ConcurrentHashMap<String, Long> of per-address generations) would tighten the false-skip rate to true positives only, but adds new unbounded state that itself needs eviction/cleanup — worse than the cost it removes, for a bounded, self-correcting cost.

Rejected alternatives (per the issue)

• Populate under the DatabaseHelper lock: would hold the lock across TrackerList.findTracker() for every DNS row candidate, on the packet-processing hot path — turns a read lock scope into something that blocks concurrent DB writers for tracker-list lookup time.
• Re-check for a newer row before caching: costs a second DB round-trip per cache miss to get the same guarantee the counter gives for free.

Testing

• ./gradlew :app:compileGithubDebugJavaWithJavac — passes.
• ./gradlew :app:testGithubDebugUnitTest — 279 tests, 0 failures (existing suite, unaffected by this change).
• No new unit test added. blockKnownTracker() is private and needs a live VpnService, so it isn't directly unit-testable, matching the precedent noted for HostsBlocklistLogic in AGENTS.md. I looked at extracting the generation-check guard the way HostsBlocklistLogic extracts hosts-file parsing, but the guard itself is a two-line long comparison with no independent behaviour — the actual race is in the interleaving of two threads through DatabaseHelper's real read/write lock and SQLite, and testing that would require pulling apart far more of blockKnownTracker() (the DB loop, TrackerList, SharedPreferences, PackageManager) than this ~30-line fix touches. Ship without a test rather than force a disproportionate extraction; this change is verified by code reading and the ordering argument above, not by a regression test.

Heads-up: rebase with #776

PR #776 (fix/shared-ip-ui-alive-rows) also edits ServiceSinkhole.java and changes both getQAName(uid, daddr, true/false) call sites to getQAName(uid, daddr) (dropping the alive parameter). One of those call sites is the same getQAName call in blockKnownTracker() this PR adds a generationBefore snapshot next to. Neither PR touches the same lines as the other, so a merge either way should only need a trivial rebase — flagging for merge sequencing.

🤖 Generated with Claude Code