fix(module): never delete a staged directory, and fall back to the mirror

Disabling or deleting a module unlinked <config>/modules/<id> with a
helper that recursively deletes a real directory, which is a developer's
own staged build and their only copy; those paths now refuse it and say
so, while pkg enable keeps replacing it as documented. The artifact loop
also treated an HTTP error as a successful download, so a deleted release
asset was 'downloaded' as an error page and the mirror after it was never
tried.

Author: afonsojramos

rust/crates/spicetify/src/module/mod.rs

@@ -116,7 +116,15 @@ pub(crate) fn install(paths: &ModulePaths, id: &StoreIdentifier) -> Result<()> {
         // Artifacts are listed in preference order and later entries are
         // mirrors of the same bytes, so a host that has gone away costs an
         // attempt rather than the install.
-        match client.get(artifact).send().and_then(reqwest::blocking::Response::bytes) {
+        // error_for_status first: reqwest calls a 404 Ok, so without it a
+        // deleted release asset would be "downloaded" as an error page and
+        // the mirror after it in the list would never be tried.
+        match client
+            .get(artifact)
+            .send()
+            .and_then(reqwest::blocking::Response::error_for_status)
+            .and_then(reqwest::blocking::Response::bytes)
+        {
             Ok(bytes) => {
                 downloaded = Some(bytes);
                 break;
@@ -176,12 +184,15 @@ pub(crate) fn enable(paths: &ModulePaths, id: &StoreIdentifier) -> Result<()> {
     }
 
     let link = id.module_link_path(&paths.modules_root);
-    if let Err(e) = crate::util::remove_dir_link(&link) {
-        tracing::warn!(error = %e, path = %link.display(), "failed to remove link");
-    }
-    if enabled.is_some() {
+    if let Some(_version) = enabled.as_ref() {
+        // create_dir_link replaces whatever is there, including a real
+        // directory: switching a hand-staged build over to a store version is
+        // what `pkg enable` is for.
         let src = id.store_path(&paths.store_root);
         super::util::link::create_dir_link(&src, &link)?;
+    } else if let Err(e) = crate::util::remove_link_only(&link) {
+        // Disabling must never delete a developer's staged directory.
+        tracing::warn!(error = %e, path = %link.display(), "failed to remove link");
     }
     vault::save(&paths.vault_path, &v)?;
     Ok(())
@@ -194,7 +205,9 @@ pub(crate) fn delete(paths: &ModulePaths, id: &StoreIdentifier) -> Result<()> {
             module.enabled = None;
             if !id.module_identifier.contains('/') {
                 let link = id.module_link_path(&paths.modules_root);
-                if let Err(e) = crate::util::remove_dir_link(&link) {
+                // Deleting a package unlinks it; it never deletes a real
+                // directory, which is a developer's own staged build.
+                if let Err(e) = crate::util::remove_link_only(&link) {
                     tracing::warn!(error = %e, path = %link.display(), "failed to remove link");
                 }
             }

rust/crates/spicetify/src/util/link.rs

@@ -77,7 +77,7 @@ mod tests {
 }
 
 #[cfg(windows)]
-pub(crate) fn remove_dir_link(link: &Path) -> Result<()> {
+fn remove_dir_link(link: &Path) -> Result<()> {
     if let Err(e) = std::fs::remove_dir(link)
         && e.kind() != std::io::ErrorKind::NotFound
     {
@@ -86,12 +86,24 @@ pub(crate) fn remove_dir_link(link: &Path) -> Result<()> {
     Ok(())
 }
 
+/// See the unix definition. `remove_dir` removes a junction but refuses a
+/// non-empty real directory, so the guarantee holds here without a check.
+#[cfg(windows)]
+pub(crate) fn remove_link_only(link: &Path) -> Result<()> {
+    remove_dir_link(link)
+}
+
 /// Removes `link`, whether it is a symlink or a real directory left behind by
 /// an older install. Branches on `symlink_metadata` rather than on the error
 /// from `remove_file`, which is `IsADirectory` on Linux but `PermissionDenied`
 /// on macOS.
+///
+/// Replacing a real directory here is intended: `pkg enable` switching a
+/// hand-staged build over to a store version is the documented way to do it,
+/// and the user asked for it. Removal on its own goes through
+/// `remove_link_only`.
 #[cfg(not(windows))]
-pub(crate) fn remove_dir_link(link: &Path) -> Result<()> {
+fn remove_dir_link(link: &Path) -> Result<()> {
     let meta = match std::fs::symlink_metadata(link) {
         Ok(meta) => meta,
         Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()),
@@ -103,3 +115,69 @@ pub(crate) fn remove_dir_link(link: &Path) -> Result<()> {
         std::fs::remove_file(link).map_err(Into::into)
     }
 }
+
+/// Unlinks `link` without ever deleting a real directory.
+///
+/// `<config>/modules/<id>` is a link into the store for anything the store
+/// installed, but it is a real directory when a developer staged a build
+/// there by hand, and that directory is their source. Disabling or deleting a
+/// module must not take it: the caller is told instead, so the removal stays
+/// a deliberate act.
+#[cfg(not(windows))]
+pub(crate) fn remove_link_only(link: &Path) -> Result<()> {
+    let meta = match std::fs::symlink_metadata(link) {
+        Ok(meta) => meta,
+        Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()),
+        Err(e) => return Err(e.into()),
+    };
+    if meta.is_dir() {
+        anyhow::bail!(
+            "{} is a real directory, not a link into the store; refusing to delete it",
+            link.display()
+        );
+    }
+    std::fs::remove_file(link).map_err(Into::into)
+}
+
+#[cfg(all(test, not(windows)))]
+mod removal_tests {
+    use super::*;
+
+    fn scratch(name: &str) -> std::path::PathBuf {
+        let dir =
+            std::env::temp_dir().join(format!("spicetify-rm-{name}-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(&dir).expect("temp dir");
+        dir
+    }
+
+    #[test]
+    fn remove_link_only_refuses_a_developers_staged_directory() {
+        let dir = scratch("staged");
+        let staged = dir.join("my-mod");
+        std::fs::create_dir_all(&staged).expect("staged dir");
+        std::fs::write(staged.join("mod.tsx"), "source that exists nowhere else").expect("source");
+
+        let err = remove_link_only(&staged).expect_err("a real directory must not be deleted");
+        assert!(err.to_string().contains("refusing to delete"), "{err}");
+        assert!(staged.join("mod.tsx").is_file(), "the developer's source survives");
+
+        std::fs::remove_dir_all(&dir).expect("cleanup");
+    }
+
+    #[test]
+    fn remove_link_only_removes_a_store_link_and_leaves_its_target() {
+        let dir = scratch("store-link");
+        let target = dir.join("store-copy");
+        let link = dir.join("my-mod");
+        std::fs::create_dir_all(&target).expect("target");
+        std::fs::write(target.join("index.js").as_path(), "built").expect("target file");
+        std::os::unix::fs::symlink(&target, &link).expect("link");
+
+        remove_link_only(&link).expect("a link is removable");
+        assert!(std::fs::symlink_metadata(&link).is_err(), "link is gone");
+        assert!(target.join("index.js").is_file(), "the store copy is untouched");
+
+        std::fs::remove_dir_all(&dir).expect("cleanup");
+    }
+}

rust/crates/spicetify/src/util/mod.rs

@@ -4,7 +4,7 @@ pub(crate) mod archive;
 pub(crate) mod link;
 
 pub(crate) use archive::{untar_zst_bytes, unzip_file};
-pub(crate) use link::{create_dir_link, remove_dir_link};
+pub(crate) use link::{create_dir_link, remove_link_only};
 
 #[derive(Debug, Error)]
 pub(crate) enum ArchiveError {