fix(api): normalise stored root paths

#243 normalised the incoming path but compared it against the value read
raw from the database, so a root stored with a trailing separator never
compared equal. Every edit of it -- a policy change included -- took the
relocation branch, where each planned destination is its own source and
the pre-check refuses. That root could not be edited at all.

`update` now normalises both sides, and hands `relocate_root` the
normalised stored value. `path_is_free` normalises the stored side in SQL
and `create` goes through it too, so `/mnt/x` and `/mnt/x/` cannot be two
roots for one directory -- the unique index compares raw strings and
cannot see that.

Migration 0031 strips the separator from rows already written. It skips
any row whose stripped form another row would also hold, rather than
tripping the unique index: a migration that cannot apply stops the daemon
booting, which is worse than two roots naming one directory.

Also from the same review: `undo` recorded only the leaf directory, so a
failed move into `/mnt/media-v2/tv/kids` left `tv` behind. It now records
every level `create_dir_all` materialised, deepest first, and still never
touches one that was already on disk.

Refs #244.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

.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"
 }

crates/arr-api/src/relocate.rs

@@ -50,11 +50,14 @@ struct PlannedRename {
 pub(crate) struct Relocation {
     performed: Vec<PlannedRename>,
     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<PathBuf>,
+    /// 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<PathBuf>,
 }
 
 /// 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<PathBuf> = 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<PathBuf> = 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<PlannedRename> = 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<PathBuf> {
+    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<PlannedRename>,
-    created_root: Option<PathBuf>,
+    created_dirs: Vec<PathBuf>,
 ) -> 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;
             }
         }
     }

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(&current.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, &current.path, &path).await?)
+        path_is_free(&state, Some(id), &path).await?;
+        Some(crate::relocate::relocate_root(&state, id, &current_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<i64> =
-        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<i64>, path: &str) -> Result<(), ApiError> {
+    let taken: Option<i64> = 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]

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
+  );

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`.