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"