diff --git a/.sqlx/query-fa3d1e4a6cae94780daf8fe20062a107963ba6ab2bcef2c8cfb9a1efbd905b59.json b/.sqlx/query-bda8991590c009ca7084fed36fae088f1b6ae12ad95f972724a417711ad37fa1.json similarity index 51% rename from .sqlx/query-fa3d1e4a6cae94780daf8fe20062a107963ba6ab2bcef2c8cfb9a1efbd905b59.json rename to .sqlx/query-bda8991590c009ca7084fed36fae088f1b6ae12ad95f972724a417711ad37fa1.json index a757088..e31f5fb 100644 --- a/.sqlx/query-fa3d1e4a6cae94780daf8fe20062a107963ba6ab2bcef2c8cfb9a1efbd905b59.json +++ b/.sqlx/query-bda8991590c009ca7084fed36fae088f1b6ae12ad95f972724a417711ad37fa1.json @@ -1,10 +1,10 @@ { "db_name": "SQLite", - "query": "SELECT id FROM roots WHERE path = ? AND id <> ?", + "query": "SELECT id AS \"id!: i64\" FROM roots\n WHERE CASE WHEN rtrim(path, '/') = '' THEN '/' ELSE rtrim(path, '/') END = ?\n AND (? IS NULL OR id <> ?)", "describe": { "columns": [ { - "name": "id", + "name": "id!: i64", "ordinal": 0, "type_info": "Integer", "origin": { @@ -16,11 +16,11 @@ } ], "parameters": { - "Right": 2 + "Right": 3 }, "nullable": [ false ] }, - "hash": "fa3d1e4a6cae94780daf8fe20062a107963ba6ab2bcef2c8cfb9a1efbd905b59" + "hash": "bda8991590c009ca7084fed36fae088f1b6ae12ad95f972724a417711ad37fa1" } diff --git a/crates/arr-api/src/relocate.rs b/crates/arr-api/src/relocate.rs index 459b387..e88d843 100644 --- a/crates/arr-api/src/relocate.rs +++ b/crates/arr-api/src/relocate.rs @@ -50,11 +50,14 @@ struct PlannedRename { pub(crate) struct Relocation { performed: Vec, rewrites: Vec<(i64, String)>, - /// The new root directory, set only when this request created it (it did - /// not already exist). [`Self::undo`] removes it, so it is cleaned up - /// exactly when the move it was created for does not complete; a move - /// that commits never calls `undo` and the directory stays. - created_root: Option, + /// Every directory level this request materialised for the new root, + /// deepest first. `create_dir_all` can make more than one — moving a + /// root to `/mnt/media-v2/tv/kids` when `/mnt/media-v2` is all that + /// exists creates both `tv` and `kids` — and [`Self::undo`] removes all + /// of them, so a failed move leaves nothing behind (#244). Levels that + /// were already on disk are never in this list and are never touched. A + /// move that commits never calls `undo` and the directories stay. + created_dirs: Vec, } /// Whether the destination root is a directory that must already be there. @@ -179,20 +182,18 @@ async fn relocate_files( } } - // Tracked only when this call is the one that created the directory, so - // a failed move can remove it again without ever touching a root path - // that already existed on disk. - let mut created_root: Option = None; + // Tracked only for the levels this call is the one to create, so a + // failed move can remove them again without ever touching a directory + // that already existed on disk. Recorded before `create_dir_all`, since + // afterwards there is no way to tell which levels it made. + let mut created_dirs: Vec = Vec::new(); if destination == Destination::Create && !renames.is_empty() { - let already_there = tokio::fs::symlink_metadata(new_root).await.is_ok(); + created_dirs = missing_levels(std::path::Path::new(new_root)).await; if let Err(error) = tokio::fs::create_dir_all(new_root).await { return Err(ApiError::Filesystem(format!( "could not create '{new_root}': {error}" ))); } - if !already_there { - created_root = Some(PathBuf::from(new_root)); - } } let mut performed: Vec = Vec::new(); @@ -209,7 +210,7 @@ async fn relocate_files( ); continue; } - Err(error) => return Err(failed(&rename, &error, performed, created_root).await), + Err(error) => return Err(failed(&rename, &error, performed, created_dirs).await), } match tokio::fs::rename(&rename.source, &rename.destination).await { Ok(()) => { @@ -220,31 +221,47 @@ async fn relocate_files( ); performed.push(rename); } - Err(error) => return Err(failed(&rename, &error, performed, created_root).await), + Err(error) => return Err(failed(&rename, &error, performed, created_dirs).await), } } Ok(Relocation { performed, rewrites, - created_root, + created_dirs, }) } +/// The levels of `path` that are not on disk, deepest first — exactly what a +/// following `create_dir_all` will materialise. The walk stops at the first +/// ancestor that exists, so nothing already there is ever listed. +async fn missing_levels(path: &std::path::Path) -> Vec { + let mut missing = Vec::new(); + for ancestor in path.ancestors() { + // `ancestors` ends in an empty path for a relative input; there is + // no level above that to create. + if ancestor.as_os_str().is_empty() || tokio::fs::symlink_metadata(ancestor).await.is_ok() { + break; + } + missing.push(ancestor.to_path_buf()); + } + missing +} + /// One rename failed: move back everything that had already moved, remove -/// the new root if this request is the one that created it, and name the -/// folder that stopped the move so the operator knows which title to look at -/// before retrying. +/// every directory level this request created, and name the folder that +/// stopped the move so the operator knows which title to look at before +/// retrying. async fn failed( rename: &PlannedRename, error: &std::io::Error, performed: Vec, - created_root: Option, + created_dirs: Vec, ) -> ApiError { Relocation { performed, rewrites: Vec::new(), - created_root, + created_dirs, } .undo() .await; @@ -297,13 +314,17 @@ impl Relocation { // `remove_dir` rather than `remove_dir_all`: it only succeeds on an // empty directory, so anything unexpected left inside it — this // request's own undo failing, say — is a reason to leave it alone. - if let Some(root) = &self.created_root { - if let Err(error) = tokio::fs::remove_dir(root).await { + // Deepest first, since a parent cannot go while its child is there; + // the first level that will not go stops the walk, because every + // level above it now has content and refusing is the right answer. + for directory in &self.created_dirs { + if let Err(error) = tokio::fs::remove_dir(directory).await { tracing::warn!( - path = %root.display(), + path = %directory.display(), %error, - "could not remove the directory created for a move that did not complete" + "could not remove a directory created for a move that did not complete" ); + break; } } } diff --git a/crates/arr-api/src/roots.rs b/crates/arr-api/src/roots.rs index 71ead45..df9ce47 100644 --- a/crates/arr-api/src/roots.rs +++ b/crates/arr-api/src/roots.rs @@ -152,6 +152,7 @@ pub async fn create( input.validate().map_err(ApiError::Invalid)?; input.policy_exists(&state).await?; let path = normalize_path(&input.path); + path_is_free(&state, None, &path).await?; let result = sqlx::query!( "INSERT INTO roots (kind, audience, path, policy_id) VALUES (?, ?, ?, ?)", input.kind, @@ -188,17 +189,25 @@ pub async fn update( input.policy_exists(&state).await?; let current = load_root(&state, id).await?; let path = normalize_path(&input.path); + // #244: rows written before #243 can hold a trailing separator, so the + // stored value is normalised too. Comparing a normalised payload against + // a raw stored value means the row can never compare equal: every edit, + // policy changes included, takes the relocation branch, and there each + // planned destination is its own source. The normalised value is what + // `relocate_root` gets as well, so no planned path carries a doubled + // separator. + let current_path = normalize_path(¤t.path); // A path change moves every §7.4 title folder under this root with the // row (issue #236), the same way changing a title's root moves one // (#228). Disk first, row second: a failed rename leaves the root row // alone, so the operator sees the library where its files actually are // and can retry. A path already taken is refused before any of it, since // the write would fail afterwards anyway. - let relocation = if path == current.path { + let relocation = if path == current_path { None } else { - path_is_free(&state, id, &path).await?; - Some(crate::relocate::relocate_root(&state, id, ¤t.path, &path).await?) + path_is_free(&state, Some(id), &path).await?; + Some(crate::relocate::relocate_root(&state, id, ¤t_path, &path).await?) }; let mut transaction = pool(&state)?.begin().await?; let written: Result<(), sqlx::Error> = async { @@ -245,12 +254,25 @@ pub async fn update( } /// The unique index on `path` would catch this after the move; catching it -/// first keeps a doomed write from touching the disk at all. -async fn path_is_free(state: &AppState, id: i64, path: &str) -> Result<(), ApiError> { - let taken: Option = - sqlx::query_scalar!("SELECT id FROM roots WHERE path = ? AND id <> ?", path, id) - .fetch_optional(pool(state)?) - .await?; +/// first keeps a doomed write from touching the disk at all. `except` is the +/// row being updated, or `None` when creating. +/// +/// The stored side is normalised in SQL, mirroring [`normalize_path`], so +/// `/mnt/media/x` and `/mnt/media/x/` cannot be two roots for one directory +/// (#244). The unique index cannot see that — it compares the raw strings — +/// and migration 0031 leaves any pair that already collides alone rather +/// than failing to apply, so such a row can still be on disk. +async fn path_is_free(state: &AppState, except: Option, path: &str) -> Result<(), ApiError> { + let taken: Option = sqlx::query_scalar!( + r#"SELECT id AS "id!: i64" FROM roots + WHERE CASE WHEN rtrim(path, '/') = '' THEN '/' ELSE rtrim(path, '/') END = ? + AND (? IS NULL OR id <> ?)"#, + path, + except, + except + ) + .fetch_optional(pool(state)?) + .await?; if taken.is_some() { return Err(ApiError::Conflict( "a root with this path already exists".into(), @@ -749,6 +771,17 @@ mod tests { assert_eq!(response.status(), StatusCode::NO_CONTENT); } + /// A path written straight into the row, no normalisation — how a root + /// created before #243 could end up holding a trailing separator. + async fn point_root_at_raw(state: &AppState, root_id: i64, path: &str) { + sqlx::query("UPDATE roots SET path = ? WHERE id = ?") + .bind(path) + .bind(root_id) + .execute(state.database().expect("database").pool()) + .await + .expect("store the raw path"); + } + async fn point_root_at(state: &AppState, root_id: i64, path: &std::path::Path) { sqlx::query("UPDATE roots SET path = ? WHERE id = ?") .bind(path.to_str().expect("utf-8 root")) @@ -1257,6 +1290,192 @@ mod tests { ); } + /// Issue #244: a root *stored* with a trailing separator — creatable + /// through the API at any point before #243 — could not be edited at + /// all. The payload was normalised and the stored value was not, so no + /// payload compared equal: every edit took the relocation branch, where + /// every planned destination is its own source and the pre-check 409s. + #[tokio::test] + async fn a_root_stored_with_a_trailing_separator_can_still_be_edited() { + let (_dir, state, base) = application().await; + let old = tempfile::tempdir().expect("old root"); + let unslashed = old.path().to_str().expect("utf-8").to_owned(); + point_root_at_raw(&state, 1, &format!("{unslashed}/")).await; + let id = add_movie(&base, 100, "Dune", 1).await; + let folder = library_folder(&state, id, old.path(), "Dune").await; + + // What the settings view sends back: the path exactly as stored, + // separator included, with only the policy changed. + let kids_policy = policy_id_named(&base, "Movies — kids").await; + let mut payload = root_payload(&base, 1, &format!("{unslashed}/")).await; + payload["policy_id"] = kids_policy.into(); + let response = reqwest::Client::new() + .put(format!("{base}/api/roots/1")) + .json(&payload) + .send() + .await + .expect("edit the root"); + assert_eq!( + response.status(), + StatusCode::OK, + "{:?}", + response.text().await + ); + + assert!(folder.join("feature.mkv").exists(), "nothing on disk moved"); + assert_eq!( + stored_path(&base, 1).await, + unslashed, + "the row is left normalised, so the next edit compares equal too" + ); + + // And the same payload without the separator is not a relocation + // either. + let response = reqwest::Client::new() + .put(format!("{base}/api/roots/1")) + .json(&root_payload(&base, 1, &unslashed).await) + .send() + .await + .expect("edit the root again"); + assert_eq!(response.status(), StatusCode::OK); + assert!(folder.join("feature.mkv").exists(), "still nothing moved"); + } + + /// Issue #244: a real path change from a root stored with a trailing + /// separator plans from the normalised value, so neither a destination + /// nor a rewritten row carries a doubled separator. + #[tokio::test] + async fn relocating_a_slash_stored_root_plans_no_doubled_separator() { + let (_dir, state, base) = application().await; + let old = tempfile::tempdir().expect("old root"); + let home = tempfile::tempdir().expect("home of the new path"); + let new = home.path().join("relocated-main"); + let unslashed = old.path().to_str().expect("utf-8").to_owned(); + point_root_at_raw(&state, 1, &format!("{unslashed}/")).await; + let id = add_movie(&base, 100, "Dune", 1).await; + let folder = library_folder(&state, id, old.path(), "Dune").await; + + let response = reqwest::Client::new() + .put(format!("{base}/api/roots/1")) + .json(&root_payload(&base, 1, new.to_str().expect("utf-8")).await) + .send() + .await + .expect("move the root"); + assert_eq!( + response.status(), + StatusCode::OK, + "{:?}", + response.text().await + ); + + assert!(!folder.exists(), "the folder left the old path"); + assert!(new.join("Dune").join("feature.mkv").exists()); + let paths = file_paths(&state).await; + assert!( + !paths[0].contains("//"), + "no doubled separator in the rewritten row: {}", + paths[0] + ); + assert!( + std::path::Path::new(&paths[0]).exists(), + "the rewritten path describes the disk: {}", + paths[0] + ); + assert_eq!(stored_path(&base, 1).await, new.to_str().expect("utf-8")); + } + + /// Issue #244: `create_dir_all` can materialise more than one level for + /// a root path pointed somewhere fresh. A failed move removes every + /// level it created, not only the leaf — and still nothing that was + /// already on disk. + #[cfg(unix)] + #[tokio::test] + async fn a_failed_move_removes_every_level_it_created() { + use std::os::unix::fs::PermissionsExt; + + let (_dir, state, base) = application().await; + let old = tempfile::tempdir().expect("old root"); + let home = tempfile::tempdir().expect("home of the new path"); + // Three levels below a directory that is already there. + let top = home.path().join("media-v2"); + let new = top.join("tv").join("kids"); + point_root_at(&state, 1, old.path()).await; + + let id = add_movie(&base, 100, "Dune", 1).await; + let stuck = library_folder(&state, id, old.path(), "Dune").await; + // Moving a directory to another parent rewrites its `..`, which + // needs write permission on the directory itself. + tokio::fs::set_permissions(&stuck, std::fs::Permissions::from_mode(0o555)) + .await + .expect("freeze the title folder"); + + let response = reqwest::Client::new() + .put(format!("{base}/api/roots/1")) + .json(&root_payload(&base, 1, new.to_str().expect("utf-8")).await) + .send() + .await + .expect("move the root"); + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + + tokio::fs::set_permissions(&stuck, std::fs::Permissions::from_mode(0o755)) + .await + .expect("thaw the title folder"); + assert!( + !top.exists(), + "every level the failed move created is gone, not only the leaf" + ); + assert!( + home.path().exists(), + "the level that was already there is left alone" + ); + assert!( + stuck.join("feature.mkv").exists(), + "the folder is still where the row says it is" + ); + assert_eq!( + stored_path(&base, 1).await, + old.path().to_str().expect("utf-8") + ); + } + + /// Issue #244: two roots naming one directory, differing only by a + /// trailing separator, are not two roots. The unique index compares raw + /// strings and cannot see it, so the check normalises both sides. + #[tokio::test] + async fn a_slash_stored_path_is_not_free_for_another_root() { + let (_dir, state, base) = application().await; + point_root_at_raw(&state, 2, "/mnt/media/movies/archive/").await; + + // An update onto the stripped form of a path another root holds. + let response = reqwest::Client::new() + .put(format!("{base}/api/roots/1")) + .json(&root_payload(&base, 1, "/mnt/media/movies/archive").await) + .send() + .await + .expect("update onto the other root's path"); + assert_eq!(response.status(), StatusCode::CONFLICT); + + // And a create, which the unique index would have let through. + let policy_ids = first_policy_ids(&base).await; + let mut payload = root_input(policy_ids[0]); + payload["kind"] = serde_json::json!("tv"); + payload["audience"] = serde_json::json!("main"); + payload["path"] = serde_json::json!("/mnt/media/movies/archive"); + let deleted = reqwest::Client::new() + .delete(format!("{base}/api/roots/3")) + .send() + .await + .expect("free the (tv, main) pair"); + assert_eq!(deleted.status(), StatusCode::NO_CONTENT); + let response = reqwest::Client::new() + .post(format!("{base}/api/roots")) + .json(&payload) + .send() + .await + .expect("create onto the other root's path"); + assert_eq!(response.status(), StatusCode::CONFLICT); + } + /// Issue #236: a path another root already holds is refused before the /// disk is touched at all. #[tokio::test] diff --git a/crates/arr-db/migrations/0031_normalise_root_paths.sql b/crates/arr-db/migrations/0031_normalise_root_paths.sql new file mode 100644 index 0000000..c67840f --- /dev/null +++ b/crates/arr-db/migrations/0031_normalise_root_paths.sql @@ -0,0 +1,34 @@ +-- Issue #244. A root's path could be stored with a trailing separator until +-- #243 normalised the incoming value. `roots::update` normalises the payload +-- and compares it against the stored value, so such a row never compares +-- equal: every edit, a policy change included, takes the relocation branch, +-- and there every planned destination is its own source. Normalising the +-- payload alone fixed the half that cannot bite; this is the other half. +-- +-- `rtrim` strips every trailing separator at once, so '/mnt/x//' normalises +-- in one pass. A bare '/' rtrims to the empty string and is put back, which +-- is what `normalize_path` in arr-api does. +-- +-- Guarded, because `roots.path` is UNIQUE and a migration that cannot apply +-- stops the daemon booting -- worse than the bug it fixes. A row is +-- normalised only when no other row shares its normalised path: neither a +-- row already holding the stripped value, nor another trailing-separator row +-- that would strip to the same thing. Every row in such a group is left +-- exactly as it is. That leaves two roots naming one directory, which is a +-- settings mistake for the operator to resolve by hand, not a reason to +-- refuse to boot. +-- +-- This cannot introduce a collision either. An updated row's new value is a +-- normalised path no other row normalises to, and a row left alone whose raw +-- path equalled that value would have had the same normalised path, which is +-- the case the guard excludes. +UPDATE roots +SET path = CASE WHEN rtrim(path, '/') = '' THEN '/' ELSE rtrim(path, '/') END +WHERE path <> CASE WHEN rtrim(path, '/') = '' THEN '/' ELSE rtrim(path, '/') END + AND NOT EXISTS ( + SELECT 1 + FROM roots AS other + WHERE other.id <> roots.id + AND CASE WHEN rtrim(other.path, '/') = '' THEN '/' ELSE rtrim(other.path, '/') END + = CASE WHEN rtrim(roots.path, '/') = '' THEN '/' ELSE rtrim(roots.path, '/') END + ); diff --git a/crates/arr-db/src/lib.rs b/crates/arr-db/src/lib.rs index 647b8a8..0d4b5ae 100644 --- a/crates/arr-db/src/lib.rs +++ b/crates/arr-db/src/lib.rs @@ -347,6 +347,93 @@ mod tests { assert_eq!(renamed_from_imported, "available"); } + /// The four seeded roots, up to but not including migration 0031, with + /// the given legacy paths written straight into the rows. + async fn roots_before_normalisation(paths: &[(i64, &str)]) -> (tempfile::TempDir, Db) { + let dir = tempfile::tempdir().expect("tempdir"); + let db = Db::connect(dir.path().join("arr.db")) + .await + .expect("connect"); + MIGRATOR + .run_to(30, db.pool()) + .await + .expect("migrations before #244"); + for (id, path) in paths { + sqlx::query("UPDATE roots SET path = ? WHERE id = ?") + .bind(path) + .bind(id) + .execute(db.pool()) + .await + .expect("legacy path, valid under the unique index"); + } + (dir, db) + } + + async fn root_path(db: &Db, id: i64) -> String { + sqlx::query_scalar("SELECT path FROM roots WHERE id = ?") + .bind(id) + .fetch_one(db.pool()) + .await + .expect("root path") + } + + /// #244: a root's path could be stored with a trailing separator until + /// #243 normalised the incoming value, and `roots::update` compares a + /// normalised payload against the stored value — so such a row could + /// never be edited again. Migration 0031 strips the separator, and skips + /// a row whose stripped form another row already holds rather than + /// tripping the unique index and refusing to apply. + #[tokio::test] + async fn root_paths_are_normalised_but_never_onto_a_path_in_use() { + let (_dir, db) = roots_before_normalisation(&[ + (1, "/mnt/media/movies/main/"), + (2, "/mnt/collide"), + (3, "/mnt/collide/"), + (4, "/mnt/media/tv/kids///"), + ]) + .await; + + db.migrate() + .await + .expect("0031 applies with a collision present"); + + assert_eq!(root_path(&db, 1).await, "/mnt/media/movies/main"); + assert_eq!( + root_path(&db, 4).await, + "/mnt/media/tv/kids", + "every trailing separator goes in one pass" + ); + assert_eq!(root_path(&db, 2).await, "/mnt/collide"); + assert_eq!( + root_path(&db, 3).await, + "/mnt/collide/", + "left exactly as it is: normalising it would collide with root 2" + ); + } + + /// #244: the harder half of the same guard. Two rows that strip to the + /// same path, *neither* of which already holds the stripped value, still + /// have to be left alone — normalising them would collide with each + /// other, and a migration that cannot apply stops the daemon booting. + #[tokio::test] + async fn two_rows_that_would_collide_with_each_other_stop_nothing() { + let (_dir, db) = + roots_before_normalisation(&[(1, "/mnt/one/"), (2, "/mnt/dup/"), (3, "/mnt/dup//")]) + .await; + + db.migrate() + .await + .expect("0031 applies with a mutually colliding pair present"); + + assert_eq!(root_path(&db, 2).await, "/mnt/dup/"); + assert_eq!(root_path(&db, 3).await, "/mnt/dup//"); + assert_eq!( + root_path(&db, 1).await, + "/mnt/one", + "the rest of the table is still normalised" + ); + } + /// #155: the table-rebuild migrations (0007, 0014, 0021) run outside a /// transaction so `PRAGMA foreign_keys = OFF` holds and dropping the old /// tables does not cascade-delete `movie_releases`/`episode_releases`.