diff --git a/crates/b2-cli/src/main.rs b/crates/b2-cli/src/main.rs index c215e86..13776d8 100644 --- a/crates/b2-cli/src/main.rs +++ b/crates/b2-cli/src/main.rs @@ -1671,6 +1671,10 @@ fn user_message(err: &CliError) -> String { CliError::Core(b2_core::Error::MoveDestination(_)) => { "That move destination isn't valid. Give a vault-relative path like `notes/new-name.md`.".to_string() } + CliError::Core(b2_core::Error::MoveIncomplete { paths, .. }) => format!( + "The move failed, and B2 could not undo its link changes in: {}. Check the links in those files; `b2 reindex` then shows any that no longer resolve.", + paths.join(", ") + ), CliError::Core(b2_core::Error::DirNotFound(p)) => format!( "Folder not found: '{p}'. Check the path (folders are vault-relative, like `notes/archive`)." ), @@ -1791,6 +1795,19 @@ mod tests { ); } + #[test] + fn a_move_that_could_not_be_undone_names_the_files_to_check() { + let msg = user_message(&CliError::Core(b2_core::Error::MoveIncomplete { + paths: vec!["b.md".into(), "notes/c.md".into()], + source: Box::new(b2_core::Error::Io(std::io::Error::other("disk full"))), + })); + assert!(msg.contains("in: b.md, notes/c.md."), "{msg}"); + assert!( + !msg.contains("disk full"), + "the cause stays internal: {msg}" + ); + } + fn view(vouched: Option, n: usize) -> SearchEvidenceView { SearchEvidenceView { results: (0..n) diff --git a/crates/b2-core/src/error.rs b/crates/b2-core/src/error.rs index 2e9e02c..059b116 100644 --- a/crates/b2-core/src/error.rs +++ b/crates/b2-core/src/error.rs @@ -64,6 +64,17 @@ pub enum Error { #[error("move target already exists: {0}")] MoveTargetExists(String), + /// A move failed part-way and B2 could not put back every file it had already + /// changed (GH #230). Rare: an ordinary failure is rolled back and surfaces as + /// its own error. Carries the vault-relative paths still holding the move's + /// rewrite, so the user can check those links by hand, and the failure that + /// started the undo, so the adapters' internal log keeps the root cause. + #[error("move failed and could not be fully undone: {} (cause: {source})", paths.join(", "))] + MoveIncomplete { + paths: Vec, + source: Box, + }, + /// `b2 mv` was given a source folder that doesn't exist in the vault — the /// directory sibling of [`Error::NoteNotFound`] (a folder is a path prefix, /// never an indexed row, so it resolves against the filesystem). diff --git a/crates/b2-core/src/mv.rs b/crates/b2-core/src/mv.rs index 110f26d..f3511c5 100644 --- a/crates/b2-core/src/mv.rs +++ b/crates/b2-core/src/mv.rs @@ -12,9 +12,22 @@ //! they belong to the chunk text, which a move does not change. //! //! It is **Markdown-first**: rewrite the inbound text, move the file, *then* re-project -//! from the now-current Markdown, so a crash mid-move leaves the Markdown correct and a -//! `b2 reindex` recovers. And bounded, not a scan: [`db::inbound_edge_targets`] names -//! exactly the files to touch, so the cost is O(inbound links). +//! from the now-current Markdown. And bounded, not a scan: [`db::inbound_edge_targets`] +//! names exactly the files to touch, so the cost is O(inbound links). +//! +//! **The vault half is all or nothing** (GH #230). Reindexing can't repair rewritten +//! link text (it projects whatever the Markdown says), so a failed move must leave every +//! file as it was. Three layers: the destination is checked before anything is written +//! ([`refuse_occupied`], [`refuse_file_ancestor`]); every rewrite is read and computed +//! in memory before the first write ([`plan_inbound`]); and the writes, the folders the +//! move creates and the rename run as one [`commit`] that undoes whatever it did if a +//! later step fails. Once the rename lands the vault is final: a failure re-keying or +//! re-projecting the index leaves correct Markdown, which `b2 reindex` projects. +//! +//! No process can undo a crash, so the one unguarded window is between the first +//! rewrite and the rename: the rewritten links already name the destination, the note +//! is still at its source. Re-running the same move finishes it (the rewrites are +//! already done, so only the rename is left). use crate::db; use crate::error::{Error, Result}; @@ -23,7 +36,8 @@ use rusqlite::Connection; use serde::Serialize; use std::collections::{BTreeMap, BTreeSet}; use std::fs; -use std::path::Path; +use std::io::Write; +use std::path::{Path, PathBuf}; /// What [`move_note`] did: the note's old and new vault-relative paths, the inbound /// files whose link text was rewritten, and the total number of `[[…]]` targets @@ -55,6 +69,7 @@ pub fn move_note(ctx: EmbedCtx, old_rel: &str, new_rel_input: &str) -> Result Result = by_file.keys().cloned().collect(); // 1. Markdown first: rewrite inbound link text in place — the `[[…]]` form, which - // is the one a note move repairs (the `[…](…)` pass is the resource move's). - // A self-link (the moved note links to itself) is rewritten here at its old - // path, before the move. - let (rewrote, links_rewritten) = rewrite_inbound(root, &by_file, &ByFile::new())?; - - // 2. Move the file on disk (creating any missing parent directories). - rename_with_parents(&old_abs, &new_abs)?; + // is the one a note move repairs (the `[…](…)` pass is the resource move's) — + // then move the file on disk, all or nothing. A self-link (the moved note links + // to itself) is rewritten at its old path, before the rename. + let plan = plan_inbound(root, &by_file, &ByFile::new())?; + commit(root, &plan.rewrites, &old_abs, &new_abs)?; + let (rewrote, links_rewritten) = (plan.rewrote(), plan.links_rewritten); - // 3. Re-key the index before anything re-projects, so path-based link resolution + // 2. Re-key the index before anything re-projects, so path-based link resolution // is independent of re-projection order (the same reason full ingest is // two-phase). One statement; the FK cascades carry the note's chunks, // aliases, centroid and outbound edges with it. db::repoint_note_path(conn, old_rel, &new_rel)?; - // 4. Re-project from the now-current Markdown: the moved note (refreshing its + // 3. Re-project from the now-current Markdown: the moved note (refreshing its // filename-derived title and mtime), then every inbound source so its edges // re-resolve at the new path. ingest::ingest_file(ctx, &new_rel)?; @@ -150,6 +164,7 @@ pub fn move_resource( let old_abs = root.join(old_rel); let new_abs = root.join(&new_rel); refuse_occupied(&old_abs, &new_abs, &new_rel)?; + refuse_file_ancestor(root, &new_rel)?; // The graph names the bounded inbound set; each authored target is rewritten in // its own convention, fragment intact ([`resource_replacement`]). @@ -165,19 +180,18 @@ pub fn move_resource( // 1. Markdown first: rewrite inbound link text in place, both syntaxes — a // resource is linked as `![[img.png]]` *or* `![](img.png)`, so the one map - // feeds both passes. - let (rewrote, links_rewritten) = rewrite_inbound(root, &by_file, &by_file)?; + // feeds both passes — then move the file on disk, all or nothing. + let plan = plan_inbound(root, &by_file, &by_file)?; + commit(root, &plan.rewrites, &old_abs, &new_abs)?; + let (rewrote, links_rewritten) = (plan.rewrote(), plan.links_rewritten); - // 2. Move the file on disk (creating any missing parent directories). - rename_with_parents(&old_abs, &new_abs)?; - - // 3. Update the inventory: same bytes at a new path (the hash is untouched; + // 2. Update the inventory: same bytes at a new path (the hash is untouched; // class re-derives from the new extension), then drop the old row — its // inbound edges re-dangle (ON DELETE SET NULL) until the re-projection // below re-resolves them at the new path. repoint_resource_row(conn, old_rel, &new_rel, &new_abs)?; - // 4. Re-project the rewritten notes from the now-current Markdown (their + // 3. Re-project the rewritten notes from the now-current Markdown (their // changed chunks re-embed inline, exactly like a note move's inbound set). for src_path in &rewrote { ingest::ingest_file(ctx, src_path)?; @@ -234,38 +248,170 @@ fn refuse_occupied(old_abs: &Path, new_abs: &Path, new_rel: &str) -> Result<()> Ok(()) } -/// Move one directory entry on disk, creating any missing destination parents -/// first — the note, resource, and whole-folder moves all rename exactly once. -fn rename_with_parents(old_abs: &Path, new_abs: &Path) -> Result<()> { - if let Some(parent) = new_abs.parent() { - fs::create_dir_all(parent)?; +/// Refuse a destination beneath a regular file (`blocked/a.md` when `blocked` is a +/// file): no rename can land there, and finding out *after* the inbound rewrites is +/// exactly the broken-links failure GH #230 reported. Walks the destination's +/// ancestors from the vault root down and stops at the first missing one — the +/// move creates everything below it. +fn refuse_file_ancestor(vault_root: &Path, new_rel: &str) -> Result<()> { + for (i, _) in new_rel.match_indices('/') { + let dir = &new_rel[..i]; + match fs::metadata(vault_root.join(dir)) { + Ok(meta) if !meta.is_dir() => { + return Err(Error::MoveDestination(format!( + "{dir} is a file, not a folder" + ))) + } + Ok(_) => {} + Err(_) => break, + } } - fs::rename(old_abs, new_abs)?; Ok(()) } -/// Markdown first, for every move: rewrite the inbound files' authored link text in -/// place. `wiki` holds each file's `[[…]]` replacements and `md` its `[…](…)` ones; a -/// file whose passes change nothing is left untouched *and* unreported, which is how a -/// relative link between two co-moved files stays a no-op. Returns the rewritten paths -/// (sorted, deduped) and the total targets replaced. -fn rewrite_inbound(vault_root: &Path, wiki: &ByFile, md: &ByFile) -> Result<(Vec, usize)> { +/// One inbound file's planned rewrite: the bytes it holds now and the bytes it will +/// hold. `original` is kept so [`commit`] can put the file back. +#[derive(Debug)] +struct Rewrite { + rel: String, + abs: PathBuf, + original: String, + rewritten: String, +} + +/// Every inbound rewrite a move will make, computed before the first write. +#[derive(Debug)] +struct Plan { + /// Sorted by path; only files whose passes change something. + rewrites: Vec, + links_rewritten: usize, +} + +impl Plan { + /// The rewritten files' vault-relative paths, sorted — every report's `rewrote`. + fn rewrote(&self) -> Vec { + self.rewrites.iter().map(|r| r.rel.clone()).collect() + } +} + +/// Markdown first, for every move: read each inbound file and compute its rewritten +/// link text, writing nothing. `wiki` holds each file's `[[…]]` replacements and `md` +/// its `[…](…)` ones; a file whose passes change nothing is left out of the plan, which +/// is how a relative link between two co-moved files stays a no-op. A file that can't +/// be read fails the move here, while the vault is still untouched. +fn plan_inbound(vault_root: &Path, wiki: &ByFile, md: &ByFile) -> Result { let none = BTreeMap::new(); - let mut rewrote = Vec::new(); + let mut rewrites = Vec::new(); let mut links_rewritten = 0usize; let touched: BTreeSet<&str> = wiki.keys().chain(md.keys()).map(String::as_str).collect(); for src_path in touched { let abs = vault_root.join(src_path); - let raw = fs::read_to_string(&abs)?; - let (pass1, n1) = rewrite_links(&raw, wiki.get(src_path).unwrap_or(&none)); - let (pass2, n2) = rewrite_md_targets(&pass1, md.get(src_path).unwrap_or(&none)); + let original = fs::read_to_string(&abs)?; + let (pass1, n1) = rewrite_links(&original, wiki.get(src_path).unwrap_or(&none)); + let (rewritten, n2) = rewrite_md_targets(&pass1, md.get(src_path).unwrap_or(&none)); if n1 + n2 > 0 { - fs::write(&abs, pass2)?; - rewrote.push(src_path.to_string()); + rewrites.push(Rewrite { + rel: src_path.to_string(), + abs, + original, + rewritten, + }); links_rewritten += n1 + n2; } } - Ok((rewrote, links_rewritten)) + Ok(Plan { + rewrites, + links_rewritten, + }) +} + +/// One vault change [`commit`] made, and so must undo if a later step fails. +#[derive(Debug)] +enum Done { + /// `rewrites[i]` was opened for writing (and so truncated). + Wrote(usize), + /// A destination folder that did not exist before the move. + CreatedDir(PathBuf), +} + +/// Apply a move's vault writes as one unit: write every planned rewrite, create any +/// missing destination folders, then rename `old_abs` → `new_abs` (the one step that +/// moves the note, resource or folder). If any step fails, undo the earlier ones in +/// reverse and return the failure; the rename is last, so once it succeeds there is +/// nothing left to undo. A filesystem has no transaction, so an undo step can fail +/// too — then [`Error::MoveIncomplete`] names the files still holding a rewrite, and +/// carries the failure that started the undo, rather than pretending the vault is whole. +fn commit(vault_root: &Path, rewrites: &[Rewrite], old_abs: &Path, new_abs: &Path) -> Result<()> { + let mut done = Vec::new(); + let Err(err) = apply(rewrites, old_abs, new_abs, &mut done) else { + return Ok(()); + }; + let unrestored = undo(vault_root, rewrites, &done); + if unrestored.is_empty() { + return Err(err); + } + Err(Error::MoveIncomplete { + paths: unrestored, + source: Box::new(err), + }) +} + +/// [`commit`]'s forward half, recording each change in `done` as it happens. +fn apply(rewrites: &[Rewrite], old_abs: &Path, new_abs: &Path, done: &mut Vec) -> Result<()> { + for (i, r) in rewrites.iter().enumerate() { + // Open (which truncates) before recording: a file that won't open is untouched. + let mut file = fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(&r.abs)?; + done.push(Done::Wrote(i)); + file.write_all(r.rewritten.as_bytes())?; + } + // The missing ancestors, outermost first, each created and recorded on its own so + // the undo removes exactly the folders this move made. + let mut missing: Vec<&Path> = new_abs + .ancestors() + .skip(1) + .take_while(|dir| !dir.exists()) + .collect(); + missing.reverse(); + for dir in missing { + fs::create_dir(dir)?; + done.push(Done::CreatedDir(dir.to_path_buf())); + } + fs::rename(old_abs, new_abs)?; + Ok(()) +} + +/// [`commit`]'s undo: reverse `done`, restoring each rewritten file's original bytes +/// and removing each folder the move created. Returns the vault-relative paths of the +/// files that could not be restored. A folder that won't go is only logged: it is +/// empty, so no authored content is lost with it. +fn undo(vault_root: &Path, rewrites: &[Rewrite], done: &[Done]) -> Vec { + let mut unrestored = Vec::new(); + for step in done.iter().rev() { + match step { + Done::Wrote(i) => { + let Some(r) = rewrites.get(*i) else { continue }; + if fs::write(&r.abs, r.original.as_bytes()).is_err() { + unrestored.push(r.rel.clone()); + } + } + Done::CreatedDir(dir) => { + if let Err(e) = fs::remove_dir(dir) { + let rel = dir.strip_prefix(vault_root).unwrap_or(dir); + tracing::warn!( + target: "b2::mv", + folder = %rel.display(), + error = %e, + "could not remove a folder created by a failed move" + ); + } + } + } + } + unrestored.sort(); + unrestored } /// The replacement for a wikilink at a note now living at `new_path`, preserving @@ -408,6 +554,7 @@ pub fn move_dir(ctx: EmbedCtx, from_input: &str, to_input: &str) -> Result Result Result Rewrite { + let abs = root.join(rel); + Rewrite { + rel: rel.to_string(), + original: fs::read_to_string(&abs).unwrap_or_default(), + abs, + rewritten: rewritten.to_string(), + } + } + + #[test] + fn a_write_failing_after_another_landed_restores_the_first() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + fs::write(root.join("a.md"), "See [[old]].\n").unwrap(); + fs::write(root.join("old.md"), "Target.\n").unwrap(); + // `b.md` is a folder, so opening it for writing fails — after `a.md` is written. + fs::create_dir(root.join("b.md")).unwrap(); + let rewrites = vec![ + planned(root, "a.md", "See [[new]].\n"), + planned(root, "b.md", "unused"), + ]; + + let mut done = Vec::new(); + let err = apply( + &rewrites, + &root.join("old.md"), + &root.join("new.md"), + &mut done, + ) + .unwrap_err(); + assert!(matches!(err, Error::Io(_)), "{err:?}"); + assert_eq!( + fs::read_to_string(root.join("a.md")).unwrap(), + "See [[new]].\n", + "the first rewrite landed before the failure" + ); + + assert!(undo(root, &rewrites, &done).is_empty()); + assert_eq!( + fs::read_to_string(root.join("a.md")).unwrap(), + "See [[old]].\n" + ); + assert!(root.join("old.md").exists() && !root.join("new.md").exists()); + } + + #[test] + fn a_failed_rename_undoes_every_write_and_created_folder() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + fs::write(root.join("a.md"), "See [[old]].\n").unwrap(); + fs::write(root.join("b.md"), "Also [[old|O]].\n").unwrap(); + let rewrites = vec![ + planned(root, "a.md", "See [[x/y/new]].\n"), + planned(root, "b.md", "Also [[x/y/new|O]].\n"), + ]; + + // No `old.md` on disk: both writes and both folders land, then the rename fails. + let err = commit( + root, + &rewrites, + &root.join("old.md"), + &root.join("x/y/new.md"), + ) + .unwrap_err(); + assert!(matches!(err, Error::Io(_)), "{err:?}"); + assert_eq!( + fs::read_to_string(root.join("a.md")).unwrap(), + "See [[old]].\n" + ); + assert_eq!( + fs::read_to_string(root.join("b.md")).unwrap(), + "Also [[old|O]].\n" + ); + assert!(!root.join("x").exists(), "the created folders are removed"); + } + + #[test] + fn an_undo_that_cannot_restore_a_file_names_it() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + // A "rewritten" path that is now a folder: writing its original back fails. + fs::create_dir(root.join("gone.md")).unwrap(); + let rewrites = vec![Rewrite { + rel: "gone.md".to_string(), + abs: root.join("gone.md"), + original: "See [[old]].\n".to_string(), + rewritten: "See [[new]].\n".to_string(), + }]; + assert_eq!( + undo(root, &rewrites, &[Done::Wrote(0)]), + vec!["gone.md".to_string()] + ); + } + + #[test] + fn a_file_ancestor_is_refused_and_a_missing_one_is_not() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + fs::create_dir(root.join("dir")).unwrap(); + fs::write(root.join("dir/file"), "x").unwrap(); + assert!(matches!( + refuse_file_ancestor(root, "dir/file/a.md"), + Err(Error::MoveDestination(m)) if m == "dir/file is a file, not a folder" + )); + assert!(refuse_file_ancestor(root, "dir/new/deeper/a.md").is_ok()); + assert!(refuse_file_ancestor(root, "a.md").is_ok()); + } + #[test] fn text_with_no_matching_link_is_returned_verbatim() { let t = targets(&[("concepts/memory", "concepts/human-memory")]); diff --git a/crates/b2-core/tests/mv.rs b/crates/b2-core/tests/mv.rs index 2e2b56e..5f6b7de 100644 --- a/crates/b2-core/tests/mv.rs +++ b/crates/b2-core/tests/mv.rs @@ -562,3 +562,239 @@ fn note_vectors(conn: &rusqlite::Connection, note_path: &str) -> Vec> { let rows = stmt.query_map([note_path], |r| r.get(0)).unwrap(); rows.map(Result::unwrap).collect() } + +// --- a failed move changes nothing (GH #230) ------------------------------------ +// +// A move writes the vault in two kinds of step: the inbound files' link text, then the +// rename. A move that fails must leave every file byte-identical — reindexing cannot +// repair rewritten link text, because it faithfully projects whatever the Markdown now +// says. Two routes to a failure: a destination refused up front, and an I/O failure +// after the rewrites have been written, which must be rolled back. + +/// A purpose-built vault under `dir/vault` from `(path, contents)` pairs, reindexed. +fn small_vault(dir: &Path, files: &[(&str, &str)]) -> (Vault, std::path::PathBuf) { + let root = dir.join("vault"); + for (path, contents) in files { + let abs = root.join(path); + fs::create_dir_all(abs.parent().unwrap()).unwrap(); + fs::write(abs, contents).unwrap(); + } + let vault = Vault::open(&root).unwrap(); + vault.reindex().unwrap(); + (vault, root) +} + +/// Every file under `root` (outside `.b2/`) with its bytes, sorted by path — the +/// whole authored vault, so an assertion on it catches a stray write anywhere. +fn vault_bytes(root: &Path) -> Vec<(String, Vec)> { + fn walk(root: &Path, dir: &Path, out: &mut Vec<(String, Vec)>) { + for entry in fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + let rel = path + .strip_prefix(root) + .unwrap() + .to_string_lossy() + .into_owned(); + if rel == ".b2" { + continue; + } + if path.is_dir() { + out.push((format!("{rel}/"), Vec::new())); + walk(root, &path, out); + } else { + out.push((rel, fs::read(&path).unwrap())); + } + } + } + let mut out = Vec::new(); + walk(root, root, &mut out); + out.sort(); + out +} + +#[test] +fn a_move_under_a_file_is_refused_and_changes_nothing() { + // The issue's reproduction: `blocked` is a regular file, so `blocked/a.md` can + // never exist. The move must be refused before a single inbound link is touched. + let tmp = tempfile::TempDir::new().unwrap(); + let (vault, root) = small_vault( + tmp.path(), + &[ + ("a.md", "Target note.\n"), + ("b.md", "See [[a]].\n"), + ("c.md", "Also [[ a | the target ]] and [[a.md]].\n"), + ("blocked", "a plain file\n"), + ], + ); + let before = vault_bytes(&root); + let backlinks = inbound(&vault, "a.md"); + assert_eq!( + backlinks, + vec![ + ("referenced-by".to_string(), "b.md".to_string()), + ("referenced-by".to_string(), "c.md".to_string()), + ("referenced-by".to_string(), "c.md".to_string()), + ], + "one per authored link: c.md links twice" + ); + + let err = vault.move_note("a.md", "blocked/a.md").unwrap_err(); + assert!(matches!(err, Error::MoveDestination(_)), "{err:?}"); + + assert_eq!(vault_bytes(&root), before, "every file byte-identical"); + assert_eq!(inbound(&vault, "a.md"), backlinks, "the index is untouched"); + // And the Markdown still says what it said: a rebuild resolves the same links. + vault.reindex().unwrap(); + assert_eq!( + inbound(&vault, "a.md"), + backlinks, + "the original links resolve" + ); +} + +#[test] +fn a_move_deeper_under_a_file_is_refused_too() { + // The blocking file may be any ancestor, not just the immediate parent. + let tmp = tempfile::TempDir::new().unwrap(); + let (vault, root) = small_vault( + tmp.path(), + &[ + ("a.md", "Target note.\n"), + ("b.md", "See [[a]].\n"), + ("blocked", "a plain file\n"), + ], + ); + let before = vault_bytes(&root); + + let err = vault.move_note("a.md", "blocked/deeper/a.md").unwrap_err(); + assert!(matches!(err, Error::MoveDestination(_)), "{err:?}"); + assert_eq!(vault_bytes(&root), before); +} + +#[test] +fn a_move_whose_rename_fails_restores_every_rewritten_file() { + // A failure *after* the rewrites: the note vanished from disk (an out-of-band + // delete the index hasn't seen), so the destination checks pass, both inbound + // files are rewritten, and only then does the rename fail. The rewrites and the + // destination folder the move created must all be undone. + let tmp = tempfile::TempDir::new().unwrap(); + let (vault, root) = small_vault( + tmp.path(), + &[ + ("a.md", "Target note.\n"), + ("b.md", "See [[a]].\n"), + ("c.md", "Also [[ a | the target ]] and [[a.md]].\n"), + ], + ); + let backlinks = inbound(&vault, "a.md"); + fs::remove_file(root.join("a.md")).unwrap(); + let before = vault_bytes(&root); + + let err = vault.move_note("a.md", "archive/2026/a.md").unwrap_err(); + assert!(matches!(err, Error::Io(_)), "{err:?}"); + + assert_eq!( + vault_bytes(&root), + before, + "inbound files restored, no archive/ folder left behind" + ); + assert_eq!(inbound(&vault, "a.md"), backlinks, "the index is untouched"); +} + +#[test] +fn a_resource_move_under_a_file_is_refused_and_changes_nothing() { + let tmp = tempfile::TempDir::new().unwrap(); + let (vault, root) = small_vault( + tmp.path(), + &[ + ("img.png", "png bytes"), + ("n.md", "![[img.png]] and ![alt](img.png)\n"), + ("blocked", "a plain file\n"), + ], + ); + let before = vault_bytes(&root); + + let err = vault + .move_resource("img.png", "blocked/img.png") + .unwrap_err(); + assert!(matches!(err, Error::MoveDestination(_)), "{err:?}"); + assert_eq!(vault_bytes(&root), before); +} + +#[test] +fn a_resource_move_whose_rename_fails_restores_every_rewritten_note() { + // The resource arm of the rollback: both link syntaxes are rewritten, then the + // rename fails because the file is gone from disk. + let tmp = tempfile::TempDir::new().unwrap(); + let (vault, root) = small_vault( + tmp.path(), + &[ + ("img.png", "png bytes"), + ("n.md", "![[img.png|caption]] and ![alt]( img.png )\n"), + ], + ); + let backlinks = vault.explain_resource("img.png").unwrap().backlinks; + assert_eq!(backlinks.len(), 2); + fs::remove_file(root.join("img.png")).unwrap(); + let before = vault_bytes(&root); + + let err = vault.move_resource("img.png", "media/img.png").unwrap_err(); + assert!(matches!(err, Error::Io(_)), "{err:?}"); + + assert_eq!( + vault_bytes(&root), + before, + "n.md restored, no media/ folder" + ); + assert_eq!( + vault.explain_resource("img.png").unwrap().backlinks, + backlinks, + "the index is untouched" + ); +} + +#[test] +fn move_dir_under_a_file_is_refused_and_changes_nothing() { + let tmp = tempfile::TempDir::new().unwrap(); + let (vault, root) = small_vault( + tmp.path(), + &[ + ("docs/x.md", "Inside, see [[docs/y]].\n"), + ("docs/y.md", "Also inside.\n"), + ("hub.md", "See [[docs/x|X]].\n"), + ("blocked", "a plain file\n"), + ], + ); + let before = vault_bytes(&root); + + let err = vault.move_dir("docs", "blocked/docs").unwrap_err(); + assert!(matches!(err, Error::MoveDestination(_)), "{err:?}"); + assert_eq!(vault_bytes(&root), before); +} + +#[test] +fn re_running_a_move_interrupted_before_its_rename_finishes_it() { + // The one window no undo covers is a crash between the rewrites and the rename: + // the inbound links already name the destination, the note is still at its source, + // and the index hasn't heard. Simulated by writing that state by hand; re-running + // the same move must complete it rather than refuse or double-rewrite. + let tmp = tempfile::TempDir::new().unwrap(); + let (vault, root) = small_vault( + tmp.path(), + &[("a.md", "Target note.\n"), ("b.md", "See [[a|A]].\n")], + ); + fs::write(root.join("b.md"), "See [[archive/a|A]].\n").unwrap(); + + let report = vault.move_note("a.md", "archive/a.md").unwrap(); + assert_eq!(report.links_rewritten, 0, "the rewrite was already done"); + assert!(root.join("archive/a.md").exists() && !root.join("a.md").exists()); + assert_eq!( + fs::read_to_string(root.join("b.md")).unwrap(), + "See [[archive/a|A]].\n" + ); + assert_eq!( + inbound(&vault, "archive/a.md"), + vec![("referenced-by".to_string(), "b.md".to_string())], + "the link resolves at the destination" + ); +} diff --git a/crates/b2-desktop/src/error.rs b/crates/b2-desktop/src/error.rs index daa94eb..9397ffa 100644 --- a/crates/b2-desktop/src/error.rs +++ b/crates/b2-desktop/src/error.rs @@ -103,6 +103,10 @@ pub fn user_message(err: &CmdError) -> String { "That destination isn't valid. Give a vault-relative name like `notes/new-name`." .to_string() } + CmdError::Core(b2_core::Error::MoveIncomplete { paths, .. }) => format!( + "The move failed, and B2 could not undo its link changes in: {}. Check the links in those files, then reindex.", + paths.join(", ") + ), CmdError::Core(b2_core::Error::DirNotFound(p)) => { format!("Folder not found: '{p}'. It may have been moved — reindex and try again.") } @@ -283,5 +287,17 @@ mod tests { let dir = CmdError::Core(b2_core::Error::DirNotFound("old-folder".into())); assert!(user_message(&dir).contains("Folder not found: 'old-folder'")); + + let incomplete = CmdError::Core(b2_core::Error::MoveIncomplete { + paths: vec!["b.md".into(), "notes/c.md".into()], + source: Box::new(b2_core::Error::Io(std::io::Error::other("disk full"))), + }); + let msg = user_message(&incomplete); + assert!(msg.contains("in: b.md, notes/c.md.")); + assert!(!msg.contains("disk full"), "the cause stays internal"); + assert!( + incomplete.to_string().contains("disk full"), + "the internal log (`log_internal`) keeps the cause" + ); } } diff --git a/docs/data-model.md b/docs/data-model.md index 117af54..a67afa9 100644 --- a/docs/data-model.md +++ b/docs/data-model.md @@ -280,9 +280,10 @@ Every edge projects to one record. This is the shape the [index-engine.md](index `[[path]]` is normalized by the resolver (the wikilink `+ ".md"` ladder) and stored as the path it named. `dst_path_raw` keeps the text exactly as written. So an edge stores what you authored, and the graph carries no identifier the vault does not. A B2-performed move is - therefore one transaction: rewrite the inbound `[[path|title]]` text, re-key the moved - note's rows, re-project the inbound sources. That is how L1's "rename keeps every backlink - resolving" holds. A move made outside B2 is a delete plus a create, and its inbound links + therefore one operation: rewrite the inbound `[[path|title]]` text and rename the file + (all or nothing on the vault), then re-key the moved note's rows and re-project the + inbound sources ([index-engine.md](index-engine.md) §8). That is how L1's "rename keeps + every backlink resolving" holds. A move made outside B2 is a delete plus a create, and its inbound links project as surfaced dangling edges (G5). - **The edge set is the union of the two homes, deduped** (G2). A frontmatter entry with a *different* verb than a body link to the same target is no duplicate; that is the augment diff --git a/docs/index-engine.md b/docs/index-engine.md index 37e43ce..3c6b73d 100644 --- a/docs/index-engine.md +++ b/docs/index-engine.md @@ -96,8 +96,8 @@ Why this shape fits B2: key; `chunks.note_path`, `note_aliases.note_path`, `note_centroids.note_path`, and `edges.src_path` are `REFERENCES notes(path) ON DELETE CASCADE ON UPDATE CASCADE`. That makes a B2-performed move a path re-key rather than a rebuild: `UPDATE notes SET path = …` - cascades through every child in one transaction, alongside the inbound link-text rewrite - and a re-projection of the inbound sources. `edges.dst_path` is deliberately *not* a + cascades through every child in one statement, after the inbound link-text rewrite and + before a re-projection of the inbound sources. `edges.dst_path` is deliberately *not* a foreign key: it must be allowed to be NULL, the dangling case (G5). "Rename keeps every backlink resolving" is therefore a property of the move operation, not of the key; the price is that a move made outside B2 is a delete plus a create (§8). @@ -589,10 +589,27 @@ be budgeted, tested, and watched. inbound link text in every file that points at it: an N-file write. It is bounded and mechanical (the materialized edges name exactly which files and links to touch, Markdown first, then the index), but moving a heavily linked note is proportional to its backlink - count, not O(1). The rewrite is transactional, so a partial move never half-updates the - vault. The index side is one cascading `UPDATE` plus re-projection of the inbound sources, - bounded by the same count. Only the exact files the graph names are touched: a - prefix-sharing `[[foo-bar]]` is never rewritten when moving `foo`. + count, not O(1). The index side is one cascading `UPDATE` plus re-projection of the + inbound sources, bounded by the same count. Only the exact files the graph names are + touched: a prefix-sharing `[[foo-bar]]` is never rewritten when moving `foo`. +- **A failed move leaves the vault as it was.** A SQLite transaction can't roll back file + writes, and reindexing can't repair rewritten link text (it projects whatever the + Markdown now says), so the vault half carries its own all-or-nothing contract (GH #230): + - *Refused before any write:* a destination that exists, or that sits beneath a regular + file. + - *Planned before any write:* every inbound file is read and its rewrite computed in + memory, so an unreadable file stops the move with nothing changed. + - *Undone on failure:* the rewrites, any destination folders the move creates, and the + rename run as one unit. If a step fails, the earlier ones are undone in reverse and + the original error is returned, with every file byte-identical. If an undo step + itself fails, the move reports the files still holding a rewrite + (`Error::MoveIncomplete`) rather than claiming a clean vault. + - *After the rename, the vault is final.* A failure re-keying or re-projecting the index + leaves correct Markdown and a stale index; `b2 reindex` projects it. + - *A crash can't be undone.* The one unguarded window is between the first rewrite and + the rename: the rewritten links name the destination while the note is still at its + source, and they surface as dangling (G5). Re-running the same move finishes it, + because the rewrites are already done and only the rename is left. - **Out-of-band moves are identified, not repaired, and that is the scope decision.** A `git mv` or Finder move is, to a path-keyed index, a delete plus a create: the old path's rows prune, the new path projects fresh, and every inbound `[[oldpath]]` becomes a surfaced diff --git a/docs/invariants.md b/docs/invariants.md index 10e0381..470f5ab 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -94,9 +94,10 @@ drops in without a redesign. link homes are already written by path: a body `[[path]]`, or a frontmatter `b2_relations:` entry. So an edge stores what you authored, resolved at projection time, with no machine id in the file. Consequence: a rename keeps every backlink resolving *when B2 does the move*. A - B2 move rewrites the inbound path text and re-keys the moved note's rows in one transaction. - A move made outside B2 is a delete plus a create: the inbound links surface as dangling - (G5), identified rather than silently dropped. + B2 move rewrites the inbound path text and re-keys the moved note's rows as one operation, + and a move that fails leaves every file as it was (GH #230). A move made outside B2 is a + delete plus a create: the inbound links surface as dangling (G5), identified rather than + silently dropped. ([data-model.md](data-model.md) §1, §3, [GH #170](https://github.com/AlteredCraft/B2/issues/170)) - **L2. A note's title is its filename.** The frontmatter `title:` key is recognized but