Skip to content

tor: tear down client deterministically on stop (ENV-3101) - #79

Open
Jacksper13 wants to merge 6 commits into
ENV-3010-apply-new-arti-release-arti-v250from
agent/env-3039-stop-client-teardown
Open

tor: tear down client deterministically on stop (ENV-3101)#79
Jacksper13 wants to merge 6 commits into
ENV-3010-apply-new-arti-release-arti-v250from
agent/env-3039-stop-client-teardown

Conversation

@Jacksper13

@Jacksper13 Jacksper13 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem

stop() only aborted the SOCKS accept-loop task. Everything else stayed alive until Dart's GC finalized the opaque wrappers — nondeterministically, potentially minutes later:

  • the arti TorClient itself (dirmgr, circmgr, its tokio runtime), and
  • one detached task per accepted SOCKS connection, each holding its own TorClient clone until its TCP stream closed.

A restart therefore created a second in-process client against the same tor_state directory while the zombie still held tor_cache/dir.lock. Arti's SqliteStore silently degrades to a read-only directory store in that case, so the new client could not fetch a consensus and every SOCKS CONNECT answered general server failure until GC happened to run. This matches the ENV-3039 field reports (Tor works only after staying foregrounded for one to two minutes; restarting does not help) and the ENV-3035 restart loops.

There was also a second hidden reference: TorInstance.client/TorInstance.proxy getters clone, so the TorInstance container kept an extra client reference alive until its own GC finalization even after the extracted wrappers were released.

Changes

  • start_tor now builds and owns the arti runtime (create_arti_runtime) instead of letting TokioNativeTlsRuntime::create() hide it, and parks the owning handle in TorProxyState.
  • stop_proxy aborts the accept loop, awaits it (bounded by PROXY_SHUTDOWN_TIMEOUT), then shuts the client runtime down — cancelling the per-connection tasks so even idle SOCKS connections release their TorClient.
  • Teardown failures propagate instead of being swallowed, so a caller cannot mistake an unconfirmed teardown for a clean slate. Only an unfinished accept loop counts as a failure: a loop that already ended, by its own error or a panic, has released its resources and is logged rather than reported as a teardown failure — a dead proxy is exactly when a caller's restart has to proceed.
  • Tor.stop() disposes the Rust client wrapper in a finally block instead of leaving it to GC; _startInternal disposes the TorInstance container immediately after extracting client/proxy/port.
  • CHANGELOG entry, pubspec version bump to 0.2.1, and the matching example/pubspec.lock bump.
  • Rebuilt the checked-in iOS SwiftPM XCFramework for device and simulator consumers, with framework metadata at 0.2.1.

Scope

No public API change, so the generated bindings are untouched.

Left to the broader lifecycle work, which should follow separately: the start/stop generation guard, bounding start_tor, and surfacing proxy/bootstrap errors to Dart as events.

Two follow-ups on the consumer side:

  • envoy-dev needs Foundation-Devices/envoy-dev#1409 merged alongside this. Envoy's only stop() caller wraps stop and start in a single try, so the propagation change above would otherwise skip the start and leave Tor enabled-but-never-bootstrapped — the state where isReady() parks every request forever. That PR gives the stop its own catch.
  • After this merges, envoy-dev's pubspec pin (currently b2482791, this branch's previous head) should be bumped to the new head.

Validation

  • cargo fmt --check, cargo clippy --all-targets and cargo test --locked clean
  • dart format clean at the package language version
  • Tests: stop_proxy_drops_an_idle_connection asserts the client's Arc strong count returns to baseline after stopping with an idle accepted connection; stop_proxy_succeeds_when_the_accept_loop_already_failed pins that a stale accept-loop error still tears down cleanly
  • Apple XCFramework rebuilt on macOS with the repository build script; both iOS framework plists verified at 0.2.1 and the uploaded artifact matched the committed files byte-for-byte
  • Artifact provenance: full release rebuild on GitHub macos-14-arm64 image 20260629.0180 with Rust/Cargo 1.96.0. With no repository Rust toolchain pin, this includes current dependency/toolchain output; the device binary is 274,376 bytes smaller and the simulator binary is 636,000 bytes smaller than the previous checked-in build.
  • Needs device QA: enable Tor → generate traffic → toggle/restart repeatedly, confirming the restarted client's dirmgr does not fall into read-only mode (arti logs) and requests recover promptly after restart

