From 7271755b454445da1486b39ce2a1a50d4f26d0b9 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Tue, 4 Aug 2026 02:19:20 +0200 Subject: [PATCH] sync: delete the unreachable mtime stamping and say why it is gone My own 62a30b8 removed the last call that stamped an origin mtime onto a materialized file. It left the machinery behind, and it left a doc comment arguing that stamping matters. Both are now wrong. write_atomic_with_mtime had exactly one caller, write_atomic, which always passed None. So the parameter was unreachable, and the comment explained the value of behaviour the code no longer had. That is the same defect as the replay-buffer text: a written claim that does not match the code, in a file I edited. The two functions collapse into one. write_atomic now carries the real reason: stamping made mtime a cross-node value, two contending entries of equal size could collide on size plus mtime, and the scan cache then reported content the file did not hold, which is the permanent three-node divergence. The comment also records the consequence, so the next reader does not have to rediscover it: fabric replicates no mtime at all today, so a consumer must not read a replica's mtime as an activity signal. Issue 27 is exactly that mistake. set_file_mtime is kept and marked test-only rather than deleted. It is not dead: the test that reconstructs the size-plus-mtime collision needs it to build the collision on purpose. Deleting it would have removed the proof that the divergence mechanism is understood. No behaviour change. 195 lib tests green. --- src/sync/engine.rs | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/sync/engine.rs b/src/sync/engine.rs index 60faa79..af0a69a 100644 --- a/src/sync/engine.rs +++ b/src/sync/engine.rs @@ -1409,35 +1409,41 @@ fn materialize_tracked( } /// Write bytes atomically: to a temp sibling, then rename over the target. -fn write_atomic(path: &Path, bytes: &[u8]) -> Result<()> { - write_atomic_with_mtime(path, bytes, None) -} - -/// Write `bytes` to `path` atomically, optionally stamping the file with the -/// modification time the manifest records for it. /// -/// Stamping matters for more than tidiness. The scan cache treats a file as -/// unchanged when its size and mtime match the recorded metadata, and a -/// materialized file carries the sender's mtime in the manifest. Without -/// stamping, the local filesystem assigns its own mtime at write time, so every -/// file this node ever received from a peer misses the cache forever and is -/// re-read and re-hashed on every scan. -fn write_atomic_with_mtime(path: &Path, bytes: &[u8], mtime: Option<(i64, u32)>) -> Result<()> { +/// A materialized file keeps the receiver's own write time. It is NOT stamped +/// with the sender's mtime, and that is deliberate. +/// +/// Stamping used to happen here, and it caused a permanent three-node +/// divergence. The scan cache treats a file as unchanged when its size and mtime +/// match the recorded metadata. Stamping made mtime a cross-node value, so two +/// contending entries of equal size could collide on size plus mtime, and the +/// cache then reported content the file did not actually hold. Versions +/// leapfrogged and never converged. Commit 62a30b8 removed the stamping; the +/// cost is one re-read per materialized file, which is the cheap side of that +/// trade. +/// +/// The consequence is that fabric does not replicate an mtime at all today, so a +/// consumer must not read a replica's mtime as an activity signal. See issue 27: +/// st2 derived agent liveness that way and reported live remote agents as +/// unknown. +fn write_atomic(path: &Path, bytes: &[u8]) -> Result<()> { let tmp = path.with_extension(format!( "{}.fabric-tmp", path.extension().and_then(|e| e.to_str()).unwrap_or("") )); std::fs::write(&tmp, bytes).with_context(|| format!("failed to write {}", tmp.display()))?; - if let Some((secs, nanos)) = mtime { - set_file_mtime(&tmp, secs, nanos) - .with_context(|| format!("failed to stamp mtime on {}", tmp.display()))?; - } std::fs::rename(&tmp, path) .with_context(|| format!("failed to rename into {}", path.display()))?; Ok(()) } /// Set a file's modification time, leaving its access time alone. +/// +/// Test-only since 62a30b8 removed mtime stamping from materialization. The +/// production path deliberately never sets an mtime, so the only caller left is +/// the test that reconstructs the size-plus-mtime collision that stamping used +/// to manufacture. +#[cfg(test)] fn set_file_mtime(path: &Path, secs: i64, nanos: u32) -> Result<()> { use std::os::unix::ffi::OsStrExt; let c_path = std::ffi::CString::new(path.as_os_str().as_bytes())