From dfc2b8dcd115736110a8290b2dbb2604a1cfc6cf Mon Sep 17 00:00:00 2001 From: Taksh Date: Sun, 16 Aug 2026 09:25:11 +0530 Subject: [PATCH] Narrow the failure marker and fix the self-cancelling restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems Codex found in the first push. "SOCKS connection error" is not a circuit failure. arti-build logs that wrapper for every error out of handle_socks_connection, including local protocol faults raised 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 have read as a dead Tor. Drop it. "Failed to connect through Tor" is logged only on client.connect() failure, and "Failed to obtain exit circuit" is arti's own detail for that error, which is what #610 shows — so the reported lines are still matched. The restart path cancels itself. restartArti stops Arti first, and stopArtiInternal cancels retryJob and inactivityJob. Both existing recovery paths call restartArti from inside one of those jobs, so the coroutine is cancelled between the stop and the start and Arti never comes back. That is already true on main for the start-failure and bootstrap-inactivity paths; routing a post-bootstrap outage into it would have turned a wrong indicator into a stopped Tor, which is worse. Run restarts from a separate restartJob that the stop path leaves alone, and cancel it explicitly when the user turns Tor off so a pending recovery cannot resurrect it. --- .../com/bitchat/android/net/ArtiTorManager.kt | 23 +++++++++++++++++-- .../android/net/TorCircuitHealthPolicy.kt | 14 ++++++++--- .../android/net/TorCircuitHealthPolicyTest.kt | 15 ++++++++++++ 3 files changed, 47 insertions(+), 5 deletions(-) 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