Do not rebuild a warning whose first argument is not its message

changelog/1372.bugfix.rst

@@ -0,0 +1,5 @@
+Warnings crossing to the controller now keep the state they carry beside their ``args``, and a warning that cannot be rebuilt no longer ends the run.
+
+Only ``args`` was transferred and the warning was rebuilt by calling the class with them, so a warning deriving its message from its own fields arrived rendered from defaults. The reduce state is transferred now and the instance is rebuilt without re-running ``__init__``; when that state cannot cross, the class is dropped rather than rebuilt without it.
+
+Resolving the warning's class is also guarded. ``importlib.import_module`` and the ``getattr`` that follows it run in the controller's receiver thread, and anything they raised ended the session, reporting a scheduler error that named nothing leading back to the import. Both degrade to a generic ``Warning`` carrying the reason, and the category degrades to ``None``. This fixes `#404 <https://github.com/pytest-dev/pytest-xdist/issues/404>`__.

src/xdist/remote.py

@@ -324,6 +324,48 @@ def pytest_warning_recorded(
         )
 
 
+def _serializable_reduce_state(message: Warning) -> tuple[Any | None, bool]:
+    """Return `(state, lost)` for the state `__reduce__` carries.
+
+    `lost` is the half that matters, and conflating it with a `None` state was a
+    defect of its own. A warning that keeps nothing beside its `args` reduces to a
+    two-tuple: there is no state, nothing is lost, and rebuilding the class is
+    exact. A warning that keeps state we cannot send is a different situation
+    entirely -- rebuilding the class then produces an instance missing the
+    attributes its own `__str__` reads, which raises `AttributeError` when the
+    controller renders it. The caller must be able to tell those apart, and it
+    can only do so if we say which one this is.
+
+    A class that replaces `__reduce__` with its own callable counts as lost. We
+    will not call something arbitrary from the payload, and pretending its state
+    was empty produces exactly the broken instance described above.
+    """
+    try:
+        reduced = message.__reduce__()
+    except Exception:
+        return None, True
+    if not isinstance(reduced, tuple) or len(reduced) < 2:
+        # `__reduce__` may return a string, and anything we cannot read is not
+        # something to guess at.
+        return None, True
+    if reduced[0] is not type(message):
+        # A custom callable, and the length tells us nothing: such a class commonly
+        # reduces to a two-tuple that carries its state inside the *args*. Checking
+        # the length first read that as "no state", which is how this shape came
+        # back missing the attributes its `__str__` needs.
+        return None, True
+    if len(reduced) < 3:
+        return None, False
+    state = reduced[2]
+    if state is None:
+        return None, False
+    try:
+        execnet.dumps(state)
+    except execnet.DumpError:
+        return None, True
+    return state, False
+
+
 def serialize_warning_message(
     warning_message: warnings.WarningMessage,
 ) -> dict[str, Any]:
