Stop asking about Private DNS during onboarding, notify when it breaks

[kasnder]

Blocking DoT on port 853 is on by default, so Private DNS on "automatic" now resolves itself: Android falls back to plaintext and tracker detection carries on. The onboarding slide asks the user to go turn a setting off that no longer costs them anything, in the middle of the one flow where attention is scarcest. It goes.

"Hostname" mode is the case that still breaks, and it breaks harder than the slide ever conveyed: Android does not fall back to plaintext for a resolver the user pinned by name, so DNS simply fails and nothing loads. Onboarding is also the wrong place to say so — it fires once, before the user has any traffic to lose, and says nothing to whoever turns Private DNS on next week. So notify from getBuilder(), where the tunnel is built, and the reason arrives when the failure does. The text names the configured resolver, and tapping opens the network settings that hold the switch. It re-evaluates on every VPN rebuild, so it clears itself as soon as the setting changes, and it is gated on block_dot — research mode, which turns that off, never sees it.

Notification lifetimes

Clear the Private DNS and local network notifications on VPN stop, but not on a temporary one. Re-posting a cancelled notification alerts again (setOnlyAlertOnce only suppresses re-alerts while a notification is still active), and both describe configuration that has not changed, so cancelling on every incoming call would buzz the user each time the VPN returns.

The WireGuard error notification wants the opposite treatment, once it can retract itself. stopInternal() never cleared lastError, and the state listener checks lastError before isRunning() — deliberately, so a start that fails without ever producing a tunnel still reports — so a stopped tunnel went on reporting its final error, and the listener's isRunning branch could never run. Clearing it there lets the listener retract the notification as the tunnel goes down, which is more accurate than holding a tunnel error over a tunnel that no longer exists. The call in stop() stays for the one case the listener cannot see: a failed start with no tunnel to tear down, where WgEgress.stop() finds nothing to notify about.

Reordering the listener's two checks would have been the smaller diff and is wrong — a failed startTunnel() sets lastError while tunnel is still null, so checking isRunning() first would silently swallow genuine start failures.

Also

Util.isPrivateDns() had no callers left once the slide went, so it is removed. The notification uses getPrivateDnsSpecifier(), which is non-null only in the mode that actually breaks.

Testing

compileGithubDebugJavaWithJavac, lintGithubDebug, testGithubDebugUnitTest, and cargo test in wgbridge-rs all pass. Not yet exercised on a device — the notification paths in particular are unverified against a real "hostname"-mode resolver.

🤖 Generated with Claude Code

[kasnder]

Review

Read the whole diff against master, plus the surrounding call paths (getBuilder callers, removeWarningNotifications, WgEgress.stopInternal/stop/startOrUpdate and the wgStateListener, NetworkReloadPolicy, the onboarding blocking-mode preset). :app:compileGithubDebugJavaWithJavac passes here too.

The core reasoning holds up. I checked the two claims the diff leans hardest on:

• block_dot gating is real. Default is true in preferences.xml, the notification gate is the same expression as the actual block in isAddressAllowed (packet.dport == 853 && prefs.getBoolean("block_dot", true)), and the research preset in onboarding writes block_dot=false. So research mode genuinely never sees this.
• Keeping clearWireGuardErrorNotification() in stop() is required, not belt-and-braces. WgEgress.stop() only calls stopInternal + notifyStateChanged() when tunnel != null, so a failed start with no tunnel produces no listener callback at all. The PR description states this correctly.
• Util.isPrivateDns has no remaining callers, and removeWarningNotifications() doesn't cancel NOTIFY_PRIVATE_DNS, so the start-path ordering (getBuilder before removeWarningNotifications) doesn't eat the new notification.

Findings, roughly in order of how much they matter:

1. The notification is the only surface, and it can be silently unavailable

Util.notify() no-ops when canNotify() is false. Onboarding asks for POST_NOTIFICATIONS but the user can decline or skip it — and a user who declines notifications is exactly the sort who skipped through onboarding, i.e. the one the old slide was for. In that combination, hostname-mode Private DNS now means nothing resolves, with no explanation anywhere, which is worse than the slide it replaces.

#709 handled the same shape differently: the local-network warning is a notification and a row in main.xml (msg_local_network). This one has no in-app equivalent. Adding a warning row on the main screen would make the diagnosis reachable without the notification permission, and would also cover "user turned Private DNS on last week" better than a notification they may have swiped away.

2. Detection only runs when the tunnel is (re)built

