From f32c040ce185ba1ae9d93d8af08fdd4e6624caa7 Mon Sep 17 00:00:00 2001 From: Takashi Yamashina Date: Wed, 12 Aug 2026 07:54:56 +0900 Subject: [PATCH] fix: route move_context_files renames through the fault-injection choke point move_context_files used a bare fs::rename, so the crate's highest-risk multi-step FS operation (a ten-file family move) was the one thing fail_persistence_ops_after could never reach, even though storage.rs's own doc claimed every persistence operation was covered. Route it through rename_persisted_file like every other rename, correct the doc to say what is and isn't covered (fsync and reads are deliberately outside the net), and update the one existing test whose comment documented the bypass as a workaround. Refs #586 --- src/registry.rs | 4 +- src/registry/lifecycle.rs | 14 ++++-- src/registry/meta_io.rs | 96 ++++++++++++++++++++++++++++++++++++++- src/storage.rs | 17 +++++-- 4 files changed, 118 insertions(+), 13 deletions(-) diff --git a/src/registry.rs b/src/registry.rs index ceda2d3e..fb63a80f 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -63,8 +63,8 @@ use crate::schema; #[cfg(test)] use crate::storage::{clear_persistence_fault, fail_persistence_ops_after, write_atomic_private}; use crate::storage::{ - commit_staged, fsync_dir, lock_data_dir, offload, remove_persisted_file, stage_bytes, - write_atomic, + commit_staged, fsync_dir, lock_data_dir, offload, remove_persisted_file, rename_persisted_file, + stage_bytes, write_atomic, }; use crate::wal::{self, WalOp}; diff --git a/src/registry/lifecycle.rs b/src/registry/lifecycle.rs index 7b89c918..fadf19d9 100644 --- a/src/registry/lifecycle.rs +++ b/src/registry/lifecycle.rs @@ -2106,11 +2106,15 @@ mod tests { ) .unwrap(); - // The file move itself bypasses this injector (`move_context_files` - // uses `fs::rename` directly), so the very first persistence op - // this boot performs is the membership rewrite's own - // `write_group` — fail that one. - fail_persistence_ops_after(0); + // `move_context_files` now routes every one of `context_files`' + // ten renames through the same injector (#586) — it consults + // the injector once per family slot regardless of whether that + // slot's source file exists (a `NotFound` is only known AFTER + // the call), so the move alone spends exactly `context_files`' + // length worth of "successes" before the membership rewrite's + // own `write_group` gets to run. Let all ten land, then fail + // the very next persistence op — `write_group`'s own stage. + fail_persistence_ops_after(context_files(&file_stem("sake")).len() as u32); let state = AppState::boot(dir.clone(), usize::MAX, None).unwrap(); let past_end = clear_persistence_fault(); assert!(!past_end, "the group write itself must be what failed"); diff --git a/src/registry/meta_io.rs b/src/registry/meta_io.rs index e089c830..20ad16e7 100644 --- a/src/registry/meta_io.rs +++ b/src/registry/meta_io.rs @@ -206,7 +206,10 @@ pub(crate) fn context_files(stem: &str) -> [String; 10] { /// is returned so the caller knows the move is incomplete and keeps the /// rename marker. All ten share `data_dir` as their parent, so one /// fsync after every rename covers the whole family durably instead of -/// paying for it (via `commit_staged`) up to ten times. The fsync's own +/// paying for it (via `commit_staged`) up to ten times. Each rename +/// itself still goes through the shared [`rename_persisted_file`] choke +/// point — this crate's highest-risk multi-step FS operation is fault- +/// injectable exactly like a single-file publish is. The fsync's own /// failure is reported too, but only when there was no earlier /// straggler to report first — a rename error names the file that /// actually didn't move, which is more actionable than a directory @@ -223,7 +226,7 @@ pub(super) fn move_context_files( .zip(context_files(to_stem)) .enumerate() { - match fs::rename(data_dir.join(from_file), data_dir.join(to_file)) { + match rename_persisted_file(data_dir.join(from_file), data_dir.join(to_file)) { Ok(()) => moved_any = true, Err(error) if error.kind() == io::ErrorKind::NotFound => {} // The pivot: fail outright so nothing else moves. @@ -350,4 +353,93 @@ mod tests { ); let _ = fs::remove_dir_all(&dir); } + + /// #586: `move_context_files` now routes every one of its ten + /// renames through the same [`crate::storage::rename_persisted_file`] + /// choke point every other publish uses — the fault injector must + /// actually reach the pivot move, the highest-risk multi-step FS + /// operation in the crate. A failure there must behave exactly like + /// a real `fs::rename` failure on the pivot always has: nothing + /// else in the family moves. + #[test] + fn an_injected_pivot_rename_failure_moves_nothing() { + let dir = scratch_dir("meta-io-move-pivot-fault"); + fs::create_dir_all(&dir).unwrap(); + let from_stem = file_stem("sake"); + let to_stem = file_stem("shochu"); + fs::write(image_path(&dir, &from_stem), b"ctx bytes").unwrap(); + fs::write(meta_path(&dir, &from_stem), b"meta bytes").unwrap(); + + fail_persistence_ops_after(0); + let error = move_context_files(&dir, &from_stem, &to_stem).unwrap_err(); + let past_end = clear_persistence_fault(); + assert!( + !past_end, + "the very first rename (the pivot) must be what failed" + ); + assert_eq!( + error.kind(), + io::ErrorKind::Other, + "an injected failure, not a real ENOENT: {error:?}" + ); + + assert!( + image_path(&dir, &from_stem).exists(), + "the pivot must stay put" + ); + assert!( + meta_path(&dir, &from_stem).exists(), + "a post-pivot file must never be touched once the pivot itself failed" + ); + assert!(!image_path(&dir, &to_stem).exists()); + assert!(!meta_path(&dir, &to_stem).exists()); + + let _ = fs::remove_dir_all(&dir); + } + + /// The best-effort half of the same contract: an injected failure + /// on a post-pivot file (here `.meta.json`, `context_files`' + /// second entry) must behave like the existing real-FS-blocker + /// tests (`lifecycle.rs`'s straggler tests) already prove for a + /// genuine `fs::rename` error — the pivot has already moved, so + /// the loop keeps going rather than stopping, and a later file in + /// the family (here `sources_path`, the third entry) still lands + /// at `to_stem`. The first straggler error is what + /// `move_context_files` reports. + #[test] + fn an_injected_straggler_rename_failure_still_lets_later_files_move() { + let dir = scratch_dir("meta-io-move-straggler-fault"); + fs::create_dir_all(&dir).unwrap(); + let from_stem = file_stem("sake"); + let to_stem = file_stem("shochu"); + fs::write(image_path(&dir, &from_stem), b"ctx bytes").unwrap(); + fs::write(meta_path(&dir, &from_stem), b"meta bytes").unwrap(); + fs::write(sources_path(&dir, &from_stem), b"sources bytes").unwrap(); + + // One success (the pivot) then fail the very next op — the + // `.meta.json` rename. + fail_persistence_ops_after(1); + let error = move_context_files(&dir, &from_stem, &to_stem).unwrap_err(); + let past_end = clear_persistence_fault(); + assert!(!past_end, "the meta rename must be what failed"); + assert_eq!(error.kind(), io::ErrorKind::Other, "{error:?}"); + + assert!( + !image_path(&dir, &from_stem).exists(), + "the pivot already moved before the fault" + ); + assert!(image_path(&dir, &to_stem).exists()); + assert!( + meta_path(&dir, &from_stem).exists(), + "the failed rename must never have touched the source" + ); + assert!(!meta_path(&dir, &to_stem).exists()); + assert!( + !sources_path(&dir, &from_stem).exists() && sources_path(&dir, &to_stem).exists(), + "best-effort continues past a post-pivot straggler: a LATER \ + family member still lands at the destination" + ); + + let _ = fs::remove_dir_all(&dir); + } } diff --git a/src/storage.rs b/src/storage.rs index f4086998..9dd9e457 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -33,11 +33,20 @@ pub(crate) fn write_atomic_private(path: &Path, bytes: &[u8]) -> io::Result<()> /// Test-only deterministic fault injection for registry persistence. /// /// The calling thread fails exactly one persistence operation after -/// `successes` stage, commit, unlink, WAL append, or WAL truncate -/// operations have run normally. +/// `successes` stage, commit (a publish rename OR a same-directory +/// move — the registry's whole-family context rename included, since +/// it routes every one of its ten renames through +/// [`rename_persisted_file`] like every other rename in the crate), +/// unlink, WAL append, or WAL truncate operations have run normally. /// Keeping the counter thread-local makes parallel tests independent, -/// and routing every operation through shared choke points avoids a -/// flag for each call site. +/// and routing every MUTATING operation through shared choke points +/// avoids a flag for each call site. Two things are deliberately +/// outside this net: [`fsync_dir`] (durability, not mutation — a +/// dropped fsync loses nothing a crash-consistency test can observe +/// without also modeling real fsync failures, which this harness does +/// not attempt) and any plain read (`fs::read` — a read has no +/// write-ordering story to get wrong, so injecting into it would only +/// test the reader's own error handling, not persistence ordering). #[cfg(test)] pub(crate) fn fail_persistence_ops_after(successes: u32) { PERSISTENCE_FAULT.with(|cell| cell.set(Some(successes)));