From d8be45b8fcc551aa611fcca208d0d4fce39587bb Mon Sep 17 00:00:00 2001 From: M57 <41645758+Hy4ri@users.noreply.github.com> Date: Sat, 15 Aug 2026 23:05:10 +0300 Subject: [PATCH] fix: reuse cached gateway downloads within a 10-minute TTL (#915) Every tap on a gateway attachment re-downloaded the whole file: the cache write used a random UUID prefix, so no two fetches ever produced the same filename and old copies were never looked up. Cache files are now named deterministically from the gateway path (UUID3 hash + filename), so repeat opens within the TTL (10 min) reuse the existing download and skip the network entirely. Stale entries fall through to a fresh fetch that overwrites the cache via temp-file + rename, so a failed re-download never destroys a previously-good copy. Content types are remembered in-memory per path so cache hits still hand the viewer the right MIME. Also adds isFreshCacheFile()/cacheFileNameFor() unit tests. Co-authored-by: M57 --- .../data/remote/GatewayFileClient.kt | 69 +++++++++++++++---- .../data/remote/GatewayFileClientTest.kt | 26 +++++++ 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/com/m57/hermescontrol/data/remote/GatewayFileClient.kt b/app/src/main/java/com/m57/hermescontrol/data/remote/GatewayFileClient.kt index 4fcce03f..ed7d7245 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/remote/GatewayFileClient.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/remote/GatewayFileClient.kt @@ -10,6 +10,7 @@ import java.net.URLDecoder import java.net.URLEncoder import java.nio.charset.StandardCharsets import java.util.UUID +import java.util.concurrent.ConcurrentHashMap import java.util.regex.Pattern import kotlin.coroutines.coroutineContext @@ -34,6 +35,14 @@ import kotlin.coroutines.coroutineContext object GatewayFileClient { private const val DOWNLOAD_PATH = "/api/files/download" + /** Downloads are reused for this long before a fresh fetch is made. */ + private const val CACHE_REUSE_TTL_MS = 10 * 60 * 1000L + + /** Path → content-type map from the last download of that path, so cache + * hits can still hand the viewer the right type (the header is only known + * when the file is actually fetched). */ + private val mimeByPath = ConcurrentHashMap() + /** * Pure builder for the authenticated download URL. * @@ -122,6 +131,20 @@ object GatewayFileClient { val url = buildDownloadUrl(baseUrl, token, path) ?: return GatewayFileResult.Failure(IllegalArgumentException("not an absolute gateway path: $path")) + // Cache fast path: a recent download of this path is reused as-is, so + // repeat opens skip the network entirely. Stale entries fall through + // to a fresh fetch, which overwrites the cache file. + val cacheName = cacheFileNameFor(path) + val fresh = File(cacheDir, cacheName).takeIf { isFreshCacheFile(it) } + if (fresh != null) { + return GatewayFileResult.Success( + GatewayFile( + name = fileNameFromPath(path), + mimeType = mimeByPath[path] ?: "application/octet-stream", + cacheFile = fresh, + ), + ) + } val call = OkHttpProvider.base.newCall(Request.Builder().url(url).build()) val resp = call.execute() return try { @@ -134,8 +157,9 @@ object GatewayFileClient { ?: fileNameFromPath(path) val mime = resp.header("Content-Type") ?: "application/octet-stream" val cacheFile = - streamBodyToCache(resp, cacheDir, name) + streamBodyToCache(resp, cacheDir, cacheName) ?: return GatewayFileResult.Failure(IOException("failed to write $name to cache")) + mimeByPath[path] = mime GatewayFileResult.Success(GatewayFile(name, mime, cacheFile)) } catch (e: CancellationException) { // Never swallow cancellation — the caller's job was cancelled. @@ -147,33 +171,54 @@ object GatewayFileClient { } } - /** Stream the response body into a fresh file under [cacheDir] in 64 KiB - * chunks (peak heap stays ~constant no matter the file size), sweeping - * cache files older than a day first. Returns the written file, or null - * on any I/O failure. Cancellation-safe: the copy loop checks the calling - * coroutine's job, and a cancelled fetch deletes the partial file. */ + /** Deterministic cache filename for a gateway path: the same path always + * maps to the same file, so repeat opens reuse the download instead of + * re-fetching (a random name would orphan every previous copy). */ + internal fun cacheFileNameFor(path: String): String { + val key = UUID.nameUUIDFromBytes(path.toByteArray(StandardCharsets.UTF_8)).toString().take(12) + val safeName = fileNameFromPath(path).replace(Regex("[/\\\\]"), "_").ifBlank { "file" } + return "$key-$safeName" + } + + /** True when [file] is a regular file young enough to reuse (TTL). */ + internal fun isFreshCacheFile( + file: File, + now: Long = System.currentTimeMillis(), + ): Boolean = file.isFile && now - file.lastModified() < CACHE_REUSE_TTL_MS + + /** Stream the response body into the deterministic [cacheName] file under + * [cacheDir] in 64 KiB chunks (peak heap stays ~constant no matter the + * file size), sweeping cache files older than a day first. The body is + * written to a temp file and renamed into place only on success, so a + * failed re-download never destroys a previously-good cache entry. + * Cancellation-safe: the copy loop checks the calling coroutine's job, + * and a cancelled fetch deletes the partial temp file. */ private suspend fun streamBodyToCache( resp: okhttp3.Response, cacheDir: File, - name: String, + cacheName: String, ): File? { val body = resp.body ?: return null - val safeName = name.replace(Regex("[/\\\\]"), "_").ifBlank { "file" } val dir = cacheDir.also { it.mkdirs() } sweepOldCacheFiles(dir) - val target = File(dir, "${UUID.randomUUID().toString().take(8)}-$safeName") + val target = File(dir, cacheName) + val tmp = File(dir, ".$cacheName.tmp-${UUID.randomUUID().toString().take(8)}") return try { body.byteStream().use { input -> - target.outputStream().use { output -> + tmp.outputStream().use { output -> copyChunked(input, output) } } + if (!tmp.renameTo(target)) { + tmp.delete() + return null + } target } catch (e: CancellationException) { - target.delete() + tmp.delete() throw e } catch (e: Throwable) { - target.delete() + tmp.delete() null } } diff --git a/app/src/test/java/com/m57/hermescontrol/data/remote/GatewayFileClientTest.kt b/app/src/test/java/com/m57/hermescontrol/data/remote/GatewayFileClientTest.kt index b9e78815..d4318412 100644 --- a/app/src/test/java/com/m57/hermescontrol/data/remote/GatewayFileClientTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/data/remote/GatewayFileClientTest.kt @@ -13,6 +13,7 @@ import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before @@ -108,6 +109,31 @@ class GatewayFileClientTest { assertEquals("a b.png", GatewayFileClient.parseFilename("inline; filename*=UTF-8''a%20b.png")) } + @Test + fun `cacheFileNameFor is deterministic per path`() { + val first = GatewayFileClient.cacheFileNameFor("/tmp/report.pdf") + val second = GatewayFileClient.cacheFileNameFor("/tmp/report.pdf") + val other = GatewayFileClient.cacheFileNameFor("/tmp/other.pdf") + assertEquals(first, second) + assertNotEquals(first, other) + assertTrue(first.endsWith("-report.pdf")) + } + + @Test + fun `isFreshCacheFile respects the TTL`() { + val dir = java.io.File(System.getProperty("java.io.tmpdir"), "gwcache_${System.nanoTime()}").apply { mkdirs() } + try { + val fresh = java.io.File(dir, "fresh").apply { writeText("x") } + fresh.setLastModified(System.currentTimeMillis() - 60_000L) // 1 min old + val stale = java.io.File(dir, "stale").apply { writeText("x") } + stale.setLastModified(System.currentTimeMillis() - 60 * 60 * 1000L) // 1 h old + assertTrue(GatewayFileClient.isFreshCacheFile(fresh)) + assertFalse(GatewayFileClient.isFreshCacheFile(stale)) + } finally { + dir.deleteRecursively() + } + } + @Test fun `copyChunked copies input to output byte-for-byte`() = runTest {