From c778b670065cc2da50632f0e916698ce32b75141 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Thu, 27 Aug 2026 23:53:50 +0200 Subject: [PATCH] feat(thumbnails): stop writing sidecars (step 10d2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write paths now persist only to the blob tiers. Until this, dual-write meant any render or upload recreated .thumbnails/ seconds after the import job removed it, so step 10e's gate — "the directory no longer exists" — could never hold. Rendered thumbnails: the fs::write in persist_rendered is gone. Safe because the read flip landed first, so nothing depended on that write to be found, and a failed derived store now costs a re-render rather than data — regenerable by definition. Existing sidecars are untouched and stay readable through the fallback until the import drains them. Uploaded previews needed a change first, and the order was not optional. upload_thumbnail_impl logged and still returned 201 when store_attached_blob failed — safe only while ext-{file_id}.jpg was a second copy. These bytes have NO server-side render path, so removing the sidecar while the store stayed best-effort would lose a user's upload behind a success response. The PUT is now fatal, and drops the RAM entry too, or the cache would keep serving a preview that was never persisted and vanishes on eviction, contradicting the error the client just received. Only then does the ext- write go. thumb_import_check.sh had to change with it: its premise was "upload, then delete the row, and what remains on disk is legacy state", which no longer holds now that nothing writes sidecars. It lays them down itself, with the bytes the API just served, at the exact paths the pre-10d2 code used. The reconstruction stays faithful — same bytes, same paths — it just no longer depends on current code to produce a shape current code has stopped producing. Both sidecars get identical bytes, which is realistic rather than a shortcut: they dedup to one blob while keeping separate mappings, which is the property the keying split exists to preserve. --- .../services/thumbnail_service.rs | 50 +++++++++---------- src/interfaces/api/handlers/file_handler.rs | 20 ++++++-- tests/api/thumb_import_check.sh | 26 ++++++++++ 3 files changed, 66 insertions(+), 30 deletions(-) diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index f852f761..72b45e97 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -309,18 +309,18 @@ impl ThumbnailService { bytes: &Bytes, dedup: Option<&DedupService>, ) { - let thumb_path = self.get_thumbnail_path(blob_hash, size, format); - if let Some(parent) = thumb_path.parent() { - let _ = fs::create_dir_all(parent).await; - } - if let Err(e) = fs::write(&thumb_path, bytes).await { - tracing::warn!( - "Failed to save thumbnail sidecar {} {:?}: {e}", - &blob_hash[..blob_hash.len().min(12)], - size - ); - } - + // Step 10d2: the sidecar write is GONE. The derived tier is the only + // durable home for a rendered thumbnail now. + // + // Safe because the read flip landed first: reads already prefer the + // derived tier, so nothing depended on this write to be found. And a + // failure below costs a re-render rather than data — a rendered + // thumbnail is regenerable by definition, which is exactly why this + // side could stop before the uploaded one. + // + // Existing sidecars are untouched. They stay readable through the + // fallback tier until the import drains them, so a box that has not + // run the job yet loses nothing. if let Some(dedup) = dedup && let Err(e) = dedup .store_derived_blob( @@ -928,19 +928,19 @@ impl ThumbnailService { let bytes = Bytes::from(jpeg_bytes); - // External thumbnails are stored by file_id (not dedup-able) - let thumb_path = self - .thumbnails_root - .join(size.dir_name()) - .join(format!("ext-{}.jpg", file_id)); - if let Some(parent) = thumb_path.parent() { - let _ = fs::create_dir_all(parent).await; - } - fs::write(&thumb_path, &bytes) - .await - .map_err(|e| ThumbnailError::IoError(e.to_string()))?; - - // Populate in-memory cache (external thumbnails are JPEG) + // Step 10d2: the `ext-{file_id}.jpg` sidecar write is GONE. The + // durable store is now `file_attached_blobs`, written by the caller — + // which is why that write had to become fatal first, in the same + // change. These bytes have no server-side render path, so a + // best-effort store with no sidecar behind it would lose a user's + // upload silently. + // + // This function now re-encodes and caches; it does not persist. The + // RAM entry stays because it is what serves the request that follows, + // and the caller drops it if the durable write fails. + // + // Existing `ext-` files remain readable through the fallback tier + // until `thumb_attached_import` drains them. let cache_key = ThumbnailCacheKey::external(file_id, size); self.cache.insert(cache_key, bytes.clone()).await; diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 193d45df..7fcf2194 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -800,16 +800,26 @@ impl FileHandler { ) .await { - // ERROR, not WARN: the sidecar keeps the feature looking healthy - // on this box, so nothing else signals that copies are silently - // losing the preview. A syntax error in the upsert hid behind a - // warning for an entire test cycle exactly this way. + // FATAL as of step 10d2, where it used to warn and return 201. + // + // That was safe only while `ext-{file_id}.jpg` existed as a + // second copy. With the sidecar gone this is the ONLY durable + // home for bytes that have no server-side render path — a + // client-generated PDF preview cannot be recreated — so + // succeeding here would lose a user's upload behind a success + // response. Silent, and unrecoverable. + // + // The RAM entry is dropped too, or the cache would keep serving a + // preview that was never persisted and vanishes on eviction, + // contradicting the error the client just received. + let _ = thumbnail_service.delete_thumbnails(&id).await; tracing::error!( target: "oxicloud::dedup", error = %e, file_id = %id, - "failed to record attached thumbnail; sidecar written, copies will NOT inherit it" + "failed to record attached thumbnail; upload rejected" ); + return AppError::internal_error("Failed to store thumbnail").into_response(); } StatusCode::CREATED.into_response() diff --git a/tests/api/thumb_import_check.sh b/tests/api/thumb_import_check.sh index 763bfd22..552f0895 100755 --- a/tests/api/thumb_import_check.sh +++ b/tests/api/thumb_import_check.sh @@ -45,6 +45,7 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" COMPOSE_FILE="$REPO_ROOT/tests/common/docker-compose.test.yml" +STORAGE_PATH="${OXICLOUD_STORAGE_PATH:-$REPO_ROOT/tests/api/storage}" # shellcheck source=test.env source "$SCRIPT_DIR/test.env" @@ -131,6 +132,31 @@ curl -sf -X PUT -H "$AUTH" -H "Content-Type: image/png" \ UPLOADED_THUMB=$(mktemp) curl -sf -H "$AUTH" "$base_url/api/files/$FILE_ID/thumbnail/preview" -o "$UPLOADED_THUMB" +# ── 1b. Lay down the sidecars the server no longer writes ──────────────── +# +# Since step 10d2 the write paths persist ONLY to the blob tiers, so an +# upload no longer leaves anything under .thumbnails/ — which is the point, +# but it removes the source this test used to manufacture legacy state from. +# +# So write them here, with the bytes the API just served, at the exact paths +# the pre-10d2 code used: `{size}/{blob_hash}.jpg` for the rendered +# thumbnail and `{size}/ext-{file_id}.jpg` for the upload. Requests omit +# `Accept`, so both negotiate JPEG. +# +# This keeps the reconstruction faithful rather than approximate: same +# bytes, same paths, same filenames a pre-migration install holds. What it +# no longer does is rely on the current code to produce them — which it +# cannot, and should not. +SIDECAR_DIR="$STORAGE_PATH/.thumbnails/preview" +mkdir -p "$SIDECAR_DIR" +cp "$UPLOADED_THUMB" "$SIDECAR_DIR/ext-$FILE_ID.jpg" +cp "$UPLOADED_THUMB" "$SIDECAR_DIR/$BLOB_HASH.jpg" +# Identical bytes in both, which is realistic rather than a shortcut: they +# dedup to one blob, so the derived and attached rows end up referencing the +# same content while keeping separate mappings — exactly the property the +# keying split exists to preserve. +log "legacy sidecars written to $SIDECAR_DIR" + DERIVED_BEFORE=$(sql "SELECT count(*) FROM storage.content_derived_blobs WHERE source_hash='$BLOB_HASH';") ATTACHED_BEFORE=$(sql "SELECT count(*) FROM storage.file_attached_blobs WHERE file_id='$FILE_ID';") [[ "$DERIVED_BEFORE" -ge 1 ]] || fail "expected a content_derived_blobs row before stripping"