diff --git a/changelog/61734.fixed.md b/changelog/61734.fixed.md new file mode 100644 index 000000000000..a3cd7daebbc4 --- /dev/null +++ b/changelog/61734.fixed.md @@ -0,0 +1 @@ +Fixed a ``KeyError`` (and possible ``RuntimeError``) from ``importlib.invalidate_caches`` when many minions run in one process -- for example a deltaproxy hosting many sub-proxy minions -- and load modules concurrently. They share the global ``sys.path_importer_cache``, so ``invalidate_caches`` (called by the loader's ``__clean_sys_path`` after each module load) could raise when another loader had already removed an entry it was walking, which failed the module function that triggered the cleanup. The loader now tolerates that shared-cache race, and removes stale ``sys.path_importer_cache`` entries with ``pop`` instead of ``del`` so a concurrent removal cannot raise. diff --git a/salt/loader/lazy.py b/salt/loader/lazy.py index 677371f6734a..6b7e9dcdb72e 100644 --- a/salt/loader/lazy.py +++ b/salt/loader/lazy.py @@ -759,8 +759,20 @@ def __clean_sys_path(self): self._clean_module_dirs = [] # Be sure that sys.path_importer_cache do not contains any - # invalid FileFinder references - importlib.invalidate_caches() + # invalid FileFinder references. + # + # When several loaders run in the same process and load modules + # concurrently -- e.g. a deltaproxy hosting many sub-proxy minions -- + # they share the global sys.path_importer_cache. Another loader may + # already have invalidated or removed an entry that CPython's + # invalidate_caches() is walking, which surfaces as a KeyError (or a + # RuntimeError if the mapping changed size mid-iteration). Tolerate it + # rather than failing the module load that triggered the cleanup + # (#61734); the entry was going to be invalidated anyway. + try: + importlib.invalidate_caches() + except (KeyError, RuntimeError): + pass # Because we are mangling with importlib, we can find from # time to time an invalidation issue with @@ -768,11 +780,10 @@ def __clean_sys_path(self): # FileFinder that remain None for the extra_module_dirs if invalidate_path_importer_cache: for directory in self.extra_module_dirs: - if ( - directory in sys.path_importer_cache - and sys.path_importer_cache[directory] is None - ): - del sys.path_importer_cache[directory] + if sys.path_importer_cache.get(directory) is None: + # pop() rather than del: a concurrent loader may already + # have removed this entry (same shared-cache race as above). + sys.path_importer_cache.pop(directory, None) def _load_module(self, name): mod = None diff --git a/tests/pytests/unit/loader/test_lazy.py b/tests/pytests/unit/loader/test_lazy.py index cf4cebd3a64c..379c96b6ecba 100644 --- a/tests/pytests/unit/loader/test_lazy.py +++ b/tests/pytests/unit/loader/test_lazy.py @@ -11,6 +11,7 @@ import salt.loader.context import salt.loader.lazy import salt.utils.files +from tests.support.mock import patch @pytest.fixture @@ -38,6 +39,29 @@ def get_opts(key): yield str(tmp_path) +def test_clean_sys_path_tolerates_shared_cache_race(loader_dir): + """ + Regression test for #61734. + + When several loaders run in one process and load modules concurrently -- + e.g. a deltaproxy hosting many sub-proxy minions -- they share the global + ``sys.path_importer_cache``. ``importlib.invalidate_caches()``, called by + ``__clean_sys_path`` after each module load, can then raise ``KeyError`` + (or ``RuntimeError`` if the mapping changed size mid-iteration) when another + loader already removed an entry it is walking. ``__clean_sys_path`` must + tolerate that rather than fail the module load that triggered the cleanup. + """ + opts = {"optimization_order": [0]} + loader = salt.loader.lazy.LazyLoader([loader_dir], opts) + for exc in ( + KeyError("/x"), + RuntimeError("dictionary changed size during iteration"), + ): + with patch("importlib.invalidate_caches", side_effect=exc): + # Must not propagate. + loader._LazyLoader__clean_sys_path() + + def test_loaders_have_uniq_context(loader_dir): """ Loaded functions run in the LazyLoader's context.