Narrow the failure marker and fix the self-cancelling restart

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.
This commit is contained in:
Taksh 2026-08-16 09:25:11 +05:30
parent e65fce66d0
commit dfc2b8dcd1
3 changed files with 47 additions and 5 deletions

View File

@ -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 {

View File

@ -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"
)
}

View File

@ -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