diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 3a89d9e3..7d199d0a 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -316,7 +316,6 @@ private static ExecutorService createEpdgResolver() { } private static final String ACTION_HOUSE_HOLDING = "eu.faircode.netguard.HOUSE_HOLDING"; - private static final String ACTION_SCREEN_OFF_DELAYED = "eu.faircode.netguard.SCREEN_OFF_DELAYED"; private static final String ACTION_WATCHDOG = "eu.faircode.netguard.WATCHDOG"; private native long jni_init(int sdk); @@ -493,7 +492,6 @@ else if (cmd == Command.reload && temporarilyStopped) { IntentFilter ifInteractive = new IntentFilter(); ifInteractive.addAction(Intent.ACTION_SCREEN_ON); ifInteractive.addAction(Intent.ACTION_SCREEN_OFF); - ifInteractive.addAction(ACTION_SCREEN_OFF_DELAYED); ContextCompat.registerReceiver(ServiceSinkhole.this, interactiveStateReceiver, ifInteractive, ContextCompat.RECEIVER_NOT_EXPORTED); registeredInteractiveState = true; @@ -2854,13 +2852,6 @@ public void onReceive(final Context context, final Intent intent) { executor.submit(new Runnable() { @Override public void run() { - AlarmManager am = (AlarmManager) context.getSystemService(Context.ALARM_SERVICE); - Intent i = new Intent(ACTION_SCREEN_OFF_DELAYED); - i.setPackage(context.getPackageName()); - PendingIntent pi = PendingIntentCompat.getBroadcast(context, 0, i, - PendingIntent.FLAG_UPDATE_CURRENT); - am.cancel(pi); - try { last_interactive = Intent.ACTION_SCREEN_ON.equals(intent.getAction()); InteractiveStatePolicy.onScreenStateChanged( @@ -2887,11 +2878,6 @@ public void onStatsInteractiveStateChanged(boolean interactive) { .onScreenStateChanged(last_interactive); } catch (Throwable ex) { Log.e(TAG, ex.toString() + "\n" + Log.getStackTraceString(ex)); - - if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) - am.set(AlarmManager.RTC_WAKEUP, new Date().getTime() + 15 * 1000L, pi); - else - am.setAndAllowWhileIdle(AlarmManager.RTC_WAKEUP, new Date().getTime() + 15 * 1000L, pi); } } }); diff --git a/app/src/main/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClient.java b/app/src/main/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClient.java index 2bc9e310..73a3ce94 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClient.java +++ b/app/src/main/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClient.java @@ -37,6 +37,7 @@ import java.util.concurrent.TimeUnit; import okhttp3.Cache; +import okhttp3.Call; import okhttp3.ConnectionPool; import okhttp3.Dns; import okhttp3.HttpUrl; @@ -188,6 +189,25 @@ public void shutdown() { @Nullable public byte[] resolve(@NonNull byte[] dnsQuery) { + return resolve(dnsQuery, null); + } + + /** + * Resolve a DNS query using DoH, reporting each wasted network attempt. + * + * @param dnsQuery Raw DNS wire format query bytes + * @param onFailedAttempt Invoked once per retryable failure (network error, + * server 5xx, or unusable response body), i.e. for + * every attempt that costs its full timeout budget. + * Not invoked for non-retryable client errors — the + * caller's own null handling still counts those once. + * Also not invoked when the request was canceled by + * our own {@link #getInstance} endpoint swap — that + * is not evidence the endpoint is unhealthy. + * @return DNS wire format response bytes, or null on failure + */ + @Nullable + public byte[] resolve(@NonNull byte[] dnsQuery, @Nullable Runnable onFailedAttempt) { if (dnsQuery.length < 12) { Log.w(TAG, "DNS query too short: " + dnsQuery.length + " bytes"); return null; @@ -210,23 +230,27 @@ public byte[] resolve(@NonNull byte[] dnsQuery) { Log.d(TAG, "DoH retry attempt " + attempt); } + Call call = client.newCall(request); try { - try (Response response = client.newCall(request).execute()) { + try (Response response = call.execute()) { if (!response.isSuccessful()) { Log.w(TAG, "DoH request failed with code: " + response.code()); if (response.code() < 500) return null; // Don't retry client errors + reportFailedAttempt(onFailedAttempt); continue; } ResponseBody responseBody = response.body(); if (responseBody == null) { Log.w(TAG, "DoH response body is null"); + reportFailedAttempt(onFailedAttempt); continue; } byte[] dnsResponse = responseBody.bytes(); if (dnsResponse.length < 12) { Log.w(TAG, "DoH response too short: " + dnsResponse.length + " bytes"); + reportFailedAttempt(onFailedAttempt); continue; } dnsResponse = finalizeResponse(dnsResponse, response, dnsQuery); @@ -235,7 +259,16 @@ public byte[] resolve(@NonNull byte[] dnsQuery) { return dnsResponse; } } catch (IOException e) { + if (call.isCanceled()) { + // Canceled by our own getInstance()/shutdown() swap (a DoH + // endpoint change), not a real network failure. Counting + // this would let an endpoint switch alone trip the circuit + // breaker against a perfectly healthy new endpoint. + Log.d(TAG, "DoH request canceled (client shutdown), not counted as a failure"); + return null; + } Log.e(TAG, "DoH request failed: " + e.getMessage()); + reportFailedAttempt(onFailedAttempt); } finally { // Screen off: never leave an idle keep-alive socket behind — a // server-side reset during doze would wake the radio. Cache @@ -249,6 +282,14 @@ public byte[] resolve(@NonNull byte[] dnsQuery) { return null; } + private static void reportFailedAttempt(@Nullable Runnable onFailedAttempt) { + if (onFailedAttempt != null) + try { + onFailedAttempt.run(); + } catch (Throwable ignored) { + } + } + static byte[] normalizeTransactionId(byte[] dnsQuery) { byte[] normalized = Arrays.copyOf(dnsQuery, dnsQuery.length); normalized[0] = 0; diff --git a/app/src/main/java/net/kollnig/missioncontrol/dns/DnsProxyServer.java b/app/src/main/java/net/kollnig/missioncontrol/dns/DnsProxyServer.java index dc4ec1da..4b410d97 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/dns/DnsProxyServer.java +++ b/app/src/main/java/net/kollnig/missioncontrol/dns/DnsProxyServer.java @@ -58,6 +58,9 @@ public class DnsProxyServer { private ServerSocket tcpServerSocket; private ExecutorService executor; private final AtomicInteger dohFailures = new AtomicInteger(0); + // Trips after this many consecutive failed network attempts (not queries): + // a single failing resolve burns up to three full timeout budgets, so a + // broken endpoint must stop being hammered within a few bad queries. private static final int CIRCUIT_BREAKER_THRESHOLD = 10; private static final long CIRCUIT_BREAKER_COOLDOWN_MS = 60_000; private static final int FALLBACK_DNS_TIMEOUT_MS = 5000; @@ -262,10 +265,11 @@ private void handleQuery(byte[] queryData, InetAddress clientAddress, int client // Circuit breaker: skip DoH if we recently had too many failures boolean circuitOpen = System.currentTimeMillis() < circuitOpenUntil; + AtomicInteger queryFailedAttempts = new AtomicInteger(0); if (!circuitOpen) { String endpoint = prefs.getString("doh_endpoint", BuildConfig.DEFAULT_DOH_ENDPOINT); DnsOverHttpsClient dohClient = DnsOverHttpsClient.getInstance(context, endpoint); - responseData = dohClient.resolve(queryData); + responseData = dohClient.resolve(queryData, queryFailedAttempts::incrementAndGet); } if (responseData != null) { @@ -279,8 +283,13 @@ private void handleQuery(byte[] queryData, InetAddress clientAddress, int client Log.d(TAG, "DoH query successful, response sent to " + clientAddress + ":" + clientPort); } else { if (!circuitOpen) { - int failures = dohFailures.incrementAndGet(); - Log.w(TAG, "DoH query returned null response, failures=" + failures); + // Each wasted attempt counts: one broken query can burn + // three full timeout budgets, and waiting for ten whole + // queries before tripping kept ~30s of hung work alive + // per query on a dead network. + int failures = dohFailures.addAndGet(Math.max(1, queryFailedAttempts.get())); + Log.w(TAG, "DoH query returned null response after " + + queryFailedAttempts.get() + " attempt(s), failures=" + failures); if (failures >= CIRCUIT_BREAKER_THRESHOLD) { circuitOpenUntil = System.currentTimeMillis() + CIRCUIT_BREAKER_COOLDOWN_MS; @@ -410,10 +419,11 @@ private void handleTcpConnection(Socket client) { byte[] responseData = null; boolean circuitOpen = System.currentTimeMillis() < circuitOpenUntil; + AtomicInteger queryFailedAttempts = new AtomicInteger(0); if (!circuitOpen) { String endpoint = prefs.getString("doh_endpoint", BuildConfig.DEFAULT_DOH_ENDPOINT); DnsOverHttpsClient dohClient = DnsOverHttpsClient.getInstance(context, endpoint); - responseData = dohClient.resolve(queryData); + responseData = dohClient.resolve(queryData, queryFailedAttempts::incrementAndGet); } if (responseData != null) { @@ -425,7 +435,8 @@ private void handleTcpConnection(Socket client) { Log.d(TAG, "DoH TCP query successful"); } else { if (!circuitOpen) { - int failures = dohFailures.incrementAndGet(); + // Same attempt-based accounting as the UDP path above. + int failures = dohFailures.addAndGet(Math.max(1, queryFailedAttempts.get())); if (failures >= CIRCUIT_BREAKER_THRESHOLD) { circuitOpenUntil = System.currentTimeMillis() + CIRCUIT_BREAKER_COOLDOWN_MS; dohFailures.set(0); diff --git a/app/src/test/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClientTest.java b/app/src/test/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClientTest.java index a7357c06..f37abcd3 100644 --- a/app/src/test/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClientTest.java +++ b/app/src/test/java/net/kollnig/missioncontrol/dns/DnsOverHttpsClientTest.java @@ -24,6 +24,7 @@ import java.net.InetAddress; import java.util.Arrays; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import mockwebserver3.MockResponse; import mockwebserver3.MockWebServer; @@ -130,6 +131,79 @@ public void resolveRetriesInvalidShortDnsResponse() { assertEquals(2, server.getRequestCount()); } + // --- failed-attempt reporting (drives the DoH circuit breaker) -------- + + /** + * Every retryable failure — here three consecutive 5xx responses — must be + * reported individually: DnsProxyServer's circuit breaker counts wasted + * network attempts (each burns a full timeout budget), not queries. + */ + @Test + public void resolveReportsEachRetryableServerError() { + server.enqueue(dnsResponse(503, new byte[0])); + server.enqueue(dnsResponse(503, new byte[0])); + server.enqueue(dnsResponse(503, new byte[0])); + + AtomicInteger failures = new AtomicInteger(0); + assertNull(client().resolve(QUERY, failures::incrementAndGet)); + + assertEquals(3, failures.get()); + assertEquals(3, server.getRequestCount()); + } + + /** A non-retryable client error is not reported as a wasted attempt. */ + @Test + public void resolveDoesNotReportClientErrorsAsAttempts() { + server.enqueue(dnsResponse(400, new byte[0])); + + AtomicInteger failures = new AtomicInteger(0); + assertNull(client().resolve(QUERY, failures::incrementAndGet)); + + assertEquals(0, failures.get()); + assertEquals(1, server.getRequestCount()); + } + + /** An unusable 200 body is a wasted attempt and still retries to success. */ + @Test + public void resolveReportsUnusableBodyThenRecovers() { + server.enqueue(dnsResponse(200, new byte[] { 1, 2, 3 })); + server.enqueue(dnsResponse(200, RESPONSE)); + + AtomicInteger failures = new AtomicInteger(0); + assertArrayEquals(RESPONSE, client().resolve(QUERY, failures::incrementAndGet)); + + assertEquals(1, failures.get()); + assertEquals(2, server.getRequestCount()); + } + + /** + * A request canceled by our own client shutdown (e.g. a DoH endpoint + * switch via {@link DnsOverHttpsClient#getInstance}) must not be reported + * as a failed attempt — see issue #760: counting it would let switching + * endpoints alone trip the circuit breaker against the new, healthy one. + */ + @Test + public void resolveDoesNotReportCanceledCallAsFailedAttempt() throws Exception { + server.enqueue(new MockResponse.Builder() + .code(200) + .addHeader("Content-Type", "application/dns-message") + .body(new Buffer().write(RESPONSE)) + .headersDelay(2, TimeUnit.SECONDS) + .build()); + + DnsOverHttpsClient client = client(); + AtomicInteger failures = new AtomicInteger(0); + Thread resolver = new Thread(() -> client.resolve(QUERY, failures::incrementAndGet)); + resolver.start(); + + // Give the request time to actually be dispatched before shutting down. + server.takeRequest(1, TimeUnit.SECONDS); + client.shutdown(); + resolver.join(5000); + + assertEquals(0, failures.get()); + } + @Test public void normalizeTransactionIdUsesZeroWithoutMutatingQuery() { byte[] query = new byte[]{0x12, 0x34, 0x01, 0x00};