Notice a pinned Private DNS when it happens, and warn in the app too

[kasnder]

Follow-up to #711, which landed the Private DNS warning. Two gaps that review turned up after it merged; both are about the warning reaching the user, not about what it says.

The warning only re-evaluated when the tunnel was built

getBuilder() is the only thing that posts or clears it, so the sequence I would expect to be the common one — VPN already running, user goes into Android settings and pins a resolver — broke name resolution and then said nothing until some unrelated network change happened to rebuild the tunnel. Device-checked on a Pixel 8 (Android 17): after changing the mode, no reload for at least 20s, and the notification appeared only once a Wi-Fi cycle forced one.

No observer is needed for this. onLinkPropertiesChanged already fires, and LinkProperties already carries the resolver name — unredacted for ordinary apps, which the app's own log confirms:

private_dns_mode	app-visible LinkProperties
hostname	UsePrivateDns: true PrivateDnsServerName: dns.google
off / opportunistic	(fields absent)

NetworkReloadPolicy.onLinkPropertiesChanged simply discarded it, comparing getDnsServers() alone — which a pinned resolver leaves untouched. It now compares the name too. The new reason is deliberately not in shouldRestartWireGuard: the tunnel is unaffected, only the warning is stale, so this must not cost a rebind and re-handshake. There is a test pinning that.

The notification was the only surface, and it can be silently unavailable

Util.notify() no-ops without POST_NOTIFICATIONS. Onboarding asks, but a user can decline — and a user who declines is exactly the one who skipped through onboarding, which is who the removed slide was for. In that combination there was no warning anywhere, in the one state where nothing loads at all.

So: a row on the main screen, next to the local-network one from #709 and re-evaluated in the same onResume, tapping through to the network settings. That also covers merely reopening the app, which reloads nothing. It is gated on the VPN actually running, since port 853 is only blocked while it is — with the VPN off a pinned resolver works fine and a warning would be wrong.

It reuses msg_private_dns, which lost its last caller when the onboarding slide went, so this adds no new string and the existing translations apply.

Also

The condition both surfaces share is now Util.isPrivateDnsBlocked() rather than duplicated at each call site, covered by PrivateDnsBlockedTest: hostname vs automatic vs off, research mode (block_dot off), and that a specifier which reads back empty must not silence the warning — failing silent in the broken state is the one outcome worth designing against.

Testing

testGithubDebugUnitTest and lintGithubDebug pass. Both behaviours are unit-tested; neither has been exercised on a device in this form — the row in particular has not been seen rendered.

🤖 Generated with Claude Code

[kasnder]

Review

Both behaviours are the right shape, and the split — new reason for the warning, no rebind — is the correct call. One real defect, fixed on the branch in 5f15815; the rest are notes.

Fixed: the burst debounce drops the WireGuard rebind (ServiceSinkhole.java)

reloadAfterNetworkChange coalesces a burst of callbacks to its last reason, and its comment said so explicitly:

Every reason string currently in use maps to the same shouldRestartWireGuard()==true branch, so collapsing to the latest reason changes no behaviour beyond the log line.

This PR is what makes that comment false. REASON_PRIVATE_DNS_CHANGED is the first reason that returns false, so it does not merely fail to trigger a rebind — it cancels one that an earlier reason in the same 1500 ms window already required.

Concretely: a network attaches, onAvailable posts network available (rebind needed). The first onLinkPropertiesChanged for that network carries no resolver name yet; a second arrives once strict-mode validation completes, same DNS server list, private_dns now dns.google. That returns private DNS changed, replaces the pending reason, and the runnable that finally fires skips WgEgress.onUnderlyingNetworkChanged(). The tunnel keeps a socket bound to the old network until some later event happens to fire.

The need for a rebind is sticky across a burst even though the reason string is not, so I accumulate it rather than reading it off the survivor: shouldRestartWireGuard(boolean pendingRestart, String reason) in NetworkReloadPolicy, an AtomicBoolean in the service (callbacks arrive off the main thread, the reload runs on it), and the stale comment corrected. Two tests added — a burst mixing reasons still rebinds, and the new reason alone still does not.

Notes, no change made

• The row does not say anything is broken. msg_private_dns is "Turn off "Private DNS" in Android network settings…" — an instruction with no symptom attached, next to a warning triangle. The notification says "nothing will load", and the row is the only surface for exactly the user who will not see that. Reusing the string to inherit the translations is a fair trade, but it means the one warning that user gets never names the problem. Worth a new string later.
• "enabled" rather than the service state. A reasonable proxy for "we are blocking 853", and consistent with how the screen reads other state; just noting it can be briefly true before the tunnel is up.
• Re-evaluated only in onResume. Right for this case — changing Private DNS means leaving the app — so nothing to do.

Verification

:app:testGithubDebugUnitTest and :app:lintGithubDebug both pass with the fix. Still not device-checked, as you noted: the row rendering and the private DNS changed reload firing on a live mode switch are the two things worth confirming on the Pixel.

[kasnder]

Device-checked on the Pixel 8 (Android 17)

Both behaviours verified end to end on net.kollnig.missioncontrol.test, VPN running, block_dot on. One cosmetic defect found and fixed (bc1c48a).

The row renders and works. With private_dns_mode=hostname it appears above the stats card with the amber warning icon, clickable="true", and tapping it lands on com.android.settings/.Settings$NetworkDashboardActivity — the screen the Private DNS setting lives on. Setting the mode back to opportunistic and reopening removes it. Absent from the hierarchy in both non-pinned states, as intended.

The live detection works, and this is the gap the PR set out to close. Switching the mode while the VPN was already up produced exactly the case the old comparison could not see — identical resolver list, only the name appearing:

Changed link properties={... DnsAddresses: [ /2a01:860:0:300::53, ... ] UsePrivateDns: true
  PrivateDnsServerName: adblock.dns.mullvad.net ...}
  DNS cur=<4 servers> DNS prv=<same 4 servers> private DNS cur=adblock.dns.mullvad.net prv=null
Reason=private DNS changed
Executing intent=... command=reload reason=private DNS changed
Private DNS set to a hostname: DoT is blocked and Android will not fall back to plaintext DNS

Reload fired ~1.5 s after the setting changed (the debounce), and clearing the mode fired the same reason in the other direction. No 20 s wait, no Wi-Fi cycle.

Fixed: the row rendered without the quotes that name the setting

msg_private_dns had no caller until this branch gave it one, and its quotes are unescaped in values/strings.xml, so aapt strips them. On screen it read:

Turn off Private DNS in Android network settings.

Which names no setting — "Private DNS" reads as prose, and the user is left looking for something called that. Every translation of the same string already escapes them, so English was the only one affected. Escaped, rebuilt, re-checked on the device: the quotes are now in the rendered text.

Device state restored (private_dns_mode back to opportunistic, specifier untouched), dumps cleaned off /sdcard. lintGithubDebug and testGithubDebugUnitTest still pass.

This also makes my earlier note about the wording sharper rather than weaker: with the quotes back the row names the setting, but it still never says anything is broken. Seeing it rendered, that reads oddest right where it matters — the row is bright red and urgent, and says only "turn this off", while the user's actual symptom is that nothing loads at all.