fix: stop showing Tor as connected after its circuits stop working - #886
fix: stop showing Tor as connected after its circuits stop working#886Chessing234 wants to merge 3 commits into
Conversation
Arti's bootstrap milestones only fire on the way up. "We have found that guard [scrubbed] is usable." latches the session to RUNNING at 100% and calls stopInactivityMonitoring(), and from there nothing downgrades the status except a deliberate stop or a state-file lock conflict. So when the network goes away after bootstrap, Arti logs a continuous stream of "Failed to connect through Tor" / "SOCKS connection error" while the header keeps showing the connected colour, which is gated on running && bootstrapPercent >= 100. isProxyEnabled() agrees, so requests keep being routed into a SOCKS port that cannot reach an exit. Track circuit failures after bootstrap and drop the published status below 100% once they both accumulate and persist. Individual failures are a normal part of Tor, so a burst inside one instant — which is what the report shows, three threads failing in the same millisecond — is ignored; it takes four failures still going twenty seconds later. Downgrading to BOOTSTRAPPING rather than ERROR keeps running = true, so callers fail closed through awaitSelectedRoute instead of leaking to clearnet, the inactivity watchdog re-arms now that bootstrapPercent is below 100, and the existing backoff retry gets to re-establish the session. A successful re-bootstrap clears the tally and restores RUNNING. The policy is kept free of Android and coroutine dependencies so the thresholds are directly testable. Refs permissionlesstech#610
Uses the log lines from the report verbatim. Pins both directions: a same-instant burst and occasional failures far apart leave the indicator alone, while four failures still going after twenty seconds downgrade it. Also covers reset and a backwards clock jump.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e65fce66d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the same style as the bootstrap milestones in ArtiTorManager. | ||
| val FAILURE_MARKERS = listOf( | ||
| "Failed to connect through Tor", | ||
| "SOCKS connection error", |
There was a problem hiding this comment.
This marker treats every SOCKS connection error line as a dead Tor circuit, but the native SOCKS server emits that wrapper for any error returned by handle_socks_connection (tools/arti-build/src/lib.rs:284), including malformed or unsupported SOCKS requests before client.connect is reached (tools/arti-build/src/lib.rs:317-342). After Tor is running, four such local SOCKS errors over 20 seconds will drive this policy into BOOTSTRAPPING/restart even while circuits are healthy; match the exit-circuit text or the Failed to connect through Tor line instead.
Useful? React with 👍 / 👎.
| startInactivityMonitoring() | ||
| currentApplication?.let { scheduleRetry(it) } |
There was a problem hiding this comment.
In this new post-bootstrap failure path, both automatic recovery paths invoke restartArti from retryJob or inactivityJob, but restartArti first calls stopArtiAndWait, whose stopArtiInternal cancels both stored jobs via stopRetryMonitoring() and stopInactivityMonitoring(). When the caller is the retry/watchdog coroutine, it cancels its own job before the later delay/start, so a sustained circuit outage drops the status below 100% and stops the SOCKS proxy but never starts it again until a manual toggle; launch the restart from a separate job or avoid cancelling the current one.
Useful? React with 👍 / 👎.
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 permissionlesstech#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.
|
both fair. dropped the "SOCKS connection error" marker — arti-build wraps every handle_socks_connection error in it, including invalid handshake/version/command before a circuit is attempted. the two remaining markers still match the lines in #610. the restart one is worse than it looks and predates this pr: restartArti stops arti first, and stopArtiInternal cancels retryJob and inactivityJob, so both existing recovery paths cancel themselves between the stop and the start. routing a post-bootstrap outage into that would have traded a wrong icon for a stopped tor. restarts now run on a separate job the stop path leaves alone, cancelled explicitly when tor is switched off. |
1 participant
Refs #610.
What
Arti's bootstrap milestones only fire on the way up.
handleArtiLogLinelatches the session on"We have found that guard [scrubbed] is usable.":After that, nothing downgrades the status except a deliberate stop or the
"Another process has the lock on our state files"line. There is no post-bootstrap health check at all —armBootstrapInactivityWatchdog()returns immediately oncebootstrapPercent >= 100.So when the network goes away after bootstrap, exactly what #610 reports happens: Arti logs a continuous stream of
while the header keeps showing the connected colour —
ChatHeader.kt:169gates it onrunning && bootstrapPercent >= 100.isProxyEnabled()agrees, soawaitSelectedRoutereturns straight away and requests keep being routed into a SOCKS port that cannot reach an exit.Fix
Track circuit failures after bootstrap and drop the published status below 100% once they both accumulate and persist.
Circuit failures are a normal part of Tor, so a burst inside one instant is ignored — the report shows three threads failing in the same millisecond. It takes 4 failures that are still going 20 seconds later.
Downgrading to
BOOTSTRAPPINGrather thanERRORis deliberate:runningstays true, so callers fail closed throughawaitSelectedRouteinstead of leaking to clearnet,bootstrapPercent < 100,RUNNING.The thresholds live in
TorCircuitHealthPolicy, kept free of Android and coroutine dependencies so they are directly testable.Evidence
TorCircuitHealthPolicyTest, 9 tests, using the log lines from the issue verbatim. Pins both directions — a same-instant burst and occasional failures far apart leave the indicator alone; four failures still going after twenty seconds downgrade it — plus reset and a backwards clock jump.Measured the same way on
mainand here, the only difference is the new class: 90 classes / 591 tests → 91 / 600. Lint unchanged frommain(305 errors, 333 warnings, 17 hints, all pre-existing/baselined).What I could not verify
I have no device that reproduces #610, so the wiring inside
ArtiTorManager— that these Arti lines reachhandleArtiLogLineand that the indicator visibly turns from connected to connecting — is reasoned from the code and the reporter's log, not observed. The policy itself is covered by tests. The failure markers are matched as substrings of Arti's output, so if Arti changes that wording the detector silently stops firing; that is the main risk in this change and it fails safe (back to today's behaviour).Happy to adjust the 4-failure / 20-second thresholds if you'd rather have them tighter or looser.