Remove GC-time __del__ cleanup: require explicit close / context manager

changelog/70122.fixed.md

@@ -0,0 +1 @@
+Route unclosed-resource ``ResourceWarning`` finalizers through Salt's logger in addition to Python's ``warnings`` module. Python filters ``ResourceWarning`` by default, so a bare ``warnings.warn(..., ResourceWarning)`` from a ``__del__`` finalizer is silently dropped in production and callers that missed a ``close()`` / ``destroy()`` / context-manager contract never see the migration signal. Adds ``salt.utils.resource_warnings.warn_until_close`` which emits both the ``ResourceWarning`` *and* a WARNING-level log record, and wires it into the ``__del__`` finalizers in ``salt/utils/event.py``, ``salt/utils/asynchronous.py``, ``salt/transport/tcp.py``, and ``salt/transport/ws.py``. Also fixes four ``warnings.warn`` sites in ``tcp.py`` / ``ws.py`` where a missing ``f`` prefix rendered ``{self!r}`` as a literal instead of interpolating.

changelog/70122.removed.md

@@ -0,0 +1 @@
+Removes the GC-time ``__del__`` cleanup fallback for ``salt.utils.event.SaltEvent`` (and related client classes ``salt.minion.MasterMinion`` / ``salt.runner.RunnerClient`` / ``salt.wheel.WheelClient``, which have not had a ``__del__`` since commit ``0c3f53d9172``). Users must now explicitly call ``.destroy()`` / ``.close()`` or use the class as a context manager (``with`` block); relying on Python's garbage collector to close the underlying event socket, IO loop or ZMQ context is no longer supported. Missing-``destroy()`` sites are still surfaced by a ``ResourceWarning`` **and** a WARNING-level Salt log record via ``salt.utils.resource_warnings.warn_until_close`` (see the companion routing change), so leaks show up in operator logs at the default log level instead of being silenced by Python's default ``ResourceWarning`` filter. The LTS branches (3006.x / 3007.x / 3008.x, PR #70100) keep the ``destroy()`` fallback alongside the loud warning so out-of-tree consumers that historically relied on GC-time cleanup (e.g. sseape's fire-and-forget ``get_master_event(...).fire_event(...)`` pattern) do not silently leak sockets while migrating.

salt/transport/tcp.py

@@ -20,7 +20,6 @@
 import time
 import urllib
 import uuid
-import warnings
 
 import tornado
 import tornado.concurrent
@@ -39,6 +38,7 @@
 import salt.utils.msgpack
 import salt.utils.platform
 import salt.utils.process
+import salt.utils.resource_warnings
 import salt.utils.versions
 from salt.exceptions import SaltClientError, SaltReqTimeoutError
 from salt.utils.network import ip_bracket
@@ -1227,8 +1227,8 @@ def close(self):
     # pylint: disable=W1701
     def __del__(self):
         if not self._closing:
-            warnings.warn(
-                f"unclosed publish subscriber {self!r}", ResourceWarning, source=self
+            salt.utils.resource_warnings.warn_until_close(
+                f"unclosed publish subscriber {self!r}", source=self, log=log
             )
 
     # pylint: enable=W1701
@@ -1577,7 +1577,9 @@ def close(self):
     # pylint: disable=W1701
     def __del__(self):
         if not self._closing:
-            warnings.warn(f"unclosed tcp puller {self!r}", ResourceWarning, source=self)
+            salt.utils.resource_warnings.warn_until_close(
+                f"unclosed tcp puller {self!r}", source=self, log=log
+            )
 
     # pylint: enable=W1701
 
@@ -1827,8 +1829,8 @@ def close(self):
     # pylint: disable=W1701
     def __del__(self):
         if not self._closing:
-            warnings.warn(
-                f"unclosed publish server {self!r}", ResourceWarning, source=self
+            salt.utils.resource_warnings.warn_until_close(
+                f"unclosed publish server {self!r}", source=self, log=log
             )
 
     # pylint: enable=W1701
@@ -1986,8 +1988,8 @@ def close(self):
     # pylint: disable=W1701
     def __del__(self):
         if not self._closing:
-            warnings.warn(
-                "unclosed publisher client {self!r}", ResourceWarning, source=self
+            salt.utils.resource_warnings.warn_until_close(
+                f"unclosed publisher client {self!r}", source=self, log=log
             )
 
     # pylint: enable=W1701

salt/transport/ws.py

@@ -4,7 +4,6 @@
 import os
 import socket
 import time
-import warnings
 
 import aiohttp
 import aiohttp.web
@@ -14,6 +13,7 @@
 import salt.transport.base
 import salt.transport.frame
 import salt.utils.asynchronous
+import salt.utils.resource_warnings
 from salt.transport.tcp import (
     USE_LOAD_BALANCER,
     LoadBalancerServer,
@@ -104,8 +104,8 @@ async def _async_cleanup(self):
     # pylint: disable=W1701
     def __del__(self):
         if not self._closing:
-            warnings.warn(
-                "unclosed publish client {self!r}", ResourceWarning, source=self
+            salt.utils.resource_warnings.warn_until_close(
+                f"unclosed publish client {self!r}", source=self, log=log
             )
 
     # pylint: enable=W1701
@@ -723,8 +723,8 @@ def get_master_uri(self, opts):
     # pylint: disable=W1701
     def __del__(self):
         if not self._closing:
-            warnings.warn(
-                "Unclosed publish client {self!r}", ResourceWarning, source=self
+            salt.utils.resource_warnings.warn_until_close(
+                f"unclosed publish client {self!r}", source=self, log=log
             )
 
     # pylint: enable=W1701

salt/utils/asynchronous.py

@@ -11,6 +11,8 @@
 import tornado.concurrent
 import tornado.ioloop
 
+import salt.utils.resource_warnings
+
 log = logging.getLogger(__name__)
 
 
@@ -295,3 +297,39 @@ def __exit__(self, exc_type, exc_val, tb):
         if hasattr(self.obj, "__aexit__"):
             self._wrap("__aexit__")(exc_type, exc_val, tb)
         self.close()
+
+    # pylint: disable=W1701
+    def __del__(self):
+        # Deliberately do NOT close the wrapped ``obj`` / io_loop /
+        # asyncio_loop from ``__del__``.  ``__del__`` fires during GC
+        # (may be arbitrarily delayed, may skip on reference cycles)
+        # and during interpreter shutdown, when the world is already
+        # tearing down and touching a tornado/asyncio loop can raise
+        # from a partially-freed C extension.  Instead, emit a
+        # ``ResourceWarning`` (routed through the Salt logger so it
+        # survives Python's default ``ResourceWarning`` filter) so
+        # callers that missed ``close()`` / context-manager surface
+        # loudly in tests / sentry / log aggregators.
+        #
+        # Unlike the LTS branch, ``master`` does NOT fall back to
+        # ``self.close()`` here -- callers MUST use a context manager
+        # or explicit ``close()``.  The WARNING-level log record is the
+        # migration signal.
+        try:
+            unclosed = getattr(self, "obj", None) is not None or (
+                getattr(self, "asyncio_loop", None) is not None
+                and not self.asyncio_loop.is_closed()
+            )
+        except Exception:  # pylint: disable=broad-except
+            return
+        if not unclosed:
+            return
+        salt.utils.resource_warnings.warn_until_close(
+            f"unclosed {type(self).__name__} for cls="
+            f"{getattr(self, 'cls', None)!r}; call ``close()`` or "
+            f"use as a context manager",
+            source=self,
+            log=log,
+        )
+
+    # pylint: enable=W1701

salt/utils/event.py

@@ -74,6 +74,7 @@
 import salt.utils.metrics
 import salt.utils.platform
 import salt.utils.process
+import salt.utils.resource_warnings
 import salt.utils.stringutils
 import salt.utils.tracing
 import salt.utils.zeromq
@@ -273,6 +274,31 @@ def __enter__(self):
     def __exit__(self, exc_type, exc_val, exc_tb):
         self.destroy()
 
+    # pylint: disable=W1701
+    def __del__(self):
+        # ``__del__`` runs at GC / interpreter shutdown and can't safely
+        # close sockets from a partially-freed C extension.  Emit a
+        # ``ResourceWarning`` (routed through the Salt logger so it
+        # survives Python's default filter) so callers that skipped
+        # ``destroy()`` / context-manager surface loudly.
+        try:
+            unclosed = (
+                getattr(self, "subscriber", None) is not None
+                or getattr(self, "pusher", None) is not None
+            )
+        except Exception:  # pylint: disable=broad-except
+            return
+        if not unclosed:
+            return
+        salt.utils.resource_warnings.warn_until_close(
+            f"unclosed {type(self).__name__} {self!r}; call "
+            f"``destroy()`` or use as a context manager",
+            source=self,
+            log=log,
+        )
+
+    # pylint: enable=W1701
+
     def __init__(
         self,
         node,

salt/utils/resource_warnings.py

@@ -0,0 +1,53 @@
+"""
+Helpers for surfacing "unclosed resource" warnings through both Python's
+``warnings`` module and Salt's logging pipeline.
+"""
+
+import logging
+import warnings
+
+_LOGGER = logging.getLogger(__name__)
+
+
+def warn_until_close(message, source, category=ResourceWarning, log=None):
+    """
+    Emit ``category`` for an unclosed resource AND log the same message
+    at WARNING level so it survives Python's default warnings filter.
+
+    ``ResourceWarning`` is filtered out by Python's default warnings
+    filter, so a bare ``warnings.warn(..., ResourceWarning)`` from a
+    ``__del__`` finalizer is silently dropped in production.  Callers
+    that missed a ``close()`` / ``destroy()`` / context-manager contract
+    therefore never see the warning, and the leaked resource
+    accumulates invisibly.
+
+    (Concrete incident: after Salt commit ``0c3f53d9172`` removed the
+    ``__del__``-based cleanup from ``SaltEvent`` / ``MasterMinion`` /
+    ``RunnerClient`` / ``WheelClient`` in favor of a
+    ``ResourceWarning``-emitting ``__del__``, out-of-tree consumers
+    like SSEAPE that relied on GC-time cleanup via
+    ``get_master_event(...).fire_event(...)`` began leaking one unix
+    socket per fire-and-forget instance -- but the intended
+    ``ResourceWarning`` was never visible because ``ResourceWarning`` is
+    silenced by default in production Python.)
+
+    Emitting a WARNING-level log record alongside the warning makes the
+    leak visible in normal Salt logs regardless of the operator's
+    warnings-filter setting.  Callers should pass their module-local
+    ``log`` so records are attributed to the right module; the
+    utility's own logger is the fallback.
+
+    Called from ``__del__`` finalizers -- must never raise.
+    """
+    try:
+        warnings.warn(message, category, source=source)
+    except Exception:  # pylint: disable=broad-except
+        # ``warnings.warn`` can raise during interpreter shutdown when
+        # the ``warnings`` module has already been torn down.  A
+        # finalizer must not propagate exceptions.
+        pass
+    try:
+        (log or _LOGGER).warning(message)
+    except Exception:  # pylint: disable=broad-except
+        # Same rationale for the logging module.
+        pass

tests/pytests/unit/utils/test_resource_warnings.py

@@ -0,0 +1,109 @@
+"""
+Unit tests for :mod:`salt.utils.resource_warnings`.
+"""
+
+import logging
+import warnings
+
+import salt.utils.resource_warnings
+
+
+def test_warn_until_close_emits_resource_warning_and_logs(caplog):
+    """
+    ``warn_until_close`` must emit a ``ResourceWarning`` *and* log at
+    WARNING level.  The log record survives Python's default warnings
+    filter (which drops ``ResourceWarning``) and is what makes leak
+    signals visible in production Salt logs.
+    """
+    logger = logging.getLogger("salt.test.resource_warning")
+    src = object()
+    with warnings.catch_warnings(record=True) as caught, caplog.at_level(
+        logging.WARNING, logger=logger.name
+    ):
+        warnings.simplefilter("always")
+        salt.utils.resource_warnings.warn_until_close(
+            "unclosed something-42", source=src, log=logger
+        )
+
+    # ResourceWarning emitted
+    assert len(caught) == 1
+    assert issubclass(caught[0].category, ResourceWarning)
+    assert "unclosed something-42" in str(caught[0].message)
+    assert caught[0].source is src
+
+    # Log record also produced at WARNING level, same message
+    matches = [r for r in caplog.records if "unclosed something-42" in r.getMessage()]
+    assert matches, "message must appear in log records"
+    assert matches[0].levelno == logging.WARNING
+
+
+def test_warn_until_close_uses_module_logger_when_no_log_passed(caplog):
+    """
+    Missing ``log`` argument must fall back to
+    ``salt.utils.resource_warnings``'s own logger.
+    """
+    with caplog.at_level(logging.WARNING, logger="salt.utils.resource_warnings"):
+        salt.utils.resource_warnings.warn_until_close(
+            "unclosed no-log-passed", source=object()
+        )
+    assert any("unclosed no-log-passed" in r.getMessage() for r in caplog.records)
+
+
+def test_warn_until_close_swallows_warnings_module_failure(monkeypatch, caplog):
+    """
+    The helper is called from ``__del__`` finalizers -- it must not
+    raise even if ``warnings.warn`` itself raises (which happens during
+    interpreter shutdown when the ``warnings`` module has been torn
+    down).  The log record must still be emitted.
+    """
+
+    def _bang(*args, **kwargs):
+        raise RuntimeError("warnings module torn down")
+
+    monkeypatch.setattr(salt.utils.resource_warnings.warnings, "warn", _bang)
+    logger = logging.getLogger("salt.test.resource_warning.warn_fail")
+    with caplog.at_level(logging.WARNING, logger=logger.name):
+        # Must not raise.
+        salt.utils.resource_warnings.warn_until_close(
+            "unclosed warnings-broken", source=object(), log=logger
+        )
+    assert any(
+        "unclosed warnings-broken" in r.getMessage() for r in caplog.records
+    ), "log must be produced even when warnings.warn raises"
+
+
+def test_warn_until_close_swallows_log_failure(caplog):
+    """
+    Same finalizer-safety guarantee for the logging path.  If the
+    passed logger raises, the call must return without propagating.
+    """
+
+    class _BrokenLogger:
+        def warning(self, *args, **kwargs):
+            raise RuntimeError("logger torn down")
+
+    # Must not raise.
+    with warnings.catch_warnings(record=True) as caught:
+        warnings.simplefilter("always")
+        salt.utils.resource_warnings.warn_until_close(
+            "unclosed log-broken", source=object(), log=_BrokenLogger()
+        )
+    # ResourceWarning still emitted despite log failure.
+    assert any("unclosed log-broken" in str(w.message) for w in caught)
+
+
+def test_warn_until_close_accepts_custom_category():
+    """
+    ``category`` defaults to ``ResourceWarning`` but callers can pass
+    another warning class (e.g. ``DeprecationWarning``) if they want to
+    reuse the helper for a different signal.
+    """
+    with warnings.catch_warnings(record=True) as caught:
+        warnings.simplefilter("always")
+        salt.utils.resource_warnings.warn_until_close(
+            "custom-category test",
+            source=object(),
+            category=DeprecationWarning,
+        )
+    assert len(caught) == 1
+    assert issubclass(caught[0].category, DeprecationWarning)