Tolerate concurrent sys.path_importer_cache races in the loader (#61734)

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.

salt/loader/lazy.py

@@ -759,20 +759,31 @@ 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
         # sys.path_importer_cache, that requires the removal of
         # 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

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.