fix(api): let a stranded folder be retried, not refused

Two findings from the integration review of this milestone, both caused
by two sessions editing the same code without seeing each other.

The retry that relocate.rs documents did not converge. The conflict
pre-check ran over every planned rename, including renames whose source
was already gone, and the skip for a missing source came after it. An
undo is best-effort, so a failed move can leave one folder at the
destination with its row still naming the source; every later attempt
then 409'd against the operator's own half-moved library and the only
way out was moving the folder back by hand. The pre-check now skips a
rename whose source is absent, which is what the perform loop already
did. Verified: the new test returns 409 without the change and 200 with.

ApiError::Filesystem rendered as "files not removed: {error}". That was
written for the delete lane; #228 and #236 then returned the same
variant for move failures, so a root path change with one unwritable
folder reported "files not removed" after an operation that removed
nothing. The variant now renders the caller's message and the two
delete lanes carry their own context.

crates/arr-api/src/movies.rs

@@ -199,12 +199,12 @@ impl IntoResponse for ApiError {
                 tracing::error!(%error, "API database error");
                 (StatusCode::INTERNAL_SERVER_ERROR, "database error".into())
             }
+            // The message is the caller's: this variant is returned by the
+            // delete lane and by the relocate lane, and "files not removed"
+            // is a lie about a move that failed.
             Self::Filesystem(error) => {
                 tracing::error!(%error, "API filesystem error");
-                (
-                    StatusCode::INTERNAL_SERVER_ERROR,
-                    format!("files not removed: {error}"),
-                )
+                (StatusCode::INTERNAL_SERVER_ERROR, error.clone())
             }
         };
         (status, Json(ErrorBody { error })).into_response()
@@ -570,7 +570,11 @@ async fn remove_library_files(state: &AppState, id: i64) -> Result<(), ApiError>
             Ok(metadata) => metadata,
             // Already gone is the state we wanted.
             Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue,
-            Err(error) => return Err(ApiError::Filesystem(error.to_string())),
+            Err(error) => {
+                return Err(ApiError::Filesystem(format!(
+                    "files not removed: {error}"
+                )))
+            }
         };
         let removed = if metadata.is_dir() {
             tokio::fs::remove_dir_all(&target).await
@@ -580,7 +584,11 @@ async fn remove_library_files(state: &AppState, id: i64) -> Result<(), ApiError>
         match removed {
             Ok(()) => tracing::info!(target = %target.display(), "removed library files"),
             Err(error) if error.kind() == std::io::ErrorKind::NotFound => {}
-            Err(error) => return Err(ApiError::Filesystem(error.to_string())),
+            Err(error) => {
+                return Err(ApiError::Filesystem(format!(
+                    "files not removed: {error}"
+                )))
+            }
         }
     }
     Ok(())

crates/arr-api/src/relocate.rs

@@ -149,8 +149,18 @@ async fn relocate_files(
     }
 
     // Every destination is checked before anything is renamed, so a conflict
-    // never leaves a half-moved library behind.
+    // never leaves a half-moved library behind. A rename whose source is
+    // already gone is skipped here rather than refused: the perform loop below
+    // skips it too, and refusing it would make the documented retry
+    // impossible. After an undo that itself failed, the folder sits at the
+    // destination while the row still names the source, and every later
+    // attempt would 409 on a conflict with the operator's own half-moved
+    // library.
     for rename in &renames {
+        match tokio::fs::symlink_metadata(&rename.source).await {
+            Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue,
+            _ => {}
+        }
         match tokio::fs::symlink_metadata(&rename.destination).await {
             Ok(_) => {
                 return Err(ApiError::Conflict(format!(

crates/arr-api/src/roots.rs

@@ -747,6 +747,59 @@ mod tests {
         );
     }
 
+    /// Integration review of the Feedback pass 2 blitz: the retry the module
+    /// documents has to actually converge. An undo is best-effort, so a move
+    /// can fail and leave one folder at the destination with its row still
+    /// naming the source. Every later attempt used to 409 against the
+    /// operator's own half-moved library, and the only way out was moving the
+    /// folder back by hand.
+    #[tokio::test]
+    async fn a_folder_left_at_the_destination_by_a_failed_undo_does_not_block_the_retry() {
+        let (_dir, state, base) = application().await;
+        let old = tempfile::tempdir().expect("old root");
+        let new = tempfile::tempdir().expect("new root");
+        point_root_at(&state, 1, old.path()).await;
+
+        let first = add_movie(&base, 100, "Dune", 1).await;
+        let second = add_movie(&base, 101, "Arrival", 1).await;
+        let dune = library_folder(&state, first, old.path(), "Dune").await;
+        let arrival = library_folder(&state, second, old.path(), "Arrival").await;
+
+        // What a failed undo leaves behind: Arrival is already at the new
+        // path, its row still points at the old one, and the root row was
+        // never changed.
+        tokio::fs::create_dir_all(new.path())
+            .await
+            .expect("the new root");
+        tokio::fs::rename(&arrival, new.path().join("Arrival"))
+            .await
+            .expect("strand the folder");
+        assert!(!arrival.exists());
+
+        let response = reqwest::Client::new()
+            .put(format!("{base}/api/roots/1"))
+            .json(&root_payload(&base, 1, new.path().to_str().expect("utf-8")).await)
+            .send()
+            .await
+            .expect("retry the move");
+        assert_eq!(
+            response.status(),
+            StatusCode::OK,
+            "the stranded folder is the state we wanted, not a conflict"
+        );
+
+        assert!(!dune.exists(), "the folder still on disk moved");
+        assert!(new.path().join("Dune").exists());
+        assert!(new.path().join("Arrival").exists(), "left where it was");
+        let paths = file_paths(&state).await;
+        let root = new.path().to_str().expect("utf-8");
+        assert!(
+            paths.iter().all(|path| path.starts_with(root)),
+            "every row follows the root, the stranded one included: {paths:?}"
+        );
+        assert_eq!(stored_path(&base, 1).await, root);
+    }
+
     /// Issue #236: the move is all or nothing. A folder that cannot be
     /// renamed part-way through takes the whole change down: what had already
     /// moved is moved back, the root row keeps the old path, and the rows

crates/arr-api/src/series.rs

@@ -734,7 +734,11 @@ async fn remove_library_files(state: &AppState, scope: FileScope) -> Result<(),
             Ok(metadata) => metadata,
             // Already gone is the state we wanted.
             Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue,
-            Err(error) => return Err(ApiError::Filesystem(error.to_string())),
+            Err(error) => {
+                return Err(ApiError::Filesystem(format!(
+                    "files not removed: {error}"
+                )))
+            }
         };
         let removed = if metadata.is_dir() {
             tokio::fs::remove_dir_all(&target).await
@@ -744,7 +748,11 @@ async fn remove_library_files(state: &AppState, scope: FileScope) -> Result<(),
         match removed {
             Ok(()) => tracing::info!(target = %target.display(), "removed library files"),
             Err(error) if error.kind() == std::io::ErrorKind::NotFound => {}
-            Err(error) => return Err(ApiError::Filesystem(error.to_string())),
+            Err(error) => {
+                return Err(ApiError::Filesystem(format!(
+                    "files not removed: {error}"
+                )))
+            }
         }
     }
     Ok(())