diff --git a/app/src/main/java/com/bitchat/android/net/ArtiTorManager.kt b/app/src/main/java/com/bitchat/android/net/ArtiTorManager.kt index b1eed65e..bb8abb89 100644 --- a/app/src/main/java/com/bitchat/android/net/ArtiTorManager.kt +++ b/app/src/main/java/com/bitchat/android/net/ArtiTorManager.kt @@ -92,7 +92,9 @@ class ArtiTorManager private constructor() { private var bindRetryAttempts = 0 private var inactivityJob: Job? = null private var retryJob: Job? = null + private var restartJob: Job? = null private var currentApplication: Application? = null + private val circuitHealth = TorCircuitHealthPolicy() private enum class LifecycleState { STOPPED, STARTING, RUNNING, STOPPING } @@ -354,6 +356,11 @@ class ArtiTorManager private constructor() { } private fun stopArti() { + // Only reached when the user turns Tor off. A pending restart is deliberately not + // cancelled by stopArtiInternal — that is what keeps a recovery alive across the stop — + // so it has to be dropped here or it would bring Arti back after an explicit off. + restartJob?.cancel() + restartJob = null stopArtiInternal() socksAddr = null _statusFlow.value = _statusFlow.value.copy( @@ -376,6 +383,19 @@ class ArtiTorManager private constructor() { startArti(application, useDelay = false) } + /** + * Runs [restartArti] on a job the stop path does not cancel. + * + * `restartArti` stops Arti first, and `stopArtiInternal` cancels both `retryJob` and + * `inactivityJob`. A restart driven from either of those coroutines therefore cancels itself + * between the stop and the start, so Arti is stopped and never comes back until the user + * toggles Tor by hand. `restartJob` is not cancelled there, so the second half survives. + */ + private fun launchRestart(application: Application) { + if (restartJob?.isActive == true) return + restartJob = appScope.launch { restartArti(application) } + } + private fun startInactivityMonitoring() { armBootstrapInactivityWatchdog() } @@ -400,7 +420,7 @@ class ArtiTorManager private constructor() { lifecycleState == LifecycleState.RUNNING ) { Log.w(TAG, "Bootstrap inactivity detected (${timeSinceLastActivity}ms), restarting Arti") - currentApplication?.let { restartArti(it) } + currentApplication?.let { launchRestart(it) } } } } @@ -420,7 +440,7 @@ class ArtiTorManager private constructor() { delay(delayMs) val currentMode = _statusFlow.value.mode if (currentMode == TorMode.ON) { - restartArti(application) + launchRestart(application) } } } else { @@ -484,6 +504,7 @@ class ArtiTorManager private constructor() { } retryAttempts = 0 bindRetryAttempts = 0 + circuitHealth.reset() startInactivityMonitoring() } @@ -498,10 +519,36 @@ class ArtiTorManager private constructor() { running = true ) } + circuitHealth.reset() stopInactivityMonitoring() completeWaitersIf(TorState.RUNNING) } + // Bootstrap milestones only fire on the way up, so a session that loses every exit + // circuit after reaching RUNNING would otherwise stay pinned at 100% while every + // request through it fails. + circuitHealth.isCircuitFailure(s) -> { + if (currentState != TorState.RUNNING || currentLifecycle != LifecycleState.RUNNING) { + return + } + if (!circuitHealth.onCircuitFailure(System.currentTimeMillis())) { + return + } + circuitHealth.reset() + Log.w(TAG, "Tor circuits failing after bootstrap; no longer reporting connected") + // Drop below 100% rather than to ERROR: callers fail closed instead of leaking + // to clearnet, the indicator stops claiming a working Tor, and the existing + // watchdog plus retry path get another chance to re-establish the session. + _statusFlow.update { + it.copy( + state = TorState.BOOTSTRAPPING, + bootstrapPercent = 75 + ) + } + startInactivityMonitoring() + currentApplication?.let { scheduleRetry(it) } + } + s.contains("AMEx: state changed to Stopping", ignoreCase = true) -> { if (currentLifecycle != LifecycleState.STOPPING) { return diff --git a/app/src/main/java/com/bitchat/android/net/TorCircuitHealthPolicy.kt b/app/src/main/java/com/bitchat/android/net/TorCircuitHealthPolicy.kt new file mode 100644 index 00000000..0cfff64e --- /dev/null +++ b/app/src/main/java/com/bitchat/android/net/TorCircuitHealthPolicy.kt @@ -0,0 +1,72 @@ +package com.bitchat.android.net + +import com.bitchat.android.util.AppConstants + +/** + * Decides when a Tor session that already finished bootstrapping should stop being reported as + * connected. + * + * Arti's bootstrap milestones only fire on the way up. Once a guard is usable the session is + * latched to 100%, and losing every exit circuit afterwards produces a stream of SOCKS errors + * that no bootstrap milestone contradicts — so the indicator keeps claiming a working Tor. + * + * Circuit failures are a normal part of Tor operation, so a single burst must not move the + * indicator. A downgrade requires [failureThreshold] failures that also span at least + * [minSpanMs], which separates "three parallel attempts lost a flaky circuit" from "still + * failing twenty seconds later". Progress on the wire clears the tally. + * + * Kept free of Android and coroutine dependencies so the policy is directly testable. + */ +internal class TorCircuitHealthPolicy( + private val failureThreshold: Int = AppConstants.Tor.CIRCUIT_FAILURE_THRESHOLD, + private val minSpanMs: Long = AppConstants.Tor.CIRCUIT_FAILURE_MIN_SPAN_MS, + private val windowMs: Long = AppConstants.Tor.CIRCUIT_FAILURE_WINDOW_MS +) { + private companion object { + // Matched as substrings, in the same style as the bootstrap milestones in ArtiTorManager. + // + // Deliberately not "SOCKS connection error": tools/arti-build/src/lib.rs logs that + // wrapper for every error out of handle_socks_connection, including local protocol + // faults raised long before a circuit is attempted ("Invalid SOCKS handshake", + // "Unsupported SOCKS version", "Unsupported SOCKS command"). Anything on the device that + // speaks SOCKS badly to the local port would otherwise read as a dead Tor. + // + // These two only appear once an exit circuit could not be built or used — + // "Failed to connect through Tor" is logged solely on client.connect() failure, and the + // second is arti's own detail for that error, which is what the report in #610 shows. + val FAILURE_MARKERS = listOf( + "Failed to connect through Tor", + "Failed to obtain exit circuit" + ) + } + + private var failureCount = 0 + private var windowStartedAtMs = 0L + + fun isCircuitFailure(line: String): Boolean = + FAILURE_MARKERS.any { line.contains(it, ignoreCase = true) } + + /** + * Records one circuit failure at [nowMs] and reports whether the session has now failed for + * long enough, and often enough, to stop being advertised as connected. + */ + @Synchronized + fun onCircuitFailure(nowMs: Long): Boolean { + val elapsed = nowMs - windowStartedAtMs + // A stale window, or a clock that moved backwards, starts counting again. + if (failureCount == 0 || elapsed > windowMs || elapsed < 0L) { + windowStartedAtMs = nowMs + failureCount = 1 + return false + } + + failureCount++ + return failureCount >= failureThreshold && elapsed >= minSpanMs + } + + @Synchronized + fun reset() { + failureCount = 0 + windowStartedAtMs = 0L + } +} diff --git a/app/src/main/java/com/bitchat/android/util/AppConstants.kt b/app/src/main/java/com/bitchat/android/util/AppConstants.kt index b1822806..2b214cce 100644 --- a/app/src/main/java/com/bitchat/android/util/AppConstants.kt +++ b/app/src/main/java/com/bitchat/android/util/AppConstants.kt @@ -115,6 +115,14 @@ object AppConstants { const val INACTIVITY_TIMEOUT_MS: Long = 5_000L const val MAX_RETRY_ATTEMPTS: Int = 5 const val STOP_TIMEOUT_MS: Long = 7_000L + + // Post-bootstrap circuit health. A Tor session that has bootstrapped can still lose every + // exit circuit — the guard stays usable, so nothing in the bootstrap path notices. + // Individual circuit failures are normal and must not move the indicator, so a downgrade + // needs both enough failures and enough elapsed time to rule out a momentary blip. + const val CIRCUIT_FAILURE_THRESHOLD: Int = 4 + const val CIRCUIT_FAILURE_MIN_SPAN_MS: Long = 20_000L + const val CIRCUIT_FAILURE_WINDOW_MS: Long = 120_000L } object UI { diff --git a/app/src/test/kotlin/com/bitchat/android/net/TorCircuitHealthPolicyTest.kt b/app/src/test/kotlin/com/bitchat/android/net/TorCircuitHealthPolicyTest.kt new file mode 100644 index 00000000..38cde2c8 --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/net/TorCircuitHealthPolicyTest.kt @@ -0,0 +1,136 @@ +package com.bitchat.android.net + +import com.bitchat.android.util.AppConstants +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class TorCircuitHealthPolicyTest { + + private val policy = TorCircuitHealthPolicy() + + // Verbatim from the report in issue #610, trimmed at the scrubbed detail. + private val obtainExitCircuit = + "Arti: ERROR: Failed to connect through Tor: Error { detail: ObtainExitCircuit " + + "{ exit_ports: [scrubbed], cause: RequestFailed(RetryError { doing: " + + "\"find or build a tunnel\", errors: [...] }) } }" + + private val socksError = + "Arti: ERROR: SOCKS connection error: tor: error connecting to Tor: " + + "Failed to obtain exit circuit for ports [scrubbed]" + + @Test + fun `recognises the failures arti actually logs`() { + assertTrue(policy.isCircuitFailure(obtainExitCircuit)) + assertTrue(policy.isCircuitFailure(socksError)) + } + + @Test + fun `does not treat bootstrap progress as a failure`() { + assertFalse(policy.isCircuitFailure("Sufficiently bootstrapped; system SOCKS now functional")) + assertFalse(policy.isCircuitFailure("We have found that guard [scrubbed] is usable.")) + assertFalse(policy.isCircuitFailure("AMEx: state changed to Running")) + } + + @Test + fun `does not treat a local SOCKS protocol fault as a circuit failure`() { + // handle_socks_connection wraps every one of its errors in "SOCKS connection error", + // including these, which are raised before a circuit is ever attempted. Anything on the + // device that speaks SOCKS badly to the local port must not read as a dead Tor. + listOf( + "Arti: ERROR: SOCKS connection error: Invalid SOCKS handshake", + "Arti: ERROR: SOCKS connection error: Invalid SOCKS request", + "Arti: ERROR: SOCKS connection error: Unsupported SOCKS version: 4", + "Arti: ERROR: SOCKS connection error: Unsupported SOCKS command: 2" + ).forEach { line -> + assertFalse(line, policy.isCircuitFailure(line)) + } + } + + @Test + fun `a burst of simultaneous failures does not downgrade`() { + // The report shows three threads failing inside the same millisecond. That is an + // ordinary circuit loss, not a dead connection. + val t = 500_000L + repeat(20) { + assertFalse( + "a same-instant burst must not move the indicator", + policy.onCircuitFailure(t) + ) + } + assertFalse(policy.onCircuitFailure(t + 1_000L)) + } + + @Test + fun `failures that persist past the minimum span downgrade`() { + val start = 500_000L + assertFalse(policy.onCircuitFailure(start)) + assertFalse(policy.onCircuitFailure(start + 5_000L)) + assertFalse(policy.onCircuitFailure(start + 10_000L)) + + // Fourth failure, and by now the outage has lasted past the minimum span. + assertTrue( + policy.onCircuitFailure(start + AppConstants.Tor.CIRCUIT_FAILURE_MIN_SPAN_MS) + ) + } + + @Test + fun `enough elapsed time alone is not enough`() { + val start = 500_000L + assertFalse(policy.onCircuitFailure(start)) + // Long span, but only two failures in it. + assertFalse(policy.onCircuitFailure(start + AppConstants.Tor.CIRCUIT_FAILURE_WINDOW_MS)) + } + + @Test + fun `failures spread beyond the window start a fresh tally`() { + var t = 500_000L + repeat(10) { + assertFalse( + "occasional failures far apart are normal Tor behaviour", + policy.onCircuitFailure(t) + ) + t += AppConstants.Tor.CIRCUIT_FAILURE_WINDOW_MS + 1_000L + } + } + + @Test + fun `reset clears an in-progress tally`() { + val start = 500_000L + policy.onCircuitFailure(start) + policy.onCircuitFailure(start + 5_000L) + policy.onCircuitFailure(start + 10_000L) + + policy.reset() + + // Without the reset this next one would have been the fourth and would downgrade. + assertFalse( + policy.onCircuitFailure(start + AppConstants.Tor.CIRCUIT_FAILURE_MIN_SPAN_MS) + ) + } + + @Test + fun `a backwards clock jump restarts the window instead of downgrading`() { + val start = 500_000L + policy.onCircuitFailure(start) + policy.onCircuitFailure(start + 5_000L) + policy.onCircuitFailure(start + 10_000L) + + assertFalse(policy.onCircuitFailure(start - 60_000L)) + } + + @Test + fun `a sustained outage downgrades exactly once per tally`() { + val start = 500_000L + var downgrades = 0 + var t = start + repeat(4) { + if (policy.onCircuitFailure(t)) { + downgrades++ + policy.reset() + } + t += 7_000L + } + assertTrue("a real outage must downgrade", downgrades == 1) + } +}