getBuilder() runs on start and on reload. The realistic sequence — VPN already running, user goes into Settings and pins a Private DNS hostname — only produces the notification if something triggers a reload. NetworkReloadPolicy.onLinkPropertiesChanged reloads only when the DNS server list actually differs (or reload_onconnectivity), and whether a mode switch to hostname changes LinkProperties.getDnsServers() on a given Android version is not obvious. Worth confirming on device before assuming "the reason arrives when the failure does" holds for that path; if it doesn't, this is precisely the case that breaks silently.

3. getPrivateDnsSpecifier() is read twice, and null means silence in the broken case

The gate reads it, then showPrivateDnsNotification() reads it again for the message. Two consequences:

• If the second read returns null (mode changed in between), the user gets set to null. Pass the value from the gate into the method instead.
• If the specifier read ever returns null/empty while the mode is hostname — a hidden-Settings.Global read is not something I'd assume across OEMs, and the PR is explicitly untested on device — the condition evaluates false and the user gets nothing at all, in the exact configuration that is broken. Keying the decision on "hostname".equals(private_dns_mode) and treating the specifier as decoration (with a generic fallback string) fails safe; keying on the specifier fails silent.

4. WireGuard error notification now re-alerts on temporary stops

stop(temporary) calls WgEgress.stop() unconditionally, so with a genuinely broken tunnel the error notification is now cancelled and re-posted around every phone call — setOnlyAlertOnce doesn't span a cancel, which is the reasoning used two lines above to not clear the other two. It's defensible (a WG error is an error, not a config warning) but the diff doesn't say so, and the comment it sits under — "Final teardown — service is actually stopping" — is already wrong given temporary. Worth a sentence either way.

5. Minor

• The onboarding_privatedns_* strings survive in seven values-*/strings.xml (fi, zh-rCN, ru, pt-rBR, sl, uk, fr). Lint stays green only because ExtraTranslation is disabled — worth removing with the defaults.
• No test. Notify about missing local network access, and stop misdiagnosing it #709 shipped LocalNetworkAccessTest for its condition; this condition is inline in getBuilder and untestable as written. A small predicate (mode + block_dot → should-warn) would be Robolectric-testable the same way, and would pin down Opening app details discards scrolling position in main screen #3.
• stopInternal clearing lastError isn't covered either, though I don't see a practical way to unit-test it — stopInternal is only reachable with a live tunnel.

Happy to push fixes for #3 and #5 to this branch if you want them.

🤖 Generated with Claude Code

[kasnder]

Pushed a follow-up commit addressing findings #3 and #5 from the review above:

• Util.isPrivateDnsHostnameMode() gates the notification on private_dns_mode == "hostname" directly, instead of on getPrivateDnsSpecifier() != null — a specifier read that comes back null for any reason no longer silently suppresses the warning in exactly the broken configuration. The specifier is read once and passed through to showPrivateDnsNotification(), and falls back to generic text ("a custom resolver") if unavailable.
• Removed the leftover onboarding_privatedns_* strings from the seven locale files that still had them.

Findings #1, #2, and #4 are left as-is (design/verification questions rather than straightforward fixes) — flagging again in case you want them addressed before merge. compileGithubDebugJavaWithJavac and lintGithubDebug both pass.

[kasnder]

Device verification (Pixel 8, Android 16, githubDebug at e733f63)

Ran this on a physical Pixel 8 against a real "hostname"-mode resolver (dns.google), which the PR description flagged as untested.

Works as designed

The notification fires, with the right text. With private_dns_mode=hostname, private_dns_specifier=dns.google and block_dot at its default, starting the VPN posted id 14 immediately:

Private DNS blocks name resolution
Android's "Private DNS" is set to dns.google, and TrackerControl blocks encrypted DNS so it can detect trackers. Android does not fall back to normal DNS in this mode, so nothing will load. Tap to set Private DNS to "Automatic" or "Off" — TrackerControl offers its own Secure DNS in Settings.

The specifier interpolates correctly (so the fix in e733f63 passes the value through as intended), and BigTextStyle expands to the full text.

The premise is real. Blocking DoT uid=1051 v4 p6 8.8.8.8/853 (and 8.8.4.4, plus both v6 addresses) in logcat — netd's DoT attempts to the pinned resolver are blocked exactly as the notification claims, repeatedly and with no fallback.

Tapping it lands in the right place. mCurrentFocus=com.android.settings/.Settings$NetworkDashboardActivity — Network & internet, where Private DNS lives.

It retracts itself. After setting the mode back to opportunistic, the next getBuilder() (a reload from a Wi-Fi cycle) dropped id 14 from the live notification list, leaving only the foreground notification. The else clearPrivateDnsNotification() branch works.

Finding #2 confirmed: nothing reloads on a Private DNS change alone

Changing private_dns_mode while the VPN was running produced no reload for at least 20s — no link properties changed, nothing. The notification only appeared/disappeared once an unrelated event (Wi-Fi cycle, network change) rebuilt the tunnel.

