From cfb3356e91a7d16e32aee065530a6563ab4e4964 Mon Sep 17 00:00:00 2001 From: matrixarkai Date: Wed, 28 Oct 2026 00:09:37 -0700 Subject: [PATCH] Validate library guards with default and consensus features --- .github/workflows/ci.yml | 29 +++++++-- CONTRIBUTING.md | 20 +++--- README.md | 7 ++- src/tests.rs | 127 ++++++++++++++++++++++++++++++--------- tools/known_failures.txt | 17 ------ tools/suite_gate.sh | 14 ++--- 6 files changed, 140 insertions(+), 74 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 27e9d285..00b73938 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -781,11 +781,8 @@ jobs: grep -q '^test result: ok. 1 passed' /tmp/guard.log \ || { echo "::error::an_object_record_owns_no_heap_before_or_after_a_snapshot_load did not run"; exit 1; } - # THESE TWO GUARD WHAT A FOLD AND A BATCHED READ COULD DROP, and both were added - # because a plant against them left the whole suite at its recorded baseline. Each - # counter helper has exactly one production caller, in a prepared batch mutation's - # apply half, and every test driving those paths is in tools/known_failures.txt -- so - # without these steps the only cover for that accounting is a failing test. + # Direct checks keep both folded counter terms observable alongside + # the prepared-batch tests that exercise their production callers. - name: The folded replica counters advance every term they fold run: | set -o pipefail @@ -1355,6 +1352,28 @@ jobs: grep -q '^running 1 test$' /tmp/repair-cursor.log grep -q '^test result: ok. 1 passed' /tmp/repair-cursor.log + - name: every guard the workflow pins by name exists + run: | + set -euo pipefail + cargo test --locked --lib tests::every_guard_the_workflow_pins_by_name_exists -- --exact | tee /tmp/library-guard.log + grep -q '^running 1 test$' /tmp/library-guard.log + grep -q '^test result: ok. 1 passed' /tmp/library-guard.log + cargo test --locked --all-features --lib tests::every_guard_the_workflow_pins_by_name_exists -- --exact | tee /tmp/library-guard.log + grep -q '^running 1 test$' /tmp/library-guard.log + grep -q '^test result: ok. 1 passed' /tmp/library-guard.log + - name: rust distributed readiness fences checksums + run: | + set -euo pipefail + cargo test --locked --lib tests::rust_distributed_readiness_fences_checksums -- --exact | tee /tmp/library-guard.log + grep -q '^running 1 test$' /tmp/library-guard.log + grep -q '^test result: ok. 1 passed' /tmp/library-guard.log + - name: chunk service batches coalesce reads and discards + run: | + set -euo pipefail + cargo test --locked --lib tests::chunk_service_batches_coalesce_reads_and_discards -- --exact | tee /tmp/library-guard.log + grep -q '^running 1 test$' /tmp/library-guard.log + grep -q '^test result: ok. 1 passed' /tmp/library-guard.log + - name: capacity rebalance reserves destination space across the batch run: | set -euo pipefail diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1a9d8f07..60ce85cb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,20 +27,16 @@ RUSTFLAGS="-D warnings" cargo build --all-targets --locked RUSTFLAGS="-D warnings" cargo build --all-targets --locked --all-features ``` -**17 tests fail on a clean checkout**, and they are recorded by -name in [`tools/known_failures.txt`](tools/known_failures.txt). That is why a bare -`cargo test` looks alarming on a clean checkout. Use the gate, which compares the -failures against that baseline in both directions — a new failure fails it, and so does a -recorded failure that has stopped happening: +**There are no recorded library failures.** Both the default-feature and all-feature +library configurations pass their enabled tests; ignored tests remain separate work. +The gate compares failures against [`tools/known_failures.txt`](tools/known_failures.txt) +in both directions, rejecting new failures and stale recorded failures: ```bash tools/suite_gate.sh ``` -Two things about the suite that will otherwise cost you an afternoon: - -- **One test does not return.** Skip it: - `cargo test --lib -- --skip round_backlog_cost::a_round_with_an_empty_backlog_still_costs_the_whole_store`. +When running individual tests: - **`cargo test ` exits 0 when the filter matches nothing.** If you run one test by name, check the output says `test result: ok. 1 passed` rather than trusting the exit code. `--exact` needs the full path, `tests::the_name`. @@ -55,7 +51,7 @@ Four jobs, all on a pinned toolchain: | job | what it does | |---|---| | **build & docs** | `cargo build --all-targets --locked` and `--all-features`, then `cargo doc --no-deps --locked` and `--all-features`, all with warnings denied | -| **format and rollback guards** | 115 individual tests in the library plus 59 integration tests, each run by name with `--exact` | +| **format and rollback guards** | 118 individual tests in the library plus 59 integration tests, each run by name with `--exact` | The **network metadata consensus** job also runs forty-one named health, routing, preparation, and consensus tests with the optional consensus feature enabled. @@ -64,9 +60,9 @@ The **network chunk consensus** job runs twenty-nine identity, routing, process, capacity, shutdown, and metadata-health tests with the chunk consensus feature. The coupled quorum test additionally enables metadata consensus and starts three voters for each service. -Across these jobs, 135 individual tests run in the library. +Across these jobs, 138 individual tests run in the library. -**CI does not run the suite.** It compiles everything and runs those 135 guards plus the integration tests. A green +**CI does not run the suite.** It compiles everything and runs those 138 guards plus the integration tests. A green tick means the tree builds and those guards hold; it does not mean the suite passed. Run `tools/suite_gate.sh` yourself. diff --git a/README.md b/README.md index d36fac79..9b80f854 100644 --- a/README.md +++ b/README.md @@ -804,9 +804,10 @@ On pushes to `main` and on every pull request, four jobs: See [chunk consensus configuration](docs/chunk-consensus.md). CI does not run the whole library suite. `tools/suite_gate.sh` does, locally: it -compares the failures by name against the 17 recorded in -`tools/known_failures.txt`, and fails in **both** directions -- a failure that is -new, and a recorded one that has started passing and should come off the list. +compares failures by name against `tools/known_failures.txt` and fails in **both** +directions: a new failure, or a recorded failure that has started passing. +There are no recorded library failures. Ignored tests are not covered by this result; +run optional consensus coverage separately with `cargo test --all-features --lib`. ## Licence diff --git a/src/tests.rs b/src/tests.rs index 0d4d1319..5822a009 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -4514,8 +4514,9 @@ fn chunk_service_batches_coalesce_reads_and_discards() { assert_eq!(batch_stats.read_stats.physical_chunk_read_count, 4); assert_eq!(batch_stats.read_stats.metadata_head_batch_requests, 2); assert_eq!(batch_stats.read_stats.metadata_physical_head_count, 4); - assert_eq!(batch_stats.read_stats.metadata_coalesced_head_count, 2); - assert_eq!(batch_stats.read_stats.metadata_cache_hits, 2); + // Page IDs were already deduplicated before the replica metadata lookup. + assert_eq!(batch_stats.read_stats.metadata_coalesced_head_count, 0); + assert_eq!(batch_stats.read_stats.metadata_cache_hits, 0); assert_eq!(batch_stats.read_stats.metadata_cache_misses, 4); assert_eq!(batch_stats.read_stats.quorum_verified_chunks, 2); assert!(matches!( @@ -11649,7 +11650,7 @@ fn rust_distributed_readiness_fences_checksums() { .get("distributed_readiness_checksum"), Some(&readiness.control_checksum) ); - assert_eq!(readiness.control_checksum_format, "MOSRSOBJECTREADY68"); + assert_eq!(readiness.control_checksum_format, "MOSRSOBJECTREADY77"); assert_eq!( readiness .to_control_fields("") @@ -11678,6 +11679,21 @@ fn rust_distributed_readiness_fences_checksums() { ), Err(ObjectError::PreconditionFailed(_)) )); + assert!(matches!( + verify_objectstore_distributed_readiness_checksum( + &readiness, + None, + Some("unsupported-format"), + None + ), + Err(ObjectError::PreconditionFailed(_)) + )); + let mut damaged = readiness.clone(); + damaged.control_checksum = "fnv1a64:0000000000000000".to_string(); + assert!(matches!( + verify_objectstore_distributed_readiness_checksum(&damaged, None, None, None), + Err(ObjectError::Corruption(_)) + )); } #[test] @@ -46323,7 +46339,7 @@ fn a_storage_class_is_one_byte_and_its_wire_is_still_a_name() { #[test] fn every_guard_the_workflow_pins_by_name_exists() { - // CI runs thirty-odd guards as their own steps, each naming one test with `--exact`. + // CI runs named guards as separate steps, each selecting one test with `--exact`. // `cargo test ` EXITS 0 WHEN THE FILTER MATCHES NOTHING, so every step greps its // own output for `test result: ok. 1 passed` and fails with "the guard did not run" // otherwise. That is what caught two renames in this change -- but it caught them in CI, @@ -46340,14 +46356,48 @@ fn every_guard_the_workflow_pins_by_name_exists() { .expect("the test harness should list its tests"); assert!(output.status.success(), "listing compiled tests failed"); let listing = String::from_utf8(output.stdout).unwrap(); - let compiled: Vec<&str> = listing.lines().filter_map(|line| line.strip_suffix(": test")).collect(); - assert!(compiled.len() >= 25, "the compiled test list must not be empty"); + let compiled: Vec<&str> = listing + .lines() + .filter_map(|line| line.strip_suffix(": test")) + .collect(); + assert!( + compiled.len() >= 25, + "the compiled test list must not be empty" + ); // `tests::` is how the workflow spells a filter. Taken from the workflow rather // than from a list kept here, so a step ADDED later is covered without anyone // remembering to add it twice. let mut pinned: Vec<&str> = Vec::new(); + // Validate every available test in this harness. Optional tests absent from + // this build are checked by the all-feature invocation in CI. + let mut requested_features_enabled = true; + let mut unavailable = BTreeSet::new(); for line in WORKFLOW.lines() { + if line.trim_start().starts_with("cargo test ") { + requested_features_enabled = true; + } + let mut words = line.split_whitespace(); + while let Some(word) = words.next() { + if word == "--all-features" { + requested_features_enabled &= cfg!(all( + feature = "matrix-raft", + feature = "metadata-consensus", + feature = "chunk-consensus" + )); + } + if word == "--features" { + for feature in words.next().expect("features need a value").split(',') { + let enabled = match feature { + "matrix-raft" => cfg!(feature = "matrix-raft"), + "metadata-consensus" => cfg!(feature = "metadata-consensus"), + "chunk-consensus" => cfg!(feature = "chunk-consensus"), + _ => panic!("workflow guard must account for feature {feature}"), + }; + requested_features_enabled &= enabled; + } + } + } let Some(at) = line.find("tests::") else { continue; }; @@ -46356,7 +46406,15 @@ fn every_guard_the_workflow_pins_by_name_exists() { .next() .unwrap_or(""); if !name.is_empty() { - pinned.push(name); + if requested_features_enabled + || compiled + .iter() + .any(|test| test.ends_with(&format!("::{name}"))) + { + pinned.push(name); + } else { + unavailable.insert(name); + } } } pinned.sort_unstable(); @@ -46371,7 +46429,11 @@ fn every_guard_the_workflow_pins_by_name_exists() { let missing: Vec<&str> = pinned .iter() .copied() - .filter(|name| !compiled.iter().any(|test| test.ends_with(&format!("::{name}")))) + .filter(|name| { + !compiled + .iter() + .any(|test| test.ends_with(&format!("::{name}"))) + }) .collect(); assert!( missing.is_empty(), @@ -46379,7 +46441,20 @@ fn every_guard_the_workflow_pins_by_name_exists() { rename them in the workflow too, or the step will pass nothing and report that it \ ran no guard" ); - println!("WORKFLOWPINS {} pinned guards, all present", pinned.len()); + if cfg!(all( + feature = "metadata-consensus", + feature = "chunk-consensus" + )) { + assert!( + unavailable.is_empty(), + "all-feature validation must check every guard" + ); + } + println!( + "WORKFLOWPINS {} enabled guards checked; {} need optional features", + pinned.len(), + unavailable.len() + ); // AND THE SPELLING THE STEPS CHECK FOR IS THE ONE CARGO PRINTS. If that line ever // changed, every step would fail open -- the grep would never match and the step would @@ -46387,7 +46462,10 @@ fn every_guard_the_workflow_pins_by_name_exists() { // the steps rely on a single test passing, so none of them may name a filter that // matches more than one test. for name in &pinned { - let matches = compiled.iter().filter(|test| test.ends_with(&format!("::{name}"))).count(); + let matches = compiled + .iter() + .filter(|test| test.ends_with(&format!("::{name}"))) + .count(); assert_eq!( matches, 1, "a pinned guard must name exactly one test, and {name} names {matches}" @@ -46669,19 +46747,18 @@ fn the_readme_describes_the_continuous_integration_it_has() { .lines() .filter(|line| !line.trim().is_empty() && !line.trim_start().starts_with('#')) .count(); - assert!( - recorded > 0, - "the baseline should list failures, and it parsed as empty" - ); - assert!( - README.contains(&format!("the {recorded} recorded in")), - "the README should say {recorded} failures are recorded, which is what \ - tools/known_failures.txt lists" - ); + if recorded == 0 { + assert!(README.contains("There are no recorded library failures.")); + } else { + assert!( + README.contains(&format!("the {recorded} recorded in")), + "the README should state the number of recorded failures" + ); + } // 3. "CI does not run the whole library suite." Every `cargo test` in the workflow must - // carry a `tests::` filter -- an unfiltered one would run the suite, and 21 of its - // tests fail, so the step would be red on arrival. Continuations are joined first, + // carry a named filter while CI claims to run only selected guards. + // Continuations are joined first, // because these commands are written across lines with a trailing backslash. assert!( README.contains("CI does not run the whole library suite"), @@ -46801,12 +46878,8 @@ fn the_readme_describes_the_continuous_integration_it_has() { /// also creates one place where a term can be dropped for every call site at once, so it /// wants a guard. /// -/// It is tested here, on the helper, because of what the production callers are: each has -/// exactly ONE, the apply half of a prepared batch mutation, and every test that drives those -/// paths is in `tools/known_failures.txt`. Measured rather than assumed -- deleting the bytes -/// term from each helper and running the whole library suite left it at exactly its recorded -/// baseline, 412 passing and the same 21 failing, both times. So there was no green path to -/// hang this on, and a test of the path would have been a test of a path that already fails. +/// Direct checks keep each folded counter independently observable, alongside +/// the prepared-batch tests that exercise the production callers. #[test] fn the_folded_replica_counters_advance_every_term_they_fold() { let mut replica = MatrixReplicaService::new(); diff --git a/tools/known_failures.txt b/tools/known_failures.txt index 3c720a16..e69de29b 100644 --- a/tools/known_failures.txt +++ b/tools/known_failures.txt @@ -1,17 +0,0 @@ -tests::bucket_lifecycle_policies_filter_transition_and_expire_bounded -tests::bucket_lifecycle_transitions_only_preserve_expirable_versions -tests::replica_service_dedupes_state_install_and_reclaim_receipts_across_snapshots -tests::page_and_object_repair_large_ranges_with_cursors -tests::chunk_service_batches_coalesce_reads_and_discards -tests::raft_backed_store_fences_metaserver_control_prepares -tests::raft_backed_store_gates_prepared_chunk_control_configs -tests::raft_backed_store_gates_prepared_metaserver_control_configs -tests::raft_backed_store_gates_prepared_placement_actions -tests::raft_backed_store_proves_metaserver_shard_migration_plans -tests::raft_backed_store_replays_direct_page_batch_allocations -tests::raft_backed_store_replays_lifecycle_policy_application -tests::raft_backed_store_replays_lifecycle_transition_only_application -tests::raft_backed_store_replays_object_capacity_rebalance_report -tests::raft_backed_store_replays_placement_commands -tests::raft_backed_store_runs_integrity_fenced_metaserver_maintenance -tests::rust_distributed_readiness_fences_checksums diff --git a/tools/suite_gate.sh b/tools/suite_gate.sh index a864a461..7abe7cf3 100755 --- a/tools/suite_gate.sh +++ b/tools/suite_gate.sh @@ -7,22 +7,16 @@ # # tools/suite_gate.sh # -# 17 tests fail on a clean checkout. They are listed in tools/known_failures.txt, -# and this script is how you tell a pre-existing failure from one you just caused. -# -# It was 21 until one of them turned out to be a stale literal rather than a broken -# surface: a test asserted the root snapshot stamp MOSRSROOT30 while the format had -# moved to MOSRSROOT32, which `every_durable_format_constant_is_accounted_for` had -# been pinning correctly all along. A recorded failure is a record of a FAILURE, not -# of a reason, so it is worth reading one before assuming the code is at fault. +# The recorded baseline is empty after validating default and all-feature +# library builds. Keep comparing both directions if failures are ever recorded. set -u cd "$(dirname "$0")/.." || exit 2 BASELINE=tools/known_failures.txt LOG=$(mktemp) -trap 'rm -f "$LOG"' EXIT +trap 'rm -f "$LOG" "$LOG.now" "$LOG.base"' EXIT -cargo test --lib > "$LOG" 2>&1 +cargo test --locked --lib -- --test-threads=1 > "$LOG" 2>&1 # A suite that failed to COMPILE prints no "test result:" line and no FAILED # lines, which would otherwise read as "every recorded failure is fixed". This