diff --git a/src/sync/engine.rs b/src/sync/engine.rs index 97985bf..3514c20 100644 --- a/src/sync/engine.rs +++ b/src/sync/engine.rs @@ -1556,6 +1556,23 @@ fn scan_into_node_observed( observed: &mut HashMap, cache: &mut HashMap, ) -> Result { + // A ROOT THAT IS NOT THERE IS NOT A FOLDER SOMEBODY EMPTIED. + // + // `scan_folder` returns an empty result for a missing root. Every tracked + // path would then be in the observed set and absent from the scan, which is + // the same shape as a local delete, so a delete-propagating policy would + // tombstone the entire entry and send that to every peer. + // + // A root vanishes for reasons that are not a deletion: an unmounted volume, + // a directory renamed while a pass runs, a mount that is not ready at boot. + // The blast radius is the whole entry rather than one path, so this is the + // one place where doing nothing is clearly right. Wait until we can see it. + // + // Deleting the CONTENTS of a folder still propagates normally, because the + // root survives that and the scan runs. + if !root.exists() { + return Ok(false); + } let scanned = scan_folder(root, entry, cache)?; // Refresh the cache from what this scan actually saw, so the next scan of an // untouched file is free. Rebuilt rather than merged, so a vanished path @@ -1625,10 +1642,33 @@ fn scan_into_node_observed( let now = now_secs(); for path in previous.keys() { - if !current.contains_key(path) && node.local_remove(path, policy, now) { + if current.contains_key(path) { + continue; + } + // A PATH THE ENTRY NO LONGER SELECTS HAS NOT BEEN DELETED. It has left + // this entry's scope, and the two are indistinguishable from here: both + // look like "in my records, absent from my scan". + // + // Treating the first as the second removed thirteen live files from + // three machines on 2026-08-25. `plans/**` was taken out of an entry's + // include, which left its paths recorded but unscannable, and the next + // release turned delete propagation on. Every one of those files was + // recoverable from git, which is the only reason it was recoverable. + // + // Narrowing an include is a scope change. Nobody deleted anything. + if !entry.includes(path) { + continue; + } + if node.local_remove(path, policy, now) { changed = true; } } + // NOT DONE HERE: dropping the excluded paths from the manifest outright. + // It is the tidier half of the rule and it needs its own change, because a + // peer whose config still selects the path would send it back on every + // reconcile and this side would drop it again, writing the whole index each + // time. Fixing a delete by inventing a write loop is not a fix. Filtering on + // adopt is the likelier answer and it changes what crosses the wire. *observed = current; Ok(changed) } @@ -3391,6 +3431,124 @@ mod tests { assert_eq!(paths, vec!["agent.toml".to_string()]); } + /// A WATCHED FOLDER THAT IS NOT THERE IS NOT A FOLDER SOMEBODY EMPTIED. + /// + /// `scan_folder` returns an EMPTY result when the root does not exist. Every + /// tracked path is then in the observed set and absent from the scan, which + /// is the same shape as a local delete, so a delete-propagating policy + /// tombstones the lot and sends that to every peer. + /// + /// The root can vanish for reasons that are not a deletion: an unmounted + /// volume, a directory renamed while a pass runs, a mount that is not ready + /// yet at boot. None of those mean the user deleted their files, and the + /// blast radius is the whole entry rather than one path. + /// + /// Prefer the annoying failure. A folder we cannot see is a folder we cannot + /// see, and the honest answer is to do nothing until we can. + #[test] + fn a_vanished_root_is_not_a_mass_delete() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("watched"); + std::fs::create_dir_all(&root).unwrap(); + for name in ["a.md", "b.md", "c.md"] { + std::fs::write(root.join(name), b"payload").unwrap(); + } + let entry = entry_with_policy("sync", &root, SyncPolicy::Bus); + let rules = entry.policy.rules(); + assert!( + rules.propagate_deletes, + "this test is about a policy that propagates deletes" + ); + + let mut node = SyncNode::new(Author([1; 32])); + let mut observed = HashMap::new(); + let mut cache = HashMap::new(); + scan_into_node_observed(&mut node, &root, &entry, rules, &mut observed, &mut cache) + .unwrap(); + assert_eq!( + node.manifest().present_paths().count(), + 3, + "the fixture never recorded the files, so losing them below proves nothing" + ); + + // The volume goes away. Nobody deleted anything. + std::fs::remove_dir_all(&root).unwrap(); + scan_into_node_observed(&mut node, &root, &entry, rules, &mut observed, &mut cache) + .unwrap(); + + assert_eq!( + node.manifest().present_paths().count(), + 3, + "a root that vanished was read as a delete of every file in the entry, \ + and that tombstone set goes to every peer" + ); + } + + /// Turning delete propagation ON must not delete a file that is simply + /// there. A delete pending for an unknown length of time is not a delete + /// anybody asked for today. + /// + /// HONESTY ABOUT THIS TEST: it passes before the fix as well as after, and I + /// am keeping it anyway. It was written to reproduce a proposed cause of the + /// 2026-08-25 file loss, that enabling propagation replayed a backlog of old + /// deletes. IT DOES NOT REPRODUCE, because that was not the cause. The cause + /// was a path removed from an entry's include, which left it recorded but + /// unscannable, and `a_path_dropped_from_include_is_forgotten_not_deleted` + /// in tests/folder_sync.rs is the test that DOES reproduce it. + /// + /// A test that passes before and after is still worth having: it proves the + /// switch itself is not the dangerous part, so nobody has to wonder again. + #[test] + fn enabling_delete_propagation_does_not_delete_what_is_still_there() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write(root.join("a.md"), b"alpha").unwrap(); + std::fs::write(root.join("b.md"), b"beta").unwrap(); + let entry = entry_with_policy("sync", root, SyncPolicy::Catalog); + let quiet = PolicyRules { + propagate_deletes: false, + sweep_tombstones: false, + }; + let loud = PolicyRules { + propagate_deletes: true, + sweep_tombstones: false, + }; + + let mut node = SyncNode::new(Author([1; 32])); + let mut observed = HashMap::new(); + let mut cache = HashMap::new(); + scan_into_node_observed(&mut node, root, &entry, quiet, &mut observed, &mut cache) + .unwrap(); + assert_eq!( + node.manifest().present_paths().count(), + 2, + "the fixture never recorded both files, so the switch below proves nothing" + ); + + // The switch. Same disk, same records, nobody deleted anything. + scan_into_node_observed(&mut node, root, &entry, loud, &mut observed, &mut cache) + .unwrap(); + let protected = observed.clone(); + materialize_tracked( + &mut node, + root, + loud, + &protected, + &mut observed, + &cache, + None, + ) + .unwrap(); + + assert!(root.join("a.md").exists(), "enabling propagation deleted a.md"); + assert!(root.join("b.md").exists(), "enabling propagation deleted b.md"); + assert_eq!( + node.manifest().present_paths().count(), + 2, + "enabling propagation tombstoned a file nobody deleted" + ); + } + /// The bookkeeping files are rewritten WHOLE every time they are written, /// and they are fabric's own index rather than anyone's data. Indenting /// them for a reader who does not exist cost 62% of every byte. diff --git a/tests/folder_sync.rs b/tests/folder_sync.rs index 27beeb9..a436a1e 100644 --- a/tests/folder_sync.rs +++ b/tests/folder_sync.rs @@ -336,3 +336,61 @@ async fn catalog_delete_while_peer_away_does_not_resurrect_on_return() -> Result node_a.shutdown().await?; Ok(()) } + +/// THE INCIDENT OF 2026-08-25, AS A TEST. +/// +/// Removing a path from an entry's `include` leaves whatever the manifest +/// already recorded for it. Under a policy that does not propagate deletes that +/// is inert. Under one that does, the entry sees a path that is Present in its +/// manifest and absent from its scan, which is exactly what a local delete looks +/// like, so it tombstones the path and DELETES THE REAL FILE. +/// +/// That is not hypothetical. It removed thirteen live plan files from three +/// machines. Every one was recoverable from git, which is the only reason it was +/// a bad night rather than a disaster. +/// +/// A path that leaves an entry's include set must be FORGOTTEN, not deleted. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_path_dropped_from_include_is_forgotten_not_deleted() -> Result<()> { + let _guard = FOLDER_SYNC_LOCK.lock().await; + let a_dir = TempDir::new()?; + let a_home = FabricHome::new(a_dir.path()); + let a_folder = a_dir.path().join("shared"); + std::fs::create_dir_all(a_folder.join("plans"))?; + std::fs::create_dir_all(a_folder.join("keep"))?; + write_sync_with_include(a_dir.path(), &a_folder, "catalog", "**"); + + let node_a = FabricNode::start(a_home.clone()).await?; + + let doomed = a_folder.join("plans/live-work.md"); + let kept = a_folder.join("keep/other.md"); + std::fs::write(&doomed, b"work someone is going to ship")?; + std::fs::write(&kept, b"still included")?; + reload_sync(&a_home).await?; + assert!( + wait_for_file(&doomed, b"work someone is going to ship").await, + "POSITIVE CONTROL FAILED: the entry never recorded the file, so dropping \ + it from the include afterwards would prove nothing" + ); + + // Now narrow the include so plans/ is no longer this entry's business. The + // file itself is untouched on disk and nobody asked for it to be deleted. + write_sync_with_include(a_dir.path(), &a_folder, "catalog", "keep/**"); + reload_sync(&a_home).await?; + + assert_stays_missing(&a_folder.join("never-existed.md"), "sanity").await; + node_a.shutdown().await?; + + assert!( + doomed.exists(), + "DROPPING A PATH FROM THE INCLUDE DELETED THE FILE. A path that leaves an \ + entry is not a file anybody deleted." + ); + assert_eq!( + std::fs::read(&doomed)?, + b"work someone is going to ship", + "the excluded file survived but its content changed" + ); + assert!(kept.exists(), "an included file was lost too"); + Ok(()) +}