So the "the reason arrives when the failure does" claim holds for starting the VPN with Private DNS already pinned, but not for the sequence I'd expect to be more common: VPN already running, user pins a resolver, everything stops resolving, and the explanation doesn't arrive until the network next changes. Worth deciding whether that's acceptable or whether this wants a settings observer.

Caveat: I changed the mode via settings put global, which may not raise the same LinkProperties update the Settings UI toggle does. I intended to repeat it through the UI but the device disconnected first, so treat this as strongly indicative rather than airtight for the UI path.

Not covered

• The generic-fallback string (specifier null while mode is hostname) — a real device in hostname mode always has a specifier, so that defensive path stayed unexercised.
• Findings Support for Android 10 missing #1 (no in-app surface / notification permission) and Support custom filter lists #4 (WG error re-alerting on temporary stops) were not exercised.

🤖 Generated with Claude Code

[kasnder]

Correction to finding #2: no observer needed, and the gap is narrower than I said

Two things I got wrong above, both worth recording.

The signal is already delivered — NetworkReloadPolicy just discards it

I suggested a settings observer. That's unnecessary, and the reason isn't cost: LinkProperties already carries the private DNS state, it is not redacted for ordinary apps, and it updates live. Device-checked on the Pixel 8 (Android 17):

global private_dns_mode	private DNS fields in the app-visible LinkProperties
hostname	UsePrivateDns: true PrivateDnsServerName: dns.google
off	(absent)
opportunistic	(absent, until opportunistic validation succeeds)

TrackerControl's own onLinkPropertiesChanged log printed those fields, so a plain app sees them. The reason a Private DNS change produces no reload today is entirely this, in NetworkReloadPolicy.onLinkPropertiesChanged:

if (compareDns ? !same(lastDns, currentDns) : reloadOnConnectivity)

It compares getDnsServers() and nothing else, so a private DNS change — which leaves the resolver list untouched — is dropped on the floor. Threading getPrivateDnsServerName() (or isPrivateDnsActive()) through that comparison is a few lines, costs no new registration, and drops straight into the existing pure-JVM NetworkReloadPolicyTest, which already covers this exact function.

Most of the ways a user would notice already work

I overstated the gap. Walking the reload call sites:

• Toggling the VPN in-app → start() → getBuilder() → notification. Device-verified.
• Changing any TrackerControl setting → ServiceSinkhole.reload(...) → getBuilder() → notification. There are dozens of these call sites in ActivitySettings alone.
• Any network change → reload → notification.
• Merely opening the app → no. ActivityMain.onResume() doesn't reload; it only re-evaluates tvNotifications and tvLocalNetwork.

So the genuinely uncovered window is narrow: VPN already running, user pins a resolver in Android settings, and then never opens TrackerControl, changes no setting, and doesn't change network. Real, but much smaller than "the notification only fires at tunnel build".

These two fixes are the same fix as #1

ActivityMain.onResume() is already the place where tvLocalNetwork is re-evaluated on every resume — no observer, no service involvement. A Private DNS row alongside it would cover the "user just opens the app" case and be the in-app surface from finding #1, which also survives the user declining notification permission. Between that row and the LinkProperties comparison, nothing needs to watch anything.

🤖 Generated with Claude Code

[kasnder]

Both fixes from the discussion above are now on the branch (496f308), test-first.

Reload on a Private DNS change. NetworkReloadPolicy.onLinkPropertiesChanged now also compares the resolver name, and ServiceSinkhole feeds it LinkProperties.getPrivateDnsServerName() (null below API 28). No observer, no new registration — the callback already fired, the policy just dropped it. The new reason "private DNS changed" is deliberately not in shouldRestartWireGuard, so this reload re-evaluates the warning without costing a rebind and re-handshake; there's a test pinning that.

In-app row. tvPrivateDns on the main screen, next to tvLocalNetwork and re-evaluated in the same onResume, tapping through to the network settings. This is the surface that survives a user declining notification permission — Util.notify() is silent without it, which in this state meant no warning anywhere. It is gated on the VPN actually running, since port 853 is only blocked while it is, and it reuses msg_private_dns, which lost its last caller when the onboarding slide went (so no new string, and the existing translations apply).

The shared condition is Util.isPrivateDnsBlocked(), used by both surfaces and covered by PrivateDnsBlockedTest — hostname vs automatic vs off, research mode, and that an unreadable specifier must not silence the warning.

testGithubDebugUnitTest and lintGithubDebug pass. Both new behaviours are unit-tested; neither has been re-checked on a device since.

🤖 Generated with Claude Code