@Jacksper13
Jacksper13 marked this pull request as ready for review August 24, 2026 17:19
@Jacksper13
Jacksper13 requested a review from InvertedX August 24, 2026 17:19
Idle accepted SOCKS tasks retained TorClient and tor_cache/dir.lock after the accept loop stopped. Own and shut down the per-client runtime so those tasks are cancelled before Dart drops the final client wrapper. Propagate teardown failures to prevent a false-success restart.
An accept loop that ended before the abort landed is already torn down: its
listeners and TorClient clone are released. Reporting its stale error as a
teardown failure made stop() throw in exactly the state that most needs a
restart, since arti treats most accept() errors as fatal and ends the loop.

Only an unfinished task now fails teardown. A stale error or panic is logged
and reported as success.
@Jacksper13

Copy link
Copy Markdown
Contributor Author

Reviewed 085865f and pushed one fix on top as e3d7eb7.

The commit is right. Owning the arti runtime explicitly and shutting it down is the correct lever: arti spawns one detached task per accepted SOCKS connection on the client's runtime, so killing that runtime is what actually releases their TorClient clones. I checked the one thing that could have made it explode — dropping a Runtime inside an async context panics — and stop_proxy is generated as wrap_normal, so it runs on FRB's plain OS-thread pool, not a tokio worker. Safe. The test is a good one: asserting Arc::strong_count returns to baseline proves the reference is actually released rather than just that the call returned. Good catch on example/pubspec.lock too.

The bug I fixed. This arm:

Ok(Ok(result)) => result.map_err(|e| TorError::ProxyStopError(e.to_string())),

fires when the task completed before the abort landed — i.e. the accept loop had already died on its own. That is not hypothetical: arti's run_proxy_with_listeners treats every accept() error other than EMFILE/ENFILE as fatal and returns Err, and a failed per-connection spawn ends it too.

In that state teardown has genuinely succeeded — the task is finished, its listeners and client clone are already dropped — yet stop_proxy reported failure. A dead proxy is exactly when a caller's restart has to proceed, so this inverted the recovery logic: the one state that most needs a restart was the state that blocked it.

Now only an unfinished task fails teardown. A stale accept-loop error or a panic is logged via log::warn! and reported as success, so the diagnostic survives without blocking recovery. Added stop_proxy_succeeds_when_the_accept_loop_already_failed to pin the invariant.

Envoy-side follow-up: Foundation-Devices/envoy-dev#1409 now guards against the propagation change. Envoy's only stop() caller wraps stop and start in one try, so a throwing stop skipped the start and left Tor enabled-but-never-bootstrapped — the state where isReady() parks every request forever. That PR gives the stop its own catch and starts the replacement route regardless. Worth merging alongside this one.

PR description is stale — it still lists the detached per-connection tasks as a known residual left to later lifecycle work, which 085865f fixes. I'll rewrite that paragraph so reviewers don't think the hole is still open.

Validation on the current head: cargo fmt --check, cargo clippy --all-targets and cargo test --locked (2/2) all clean, and dart format clean — that is every gate in check.yml except flutter analyze, which needs a Flutter toolchain I don't have here.

The Dart package advanced to 0.2.1 while the native crate and generated framework metadata stayed at 0.2.0. Make Cargo authoritative so rebuilt SwiftPM binaries carry the release version without another synchronized constant.
Refresh the checked-in iOS device and simulator binaries so SwiftPM consumers receive the deterministic proxy teardown implementation. Record crate version 0.2.1 in both framework bundles.
The separately owned Tokio runtime made the first iOS SOCKS route accept requests but fail every circuit immediately. Keep Arti's standard owned runtime and cancel its tracked tasks explicitly so restart teardown still releases accepted connections and dir.lock.
@Jacksper13 Jacksper13 changed the title tor: tear down client deterministically on stop tor: tear down client deterministically on stop (ENV-3101) Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant