Skip to content

Revert counting missing blobs as checked #1283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Revert counting missing blobs as checked #1283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f6ef33
Another attempt to fix spurious test failures
jrray Oct 3, 2025
9bc8ee9
Revert counting missing blobs as checked
jrray Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/spfs/src/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
},
Expand Down
2 changes: 1 addition & 1 deletion crates/spfs/src/check_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
188 changes: 129 additions & 59 deletions crates/spfs/src/graph/layer_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -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(
Expand All @@ -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())
}
}
}
}
Loading
Loading