Fix loader race poisoning OS-specific virtualnames

changelog/69806.fixed.md

@@ -0,0 +1 @@
+Fix loader race that could randomly mark OS-specific virtual modules (e.g. ``postgres``) as unavailable when a sibling implementation (e.g. ``deb_postgres``) was evaluated first and poisoned the shared ``__virtualname__`` in the missing-modules cache.

salt/loader/lazy.py

@@ -310,6 +310,13 @@ def __init__(
 
         # names of modules that we don't have (errors, __virtual__, etc.)
         self.missing_modules = {}  # mapping of name -> error
+        # mapping of __virtualname__ -> list of error reasons from every file
+        # that claimed the virtualname and whose __virtual__() returned False.
+        # Kept separate from missing_modules so a failed sibling (e.g.
+        # deb_postgres) does not poison the shared virtualname (e.g. postgres)
+        # and prevent the real module from loading.  Consulted by
+        # missing_fun_string() to surface every failure reason.
+        self.missing_virtualnames = {}
         self.loaded_modules = set()
         self.loaded_files = set()  # TODO: just remove them from file_mapping?
         self.static_modules = static_modules if static_modules else []
@@ -370,6 +377,8 @@ def destroy(self):
             self.loaded_modules.clear()
         if hasattr(self, "missing_modules"):
             self.missing_modules.clear()
+        if hasattr(self, "missing_virtualnames"):
+            self.missing_virtualnames.clear()
 
     def clean_modules(self):
         """
@@ -491,18 +500,34 @@ def missing_fun_string(self, function_name):
         mod_name = function_name.split(".")[0]
         if mod_name in self.loaded_modules:
             return f"'{function_name}' is not available."
-        else:
-            try:
-                reason = self.missing_modules[mod_name]
-            except KeyError:
-                return f"'{function_name}' is not available."
-            else:
-                if reason is not None:
-                    return "'{}' __virtual__ returned False: {}".format(
-                        mod_name, reason
-                    )
-                else:
-                    return f"'{mod_name}' __virtual__ returned False"
+
+        # Collect reasons from missing_modules (keyed by file basename) and
+        # from missing_virtualnames (keyed by shared __virtualname__).  The
+        # latter lets us surface every failure reason when multiple files
+        # collide on a single virtualname (e.g. x509 and x509_v2).
+        reasons = []
+        seen = set()
+        primary = self.missing_modules.get(mod_name, KeyError)
+        if primary is not KeyError and primary is not None:
+            reason_str = str(primary)
+            if reason_str not in seen:
+                seen.add(reason_str)
+                reasons.append(reason_str)
+        for reason in self.missing_virtualnames.get(mod_name, ()):
+            if reason is None:
+                continue
+            reason_str = str(reason)
+            if reason_str not in seen:
+                seen.add(reason_str)
+                reasons.append(reason_str)
+
+        if reasons:
+            return "'{}' __virtual__ returned False: {}".format(
+                mod_name, "; ".join(reasons)
+            )
+        if mod_name in self.missing_modules or mod_name in self.missing_virtualnames:
+            return f"'{mod_name}' __virtual__ returned False"
+        return f"'{function_name}' is not available."
 
     def _refresh_file_mapping(self):
         """
@@ -670,6 +695,7 @@ def clear(self):
             super().clear()  # clear the lazy loader
             self.loaded_files = set()
             self.missing_modules = {}
+            self.missing_virtualnames = {}
             self.loaded_modules = set()
             # if we have been loaded before, lets clear the file mapping since
             # we obviously want a re-do
@@ -1041,26 +1067,28 @@ def _load_module(self, name):
                 # if _process_virtual returned a non-True value then we are
                 # supposed to not process this module
                 if virtual_ret is not True:
-                    # Always record the per-file reason; `name` is unique.
+                    # Record the failure under both the file path (`name`)
+                    # and the file basename (`module_name`).  We intentionally
+                    # do NOT record it under __virtualname__ in
+                    # missing_modules: a sibling file failing (e.g.
+                    # deb_postgres on RHEL) must never poison the shared
+                    # virtualname (e.g. postgres) and block the real module
+                    # from loading (issue #69806).
                     self.missing_modules[name] = virtual_err
-                    # The virtualname (module_name) can collide when multiple
-                    # files declare the same __virtualname__ (e.g. x509 and
-                    # x509_v2 both use "x509"). If we've already recorded a
-                    # reason for this virtualname, append the new one so the
-                    # user sees every failure, not just the first.
                     if module_name not in self.missing_modules:
                         self.missing_modules[module_name] = virtual_err
-                    elif virtual_err is not None:
-                        existing = self.missing_modules[module_name]
-                        if existing is None:
-                            self.missing_modules[module_name] = virtual_err
-                        else:
-                            existing_str = str(existing)
-                            new_str = str(virtual_err)
-                            if new_str and new_str not in existing_str.split("; "):
-                                self.missing_modules[module_name] = (
-                                    f"{existing_str}; {new_str}"
-                                )
+                    # For error-message quality (issue #68625), track every
+                    # failure reason for a shared __virtualname__ in a
+                    # separate structure that missing_fun_string() consults.
+                    virtualname = getattr(mod, "__virtualname__", None)
+                    if (
+                        isinstance(virtualname, str)
+                        and virtualname
+                        and virtualname != module_name
+                    ):
+                        reasons = self.missing_virtualnames.setdefault(virtualname, [])
+                        if virtual_err not in reasons:
+                            reasons.append(virtual_err)
                     return False
         else:
             virtual_aliases = ()
@@ -1309,16 +1337,18 @@ def _process_virtual(self, mod, module_name, virtual_func="__virtual__"):
                             module_name,
                         )
 
