From 1f4168790610fba43098cde854a4cef2058019d0 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 14:33:48 -0700 Subject: [PATCH 1/9] ref(snapshots): Key manifest entries by relative path instead of hash Switch the snapshot manifest dictionary key from content hash to relative file path. The content hash is preserved in the image metadata extra field, enabling the backend to support both key formats during the transition. Co-Authored-By: Claude --- src/commands/build/snapshots.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index 25d74e09d5..1099b643f9 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -360,13 +360,19 @@ fn upload_images( .to_string_lossy() .into_owned(); - let extra = read_sidecar_metadata(&image.path).unwrap_or_else(|err| { + let relative_path_key = image.relative_path.to_string_lossy().into_owned(); + + let mut extra = read_sidecar_metadata(&image.path).unwrap_or_else(|err| { warn!("Error reading sidecar metadata, ignoring it instead: {err:#}"); HashMap::new() }); + extra.insert( + "content_hash".to_string(), + serde_json::Value::String(hash), + ); manifest_entries.insert( - hash, + relative_path_key, ImageMetadata::new(image_file_name, image.width, image.height, extra), ); } From f9a235def142dd922362d431f7f8af4a3f910d7e Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 14:44:06 -0700 Subject: [PATCH 2/9] fix(snapshots): Normalize path separators in manifest keys Use path_as_url to normalize backslashes to forward slashes on Windows, ensuring manifest keys are platform-independent. Co-Authored-By: Claude --- src/commands/build/snapshots.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index 1099b643f9..c8ddf11330 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -360,16 +360,13 @@ fn upload_images( .to_string_lossy() .into_owned(); - let relative_path_key = image.relative_path.to_string_lossy().into_owned(); + let relative_path_key = crate::utils::fs::path_as_url(&image.relative_path); let mut extra = read_sidecar_metadata(&image.path).unwrap_or_else(|err| { warn!("Error reading sidecar metadata, ignoring it instead: {err:#}"); HashMap::new() }); - extra.insert( - "content_hash".to_string(), - serde_json::Value::String(hash), - ); + extra.insert("content_hash".to_string(), serde_json::Value::String(hash)); manifest_entries.insert( relative_path_key, From e2b1f144292f964ba554d776236e76b2a83507a8 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 14:45:57 -0700 Subject: [PATCH 3/9] fix(snapshots): Use to_owned instead of to_string for str literal Fixes clippy::str_to_string lint. Co-Authored-By: Claude --- src/commands/build/snapshots.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index c8ddf11330..256f3a6888 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -366,7 +366,7 @@ fn upload_images( warn!("Error reading sidecar metadata, ignoring it instead: {err:#}"); HashMap::new() }); - extra.insert("content_hash".to_string(), serde_json::Value::String(hash)); + extra.insert("content_hash".to_owned(), serde_json::Value::String(hash)); manifest_entries.insert( relative_path_key, From 1389be15fbc1c694ae8a35269330be8defc4472a Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 15:20:29 -0700 Subject: [PATCH 4/9] ref(snapshots): Key manifest by file name with collision warnings Switch manifest key from relative path to file name (basename). When multiple images share the same file name from different directories, keep the first and warn with the full relative paths of all collisions so the user can debug and resolve duplicates. Co-Authored-By: Claude --- src/commands/build/snapshots.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index 256f3a6888..643d5cabec 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -331,6 +331,7 @@ fn upload_images( let mut many_builder = session.many(); let mut manifest_entries = HashMap::new(); + let mut collisions: HashMap> = HashMap::new(); let image_count = images.len(); for image in images { @@ -360,7 +361,15 @@ fn upload_images( .to_string_lossy() .into_owned(); - let relative_path_key = crate::utils::fs::path_as_url(&image.relative_path); + let relative_path = crate::utils::fs::path_as_url(&image.relative_path); + + if manifest_entries.contains_key(&image_file_name) { + collisions + .entry(image_file_name) + .or_default() + .push(relative_path); + continue; + } let mut extra = read_sidecar_metadata(&image.path).unwrap_or_else(|err| { warn!("Error reading sidecar metadata, ignoring it instead: {err:#}"); @@ -369,11 +378,22 @@ fn upload_images( extra.insert("content_hash".to_owned(), serde_json::Value::String(hash)); manifest_entries.insert( - relative_path_key, + image_file_name.clone(), ImageMetadata::new(image_file_name, image.width, image.height, extra), ); } + if !collisions.is_empty() { + let mut details = String::new(); + for (name, paths) in &collisions { + details.push_str(&format!("\n {name}:")); + for path in paths { + details.push_str(&format!("\n - {path}")); + } + } + warn!("Some images have been excluded due to having identical file names!{details}"); + } + let result = runtime.block_on(async { many_builder.send().error_for_failures().await }); match result { From c0f2341ea7ca8d7141191172c007dec78f45c0c0 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 15:29:31 -0700 Subject: [PATCH 5/9] ref(snapshots): Remove image_file_name from manifest values The manifest key is now the image file name, so storing it again inside the value is redundant. Remove the field from ImageMetadata and let the backend derive it from the key instead. Co-Authored-By: Claude --- src/api/data_types/snapshots.rs | 16 ++-------------- src/commands/build/snapshots.rs | 4 ++-- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/api/data_types/snapshots.rs b/src/api/data_types/snapshots.rs index 698c45e4a0..62fe80d4dd 100644 --- a/src/api/data_types/snapshots.rs +++ b/src/api/data_types/snapshots.rs @@ -7,7 +7,6 @@ use serde_json::Value; use super::VcsInfo; -const IMAGE_FILE_NAME_FIELD: &str = "image_file_name"; const WIDTH_FIELD: &str = "width"; const HEIGHT_FIELD: &str = "height"; @@ -42,16 +41,7 @@ pub struct ImageMetadata { } impl ImageMetadata { - pub fn new( - image_file_name: String, - width: u32, - height: u32, - mut extra: HashMap, - ) -> Self { - extra.insert( - IMAGE_FILE_NAME_FIELD.to_owned(), - Value::String(image_file_name), - ); + pub fn new(width: u32, height: u32, mut extra: HashMap) -> Self { extra.insert(WIDTH_FIELD.to_owned(), Value::from(width)); extra.insert(HEIGHT_FIELD.to_owned(), Value::from(height)); @@ -68,18 +58,16 @@ mod tests { #[test] fn cli_managed_fields_override_sidecar_fields() { let extra = serde_json::from_value(json!({ - (IMAGE_FILE_NAME_FIELD): "from-sidecar.png", (WIDTH_FIELD): 1, (HEIGHT_FIELD): 2, "custom": "keep-me" })) .unwrap(); - let metadata = ImageMetadata::new("from-cli.png".to_owned(), 100, 200, extra); + let metadata = ImageMetadata::new(100, 200, extra); let serialized = serde_json::to_value(metadata).unwrap(); let expected = json!({ - (IMAGE_FILE_NAME_FIELD): "from-cli.png", (WIDTH_FIELD): 100, (HEIGHT_FIELD): 200, "custom": "keep-me" diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index 643d5cabec..4d27e98ff8 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -378,8 +378,8 @@ fn upload_images( extra.insert("content_hash".to_owned(), serde_json::Value::String(hash)); manifest_entries.insert( - image_file_name.clone(), - ImageMetadata::new(image_file_name, image.width, image.height, extra), + image_file_name, + ImageMetadata::new(image.width, image.height, extra), ); } From 81343bffb8ebf29ebe11eb8e35d3c214d15983cb Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 15:43:53 -0700 Subject: [PATCH 6/9] fix(snapshots): Skip collision uploads before queueing to objectstore Move the collision check before the file open and upload queue so colliding images are not uploaded unnecessarily. Co-Authored-By: Claude --- src/commands/build/snapshots.rs | 34 ++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index 4d27e98ff8..6a76e7fd23 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -337,23 +337,6 @@ fn upload_images( for image in images { debug!("Processing image: {}", image.path.display()); - let hash = compute_sha256_hash(&image.path)?; - let file = runtime - .block_on(tokio::fs::File::open(&image.path)) - .with_context(|| { - format!("Failed to open image for upload: {}", image.path.display()) - })?; - - let key = format!("{org_id}/{project_id}/{hash}"); - info!("Queueing {} as {key}", image.relative_path.display()); - - many_builder = many_builder.push( - session - .put_file(file) - .key(&key) - .expiration_policy(expiration), - ); - let image_file_name = image .relative_path .file_name() @@ -371,6 +354,23 @@ fn upload_images( continue; } + let hash = compute_sha256_hash(&image.path)?; + let file = runtime + .block_on(tokio::fs::File::open(&image.path)) + .with_context(|| { + format!("Failed to open image for upload: {}", image.path.display()) + })?; + + let key = format!("{org_id}/{project_id}/{hash}"); + info!("Queueing {} as {key}", image.relative_path.display()); + + many_builder = many_builder.push( + session + .put_file(file) + .key(&key) + .expiration_policy(expiration), + ); + let mut extra = read_sidecar_metadata(&image.path).unwrap_or_else(|err| { warn!("Error reading sidecar metadata, ignoring it instead: {err:#}"); HashMap::new() From b292b2507c635236cf439f940e3f03b0992c89f7 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 16:11:32 -0700 Subject: [PATCH 7/9] fix(snapshots): Report actual uploaded count excluding collisions Use manifest_entries.len() instead of the initial images count so the success and error messages reflect skipped collisions. Co-Authored-By: Claude --- src/commands/build/snapshots.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index 6a76e7fd23..b08e8d9828 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -332,8 +332,6 @@ fn upload_images( let mut many_builder = session.many(); let mut manifest_entries = HashMap::new(); let mut collisions: HashMap> = HashMap::new(); - let image_count = images.len(); - for image in images { debug!("Processing image: {}", image.path.display()); @@ -396,13 +394,15 @@ fn upload_images( let result = runtime.block_on(async { many_builder.send().error_for_failures().await }); + let uploaded_count = manifest_entries.len(); + match result { Ok(()) => { println!( "{} Uploaded {} image {}", style(">").dim(), - style(image_count).yellow(), - if image_count == 1 { "file" } else { "files" } + style(uploaded_count).yellow(), + if uploaded_count == 1 { "file" } else { "files" } ); Ok(manifest_entries) } @@ -414,7 +414,7 @@ fn upload_images( eprintln!(" {}", style(format!("{error:#}")).red()); error_count += 1; } - anyhow::bail!("Failed to upload {error_count} out of {image_count} images") + anyhow::bail!("Failed to upload {error_count} out of {uploaded_count} images") } } } From dff013c57e4a66ea40d0ef4357618f8a8e109f5b Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 16:38:48 -0700 Subject: [PATCH 8/9] fix(snapshots): Include kept path in collision warning message The collision warning now lists all paths sharing a filename (kept and excluded) on one line per group, making it clear which image was retained and which were skipped. Co-Authored-By: Claude --- src/commands/build/snapshots.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index b08e8d9828..cc48284323 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -332,6 +332,7 @@ fn upload_images( let mut many_builder = session.many(); let mut manifest_entries = HashMap::new(); let mut collisions: HashMap> = HashMap::new(); + let mut kept_paths: HashMap = HashMap::new(); for image in images { debug!("Processing image: {}", image.path.display()); @@ -375,6 +376,7 @@ fn upload_images( }); extra.insert("content_hash".to_owned(), serde_json::Value::String(hash)); + kept_paths.insert(image_file_name.clone(), relative_path); manifest_entries.insert( image_file_name, ImageMetadata::new(image.width, image.height, extra), @@ -383,13 +385,12 @@ fn upload_images( if !collisions.is_empty() { let mut details = String::new(); - for (name, paths) in &collisions { - details.push_str(&format!("\n {name}:")); - for path in paths { - details.push_str(&format!("\n - {path}")); - } + for (name, excluded_paths) in &collisions { + let mut all_paths = vec![kept_paths[name].as_str()]; + all_paths.extend(excluded_paths.iter().map(|s| s.as_str())); + details.push_str(&format!("\n {name}: {}", all_paths.join(", "))); } - warn!("Some images have been excluded due to having identical file names!{details}"); + warn!("Some images share identical file names. Only the first occurrence of each is included:{details}"); } let result = runtime.block_on(async { many_builder.send().error_for_failures().await }); From 14356794993dd305bf9e2e26eec8ecf8a991594e Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 24 Mar 2026 18:04:56 -0700 Subject: [PATCH 9/9] also include codeowners for snapshots file --- .github/CODEOWNERS | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index f15ac7c9d7..5579263b31 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -13,6 +13,7 @@ # Files co-owned by Emerge Tools team /apple-catalog-parsing @getsentry/emerge-tools @getsentry/owners-sentry-cli +/src/api/data_types/snapshots.rs @getsentry/emerge-tools @getsentry/owners-sentry-cli /src/commands/build @getsentry/emerge-tools @getsentry/owners-sentry-cli /src/utils/build @getsentry/emerge-tools @getsentry/owners-sentry-cli /tests/integration/build @getsentry/emerge-tools @getsentry/owners-sentry-cli