fix: honor explicit cache dir in delete_model

[andrewpaprotsky]

delete_model previously built an hf_hub client without the per-call cache root, so it could target env/default cache instead of the directory used by download_model.

This change threads cache_dir through ModelProviderTrait::delete_model and configures the Hugging Face client with get_cache_dir(cache_dir) + ApiBuilder::with_cache_dir(...).

Also adds a regression test that sets env cache to a different directory and verifies explicit cache-dir precedence during delete.

Summary by CodeRabbit

Release Notes

• New Features

  • Added optional cache directory selection for model deletion, enabling explicit override of default cache settings.

• Bug Fixes

  • Enhanced model deletion to automatically remove empty model directories after successful file cleanup.

[coderabbitai]

Walkthrough

The pull request updates the delete_model method signature across the trait definition, mock implementations, and the HuggingFace provider implementation to accept an optional cache_dir parameter. The HuggingFace provider implementation is enhanced to prioritize explicit cache directory arguments and clean up empty model directories post-deletion.

Changes

Cohort / File(s)	Summary
Trait Definition and Mock Implementations modelexpress_common/src/providers.rs, modelexpress_common/src/download.rs	Updated delete_model method signature in ModelProviderTrait and mock/test provider implementations to accept optional cache_dir: Option<PathBuf> parameter.
HuggingFace Provider Implementation modelexpress_common/src/providers/huggingface.rs	Implemented new delete_model signature with cache directory preference logic. Added get_cache_dir() for explicit cache_dir evaluation, updated API client construction with ApiBuilder::from_env() and .with_cache_dir(), introduced model directory tracking for empty directory cleanup after file deletion, and added comprehensive tests including environment mutex serialization and cache_dir precedence validation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit hops through cache with care,
Directories cleaned, now sparse and fair,
The optional path guides where we go,
Empty nests removed, a tidy tableau! 🌿

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: honor explicit cache dir in delete_model' accurately and concisely describes the main change: enabling delete_model to respect an explicitly provided cache directory parameter.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

modelexpress_common/src/providers/huggingface.rs (1)

494-506: Make env-var cleanup panic-safe in the new precedence test.

The manual set/remove blocks work on the happy path, but a panic between them can leak env state to later tests. Consider a tiny RAII guard for deterministic cleanup.

Proposed refactor

+    struct EnvVarCleanup<'a> {
+        keys: Vec<&'a str>,
+    }
+
+    impl<'a> EnvVarCleanup<'a> {
+        fn new(keys: Vec<&'a str>) -> Self {
+            Self { keys }
+        }
+    }
+
+    impl Drop for EnvVarCleanup<'_> {
+        fn drop(&mut self) {
+            for key in &self.keys {
+                unsafe {
+                    env::remove_var(key);
+                }
+            }
+        }
+    }
+
     #[tokio::test]
     #[allow(clippy::await_holding_lock)]
     async fn test_delete_model_prefers_explicit_cache_dir_over_env() {
         let _guard = acquire_env_mutex();
@@
         unsafe {
             env::set_var(MODEL_EXPRESS_CACHE_ENV_VAR, env_cache.path());
             env::set_var(HF_HUB_CACHE_ENV_VAR, env_cache.path());
         }
+        let _env_cleanup = EnvVarCleanup::new(vec![
+            MODEL_EXPRESS_CACHE_ENV_VAR,
+            HF_HUB_CACHE_ENV_VAR,
+        ]);
 
         let delete_result = provider
             .delete_model("test/model", Some(explicit_cache.path().to_path_buf()))
             .await;
-
-        unsafe {
-            env::remove_var(MODEL_EXPRESS_CACHE_ENV_VAR);
-            env::remove_var(HF_HUB_CACHE_ENV_VAR);
-        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@modelexpress_common/src/providers/huggingface.rs` around lines 494 - 506, The
env var setup/cleanup around the provider.delete_model call is not panic-safe;
replace the manual unsafe set_var/remove_var blocks with a small RAII guard
(e.g., EnvVarGuard) that sets MODEL_EXPRESS_CACHE_ENV_VAR and
HF_HUB_CACHE_ENV_VAR to env_cache.path() on creation and unsets them in Drop so
cleanup runs even if provider.delete_model("test/model", ...) panics; construct
the guard before calling provider.delete_model and let it drop afterward (no
manual remove_var calls).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@modelexpress_common/src/providers/huggingface.rs`:
- Around line 494-506: The env var setup/cleanup around the
provider.delete_model call is not panic-safe; replace the manual unsafe
set_var/remove_var blocks with a small RAII guard (e.g., EnvVarGuard) that sets
MODEL_EXPRESS_CACHE_ENV_VAR and HF_HUB_CACHE_ENV_VAR to env_cache.path() on
creation and unsets them in Drop so cleanup runs even if
provider.delete_model("test/model", ...) panics; construct the guard before
calling provider.delete_model and let it drop afterward (no manual remove_var
calls).

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3376ee5 and e22f5ac.

📒 Files selected for processing (3)

• modelexpress_common/src/download.rs
• modelexpress_common/src/providers.rs
• modelexpress_common/src/providers/huggingface.rs