-                    # If the module explicitly declares __virtualname__, report
-                    # the failure under that name so the caller can detect
-                    # collisions with other modules claiming the same name.
-                    if (
-                        hasattr(mod, "__virtualname__")
-                        and isinstance(virtualname, str)
-                        and virtualname
-                    ):
-                        module_name = virtualname
-
+                    # NOTE: Do NOT reassign ``module_name`` to the module's
+                    # __virtualname__ on failure here.  Doing so caused the
+                    # caller to poison ``missing_modules[virtualname]`` (issue
+                    # #69806): when a sibling module (e.g. deb_postgres on a
+                    # non-Debian host) failed its __virtual__ check first, the
+                    # real module claiming the same virtualname (postgres) was
+                    # skipped by ``_load()`` because that virtualname was
+                    # already marked missing.  The caller now tracks failure
+                    # reasons per-__virtualname__ separately (see
+                    # ``missing_virtualnames``) so error surfacing for
+                    # collisions (issue #68625) is preserved without the
+                    # poisoning race.
                     return (False, module_name, error_reason, virtual_aliases)
 
                 # At this point, __virtual__ did not return a

tests/pytests/unit/loader/test_lazy.py

@@ -208,11 +208,74 @@ def expires(*args, **kwargs):
     with pytest.raises(KeyError):
         _ = loader["x509.expires"]
 
+    # The primary failure (x509.py has file basename == virtualname) is
+    # recorded in missing_modules; the sibling x509_v2 failure is tracked
+    # separately in missing_virtualnames to avoid poisoning the shared
+    # virtualname for unrelated modules (see #69806).
     reason = loader.missing_modules.get("x509")
     assert reason is not None
-    assert "Superseded, using x509_v2" in reason
-    assert "Could not load cryptography" in reason
+    assert "Superseded, using x509_v2" in str(reason)
+
+    extra = loader.missing_virtualnames.get("x509", [])
+    assert any("Could not load cryptography" in str(r) for r in extra)
 
     msg = loader.missing_fun_string("x509.expires")
     assert "Superseded, using x509_v2" in msg
     assert "Could not load cryptography" in msg
+
+
+def test_virtualname_sibling_failure_does_not_poison_real_module(tmp_path):
+    """
+    A sibling module whose __virtual__() returns False must not poison the
+    shared __virtualname__ in missing_modules and block the real module
+    from loading.
+
+    Regression test for #69806: on non-Debian OSes, deb_postgres.py's
+    __virtual__() returns False and (under the buggy code) recorded the
+    failure under missing_modules["postgres"] via its __virtualname__.  On
+    the next _load("postgres.foo") call the loader early-returned because
+    "postgres" was already marked missing and the real postgres.py module
+    was never loaded.
+    """
+    (tmp_path / "deb_postgres.py").write_text(
+        textwrap.dedent(
+            """
+            __virtualname__ = "postgres"
+
+            def __virtual__():
+                return (False, "Not a Debian host")
+
+            def user_create(*args, **kwargs):
+                return True
+            """
+        )
+    )
+    (tmp_path / "postgres.py").write_text(
+        textwrap.dedent(
+            """
+            __virtualname__ = "postgres"
+
+            def __virtual__():
+                return True
+
+            def user_create(*args, **kwargs):
+                return "real-postgres"
+            """
+        )
+    )
+
+    opts = {"optimization_order": [0, 1, 2]}
+    loader = salt.loader.lazy.LazyLoader([str(tmp_path)], opts)
+
+    # Force the failing sibling to be processed first so we hit the race the
+    # bug produced (non-deterministic directory ordering in the wild).
+    loader._load_module("deb_postgres")
+
+    # The failing sibling must be recorded under its own basename, NOT
+    # under the shared virtualname.
+    assert "deb_postgres" in loader.missing_modules
+    assert "postgres" not in loader.missing_modules
+
+    # The real postgres.py must still load and provide postgres.user_create.
+    fun = loader["postgres.user_create"]
+    assert fun() == "real-postgres"