fix(thumbnails): two defects the copy test exposed

1. Per-file overrides must beat the content tier in RAM as well as on
   disk. The content-keyed lookup ran first, so a thumbnail already
   rendered from the file's content sat in RAM under content(blob_hash)
   and shadowed a preview uploaded afterwards — permanently. Invisible
   before the moka rekey, because both lived under one file-id key and
   the upload simply overwrote the render. Order is now uniform:
   per-file RAM, per-file disk (ext-), per-file DB, then content RAM,
   blob-hash disk, derived blob.

2. store_attached_blob never wrote a row. Its RETURNING clause compared
   the stored hash against EXCLUDED, and PostgreSQL only permits
   EXCLUDED in the SET and WHERE of DO UPDATE — a runtime syntax error
   on every call. The superseded hash now comes from a SELECT taken
   before the upsert; losing that race leaves one stale reference, which
   the manifest recompute reports, rather than anything being lost.

The second hid behind the first for a whole cycle, and behind
`ext-{file_id}.jpg`: the ORIGINAL kept serving its uploaded preview from
local disk, so the feature looked healthy. Only a copy, which has a
different file_id and therefore no ext- file, depends on the row — and
the row was never there. The handler's best-effort warn! completed the
disguise, so it is now error!: a failure there means copies silently
lose the preview, and nothing else signals it.
This commit is contained in:
Edouard Vanbelle
2026-08-25 08:13:51 +02:00
parent 6c5e53fee4
commit d71dd973e7
3 changed files with 66 additions and 23 deletions
+26 -8
View File
@@ -607,10 +607,29 @@ impl DedupService {
.await?;
let attached_hash = stored.hash().to_string();
// `previous` is the hash this row pointed at before, when it existed
// and differed — the reference to release once the row no longer
// holds it.
let previous: Option<(Option<String>,)> = sqlx::query_as(
// Read the hash being superseded BEFORE upserting.
//
// It cannot come from `RETURNING`: PostgreSQL only permits `EXCLUDED`
// in the `SET` and `WHERE` of `DO UPDATE`, so a RETURNING clause
// comparing old against new is a syntax error — and one that surfaces
// only at runtime, where this method's best-effort caller swallows it
// into a warning while the sidecar keeps the feature looking healthy.
//
// The gap between this SELECT and the upsert is benign: losing the
// race leaves one stale reference, which the manifest recompute
// reports rather than anything being lost or served wrongly.
let previous: Option<(String,)> = sqlx::query_as(
"SELECT blob_hash FROM storage.file_attached_blobs
WHERE file_id = $1::uuid AND kind = $2 AND variant = $3",
)
.bind(file_id)
.bind(kind)
.bind(variant)
.fetch_optional(self.pool.as_ref())
.await
.map_err(|e| DomainError::internal_error("Dedup", format!("read attached blob: {e}")))?;
sqlx::query(
"INSERT INTO storage.file_attached_blobs
(file_id, kind, variant, blob_hash, content_type, uploaded_by)
VALUES ($1::uuid, $2, $3, $4, $5, $6)
@@ -618,8 +637,7 @@ impl DedupService {
SET blob_hash = EXCLUDED.blob_hash,
content_type = EXCLUDED.content_type,
uploaded_by = EXCLUDED.uploaded_by,
created_at = now()
RETURNING NULLIF(storage.file_attached_blobs.blob_hash, EXCLUDED.blob_hash)",
created_at = now()",
)
.bind(file_id)
.bind(kind)
@@ -627,14 +645,14 @@ impl DedupService {
.bind(&attached_hash)
.bind(content_type)
.bind(uploaded_by)
.fetch_optional(self.pool.as_ref())
.execute(self.pool.as_ref())
.await
.map_err(|e| DomainError::internal_error("Dedup", format!("record attached blob: {e}")))?;
// A replaced row's old blob loses its only reference from here. Not
// releasing it would pin those bytes forever — nothing else points at
// a superseded preview.
if let Some((Some(old_hash),)) = previous
if let Some((old_hash,)) = previous
&& old_hash != attached_hash
&& let Err(e) = self.remove_reference(&old_hash).await
{