From 41a6cc8692bd47ba0e4590b45bdf88d24b15ef33 Mon Sep 17 00:00:00 2001
From: Daniel Han <danielhanchen@gmail.com>
Date: Thu, 23 Apr 2026 00:22:40 -0700
Subject: [PATCH] Studio: split vision-cache exception test to match transient
 vs permanent (#5145)

`TestVisionCacheOnException::test_exception_result_cached` currently
patches `load_model_config` with `side_effect=OSError("network down")`
and asserts `assert_called_once()`. That assertion is impossible by
design: `_is_vision_model_uncached` in
`studio/backend/utils/models/model_config.py` intentionally returns
`None` for `OSError` so `is_vision_model` does not cache the fallback
and retries on the next call. The module docstring on
`_vision_detection_cache` itself spells this out:

    Only definitive results (True/False from successful detection) are
    cached; transient failures (network errors, timeouts) are NOT
    cached so they can be retried.

The test has been failing identically on every downstream review run
against `unslothai/unsloth` main (e.g. `unsloth#5115`, `unsloth#5080`),
but the failure is not introduced by any of those PRs and does not
gate correctness.

Fix the collision by splitting the class into the two contracts the
code actually implements:

1. `test_permanent_exception_result_cached` keeps the original
   intent ("exception falls back to False and that False is cached")
   but uses `ValueError`, which is one of the exception types
   `_is_vision_model_uncached` treats as permanent and caches. No
   `huggingface_hub` import needed.

2. `test_transient_exception_not_cached` pins the opposite contract
   with the original `OSError("network down")`: the call returns
   False but the second invocation re-runs detection
   (`call_count == 2`). This guards against a future regression
   where somebody caches transient failures and then users with a
   flaky network permanently see wrong detection for a model.

Both tests use `assert ... is False` on the public API and mock-count
assertions on `load_model_config`; no private helpers are touched.
---
 studio/backend/tests/test_vision_cache.py | 45 ++++++++++++++++++-----
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/studio/backend/tests/test_vision_cache.py b/studio/backend/tests/test_vision_cache.py
index fae1e95311..9e7bbdd1fb 100644
--- a/studio/backend/tests/test_vision_cache.py
+++ b/studio/backend/tests/test_vision_cache.py
@@ -124,23 +124,50 @@ class TestVisionCacheSubprocessPath:
 
 
 class TestVisionCacheOnException:
-    """When detection raises an exception, _is_vision_model_uncached catches
-    it and returns False.  That False must be cached so subsequent calls don't
-    retry and fail again."""
+    """When detection raises an exception, _is_vision_model_uncached
+    distinguishes permanent failures (cached as False) from transient
+    failures (returned as None, not cached so the next call can retry).
+    Verify both contracts."""
+
+    @patch(
+        "utils.models.model_config.load_model_config",
+        side_effect = ValueError("bad config"),
+    )
+    @patch("utils.transformers_version.needs_transformers_5", return_value = False)
+    def test_permanent_exception_result_cached(self, mock_needs_t5, mock_load_config):
+        """A permanent failure (ValueError / RepositoryNotFoundError /
+        GatedRepoError / JSONDecodeError) should be caught, return False,
+        and that False should be cached so subsequent calls don't retry.
+
+        ValueError is used here because it's the simplest of the
+        code-path's cacheable exception types and does not require an
+        import of huggingface_hub errors (whose module path varies
+        across versions)."""
+        # First call: load_model_config raises -> except branch -> False.
+        assert is_vision_model("broken/model") is False
+        # Second call: cache hit, load_model_config not called again.
+        assert is_vision_model("broken/model") is False
+        mock_load_config.assert_called_once()
 
     @patch(
         "utils.models.model_config.load_model_config",
         side_effect = OSError("network down"),
     )
     @patch("utils.transformers_version.needs_transformers_5", return_value = False)
-    def test_exception_result_cached(self, mock_needs_t5, mock_load_config):
-        """A real exception inside _is_vision_model_uncached should be caught,
-        return False, and that False should be cached for subsequent calls."""
-        # First call: load_model_config raises → except branch → False
+    def test_transient_exception_not_cached(self, mock_needs_t5, mock_load_config):
+        """A transient failure (OSError, timeouts) should return None from
+        _is_vision_model_uncached, surface as False to the caller, and
+        NOT be cached, so the next call retries detection.  This matches
+        the documented behaviour on _vision_detection_cache:
+        'transient failures (network errors, timeouts) are NOT cached so
+        they can be retried.'"""
+        # First call: load_model_config raises OSError -> uncached None
+        # -> caller returns False without caching.
         assert is_vision_model("broken/model") is False
-        # Second call: cache hit, load_model_config not called again
+        # Second call: cache miss again, load_model_config called a
+        # second time.
         assert is_vision_model("broken/model") is False
-        mock_load_config.assert_called_once()
+        assert mock_load_config.call_count == 2
 
 
 # ---------------------------------------------------------------------------