@@ -339,11 +381,21 @@ def serialize_warning_message(
             message_args = None
         else:
             message_args = warning_message.message.args
+        # `args` alone does not describe a warning. `BaseException.__reduce__`
+        # carries the instance dictionary as a third element, and a class may
+        # replace `__reduce__` entirely. Sending only `args` drops whatever the
+        # instance keeps beside them, and the controller then rebuilds something
+        # that renders plausibly and is not the same warning.
+        message_state, message_state_lost = _serializable_reduce_state(
+            warning_message.message
+        )
     else:
         message_str = warning_message.message
         message_module = None
         message_class_name = None
         message_args = None
+        message_state = None
+        message_state_lost = False
     if warning_message.category:
         category_module = warning_message.category.__module__
         category_class_name = warning_message.category.__name__
@@ -356,6 +408,8 @@ def serialize_warning_message(
         "message_module": message_module,
         "message_class_name": message_class_name,
         "message_args": message_args,
+        "message_state": message_state,
+        "message_state_lost": message_state_lost,
         "category_module": category_module,
         "category_class_name": category_class_name,
     }

src/xdist/workermanage.py

@@ -476,18 +476,69 @@ def process_from_remote(
             self.notify_inproc("errordown", node=self, error=excinfo)
 
 
+def _restore_warning_state(message: BaseException, state: Any) -> None:
+    """Apply the state `__reduce__` carried, in the two shapes it comes in.
+
+    `BaseException` implements `__setstate__` as a `setattr` per key, which is
+    the branch pickle takes for exceptions. A class using `__slots__` reduces to
+    a `(dict, slots)` two-tuple instead, and that one has to be applied by hand.
+    """
+    setstate = getattr(message, "__setstate__", None)
+    if isinstance(state, tuple) and len(state) == 2:
+        instance_dict, slot_state = state
+        if instance_dict:
+            if setstate is not None:
+                setstate(instance_dict)
+            else:
+                message.__dict__.update(instance_dict)
+        for key, value in (slot_state or {}).items():
+            setattr(message, key, value)
+        return
+    if setstate is not None:
+        setstate(state)
+    else:
+        message.__dict__.update(state)
+
+
 def unserialize_warning_message(data: dict[str, Any]) -> warnings.WarningMessage:
     import importlib
 
     if data["message_module"]:
-        mod = importlib.import_module(data["message_module"])
-        cls = getattr(mod, data["message_class_name"])
+        # Resolving the class means importing an arbitrary module, and this runs in
+        # the controller's receiver thread. A package that raises on import -- for
+        # any reason of its own -- used to end the session here: the thread died,
+        # the node went down mid-test, and the run failed in the scheduler with
+        # `KeyError: <WorkerController gwN>`, two steps away and naming nothing that
+        # leads back to this line. The fallback below already exists for a warning
+        # we cannot rebuild; an unimportable module is the same situation.
+        cls: type[Warning] | None
+        unresolved = ""
+        try:
+            mod = importlib.import_module(data["message_module"])
+            cls = getattr(mod, data["message_class_name"])
+        except Exception as exc:
+            cls = None
+            unresolved = f"{type(exc).__name__}: {exc}"
         message = None
-        if data["message_args"] is not None:
+        if (
+            cls is not None
+            and data["message_args"] is not None
+            and not data.get("message_state_lost")
+        ):
+            # Rebuilt without running `__init__`. A warning is free to derive its
+            # message from its own fields, and calling the class with the args
+            # would hand it back its rendered text as if it were input: the
+            # instance then renders around its own output, or silently falls back
+            # to whatever its defaults say. `BaseException.__setstate__` restores
+            # the rest, which is the half we used not to send at all.
             try:
-                message = cls(*data["message_args"])
-            except TypeError:
-                pass
+                message = cls.__new__(cls)
+                BaseException.__init__(message, *data["message_args"])
+                state = data.get("message_state")
+                if state is not None:
+                    _restore_warning_state(message, state)
+            except Exception:
+                message = None
         if message is None:
             # could not recreate the original warning instance;
             # create a generic Warning instance with the original
@@ -497,13 +548,22 @@ def unserialize_warning_message(data: dict[str, Any]) -> warnings.WarningMessage
                 cls=data["message_class_name"],
                 msg=data["message_str"],
             )
+            if cls is None:
+                # Say why. Widening a guard around an import is how a genuinely
+                # broken package turns into a warning nobody can explain.
+                message_text = f"{message_text} (class not resolved: {unresolved})"
             message = Warning(message_text)
     else:
         message = data["message_str"]
 
     if data["category_module"]:
-        mod = importlib.import_module(data["category_module"])
-        category = getattr(mod, data["category_class_name"])
+        # Same exposure, and no guard at all until now. `category` has a `None`
+        # branch already, so an unresolvable one degrades instead of ending the run.
+        try:
+            mod = importlib.import_module(data["category_module"])
+            category = getattr(mod, data["category_class_name"])
+        except Exception:
+            category = None
     else:
         category = None
 

testing/acceptance_test.py

@@ -867,6 +867,40 @@ def test_func(request):
         result = pytester.runpytest(n)
         result.stdout.fnmatch_lines(["*MyWarning*", "*1 passed, 1 warning*"])
 
+    @pytest.mark.parametrize("n", ["-n0", "-n1"])
+    def test_state_beyond_args_survives(
+        self, pytester: pytest.Pytester, n: str
+    ) -> None:
+        """A warning keeping state beside its args must report the same either way.
+
+        `-n0` is the reference: no serialization happens, so whatever it prints is
+        what the warning says. `-n1` must match it. The class below renders its
+        message from `self.code`, which `args` does not carry, so rebuilding it by
+        calling the class reports the default instead — and reads like a real
+        message while doing it.
+        """
+        pytester.makepyfile(
+            """
+            import warnings
+
+            class CodeWarning(UserWarning):
+
+                def __init__(self, code=None):
+                    self.code = code
+                    super().__init__()
+
+                def __str__(self):
+                    return "code {} tripped".format(self.code or "unknown")
+
+            def test_func():
+                warnings.warn(CodeWarning(42))
+        """
+        )
+        pytester.syspathinsert()
+        result = pytester.runpytest(n)
+        result.stdout.fnmatch_lines(["*code 42 tripped*", "*1 passed, 1 warning*"])
+        result.stdout.no_fnmatch_line("*code unknown tripped*")
+
     @pytest.mark.parametrize("n", ["-n0", "-n1"])
     def test_unserializable_arguments(self, pytester: pytest.Pytester, n: str) -> None:
         """Check that warnings with unserializable arguments are handled correctly (#349)."""