From 1f6ef33092b39f7c8c8939869b557a968152c545 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Thu, 2 Oct 2025 17:33:00 -0700 Subject: [PATCH 1/2] Another attempt to fix spurious test failures Following up on #1052 there are still spurious test failures in other tests, e.g., sync::sync_test::test_sync_ref_including_annotation_blob, that fail because the spfs config is set to legacy encoding format. These tests that change the config were not putting the config back to defaults, so that could explain the spurious failures depending on the order that tests run. Although, it is suspicious that the tests weren't failing more often if this is the cause. Signed-off-by: J Robert Ray --- crates/spfs/src/graph/layer_test.rs | 188 +++++--- crates/spfs/src/runtime/storage_test.rs | 572 ++++++++++++------------ 2 files changed, 420 insertions(+), 340 deletions(-) diff --git a/crates/spfs/src/graph/layer_test.rs b/crates/spfs/src/graph/layer_test.rs index a540d151f4..ee976bea74 100644 --- a/crates/spfs/src/graph/layer_test.rs +++ b/crates/spfs/src/graph/layer_test.rs @@ -22,6 +22,72 @@ fn test_layer_encoding_manifest_only() { assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()) } +/// A macro to reset the global config after running a block of code, +/// even if that block of code panics. +#[macro_export] +macro_rules! reset_config { + ($($body:tt)*) => {{ + let reset_config_original = Config::current().unwrap(); + let _ = std::panic::catch_unwind(|| { + $($body)* + }).unwrap_or_else(|e| { + (*reset_config_original).clone().make_current().unwrap(); + + std::panic::resume_unwind(e); + }); + (*reset_config_original).clone().make_current().unwrap(); + }}; +} + +/// Sanity test to ensure that the reset_config macro catches panics +#[rstest] +#[should_panic] +fn reset_config_catches_panics() { + reset_config! { + panic!("This is a test panic"); + + #[allow(unreachable_code)] + Ok::<(), ()>(()) + }; +} + +/// A macro to reset the global config after running a block of code, +/// even if that block of code panics. +#[macro_export] +macro_rules! reset_config_async { + ($($body:tt)*) => {{ + let reset_config_original = Config::current().unwrap(); + let reset_config_handle = tokio::task::spawn(async move { + $($body)* + }); + let result = reset_config_handle.await; + (*reset_config_original).clone().make_current().unwrap(); + match result { + Ok(_) => {} + Err(err) if err.is_panic() => { + let err = err.into_panic(); + std::panic::resume_unwind(err); + } + Err(err) => { + panic!("Task failed to complete: {err}"); + } + } + }}; +} + +/// Sanity test to ensure that the reset_config_async macro catches panics +#[rstest] +#[should_panic] +#[tokio::test] +async fn reset_config_async_catches_panics() { + reset_config_async! { + panic!("This is a test panic"); + + #[allow(unreachable_code)] + Ok::<(), ()>(()) + }; +} + #[rstest( write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], @@ -31,34 +97,36 @@ fn test_layer_encoding_annotation_only( write_encoding_format: EncodingFormat, write_digest_strategy: DigestStrategy, ) { - let mut config = Config::default(); - config.storage.encoding_format = write_encoding_format; - config.storage.digest_strategy = write_digest_strategy; - config.make_current().unwrap(); + reset_config! { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); - let expected = Layer::new_with_annotation("key", AnnotationValue::string("value")); + let expected = Layer::new_with_annotation("key", AnnotationValue::string("value")); - let mut stream = Vec::new(); - match expected.encode(&mut stream) { - Ok(_) if write_encoding_format == EncodingFormat::Legacy => { - panic!("Encode should fail if encoding format is legacy") - } - Ok(_) => {} - Err(_) if write_encoding_format == EncodingFormat::Legacy => { - // This error is expected - return; - } - Err(e) => { - panic!("Error encoding layer: {e}") - } - }; + let mut stream = Vec::new(); + match expected.encode(&mut stream) { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Encode should fail if encoding format is legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error encoding layer: {e}") + } + }; - let decoded = Object::decode(&mut stream.as_slice()); + let decoded = Object::decode(&mut stream.as_slice()); - let actual = decoded.unwrap().into_layer().unwrap(); - println!(" Actual: {:?}", actual); + let actual = decoded.unwrap().into_layer().unwrap(); + println!(" Actual: {:?}", actual); - assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()) + assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()) + }; } #[rstest( @@ -70,46 +138,48 @@ fn test_layer_encoding_manifest_and_annotations( write_encoding_format: EncodingFormat, write_digest_strategy: DigestStrategy, ) { - let mut config = Config::default(); - config.storage.encoding_format = write_encoding_format; - config.storage.digest_strategy = write_digest_strategy; - config.make_current().unwrap(); + reset_config! { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); - let expected = Layer::new_with_manifest_and_annotations( - encoding::EMPTY_DIGEST.into(), - vec![("key", AnnotationValue::string("value"))], - ); - println!("Expected: {:?}", expected); + let expected = Layer::new_with_manifest_and_annotations( + encoding::EMPTY_DIGEST.into(), + vec![("key", AnnotationValue::string("value"))], + ); + println!("Expected: {:?}", expected); - let mut stream = Vec::new(); - match expected.encode(&mut stream) { - Ok(_) if write_encoding_format == EncodingFormat::Legacy => { - panic!("Encode should fail if encoding format is legacy") - } - Ok(_) => {} - Err(_) if write_encoding_format == EncodingFormat::Legacy => { - // This error is expected - return; - } - Err(e) => { - panic!("Error encoding layer: {e}") - } - }; + let mut stream = Vec::new(); + match expected.encode(&mut stream) { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Encode should fail if encoding format is legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error encoding layer: {e}") + } + }; - let actual = Object::decode(&mut stream.as_slice()) - .unwrap() - .into_layer() - .unwrap(); - println!(" Actual: {:?}", actual); + let actual = Object::decode(&mut stream.as_slice()) + .unwrap() + .into_layer() + .unwrap(); + println!(" Actual: {:?}", actual); - match write_encoding_format { - EncodingFormat::Legacy => { - unreachable!(); - } - EncodingFormat::FlatBuffers => { - // Under flatbuffers encoding both will contain the - // annotation data and will match - assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()) + match write_encoding_format { + EncodingFormat::Legacy => { + unreachable!(); + } + EncodingFormat::FlatBuffers => { + // Under flatbuffers encoding both will contain the + // annotation data and will match + assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()) + } } } } diff --git a/crates/spfs/src/runtime/storage_test.rs b/crates/spfs/src/runtime/storage_test.rs index 0eaab72eb4..add11be58f 100644 --- a/crates/spfs/src/runtime/storage_test.rs +++ b/crates/spfs/src/runtime/storage_test.rs @@ -17,7 +17,7 @@ use crate::graph::object::{DigestStrategy, EncodingFormat}; use crate::graph::{AnnotationValue, Layer, Platform}; use crate::runtime::{BindMount, KeyValuePair, LiveLayer, LiveLayerContents, SpecApiVersion}; use crate::storage::prelude::DatabaseExt; -use crate::{Config, encoding}; +use crate::{Config, encoding, reset_config_async}; #[rstest] fn test_config_serialization() { @@ -68,59 +68,61 @@ async fn test_storage_runtime_with_annotation( write_encoding_format: EncodingFormat, write_digest_strategy: DigestStrategy, ) { - let mut config = Config::default(); - config.storage.encoding_format = write_encoding_format; - config.storage.digest_strategy = write_digest_strategy; - config.make_current().unwrap(); - - let root = tmpdir.path().to_string_lossy().to_string(); - let repo = crate::storage::RepositoryHandle::from( - crate::storage::fs::MaybeOpenFsRepository::create(root) + reset_config_async! { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + + let root = tmpdir.path().to_string_lossy().to_string(); + let repo = crate::storage::RepositoryHandle::from( + crate::storage::fs::MaybeOpenFsRepository::create(root) + .await + .unwrap(), + ); + let storage = Storage::new(repo).unwrap(); + let limit: usize = 16 * 1024; + + let keep_runtime = false; + let live_layers = Vec::new(); + let mut runtime = storage + .create_named_runtime("test-with-annotation-data", keep_runtime, live_layers) .await - .unwrap(), - ); - let storage = Storage::new(repo).unwrap(); - let limit: usize = 16 * 1024; - - let keep_runtime = false; - let live_layers = Vec::new(); - let mut runtime = storage - .create_named_runtime("test-with-annotation-data", keep_runtime, live_layers) - .await - .expect("failed to create runtime in storage"); - - // Test - insert data - let key = "some_field".to_string(); - let value = "some value".to_string(); - match runtime.add_annotation(&key, &value, limit).await { - Ok(_) if write_encoding_format == EncodingFormat::Legacy => { - panic!("Writing annotations should fail when using EncodingFormat::Legacy") - } - Ok(_) => {} - Err(_) if write_encoding_format == EncodingFormat::Legacy => { - // This error is expected - return; - } - Err(e) => { - panic!("Error adding annotations: {e}") - } - }; + .expect("failed to create runtime in storage"); + + // Test - insert data + let key = "some_field".to_string(); + let value = "some value".to_string(); + match runtime.add_annotation(&key, &value, limit).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; - // Test - insert some more data - let value2 = "some other value".to_string(); - assert!(runtime.add_annotation(&key, &value2, limit).await.is_ok()); + // Test - insert some more data + let value2 = "some other value".to_string(); + assert!(runtime.add_annotation(&key, &value2, limit).await.is_ok()); - // Test - retrieve data - the first inserted data should be the - // what is retrieved because of how adding to the runtime stack - // works. - if write_encoding_format == EncodingFormat::Legacy { - unreachable!(); - }; + // Test - retrieve data - the first inserted data should be the + // what is retrieved because of how adding to the runtime stack + // works. + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); + }; - let result = runtime.annotation(&key).await.unwrap(); - assert!(result.is_some()); + let result = runtime.annotation(&key).await.unwrap(); + assert!(result.is_some()); - assert!(value == *result.unwrap()); + assert!(value == *result.unwrap()); + } } #[rstest( @@ -134,64 +136,66 @@ async fn test_storage_runtime_add_annotations_list( write_encoding_format: EncodingFormat, write_digest_strategy: DigestStrategy, ) { - let mut config = Config::default(); - config.storage.encoding_format = write_encoding_format; - config.storage.digest_strategy = write_digest_strategy; - config.make_current().unwrap(); - - let root = tmpdir.path().to_string_lossy().to_string(); - let repo = crate::storage::RepositoryHandle::from( - crate::storage::fs::MaybeOpenFsRepository::create(root) + reset_config_async! { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + + let root = tmpdir.path().to_string_lossy().to_string(); + let repo = crate::storage::RepositoryHandle::from( + crate::storage::fs::MaybeOpenFsRepository::create(root) + .await + .unwrap(), + ); + let storage = Storage::new(repo).unwrap(); + let limit: usize = 16 * 1024; + + let keep_runtime = false; + let live_layers = Vec::new(); + let mut runtime = storage + .create_named_runtime("test-with-annotation-data", keep_runtime, live_layers) .await - .unwrap(), - ); - let storage = Storage::new(repo).unwrap(); - let limit: usize = 16 * 1024; + .expect("failed to create runtime in storage"); - let keep_runtime = false; - let live_layers = Vec::new(); - let mut runtime = storage - .create_named_runtime("test-with-annotation-data", keep_runtime, live_layers) - .await - .expect("failed to create runtime in storage"); + // Test - insert data + let key = "some_field".to_string(); + let value = "some value".to_string(); + let key2 = "some_other_field".to_string(); + let value2 = "some other value".to_string(); - // Test - insert data - let key = "some_field".to_string(); - let value = "some value".to_string(); - let key2 = "some_other_field".to_string(); - let value2 = "some other value".to_string(); + let annotations: Vec = vec![(&key, &value), (&key2, &value2)]; - let annotations: Vec = vec![(&key, &value), (&key2, &value2)]; - - match runtime.add_annotations(annotations, limit).await { - Ok(_) if write_encoding_format == EncodingFormat::Legacy => { - panic!("Writing annotations should fail when using EncodingFormat::Legacy") - } - Ok(_) => {} - Err(_) if write_encoding_format == EncodingFormat::Legacy => { - // This error is expected - return; - } - Err(e) => { - panic!("Error adding annotations: {e}") + match runtime.add_annotations(annotations, limit).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; + + // Test - retrieve data both pieces of data + let result = runtime.annotation(&key).await.unwrap(); + if write_encoding_format == EncodingFormat::Legacy { + unreachable!() + } else { + assert!(result.is_some()); + assert!(value == *result.unwrap()); } - }; - - // Test - retrieve data both pieces of data - let result = runtime.annotation(&key).await.unwrap(); - if write_encoding_format == EncodingFormat::Legacy { - unreachable!() - } else { - assert!(result.is_some()); - assert!(value == *result.unwrap()); - } - let result2 = runtime.annotation(&key2).await.unwrap(); - if write_encoding_format == EncodingFormat::Legacy { - unreachable!() - } else { - assert!(result2.is_some()); - assert!(value2 == *result2.unwrap()); + let result2 = runtime.annotation(&key2).await.unwrap(); + if write_encoding_format == EncodingFormat::Legacy { + unreachable!() + } else { + assert!(result2.is_some()); + assert!(value2 == *result2.unwrap()); + } } } @@ -206,63 +210,65 @@ async fn test_storage_runtime_with_nested_annotation( write_encoding_format: EncodingFormat, write_digest_strategy: DigestStrategy, ) { - let mut config = Config::default(); - config.storage.encoding_format = write_encoding_format; - config.storage.digest_strategy = write_digest_strategy; - config.make_current().unwrap(); - - // Setup the objects needed for the runtime used in the test - let root = tmpdir.path().to_string_lossy().to_string(); - let repo = crate::storage::RepositoryHandle::from( - crate::storage::fs::MaybeOpenFsRepository::create(root) + reset_config_async! { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + + // Setup the objects needed for the runtime used in the test + let root = tmpdir.path().to_string_lossy().to_string(); + let repo = crate::storage::RepositoryHandle::from( + crate::storage::fs::MaybeOpenFsRepository::create(root) + .await + .unwrap(), + ); + + // make an annotation layer + let key = "some_field"; + let value = "somevalue"; + let annotation_value = AnnotationValue::string(value); + let layer = Layer::new_with_annotation(key, annotation_value); + match repo.write_object(&layer).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; + + // make a platform that contains the annotation layer + let layers: Vec = vec![layer.digest().unwrap()]; + let platform = Platform::from_iter(layers); + repo.write_object(&platform.clone()).await.unwrap(); + + // put the platform into a runtime + let storage = Storage::new(repo).unwrap(); + let keep_runtime = false; + let live_layers = Vec::new(); + let mut runtime = storage + .create_named_runtime("test-with-annotation-nested", keep_runtime, live_layers) .await - .unwrap(), - ); - - // make an annotation layer - let key = "some_field"; - let value = "somevalue"; - let annotation_value = AnnotationValue::string(value); - let layer = Layer::new_with_annotation(key, annotation_value); - match repo.write_object(&layer).await { - Ok(_) if write_encoding_format == EncodingFormat::Legacy => { - panic!("Writing annotations should fail when using EncodingFormat::Legacy") - } - Ok(_) => {} - Err(_) if write_encoding_format == EncodingFormat::Legacy => { - // This error is expected - return; - } - Err(e) => { - panic!("Error adding annotations: {e}") - } - }; + .expect("failed to create runtime in storage"); + runtime.push_digest(platform.digest().unwrap()); - // make a platform that contains the annotation layer - let layers: Vec = vec![layer.digest().unwrap()]; - let platform = Platform::from_iter(layers); - repo.write_object(&platform.clone()).await.unwrap(); + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); + }; - // put the platform into a runtime - let storage = Storage::new(repo).unwrap(); - let keep_runtime = false; - let live_layers = Vec::new(); - let mut runtime = storage - .create_named_runtime("test-with-annotation-nested", keep_runtime, live_layers) - .await - .expect("failed to create runtime in storage"); - runtime.push_digest(platform.digest().unwrap()); - - if write_encoding_format == EncodingFormat::Legacy { - unreachable!(); - }; - - // Test - retrieve the data even though it is nested inside a - // platform object in the runtime. - let result = runtime.annotation(key).await.unwrap(); - assert!(result.is_some()); + // Test - retrieve the data even though it is nested inside a + // platform object in the runtime. + let result = runtime.annotation(key).await.unwrap(); + assert!(result.is_some()); - assert!(value == result.unwrap()); + assert!(value == result.unwrap()); + } } #[rstest( @@ -276,64 +282,66 @@ async fn test_storage_runtime_with_annotation_all( write_encoding_format: EncodingFormat, write_digest_strategy: DigestStrategy, ) { - let mut config = Config::default(); - config.storage.encoding_format = write_encoding_format; - config.storage.digest_strategy = write_digest_strategy; - config.make_current().unwrap(); - - let root = tmpdir.path().to_string_lossy().to_string(); - let repo = crate::storage::RepositoryHandle::from( - crate::storage::fs::MaybeOpenFsRepository::create(root) + reset_config_async! { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + + let root = tmpdir.path().to_string_lossy().to_string(); + let repo = crate::storage::RepositoryHandle::from( + crate::storage::fs::MaybeOpenFsRepository::create(root) + .await + .unwrap(), + ); + let storage = Storage::new(repo).unwrap(); + let limit: usize = 16 * 1024; + + let keep_runtime = false; + let live_layers = Vec::new(); + let mut runtime = storage + .create_named_runtime("test-with-annotation-all", keep_runtime, live_layers) .await - .unwrap(), - ); - let storage = Storage::new(repo).unwrap(); - let limit: usize = 16 * 1024; - - let keep_runtime = false; - let live_layers = Vec::new(); - let mut runtime = storage - .create_named_runtime("test-with-annotation-all", keep_runtime, live_layers) - .await - .expect("failed to create runtime in storage"); - - // Test - insert two distinct data values - let key = "some_field"; - let value = "somevalue"; - - match runtime.add_annotation(key, value, limit).await { - Ok(_) if write_encoding_format == EncodingFormat::Legacy => { - panic!("Writing annotations should fail when using EncodingFormat::Legacy") - } - Ok(_) => {} - Err(_) if write_encoding_format == EncodingFormat::Legacy => { - // This error is expected - return; - } - Err(e) => { - panic!("Error adding annotations: {e}") - } - }; - - let key2 = "some_field2"; - let value2 = "somevalue2"; - assert!(runtime.add_annotation(key2, value2, limit).await.is_ok()); - - // Test - get all the data back out - if write_encoding_format == EncodingFormat::Legacy { - unreachable!(); - }; + .expect("failed to create runtime in storage"); - let result = runtime.all_annotations().await.unwrap(); + // Test - insert two distinct data values + let key = "some_field"; + let value = "somevalue"; - assert!(result.len() == 2); - for (expected_key, expected_value) in [(key, value), (key2, value2)].iter() { - assert!(result.contains_key(*expected_key)); - match result.get(*expected_key) { - Some(v) => { - assert!(v == expected_value); + match runtime.add_annotation(key, value, limit).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; + + let key2 = "some_field2"; + let value2 = "somevalue2"; + assert!(runtime.add_annotation(key2, value2, limit).await.is_ok()); + + // Test - get all the data back out + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); + }; + + let result = runtime.all_annotations().await.unwrap(); + + assert!(result.len() == 2); + for (expected_key, expected_value) in [(key, value), (key2, value2)].iter() { + assert!(result.contains_key(*expected_key)); + match result.get(*expected_key) { + Some(v) => { + assert!(v == expected_value); + } + None => panic!("Value missing for {expected_key} when getting all annotation"), } - None => panic!("Value missing for {expected_key} when getting all annotation"), } } } @@ -349,81 +357,83 @@ async fn test_storage_runtime_with_nested_annotation_all( write_encoding_format: EncodingFormat, write_digest_strategy: DigestStrategy, ) { - let mut config = Config::default(); - config.storage.encoding_format = write_encoding_format; - config.storage.digest_strategy = write_digest_strategy; - config.make_current().unwrap(); - - // setup the objects needed for the runtime used in the test - let root = tmpdir.path().to_string_lossy().to_string(); - let repo = crate::storage::RepositoryHandle::from( - crate::storage::fs::MaybeOpenFsRepository::create(root) + reset_config_async! { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + + // setup the objects needed for the runtime used in the test + let root = tmpdir.path().to_string_lossy().to_string(); + let repo = crate::storage::RepositoryHandle::from( + crate::storage::fs::MaybeOpenFsRepository::create(root) + .await + .unwrap(), + ); + + // make two distinct data values + let key = "some_field"; + let value = "somevalue"; + let annotation_value = AnnotationValue::string(value); + let layer = Layer::new_with_annotation(key, annotation_value); + match repo.write_object(&layer.clone()).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; + + let key2 = "some_field2"; + let value2 = "somevalue2"; + let annotation_value2 = AnnotationValue::string(value2); + let layer2 = Layer::new_with_annotation(key2, annotation_value2); + repo.write_object(&layer2.clone()).await.unwrap(); + + // make a platform with one annotation layer + let layers: Vec = vec![layer.digest().unwrap()]; + let platform = Platform::from_iter(layers); + repo.write_object(&platform.clone()).await.unwrap(); + + // make another platform with the first platform and the other + // annotation layer. this second platform is in the runtime + let layers2: Vec = vec![platform.digest().unwrap(), layer2.digest().unwrap()]; + let platform2 = Platform::from_iter(layers2); + repo.write_object(&platform2.clone()).await.unwrap(); + + // finally set up the runtime + let storage = Storage::new(repo).unwrap(); + + let keep_runtime = false; + let live_layers = Vec::new(); + let mut runtime = storage + .create_named_runtime("test-with-annotation-all-nested", keep_runtime, live_layers) .await - .unwrap(), - ); - - // make two distinct data values - let key = "some_field"; - let value = "somevalue"; - let annotation_value = AnnotationValue::string(value); - let layer = Layer::new_with_annotation(key, annotation_value); - match repo.write_object(&layer.clone()).await { - Ok(_) if write_encoding_format == EncodingFormat::Legacy => { - panic!("Writing annotations should fail when using EncodingFormat::Legacy") - } - Ok(_) => {} - Err(_) if write_encoding_format == EncodingFormat::Legacy => { - // This error is expected - return; - } - Err(e) => { - panic!("Error adding annotations: {e}") - } - }; - - let key2 = "some_field2"; - let value2 = "somevalue2"; - let annotation_value2 = AnnotationValue::string(value2); - let layer2 = Layer::new_with_annotation(key2, annotation_value2); - repo.write_object(&layer2.clone()).await.unwrap(); - - // make a platform with one annotation layer - let layers: Vec = vec![layer.digest().unwrap()]; - let platform = Platform::from_iter(layers); - repo.write_object(&platform.clone()).await.unwrap(); - - // make another platform with the first platform and the other - // annotation layer. this second platform is in the runtime - let layers2: Vec = vec![platform.digest().unwrap(), layer2.digest().unwrap()]; - let platform2 = Platform::from_iter(layers2); - repo.write_object(&platform2.clone()).await.unwrap(); - - // finally set up the runtime - let storage = Storage::new(repo).unwrap(); - - let keep_runtime = false; - let live_layers = Vec::new(); - let mut runtime = storage - .create_named_runtime("test-with-annotation-all-nested", keep_runtime, live_layers) - .await - .expect("failed to create runtime in storage"); - runtime.push_digest(platform2.digest().unwrap()); - - // Test - get all the data back out even thought it is nested at - // different levels in different platform objects in the runtime - if write_encoding_format == EncodingFormat::Legacy { - unreachable!(); - }; - - let result = runtime.all_annotations().await.unwrap(); - assert!(result.len() == 2); - for (expected_key, expected_value) in [(key, value), (key2, value2)].iter() { - assert!(result.contains_key(*expected_key)); - match result.get(*expected_key) { - Some(v) => { - assert!(v == expected_value); + .expect("failed to create runtime in storage"); + runtime.push_digest(platform2.digest().unwrap()); + + // Test - get all the data back out even thought it is nested at + // different levels in different platform objects in the runtime + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); + }; + + let result = runtime.all_annotations().await.unwrap(); + assert!(result.len() == 2); + for (expected_key, expected_value) in [(key, value), (key2, value2)].iter() { + assert!(result.contains_key(*expected_key)); + match result.get(*expected_key) { + Some(v) => { + assert!(v == expected_value); + } + None => panic!("Value missing for {expected_key} when getting all annotations"), } - None => panic!("Value missing for {expected_key} when getting all annotations"), } } } From 9bc8ee92ecc5aae59626fb9880148790e527c804 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Tue, 21 Oct 2025 13:16:19 -0700 Subject: [PATCH 2/2] Revert counting missing blobs as checked Contrary to dc38b371, the `checked_objects` field is documented with "the number of objects checked and found to be okay". Signed-off-by: J Robert Ray --- crates/spfs/src/check.rs | 1 - crates/spfs/src/check_test.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/spfs/src/check.rs b/crates/spfs/src/check.rs index 4db6c6f0b8..e329c48bb5 100644 --- a/crates/spfs/src/check.rs +++ b/crates/spfs/src/check.rs @@ -1030,7 +1030,6 @@ impl CheckBlobResult { match self { Self::Duplicate => CheckSummary::default(), Self::Missing(digest) => CheckSummary { - checked_objects: 1, missing_objects: Some(*digest).into_iter().collect(), ..Default::default() }, diff --git a/crates/spfs/src/check_test.rs b/crates/spfs/src/check_test.rs index 5bf70fb325..2f67d3ea3d 100644 --- a/crates/spfs/src/check_test.rs +++ b/crates/spfs/src/check_test.rs @@ -295,7 +295,7 @@ async fn check_missing_annotation_blob(#[future] tmprepo: TempRepo) { let summary: CheckSummary = results.iter().map(|r| r.summary()).sum(); tracing::info!("{summary:#?}"); - assert_eq!(summary.checked_objects, 2); + assert_eq!(summary.checked_objects, 1); assert_eq!(summary.checked_payloads, 0); assert!( summary.missing_objects.contains(&blob),