diff --git a/changelog/70175.fixed.md b/changelog/70175.fixed.md new file mode 100644 index 00000000000..4a531142e03 --- /dev/null +++ b/changelog/70175.fixed.md @@ -0,0 +1 @@ +Fixed leak of the minion's local ``PublishServer`` graph (``event_publisher`` -> ``pub_sock`` SyncWrapper -> ``_TCPPubServerPublisher``) when the minion exits through ``cli.daemons.Minion.shutdown`` (KeyboardInterrupt, SaltSystemExit, early-exit guards) or ``MinionManager`` GC. ``MinionManager.destroy`` now closes ``event_publisher`` and destroys ``event`` -- previously only the SIGTERM ``stop_async`` path did, so non-SIGTERM shutdown paths triggered the three-warning cascade in issue #70175. diff --git a/salt/minion.py b/salt/minion.py index 8292806508a..1951fbe79e2 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -1553,6 +1553,33 @@ def destroy(self): if hasattr(minion, "destroy"): minion.destroy() self.minions = [] + # Close the local event publisher and event bus. ``stop_async`` + # (invoked from the SIGTERM signal handler) already does this, + # but ``destroy`` is *also* reached from + # ``cli.daemons.Minion.shutdown`` (KeyboardInterrupt / SaltSystemExit + # / early-exit ``shutdown(1)`` guards) and from ``__del__`` on GC. + # Without this the ``PublishServer`` graph created in ``_bind`` + # (``event_publisher`` -> ``pub_sock`` SyncWrapper -> + # ``_TCPPubServerPublisher``) leaks at process exit, surfacing as + # the three-warning cascade in issue #70175. + if getattr(self, "event_publisher", None) is not None: + try: + self.event_publisher.close() + except Exception: # pylint: disable=broad-except + log.debug( + "Error closing event_publisher during MinionManager.destroy", + exc_info=True, + ) + self.event_publisher = None + if getattr(self, "event", None) is not None: + try: + self.event.destroy() + except Exception: # pylint: disable=broad-except + log.debug( + "Error destroying event during MinionManager.destroy", + exc_info=True, + ) + self.event = None def _create_minion_object( self, diff --git a/tests/pytests/unit/test_minion.py b/tests/pytests/unit/test_minion.py index de28616b416..f87932ad1c6 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -2045,6 +2045,56 @@ async def test_minion_manager_async_stop(io_loop, minion_opts, tmp_path): assert mm.event is None +async def test_minion_manager_destroy_closes_event_publisher( + io_loop, minion_opts, tmp_path +): + """ + Regression test for issue #70175. + + ``MinionManager.destroy()`` is invoked from + ``cli.daemons.Minion.shutdown()`` (KeyboardInterrupt, SaltSystemExit, + the ``shutdown(1)`` guard in ``prepare()``) and from + ``MinionManager.__del__`` on GC. It must close the ``event_publisher`` + ``PublishServer`` graph -- otherwise the three-warning cascade + from #70175 fires at interpreter shutdown: + + - ``unclosed publish server `` + - ``unclosed SyncWrapper for cls=<_TCPPubServerPublisher>`` + - ``unclosed publisher client <_TCPPubServerPublisher>`` + + Only the ``stop_async`` shutdown path (invoked from the SIGTERM + signal handler) used to close these; ``destroy()`` did not, so any + non-SIGTERM exit leaked them. + """ + minion_opts["sock_dir"] = str(tmp_path / "sock") + os.makedirs(minion_opts["sock_dir"]) + + mm = salt.minion.MinionManager(minion_opts) + mm._bind() + assert mm.event_publisher is not None + assert mm.event is not None + + # Wait for pub server to bind so the underlying PublishServer graph + # is fully constructed. + while not list(pathlib.Path(minion_opts["sock_dir"]).glob("*")): + await tornado.gen.sleep(0.1) + + ep = mm.event_publisher + ev = mm.event + + # Call destroy directly (the buggy path). Post-fix it must close + # both resources and null the references. + mm.destroy() + + assert mm.event_publisher is None + assert mm.event is None + # PublishServer.close() sets _closing=True so __del__ won't warn. + assert ep._closing is True + # SaltEvent.destroy() closes pusher / subscriber and clears them. + assert ev.subscriber is None + assert ev.pusher is None + + def test_minion_io_loop_is_asyncio_loop(minion_opts): """ Test that Minion io_loop is converted to asyncio.AbstractEventLoop.