fix(storage): thumb_derived_import claims JPEG sidecars too
The filter was strip_suffix(".webp"), but persist_rendered writes
{hash}.{format.ext()} — so any client not advertising WebP leaves
{hash}.jpg on disk. Correct only while the derived tier was WebP-only;
once variant carried the format (20261022000000) a JPEG sidecar became
ordinary content, and leaving it unclaimed would keep .thumbnails/
permanently non-empty — the very signal step 10e gates on. The migration
could never finish.
Both codecs are now claimed and the format comes from the file's own
extension, so a .jpg imports AS JPEG. Deriving the variant and
content_type from it rather than hardcoding WebP is the point: a
mislabelled row would serve the wrong codec to whoever the read path
then matched it for.
ThumbnailFormat::ALL exists so the claim list and the write path cannot
drift — adding a format without teaching the import about it would
strand that codec silently.
The `ext-` rejection now carries real weight. Previously .jpg was
rejected wholesale, so the two jobs could not overlap by construction;
now they share an extension and only the prefix separates them. Both
directions stay under test.
Caught by the cross-job assertion, which counts every real sidecar being
claimed exactly once — the fixture gained a .jpg and the total moved 3
to 4, which is the test noticing rather than a test to update.
This commit is contained in:
@@ -77,6 +77,12 @@ pub enum ThumbnailFormat {
|
|||||||
}
|
}
|
||||||
|
|
||||||
impl ThumbnailFormat {
|
impl ThumbnailFormat {
|
||||||
|
/// Every format, for callers that must handle all of them — notably
|
||||||
|
/// `thumb_derived_import`, which claims one sidecar extension per format
|
||||||
|
/// and would silently strand a codec if this list and the write path
|
||||||
|
/// drifted apart.
|
||||||
|
pub const ALL: [ThumbnailFormat; 2] = [ThumbnailFormat::Webp, ThumbnailFormat::Jpeg];
|
||||||
|
|
||||||
/// Stable name, byte-identical to the derived `Debug` output (see
|
/// Stable name, byte-identical to the derived `Debug` output (see
|
||||||
/// [`ThumbnailSize::as_str`] — same ETag-stability contract).
|
/// [`ThumbnailSize::as_str`] — same ETag-stability contract).
|
||||||
pub fn as_str(self) -> &'static str {
|
pub fn as_str(self) -> &'static str {
|
||||||
|
|||||||
@@ -386,10 +386,13 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
// And nothing real is dropped: README.txt is the only unclaimed file.
|
// And nothing real is dropped: README.txt is the only unclaimed file.
|
||||||
|
// Two content-keyed .webp, one content-keyed .jpg, one ext- upload.
|
||||||
|
// The .jpg pair is the interesting one: same extension, opposite
|
||||||
|
// keying, and only the `ext-` prefix separates them.
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
attached.len() + derived.len(),
|
attached.len() + derived.len(),
|
||||||
3,
|
4,
|
||||||
"the three real sidecars must be claimed exactly once between them"
|
"every real sidecar must be claimed exactly once between the two jobs"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -39,7 +39,7 @@ use async_trait::async_trait;
|
|||||||
use bytes::Bytes;
|
use bytes::Bytes;
|
||||||
use tokio::fs;
|
use tokio::fs;
|
||||||
|
|
||||||
use crate::application::ports::thumbnail_ports::ThumbnailSize;
|
use crate::application::ports::thumbnail_ports::{ThumbnailFormat, ThumbnailSize};
|
||||||
use crate::infrastructure::scheduler::{
|
use crate::infrastructure::scheduler::{
|
||||||
JobRegistry, JobRunArgs, JobStore, JobStoreProvider, RecoverableJobHandler, RunOutcome,
|
JobRegistry, JobRunArgs, JobStore, JobStoreProvider, RecoverableJobHandler, RunOutcome,
|
||||||
RunStatus, record_or_log,
|
RunStatus, record_or_log,
|
||||||
@@ -76,19 +76,35 @@ impl ThumbDerivedImport {
|
|||||||
self
|
self
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The hash a sidecar filename names, or `None` when the file is not one.
|
/// The hash and format a sidecar filename names, or `None` when the file
|
||||||
|
/// is not one of ours.
|
||||||
///
|
///
|
||||||
/// Strict, and deliberately rejects `ext-{file_id}.jpg`: those are
|
/// Strict, and deliberately rejects `ext-{file_id}.jpg`: those are
|
||||||
/// user-supplied, file-keyed bytes. Importing them here would content-key
|
/// user-supplied, file-keyed bytes. Importing them here would content-key
|
||||||
/// them and share one user's uploaded preview onto every file with
|
/// them and share one user's uploaded preview onto every file with
|
||||||
/// identical content — the poisoning `file_attached_blobs` exists to
|
/// identical content — the poisoning `file_attached_blobs` exists to
|
||||||
/// prevent. They belong to `thumb_attached_import`.
|
/// prevent. They belong to `thumb_attached_import`. That rejection
|
||||||
fn hash_from_sidecar_name(name: &str) -> Option<&str> {
|
/// carries the weight now that `.jpg` is otherwise claimed, since the two
|
||||||
let stem = name.strip_suffix(".webp")?;
|
/// jobs would otherwise both want it.
|
||||||
|
///
|
||||||
|
/// Returns the format too, because the row
|
||||||
|
/// key needs both since migration `20261022000000`.
|
||||||
|
///
|
||||||
|
/// Both codecs are claimed. `persist_rendered` writes
|
||||||
|
/// `{hash}.{format.ext()}`, so any client that does not advertise WebP
|
||||||
|
/// leaves `{hash}.jpg` on disk. While the derived tier was WebP-only
|
||||||
|
/// those were unmigratable by design; now that `variant` carries the
|
||||||
|
/// format they are ordinary content, and skipping them would leave
|
||||||
|
/// `.thumbnails/` permanently non-empty — which is the signal step 10e
|
||||||
|
/// gates the fallback removal on.
|
||||||
|
fn hash_from_sidecar_name(name: &str) -> Option<(&str, ThumbnailFormat)> {
|
||||||
|
let (stem, format) = ThumbnailFormat::ALL
|
||||||
|
.iter()
|
||||||
|
.find_map(|f| name.strip_suffix(&format!(".{}", f.ext())).map(|s| (s, *f)))?;
|
||||||
if stem.len() != 64 || !stem.chars().all(|c| c.is_ascii_hexdigit()) {
|
if stem.len() != 64 || !stem.chars().all(|c| c.is_ascii_hexdigit()) {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
Some(stem)
|
Some((stem, format))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Sorted sidecar filenames for one size directory.
|
/// Sorted sidecar filenames for one size directory.
|
||||||
@@ -159,25 +175,14 @@ impl RecoverableJobHandler for ThumbDerivedImport {
|
|||||||
let mut already = 0u64;
|
let mut already = 0u64;
|
||||||
let mut failed = 0u64;
|
let mut failed = 0u64;
|
||||||
let mut since_checkpoint = 0usize;
|
let mut since_checkpoint = 0usize;
|
||||||
// Two different strings, and conflating them is a real trap: the
|
// The DIRECTORY is `{size}` on disk; the VARIANT is `{size}.{ext}`
|
||||||
// DIRECTORY is `{size}` on disk, while the VARIANT is
|
// since migration `20261022000000`. Conflating them is a real trap:
|
||||||
// `{size}.{ext}` since migration `20261022000000`. Using the variant
|
// using the variant as a path yields `.thumbnails/preview.webp/…`,
|
||||||
// as a path yields `.thumbnails/preview.webp/…`, which does not
|
// which does not exist, so every file reads as unreadable and nothing
|
||||||
// exist, so every file reads as unreadable and nothing imports.
|
// imports. The variant is therefore built per FILE, from the format
|
||||||
//
|
// its extension names, not once per size.
|
||||||
// Sidecars here are always `{hash}.webp` — the name filter requires
|
|
||||||
// that extension — so the variant is unconditionally the WebP one.
|
|
||||||
let variant_of = |s: ThumbnailSize| {
|
|
||||||
format!(
|
|
||||||
"{}.{}",
|
|
||||||
s.dir_name(),
|
|
||||||
crate::application::ports::thumbnail_ports::ThumbnailFormat::Webp.ext()
|
|
||||||
)
|
|
||||||
};
|
|
||||||
|
|
||||||
for size in ThumbnailSize::all() {
|
for size in ThumbnailSize::all() {
|
||||||
let dir_name = size.dir_name(); // on-disk directory
|
let dir_name = size.dir_name(); // on-disk directory
|
||||||
let variant = variant_of(*size); // content_derived_blobs.variant
|
|
||||||
for name in Self::sidecar_names(&self.thumbnails_root, *size).await {
|
for name in Self::sidecar_names(&self.thumbnails_root, *size).await {
|
||||||
// Cursor position uses the DIRECTORY, so a run paused before
|
// Cursor position uses the DIRECTORY, so a run paused before
|
||||||
// this change resumes at the same place.
|
// this change resumes at the same place.
|
||||||
@@ -204,9 +209,15 @@ impl RecoverableJobHandler for ThumbDerivedImport {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
let Some(hash) = Self::hash_from_sidecar_name(&name) else {
|
let Some((hash, format)) = Self::hash_from_sidecar_name(&name) else {
|
||||||
continue;
|
continue;
|
||||||
};
|
};
|
||||||
|
// Both derived from the file's OWN extension, so a `.jpg`
|
||||||
|
// sidecar becomes a JPEG row rather than being mislabelled
|
||||||
|
// WebP — which would serve the wrong codec to anyone the read
|
||||||
|
// path then matched it for.
|
||||||
|
let variant = format!("{dir_name}.{}", format.ext());
|
||||||
|
let content_type = format.mime();
|
||||||
|
|
||||||
// Already mapped — the common case on a re-run, and the
|
// Already mapped — the common case on a re-run, and the
|
||||||
// reason this job is safe to trigger repeatedly.
|
// reason this job is safe to trigger repeatedly.
|
||||||
@@ -227,7 +238,7 @@ impl RecoverableJobHandler for ThumbDerivedImport {
|
|||||||
hash,
|
hash,
|
||||||
"thumbnail",
|
"thumbnail",
|
||||||
&variant,
|
&variant,
|
||||||
"image/webp",
|
content_type,
|
||||||
Bytes::from(data),
|
Bytes::from(data),
|
||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
@@ -310,12 +321,27 @@ pub(crate) mod tests {
|
|||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
const H: &str = "0a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6d7e8f9";
|
const H: &str = "0a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6d7e8f9";
|
||||||
|
/// A second hash, for the JPEG sidecar in `legacy_tree`.
|
||||||
|
const H2: &str = "c222222222222222222222222222222222222222222222222222222222222222";
|
||||||
|
|
||||||
|
/// BOTH codecs are claimed, and the format comes from the extension.
|
||||||
|
///
|
||||||
|
/// `.jpg` was previously rejected here, which was correct only while the
|
||||||
|
/// derived tier was WebP-only. Once `variant` carried the format
|
||||||
|
/// (migration `20261022000000`) a JPEG sidecar became ordinary content,
|
||||||
|
/// and leaving it unclaimed would keep `.thumbnails/` permanently
|
||||||
|
/// non-empty — the very signal step 10e gates on.
|
||||||
#[test]
|
#[test]
|
||||||
fn accepts_a_canonical_sidecar_name() {
|
fn accepts_both_codecs_and_reports_the_format() {
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
ThumbDerivedImport::hash_from_sidecar_name(&format!("{H}.webp")),
|
ThumbDerivedImport::hash_from_sidecar_name(&format!("{H}.webp")),
|
||||||
Some(H)
|
Some((H, ThumbnailFormat::Webp))
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
ThumbDerivedImport::hash_from_sidecar_name(&format!("{H}.jpg")),
|
||||||
|
Some((H, ThumbnailFormat::Jpeg)),
|
||||||
|
"a JPEG sidecar must import, and as JPEG — labelling it WebP \
|
||||||
|
would serve the wrong codec"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -349,6 +375,13 @@ pub(crate) mod tests {
|
|||||||
.await
|
.await
|
||||||
.unwrap();
|
.unwrap();
|
||||||
// Neither: a stray file that must be claimed by no one.
|
// Neither: a stray file that must be claimed by no one.
|
||||||
|
// Server-rendered JPEG: what a client not advertising WebP
|
||||||
|
// leaves behind. Claimed by the derived import, and must not be
|
||||||
|
// confused with the `ext-` upload above despite sharing an
|
||||||
|
// extension.
|
||||||
|
tokio::fs::write(dir.join(format!("{H2}.jpg")), b"jpeg")
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
tokio::fs::write(dir.join("README.txt"), b"nope")
|
tokio::fs::write(dir.join("README.txt"), b"nope")
|
||||||
.await
|
.await
|
||||||
.unwrap();
|
.unwrap();
|
||||||
@@ -371,8 +404,10 @@ pub(crate) mod tests {
|
|||||||
vec![
|
vec![
|
||||||
format!("{H}.webp"),
|
format!("{H}.webp"),
|
||||||
"b111111111111111111111111111111111111111111111111111111111111111.webp".to_string(),
|
"b111111111111111111111111111111111111111111111111111111111111111.webp".to_string(),
|
||||||
|
format!("{H2}.jpg"),
|
||||||
],
|
],
|
||||||
"must claim both content-keyed sidecars, sorted, and nothing else"
|
"must claim every content-keyed sidecar of EITHER codec, sorted, \
|
||||||
|
and nothing else"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -394,12 +429,15 @@ pub(crate) mod tests {
|
|||||||
#[test]
|
#[test]
|
||||||
fn rejects_external_and_malformed_names() {
|
fn rejects_external_and_malformed_names() {
|
||||||
for name in [
|
for name in [
|
||||||
|
// `ext-` prefixed: user-supplied and file-keyed, whatever the
|
||||||
|
// extension. Now that .jpg is otherwise claimed, this is the case
|
||||||
|
// that keeps the two jobs disjoint.
|
||||||
format!("ext-{H}.jpg"),
|
format!("ext-{H}.jpg"),
|
||||||
"ext-3f2b1c00-0000-0000-0000-000000000000.jpg".to_string(),
|
"ext-3f2b1c00-0000-0000-0000-000000000000.jpg".to_string(),
|
||||||
format!("{H}.jpg"),
|
|
||||||
format!("{}.webp", &H[..63]),
|
format!("{}.webp", &H[..63]),
|
||||||
H.to_string(),
|
H.to_string(),
|
||||||
"junk.webp".to_string(),
|
"junk.webp".to_string(),
|
||||||
|
"junk.jpg".to_string(),
|
||||||
] {
|
] {
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
ThumbDerivedImport::hash_from_sidecar_name(&name),
|
ThumbDerivedImport::hash_from_sidecar_name(&name),
|
||||||
|
|||||||
Reference in New Issue
Block a user