Validate library guards with default and consensus features

.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

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 <filter>` 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.
 

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
 

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 <filter>` 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::<name>` 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,23 +46429,43 @@ 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(),
         "the workflow runs these by name and this tree has no such test: {missing:?} -- \
          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
     // always error, which is noisy rather than dangerous, but the reverse is worth pinning:
     // 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();

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

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