From 4baee0a1fb42f795eea8a0e1652554e5bcce704b Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 30 Aug 2026 22:20:47 +0200 Subject: [PATCH] feat(transcode): key the memory cache by content, not by file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The durable tier has been content-keyed since it was introduced — `content_derived_blobs(source_hash, kind, variant)` — but the moka cache in front of it was still `{file_id}:{ext}`, so the layer closest to the request used the wrong axis while the layer behind it used the right one. That was legacy shape, and I had defended it in a comment as "deliberate: per-request-path and short-lived", which was a rationalisation rather than a reason. Ed asked why, and there is no why. Transcoding is a pure function of the source bytes. Under file keying, two files with identical content held two RAM entries for identical bytes, and the second file was a guaranteed miss that fell through to a DB lookup plus a blob read to fetch what was already in memory under another key. Now keyed by content hash when the caller has one, by file id only when it does not — the same `content` / `external` split `ThumbnailCacheKey` already makes, and for the same reason: hash-less callers (external mounts) have no content identity to key on. Prefixed `c:` / `f:` so the namespaces stay disjoint; a hash and a UUID cannot collide in practice, but "in practice" is how a file ends up served another file's bytes. `invalidate` now clears only the file-keyed entry. Dropping content entries there would be wrong, not merely wasteful: one file's content changing says nothing about the other files sharing the old bytes, and evicting theirs would make one user's edit cost everyone else a re-transcode. Content entries need no eviction — new content is a new hash, so the old key is never consulted again. transcode_cache.hurl updated to match, and its header corrected: the second file is now a RAM hit rather than a derived-tier read, so that scenario can no longer isolate the durable tier. It says so, and points at satellites_consistency and a restart as what covers it instead. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/image_transcode_service.rs | 45 ++++++++++++++++--- tests/api/transcode_cache.hurl | 32 +++++++++---- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/infrastructure/services/image_transcode_service.rs b/src/infrastructure/services/image_transcode_service.rs index b361b973..6b2e6a62 100644 --- a/src/infrastructure/services/image_transcode_service.rs +++ b/src/infrastructure/services/image_transcode_service.rs @@ -234,6 +234,31 @@ impl ImageTranscodeService { /// The `content_derived_blobs.kind` for everything this service writes. const DERIVED_KIND: &'static str = "transcode"; + /// Memory-cache key: by CONTENT when the caller supplied a hash, by + /// file id only when it could not. + /// + /// Transcoding is a pure function of the source bytes, so file keying + /// was always the wrong axis for this cache — it just predated the + /// content-keyed tier. Two files with identical content held two + /// entries for identical bytes, and the second file missed RAM and + /// paid a DB lookup plus a blob read to fetch what was already in + /// memory under another key. + /// + /// The `c:` / `f:` prefixes keep the two namespaces disjoint. A + /// 64-hex hash and a UUID cannot collide in practice, but relying on + /// "in practice" for a cache key is how a file ends up served another + /// file's bytes. + /// + /// Same shape as `ThumbnailCacheKey`'s `content` / `external` split, + /// for the same reason: hash-less callers (external mounts) have no + /// content identity to key on, so they keep the per-file entry. + fn cache_key(source_hash: Option<&str>, file_id: &str, format: OutputFormat) -> String { + match source_hash { + Some(hash) => format!("c:{}:{}", hash, format.extension()), + None => format!("f:{}:{}", file_id, format.extension()), + } + } + /// Initialize the service. /// /// Still creates the local cache directories, because this service DOES @@ -329,12 +354,7 @@ impl ImageTranscodeService { original_mime: &str, target_format: OutputFormat, ) -> Result<(Bytes, String, bool), String> { - // Keyed by file_id, not content hash. Deliberate: this cache is - // per-request-path and short-lived (10 min TTL), and the file id is - // what the caller has in hand on every request. The DURABLE tier - // below is content-keyed, which is where dedup across identical - // files actually pays. - let cache_key = format!("{}:{}", file_id, target_format.extension()); + let cache_key = Self::cache_key(source_hash, file_id, target_format); // ── 1. Fast path: moka memory cache (lock-free read) ── // An empty-Bytes entry is the negative sentinel: "transcoding this @@ -650,7 +670,18 @@ impl ImageTranscodeService { /// Invalidate cached transcodes for a file pub async fn invalidate(&self, file_id: &str) { - let cache_key = format!("{}:{}", file_id, OutputFormat::WebP.extension()); + // Only the FILE-keyed entry, deliberately. + // + // Content-keyed entries must not be dropped here: this file's + // content changing says nothing about the other files sharing the + // old bytes, and evicting theirs would make one user's edit cost + // everyone else a re-transcode. They need no eviction anyway — + // new content is a new hash, so the old key is simply never + // consulted again, and moka's TTL reclaims it. + // + // What remains here is the fallback entry for hash-less callers, + // plus the legacy on-disk pair, which are genuinely per-file. + let cache_key = Self::cache_key(None, file_id, OutputFormat::WebP); self.memory_cache.invalidate(&cache_key).await; let cache_path = self.get_cache_path(file_id, OutputFormat::WebP); diff --git a/tests/api/transcode_cache.hurl b/tests/api/transcode_cache.hurl index 887f02c3..466a21f6 100644 --- a/tests/api/transcode_cache.hurl +++ b/tests/api/transcode_cache.hurl @@ -20,13 +20,20 @@ # # ## Why each case uploads the same bytes twice # -# The in-memory cache is keyed `{file_id}:{ext}`, so re-fetching the SAME -# file proves only that moka works. Uploading identical content as a -# SECOND file gives a different file id and therefore a guaranteed memory -# miss — but the same content hash. If the second fetch still avoids a -# transcode, only the content-keyed tier can have answered it. That is -# precisely what the migration bought, and it is unobservable any other -# way. +# Re-fetching the SAME file proves only that a cache exists. Uploading +# identical content as a SECOND file gives a different file id but the +# same content hash, which is the case that distinguishes content keying +# from file keying — and both caches here are now content-keyed. +# +# So the assertion is that the second file costs NO transcode, and is +# served from RAM. Under the previous file-keyed memory cache it was a +# guaranteed RAM miss: two entries for identical bytes, and a DB lookup +# plus a blob read to fetch what was already in memory under another key. +# +# What this can no longer isolate is the DURABLE tier, because the RAM +# cache now answers first for anything within one process lifetime. +# That tier is covered instead by `satellites_consistency` (every row +# points at a live blob) and by a restart, which hurl cannot perform. # # ## Fixtures # @@ -160,6 +167,7 @@ HTTP 200 [Captures] after_positive: jsonpath "$.transcodes" after_positive_disk: jsonpath "$.disk_hits" +after_positive_ram: jsonpath "$.cache_hits" [Asserts] jsonpath "$.not_beneficial" == {{base_not_beneficial}} @@ -210,9 +218,17 @@ Authorization: Bearer {{token}} HTTP 200 [Asserts] +# The property that matters, whichever tier answered: identical content is +# transcoded ONCE, however many files carry it. jsonpath "$.transcodes" == {{after_positive}} jsonpath "$.not_beneficial" == {{base_not_beneficial}} -jsonpath "$.disk_hits" != {{after_positive_disk}} +# Served from RAM, and that is the point of the memory cache being keyed by +# CONTENT rather than by file id. Under file keying this second file was a +# guaranteed RAM miss that fell through to a DB lookup plus a blob read to +# fetch bytes already in memory under another key — `disk_hits` moved and +# `cache_hits` did not. Now it is the other way round. +jsonpath "$.cache_hits" != {{after_positive_ram}} +jsonpath "$.disk_hits" == {{after_positive_disk}} # ═════════════════════════════════════════════════════════════