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/AppModules.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/AppModules.kt index f9d26eafe5..bf1eeabdc5 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,22 @@ 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. + // + // 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 the check both land on IO rather than on whichever thread loads an image first. + applicationIOScope.launch { + 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)" } + } + } + applicationIOScope.launch { // loads main account quickly. LocalPreferences.loadAccountConfigFromEncryptedStorage() 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..c068e59e69 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/AnimatedImageDecoderFactory.kt @@ -0,0 +1,100 @@ +/* + * 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.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 stops asking for the GIF + * frame-delay rewrite when the file does not need it. + * + * 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 { + 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], 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. + */ +@RequiresApi(28) +fun newAnimatedImageDecoder( + source: ImageSource, + options: Options, + mayNeedFrameDelayRewrite: Boolean, +): 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. + */ +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() } +} 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/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/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..86d271c53f --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDecoderSources.kt @@ -0,0 +1,112 @@ +/* + * 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 coil3.fetch.FetchResult +import coil3.fetch.Fetcher +import coil3.fetch.SourceFetchResult +import com.vitorpamplona.amethyst.commons.service.image.DeferredDeleteFileSystem +import okio.FileSystem + +/** + * 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 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) { + * val file = fileOrNull() + * if (file != null) return ImageDecoder.createSource(file.toFile()) + * } + * ``` + * + * 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: + * + * - `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`. + * + * 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. + */ +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 null + + val file = fileOrNull() ?: return null + + 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/ImageDiskCacheReconciler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt new file mode 100644 index 0000000000..84c19da59c --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconciler.kt @@ -0,0 +1,220 @@ +/* + * 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. 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 { + /** + * 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 + + /** 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, + slackFraction: Double = DEFAULT_SLACK_FRACTION, + minSlackBytes: Long = DEFAULT_MIN_SLACK_BYTES, + ): Long = budgetBytes + maxOf((budgetBytes * slackFraction).toLong(), minSlackBytes) + + /** + * 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 while the feed is scrolling. + */ + 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 PRESERVED_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/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/images/ImageLoaderSetup.kt index 7a4098f5ed..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 @@ -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() } @@ -169,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, @@ -179,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/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/ImageDiskCacheReconcilerTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt new file mode 100644 index 0000000000..65f7812c5c --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/images/ImageDiskCacheReconcilerTest.kt @@ -0,0 +1,367 @@ +/* + * 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.assertNull +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)) + } + + // ---- 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 skipped = ImageDiskCacheReconciler.reconcileIfDue(diskCache, now = now + 1000, minSlackBytes = 0) + + assertNull("within the interval the pass must not run", skipped) + // 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 theIntervalIsMeasuredFromTheRecordedPass() { + val fs = DeferredDeleteFileSystem(FileSystem.SYSTEM, scope) + val diskCache = newCache(fs, 1024L * 1024) + try { + val interval = 24L * 60 * 60 * 1000 + ImageDiskCacheReconciler.reconcileIfDue(diskCache, 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 + // 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() + } + } +} 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()) + } +}