From e076b1eaee39d2908f9e275d91c05054807bb641 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:55:22 +0000 Subject: [PATCH 1/7] fix(media): stop a no-imeta GIF rendering as an invisible note while it loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GifVideoView's Loading branch emitted DisplayBlurHash unconditionally, and DisplayBlurHash renders nothing at all when both hashes are absent — placeholderModel(null, null) returns null and the composable early-returns. That is exactly the state a post with no imeta lands in. With no `dim` tag and nothing in MediaAspectRatioCache, `ratio` is null, so mediaSizingModifier falls to a bare fillMaxWidth() with no height constraint. The container then wraps an empty loading state and the whole note collapses to zero height: no picture, no URL, no spinner, just a gap in the feed for however long the fetch takes, and then the image appearing from nowhere. Seen on a kind-1111 comment from Sidecar whose content is a single blossom .gif URL. UrlImageView already has the ladder this needs, so mirror it: blurhash/thumbhash when there is one, a spinner in the reserved box when only a ratio is known, and otherwise the URL plus a loading symbol via WaitAndDisplay. Every branch now emits something with a height. This is the missing feedback, not the latency — a slow fetch still takes as long as it takes, it just stops being invisible while it does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5 --- .../amethyst/ui/components/GifVideoView.kt | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt index 6620020e49..f62f7ede81 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt @@ -47,10 +47,13 @@ import coil3.compose.SubcomposeAsyncImage import coil3.compose.SubcomposeAsyncImageContent import com.vitorpamplona.amethyst.commons.resources.Res import com.vitorpamplona.amethyst.commons.resources.gif +import com.vitorpamplona.amethyst.commons.ui.components.LoadingAnimation import com.vitorpamplona.amethyst.model.MediaAspectRatioCache import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.theme.Font10SP +import com.vitorpamplona.amethyst.ui.theme.Size40dp +import com.vitorpamplona.amethyst.ui.theme.Size6dp import com.vitorpamplona.amethyst.ui.theme.SmallBorder import com.vitorpamplona.amethyst.ui.theme.imageModifier import com.vitorpamplona.quartz.nip94FileMetadata.tags.DimensionTag @@ -108,13 +111,31 @@ fun GifVideoView( when (state) { is AsyncImagePainter.State.Loading -> { - DisplayBlurHash( - blurhash, - contentDescription, - contentScale, - Modifier.fillMaxSize(), - thumbhash = thumbhash, - ) + // Every branch here MUST emit something with a height. [containerModifier] + // only constrains the height when `ratio` is known (imeta `dim` or a cached + // ratio); without one it is a bare fillMaxWidth(), so the box wraps its + // content and an empty loading state collapses the whole note to zero + // height -- no picture, no URL, no spinner, just a gap in the feed until + // the load finishes. DisplayBlurHash renders NOTHING when both hashes are + // absent (placeholderModel returns null), which is exactly the case a + // no-imeta post hits. Mirrors UrlImageView's ladder. + if (blurhash != null || thumbhash != null) { + DisplayBlurHash( + blurhash, + contentDescription, + contentScale, + Modifier.fillMaxSize(), + thumbhash = thumbhash, + ) + } else if (ratio != null) { + Box(Modifier.fillMaxSize(), contentAlignment = Alignment.Center) { + LoadingAnimation(Size40dp, Size6dp) + } + } else { + WaitAndDisplay { + DisplayUrlWithLoadingSymbol(videoUri) + } + } } is AsyncImagePainter.State.Success -> { From 943e137d4e0a698505d1eff6caa8ac701c4682d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:55:41 +0000 Subject: [PATCH 2/7] perf(http): stop wiping the media connection pool, and size the dispatcher for a phone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems in the shared non-relay client, both of which make every HTTP request cost more than it should. The connection pool was being emptied constantly. buildHttpClient() evicted the whole pool whenever the proxy differed from the last one it was handed, tracked in a single field. But one factory mints BOTH long-lived variants: DualHttpClientManager builds defaultHttpClient (always SOCKS, since buildLocalSocksProxy falls back to 9050 rather than returning null) and defaultHttpClientWithoutProxy (always null) from the same instance, and the two share one rootClient.connectionPool. So the field alternated between the proxy and null forever, and every rebuild read as a route change and wiped the pool they share. Both are stateIn(WhileSubscribed(1000)) flows collected from a composable, so that happened on every isMobileDataProvider change and every foreground round trip — and the next image then paid a fresh DNS + TCP + TLS. This is a plausible source of the "first image after a pause takes forever" stall the pingInterval above it was added for. ProxyRouteTracker narrows it to what the eviction was actually for: a direct build never evicts (null is that variant's permanent route), and a proxied build evicts only when the Tor port really moved. Nothing is lost by being this narrow — OkHttp's Address, the pool key, already includes the proxy, so proxied and direct connections to the same host are distinct entries that can never be handed to each other's calls. Second, maxRequests was 128. Dispatcher's executor is an unbounded cached pool, so that is the thread ceiling: up to 128 threads running 128 concurrent TLS handshakes on a handset. Nothing upstream bounds the arrival rate either — Coil's enqueue is unbounded and PrefetchFeedMedia warms ±3 notes on both sides of the viewport on every visible-range change — so a fast scroll really does reach it. Past saturation, more concurrency slices the same bandwidth thinner and pushes every image's completion out together, the on-screen one included. 32 total / 8 per host keeps the per-host lift that feeds need while letting visible images finish and paint. The pool fix is covered by tests. The dispatcher numbers are a reasoned choice, not a measured one — worth a run against benchmark/ before release. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5 --- .../service/http/OkHttpClientFactory.kt | 63 +++++++++++-- .../service/http/ProxyRouteTrackerTest.kt | 90 +++++++++++++++++++ 2 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt index 01a9ed7b5c..10d9d2fa4d 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt @@ -84,13 +84,24 @@ class OkHttpClientFactory( // Most images/videos in a feed come from a small set of hosts (e.g. a single // Blossom/imgproxy server). OkHttp's default dispatcher caps inflight requests - // per host at 5, which serializes feed loading. Raise the limits so the feed - // can parallelize downloads the way a browser does. + // per host at 5, which serializes feed loading, so we lift that. + // + // The TOTAL, though, has to stay in phone territory. Dispatcher's executor is + // an unbounded cached pool (SynchronousQueue), so maxRequests is literally the + // thread ceiling: 128 meant up to 128 threads doing 128 concurrent TLS + // handshakes on a handset. Nothing upstream bounds the arrival rate either -- + // Coil's enqueue is unbounded and PrefetchFeedMedia warms ±3 notes on BOTH + // sides of the viewport on every visible-range change -- so the queue really + // does reach the cap on a fast scroll. Past the point where the radio and the + // CPU are saturated, more concurrency doesn't add throughput, it just slices + // the same bandwidth thinner and pushes every image's completion out + // together, including the one actually on screen. A tighter total lets the + // visible images finish and paint while the rest wait their turn. private val dispatcher = Dispatcher().apply { if (!HttpClientEnvironment.isEmulator) { - maxRequestsPerHost = 16 - maxRequests = 128 + maxRequestsPerHost = 8 + maxRequests = 32 } else { maxRequestsPerHost = 5 maxRequests = 64 @@ -138,15 +149,14 @@ class OkHttpClientFactory( .addInterceptor(OnionLocationInterceptor(onionCache)) .build() - private var lastProxy: Proxy? = null + private val proxyRoutes = ProxyRouteTracker() fun buildHttpClient( proxy: Proxy?, timeoutSeconds: Int, ): OkHttpClient { - if (proxy != lastProxy) { + if (proxyRoutes.shouldEvictFor(proxy)) { rootClient.connectionPool.evictAll() - lastProxy = proxy } val seconds = if (proxy != null) timeoutSeconds * 3 else timeoutSeconds return rootClient @@ -193,3 +203,42 @@ class OkHttpClientFactory( const val HTTP2_PING_INTERVAL_SECS: Long = 10 } } + +/** + * Decides when a rebuilt client must drop the pooled connections it shares with every other + * client [OkHttpClientFactory] mints. + * + * The eviction exists so a changed Tor route doesn't leave usable connections behind on the old + * one. The trap is that a single factory mints BOTH long-lived variants — [DualHttpClientManager] + * builds `defaultHttpClient` (always SOCKS, because `buildLocalSocksProxy` falls back to 9050 + * rather than returning null) and `defaultHttpClientWithoutProxy` (always null) from the same + * instance, and they share one `rootClient.connectionPool`. Comparing every build against one + * "last proxy" field therefore saw the two variants alternate forever: each rebuild looked like a + * route change and wiped the pool they share. Both `stateIn` flows re-emit on every + * `isMobileDataProvider` change and every resubscribe (they are `WhileSubscribed(1000)`, collected + * from a composable), so in practice the pool was emptied whenever the network flapped or the app + * came back to the foreground — and the next image then paid a fresh DNS + TCP + TLS. + * + * Two rules fix it: + * + * - A direct build (`proxy == null`) never evicts. `null` is that variant's permanent route, so + * it can never have changed. + * - A proxied build evicts only when the proxy differs from the one the PREVIOUS proxied build + * used, i.e. the Tor port actually moved. + * + * Nothing is lost by being this narrow: OkHttp's `Address` — the connection-pool key — includes + * the proxy, so a direct connection and a SOCKS connection to the same host are already distinct + * entries that can never be handed to each other's calls. + */ +internal class ProxyRouteTracker { + private var lastProxy: Proxy? = null + + @Synchronized + fun shouldEvictFor(proxy: Proxy?): Boolean { + if (proxy == null) return false + + val previous = lastProxy + lastProxy = proxy + return previous != null && previous != proxy + } +} diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt new file mode 100644 index 0000000000..99df3b3c71 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.service.http + +import java.net.InetSocketAddress +import java.net.Proxy +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class ProxyRouteTrackerTest { + private fun socks(port: Int) = Proxy(Proxy.Type.SOCKS, InetSocketAddress("127.0.0.1", port)) + + @Test + fun firstProxiedBuildDoesNotEvict() { + // Nothing is pooled on the old route because there is no old route. + assertFalse(ProxyRouteTracker().shouldEvictFor(socks(9050))) + } + + @Test + fun directBuildNeverEvicts() { + val tracker = ProxyRouteTracker() + + assertFalse(tracker.shouldEvictFor(null)) + assertFalse(tracker.shouldEvictFor(null)) + } + + @Test + fun sameProxyRebuiltDoesNotEvict() { + val tracker = ProxyRouteTracker() + tracker.shouldEvictFor(socks(9050)) + + // A fresh-but-equal Proxy is what every rebuild hands us: buildLocalSocksProxy + // allocates a new instance each time, so this must compare by value. + assertFalse(tracker.shouldEvictFor(socks(9050))) + } + + @Test + fun changedProxyPortEvicts() { + val tracker = ProxyRouteTracker() + tracker.shouldEvictFor(socks(9050)) + + assertTrue(tracker.shouldEvictFor(socks(9150))) + } + + /** + * The regression this class exists for. [DualHttpClientManager] mints both variants from one + * factory — `defaultHttpClient` always proxied, `defaultHttpClientWithoutProxy` always direct + * — and both `stateIn` flows re-emit on every `isMobileDataProvider` change and every + * resubscribe. A single "last proxy" field saw that alternation as a route change every time + * and wiped the connection pool the two variants SHARE. + */ + @Test + fun alternatingBetweenProxiedAndDirectNeverEvicts() { + val tracker = ProxyRouteTracker() + + repeat(10) { + assertFalse(tracker.shouldEvictFor(socks(9050))) + assertFalse(tracker.shouldEvictFor(null)) + } + } + + @Test + fun directBuildsBetweenProxyChangesDoNotMaskTheChange() { + val tracker = ProxyRouteTracker() + tracker.shouldEvictFor(socks(9050)) + tracker.shouldEvictFor(null) + + // The direct build in the middle must not reset what the proxied variant last used. + assertTrue(tracker.shouldEvictFor(socks(9150))) + } +} From add7bfe3ca009f8fe13ab9afadd0dbe5341d6a86 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 22:52:59 +0000 Subject: [PATCH 3/7] fix(http): close the read-auth single-flight window left open before putIfAbsent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit signOnce() looked at the cache a second time to catch a leader that had already finished, but it looked too early — before claiming the in-flight slot. The gap between that look and putIfAbsent still spans signerProvider() and an allocation, and a leader caches its token and retires its entry inside it. A straggler in that gap therefore put into a map the leader had just emptied, won the slot, and signed a duplicate. Winning the slot is not proof that nobody signed; only a look from inside it is. Once we hold the entry no one else can be leader, and our successful put observed the map after that leader's removal, which its cache write is ordered before — so a token visible at that point is the last word. Hand it over and stand down. This is pre-existing, not fallout from the dispatcher change: the same test fails on an unmodified origin/main worktree, and aFastSignerStillSharesOneSignature already says the window is "microseconds wide, so one round hits it only now and then" and runs 200 rounds to catch it. It cost a duplicate signature — with a NIP-55 external signer that is a second IPC round trip, and potentially a second prompt, for a burst of images from one gated host. Verified with 8 consecutive runs of the suite (1600 signing rounds), all green, against a baseline that failed inside the first run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5 --- .../service/http/BlossomReadAuthTokenProvider.kt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt index 7c635ce442..4f1b629477 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt @@ -129,6 +129,20 @@ class BlossomReadAuthTokenProvider( val fresh = CompletableDeferred() inFlight.putIfAbsent(host, fresh)?.let { return it } + // Winning the slot is not proof that nobody signed. The look above only narrows + // the gap — a leader that finished between it and this putIfAbsent has already + // cached its token AND retired its entry, so this put landed in a map it had + // just emptied and we would sign a duplicate. Only a look from *inside* the slot + // closes it: no one else can be leader while we hold the entry, and our put + // observed the map after that leader's removal, which its cache write is ordered + // before. So a token visible here is the last word, and the right move is to hand + // it over and stand down rather than sign again. + cachedHeader(host)?.let { cached -> + inFlight.remove(host, fresh) + fresh.complete(cached) + return fresh + } + scope .launch { val header = From 64e0ce4b3e30f2c2f133d01c7b617fb30be2e88e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 00:46:21 +0000 Subject: [PATCH 4/7] revert(http): restore the dispatcher limits, and record why they stay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I lowered these to 32/8 on reasoning that does not survive reading the source. `maxRequests` is the thread ceiling — Dispatcher's executor really is corePoolSize=0 / maxPoolSize=MAX_VALUE over a SynchronousQueue, and its own kdoc notes a pool sized exactly to maxRequests is not even sufficient. That much was right. The conclusions drawn from it were not: - "128 concurrent TLS handshakes" is wrong. These hosts are HTTP/2, so concurrent calls to one host multiplex over a single connection; handshake count is bounded by distinct hosts and the pool, not by maxRequests. - "A tighter total lets the visible images finish first" is backwards. promoteAndExecute walks readyAsyncCalls as a strict FIFO with no priority, and PrefetchFeedMedia enqueues notes before the user reaches them — so a lower cap makes the on-screen image queue behind those prefetches rather than start immediately. That is the very symptom under investigation. - Halving maxRequestsPerHost also halves HTTP/2 stream concurrency against the single Blossom host a feed pulls from, which is what these were tuned for. What is left is thread memory, and blocked threads commit little. No measurement justified the change, so the values go back as they were. The comment now carries the analysis so the next reader does not re-derive the same wrong intuition. The ProxyRouteTracker fix from the same commit is unaffected and stands. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5 --- .../service/http/OkHttpClientFactory.kt | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt index 10d9d2fa4d..4cbd2fd6a4 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt @@ -84,24 +84,23 @@ class OkHttpClientFactory( // Most images/videos in a feed come from a small set of hosts (e.g. a single // Blossom/imgproxy server). OkHttp's default dispatcher caps inflight requests - // per host at 5, which serializes feed loading, so we lift that. + // per host at 5, which serializes feed loading. Raise the limits so the feed + // can parallelize downloads the way a browser does. // - // The TOTAL, though, has to stay in phone territory. Dispatcher's executor is - // an unbounded cached pool (SynchronousQueue), so maxRequests is literally the - // thread ceiling: 128 meant up to 128 threads doing 128 concurrent TLS - // handshakes on a handset. Nothing upstream bounds the arrival rate either -- - // Coil's enqueue is unbounded and PrefetchFeedMedia warms ±3 notes on BOTH - // sides of the viewport on every visible-range change -- so the queue really - // does reach the cap on a fast scroll. Past the point where the radio and the - // CPU are saturated, more concurrency doesn't add throughput, it just slices - // the same bandwidth thinner and pushes every image's completion out - // together, including the one actually on screen. A tighter total lets the - // visible images finish and paint while the rest wait their turn. + // Resist trimming these on intuition. `maxRequests` is effectively the thread + // ceiling (Dispatcher's executor is corePoolSize=0 / maxPoolSize=MAX_VALUE over + // a SynchronousQueue), which makes a lower number look free -- but blocked + // threads commit little, these hosts are HTTP/2 so concurrent calls to one host + // multiplex over a single connection rather than a handshake each, and + // `readyAsyncCalls` is strict FIFO with no priority. PrefetchFeedMedia enqueues + // notes BEFORE the user reaches them, so a tighter cap makes the image actually + // on screen queue behind those prefetches instead of starting straight away. + // Change these with a benchmark/ run, not a hunch. private val dispatcher = Dispatcher().apply { if (!HttpClientEnvironment.isEmulator) { - maxRequestsPerHost = 8 - maxRequests = 32 + maxRequestsPerHost = 16 + maxRequests = 128 } else { maxRequestsPerHost = 5 maxRequests = 64 From 9e380cf9dc9da8e30ec643545aa3b3814b3c244b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 01:43:43 +0000 Subject: [PATCH 5/7] perf(http): drop the proxy-change pool eviction in both OkHttp factories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both factories emptied the whole connection pool whenever the proxy differed from the last one they were handed. It was defensive, and it was not free. It is not needed. OkHttp keys the pool by `Address`, and `Address.equalsNonHost` compares `proxy` — so a call is only ever given a connection opened through the very same route. A connection left over from an old proxy is already unreachable by anything using the new one; it just ages out of the pool on its own. There was never a stale-route connection to protect against. The cost was real, though. `evictAll()` empties the ENTIRE shared pool, and each factory mints BOTH clients: `DualHttpClientManager` builds defaultHttpClient (always SOCKS, since buildLocalSocksProxy falls back to 9050 rather than returning null) and defaultHttpClientWithoutProxy (always null) from one instance, and they share one `rootClient.connectionPool`. A single "last proxy" field therefore alternated forever, and every rebuild read as a route change and dropped every warm connection the other client was relying on. Both are stateIn(WhileSubscribed(1000)) flows collected from a composable, so it fired on each isMobileDataProvider change and each foreground round trip — and the next image then paid a fresh DNS + TCP + TLS. Plausibly the "first image after a pause takes forever" stall the pingInterval above it was added for. DualHttpClientManagerForRelays has the identical shape, so the relay factory is fixed the same way. Less damaging there — evictAll spares connections with active calls, so live relay sockets survived — but it was still discarding idle pooled connections on every network flap. This replaces the ProxyRouteTracker approach from earlier on this branch, which kept the eviction and merely made its bookkeeping correct. Deleting the mechanism is the better answer, and it takes the class and its tests with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5 --- .../service/http/OkHttpClientFactory.kt | 56 +++--------- .../http/OkHttpClientFactoryForRelays.kt | 18 ++-- .../service/http/ProxyRouteTrackerTest.kt | 90 ------------------- 3 files changed, 24 insertions(+), 140 deletions(-) delete mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt index 4cbd2fd6a4..2fb46eb1c6 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt @@ -148,15 +148,22 @@ class OkHttpClientFactory( .addInterceptor(OnionLocationInterceptor(onionCache)) .build() - private val proxyRoutes = ProxyRouteTracker() - + // No connection-pool eviction when the proxy changes. OkHttp's `Address` -- the + // pool's lookup key -- includes the proxy (`Address.equalsNonHost`), so a call + // is only ever handed a connection opened through the very same route. A + // connection left over from an old proxy is already unreachable and simply ages + // out of the pool; evicting was defensive, not load-bearing. + // + // It also cost more than it looked. `evictAll()` empties the ENTIRE shared pool, + // and this one factory mints both the proxied and the direct client (see + // [DualHttpClientManager]) -- `buildLocalSocksProxy` never returns null, so those + // two alternated a single "last proxy" field forever. Every rebuild read as a + // route change and dropped every warm connection the other client was using, on + // each network-state emission and each resubscribe. fun buildHttpClient( proxy: Proxy?, timeoutSeconds: Int, ): OkHttpClient { - if (proxyRoutes.shouldEvictFor(proxy)) { - rootClient.connectionPool.evictAll() - } val seconds = if (proxy != null) timeoutSeconds * 3 else timeoutSeconds return rootClient .newBuilder() @@ -202,42 +209,3 @@ class OkHttpClientFactory( const val HTTP2_PING_INTERVAL_SECS: Long = 10 } } - -/** - * Decides when a rebuilt client must drop the pooled connections it shares with every other - * client [OkHttpClientFactory] mints. - * - * The eviction exists so a changed Tor route doesn't leave usable connections behind on the old - * one. The trap is that a single factory mints BOTH long-lived variants — [DualHttpClientManager] - * builds `defaultHttpClient` (always SOCKS, because `buildLocalSocksProxy` falls back to 9050 - * rather than returning null) and `defaultHttpClientWithoutProxy` (always null) from the same - * instance, and they share one `rootClient.connectionPool`. Comparing every build against one - * "last proxy" field therefore saw the two variants alternate forever: each rebuild looked like a - * route change and wiped the pool they share. Both `stateIn` flows re-emit on every - * `isMobileDataProvider` change and every resubscribe (they are `WhileSubscribed(1000)`, collected - * from a composable), so in practice the pool was emptied whenever the network flapped or the app - * came back to the foreground — and the next image then paid a fresh DNS + TCP + TLS. - * - * Two rules fix it: - * - * - A direct build (`proxy == null`) never evicts. `null` is that variant's permanent route, so - * it can never have changed. - * - A proxied build evicts only when the proxy differs from the one the PREVIOUS proxied build - * used, i.e. the Tor port actually moved. - * - * Nothing is lost by being this narrow: OkHttp's `Address` — the connection-pool key — includes - * the proxy, so a direct connection and a SOCKS connection to the same host are already distinct - * entries that can never be handed to each other's calls. - */ -internal class ProxyRouteTracker { - private var lastProxy: Proxy? = null - - @Synchronized - fun shouldEvictFor(proxy: Proxy?): Boolean { - if (proxy == null) return false - - val previous = lastProxy - lastProxy = proxy - return previous != null && previous != proxy - } -} diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt index c547a62e73..0b931b08c6 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt @@ -86,16 +86,22 @@ class OkHttpClientFactoryForRelays( .addInterceptor(OnionLocationInterceptor(onionCache)) .build() - private var lastProxy: Proxy? = null - + // No connection-pool eviction when the proxy changes. OkHttp's `Address` -- the + // pool's lookup key -- includes the proxy (`Address.equalsNonHost`), so a call + // is only ever handed a connection opened through the very same route. A + // connection left over from an old proxy is already unreachable and simply ages + // out of the pool; evicting was defensive, not load-bearing. + // + // It also cost more than it looked. `evictAll()` empties the ENTIRE shared pool, + // and this one factory mints both the proxied and the direct client (see + // [DualHttpClientManagerForRelays]) -- `buildLocalSocksProxy` never returns null, so those + // two alternated a single "last proxy" field forever. Every rebuild read as a + // route change and dropped every warm connection the other client was using, on + // each network-state emission and each resubscribe. fun buildHttpClient( proxy: Proxy?, timeoutSeconds: Int, ): OkHttpClient { - if (proxy != lastProxy) { - rootClient.connectionPool.evictAll() - lastProxy = proxy - } val seconds = if (proxy != null) timeoutSeconds * 3 else timeoutSeconds return rootClient .newBuilder() diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt deleted file mode 100644 index 99df3b3c71..0000000000 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright (c) 2025 Vitor Pamplona - * - * Permission is hereby granted, free of charge, to any person obtaining a copy of - * this software and associated documentation files (the "Software"), to deal in - * the Software without restriction, including without limitation the rights to use, - * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the - * Software, and to permit persons to whom the Software is furnished to do so, - * subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS - * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR - * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN - * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION - * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. - */ -package com.vitorpamplona.amethyst.commons.service.http - -import java.net.InetSocketAddress -import java.net.Proxy -import kotlin.test.Test -import kotlin.test.assertFalse -import kotlin.test.assertTrue - -class ProxyRouteTrackerTest { - private fun socks(port: Int) = Proxy(Proxy.Type.SOCKS, InetSocketAddress("127.0.0.1", port)) - - @Test - fun firstProxiedBuildDoesNotEvict() { - // Nothing is pooled on the old route because there is no old route. - assertFalse(ProxyRouteTracker().shouldEvictFor(socks(9050))) - } - - @Test - fun directBuildNeverEvicts() { - val tracker = ProxyRouteTracker() - - assertFalse(tracker.shouldEvictFor(null)) - assertFalse(tracker.shouldEvictFor(null)) - } - - @Test - fun sameProxyRebuiltDoesNotEvict() { - val tracker = ProxyRouteTracker() - tracker.shouldEvictFor(socks(9050)) - - // A fresh-but-equal Proxy is what every rebuild hands us: buildLocalSocksProxy - // allocates a new instance each time, so this must compare by value. - assertFalse(tracker.shouldEvictFor(socks(9050))) - } - - @Test - fun changedProxyPortEvicts() { - val tracker = ProxyRouteTracker() - tracker.shouldEvictFor(socks(9050)) - - assertTrue(tracker.shouldEvictFor(socks(9150))) - } - - /** - * The regression this class exists for. [DualHttpClientManager] mints both variants from one - * factory — `defaultHttpClient` always proxied, `defaultHttpClientWithoutProxy` always direct - * — and both `stateIn` flows re-emit on every `isMobileDataProvider` change and every - * resubscribe. A single "last proxy" field saw that alternation as a route change every time - * and wiped the connection pool the two variants SHARE. - */ - @Test - fun alternatingBetweenProxiedAndDirectNeverEvicts() { - val tracker = ProxyRouteTracker() - - repeat(10) { - assertFalse(tracker.shouldEvictFor(socks(9050))) - assertFalse(tracker.shouldEvictFor(null)) - } - } - - @Test - fun directBuildsBetweenProxyChangesDoNotMaskTheChange() { - val tracker = ProxyRouteTracker() - tracker.shouldEvictFor(socks(9050)) - tracker.shouldEvictFor(null) - - // The direct build in the middle must not reset what the proxied variant last used. - assertTrue(tracker.shouldEvictFor(socks(9150))) - } -} From a2b0fd9405cb189bcdc85656039e2fb6162571ab Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 02:42:06 +0000 Subject: [PATCH 6/7] fix(http): drop pooled connections once per real Tor route change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing the per-rebuild eviction left one sliver: when the user switches Tor ON, the direct client's idle sockets to real hosts stayed pooled for the 5-minute keepalive. Nothing could route a request through them — OkHttp keys the pool by `Address`, which includes the proxy — but they are real connections to real hosts outliving the moment the user asked for everything to go through Tor. [evictOnProxyRouteChange] closes that by watching the one signal that means the route actually moved: `torManager.activePortOrNull`. Tor coming up (null -> 9050), going away (9050 -> null), or moving (9050 -> 9150) each evict exactly once. `drop(1)` keeps subscribing from counting as a change. Deliberately not driven by the two things that misled the old code: - Client rebuilds. One factory mints both the proxied and the direct client and they share a pool, so a per-rebuild check fired on every isMobileDataProvider emission and every resubscribe — constantly, and never specifically on a Tor toggle. - The per-feature Tor switches (imagesViaTor, videosViaTor, …). Those change which of the two existing clients a request picks, not the route either one uses, so no pooled connection goes stale. Wired in both managers' init, so the media and relay pools behave identically. Six tests cover the trigger, including that re-emitting the same port never evicts — the regression the old design had. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5 --- .../service/http/DualHttpClientManager.kt | 6 + .../http/DualHttpClientManagerForRelays.kt | 6 + .../service/http/OkHttpClientFactory.kt | 7 + .../http/OkHttpClientFactoryForRelays.kt | 7 + .../commons/service/http/ProxyRouteChange.kt | 62 ++++++++ .../service/http/ProxyRouteChangeTest.kt | 135 ++++++++++++++++++ 6 files changed, 223 insertions(+) create mode 100644 commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt index 8b13761f03..6fa4af9c44 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt @@ -56,6 +56,12 @@ class DualHttpClientManager( ) : IHttpClientManager { val factory = OkHttpClientFactory(keyCache, userAgent, dns, shouldBridgeBlossomCache, onionCache, usageInterceptor, blossomReadAuth) + init { + // One eviction per real Tor route change. See [evictOnProxyRouteChange] for why this is + // driven by the port rather than by client rebuilds. + scope.evictOnProxyRouteChange(proxyPortProvider, factory::evictPooledConnections) + } + val defaultHttpClient: StateFlow = combine(proxyPortProvider, isMobileDataProvider) { proxy, mobile -> factory.buildHttpClient(proxy, mobile) diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt index 77b844b0d9..aade203faf 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt @@ -41,6 +41,12 @@ class DualHttpClientManagerForRelays( ) : IHttpClientManager { val factory = OkHttpClientFactoryForRelays(userAgent, dns, onionCache) + init { + // One eviction per real Tor route change. See [evictOnProxyRouteChange] for why this is + // driven by the port rather than by client rebuilds. + scope.evictOnProxyRouteChange(proxyPortProvider, factory::evictPooledConnections) + } + val defaultHttpClient: StateFlow = combine(proxyPortProvider, isMobileDataProvider) { proxy, mobile -> factory.buildHttpClient(proxy, mobile) diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt index 2fb46eb1c6..0e9fd0892f 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt @@ -178,6 +178,13 @@ class OkHttpClientFactory( .build() } + /** + * Closes every idle pooled connection. Call only on a real proxy-route change (see + * [evictOnProxyRouteChange]) — connections on a dead route are already unreachable, so this + * is hygiene, not correctness, and it empties the pool BOTH clients share. + */ + fun evictPooledConnections() = rootClient.connectionPool.evictAll() + fun buildHttpClient( localSocksProxyPort: Int?, isMobile: Boolean?, diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt index 0b931b08c6..319fa34723 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt @@ -114,6 +114,13 @@ class OkHttpClientFactoryForRelays( .build() } + /** + * Closes every idle pooled connection. Call only on a real proxy-route change (see + * [evictOnProxyRouteChange]) — connections on a dead route are already unreachable, so this + * is hygiene, not correctness, and it empties the pool BOTH clients share. + */ + fun evictPooledConnections() = rootClient.connectionPool.evictAll() + fun buildHttpClient( localSocksProxyPort: Int?, isMobile: Boolean?, diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt new file mode 100644 index 0000000000..79d6fbea50 --- /dev/null +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.service.http + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.drop +import kotlinx.coroutines.launch + +/** + * Runs [onChange] whenever the Tor SOCKS port this app routes through actually changes — Tor + * coming up (`null` -> 9050), going away (9050 -> `null`), or moving (9050 -> 9150). + * + * The managers use this to drop pooled connections once per real route change. Reuse is *not* + * the reason: OkHttp keys its pool by `Address`, which includes the proxy, so a connection on + * the old route is already unreachable by calls on the new one and would age out on its own. + * The reason is hygiene at the moment the user's intent changes — when Tor is switched on, idle + * sockets the direct client opened to real hosts should not linger for the pool's 5-minute + * keepalive after the user has asked for everything to go through Tor. + * + * Deliberately driven by the port and nothing else: + * + * - **Not by client rebuilds.** That is what the old `lastProxy`-per-factory check did, and + * since one factory mints both the proxied and the direct client, the two alternated that + * field forever and wiped the shared pool on every network-state emission and resubscribe. + * - **Not by the per-feature Tor toggles** (`imagesViaTor`, `videosViaTor`, …). Those change + * which of the two existing clients a request picks, not the route either one uses, so no + * pooled connection becomes stale. + * + * [drop] skips the current value, so subscribing does not itself count as a change — only a + * later move away from the port in force when this was wired. + */ +fun CoroutineScope.evictOnProxyRouteChange( + proxyPortProvider: StateFlow, + onChange: () -> Unit, +): Job = + launch { + proxyPortProvider + .drop(1) + .distinctUntilChanged() + .collect { onChange() } + } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt new file mode 100644 index 0000000000..a65ed59343 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt @@ -0,0 +1,135 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.service.http + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.Test +import kotlin.test.assertEquals + +@OptIn(ExperimentalCoroutinesApi::class) +class ProxyRouteChangeTest { + @Test + fun subscribingIsNotAChange() = + runTest { + val port = MutableStateFlow(9050) + val evictions = AtomicInteger() + + val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } + runCurrent() + + // The port in force when we wired up is the status quo, not a route change. + assertEquals(0, evictions.get()) + job.cancel() + } + + @Test + fun torComingUpEvictsOnce() = + runTest { + val port = MutableStateFlow(null) + val evictions = AtomicInteger() + + val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } + runCurrent() + + port.value = 9050 + runCurrent() + + assertEquals(1, evictions.get()) + job.cancel() + } + + @Test + fun torGoingAwayEvictsOnce() = + runTest { + val port = MutableStateFlow(9050) + val evictions = AtomicInteger() + + val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } + runCurrent() + + port.value = null + runCurrent() + + assertEquals(1, evictions.get()) + job.cancel() + } + + @Test + fun movingToAnotherPortEvictsOnce() = + runTest { + val port = MutableStateFlow(9050) + val evictions = AtomicInteger() + + val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } + runCurrent() + + port.value = 9150 // Tor Browser's port + runCurrent() + + assertEquals(1, evictions.get()) + job.cancel() + } + + /** + * The regression this whole seam exists for. The old per-factory check fired on every client + * rebuild — which happens on each network-state emission and each resubscribe — and wiped the + * pool both clients share. Re-emitting the same port must be free. + */ + @Test + fun reEmittingTheSamePortNeverEvicts() = + runTest { + val port = MutableStateFlow(9050) + val evictions = AtomicInteger() + + val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } + runCurrent() + + repeat(20) { + port.value = 9050 + runCurrent() + } + + assertEquals(0, evictions.get()) + job.cancel() + } + + @Test + fun aRoundTripEvictsOncePerLeg() = + runTest { + val port = MutableStateFlow(null) + val evictions = AtomicInteger() + + val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } + runCurrent() + + listOf(9050, null, 9050).forEach { + port.value = it + runCurrent() + } + + assertEquals(3, evictions.get()) + job.cancel() + } +} From 25e61542ba065dce174033a4659783ffd9e318da Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 12:16:56 +0000 Subject: [PATCH 7/7] fix: three defects this branch introduced, found auditing its own diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Tor's control flow was being pinned alive for the process lifetime.** The eviction wiring subscribed to `torManager.activePortOrNull` from a never-cancelled coroutine on `applicationIOScope`, in both managers. That flow chains to `TorManager.status`, whose upstream is `WhileSubscribed` and runs `launch { service.start() }` when collected — so a permanent subscriber starts Arti at process construction and never lets it unsubscribe on background. AppModules documents this exact hazard for the battery ledger and deliberately watches the raw `TorService.status` instead; I wired the poisoned well four lines away from the warning. Rewired from signals that are plain StateFlows and therefore free to observe: `torPrefs.torType`, `torPrefs.externalSocksPort`, and `torService.status`'s socks port. It moves to AppModules, which is where those live and where the precedent is; commons had no business knowing Tor's subscription hazards anyway. **`drop(1)` promised more than it delivered.** In the manager it skipped whatever was present when the *coroutine started*, not when the call was made, so a route change landing in that window was swallowed. In its new home the two coincide — this runs during AppModules construction, before anything is pooled — so the operator now means what the comment says. **Animated media in a Crop cell flashed its raw URL.** The loading ladder keyed its last branch on `ratio != null`, but `mediaSizingModifier` also bounds the height for `ContentScale.Crop`. MyAsyncImage passes dimensions/blurhash/thumbhash all null, so every gif in a card slot — DVM covers, long-form headers, follow-set/calendar/music cards — hit the unbounded branch on first load and drew URL text where it used to draw nothing. The predicate wanted "is the height bounded", so it now says so: `contentScale == Crop || ratio != null`. Drops ProxyRouteChange.kt and its tests with the rewire. The trigger is now three stdlib flow operators; what needed judgement was which signals are safe to watch, and that is recorded in the comment rather than in a test of combine(). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5 --- .../com/vitorpamplona/amethyst/AppModules.kt | 36 +++++ .../amethyst/ui/components/GifVideoView.kt | 25 ++-- .../service/http/DualHttpClientManager.kt | 6 - .../http/DualHttpClientManagerForRelays.kt | 6 - .../commons/service/http/ProxyRouteChange.kt | 62 -------- .../service/http/ProxyRouteChangeTest.kt | 135 ------------------ 6 files changed, 52 insertions(+), 218 deletions(-) delete mode 100644 commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt delete mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt index 1b8397a2b0..7fd2db22f8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt @@ -184,6 +184,7 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.asSharedFlow import kotlinx.coroutines.flow.collectLatest +import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.conflate import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.drop @@ -647,6 +648,41 @@ class AppModules( onionCache = onionLocationCache, ) + // Drops pooled connections once per real Tor route change. When the user switches + // Tor on, the direct clients' idle sockets to real hosts would otherwise sit in the + // pool for its 5-minute keepalive after the user has asked for everything to go + // through Tor. No request could use them either way -- OkHttp keys the pool by + // `Address`, which includes the proxy, so a connection on a dead route is already + // unreachable -- which is why this is hygiene and not correctness, and why it is + // fine for it to be a little late. + // + // Every source here is a plain StateFlow, so subscribing costs nothing. Deliberately + // NOT torManager.activePortOrNull: that chains to TorManager.status, whose upstream + // is WhileSubscribed and calls service.start() when collected, so a process-lifetime + // subscription there would hold Arti's control flow open forever -- the same hazard + // the battery ledger above documents and sidesteps the same way. + // + // Also deliberately not the per-feature Tor switches (imagesViaTor, videosViaTor, ...): + // those change which of the two existing clients a request picks, not the route either + // one uses, so no pooled connection goes stale. + init { + applicationIOScope.launch { + combine( + torPrefs.torType, + torPrefs.externalSocksPort, + torService.status.map { it.socksPort }, + ) { torType, externalPort, artiPort -> Triple(torType, externalPort, artiPort) } + .distinctUntilChanged() + // Only later moves count; the route in force at process construction is the + // status quo, and nothing is pooled yet to evict. + .drop(1) + .collect { + okHttpClients.factory.evictPooledConnections() + okHttpClientForRelays.factory.evictPooledConnections() + } + } + } + // Connects the INostrClient class with okHttp val websocketBuilder = OkHttpWebSocket.Builder( diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt index f62f7ede81..b2107b5979 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt @@ -75,6 +75,15 @@ fun GifVideoView( // remember() to avoid the recompute would cost more (slot read + N equality checks) // than the work it saves; that's why this stays as a plain expression. val ratio = dimensions?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(videoUri) + + // Mirrors [mediaSizingModifier]: Crop gets fillMaxSize() and a known ratio gets + // aspectRatio(), both of which bound the height. Everything else is a bare + // fillMaxWidth() that wraps its content, and only THAT case needs a loading state + // with intrinsic height to keep the note from collapsing to nothing. Keying the + // fallback on `ratio` alone would put raw URL text inside every Crop card cell -- + // MyAsyncImage passes dimensions/blurhash/thumbhash all null, so a gif in a card + // slot hits this on first load, before MediaAspectRatioCache knows its size. + val heightIsBounded = contentScale == ContentScale.Crop || ratio != null val autoPlay = accountViewModel.settings.autoPlayVideos() val borderModifier = if (roundedCorner) MaterialTheme.colorScheme.imageModifier else Modifier val context = LocalContext.current @@ -111,14 +120,12 @@ fun GifVideoView( when (state) { is AsyncImagePainter.State.Loading -> { - // Every branch here MUST emit something with a height. [containerModifier] - // only constrains the height when `ratio` is known (imeta `dim` or a cached - // ratio); without one it is a bare fillMaxWidth(), so the box wraps its - // content and an empty loading state collapses the whole note to zero - // height -- no picture, no URL, no spinner, just a gap in the feed until - // the load finishes. DisplayBlurHash renders NOTHING when both hashes are - // absent (placeholderModel returns null), which is exactly the case a - // no-imeta post hits. Mirrors UrlImageView's ladder. + // When the height is unbounded (see [heightIsBounded]) this branch MUST + // emit something with an intrinsic height, or the box wraps nothing and the + // whole note collapses to zero -- no picture, no URL, no spinner, just a gap + // in the feed until the load finishes. DisplayBlurHash renders NOTHING when + // both hashes are absent (placeholderModel returns null), which is exactly + // what a no-imeta post hits. Mirrors UrlImageView's ladder. if (blurhash != null || thumbhash != null) { DisplayBlurHash( blurhash, @@ -127,7 +134,7 @@ fun GifVideoView( Modifier.fillMaxSize(), thumbhash = thumbhash, ) - } else if (ratio != null) { + } else if (heightIsBounded) { Box(Modifier.fillMaxSize(), contentAlignment = Alignment.Center) { LoadingAnimation(Size40dp, Size6dp) } diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt index 6fa4af9c44..8b13761f03 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManager.kt @@ -56,12 +56,6 @@ class DualHttpClientManager( ) : IHttpClientManager { val factory = OkHttpClientFactory(keyCache, userAgent, dns, shouldBridgeBlossomCache, onionCache, usageInterceptor, blossomReadAuth) - init { - // One eviction per real Tor route change. See [evictOnProxyRouteChange] for why this is - // driven by the port rather than by client rebuilds. - scope.evictOnProxyRouteChange(proxyPortProvider, factory::evictPooledConnections) - } - val defaultHttpClient: StateFlow = combine(proxyPortProvider, isMobileDataProvider) { proxy, mobile -> factory.buildHttpClient(proxy, mobile) diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt index aade203faf..77b844b0d9 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/DualHttpClientManagerForRelays.kt @@ -41,12 +41,6 @@ class DualHttpClientManagerForRelays( ) : IHttpClientManager { val factory = OkHttpClientFactoryForRelays(userAgent, dns, onionCache) - init { - // One eviction per real Tor route change. See [evictOnProxyRouteChange] for why this is - // driven by the port rather than by client rebuilds. - scope.evictOnProxyRouteChange(proxyPortProvider, factory::evictPooledConnections) - } - val defaultHttpClient: StateFlow = combine(proxyPortProvider, isMobileDataProvider) { proxy, mobile -> factory.buildHttpClient(proxy, mobile) diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt deleted file mode 100644 index 79d6fbea50..0000000000 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChange.kt +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright (c) 2025 Vitor Pamplona - * - * Permission is hereby granted, free of charge, to any person obtaining a copy of - * this software and associated documentation files (the "Software"), to deal in - * the Software without restriction, including without limitation the rights to use, - * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the - * Software, and to permit persons to whom the Software is furnished to do so, - * subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS - * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR - * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN - * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION - * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. - */ -package com.vitorpamplona.amethyst.commons.service.http - -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Job -import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.drop -import kotlinx.coroutines.launch - -/** - * Runs [onChange] whenever the Tor SOCKS port this app routes through actually changes — Tor - * coming up (`null` -> 9050), going away (9050 -> `null`), or moving (9050 -> 9150). - * - * The managers use this to drop pooled connections once per real route change. Reuse is *not* - * the reason: OkHttp keys its pool by `Address`, which includes the proxy, so a connection on - * the old route is already unreachable by calls on the new one and would age out on its own. - * The reason is hygiene at the moment the user's intent changes — when Tor is switched on, idle - * sockets the direct client opened to real hosts should not linger for the pool's 5-minute - * keepalive after the user has asked for everything to go through Tor. - * - * Deliberately driven by the port and nothing else: - * - * - **Not by client rebuilds.** That is what the old `lastProxy`-per-factory check did, and - * since one factory mints both the proxied and the direct client, the two alternated that - * field forever and wiped the shared pool on every network-state emission and resubscribe. - * - **Not by the per-feature Tor toggles** (`imagesViaTor`, `videosViaTor`, …). Those change - * which of the two existing clients a request picks, not the route either one uses, so no - * pooled connection becomes stale. - * - * [drop] skips the current value, so subscribing does not itself count as a change — only a - * later move away from the port in force when this was wired. - */ -fun CoroutineScope.evictOnProxyRouteChange( - proxyPortProvider: StateFlow, - onChange: () -> Unit, -): Job = - launch { - proxyPortProvider - .drop(1) - .distinctUntilChanged() - .collect { onChange() } - } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt deleted file mode 100644 index a65ed59343..0000000000 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteChangeTest.kt +++ /dev/null @@ -1,135 +0,0 @@ -/* - * Copyright (c) 2025 Vitor Pamplona - * - * Permission is hereby granted, free of charge, to any person obtaining a copy of - * this software and associated documentation files (the "Software"), to deal in - * the Software without restriction, including without limitation the rights to use, - * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the - * Software, and to permit persons to whom the Software is furnished to do so, - * subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS - * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR - * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN - * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION - * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. - */ -package com.vitorpamplona.amethyst.commons.service.http - -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.test.runCurrent -import kotlinx.coroutines.test.runTest -import java.util.concurrent.atomic.AtomicInteger -import kotlin.test.Test -import kotlin.test.assertEquals - -@OptIn(ExperimentalCoroutinesApi::class) -class ProxyRouteChangeTest { - @Test - fun subscribingIsNotAChange() = - runTest { - val port = MutableStateFlow(9050) - val evictions = AtomicInteger() - - val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } - runCurrent() - - // The port in force when we wired up is the status quo, not a route change. - assertEquals(0, evictions.get()) - job.cancel() - } - - @Test - fun torComingUpEvictsOnce() = - runTest { - val port = MutableStateFlow(null) - val evictions = AtomicInteger() - - val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } - runCurrent() - - port.value = 9050 - runCurrent() - - assertEquals(1, evictions.get()) - job.cancel() - } - - @Test - fun torGoingAwayEvictsOnce() = - runTest { - val port = MutableStateFlow(9050) - val evictions = AtomicInteger() - - val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } - runCurrent() - - port.value = null - runCurrent() - - assertEquals(1, evictions.get()) - job.cancel() - } - - @Test - fun movingToAnotherPortEvictsOnce() = - runTest { - val port = MutableStateFlow(9050) - val evictions = AtomicInteger() - - val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } - runCurrent() - - port.value = 9150 // Tor Browser's port - runCurrent() - - assertEquals(1, evictions.get()) - job.cancel() - } - - /** - * The regression this whole seam exists for. The old per-factory check fired on every client - * rebuild — which happens on each network-state emission and each resubscribe — and wiped the - * pool both clients share. Re-emitting the same port must be free. - */ - @Test - fun reEmittingTheSamePortNeverEvicts() = - runTest { - val port = MutableStateFlow(9050) - val evictions = AtomicInteger() - - val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } - runCurrent() - - repeat(20) { - port.value = 9050 - runCurrent() - } - - assertEquals(0, evictions.get()) - job.cancel() - } - - @Test - fun aRoundTripEvictsOncePerLeg() = - runTest { - val port = MutableStateFlow(null) - val evictions = AtomicInteger() - - val job = evictOnProxyRouteChange(port) { evictions.incrementAndGet() } - runCurrent() - - listOf(9050, null, 9050).forEach { - port.value = it - runCurrent() - } - - assertEquals(3, evictions.get()) - job.cancel() - } -}