From 95f70206aaaf70a0d8a64304e3879690a1014cb5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 21:09:12 +0000 Subject: [PATCH 1/5] fix(images): stop copying whole GIFs into RAM before decoding them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 69.8 MB, 1138x640, 201-frame GIF from a Ditto note froze the feed. The media is pathological, but the app made it far worse than it had to be. Coil hands `android.graphics.ImageDecoder` a *file* only when the image source's file system is **referentially** `FileSystem.SYSTEM` (`ImageSource.toImageDecoderSourceOrNull`, Coil 3.5.0): if (fileSystem === FileSystem.SYSTEM) { val file = fileOrNull() if (file != null) return ImageDecoder.createSource(file.toFile()) } `NetworkFetcher` stamps the source with `diskCache.fileSystem`, and ours is a `DeferredDeleteFileSystem` wrapper, so that identity check fails for every image we fetch from the network. For an animated image the fallback is `ImageDecoder.createSource(source.squashToDirectByteBuffer())`: the entire encoded animation is pulled onto the heap and then copied into an equally large direct `ByteBuffer` that stays alive for as long as the `AnimatedImageDrawable` does. Replaying that path over the reported GIF measured 66 MB of heap plus 66 MB of native memory, and ~700 ms of pure copying on desktop x86. Animated results are never memory-cached (`DrawableImage.shareable` is false), so the feed paid it again on every scroll back into view. Coil then compounds it below API 34: it wraps every GIF in a stream-backed `FrameDelayRewritingSource` to clamp sub-threshold frame delays, which forfeits the file fast path even when the identity check would have passed. So: - `onSystemFileSystem()` re-points a disk-cache-backed source at the real `FileSystem.SYSTEM` before it reaches a decoder. Only deletes are deferred by the wrapper; reads already go straight through. - `hasSubThresholdGifFrameDelay()` streams a 64 KiB window over the file and asks for the rewrite only when a graphics control block really declares a delay below 2/100 s. The reported GIF declares 5 on all 201 frames, as do the overwhelming majority of GIFs, so they now decode from the file with no heap copy at all. Files that do need the clamp keep Coil's behaviour. - `AnimatedImageDecoderFactory` replaces Coil's factory with the same sniff and those two changes; `AvifAnimatedDecoderFactory` shares it. Still images take a similar hit from the same identity check (they fall back from `StaticImageDecoder` to `BitmapFactoryDecoder`) — left alone here since it changes the decode path for every image in the app. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017RYCgbvhtBCBNVLMxSWoCJ --- .../images/AnimatedImageDecoderFactory.kt | 128 +++++++++++++++++ .../images/AvifAnimatedDecoderFactory.kt | 5 +- .../amethyst/service/images/GifFrameDelays.kt | 92 ++++++++++++ .../service/images/ImageDecoderSources.kt | 71 ++++++++++ .../service/images/ImageLoaderSetup.kt | 5 +- .../service/images/GifFrameDelaysTest.kt | 132 ++++++++++++++++++ .../service/images/ImageDecoderSourcesTest.kt | 110 +++++++++++++++ 7 files changed, 539 insertions(+), 4 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/GifFrameDelays.kt create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/GifFrameDelaysTest.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt new file mode 100644 index 0000000000..a6a1ac5a15 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt @@ -0,0 +1,128 @@ +/* + * 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.service.images + +import android.os.Build.VERSION.SDK_INT +import androidx.annotation.RequiresApi +import coil3.ImageLoader +import coil3.decode.DecodeResult +import coil3.decode.DecodeUtils +import coil3.decode.Decoder +import coil3.decode.ImageSource +import coil3.fetch.SourceFetchResult +import coil3.gif.AnimatedImageDecoder +import coil3.gif.isAnimatedHeif +import coil3.gif.isAnimatedWebP +import coil3.gif.isGif +import coil3.request.Options +import okio.BufferedSource +import okio.buffer + +/** + * Drop-in replacement for Coil's `AnimatedImageDecoder.Factory` that keeps the platform + * `ImageDecoder` reading straight from the disk-cache file instead of a RAM copy of it. + * + * The sniffing is identical to Coil's. Two things differ, both about *how the bytes reach* + * `ImageDecoder`: + * + * 1. The source is re-homed onto `FileSystem.SYSTEM` — see [onSystemFileSystem] for why + * Amethyst's disk cache otherwise forfeits the file fast path on every network image. + * 2. The GIF frame-delay rewrite Coil applies on API < 34 is asked for only when the file + * actually contains a sub-threshold delay — see [hasSubThresholdGifFrameDelay]. The + * rewrite is stream-backed, so requesting it unconditionally undoes (1) for every GIF. + * + * Together those keep a large GIF off the heap entirely: a 69.8 MB / 201-frame GIF cost + * 66 MB of heap plus a 66 MB direct `ByteBuffer` per decode before this, and animated results + * are never memory-cached, so that repeated on every scroll back into view. + */ +@RequiresApi(28) +class AnimatedImageDecoderFactory : Decoder.Factory { + override fun create( + result: SourceFetchResult, + options: Options, + imageLoader: ImageLoader, + ): Decoder? { + val source = result.source.source() + val isGif = DecodeUtils.isGif(source) + if (!isGif && !isAnimatedNonGif(source)) return null + + return newAnimatedImageDecoder(result.source, options, isGif) + } + + private fun isAnimatedNonGif(source: BufferedSource): Boolean = DecodeUtils.isAnimatedWebP(source) || (SDK_INT >= 30 && DecodeUtils.isAnimatedHeif(source)) +} + +/** + * Builds Coil's [AnimatedImageDecoder] over the cheapest source we can give it. + * + * [mayNeedFrameDelayRewrite] should be true only for GIFs: Coil's rewriter no-ops on every + * other format, so scanning one would be pure IO for a decision already made. + */ +@RequiresApi(28) +fun newAnimatedImageDecoder( + source: ImageSource, + options: Options, + mayNeedFrameDelayRewrite: Boolean, +): Decoder { + val onSystem = source.onSystemFileSystem() + val decoder = AnimatedImageDecoder(onSystem, options, enforceMinimumFrameDelay(onSystem, mayNeedFrameDelayRewrite)) + + // A re-homed source is ours, so nobody else will close it: Coil's engine closes the + // source it handed us, and AnimatedImageDecoder only closes the frame-delay wrapper. + return if (onSystem === source) decoder else ClosingDecoder(onSystem, decoder) +} + +/** + * Whether to ask Coil to clamp this GIF's sub-threshold frame delays. From API 34 the + * platform decoder does it itself, which is why Coil's own default turns the rewrite off + * there; below that we pay for it only when the file really has a delay to clamp. + */ +private fun enforceMinimumFrameDelay( + source: ImageSource, + mayNeedFrameDelayRewrite: Boolean, +): Boolean { + if (!mayNeedFrameDelayRewrite || SDK_INT >= 34) return false + + // Not file-backed: the decode squashes the stream into RAM either way, so there is no + // fast path to protect and no reason to deviate from Coil's default. + val file = source.fileOrNull() ?: return true + + return source.fileSystem + .source(file) + .buffer() + .use { it.hasSubThresholdGifFrameDelay() } +} + +/** Closes [source] once [delegate] is done with it. */ +private class ClosingDecoder( + private val source: ImageSource, + private val delegate: Decoder, +) : Decoder { + override suspend fun decode(): DecodeResult? = + try { + delegate.decode() + } finally { + try { + source.close() + } catch (_: Exception) { + } + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AvifAnimatedDecoderFactory.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AvifAnimatedDecoderFactory.kt index 8406755a23..b07c74c606 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AvifAnimatedDecoderFactory.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AvifAnimatedDecoderFactory.kt @@ -41,7 +41,8 @@ import okio.ByteString.Companion.encodeUtf8 * * On API 31+, the platform [android.graphics.ImageDecoder] produces an * [android.graphics.drawable.AnimatedImageDrawable] for animated AVIF. We delegate the - * actual decode to Coil's [AnimatedImageDecoder] — only the brand sniff is custom. + * actual decode to Coil's [AnimatedImageDecoder] — only the brand sniff is custom. It is built + * through [newAnimatedImageDecoder] so an AVIF gets the same disk-cache file fast path a GIF does. * * Below API 31 the platform decoder cannot handle AVIF at all, so we decline and let * other decoders try (they will also fail, and Coil falls through to its error slot, @@ -62,7 +63,7 @@ class AvifAnimatedDecoderFactory : Decoder.Factory { private fun createAnimatedImageDecoder( result: SourceFetchResult, options: Options, - ): Decoder = AnimatedImageDecoder(result.source, options) + ): Decoder = newAnimatedImageDecoder(result.source, options, mayNeedFrameDelayRewrite = false) private fun isAvif(source: BufferedSource): Boolean = source.rangeEquals(4, FTYP) && diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/GifFrameDelays.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/GifFrameDelays.kt new file mode 100644 index 0000000000..e4bb0321f7 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/GifFrameDelays.kt @@ -0,0 +1,92 @@ +/* + * 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.service.images + +import okio.BufferedSource + +/** + * Frame delay, in hundredths of a second, below which Coil rewrites a GIF's graphics control + * blocks. Mirrors `FrameDelayRewritingSource.MINIMUM_FRAME_DELAY` in Coil 3.5.0. + */ +private const val MINIMUM_FRAME_DELAY = 2 + +/** `00 21 F9 04` — block terminator, extension introducer, graphics control label, block size. */ +private const val MARKER_0: Byte = 0x00 +private const val MARKER_1: Byte = 0x21 +private const val MARKER_2: Byte = 0xF9.toByte() +private const val MARKER_3: Byte = 0x04 + +/** + * Bytes one graphics control block occupies from the first marker byte: the 4 marker bytes, + * the packed field, the two delay bytes, the transparent colour index and the terminator. + */ +private const val BLOCK_SIZE = 9 + +private const val SCAN_CHUNK = 64 * 1024 + +/** + * Streams over a GIF looking for a graphics control block whose frame delay is below + * [MINIMUM_FRAME_DELAY] — i.e. one that Coil's `FrameDelayRewritingSource` would actually + * rewrite. + * + * Coil wraps every GIF in that rewriting source on API < 34 (the platform clamps sub-threshold + * delays itself from 34 on). The wrapper is stream-backed, which costs the file fast path in + * [onSystemFileSystem] and forces the whole encoded animation into RAM twice. The overwhelming + * majority of GIFs declare a sane delay and come out of the rewriter byte-for-byte identical, + * so this scan buys the fast path back for them and leaves the rewrite in place only for the + * files that need it. + * + * The condition matches Coil's exactly (verified against Coil 3.5.0): a delay below the + * threshold in a block whose terminator byte is zero. Reading the whole stream costs one + * sequential pass over a [SCAN_CHUNK] buffer and allocates nothing else, against the + * file-sized heap **and** direct-buffer copies the rewrite path would otherwise make. + */ +fun BufferedSource.hasSubThresholdGifFrameDelay(): Boolean { + // Carries the last BLOCK_SIZE - 1 bytes of a chunk forward so a block that straddles a + // chunk boundary is still seen whole. + val window = ByteArray(SCAN_CHUNK + BLOCK_SIZE) + var carried = 0 + + while (true) { + val read = read(window, carried, SCAN_CHUNK) + if (read == -1) return false + + val filled = carried + read + var i = 0 + val last = filled - BLOCK_SIZE + while (i <= last) { + if (window[i] == MARKER_0 && + window[i + 1] == MARKER_1 && + window[i + 2] == MARKER_2 && + window[i + 3] == MARKER_3 && + window[i + 8] == 0.toByte() + ) { + // Delay is two unsigned bytes, least significant first. + val delay = ((window[i + 6].toInt() and 0xFF) shl 8) or (window[i + 5].toInt() and 0xFF) + if (delay < MINIMUM_FRAME_DELAY) return true + } + i++ + } + + carried = minOf(filled, BLOCK_SIZE - 1) + window.copyInto(window, 0, filled - carried, filled) + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt new file mode 100644 index 0000000000..10c9e6247e --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt @@ -0,0 +1,71 @@ +/* + * 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.service.images + +import coil3.decode.ImageSource +import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem +import okio.FileSystem + +/** + * Re-points a disk-cache-backed [ImageSource] at [FileSystem.SYSTEM] so Coil's platform + * `ImageDecoder` fast path stays reachable. + * + * Coil hands `android.graphics.ImageDecoder` a *file* only when the source's file system is + * **referentially** [FileSystem.SYSTEM] (`ImageSource.toImageDecoderSourceOrNull`, verified + * against Coil 3.5.0): + * + * ``` + * if (fileSystem === FileSystem.SYSTEM) { + * val file = fileOrNull() + * if (file != null) return ImageDecoder.createSource(file.toFile()) + * } + * ``` + * + * The source Coil's `NetworkFetcher` builds carries `diskCache.fileSystem`, and ours is a + * [DeferredDeleteFileSystem] wrapper, so that identity check fails for **every** image we + * fetch from the network. The fallback for an animated image is + * `ImageDecoder.createSource(source.squashToDirectByteBuffer())`, which pulls the entire + * encoded animation onto the heap and then copies it into an equally large direct + * `ByteBuffer` that stays alive for as long as the `AnimatedImageDrawable` does. Measured on + * a 69.8 MB / 201-frame GIF: 66 MB of heap plus 66 MB of native memory per decode — and + * animated results are never memory-cached (`DrawableImage.shareable` is false), so the feed + * pays it again every time the note scrolls back into view. + * + * Only the delete path is deferred by the wrapper; reads go straight to + * [FileSystem.SYSTEM]. So handing the decoder the same file on the real system file system + * is equivalent, and it is exactly what Coil does when no wrapper is installed. + * + * Returns `this` unchanged when there is nothing to gain: already on [FileSystem.SYSTEM], not + * a wrapper we know unwraps to it, or not backed by a file yet. [ImageSource.fileOrNull] is + * used rather than [ImageSource.file] on purpose — the latter would materialise a temp copy + * of a stream-backed source, which is the very cost this exists to avoid. + */ +fun ImageSource.onSystemFileSystem(): ImageSource { + if (fileSystem === FileSystem.SYSTEM) return this + + var unwrapped: FileSystem = fileSystem + while (unwrapped is DeferredDeleteFileSystem) unwrapped = unwrapped.delegate + if (unwrapped !== FileSystem.SYSTEM) return this + + val file = fileOrNull() ?: return this + + return ImageSource(file = file, fileSystem = FileSystem.SYSTEM, metadata = metadata) +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt index 7a4098f5ed..8fd444ffea 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt @@ -29,7 +29,6 @@ import coil3.annotation.DelicateCoilApi import coil3.annotation.ExperimentalCoilApi import coil3.disk.DiskCache import coil3.fetch.Fetcher -import coil3.gif.AnimatedImageDecoder import coil3.gif.GifDecoder import coil3.memory.MemoryCache import coil3.network.CacheStrategy @@ -61,7 +60,9 @@ class ImageLoaderSetup { companion object { val gifFactory = if (Build.VERSION.SDK_INT >= 28) { - AnimatedImageDecoder.Factory() + // Not Coil's AnimatedImageDecoder.Factory: see [AnimatedImageDecoderFactory] for + // why decoding straight from the disk-cache file matters here. + AnimatedImageDecoderFactory() } else { GifDecoder.Factory() } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/GifFrameDelaysTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/GifFrameDelaysTest.kt new file mode 100644 index 0000000000..dccca4fce2 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/GifFrameDelaysTest.kt @@ -0,0 +1,132 @@ +/* + * 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.service.images + +import okio.Buffer +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The scan decides whether a GIF still needs Coil's in-RAM frame-delay rewrite on API < 34, so + * a false negative would silently change how a GIF animates. Its condition therefore has to + * match `FrameDelayRewritingSource` byte for byte: a delay below 2/100 s in a graphics control + * block whose terminator is zero. + */ +class GifFrameDelaysTest { + /** + * `00 21 F9 04` marker, packed field, delay (low, high), transparent colour index, block + * terminator — the nine bytes Coil inspects. + */ + private fun graphicsControlBlock( + delay: Int, + terminator: Int = 0, + ) = byteArrayOf( + 0x00, + 0x21, + 0xF9.toByte(), + 0x04, + 0x00, + (delay and 0xFF).toByte(), + ((delay shr 8) and 0xFF).toByte(), + 0x00, + terminator.toByte(), + ) + + private fun gif(vararg blocks: ByteArray) = + Buffer().apply { + write("GIF89a".toByteArray()) + write(ByteArray(7)) // logical screen descriptor + blocks.forEach { write(it) } + writeByte(0x3B) // trailer + } + + @Test + fun aSaneDelayNeedsNoRewrite() { + // 5/100 s is what the 69.8 MB GIF from the bug report declares on all 201 frames. + assertFalse(gif(graphicsControlBlock(delay = 5)).hasSubThresholdGifFrameDelay()) + } + + @Test + fun zeroAndOneHundredthsAreBelowTheThreshold() { + assertTrue(gif(graphicsControlBlock(delay = 0)).hasSubThresholdGifFrameDelay()) + assertTrue(gif(graphicsControlBlock(delay = 1)).hasSubThresholdGifFrameDelay()) + } + + @Test + fun twoHundredthsIsTheFirstAcceptedDelay() { + assertFalse(gif(graphicsControlBlock(delay = 2)).hasSubThresholdGifFrameDelay()) + } + + @Test + fun aDelayAboveOneByteIsReadLeastSignificantByteFirst() { + // 0x0100 = 256/100 s. Reading the bytes in the wrong order would see 1 and rewrite. + assertFalse(gif(graphicsControlBlock(delay = 256)).hasSubThresholdGifFrameDelay()) + } + + @Test + fun oneBadBlockAmongGoodOnesIsEnough() { + val stream = + gif( + graphicsControlBlock(delay = 10), + graphicsControlBlock(delay = 10), + graphicsControlBlock(delay = 0), + graphicsControlBlock(delay = 10), + ) + + assertTrue(stream.hasSubThresholdGifFrameDelay()) + } + + @Test + fun aNonZeroTerminatorIsSkipped() { + // Coil bails out of the rewrite when the block does not end where it expects, so a + // marker-shaped run of bytes inside image data must not count as a frame delay. + assertFalse(gif(graphicsControlBlock(delay = 0, terminator = 0x2C)).hasSubThresholdGifFrameDelay()) + } + + @Test + fun aBlockStraddlingTheScanChunkBoundaryIsStillFound() { + // The scan reads in 64 KiB chunks; without the carried-over window a block landing on + // the seam would be missed and the GIF would animate at the wrong speed. + for (offset in 0..8) { + val padding = ByteArray(64 * 1024 - 8 + offset) + val stream = + Buffer().apply { + write(padding) + write(graphicsControlBlock(delay = 0)) + } + + assertTrue("block at 64KiB - 8 + $offset", stream.hasSubThresholdGifFrameDelay()) + } + } + + @Test + fun anEmptySourceNeedsNoRewrite() { + assertFalse(Buffer().hasSubThresholdGifFrameDelay()) + } + + @Test + fun aTruncatedBlockAtTheEndOfTheStreamNeedsNoRewrite() { + val stream = Buffer().apply { write(graphicsControlBlock(delay = 0).copyOfRange(0, 8)) } + + assertFalse(stream.hasSubThresholdGifFrameDelay()) + } +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt new file mode 100644 index 0000000000..7819ed13e8 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt @@ -0,0 +1,110 @@ +/* + * 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.service.images + +import coil3.decode.ImageSource +import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import okio.Buffer +import okio.FileSystem +import okio.ForwardingFileSystem +import okio.Path +import okio.Path.Companion.toOkioPath +import org.junit.After +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Before +import org.junit.Test +import java.io.File +import java.nio.file.Files + +/** + * [onSystemFileSystem] exists to satisfy an identity check inside Coil + * (`ImageSource.toImageDecoderSourceOrNull` only reaches for the file when + * `fileSystem === FileSystem.SYSTEM`). These tests pin the property that check reads, since + * getting it wrong is silent: the decode still succeeds, it just copies the whole encoded + * image into RAM first. + */ +class ImageDecoderSourcesTest { + private lateinit var tmpRoot: File + private lateinit var file: Path + private lateinit var scope: CoroutineScope + + private val bytes = ByteArray(64) { it.toByte() } + + @Before + fun setUp() { + tmpRoot = Files.createTempDirectory("image-decoder-sources-test").toFile() + file = tmpRoot.resolve("blob.gif").toOkioPath() + FileSystem.SYSTEM.write(file) { write(bytes) } + scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + } + + @After + fun tearDown() { + scope.cancel() + tmpRoot.deleteRecursively() + } + + @Test + fun deferredDeleteWrapper_isUnwrappedToTheSystemFileSystem() { + val source = ImageSource(file = file, fileSystem = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope)) + + val rehomed = source.onSystemFileSystem() + + assertSame(FileSystem.SYSTEM, rehomed.fileSystem) + assertSame(file, rehomed.fileOrNull()) + assertArrayEquals(bytes, rehomed.source().readByteArray()) + rehomed.close() + } + + @Test + fun alreadyOnTheSystemFileSystem_isReturnedUntouched() { + val source = ImageSource(file = file, fileSystem = FileSystem.SYSTEM) + + assertSame(source, source.onSystemFileSystem()) + } + + @Test + fun anUnknownWrapper_isLeftAlone() { + // Only DeferredDeleteFileSystem is known to forward reads verbatim. Anything else may + // rewrite paths or serve different bytes, so the file underneath is not ours to hand out. + val source = ImageSource(file = file, fileSystem = object : ForwardingFileSystem(FileSystem.SYSTEM) {}) + + assertSame(source, source.onSystemFileSystem()) + } + + @Test + fun aStreamBackedSource_isNotMaterialisedIntoATempFile() { + val source = + ImageSource( + source = Buffer().write(bytes), + fileSystem = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope), + ) + + assertSame(source, source.onSystemFileSystem()) + assertNull("re-homing must not force a temp copy of a stream", source.fileOrNull()) + } +} From 8b594981e908006bd673e852352af444588f2ebc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 22:27:35 +0000 Subject: [PATCH 2/5] fix(images): restore the ImageDecoder path for still images too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The identity check that cost animated images their file source cost still images a decoder outright: `StaticImageDecoder.Factory` returns null when it cannot get an `ImageDecoder.Source`, so with our `DeferredDeleteFileSystem` on the source every network still image fell through to `BitmapFactoryDecoder`. Move the re-home from the animated decoder to the fetcher, where it fixes both at one seam. `SystemFileSystemFetcher` wraps the three network-backed fetchers the app builds (`OkHttpFactory`, `BlossomFetcher`, `ProfilePictureFetcher`) and re-points a disk-cache-backed result at `FileSystem.SYSTEM` before any decoder sees it. Doing it here rather than at each decoder: - One wrapper covers every decoder, including Coil's own registered `StaticImageDecoder.Factory`. Adding a second static factory would instead have introduced a second decode-parallelism semaphore alongside the one Coil's bitmap decoders share, and put us in charge of registry ordering against SVG and video frame decoding. - It is the only place that still knows the disk cache key. `FileImageSource.diskCacheKey` is internal to Coil and cannot be copied off an existing source, but `NetworkFetcher` derives it as `options.diskCacheKey ?: url` — so the wrapper is handed the same value and `SuccessResult.diskCacheKey` survives the swap. The re-homed source takes ownership of the one it replaces (`closeable = this`), since the engine closes exactly one source per fetch and that is now the replacement — without it the disk-cache snapshot would leak. `AnimatedImageDecoderFactory` keeps only the frame-delay scan, which is a separate matter: Coil's sub-threshold rewrite is stream-backed and would forfeit the file source again for GIFs on API < 34. Tests: `SystemFileSystemFetcherTest` covers the swap, the pass-throughs (already on SYSTEM, unknown wrapper, stream-backed, non-source result, declining fetcher), the ownership transfer and the carried key — the last two verified by mutation. `SystemFileSystemImageDecoderInstrumentedTest` pins the platform half the JVM tests cannot reach: that `toImageDecoderSourceOrNull` really does return null on our disk cache's file system and non-null once re-homed, and that `StaticImageDecoder.Factory` declines the former and accepts the latter. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017RYCgbvhtBCBNVLMxSWoCJ --- ...mFileSystemImageDecoderInstrumentedTest.kt | 137 +++++++++++ .../images/AnimatedImageDecoderFactory.kt | 66 ++--- .../amethyst/service/images/BlossomFetcher.kt | 4 +- .../service/images/ImageDecoderSources.kt | 93 +++++-- .../service/images/ImageLoaderSetup.kt | 5 +- .../service/images/ProfilePictureFetcher.kt | 2 +- .../service/images/ImageDecoderSourcesTest.kt | 110 --------- .../images/SystemFileSystemFetcherTest.kt | 231 ++++++++++++++++++ 8 files changed, 462 insertions(+), 186 deletions(-) create mode 100644 amethyst/src/androidTest/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemImageDecoderInstrumentedTest.kt delete mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemFetcherTest.kt diff --git a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemImageDecoderInstrumentedTest.kt b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemImageDecoderInstrumentedTest.kt new file mode 100644 index 0000000000..2b7a0072b7 --- /dev/null +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemImageDecoderInstrumentedTest.kt @@ -0,0 +1,137 @@ +/* + * 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.service.images + +import android.graphics.Bitmap +import android.os.Build +import androidx.core.graphics.createBitmap +import androidx.test.ext.junit.runners.AndroidJUnit4 +import coil3.ImageLoader +import coil3.annotation.InternalCoilApi +import coil3.decode.DataSource +import coil3.decode.ImageSource +import coil3.decode.StaticImageDecoder +import coil3.decode.toImageDecoderSourceOrNull +import coil3.fetch.SourceFetchResult +import coil3.request.Options +import com.vitorpamplona.amethyst.AvifInstrumentedTestSupport.appContext +import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import okio.FileSystem +import okio.Path +import okio.Path.Companion.toOkioPath +import org.junit.After +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assume.assumeTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import java.io.File +import java.util.UUID + +/** + * Pins the platform-side half of [SystemFileSystemFetcher], which the JVM unit tests cannot + * reach: `android.graphics.ImageDecoder` is what Coil's identity check gates access to, so only + * a device can show that the check really does fail on our disk cache's file system and really + * does pass once the source is re-homed. + * + * If these ever start passing without the re-home, Coil has loosened the check and + * [SystemFileSystemFetcher] can go. + */ +@RunWith(AndroidJUnit4::class) +class SystemFileSystemImageDecoderInstrumentedTest { + private lateinit var pngFile: File + private lateinit var path: Path + private lateinit var scope: CoroutineScope + private lateinit var deferredDelete: DeferredDeleteFileSystem + + private val imageLoader by lazy { ImageLoader.Builder(appContext).build() } + private val options by lazy { Options(appContext) } + + @Before + fun setUp() { + pngFile = File(appContext.cacheDir.also { it.mkdirs() }, "${UUID.randomUUID()}.png") + pngFile.outputStream().use { createBitmap(4, 4).compress(Bitmap.CompressFormat.PNG, 100, it) } + path = pngFile.toOkioPath() + scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + deferredDelete = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + } + + @After + fun tearDown() { + scope.cancel() + pngFile.delete() + } + + /** How Coil's `NetworkFetcher` builds the source when the disk cache wraps its file system. */ + private fun diskCacheSource() = ImageSource(file = path, fileSystem = deferredDelete, diskCacheKey = KEY) + + private fun reHomedSource() = requireNotNull(diskCacheSource().onSystemFileSystem(KEY)) + + @OptIn(InternalCoilApi::class) + @Test + fun ourDiskCacheFileSystemHidesTheFileFromImageDecoder() { + assertNull( + "still decode path", + diskCacheSource().toImageDecoderSourceOrNull(options, animated = false), + ) + assertNull( + "animated decode path", + diskCacheSource().toImageDecoderSourceOrNull(options, animated = true), + ) + } + + @OptIn(InternalCoilApi::class) + @Test + fun theReHomedSourceReachesImageDecoder() { + assertNotNull( + "still decode path", + reHomedSource().toImageDecoderSourceOrNull(options, animated = false), + ) + assertNotNull( + "animated decode path", + reHomedSource().toImageDecoderSourceOrNull(options, animated = true), + ) + } + + @Test + fun staticImageDecoderDeclinesOurDiskCacheSourceAndAcceptsTheReHomedOne() { + assumeTrue("StaticImageDecoder requires API 29+", Build.VERSION.SDK_INT >= 29) + + // Declining is why every still image in the app was decoding through BitmapFactoryDecoder. + assertNull( + StaticImageDecoder.Factory().create(fetchResult(diskCacheSource()), options, imageLoader), + ) + assertNotNull( + StaticImageDecoder.Factory().create(fetchResult(reHomedSource()), options, imageLoader), + ) + } + + private fun fetchResult(source: ImageSource) = SourceFetchResult(source, "image/png", DataSource.DISK) + + companion object { + private const val KEY = "https://example.com/blob.png" + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt index a6a1ac5a15..c068e59e69 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt @@ -23,7 +23,6 @@ package com.vitorpamplona.amethyst.service.images import android.os.Build.VERSION.SDK_INT import androidx.annotation.RequiresApi import coil3.ImageLoader -import coil3.decode.DecodeResult import coil3.decode.DecodeUtils import coil3.decode.Decoder import coil3.decode.ImageSource @@ -37,21 +36,16 @@ import okio.BufferedSource import okio.buffer /** - * Drop-in replacement for Coil's `AnimatedImageDecoder.Factory` that keeps the platform - * `ImageDecoder` reading straight from the disk-cache file instead of a RAM copy of it. + * Drop-in replacement for Coil's `AnimatedImageDecoder.Factory` that stops asking for the GIF + * frame-delay rewrite when the file does not need it. * - * The sniffing is identical to Coil's. Two things differ, both about *how the bytes reach* - * `ImageDecoder`: - * - * 1. The source is re-homed onto `FileSystem.SYSTEM` — see [onSystemFileSystem] for why - * Amethyst's disk cache otherwise forfeits the file fast path on every network image. - * 2. The GIF frame-delay rewrite Coil applies on API < 34 is asked for only when the file - * actually contains a sub-threshold delay — see [hasSubThresholdGifFrameDelay]. The - * rewrite is stream-backed, so requesting it unconditionally undoes (1) for every GIF. - * - * Together those keep a large GIF off the heap entirely: a 69.8 MB / 201-frame GIF cost - * 66 MB of heap plus a 66 MB direct `ByteBuffer` per decode before this, and animated results - * are never memory-cached, so that repeated on every scroll back into view. + * The sniffing is identical to Coil's. The one difference: below API 34 Coil wraps every GIF in a + * stream-backed `FrameDelayRewritingSource` to clamp sub-threshold frame delays, and a + * stream-backed source has no file for `ImageDecoder` to read, so the decode falls back to + * squashing the whole encoded animation into RAM (see [SystemFileSystemFetcher] for what that + * costs). Nearly every GIF comes out of that rewriter byte-for-byte identical, so + * [hasSubThresholdGifFrameDelay] checks first and the rewrite is requested only for the files + * that would actually change. */ @RequiresApi(28) class AnimatedImageDecoderFactory : Decoder.Factory { @@ -71,29 +65,23 @@ class AnimatedImageDecoderFactory : Decoder.Factory { } /** - * Builds Coil's [AnimatedImageDecoder] over the cheapest source we can give it. + * Builds Coil's [AnimatedImageDecoder], asking for the frame-delay rewrite only when it is both + * possible and needed. * - * [mayNeedFrameDelayRewrite] should be true only for GIFs: Coil's rewriter no-ops on every - * other format, so scanning one would be pure IO for a decision already made. + * [mayNeedFrameDelayRewrite] should be true only for GIFs: Coil's rewriter no-ops on every other + * format, so scanning one would be pure IO for a decision already made. */ @RequiresApi(28) fun newAnimatedImageDecoder( source: ImageSource, options: Options, mayNeedFrameDelayRewrite: Boolean, -): Decoder { - val onSystem = source.onSystemFileSystem() - val decoder = AnimatedImageDecoder(onSystem, options, enforceMinimumFrameDelay(onSystem, mayNeedFrameDelayRewrite)) - - // A re-homed source is ours, so nobody else will close it: Coil's engine closes the - // source it handed us, and AnimatedImageDecoder only closes the frame-delay wrapper. - return if (onSystem === source) decoder else ClosingDecoder(onSystem, decoder) -} +): Decoder = AnimatedImageDecoder(source, options, enforceMinimumFrameDelay(source, mayNeedFrameDelayRewrite)) /** - * Whether to ask Coil to clamp this GIF's sub-threshold frame delays. From API 34 the - * platform decoder does it itself, which is why Coil's own default turns the rewrite off - * there; below that we pay for it only when the file really has a delay to clamp. + * Whether to ask Coil to clamp this GIF's sub-threshold frame delays. From API 34 the platform + * decoder does it itself, which is why Coil's own default turns the rewrite off there; below that + * we pay for it only when the file really has a delay to clamp. */ private fun enforceMinimumFrameDelay( source: ImageSource, @@ -101,8 +89,8 @@ private fun enforceMinimumFrameDelay( ): Boolean { if (!mayNeedFrameDelayRewrite || SDK_INT >= 34) return false - // Not file-backed: the decode squashes the stream into RAM either way, so there is no - // fast path to protect and no reason to deviate from Coil's default. + // Not file-backed: the decode squashes the stream into RAM either way, so there is no fast + // path to protect and no reason to deviate from Coil's default. val file = source.fileOrNull() ?: return true return source.fileSystem @@ -110,19 +98,3 @@ private fun enforceMinimumFrameDelay( .buffer() .use { it.hasSubThresholdGifFrameDelay() } } - -/** Closes [source] once [delegate] is done with it. */ -private class ClosingDecoder( - private val source: ImageSource, - private val delegate: Decoder, -) : Decoder { - override suspend fun decode(): DecodeResult? = - try { - delegate.decode() - } finally { - try { - source.close() - } catch (_: Exception) { - } - } -} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/BlossomFetcher.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/BlossomFetcher.kt index 1ac58c8803..c935cd1b41 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/BlossomFetcher.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/BlossomFetcher.kt @@ -89,7 +89,9 @@ class BlossomFetcher( connectivityChecker = lazy { connectivityCheckerLazy.get(options.context) }, concurrentRequestStrategy = concurrentRequestStrategyLazy, ) - } + // Keyed on the resolved server url, which is what the NetworkFetcher above + // caches under -- not on the `blossom:` uri the request came in as. + }.onSystemFileSystem(options.diskCacheKey ?: url) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt index 10c9e6247e..86d271c53f 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt @@ -21,16 +21,19 @@ package com.vitorpamplona.amethyst.service.images import coil3.decode.ImageSource +import coil3.fetch.FetchResult +import coil3.fetch.Fetcher +import coil3.fetch.SourceFetchResult import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem import okio.FileSystem /** - * Re-points a disk-cache-backed [ImageSource] at [FileSystem.SYSTEM] so Coil's platform - * `ImageDecoder` fast path stays reachable. + * Wraps a network [Fetcher] so the disk-cache file it hands back is addressed through + * [FileSystem.SYSTEM] itself instead of through our [DeferredDeleteFileSystem] wrapper. * - * Coil hands `android.graphics.ImageDecoder` a *file* only when the source's file system is - * **referentially** [FileSystem.SYSTEM] (`ImageSource.toImageDecoderSourceOrNull`, verified - * against Coil 3.5.0): + * Coil reaches for the platform `android.graphics.ImageDecoder` only when an image source's + * file system is **referentially** [FileSystem.SYSTEM] (`ImageSource.toImageDecoderSourceOrNull`, + * verified against Coil 3.5.0): * * ``` * if (fileSystem === FileSystem.SYSTEM) { @@ -39,33 +42,71 @@ import okio.FileSystem * } * ``` * - * The source Coil's `NetworkFetcher` builds carries `diskCache.fileSystem`, and ours is a - * [DeferredDeleteFileSystem] wrapper, so that identity check fails for **every** image we - * fetch from the network. The fallback for an animated image is - * `ImageDecoder.createSource(source.squashToDirectByteBuffer())`, which pulls the entire - * encoded animation onto the heap and then copies it into an equally large direct - * `ByteBuffer` that stays alive for as long as the `AnimatedImageDrawable` does. Measured on - * a 69.8 MB / 201-frame GIF: 66 MB of heap plus 66 MB of native memory per decode — and - * animated results are never memory-cached (`DrawableImage.shareable` is false), so the feed - * pays it again every time the note scrolls back into view. + * Coil's `NetworkFetcher` stamps the source it builds with `diskCache.fileSystem`, and ours is a + * [DeferredDeleteFileSystem] wrapper, so that identity check failed for **every** image the app + * fetched from the network. Two decoders quietly lost their fast path because of it: * - * Only the delete path is deferred by the wrapper; reads go straight to - * [FileSystem.SYSTEM]. So handing the decoder the same file on the real system file system - * is equivalent, and it is exactly what Coil does when no wrapper is installed. + * - `AnimatedImageDecoder` fell back to + * `ImageDecoder.createSource(source.squashToDirectByteBuffer())`, pulling the whole encoded + * animation onto the heap and then copying it into an equally large direct `ByteBuffer` that + * lives as long as the `AnimatedImageDrawable`. Replaying that over a 69.8 MB GIF measured + * 66 MB of heap plus 66 MB of native memory, and animated results are never memory-cached + * (`DrawableImage.shareable` is false), so the feed paid it again on every scroll back. + * - `StaticImageDecoder.Factory` declined outright (it returns null when it cannot get an + * `ImageDecoder.Source`), so every still image decoded through `BitmapFactoryDecoder`. * - * Returns `this` unchanged when there is nothing to gain: already on [FileSystem.SYSTEM], not - * a wrapper we know unwraps to it, or not backed by a file yet. [ImageSource.fileOrNull] is - * used rather than [ImageSource.file] on purpose — the latter would materialise a temp copy - * of a stream-backed source, which is the very cost this exists to avoid. + * Only deletes are deferred by the wrapper — reads already go straight to [FileSystem.SYSTEM] — + * so handing decoders the same file on the real system file system is equivalent, and it is what + * Coil does when no wrapper is installed. + * + * This sits at the fetcher rather than at each decoder on purpose: one seam fixes every decoder + * at once, it keeps Coil's own decoder registration (and with it the parallelism semaphore the + * bitmap decoders share), and it is the only place that still knows [diskCacheKey], which + * `FileImageSource` exposes internally and cannot be copied off an existing source. */ -fun ImageSource.onSystemFileSystem(): ImageSource { - if (fileSystem === FileSystem.SYSTEM) return this +class SystemFileSystemFetcher( + private val delegate: Fetcher, + private val diskCacheKey: String, +) : Fetcher { + override suspend fun fetch(): FetchResult? { + val result = delegate.fetch() + if (result !is SourceFetchResult) return result + + val onSystem = result.source.onSystemFileSystem(diskCacheKey) ?: return result + + return SourceFetchResult(onSystem, result.mimeType, result.dataSource) + } +} + +/** This fetcher, with any disk-cache-backed result re-homed onto [FileSystem.SYSTEM]. */ +fun Fetcher.onSystemFileSystem(diskCacheKey: String): Fetcher = SystemFileSystemFetcher(this, diskCacheKey) + +/** + * The same bytes, addressed through [FileSystem.SYSTEM], or null when there is nothing to gain: + * already on it, wrapped in something other than a [DeferredDeleteFileSystem] (which may rewrite + * paths or serve different bytes, so the file underneath is not ours to hand out), or not backed + * by a file yet. + * + * [ImageSource.fileOrNull] is used rather than [ImageSource.file] on purpose — the latter would + * materialise a temp copy of a stream-backed source, which is the very cost this exists to avoid. + */ +fun ImageSource.onSystemFileSystem(diskCacheKey: String): ImageSource? { + if (fileSystem === FileSystem.SYSTEM) return null var unwrapped: FileSystem = fileSystem while (unwrapped is DeferredDeleteFileSystem) unwrapped = unwrapped.delegate - if (unwrapped !== FileSystem.SYSTEM) return this + if (unwrapped !== FileSystem.SYSTEM) return null - val file = fileOrNull() ?: return this + val file = fileOrNull() ?: return null - return ImageSource(file = file, fileSystem = FileSystem.SYSTEM, metadata = metadata) + return ImageSource( + file = file, + fileSystem = FileSystem.SYSTEM, + diskCacheKey = diskCacheKey, + // Takes ownership of the source it replaces. Only the returned source reaches the engine, + // and the engine closes exactly one source per fetch, so this is what releases the + // disk-cache snapshot holding `file` open. + closeable = this, + metadata = metadata, + ) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt index 8fd444ffea..96cf0f2ff9 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt @@ -170,6 +170,9 @@ class OkHttpFactory( val url = data.toString() + // onSystemFileSystem keeps the platform ImageDecoder reachable for whatever comes back + // -- see [SystemFileSystemFetcher]. The key it carries is the one NetworkFetcher derives + // for the same request (`options.diskCacheKey ?: url`). return readAuthAware(url, readAuth) { authHeader -> NetworkFetcher( url = url, @@ -180,7 +183,7 @@ class OkHttpFactory( connectivityChecker = lazy { connectivityCheckerLazy.get(options.context) }, concurrentRequestStrategy = concurrentRequestStrategyLazy, ) - } + }.onSystemFileSystem(options.diskCacheKey ?: url) } private fun isApplicable(data: Uri): Boolean = data.scheme == "http" || data.scheme == "https" diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ProfilePictureFetcher.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ProfilePictureFetcher.kt index b8f56eaba4..8eb0154855 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ProfilePictureFetcher.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ProfilePictureFetcher.kt @@ -127,7 +127,7 @@ class ProfilePictureFetcher( connectivityChecker = lazy { connectivityCheckerLazy.get(options.context) }, concurrentRequestStrategy = concurrentRequestStrategyLazy, ) - } + }.onSystemFileSystem(options.diskCacheKey ?: data.url) return ProfilePictureFetcher( data.url, diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt deleted file mode 100644 index 7819ed13e8..0000000000 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSourcesTest.kt +++ /dev/null @@ -1,110 +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.service.images - -import coil3.decode.ImageSource -import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancel -import okio.Buffer -import okio.FileSystem -import okio.ForwardingFileSystem -import okio.Path -import okio.Path.Companion.toOkioPath -import org.junit.After -import org.junit.Assert.assertArrayEquals -import org.junit.Assert.assertNull -import org.junit.Assert.assertSame -import org.junit.Before -import org.junit.Test -import java.io.File -import java.nio.file.Files - -/** - * [onSystemFileSystem] exists to satisfy an identity check inside Coil - * (`ImageSource.toImageDecoderSourceOrNull` only reaches for the file when - * `fileSystem === FileSystem.SYSTEM`). These tests pin the property that check reads, since - * getting it wrong is silent: the decode still succeeds, it just copies the whole encoded - * image into RAM first. - */ -class ImageDecoderSourcesTest { - private lateinit var tmpRoot: File - private lateinit var file: Path - private lateinit var scope: CoroutineScope - - private val bytes = ByteArray(64) { it.toByte() } - - @Before - fun setUp() { - tmpRoot = Files.createTempDirectory("image-decoder-sources-test").toFile() - file = tmpRoot.resolve("blob.gif").toOkioPath() - FileSystem.SYSTEM.write(file) { write(bytes) } - scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) - } - - @After - fun tearDown() { - scope.cancel() - tmpRoot.deleteRecursively() - } - - @Test - fun deferredDeleteWrapper_isUnwrappedToTheSystemFileSystem() { - val source = ImageSource(file = file, fileSystem = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope)) - - val rehomed = source.onSystemFileSystem() - - assertSame(FileSystem.SYSTEM, rehomed.fileSystem) - assertSame(file, rehomed.fileOrNull()) - assertArrayEquals(bytes, rehomed.source().readByteArray()) - rehomed.close() - } - - @Test - fun alreadyOnTheSystemFileSystem_isReturnedUntouched() { - val source = ImageSource(file = file, fileSystem = FileSystem.SYSTEM) - - assertSame(source, source.onSystemFileSystem()) - } - - @Test - fun anUnknownWrapper_isLeftAlone() { - // Only DeferredDeleteFileSystem is known to forward reads verbatim. Anything else may - // rewrite paths or serve different bytes, so the file underneath is not ours to hand out. - val source = ImageSource(file = file, fileSystem = object : ForwardingFileSystem(FileSystem.SYSTEM) {}) - - assertSame(source, source.onSystemFileSystem()) - } - - @Test - fun aStreamBackedSource_isNotMaterialisedIntoATempFile() { - val source = - ImageSource( - source = Buffer().write(bytes), - fileSystem = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope), - ) - - assertSame(source, source.onSystemFileSystem()) - assertNull("re-homing must not force a temp copy of a stream", source.fileOrNull()) - } -} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemFetcherTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemFetcherTest.kt new file mode 100644 index 0000000000..98ebcd8fd8 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/SystemFileSystemFetcherTest.kt @@ -0,0 +1,231 @@ +/* + * 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.service.images + +import coil3.ColorImage +import coil3.decode.DataSource +import coil3.decode.ImageSource +import coil3.fetch.FetchResult +import coil3.fetch.Fetcher +import coil3.fetch.ImageFetchResult +import coil3.fetch.SourceFetchResult +import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.test.runTest +import okio.Buffer +import okio.FileSystem +import okio.ForwardingFileSystem +import okio.Path +import okio.Path.Companion.toOkioPath +import org.junit.After +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.io.File +import java.nio.file.Files + +/** + * Coil hands the platform `ImageDecoder` a file only when the source's file system is + * *referentially* `FileSystem.SYSTEM` (`ImageSource.toImageDecoderSourceOrNull`). Our disk cache + * wraps it in a [DeferredDeleteFileSystem], so without this fetcher every network image loses + * that path — silently: the decode still succeeds, it just copies the encoded image into RAM + * first (animated) or drops to `BitmapFactoryDecoder` (static). These tests pin the property that + * identity check reads, and the ownership transfer that keeps the disk-cache snapshot alive + * exactly as long as the source that replaces it. + */ +class SystemFileSystemFetcherTest { + private lateinit var tmpRoot: File + private lateinit var file: Path + private lateinit var scope: CoroutineScope + private lateinit var deferredDelete: DeferredDeleteFileSystem + + private val bytes = ByteArray(64) { it.toByte() } + + @Before + fun setUp() { + tmpRoot = Files.createTempDirectory("system-file-system-fetcher-test").toFile() + file = tmpRoot.resolve("blob.gif").toOkioPath() + FileSystem.SYSTEM.write(file) { write(bytes) } + scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + deferredDelete = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + } + + @After + fun tearDown() { + scope.cancel() + tmpRoot.deleteRecursively() + } + + private class FakeFetcher( + private val result: FetchResult?, + ) : Fetcher { + override suspend fun fetch() = result + } + + /** Records whether the disk-cache snapshot Coil would hand us has been released. */ + private class Snapshot : AutoCloseable { + var closed = false + private set + + override fun close() { + closed = true + } + } + + private fun diskCacheSource(snapshot: AutoCloseable? = null) = + ImageSource( + file = file, + fileSystem = deferredDelete, + diskCacheKey = "https://example.com/blob.gif", + closeable = snapshot, + ) + + @Test + fun aDiskCacheSourceIsHandedOnTheSystemFileSystem() = + runTest { + val fetcher = + FakeFetcher(SourceFetchResult(diskCacheSource(), "image/gif", DataSource.DISK)) + .onSystemFileSystem("https://example.com/blob.gif") + + val result = fetcher.fetch() as SourceFetchResult + + assertSame( + "Coil only reaches ImageDecoder when this is referentially FileSystem.SYSTEM", + FileSystem.SYSTEM, + result.source.fileSystem, + ) + assertSame(file, result.source.fileOrNull()) + assertArrayEquals(bytes, result.source.source().readByteArray()) + result.source.close() + } + + @Test + fun theRestOfTheResultSurvivesTheSwap() = + runTest { + val fetcher = + FakeFetcher(SourceFetchResult(diskCacheSource(), "image/gif", DataSource.NETWORK)) + .onSystemFileSystem("https://example.com/blob.gif") + + val result = fetcher.fetch() as SourceFetchResult + + assertEquals("image/gif", result.mimeType) + assertEquals(DataSource.NETWORK, result.dataSource) + } + + @Test + fun closingTheReplacementReleasesTheDiskCacheSnapshot() = + runTest { + // The engine closes exactly one source per fetch, and after the swap that is the + // replacement. If it did not own the original, the snapshot would leak and the cache + // entry would stay open forever. + val snapshot = Snapshot() + val fetcher = + FakeFetcher(SourceFetchResult(diskCacheSource(snapshot), null, DataSource.DISK)) + .onSystemFileSystem("https://example.com/blob.gif") + + val result = fetcher.fetch() as SourceFetchResult + assertFalse(snapshot.closed) + + result.source.close() + + assertTrue("closing the re-homed source must release the snapshot", snapshot.closed) + } + + @Test + fun theDiskCacheKeyIsCarriedOver() = + runTest { + // FileImageSource.diskCacheKey is internal to Coil, but the engine reads it off the + // source to populate SuccessResult.diskCacheKey. Rebuilding the source without it + // would drop that silently, so reach for the field directly. + val fetcher = + FakeFetcher(SourceFetchResult(diskCacheSource(), null, DataSource.DISK)) + .onSystemFileSystem("https://example.com/blob.gif") + + val result = fetcher.fetch() as SourceFetchResult + + val field = + result.source.javaClass.declaredFields + .firstOrNull { it.name == "diskCacheKey" } + assertNotNull("Coil renamed FileImageSource.diskCacheKey; re-check the re-home", field) + field!!.isAccessible = true + assertEquals("https://example.com/blob.gif", field.get(result.source)) + } + + @Test + fun aSourceAlreadyOnTheSystemFileSystemIsLeftAlone() = + runTest { + val original = SourceFetchResult(ImageSource(file, FileSystem.SYSTEM), null, DataSource.DISK) + val fetcher = FakeFetcher(original).onSystemFileSystem("key") + + assertSame(original, fetcher.fetch()) + } + + @Test + fun anUnknownFileSystemWrapperIsLeftAlone() = + runTest { + // Only DeferredDeleteFileSystem is known to forward reads verbatim. Anything else may + // rewrite paths or serve different bytes, so the file underneath is not ours to hand out. + val original = + SourceFetchResult( + ImageSource(file, object : ForwardingFileSystem(FileSystem.SYSTEM) {}), + null, + DataSource.DISK, + ) + val fetcher = FakeFetcher(original).onSystemFileSystem("key") + + assertSame(original, fetcher.fetch()) + } + + @Test + fun aStreamBackedSourceIsNotMaterialisedIntoATempFile() = + runTest { + val source = ImageSource(Buffer().write(bytes), deferredDelete) + val original = SourceFetchResult(source, null, DataSource.NETWORK) + val fetcher = FakeFetcher(original).onSystemFileSystem("key") + + assertSame(original, fetcher.fetch()) + assertNull("re-homing must not force a temp copy of a stream", source.fileOrNull()) + } + + @Test + fun aNonSourceResultPassesStraightThrough() = + runTest { + // ProfilePictureFetcher answers thumbnail hits with an ImageFetchResult. + val original = ImageFetchResult(ColorImage(0), isSampled = true, DataSource.DISK) + + assertSame(original, FakeFetcher(original).onSystemFileSystem("key").fetch()) + } + + @Test + fun aFetcherThatDeclinesStillDeclines() = + runTest { + assertNull(FakeFetcher(null).onSystemFileSystem("key").fetch()) + } +} From 8ded3c13c50adfc4d6dde77c64bc7f834d0f2eb6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 15:15:07 +0000 Subject: [PATCH 3/5] fix(images): reclaim image-cache files orphaned by a process death MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DeferredDeleteFileSystem moves Coil's eviction unlink() off the DiskLruCache lock by queueing it in memory. Anything still queued when the process dies is never unlinked — and Coil cannot recover it: DiskLruCache.processJournal() derives `size` purely from the journal's recorded lengths and never scans the directory for files it does not know about. The orphan counts toward neither `size` nor eviction, so the cache directory keeps the residue of every killed process, forever, drifting past its own cap. Measured against the real Coil DiskCache: 48 unlinks lost to one process death left Coil reporting 16 KB against a 16 KB budget while the directory actually held 48 KB, and reopening as a fresh process reclaimed none of it. With maxSizePercent(0.2) capped at 1 GB, that drift is unbounded over the app's life. ImageDiskCacheReconciler runs once at startup on the IO scope: it walks the cache directory and, only if it holds more than its budget plus slack, empties it. Two steps, because DiskCache.clear() alone is not enough — evictAll() walks lruEntries, exactly the set of files the journal knows about, so it goes right past the orphans. So: clear() to make the journal's truth empty, then unlink every non-journal file left in the directory, which is by then unreferenced by definition. The unlinks go through the same deferred file system as any other eviction; the pending set dedupes the paths clear() already queued. It is deliberately blunt — it costs the whole cache — so the trigger sits well past what normal operation needs: budget + max(25%, 4 MiB). Async eviction and the dirty files of writes in flight both put a healthy directory a little over budget; only real drift is wiped. On a healthy cache the pass is one directory walk and nothing else, and it never reads DiskCache.size, so it does not force the journal parse on the happy path or couple to Coil's on-disk format. The startup call is also the one place that forces the `diskCache` lazy, so its build (a statvfs for the size budget) and this walk both land on IO rather than on whichever thread happens to load the first image. Tests cover the end-to-end leak (inert drainer stands in for the dead process, then a fresh cache over the same directory reconciles it back under budget), the healthy no-op, drift inside the slack, a missing directory, and the shipped ceiling arithmetic. clearAlone_wouldNotHaveReclaimedThem pins the reason this class exists — if Coil ever learns to sweep, it fails and the class can go. Verified by mutation: dropping the orphan sweep fails the leak test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017RYCgbvhtBCBNVLMxSWoCJ --- .../com/vitorpamplona/amethyst/AppModules.kt | 14 ++ .../images/ImageDiskCacheReconciler.kt | 149 +++++++++++ .../images/ImageDiskCacheReconcilerTest.kt | 237 ++++++++++++++++++ 3 files changed, 400 insertions(+) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt index f9d26eafe5..c59319adb1 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt @@ -82,6 +82,7 @@ import com.vitorpamplona.amethyst.service.crashreports.CrashReportCache import com.vitorpamplona.amethyst.service.crashreports.UnexpectedCrashSaver import com.vitorpamplona.amethyst.service.eventCache.MemoryTrimmingService import com.vitorpamplona.amethyst.service.images.ImageCacheFactory +import com.vitorpamplona.amethyst.service.images.ImageDiskCacheReconciler import com.vitorpamplona.amethyst.service.images.ImageLoaderSetup import com.vitorpamplona.amethyst.service.images.ThumbnailDiskCache import com.vitorpamplona.amethyst.service.location.LocationState @@ -1136,6 +1137,19 @@ class AppModules( } } + // Reclaim image-cache files orphaned by a process death that lost DeferredDeleteFileSystem's + // queued unlinks — Coil cannot see them, so without this the directory keeps every killed + // process's residue and drifts past its own cap forever. See ImageDiskCacheReconciler. + // + // Also the one place that forces the `diskCache` lazy, so its build (a statvfs for the size + // budget) and this walk both land on IO rather than on whichever thread loads an image first. + applicationIOScope.launch { + val result = ImageDiskCacheReconciler.reconcile(diskCache) + if (result.wasOverBudget) { + Log.i("AppModules") { "Image cache was over budget: wiped ${result.reclaimedFiles} files (${result.bytesOnDisk} bytes on disk, ${result.budgetBytes} budget)" } + } + } + applicationIOScope.launch { // loads main account quickly. LocalPreferences.loadAccountConfigFromEncryptedStorage() diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt new file mode 100644 index 0000000000..4c4ad37bab --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt @@ -0,0 +1,149 @@ +/* + * 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.service.images + +import coil3.disk.DiskCache +import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem +import com.vitorpamplona.quartz.utils.Log +import okio.IOException +import okio.Path + +/** What one [ImageDiskCacheReconciler] pass found, and how many files it unlinked. */ +data class ImageCacheReconciliation( + val bytesOnDisk: Long, + val budgetBytes: Long, + val ceilingBytes: Long, + val reclaimedFiles: Int, +) { + val wasOverBudget get() = reclaimedFiles > 0 +} + +/** + * Brings the image cache directory back under its own size budget at startup. + * + * [DeferredDeleteFileSystem] moves Coil's eviction `unlink()` off the `DiskLruCache` lock by + * queueing it in memory. Anything still queued when the process dies is never unlinked — and Coil + * cannot recover it, because `DiskLruCache.processJournal()` computes `size` purely from the + * journal's recorded lengths and never scans the directory for files it does not know about. The + * orphan counts toward neither `size` nor eviction, so the directory keeps whatever residue every + * killed process left behind, forever. + * + * Measured against the real Coil `DiskCache`: 48 unlinks lost to one process death left Coil + * reporting 16 KB against a 16 KB budget while the directory actually held 48 KB — and reopening + * as a fresh process reclaimed none of it. + * + * `DiskCache.clear()` is not enough on its own: it evicts `lruEntries`, which is exactly the set of + * files the journal knows about, so it walks straight past the orphans. Hence the two steps below — + * empty the journal, then unlink whatever is still sitting in the directory. + * + * This is deliberately blunt. It costs the whole cache, so it only fires once the directory has + * drifted past [ceilingBytes] — well beyond the slack normal operation needs — and on a healthy + * cache it is one directory walk and nothing else. It does not read the journal, so it stays + * independent of Coil's on-disk format. + */ +object ImageDiskCacheReconciler { + /** + * Fraction of the cache's own budget allowed on top of it before the directory counts as + * drifted rather than merely busy. + */ + private const val DEFAULT_SLACK_FRACTION = 0.25 + + /** + * Floor for that slack, so a cache with a small budget (20% of a nearly-full disk) is not wiped + * over a few megabytes. Covers the journal plus the dirty files of any writes in flight. + */ + private const val DEFAULT_MIN_SLACK_BYTES = 4L * 1024 * 1024 + + /** Coil's own bookkeeping, which is not entry data and must survive a wipe. */ + private val JOURNAL_FILES = setOf("journal", "journal.tmp", "journal.bkp") + + /** Bytes the directory may hold before [reconcile] wipes it. */ + fun ceilingBytes( + budgetBytes: Long, + slackFraction: Double = DEFAULT_SLACK_FRACTION, + minSlackBytes: Long = DEFAULT_MIN_SLACK_BYTES, + ): Long = budgetBytes + maxOf((budgetBytes * slackFraction).toLong(), minSlackBytes) + + /** + * Walks the cache directory and, if it holds more than [ceilingBytes], empties it. + * + * Blocking IO — call it from a background dispatcher. + * + * Deliberately does not read `DiskCache.size`: that would force the journal parse on the happy + * path, and the decision does not need it. A request racing the wipe can lose the entry it was + * writing; Coil treats that as a cache miss and re-fetches, which is why this runs at startup + * rather than on a timer. + */ + fun reconcile( + diskCache: DiskCache, + slackFraction: Double = DEFAULT_SLACK_FRACTION, + minSlackBytes: Long = DEFAULT_MIN_SLACK_BYTES, + ): ImageCacheReconciliation { + val budget = diskCache.maxSize + val ceiling = ceilingBytes(budget, slackFraction, minSlackBytes) + + val bytesOnDisk = regularFiles(diskCache).sumOf { sizeOf(diskCache, it) } + if (bytesOnDisk <= ceiling) { + return ImageCacheReconciliation(bytesOnDisk, budget, ceiling, 0) + } + + Log.w( + "ImageDiskCache", + "Image cache holds $bytesOnDisk bytes against a $budget budget (ceiling $ceiling) — " + + "wiping it. Deferred unlinks lost to a process death leave files Coil can no longer see.", + ) + + // Empties the journal, so every entry file left below is unreferenced by definition. + // Its own unlinks go through the deferred file system like any other eviction; the paths + // below are re-queued rather than double-unlinked, which the pending set dedupes. + diskCache.clear() + + var reclaimed = 0 + regularFiles(diskCache).forEach { path -> + if (path.name in JOURNAL_FILES) return@forEach + try { + diskCache.fileSystem.delete(path, mustExist = false) + reclaimed++ + } catch (e: IOException) { + Log.w("ImageDiskCache", "could not unlink $path", e) + } + } + + return ImageCacheReconciliation(bytesOnDisk, budget, ceiling, reclaimed) + } + + private fun regularFiles(diskCache: DiskCache): List = + try { + diskCache.fileSystem + .listOrNull(diskCache.directory) + .orEmpty() + .filter { diskCache.fileSystem.metadataOrNull(it)?.isRegularFile == true } + } catch (e: IOException) { + // A missing directory is the normal first-launch state, not a failure. + Log.d("ImageDiskCache") { "could not list ${diskCache.directory}: ${e.message}" } + emptyList() + } + + private fun sizeOf( + diskCache: DiskCache, + path: Path, + ): Long = diskCache.fileSystem.metadataOrNull(path)?.size ?: 0L +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt new file mode 100644 index 0000000000..3f458371ed --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt @@ -0,0 +1,237 @@ +/* + * 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.service.images + +import coil3.disk.DiskCache +import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import okio.FileSystem +import okio.Path +import okio.Path.Companion.toOkioPath +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.io.File +import java.nio.file.Files + +/** + * The leak this guards against is silent from inside Coil: a deferred unlink lost to a process + * death leaves a file that `DiskLruCache.processJournal()` never counts and never evicts, because + * it derives `size` from the journal alone. So these tests measure the directory, not the cache. + */ +class ImageDiskCacheReconcilerTest { + private lateinit var tmpRoot: File + private lateinit var cacheDir: Path + private lateinit var scope: CoroutineScope + + @Before + fun setUp() { + tmpRoot = Files.createTempDirectory("image-cache-reconciler-test").toFile() + cacheDir = tmpRoot.resolve("image_cache").toOkioPath() + scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + } + + @After + fun tearDown() { + scope.cancel() + tmpRoot.deleteRecursively() + } + + private fun bytesOnDisk(): Long = + FileSystem.SYSTEM + .listOrNull(cacheDir) + .orEmpty() + .sumOf { FileSystem.SYSTEM.metadataOrNull(it)?.size ?: 0L } + + private fun newCache( + fileSystem: FileSystem, + maxSize: Long, + ) = DiskCache + .Builder() + .directory(cacheDir) + .fileSystem(fileSystem) + .maxSizeBytes(maxSize) + .build() + + private fun writeEntry( + diskCache: DiskCache, + key: String, + bytes: Int, + ) { + val editor = diskCache.openEditor(key) ?: error("openEditor returned null for $key") + try { + diskCache.fileSystem.write(editor.data) { write(ByteArray(bytes) { 1 }) } + editor.commit() + } catch (e: Throwable) { + editor.abort() + throw e + } + } + + @Test + fun orphansLostToAProcessDeath_areReclaimed() { + // "Process one": an inert drainer stands in for a process that died with eviction's + // unlinks still queued in memory. + val deadProcessFs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + scope.cancel("the drainer never runs — the process died first") + + val budget = 16L * 1024 + val first = newCache(deadProcessFs, budget) + repeat(40) { i -> writeEntry(first, "key$i", 1024) } + val deadline = System.currentTimeMillis() + 5_000 + while (first.size > budget && System.currentTimeMillis() < deadline) Thread.sleep(20) + first.shutdown() + + val leaked = bytesOnDisk() + assertTrue( + "the orphans must actually exceed the ceiling, or the test proves nothing (onDisk=$leaked)", + leaked > ImageDiskCacheReconciler.ceilingBytes(budget, minSlackBytes = 0), + ) + + // "Process two": a fresh start over the same directory. + val liveScope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val liveFs = DeferredDeleteFileSystem(FileSystem.SYSTEM, liveScope) + val second = newCache(liveFs, budget) + try { + val result = ImageDiskCacheReconciler.reconcile(second, minSlackBytes = 0) + + assertTrue("should have judged the directory over budget", result.wasOverBudget) + assertEquals(leaked, result.bytesOnDisk) + + // The reconciler unlinks through the same deferred file system as everything else. + liveFs.drainNow() + + assertTrue( + "directory must come back under budget (was $leaked, now ${bytesOnDisk()})", + bytesOnDisk() <= budget, + ) + } finally { + second.shutdown() + liveScope.cancel() + } + } + + @Test + fun clearAlone_wouldNotHaveReclaimedThem() { + // Pins why the reconciler does not just call DiskCache.clear(): evictAll() walks + // lruEntries, which is exactly the set of files the journal knows about, so orphans + // survive it. If this ever starts failing, Coil learned to sweep and this class can go. + val deadProcessFs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + scope.cancel("inert drainer") + + val budget = 16L * 1024 + val first = newCache(deadProcessFs, budget) + repeat(40) { i -> writeEntry(first, "key$i", 1024) } + val deadline = System.currentTimeMillis() + 5_000 + while (first.size > budget && System.currentTimeMillis() < deadline) Thread.sleep(20) + first.shutdown() + + val leaked = bytesOnDisk() + + val liveScope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val liveFs = DeferredDeleteFileSystem(FileSystem.SYSTEM, liveScope) + val second = newCache(liveFs, budget) + try { + second.clear() + liveFs.drainNow() + + assertTrue( + "clear() should leave the orphans behind (was $leaked, now ${bytesOnDisk()})", + bytesOnDisk() > budget, + ) + } finally { + second.shutdown() + liveScope.cancel() + } + } + + @Test + fun aHealthyCacheIsLeftAlone() { + val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + val budget = 1024L * 1024 + val diskCache = newCache(fs, budget) + try { + repeat(10) { i -> writeEntry(diskCache, "key$i", 1024) } + val before = bytesOnDisk() + + val result = ImageDiskCacheReconciler.reconcile(diskCache) + + assertFalse(result.wasOverBudget) + assertEquals(0, result.reclaimedFiles) + assertEquals(before, bytesOnDisk()) + assertNotNull("entries must still be readable", diskCache.openSnapshot("key9")?.also { it.close() }) + } finally { + diskCache.shutdown() + } + } + + @Test + fun driftWithinTheSlackIsLeftAlone() { + // Being a little over the budget is normal: eviction is async, and a write in flight has a + // dirty file on disk that the journal has not accounted for yet. Only real drift is wiped. + val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + val budget = 16L * 1024 + val diskCache = newCache(fs, budget) + try { + repeat(18) { i -> writeEntry(diskCache, "key$i", 1024) } + + val result = ImageDiskCacheReconciler.reconcile(diskCache, minSlackBytes = 8 * 1024) + + assertFalse("18 KB against a 16 KB budget + 8 KB slack is not drift", result.wasOverBudget) + } finally { + diskCache.shutdown() + } + } + + @Test + fun aMissingDirectoryIsANoOp() { + val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + val diskCache = newCache(fs, 1024L * 1024) + try { + FileSystem.SYSTEM.deleteRecursively(cacheDir) + + val result = ImageDiskCacheReconciler.reconcile(diskCache) + + assertEquals(0L, result.bytesOnDisk) + assertFalse(result.wasOverBudget) + } finally { + diskCache.shutdown() + } + } + + @Test + fun theShippedCeilingIsBudgetPlusAQuarter() { + // Pins the production numbers the injectable overrides above bypass. + val oneGb = 1024L * 1024 * 1024 + assertEquals(oneGb + oneGb / 4, ImageDiskCacheReconciler.ceilingBytes(oneGb)) + + // ...but never less than the 4 MiB floor, so a small budget on a full disk is not wiped + // over the journal plus a couple of in-flight writes. + assertEquals(1024L * 1024 + 4L * 1024 * 1024, ImageDiskCacheReconciler.ceilingBytes(1024L * 1024)) + } +} From 0abcf02384f242dadd54a97c7edad8e3c4182425 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 15:41:29 +0000 Subject: [PATCH 4/5] perf(images): rate-limit the cache reconciler to once a day MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reconciler ran on every AppModules.initiate(), which is every process start — including the WorkManager wake-ups that cold-start the whole graph, and which the ledger already counts because that churn is a known problem. Even the healthy pass is a readdir plus a stat per file: on a full 1 GB cache that is tens of thousands of syscalls, paid on every start, to look for drift that accrues only when a process dies with unlinks still queued. reconcileIfDue() gates the walk on the mtime of an empty `.reconciled` marker in the cache directory, so a start inside the interval costs one stat instead. The marker lives with the thing it describes: clearing the app's cache from Settings takes it too, and the next start reconciles a directory whose history we no longer know. A marker dated in the future — a clock that jumped back, or a restored backup — counts as due, so it cannot park the check until real time catches up. The marker is a plain file in the swept directory, so it joins the journal files in the preserved set. reconcileIfDue() rewrites it after a pass anyway, which is exactly why theMarkerSurvivesAWipe drives reconcile() directly — through reconcileIfDue() the rewrite masks the deletion and the test guards nothing. Verified by mutation: dropping the marker from the preserved set fails that test, and short-circuiting isDue() fails both interval tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017RYCgbvhtBCBNVLMxSWoCJ --- .../com/vitorpamplona/amethyst/AppModules.kt | 9 +- .../images/ImageDiskCacheReconciler.kt | 83 ++++++++++++- .../images/ImageDiskCacheReconcilerTest.kt | 109 ++++++++++++++++++ 3 files changed, 192 insertions(+), 9 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt index c59319adb1..bf1eeabdc5 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt @@ -1141,11 +1141,14 @@ class AppModules( // queued unlinks — Coil cannot see them, so without this the directory keeps every killed // process's residue and drifts past its own cap forever. See ImageDiskCacheReconciler. // + // Rate-limited to once a day: drift accrues over process deaths, not over startups, and this + // runs on every one of them — including the WorkManager wake-ups that cold-start the graph. + // // Also the one place that forces the `diskCache` lazy, so its build (a statvfs for the size - // budget) and this walk both land on IO rather than on whichever thread loads an image first. + // budget) and the check both land on IO rather than on whichever thread loads an image first. applicationIOScope.launch { - val result = ImageDiskCacheReconciler.reconcile(diskCache) - if (result.wasOverBudget) { + val result = ImageDiskCacheReconciler.reconcileIfDue(diskCache) + if (result != null && result.wasOverBudget) { Log.i("AppModules") { "Image cache was over budget: wiped ${result.reclaimedFiles} files (${result.bytesOnDisk} bytes on disk, ${result.budgetBytes} budget)" } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt index 4c4ad37bab..84c19da59c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt @@ -55,9 +55,15 @@ data class ImageCacheReconciliation( * empty the journal, then unlink whatever is still sitting in the directory. * * This is deliberately blunt. It costs the whole cache, so it only fires once the directory has - * drifted past [ceilingBytes] — well beyond the slack normal operation needs — and on a healthy - * cache it is one directory walk and nothing else. It does not read the journal, so it stays - * independent of Coil's on-disk format. + * drifted past [ceilingBytes] — well beyond the slack normal operation needs. It does not read the + * journal, so it stays independent of Coil's on-disk format. + * + * Drift accrues only when a process dies with unlinks still queued, which is slow, so + * [reconcileIfDue] rate-limits the check to [DEFAULT_INTERVAL_MS]. Even the healthy pass is a + * `readdir` plus a `stat` per file — on a full 1 GB cache, tens of thousands of syscalls — and + * `AppModules.initiate()` runs on every process start, including the WorkManager wake-ups that + * cold-start the whole graph. Gating it on a marker file's mtime costs one `stat` on the starts + * that skip. */ object ImageDiskCacheReconciler { /** @@ -72,9 +78,22 @@ object ImageDiskCacheReconciler { */ private const val DEFAULT_MIN_SLACK_BYTES = 4L * 1024 * 1024 + /** How long one pass is good for. Drift accrues over days, so checking daily is ample. */ + private const val DEFAULT_INTERVAL_MS = 24L * 60 * 60 * 1000 + + /** + * Empty file whose mtime is the last pass. It lives inside the cache directory so it travels + * with the thing it describes: clearing the app's cache from Settings takes the marker with it, + * and the next start reconciles a directory whose history we no longer know. + */ + private const val MARKER_FILE = ".reconciled" + /** Coil's own bookkeeping, which is not entry data and must survive a wipe. */ private val JOURNAL_FILES = setOf("journal", "journal.tmp", "journal.bkp") + /** Files in the cache directory that are not entry data and must survive a wipe. */ + private val PRESERVED_FILES = JOURNAL_FILES + MARKER_FILE + /** Bytes the directory may hold before [reconcile] wipes it. */ fun ceilingBytes( budgetBytes: Long, @@ -83,14 +102,66 @@ object ImageDiskCacheReconciler { ): Long = budgetBytes + maxOf((budgetBytes * slackFraction).toLong(), minSlackBytes) /** - * Walks the cache directory and, if it holds more than [ceilingBytes], empties it. + * Runs [reconcile] if the last pass is older than [intervalMs], else returns null having done + * one `stat`. * * Blocking IO — call it from a background dispatcher. + */ + fun reconcileIfDue( + diskCache: DiskCache, + now: Long = System.currentTimeMillis(), + intervalMs: Long = DEFAULT_INTERVAL_MS, + slackFraction: Double = DEFAULT_SLACK_FRACTION, + minSlackBytes: Long = DEFAULT_MIN_SLACK_BYTES, + ): ImageCacheReconciliation? { + if (!isDue(diskCache, now, intervalMs)) return null + + return reconcile(diskCache, slackFraction, minSlackBytes).also { markPass(diskCache) } + } + + /** + * True when no pass is recorded, or the recorded one is [intervalMs] old. + * + * A marker dated in the future — a clock that jumped back, or a restored backup — would + * otherwise park the check until real time caught up, so that also counts as due. + */ + private fun isDue( + diskCache: DiskCache, + now: Long, + intervalMs: Long, + ): Boolean { + val lastPass = + try { + diskCache.fileSystem.metadataOrNull(diskCache.directory / MARKER_FILE)?.lastModifiedAtMillis + } catch (e: IOException) { + Log.d("ImageDiskCache") { "could not stat the marker: ${e.message}" } + null + } ?: return true + + return now - lastPass >= intervalMs || lastPass > now + } + + /** Records that a pass just happened, by writing the marker's mtime to now. */ + private fun markPass(diskCache: DiskCache) { + try { + diskCache.fileSystem.createDirectories(diskCache.directory) + diskCache.fileSystem.write(diskCache.directory / MARKER_FILE) {} + } catch (e: IOException) { + // Losing the marker only costs a redundant walk on the next start. + Log.d("ImageDiskCache") { "could not write the marker: ${e.message}" } + } + } + + /** + * Walks the cache directory and, if it holds more than [ceilingBytes], empties it. + * + * Blocking IO — call it from a background dispatcher. Prefer [reconcileIfDue]; this is the + * unconditional pass. * * Deliberately does not read `DiskCache.size`: that would force the journal parse on the happy * path, and the decision does not need it. A request racing the wipe can lose the entry it was * writing; Coil treats that as a cache miss and re-fetches, which is why this runs at startup - * rather than on a timer. + * rather than while the feed is scrolling. */ fun reconcile( diskCache: DiskCache, @@ -118,7 +189,7 @@ object ImageDiskCacheReconciler { var reclaimed = 0 regularFiles(diskCache).forEach { path -> - if (path.name in JOURNAL_FILES) return@forEach + if (path.name in PRESERVED_FILES) return@forEach try { diskCache.fileSystem.delete(path, mustExist = false) reclaimed++ diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt index 3f458371ed..2cf1c79399 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt @@ -33,6 +33,7 @@ import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -234,4 +235,112 @@ class ImageDiskCacheReconcilerTest { // over the journal plus a couple of in-flight writes. assertEquals(1024L * 1024 + 4L * 1024 * 1024, ImageDiskCacheReconciler.ceilingBytes(1024L * 1024)) } + + // ---- cadence ---------------------------------------------------------------------------- + // The healthy pass is a readdir plus a stat per file, and AppModules.initiate() runs on every + // process start — including the WorkManager wake-ups that cold-start the whole graph. Drift + // accrues over process deaths, not startups, so the check is rate-limited. + + private val markerPath get() = cacheDir / ".reconciled" + + @Test + fun aCacheWithNoRecordedPassIsDue() { + val diskCache = newCache(DeferredDeleteFileSystem(FileSystem.SYSTEM, scope), 1024L * 1024) + try { + assertNotNull(ImageDiskCacheReconciler.reconcileIfDue(diskCache)) + assertNotNull("the pass must record itself", FileSystem.SYSTEM.metadataOrNull(markerPath)) + } finally { + diskCache.shutdown() + } + } + + @Test + fun aSecondStartWithinTheIntervalSkipsTheWalk() { + val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + val budget = 16L * 1024 + val diskCache = newCache(fs, budget) + try { + val now = System.currentTimeMillis() + assertNotNull(ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now)) + + // Even a directory well over budget is left alone until the next pass is due — the + // point of the gate is that the common start does no work at all. + repeat(40) { i -> writeEntry(diskCache, "key$i", 1024) } + val overBudget = bytesOnDisk() + + val skipped = ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now + 1000, minSlackBytes = 0) + + assertNull("within the interval the pass must not run", skipped) + assertEquals(overBudget, bytesOnDisk()) + } finally { + diskCache.shutdown() + } + } + + @Test + fun aStartAfterTheIntervalIsDueAgain() { + val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + val diskCache = newCache(fs, 1024L * 1024) + try { + val now = System.currentTimeMillis() + val interval = 24L * 60 * 60 * 1000 + ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now, intervalMs = interval) + + assertNull(ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now + interval - 1, intervalMs = interval)) + assertNotNull(ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now + interval, intervalMs = interval)) + } finally { + diskCache.shutdown() + } + } + + @Test + fun aMarkerDatedInTheFutureDoesNotParkTheCheck() { + // A clock that jumped back, or a restored backup, would otherwise strand the check until + // real time caught up with the marker. + val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + val diskCache = newCache(fs, 1024L * 1024) + try { + val now = System.currentTimeMillis() + ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now) + + assertNotNull(ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now - 365L * 24 * 60 * 60 * 1000)) + } finally { + diskCache.shutdown() + } + } + + @Test + fun theMarkerSurvivesAWipe() { + // The marker is a plain file in the cache directory, so the sweep would take it unless it is + // preserved explicitly — and a wipe that erases its own record makes every later start look + // due and walk again. Drives reconcile() rather than reconcileIfDue(), because the latter + // rewrites the marker afterwards and would mask the deletion. + val deadProcessFs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + scope.cancel("inert drainer") + + val budget = 16L * 1024 + val first = newCache(deadProcessFs, budget) + repeat(40) { i -> writeEntry(first, "key$i", 1024) } + val deadline = System.currentTimeMillis() + 5_000 + while (first.size > budget && System.currentTimeMillis() < deadline) Thread.sleep(20) + first.shutdown() + + val liveScope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val liveFs = DeferredDeleteFileSystem(FileSystem.SYSTEM, liveScope) + val second = newCache(liveFs, budget) + try { + // Lay down a marker the way a previous pass would have. + FileSystem.SYSTEM.write(markerPath) {} + + val result = ImageDiskCacheReconciler.reconcile(second, minSlackBytes = 0) + assertTrue("expected a wipe", result.wasOverBudget) + + liveFs.drainNow() + + assertNotNull("the marker must outlive the wipe", FileSystem.SYSTEM.metadataOrNull(markerPath)) + } finally { + second.shutdown() + liveScope.cancel() + } + } } From 2d229d859e1c65a88a04295f25de1ee05ac8f423 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 23:59:13 +0000 Subject: [PATCH 5/5] test(images): stop the reconciler's cadence tests racing the clock and eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two flakes in ImageDiskCacheReconcilerTest, both mine, both green locally and red on CI. aStartAfterTheIntervalIsDueAgain measured the interval from the clock it sampled, but isDue() compares against the marker's file system mtime. A file system that keeps mtime at whole-second resolution reads the marker back up to a second before the write that made it, so `now + interval - 1` was already past the interval and the pass ran when the test expected it skipped. Reproduced exactly by truncating the marker's mtime to its whole second: same result object CI reported, ceilingBytes and all. It now pins the recorded pass to a whole second and measures from that, so the boundary holds at any mtime resolution — and asserts the file system kept the value, so an environment that cannot would fail loudly instead of flaking. Renamed to theIntervalIsMeasuredFromTheRecordedPass, which is the property. aSecondStartWithinTheIntervalSkipsTheWalk asserted the directory's byte total was unchanged across the skipped call. Coil evicts asynchronously on its own scope and the drainer unlinks behind it, so the two measurements raced both: CI saw 24103 where the test had recorded 25127. What the test needs to rule out is a wipe, and clear() takes DiskCache.size to zero — so it asserts on that instead, which no amount of eviction churn can move. Verified by running the class ten times, and by running CI's own task list locally (both lintBenchmark variants and both unit-test variants — the pre-push hook covers only testPlayDebugUnitTest, which is how these reached CI). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017RYCgbvhtBCBNVLMxSWoCJ --- .../images/ImageDiskCacheReconcilerTest.kt | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt index 2cf1c79399..65f7812c5c 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt @@ -266,33 +266,54 @@ class ImageDiskCacheReconcilerTest { // Even a directory well over budget is left alone until the next pass is due — the // point of the gate is that the common start does no work at all. repeat(40) { i -> writeEntry(diskCache, "key$i", 1024) } - val overBudget = bytesOnDisk() val skipped = ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now + 1000, minSlackBytes = 0) assertNull("within the interval the pass must not run", skipped) - assertEquals(overBudget, bytesOnDisk()) + // Coil's size, not the directory's bytes: eviction runs asynchronously on its own scope + // and the drainer unlinks behind it, so comparing byte totals across the call races + // both. A wipe is what this needs to rule out, and clear() takes the size to zero. + assertTrue("a skipped pass must not have cleared the cache", diskCache.size > 0) } finally { diskCache.shutdown() } } @Test - fun aStartAfterTheIntervalIsDueAgain() { + fun theIntervalIsMeasuredFromTheRecordedPass() { val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) val diskCache = newCache(fs, 1024L * 1024) try { - val now = System.currentTimeMillis() val interval = 24L * 60 * 60 * 1000 - ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now, intervalMs = interval) + ImageDiskCacheReconciler.reconcileIfDue(diskCache, intervalMs = interval) - assertNull(ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now + interval - 1, intervalMs = interval)) - assertNotNull(ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now + interval, intervalMs = interval)) + // Pin the recorded pass to a whole second before measuring from it. isDue() compares + // against the marker's mtime, and a file system that keeps mtime at whole-second + // resolution reads it back up to a second before the write that made it — so a boundary + // measured from our own clock instead lands a second early there. That is what made the + // first version of this test pass on a dev box and fail on CI. + val lastPass = (System.currentTimeMillis() / 1000) * 1000 + assertTrue("could not set the marker's mtime", File(markerPath.toString()).setLastModified(lastPass)) + assertEquals("the file system must keep the timestamp we set", lastPass, recordedPassAt()) + + assertNull( + "a millisecond before the interval is up, the pass must not run", + ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = lastPass + interval - 1, intervalMs = interval), + ) + assertNotNull( + "one interval after the recorded pass, it is due again", + ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = lastPass + interval, intervalMs = interval), + ) } finally { diskCache.shutdown() } } + private fun recordedPassAt(): Long = + requireNotNull(FileSystem.SYSTEM.metadataOrNull(markerPath)?.lastModifiedAtMillis) { + "a pass must record a timestamp" + } + @Test fun aMarkerDatedInTheFutureDoesNotParkTheCheck() { // A clock that jumped back, or a restored backup, would otherwise strand the check until