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 1a93e8d7..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,6 +92,7 @@ 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() @@ -355,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( @@ -377,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() } @@ -401,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) } } } } @@ -421,7 +440,7 @@ class ArtiTorManager private constructor() { delay(delayMs) val currentMode = _statusFlow.value.mode if (currentMode == TorMode.ON) { - restartArti(application) + launchRestart(application) } } } else { diff --git a/app/src/main/java/com/bitchat/android/net/TorCircuitHealthPolicy.kt b/app/src/main/java/com/bitchat/android/net/TorCircuitHealthPolicy.kt index 5a79be2a..0cfff64e 100644 --- a/app/src/main/java/com/bitchat/android/net/TorCircuitHealthPolicy.kt +++ b/app/src/main/java/com/bitchat/android/net/TorCircuitHealthPolicy.kt @@ -23,11 +23,19 @@ internal class TorCircuitHealthPolicy( private val windowMs: Long = AppConstants.Tor.CIRCUIT_FAILURE_WINDOW_MS ) { private companion object { - // Arti emits these once it cannot build or use an exit circuit. Matched as substrings in - // the same style as the bootstrap milestones in ArtiTorManager. + // 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", - "SOCKS connection error", "Failed to obtain exit circuit" ) } diff --git a/app/src/test/kotlin/com/bitchat/android/net/TorCircuitHealthPolicyTest.kt b/app/src/test/kotlin/com/bitchat/android/net/TorCircuitHealthPolicyTest.kt index b35b4eca..38cde2c8 100644 --- a/app/src/test/kotlin/com/bitchat/android/net/TorCircuitHealthPolicyTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/net/TorCircuitHealthPolicyTest.kt @@ -32,6 +32,21 @@ class TorCircuitHealthPolicyTest { 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