refactor(thumbnails): one persist_rendered for every render path
Step 10(a), the blocker. Four render paths each wrote the sidecar and
exactly one also recorded the content_derived_blobs row, so an on-demand
render — a cache miss, a size never generated, an evicted sidecar —
produced state the migration could never see. That breaks the
migration's premise rather than being untidy: thumb_derived_import would
never reach an empty tail, so the gate for deleting the sidecar would
never open.
Now every rendered thumbnail goes through persist_rendered, which owns
what persisting means. Raw `fs::write(&thumb_path, …)` drops from five
sites to two: the one inside persist_rendered, and
store_external_thumbnail's `ext-{file_id}.jpg`, which is file-keyed and
legitimately a different thing.
The path that matters most already had what it needed:
get_thumbnail_from_blob — the REST handler's fallthrough on a cache miss
— holds `dedup` and simply never used it for persistence. It now
dual-writes at no cost.
render_and_persist_all_webp had its own copy of the dual-write logic;
that copy is gone, so retiring the interim dual-write later is one edit
here rather than a hunt.
Two paths still pass `None` and remain sidecar-only: `get_thumbnail`
(renders from an on-disk original) and `generate_all_sizes_background`
(the path variant; the _from_blob sibling has dedup). Closing those
means threading a DedupService in from their callers. Left visible as an
explicit `None` at the call site rather than an absent write — the gap
is now something a reader trips over instead of something they have to
notice is missing.
Adds ThumbnailFormat::mime() beside ext(), since the derived row needs a
media type and an extension without a matching one is how a WebP ends up
labelled JPEG.
This commit is contained in:
@@ -94,6 +94,18 @@ impl ThumbnailFormat {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Media type, for `content_derived_blobs.content_type` and for any
|
||||||
|
/// response serving these bytes.
|
||||||
|
///
|
||||||
|
/// Beside `ext` deliberately: the two must agree, and an extension
|
||||||
|
/// without a matching media type is how a WebP ends up labelled JPEG.
|
||||||
|
pub fn mime(self) -> &'static str {
|
||||||
|
match self {
|
||||||
|
ThumbnailFormat::Webp => "image/webp",
|
||||||
|
ThumbnailFormat::Jpeg => "image/jpeg",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Pick the output format from a request `Accept` header: WebP when the
|
/// Pick the output format from a request `Accept` header: WebP when the
|
||||||
/// client advertises `image/webp`, JPEG otherwise. A plain substring check
|
/// client advertises `image/webp`, JPEG otherwise. A plain substring check
|
||||||
/// is sufficient — no client sends `image/webp;q=0`, and every WebP-capable
|
/// is sufficient — no client sends `image/webp;q=0`, and every WebP-capable
|
||||||
|
|||||||
@@ -261,6 +261,71 @@ impl ThumbnailService {
|
|||||||
.join(format!("{}.{}", blob_hash, format.ext()))
|
.join(format!("{}.{}", blob_hash, format.ext()))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Persist a freshly rendered thumbnail to every durable tier.
|
||||||
|
///
|
||||||
|
/// **One place that knows what persisting a thumbnail means.** Before
|
||||||
|
/// this, four render paths each wrote the sidecar and exactly one also
|
||||||
|
/// recorded the `content_derived_blobs` row, so an on-demand render — a
|
||||||
|
/// cache miss, a size never generated, an evicted sidecar — produced
|
||||||
|
/// state the migration could never see. That is not untidy, it breaks
|
||||||
|
/// the migration's premise: `thumb_derived_import` would never reach an
|
||||||
|
/// empty tail, and the gate for deleting the sidecar would never open.
|
||||||
|
///
|
||||||
|
/// Scope is deliberately *durable* tiers only. The moka entry is left to
|
||||||
|
/// callers because several persist through `cache.entry().or_insert_with`,
|
||||||
|
/// which already owns the insert; doing it here too would write twice.
|
||||||
|
///
|
||||||
|
/// Dual-write is the interim setting, not the destination. Once the
|
||||||
|
/// derived tier is authoritative and the imports have drained, dropping
|
||||||
|
/// the sidecar becomes a one-line change *here* rather than four edits
|
||||||
|
/// spread across the file — which is the point of consolidating first.
|
||||||
|
///
|
||||||
|
/// Both writes are best-effort and logged: the bytes are already rendered
|
||||||
|
/// and about to be served, so a persistence failure must cost a
|
||||||
|
/// re-render later, never the response now. `dedup: None` means
|
||||||
|
/// sidecar-only — a caller that could not supply one, which is visible at
|
||||||
|
/// the call site rather than hidden as a missing line.
|
||||||
|
async fn persist_rendered(
|
||||||
|
&self,
|
||||||
|
blob_hash: &str,
|
||||||
|
size: ThumbnailSize,
|
||||||
|
format: ThumbnailFormat,
|
||||||
|
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
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
if let Some(dedup) = dedup
|
||||||
|
&& let Err(e) = dedup
|
||||||
|
.store_derived_blob(
|
||||||
|
blob_hash,
|
||||||
|
"thumbnail",
|
||||||
|
size.dir_name(),
|
||||||
|
format.mime(),
|
||||||
|
bytes.clone(),
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
tracing::warn!(
|
||||||
|
target: "oxicloud::dedup",
|
||||||
|
error = %e,
|
||||||
|
"failed to record derived blob for {} {:?}",
|
||||||
|
&blob_hash[..blob_hash.len().min(12)],
|
||||||
|
size,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Get a thumbnail, generating it if needed.
|
/// Get a thumbnail, generating it if needed.
|
||||||
///
|
///
|
||||||
/// # Arguments
|
/// # Arguments
|
||||||
@@ -284,6 +349,7 @@ impl ThumbnailService {
|
|||||||
let thumb_path = self.get_thumbnail_path(blob_hash, size, format);
|
let thumb_path = self.get_thumbnail_path(blob_hash, size, format);
|
||||||
let original_owned = original_path.to_path_buf();
|
let original_owned = original_path.to_path_buf();
|
||||||
let file_id_owned = file_id.to_string();
|
let file_id_owned = file_id.to_string();
|
||||||
|
let blob_hash_owned = blob_hash.to_string();
|
||||||
|
|
||||||
// Moka's entry().or_insert_with() guarantees that for the same key
|
// Moka's entry().or_insert_with() guarantees that for the same key
|
||||||
// only ONE init closure runs; concurrent callers await the same
|
// only ONE init closure runs; concurrent callers await the same
|
||||||
@@ -306,11 +372,11 @@ impl ThumbnailService {
|
|||||||
tracing::info!("🎨 Generating thumbnail: {} {:?}", file_id_owned, size);
|
tracing::info!("🎨 Generating thumbnail: {} {:?}", file_id_owned, size);
|
||||||
match self.generate_thumbnail(&original_owned, size, format).await {
|
match self.generate_thumbnail(&original_owned, size, format).await {
|
||||||
Ok(bytes) => {
|
Ok(bytes) => {
|
||||||
// Save to disk (best-effort — don't fail the request)
|
// `None`: renders from an on-disk original and holds
|
||||||
if let Some(parent) = thumb_path.parent() {
|
// no DedupService, so sidecar-only. Visible here
|
||||||
let _ = fs::create_dir_all(parent).await;
|
// rather than absent.
|
||||||
}
|
self.persist_rendered(&blob_hash_owned, size, format, &bytes, None)
|
||||||
let _ = fs::write(&thumb_path, &bytes).await;
|
.await;
|
||||||
bytes
|
bytes
|
||||||
}
|
}
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
@@ -357,6 +423,7 @@ impl ThumbnailService {
|
|||||||
|
|
||||||
let thumb_path = self.get_thumbnail_path(blob_hash, size, format);
|
let thumb_path = self.get_thumbnail_path(blob_hash, size, format);
|
||||||
let file_id_owned = file_id.to_string();
|
let file_id_owned = file_id.to_string();
|
||||||
|
let blob_hash_owned = blob_hash.to_string();
|
||||||
|
|
||||||
let entry = self
|
let entry = self
|
||||||
.cache
|
.cache
|
||||||
@@ -375,8 +442,19 @@ impl ThumbnailService {
|
|||||||
tracing::warn!("Decode semaphore closed, skipping {}", file_id_owned);
|
tracing::warn!("Decode semaphore closed, skipping {}", file_id_owned);
|
||||||
return Bytes::new();
|
return Bytes::new();
|
||||||
};
|
};
|
||||||
self.generate_and_persist(&file_id_owned, &thumb_path, size, format, original_data)
|
// `None`: this entry point takes the original bytes directly
|
||||||
.await
|
// and has no DedupService, so it persists sidecar-only. The
|
||||||
|
// gap is visible here rather than hidden as a missing write,
|
||||||
|
// and closing it means threading dedup in from its callers.
|
||||||
|
self.generate_and_persist(
|
||||||
|
&file_id_owned,
|
||||||
|
&blob_hash_owned,
|
||||||
|
size,
|
||||||
|
format,
|
||||||
|
original_data,
|
||||||
|
None,
|
||||||
|
)
|
||||||
|
.await
|
||||||
})
|
})
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
@@ -440,8 +518,19 @@ impl ThumbnailService {
|
|||||||
return Bytes::new();
|
return Bytes::new();
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
self.generate_and_persist(&file_id_owned, &thumb_path, size, format, original_data)
|
// The on-demand render the REST handler falls through to on a
|
||||||
.await
|
// cache miss — the busiest path that previously wrote a
|
||||||
|
// sidecar and no row. `dedup` is already in scope here, so
|
||||||
|
// dual-writing costs nothing.
|
||||||
|
self.generate_and_persist(
|
||||||
|
&file_id_owned,
|
||||||
|
&blob_hash_owned,
|
||||||
|
size,
|
||||||
|
format,
|
||||||
|
original_data,
|
||||||
|
Some(dedup.as_ref()),
|
||||||
|
)
|
||||||
|
.await
|
||||||
})
|
})
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
@@ -464,10 +553,11 @@ impl ThumbnailService {
|
|||||||
async fn generate_and_persist(
|
async fn generate_and_persist(
|
||||||
&self,
|
&self,
|
||||||
file_id: &str,
|
file_id: &str,
|
||||||
thumb_path: &Path,
|
blob_hash: &str,
|
||||||
size: ThumbnailSize,
|
size: ThumbnailSize,
|
||||||
format: ThumbnailFormat,
|
format: ThumbnailFormat,
|
||||||
original_data: Bytes,
|
original_data: Bytes,
|
||||||
|
dedup: Option<&DedupService>,
|
||||||
) -> Bytes {
|
) -> Bytes {
|
||||||
tracing::info!("🎨 Generating thumbnail: {} {:?}", file_id, size);
|
tracing::info!("🎨 Generating thumbnail: {} {:?}", file_id, size);
|
||||||
match Self::generate_thumbnail_from_data(
|
match Self::generate_thumbnail_from_data(
|
||||||
@@ -479,10 +569,8 @@ impl ThumbnailService {
|
|||||||
.await
|
.await
|
||||||
{
|
{
|
||||||
Ok(bytes) => {
|
Ok(bytes) => {
|
||||||
if let Some(parent) = thumb_path.parent() {
|
self.persist_rendered(blob_hash, size, format, &bytes, dedup)
|
||||||
let _ = fs::create_dir_all(parent).await;
|
.await;
|
||||||
}
|
|
||||||
let _ = fs::write(&thumb_path, &bytes).await;
|
|
||||||
bytes
|
bytes
|
||||||
}
|
}
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
@@ -1202,13 +1290,12 @@ impl ThumbnailService {
|
|||||||
// Save each size to disk and populate moka — both keyed by
|
// Save each size to disk and populate moka — both keyed by
|
||||||
// blob_hash, so the two tiers agree and a copy shares them.
|
// blob_hash, so the two tiers agree and a copy shares them.
|
||||||
for (size, bytes) in thumbnails {
|
for (size, bytes) in thumbnails {
|
||||||
let thumb_path = self.get_thumbnail_path(&blob_hash, size, ThumbnailFormat::Webp);
|
// `None`: this variant renders from a path and holds no
|
||||||
if let Some(parent) = thumb_path.parent() {
|
// DedupService — `generate_all_sizes_background_from_blob` is
|
||||||
let _ = fs::create_dir_all(parent).await;
|
// the one that does. Sidecar-only, visibly so.
|
||||||
}
|
self.persist_rendered(&blob_hash, size, ThumbnailFormat::Webp, &bytes, None)
|
||||||
if let Err(e) = fs::write(&thumb_path, &bytes).await {
|
.await;
|
||||||
tracing::warn!("Failed to save thumbnail {} {:?}: {}", file_id, size, e);
|
{
|
||||||
} else {
|
|
||||||
// Populate in-memory cache for instant first-hit serving
|
// Populate in-memory cache for instant first-hit serving
|
||||||
let cache_key =
|
let cache_key =
|
||||||
ThumbnailCacheKey::content(&blob_hash, size, ThumbnailFormat::Webp);
|
ThumbnailCacheKey::content(&blob_hash, size, ThumbnailFormat::Webp);
|
||||||
@@ -1346,37 +1433,12 @@ impl ThumbnailService {
|
|||||||
};
|
};
|
||||||
|
|
||||||
for (size, bytes) in thumbnails {
|
for (size, bytes) in thumbnails {
|
||||||
let thumb_path = self.get_thumbnail_path(blob_hash, size, ThumbnailFormat::Webp);
|
// Was the only path that wrote both tiers, with its own copy of
|
||||||
if let Some(parent) = thumb_path.parent() {
|
// the logic. Now the same `persist_rendered` every other render
|
||||||
let _ = fs::create_dir_all(parent).await;
|
// path uses, so there is one definition of what persisting means
|
||||||
}
|
// and the interim dual-write can be retired in one place.
|
||||||
if let Err(e) = fs::write(&thumb_path, &bytes).await {
|
self.persist_rendered(blob_hash, size, ThumbnailFormat::Webp, &bytes, dedup)
|
||||||
tracing::warn!("Failed to save thumbnail {} {:?}: {}", file_id, size, e);
|
.await;
|
||||||
continue;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Tier-3 copy. Best-effort and logged: a failure here must not
|
|
||||||
// cost the user their thumbnail, which is already on disk and in
|
|
||||||
// the cache. `derived_import` sweeps anything missed.
|
|
||||||
if let Some(dedup) = dedup
|
|
||||||
&& let Err(e) = dedup
|
|
||||||
.store_derived_blob(
|
|
||||||
blob_hash,
|
|
||||||
"thumbnail",
|
|
||||||
size.dir_name(),
|
|
||||||
"image/webp",
|
|
||||||
bytes.clone(),
|
|
||||||
)
|
|
||||||
.await
|
|
||||||
{
|
|
||||||
tracing::warn!(
|
|
||||||
target: "oxicloud::dedup",
|
|
||||||
error = %e,
|
|
||||||
"failed to record derived blob for {} {:?}",
|
|
||||||
file_id,
|
|
||||||
size,
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
let cache_key = ThumbnailCacheKey::content(blob_hash, size, ThumbnailFormat::Webp);
|
let cache_key = ThumbnailCacheKey::content(blob_hash, size, ThumbnailFormat::Webp);
|
||||||
self.cache.insert(cache_key, bytes).await;
|
self.cache.insert(cache_key, bytes).await;
|
||||||
|
|||||||
Reference in New Issue
Block a user