diff --git a/docs/architecture/caching.md b/docs/architecture/caching.md index 7e2ba448..becfb8f6 100644 --- a/docs/architecture/caching.md +++ b/docs/architecture/caching.md @@ -16,8 +16,33 @@ The two layers are orthogonal — the moka caches shave query round-trips regard | Thumbnail cache | configurable | 1 000 | Generated WebP/AVIF thumbnails | | Image transcode | configurable | 500 | On-the-fly image transcoding results | | Blob hash | 30 s TTI | 5 000 | BLAKE3 hashes for dedup lookups | +| Attached blob | 60 s TTL | 50 000 | `file_attached_blobs` row lookups on the thumbnail hot path (ETag + tier-2b, also the Nextcloud preview endpoint) | | Audio metadata | — | 2 000 | ID3 tags and duration | +### The attached-blob cache + +Every thumbnail request pays a `storage.file_attached_blobs` point query +before it can even answer "304 Not Modified" — the ETag names the attached +blob's hash. A photos grid revalidating 60 thumbnails per visit means +60+ point queries per browse. The cache sits in `DedupService` in front of +that lookup (`find_attached_blob`), keyed by the row's `(file_id, kind, +variant)` primary key, and caches **both directions**: `Some(row)` and +`None` (most files have no attached preview, so the negative side is where +most of the win is). + +Two rules keep it honest: + +- **DB faults are never cached.** The uncached lookup surfaces errors as + `Err`; only a genuine `Ok(None)` fills a negative entry. A transient + outage must not freeze "no attached blob" into place for a full TTL — + a read failure is never proof that data is absent. +- **TTL is the bound, not the invalidation strategy.** Writes invalidate + eagerly — `store_attached_blob` / `store_attached_blob_if_absent` on + success, deletions via `ThumbnailRefreshHook::on_file_deleted` (which + all three production delete paths fire). The 60 s TTL only bounds what + the process cannot see: bare SQL, the `copy_file_satellites` race + window, a hypothetical second instance. + ### How it works 1. **Read path:** check cache → if hit, return immediately (sub-ms); if miss, query PostgreSQL, populate cache, return diff --git a/docs/architecture/derived-and-attached-blobs.md b/docs/architecture/derived-and-attached-blobs.md index 19825431..4fcbdb2b 100644 --- a/docs/architecture/derived-and-attached-blobs.md +++ b/docs/architecture/derived-and-attached-blobs.md @@ -248,6 +248,19 @@ on `DELETE`. The trigger fires on DELETE only; replacing a preview updates `blob_hash` in place and the Rust path handles that reference swap. +**Reads are cached; the cache never outlives the truth by design.** +`DedupService::find_attached_blob` — the lookup the thumbnail ETag path +pays on *every* request, 304 or not — reads through an in-process moka +cache keyed by the row's PK, positive and negative entries alike. Two +properties make that safe rather than merely fast: a DB fault is +surfaced as an error and never fills a negative entry (a failed lookup +is not a missing row), and every write path that can change an answer +invalidates first — the two `store_attached_blob*` variants on success, +deletes via `ThumbnailRefreshHook::on_file_deleted` after the CASCADE +committed. The 60 s TTL exists for the residual cases the process +cannot observe (bare SQL, `copy_file_satellites` racing a concurrent +new file), not as the primary coherence mechanism. + **Writing a derived row requires its source to exist.** `store_derived_blob` guards the insert with an `EXISTS` on `chunk_manifests`/`blobs`. Without it, a row written just after its diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index 589a1c80..57ec1146 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -58,7 +58,7 @@ use crate::application::ports::blob_lifecycle::BlobLifecycleHook; use crate::application::ports::blob_reference_ports::{BlobReferenceRegistry, RefLevel}; use crate::application::ports::blob_storage_ports::BlobStorageBackend; use crate::application::ports::dedup_ports::{ - BlobMetadataDto, DedupPort, DedupResultDto, DedupStatsDto, + BlobMetadataDto, DedupPort, DedupResultDto, DedupStatsDto, DerivedBlobRef, }; use crate::application::services::blob_lifecycle_service::BlobLifecycleService; use crate::domain::errors::{DomainError, ErrorKind}; @@ -646,6 +646,49 @@ fn manifest_reap_sql(registry: &BlobReferenceRegistry) -> String { ) } +// ── Attached-blob lookup cache ─────────────────────────────────────────────── + +/// Cache size cap for [`DedupService::attached_blob_cache`] — plain entry +/// count (no weigher): an entry is three short strings + two short strings, +/// tens of bytes; 50k entries ≈ a few MB, noise next to the manifest cache. +pub(crate) const ATTACHED_BLOB_CACHE_MAX_ENTRIES: u64 = 50_000; +/// Hard staleness bound for [`DedupService::attached_blob_cache`]. +/// +/// Deliberately [`moka::future::Cache::builder().time_to_live`] and NOT +/// `time_to_idle`: a hot negative entry under TTI never expires, and TTL must +/// be the last-resort bound for writes this process never saw (bare SQL, a +/// future second instance, the `copy_file_satellites` race window). +pub(crate) const ATTACHED_BLOB_CACHE_TTL_SECS: u64 = 60; + +/// Cache key for [`DedupService::attached_blob_cache`] — the +/// `storage.file_attached_blobs` primary key. A struct, not a +/// `(String, String, String)` tuple: three same-typed fields read by position +/// would force every construction site (and the `invalidate_for_file` scan) +/// to guess semantics; self-documenting beats positional here. +#[derive(Clone, PartialEq, Eq, Hash, Debug)] +struct AttachedBlobKey { + file_id: String, + kind: String, + variant: String, +} + +impl AttachedBlobKey { + fn new(file_id: &str, kind: &str, variant: &str) -> Self { + Self { + file_id: file_id.to_string(), + kind: kind.to_string(), + variant: variant.to_string(), + } + } +} + +/// Loader-error sentinel for the `try_get_with` cache wrapper on +/// [`Self::find_attached_blob`]. The SQL lookup treats a DB fault the same as +/// "no row" only at the very last moment — the cache must never see it, or a +/// transient outage would freeze "no attached blob" into place for a full +/// TTL while rows exist (a read failure is never proof that data is absent). +struct AttachedLookupFault; + pub struct DedupService { /// Pluggable blob storage backend (local FS, S3, …). backend: Arc, @@ -664,6 +707,28 @@ pub struct DedupService { /// seen immediately), weight-bounded (a manifest is ~72 B per chunk), /// short TTL so GC'd manifests age out fast (benches/MANIFEST-CACHE.md). manifest_cache: moka::future::Cache>, + /// `file_id → attached blob` lookup cache (`storage.file_attached_blobs` + /// rows) for the thumbnail hot path — `ThumbnailService:: + /// thumbnail_content_id` hits it on EVERY request (including 304 + /// revalidations and RAM thumbnail hits, `thumbnail_service.rs` ~:744) + /// and `get_cached_thumbnail` tier 2b hits it again with the same key + /// (~:851); the Nextcloud preview endpoint rides the same lookup. + /// + /// Positive AND negative (`Option` — most files have no + /// attached preview row, so the negative side is where the win is). The + /// loader NEVER caches a DB error: `find_attached_blob_uncached` returns + /// `Err` and `try_get_with` drops it, so a transient outage cannot freeze + /// "no attached blob" into the cache for a full TTL (a read failure is + /// never proof that data is absent). + /// + /// Writes invalidate through the same type: `store_attached_blob` / + /// `store_attached_blob_if_absent` on success, file deletions via the + /// `ThumbnailRefreshHook::on_file_deleted` piggyback. The TTL above + /// remains the bound for anything this process cannot see (bare SQL, + /// `copy_file_satellites` races); invalidate-vs-inflight-REFILL races are + /// narrowed by `try_get_with` but not eliminated, and the residual window + /// is ≤ one TTL. + attached_blob_cache: moka::future::Cache>, /// Every table that holds blob references, so GC agrees with the /// consistency jobs on what "referenced" means. Defaults to the two /// built-in sources; DI replaces it once more tables exist. Never @@ -698,6 +763,7 @@ impl DedupService { maintenance_pool, blob_lifecycle: None, manifest_cache: Self::build_manifest_cache(), + attached_blob_cache: Self::build_attached_blob_cache(), reference_registry: registry.clone(), manifest_reap_sql: manifest_reap_sql(®istry), blob_reap_sql: blob_reap_sql(®istry), @@ -729,6 +795,19 @@ impl DedupService { .build() } + /// See the `attached_blob_cache` field docs. Plain entry-count cap (no + /// weigher — an entry is a handful of short strings), TTL as the hard + /// staleness bound; same hard-coded-const treatment as the manifest + /// cache rather than config: an internal accelerator with strict + /// write-side invalidation, where a misconfiguration costs performance, + /// never correctness. + fn build_attached_blob_cache() -> moka::future::Cache> { + moka::future::Cache::builder() + .max_capacity(ATTACHED_BLOB_CACHE_MAX_ENTRIES) + .time_to_live(std::time::Duration::from_secs(ATTACHED_BLOB_CACHE_TTL_SECS)) + .build() + } + /// Registers the blob-reference registry used by the manifest reap /// predicate. Without it `garbage_collect` skips manifest collection /// entirely — see `docs/plan/derived-blobs.md`. @@ -831,6 +910,14 @@ impl DedupService { .await .map_err(|e| DomainError::internal_error("Dedup", format!("record attached blob: {e}")))?; + // The row is replaced — drop any cached (possibly negative) entry so + // the next lookup refills from the new truth. Only on the success + // path: if the execute had failed, the row is unchanged and the + // cache is still accurate, so invalidating would just cost a refill. + self.attached_blob_cache + .invalidate(&AttachedBlobKey::new(file_id, kind, variant)) + .await; + // Two shapes to balance depending on whether the UPSERT was a // real content replacement or a same-content re-store: // @@ -960,12 +1047,27 @@ impl DedupService { // if the row gets updated in it, the sidecar delete // path fails its verify and keeps the sidecar — the // conservative fallback. + // + // This readback now flows through the `attached_blob_cache`. + // Safe in-process: any write this process made already + // invalidated the key. The only degraded case is a negative + // entry cached before some OTHER process inserted the row — + // nonexistent in a single-instance deployment, and even then + // the consequence is `existing_hash: ""` → the import keeps + // its sidecar, the documented conservative fallback. let existing = self.find_attached_blob(file_id, kind, variant).await; return Ok(AttachedBlobInsertOutcome::AlreadyPresent { existing_hash: existing.map(|r| r.blob_hash).unwrap_or_default(), }); } + // We wrote a row for a key the cache may hold a negative entry for + // (the common "import backfill" case) — drop it so the new row is + // immediately visible to the thumbnail path. + self.attached_blob_cache + .invalidate(&AttachedBlobKey::new(file_id, kind, variant)) + .await; + Ok(AttachedBlobInsertOutcome::Inserted { hash: attached_hash, }) @@ -973,12 +1075,51 @@ impl DedupService { /// Look up bytes attached to a file. File-keyed counterpart of /// [`Self::find_derived_blob`]. + /// + /// Cached read-through of [`Self::attached_blob_cache`] (positive AND + /// negative); see the field docs for why. The public signature is + /// unchanged — including the historical "DB fault reads as no row" + /// behaviour — but the fault now dies BEFORE the cache instead of being + /// indistinguishable from an absent row. pub async fn find_attached_blob( &self, file_id: &str, kind: &str, variant: &str, - ) -> Option { + ) -> Option { + match self + .attached_blob_cache + .try_get_with(AttachedBlobKey::new(file_id, kind, variant), async { + self.find_attached_blob_uncached(file_id, kind, variant) + .await + .map_err(|_| AttachedLookupFault) // Err ⇒ never cached + }) + .await + { + Ok(attached) => attached, + Err(_) => { + tracing::debug!( + target: "oxicloud::dedup", + "attached-blob lookup failed (not cached): file={} kind={} variant={}", + file_id, + kind, + variant + ); + None + } + } + } + + /// The uncached lookup — one indexed point query on the + /// `file_attached_blobs` primary key. Unlike the historical inlined + /// body, a DB fault surfaces as `Err` so the cache wrapper can refuse to + /// store it; only a genuine `Ok(None)` means "no row". + async fn find_attached_blob_uncached( + &self, + file_id: &str, + kind: &str, + variant: &str, + ) -> sqlx::Result> { sqlx::query_as::<_, (String, String)>( "SELECT blob_hash, content_type FROM storage.file_attached_blobs WHERE file_id = $1::uuid AND kind = $2 AND variant = $3", @@ -988,16 +1129,31 @@ impl DedupService { .bind(variant) .fetch_optional(self.pool.as_ref()) .await - .ok() - .flatten() - .map(|(blob_hash, content_type)| { - crate::application::ports::dedup_ports::DerivedBlobRef { + .map(|row| { + row.map(|(blob_hash, content_type)| DerivedBlobRef { blob_hash, content_type, - } + }) }) } + /// Invalidate every `(kind, variant)` entry cached for one file. + /// + /// Fired from `ThumbnailRefreshHook::on_file_deleted` so all three + /// production delete paths (single file, folder cascade, trash clear) + /// drop their cached rows after the DELETE commits. A linear scan over + /// the keys is fine here: deletions are rare and the cache is capped at + /// [`ATTACHED_BLOB_CACHE_MAX_ENTRIES`]. + pub async fn invalidate_attached_blobs_for_file(&self, file_id: &str) { + // moka's `Iter` yields `(Arc, V)` synchronously — the await lives + // in `invalidate`, not in the scan itself. + for (key, _) in self.attached_blob_cache.iter() { + if key.file_id == file_id { + self.attached_blob_cache.invalidate(&*key).await; + } + } + } + pub async fn store_derived_blob( &self, source_hash: &str, @@ -1307,6 +1463,7 @@ impl DedupService { maintenance_pool: stub_pool.clone(), blob_lifecycle: None, manifest_cache: Self::build_manifest_cache(), + attached_blob_cache: Self::build_attached_blob_cache(), reference_registry: stub_registry.clone(), manifest_reap_sql: manifest_reap_sql(&stub_registry), blob_reap_sql: blob_reap_sql(&stub_registry), @@ -4184,6 +4341,138 @@ impl crate::infrastructure::scheduler::JobHandler for DedupService { mod tests { use super::*; + // ── attached_blob_cache — find_attached_blob read-through ─────────────── + // + // Pure in-memory contract tests: `new_stub()` connects lazily to an + // unreachable pool, so anything that reaches the "DB" fails loudly. That + // is exactly what makes these work — a served `Some` proves the cache was + // consulted, and a missing entry after a fault proves the fault was not + // cached. Same no-SQL style as the hash_cache tests in + // `file_blob_read_repository.rs`. + + fn attached_key(file_id: &str, kind: &str, variant: &str) -> AttachedBlobKey { + AttachedBlobKey::new(file_id, kind, variant) + } + + fn sample_ref(hash: &str) -> DerivedBlobRef { + DerivedBlobRef { + blob_hash: hash.to_string(), + content_type: "image/jpeg".to_string(), + } + } + + /// A seeded entry is served without touching the (unreachable) stub pool + /// — returning `Some` at all proves the read-through hit the cache. + #[tokio::test] + async fn attached_lookup_serves_a_seeded_entry() { + let svc = DedupService::new_stub(); + let k = attached_key("0189d1b3-8f2a-7cde-b1ad-000000000001", "preview", "icon"); + svc.attached_blob_cache + .insert(k.clone(), Some(sample_ref("abc"))) + .await; + assert_eq!( + svc.find_attached_blob(&k.file_id, "preview", "icon").await, + Some(sample_ref("abc")) + ); + } + + /// Negative entries are where most of the win is (most files have no + /// attached preview). A cached `None` must be served as `None` AND + /// survive the call — not be evicted by the miss path. + #[tokio::test] + async fn attached_lookup_serves_and_keeps_a_negative_entry() { + let svc = DedupService::new_stub(); + let k = attached_key("0189d1b3-8f2a-7cde-b1ad-000000000002", "preview", "icon"); + svc.attached_blob_cache.insert(k.clone(), None).await; + assert_eq!( + svc.find_attached_blob(&k.file_id, "preview", "icon").await, + None + ); + assert!( + svc.attached_blob_cache.get(&k).await.is_some(), + "negative entry was dropped by the lookup" + ); + } + + /// THE contract this change exists for: a DB fault must not be cached. + /// The stub pool cannot connect, so the uncached lookup errors; the + /// wrapper returns `None` (historical behaviour) and leaves the cache + /// empty — a row that appears after a transient outage must be visible + /// on the very next call, not hidden behind a frozen negative entry. + #[tokio::test] + async fn attached_lookup_does_not_cache_a_db_fault() { + let svc = DedupService::new_stub(); + let k = attached_key("0189d1b3-8f2a-7cde-b1ad-000000000003", "preview", "icon"); + assert_eq!( + svc.find_attached_blob(&k.file_id, "preview", "icon").await, + None + ); + assert!( + svc.attached_blob_cache.get(&k).await.is_none(), + "DB fault was cached as a negative entry" + ); + } + + /// Per-file invalidation drops every `(kind, variant)` of that file and + /// leaves other files' entries alone. + #[tokio::test] + async fn invalidate_attached_blobs_for_file_is_scoped_to_the_file() { + let svc = DedupService::new_stub(); + let k1 = attached_key("0189d1b3-8f2a-7cde-b1ad-000000000004", "preview", "icon"); + let k2 = attached_key("0189d1b3-8f2a-7cde-b1ad-000000000004", "preview", "large"); + let k3 = attached_key("0189d1b3-8f2a-7cde-b1ad-000000000005", "preview", "icon"); + for (k, v) in [ + (k1.clone(), Some(sample_ref("a"))), + (k2.clone(), None), + (k3.clone(), Some(sample_ref("c"))), + ] { + svc.attached_blob_cache.insert(k, v).await; + } + + svc.invalidate_attached_blobs_for_file(&k1.file_id).await; + + assert!(svc.attached_blob_cache.get(&k1).await.is_none()); + assert!(svc.attached_blob_cache.get(&k2).await.is_none()); + assert!( + svc.attached_blob_cache.get(&k3).await.is_some(), + "another file's entry must survive" + ); + } + + /// Invalidation happens only after a SUCCESSFUL write: the store path + /// fails (unreachable pool) before any row is touched, so the previously + /// cached entry must still be there. Invalidating on failure would be + /// harmless but pointless — the row is unchanged and the cache accurate. + #[tokio::test] + async fn failed_attached_store_leaves_the_cache_alone() { + let svc = DedupService::new_stub(); + let k = attached_key("0189d1b3-8f2a-7cde-b1ad-000000000006", "preview", "icon"); + svc.attached_blob_cache + .insert(k.clone(), Some(sample_ref("xyz"))) + .await; + + let result = svc + .store_attached_blob( + &k.file_id, + "preview", + "icon", + "image/png", + Bytes::from_static(b"nope"), + uuid::Uuid::nil(), + ) + .await; + assert!( + result.is_err(), + "stub pool is unreachable — store must fail" + ); + + assert_eq!( + svc.attached_blob_cache.get(&k).await, + Some(Some(sample_ref("xyz"))), + "failed store must not disturb the cache" + ); + } + /// Golden test for the statement `garbage_collect` runs against production /// data. It is assembled from the registered reference sources rather than /// written as a literal, so this pins the whole thing byte-for-byte — the diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 4142d2e1..d2918c5f 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -1874,10 +1874,16 @@ impl crate::application::ports::file_lifecycle::FileLifecycleHook for ThumbnailR fn on_file_deleted(&self, file_id: &str) { let thumbnail = self.thumbnail.clone(); let file_id = file_id.to_string(); + // The row is gone (CASCADE cleared file_attached_blobs) — drop any + // cached attached-blob lookup for this file too. TTL would bound the + // staleness anyway, but deletes are rare and the cache lookup after a + // delete is pure waste. + let dedup = self.dedup.clone(); tokio::spawn(async move { if let Err(e) = thumbnail.delete_thumbnails(&file_id).await { tracing::warn!("Failed to delete thumbnails for file {}: {}", file_id, e); } + dedup.invalidate_attached_blobs_for_file(&file_id).await; }); } }