From 79b70b8970dd859b02627be31182c45fa69781dd Mon Sep 17 00:00:00 2001 From: James Pine Date: Sun, 19 Apr 2026 19:18:43 -0700 Subject: [PATCH] fix(offline): atomic entry rollback + tidy test assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: - Wrap the `_offline_refcount == 0` setup in a try/except so any failure during the cached-constant mutation (including unexpected non-ImportError like RuntimeError or AttributeError from a half-initialized module) rolls back *all* partial state before re-raising. Without this, a mid-setup crash could leave `huggingface_hub.constants.HF_HUB_OFFLINE` mutated but the refcount at 0 — a persistent offline flag outliving the process. - Swap ruff-flagged Yoda comparisons in the new test file (SIM300) and add a module-level note warning that these tests mutate global state and are not safe under cross-process parallelism. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/tests/test_offline_guard.py | 21 ++++++---- backend/utils/hf_offline_patch.py | 62 +++++++++++++++++++++++------ 2 files changed, 62 insertions(+), 21 deletions(-) diff --git a/backend/tests/test_offline_guard.py b/backend/tests/test_offline_guard.py index 2c9d3963..8a5a4e8f 100644 --- a/backend/tests/test_offline_guard.py +++ b/backend/tests/test_offline_guard.py @@ -5,6 +5,11 @@ Verifies that the helper mutates the cached module constants in ``huggingface_hub.constants`` and ``transformers.utils.hub`` — not just ``os.environ`` — and that concurrent users are refcount-coordinated so one thread's exit can't strip another thread's offline protection. + +NOTE: These tests mutate process-global state in ``huggingface_hub.constants`` +and ``transformers.utils.hub``. They are not safe under cross-process +parallelism (e.g. ``pytest-xdist`` with ``--dist=loadfile``/``loadscope``); +run this file serially. """ import os @@ -36,27 +41,27 @@ def test_mutates_cached_huggingface_hub_constant(): original = _hf_const().HF_HUB_OFFLINE with force_offline_if_cached(True, "t"): assert _hf_const().HF_HUB_OFFLINE is True - assert _hf_const().HF_HUB_OFFLINE == original + assert original == _hf_const().HF_HUB_OFFLINE def test_mutates_cached_transformers_constant(): original = _tf_hub()._is_offline_mode with force_offline_if_cached(True, "t"): assert _tf_hub()._is_offline_mode is True - assert _tf_hub()._is_offline_mode == original + assert original == _tf_hub()._is_offline_mode def test_sets_env_variable(): original = os.environ.get("HF_HUB_OFFLINE") with force_offline_if_cached(True, "t"): - assert os.environ.get("HF_HUB_OFFLINE") == "1" - assert os.environ.get("HF_HUB_OFFLINE") == original + assert "1" == os.environ.get("HF_HUB_OFFLINE") + assert original == os.environ.get("HF_HUB_OFFLINE") def test_noop_when_not_cached(): before = _hf_const().HF_HUB_OFFLINE with force_offline_if_cached(False, "t"): - assert _hf_const().HF_HUB_OFFLINE == before + assert before == _hf_const().HF_HUB_OFFLINE def test_nested_contexts_respect_refcount(): @@ -67,7 +72,7 @@ def test_nested_contexts_respect_refcount(): assert _hf_const().HF_HUB_OFFLINE is True # inner exit must not restore while outer is still active assert _hf_const().HF_HUB_OFFLINE is True - assert _hf_const().HF_HUB_OFFLINE == original + assert original == _hf_const().HF_HUB_OFFLINE def test_concurrent_threads_share_offline_window(): @@ -101,8 +106,8 @@ def test_concurrent_threads_share_offline_window(): t_fast.join(timeout=5) assert not errors, errors - assert observations == [True], "slow thread lost offline protection" - assert _hf_const().HF_HUB_OFFLINE == original + assert [True] == observations, "slow thread lost offline protection" + assert original == _hf_const().HF_HUB_OFFLINE if __name__ == "__main__": diff --git a/backend/utils/hf_offline_patch.py b/backend/utils/hf_offline_patch.py index 8e2db504..354b6110 100644 --- a/backend/utils/hf_offline_patch.py +++ b/backend/utils/hf_offline_patch.py @@ -54,23 +54,59 @@ def force_offline_if_cached(is_cached: bool, model_label: str = ""): with _offline_lock: if _offline_refcount == 0: - _saved_env = os.environ.get("HF_HUB_OFFLINE") + # Snapshot prior state, apply new state, roll back on *any* + # failure. Catching only ImportError here would let a partially + # broken install (RuntimeError, AttributeError from a half-init + # module, etc.) leave the cached HF constants mutated without + # bumping the refcount — a persistent offline leak that outlives + # the process and is miserable to debug. + prev_env = os.environ.get("HF_HUB_OFFLINE") + prev_hf: Optional[bool] = None + prev_tf: Optional[bool] = None try: - import huggingface_hub.constants as hf_const + try: + import huggingface_hub.constants as hf_const - _saved_hf_const = hf_const.HF_HUB_OFFLINE - hf_const.HF_HUB_OFFLINE = True - except ImportError: - _saved_hf_const = None - try: - import transformers.utils.hub as tf_hub + prev_hf = hf_const.HF_HUB_OFFLINE + hf_const.HF_HUB_OFFLINE = True + except ImportError: + prev_hf = None - _saved_transformers_const = tf_hub._is_offline_mode - tf_hub._is_offline_mode = True - except ImportError: - _saved_transformers_const = None + try: + import transformers.utils.hub as tf_hub - os.environ["HF_HUB_OFFLINE"] = "1" + prev_tf = tf_hub._is_offline_mode + tf_hub._is_offline_mode = True + except ImportError: + prev_tf = None + + os.environ["HF_HUB_OFFLINE"] = "1" + except BaseException: + # Roll back whatever we already changed, then re-raise so + # the caller sees the real failure. + if prev_hf is not None: + try: + import huggingface_hub.constants as hf_const + + hf_const.HF_HUB_OFFLINE = prev_hf + except ImportError: + pass + if prev_tf is not None: + try: + import transformers.utils.hub as tf_hub + + tf_hub._is_offline_mode = prev_tf + except ImportError: + pass + if prev_env is not None: + os.environ["HF_HUB_OFFLINE"] = prev_env + else: + os.environ.pop("HF_HUB_OFFLINE", None) + raise + + _saved_env = prev_env + _saved_hf_const = prev_hf + _saved_transformers_const = prev_tf logger.info( "[offline-guard] %s is cached — forcing offline mode", model_label or "model",