test(storage): make the harnesses hold on CI's machines, not just this one

Three failures, each real rather than a runner being slow.

**A fixed delay decided whether the migration crash test tested anything.** On
the runner the child had copied nothing in its 150 ms; on this machine it had
copied some. It uses the same failpoint as the other children now: ten chunks
copied, the eleventh interrupted, the rest untouched, the same every time. The
progress handshake it used instead is gone with it.

**btrfs does not report freed space immediately.** The test slept 200 ms and
took one reading, which on ext4 was enough and on btrfs was not. It polls for
the space to come back, with a deadline, and returns the last real reading so
a genuine failure still fails on the number rather than on the wait.

**Two things only CI's toolchain sees.** Clippy 1.98 rejects an unbounded
range in a for loop where 1.95 did not, and making the file store public under
the test feature put a doc link to a private item in front of rustdoc for the
first time. Local checks now include `cargo doc --features test-utils`, which
is what would have caught the second one here.

Author: grumbach

src/storage/file_store.rs

@@ -1749,8 +1749,7 @@ fn decode_chunk_name(name: &str) -> Option<XorName> {
 /// # Errors
 ///
 /// Returns the underlying I/O error. Off Unix there is no way to flush a directory through
-/// the standard library, so this reports success without being able to promise anything;
-/// see [`fsync_dir`].
+/// the standard library, so this reports success without being able to promise anything.
 pub fn fsync_path(path: &Path) -> std::io::Result<()> {
     fsync_dir(path)
 }

tests/migration_crash_safety.rs

@@ -102,51 +102,6 @@ fn kill_child_at_failpoint(role: &str, root: &Path, let_through: u64) -> PathBuf
     marker
 }
 
-/// Run a child for a while and then kill it, without a failpoint.
-///
-/// For the cases where the point is that the kill lands somewhere in a long stretch of
-/// work rather than at one named instant. The child reports progress so this never kills
-/// one that has not started.
-fn kill_child_once_it_is_working(role: &str, root: &Path, run_for: Duration) {
-    let progress = root.join(format!("working-{role}"));
-    let _ = std::fs::remove_file(&progress);
-
-    let exe = std::env::current_exe().expect("this test binary");
-    let mut child = Command::new(exe)
-        .arg("--exact")
-        .arg(role)
-        .arg("--nocapture")
-        .arg("--ignored")
-        .env("ANT_CRASH_TEST_ROOT", root)
-        .env("ANT_CRASH_TEST_PROGRESS", &progress)
-        .stdout(Stdio::null())
-        .stderr(Stdio::null())
-        .spawn()
-        .expect("spawn the child");
-
-    let deadline = std::time::Instant::now() + Duration::from_secs(120);
-    while !progress.exists() {
-        if let Ok(Some(status)) = child.try_wait() {
-            panic!("the child exited before doing any work: {status}");
-        }
-        if std::time::Instant::now() > deadline {
-            let _ = child.kill();
-            panic!("the child never started working");
-        }
-        std::thread::sleep(Duration::from_millis(10));
-    }
-    std::thread::sleep(run_for);
-    child.kill().expect("kill the child");
-    let _ = child.wait();
-}
-
-/// Say that this child has started doing the work it was spawned for.
-fn report_working() {
-    if let Ok(path) = std::env::var("ANT_CRASH_TEST_PROGRESS") {
-        let _ = std::fs::write(path, b"working");
-    }
-}
-
 /// Where the child was told to work.
 fn child_root() -> PathBuf {
     PathBuf::from(std::env::var("ANT_CRASH_TEST_ROOT").expect("the child needs a root"))
@@ -165,14 +120,15 @@ async fn child_writes_until_killed() {
     .await
     .expect("open");
 
-    report_working();
     // Always a chunk it has not written before, so the kill lands in real work rather
     // than in a re-offer of something already on disk. An earlier version cycled the same
     // hundred keys and spent almost all its time confirming duplicates.
-    for n in 0.. {
+    let mut n = 0usize;
+    loop {
         let content = chunk_bytes(n);
         let address = ant_node::client::compute_address(&content);
         let _ = store.put(&address, &content).await;
+        n += 1;
     }
 }
 
@@ -202,11 +158,6 @@ async fn child_migrates_until_killed() {
             break;
         };
         let _ = store.copy_batch(&[*key], 0, 0, &shutdown).await;
-        // Said only after a copy has actually happened, so a copier that did nothing at
-        // all cannot be mistaken for one that was interrupted part-way.
-        if store.legacy_only_keys().len() < keys.len() {
-            report_working();
-        }
     }
     panic!("the child copied everything before it was killed, so nothing was interrupted");
 }
@@ -352,11 +303,11 @@ async fn a_killed_migration_still_has_every_chunk_somewhere() {
     std::fs::create_dir_all(&root).expect("mkdir");
     let keys = seed_legacy_from(&root, 0).await;
 
-    kill_child_once_it_is_working(
-        "child_migrates_until_killed",
-        &root,
-        Duration::from_millis(150),
-    );
+    // Ten chunks copied, the eleventh interrupted. An earlier version killed the child
+    // after a fixed delay, which on a fast runner meant it had copied everything and on a
+    // slow one meant it had copied nothing; both make this test say something other than
+    // what it claims.
+    kill_child_at_failpoint("child_migrates_until_killed", &root, 10);
 
     // Interrupted, which is two claims and not one: some chunks copied, and some not.
     // Only the upper bound was checked before, so a copier that did nothing at all passed

tests/migration_reclaims_disk.rs

@@ -253,8 +253,12 @@ async fn retiring_the_legacy_environment_returns_its_bytes_to_the_filesystem() {
     // still holds it open keeps its blocks and shows in no directory, so measuring before
     // this point would be measuring the wrong thing.
     drop(store);
-    tokio::time::sleep(std::time::Duration::from_millis(200)).await;
-    let free_at_end = free_space(&root);
+
+    // Polled rather than sampled once after a fixed pause. Not every filesystem updates
+    // its accounting the instant a file goes: btrfs in particular defers it, and a single
+    // reading taken too early says the space never came back when it is on its way. The
+    // deadline is what makes this a test rather than a wait.
+    let free_at_end = wait_for_space(&root, free_at_peak + environment_blocks / 2).await;
 
     // Only the file store's copy should be left.
     let left_on_disk = allocated_bytes(&root);
@@ -297,6 +301,22 @@ async fn retiring_the_legacy_environment_returns_its_bytes_to_the_filesystem() {
     }
 }
 
+/// Wait for the filesystem to report at least `wanted` bytes free, and return what it
+/// reports at the end.
+///
+/// Returns whatever it last saw when the deadline passes, so the caller's assertion is
+/// what fails rather than this helper, and the number in the failure is a real reading.
+async fn wait_for_space(path: &Path, wanted: u64) -> u64 {
+    let deadline = std::time::Instant::now() + std::time::Duration::from_secs(60);
+    loop {
+        let free = free_space(path);
+        if free >= wanted || std::time::Instant::now() > deadline {
+            return free;
+        }
+        tokio::time::sleep(std::time::Duration::from_millis(100)).await;
+    }
+}
+
 /// Open the store again from scratch, which is what a restart does.
 async fn store_reopened(root: &Path) -> ChunkStore {
     ChunkStore::new(ChunkStoreConfig {