[3008.x] Fix minion PublishServer / _TCPPubServerPublisher unclosed leaks (#70175) by dwoz · Pull Request #70206 · saltstack/salt

dwoz · 2026-09-01T22:56:23Z

Symptom

Issue #70175: three ResourceWarnings fire at minion shutdown

WARNING salt.transport.tcp: unclosed publish server <PublishServer ...>
WARNING salt.utils.asynchronous: unclosed SyncWrapper for cls=<_TCPPubServerPublisher>
WARNING salt.transport.tcp: unclosed publisher client <_TCPPubServerPublisher ...>

Root cause

MinionManager._bind creates the local event_publisher (a PublishServer graph) and event (a SaltEvent). Only MinionManager.stop_async (the SIGTERM handler path) closes them. MinionManager.destroy -- reached from cli.daemons.Minion.shutdown on KeyboardInterrupt / SaltSystemExit / early-exit guards, and from __del__ on GC -- did not, so any non-SIGTERM shutdown leaked the graph.

Fix

Add the missing close/destroy chain to MinionManager.destroy, guarded with try/except so a finalizer never propagates.

Test plan

New unit test test_minion_manager_destroy_closes_event_publisher in tests/pytests/unit/test_minion.py asserts destroy() closes event_publisher (_closing=True) and destroys event (subscriber/pusher cleared). Verified to fail against unfixed 3008.x and pass with the fix.
Existing test_minion_manager_async_stop / test_stop_async_calls_notify_stopping_and_terminates_subprocess_list still pass.
Full tests/pytests/unit/test_minion.py: 50 passed, 28 skipped.

…utdown MinionManager.destroy() (invoked from cli.daemons.Minion.shutdown on KeyboardInterrupt / SaltSystemExit / early-exit and from __del__ on GC) was missing the close/destroy chain for the local event_publisher PublishServer graph and the event SaltEvent that MinionManager._bind creates. Only the SIGTERM stop_async path closed them, so any non-SIGTERM shutdown leaked the graph and surfaced the three-warning cascade reported in saltstack#70175 (unclosed publish server / SyncWrapper / publisher client).

…altstack#70175) PublishServer.close iterated the per-loop publisher cache but called stream.close() on each cached _TCPPubServerPublisher rather than pub.close(). The stream FD was released (Bug 1 fix) but pub._closing stayed False, so _TCPPubServerPublisher.__del__ still fired the "unclosed publisher client" ResourceWarning at GC -- the third warning of the cascade twangboy reported on 3008.2+506. Round 1 of this PR closed the outer PublishServer + pub_sock SyncWrapper via MinionManager.destroy (silences warnings 1 and 2); this round covers the raw cached publishers created in the async-context bypass at tcp.py:2242/:2281. _TCPPubServerPublisher.close is idempotent and subsumes the previous stream-only close.

Companion to saltstack#70206 which fixed the shutdown path for the same issue saltstack#70175 symptoms. This handles the steady-state per-job path. Root cause: ``PubServer.handle_stream`` scheduled ``_stream_read`` as an asyncio Task at accept time; the task awaited ``stream.read_bytes(...)`` and its coroutine frame held a 1 MiB msgpack ``Unpacker`` local and a ``client`` local. When the peer closed the connection, ``_discard_on_close`` removed the ``Subscriber`` from ``self.clients`` and cleared presence, but did NOT cancel the Task. If tornado did not translate the FIN to a prompt ``StreamClosedError`` (observed on real 3008.x minions), the Task stayed pending in ``asyncio.all_tasks()`` forever, pinning the ``Unpacker`` buffer, the ``client``, and the ``IOStream`` graph. Tracemalloc on a live 3008.x minion under a 132-job / 5-min mixed load: +140 pinned ``Subscriber`` and +140 pinned ``Unpacker`` instances (~142 MiB retention). Fix: ``handle_stream`` and ``_validate_ssl_and_add_client`` now store the ``_stream_read`` Task on ``Subscriber._read_task``; ``_discard_on_close`` cancels that Task (idempotent) and calls ``client.close()`` (also idempotent) so the coroutine frame releases promptly regardless of whether the FIN raised ``StreamClosedError``. Regression test: ``test_pub_server_discard_on_close_cancels_read_task`` registers 50 subscribers with a never-completing ``read_bytes`` stream (the pathological state), fires the close callback, and asserts zero pending ``_stream_read`` Tasks remain. Fails on unpatched 3008.x (50 leaked tasks), passes with the fix. Related-to: saltstack#70175, saltstack#70206

…0175) ``salt.modules.event.fire_master`` and ``salt.modules.mine._mine_send`` constructed a temporary ``MinionEvent``, called ``fire_event()`` on it, and dropped the reference. With no context manager and no explicit ``destroy()``, cleanup ran only when GC eventually invoked ``SaltEvent.__del__`` -- and each finalization emitted the three-warning triad from the underlying transport chain: unclosed publish server <PublishServer ...> unclosed publisher client <_TCPPubServerPublisher ...> unclosed SyncWrapper for cls=<class '..._TCPPubServerPublisher'> Under mixed-job load on a 3008.x minion this surfaced as ~3 ResourceWarnings per job (issue #70175 reports 646 warnings across ~200 jobs). The FDs backing each unclosed publisher pinned an in-flight IPC socket + IOStream + Unpacker until GC ran, contributing to the RSS growth and unclosed-resource noise flagged in the issue. Fix: wrap the ``MinionEvent`` in a ``with`` block so ``__exit__`` calls ``destroy()`` -> ``close_pub()`` / ``close_pull()`` -> ``SyncWrapper.close()`` synchronously, tearing down the ``_TCPPubServerPublisher`` chain in the same call. Companion to PR #70206 (shutdown-time ``MinionManager`` destroy chain) and the sibling PR on branch ``dwoz/fix/70175-pubserver-perjob-leak`` (server-side ``PubServer._discard_on_close`` fix). This PR closes the third leg: per-job caller-side leaks on the minion. Regression test --------------- New unit test ``test_fire_master_context_managed_no_unclosed_warnings`` in ``tests/pytests/unit/modules/test_event.py`` runs ``salt.modules.event.fire_master`` 50 times and asserts no ``unclosed publish server`` / ``unclosed publisher client`` / ``unclosed SyncWrapper`` warnings are emitted. Pre-patch (verified by stashing this commit's changes to ``salt/modules/event.py`` + ``salt/modules/mine.py`` and re-running):: E AssertionError: fire_master leaked 74 unclosed-resource warnings E across 50 iterations (expected 0). Sample warnings: E - unclosed publisher client <_TCPPubServerPublisher ...> E - unclosed publish server <PublishServer ...> E - unclosed SyncWrapper for cls=<class '..._TCPPubServerPublisher'> FAILED tests/pytests/unit/modules/test_event.py::test_fire_master_context_managed_no_unclosed_warnings Post-patch: PASSED (0 warnings across 50 iterations).

Companion to saltstack#70206 which fixed the shutdown path for the same issue saltstack#70175 symptoms. This handles the steady-state per-job path. Root cause: ``PubServer.handle_stream`` scheduled ``_stream_read`` as an asyncio Task at accept time; the task awaited ``stream.read_bytes(...)`` and its coroutine frame held a 1 MiB msgpack ``Unpacker`` local and a ``client`` local. When the peer closed the connection, ``_discard_on_close`` removed the ``Subscriber`` from ``self.clients`` and cleared presence, but did NOT cancel the Task. If tornado did not translate the FIN to a prompt ``StreamClosedError`` (observed on real 3008.x minions), the Task stayed pending in ``asyncio.all_tasks()`` forever, pinning the ``Unpacker`` buffer, the ``client``, and the ``IOStream`` graph. Tracemalloc on a live 3008.x minion under a 132-job / 5-min mixed load: +140 pinned ``Subscriber`` and +140 pinned ``Unpacker`` instances (~142 MiB retention). Fix: ``handle_stream`` and ``_validate_ssl_and_add_client`` now store the ``_stream_read`` Task on ``Subscriber._read_task``; ``_discard_on_close`` cancels that Task (idempotent) and calls ``client.close()`` (also idempotent) so the coroutine frame releases promptly regardless of whether the FIN raised ``StreamClosedError``. Regression test: ``test_pub_server_discard_on_close_cancels_read_task`` registers 50 subscribers with a never-completing ``read_bytes`` stream (the pathological state), fires the close callback, and asserts zero pending ``_stream_read`` Tasks remain. Fails on unpatched 3008.x (50 leaked tasks), passes with the fix. Related-to: saltstack#70175, saltstack#70206

Companion to #70206 which fixed the shutdown path for the same issue #70175 symptoms. This handles the steady-state per-job path. Root cause: ``PubServer.handle_stream`` scheduled ``_stream_read`` as an asyncio Task at accept time; the task awaited ``stream.read_bytes(...)`` and its coroutine frame held a 1 MiB msgpack ``Unpacker`` local and a ``client`` local. When the peer closed the connection, ``_discard_on_close`` removed the ``Subscriber`` from ``self.clients`` and cleared presence, but did NOT cancel the Task. If tornado did not translate the FIN to a prompt ``StreamClosedError`` (observed on real 3008.x minions), the Task stayed pending in ``asyncio.all_tasks()`` forever, pinning the ``Unpacker`` buffer, the ``client``, and the ``IOStream`` graph. Tracemalloc on a live 3008.x minion under a 132-job / 5-min mixed load: +140 pinned ``Subscriber`` and +140 pinned ``Unpacker`` instances (~142 MiB retention). Fix: ``handle_stream`` and ``_validate_ssl_and_add_client`` now store the ``_stream_read`` Task on ``Subscriber._read_task``; ``_discard_on_close`` cancels that Task (idempotent) and calls ``client.close()`` (also idempotent) so the coroutine frame releases promptly regardless of whether the FIN raised ``StreamClosedError``. Regression test: ``test_pub_server_discard_on_close_cancels_read_task`` registers 50 subscribers with a never-completing ``read_bytes`` stream (the pathological state), fires the close callback, and asserts zero pending ``_stream_read`` Tasks remain. Fails on unpatched 3008.x (50 leaked tasks), passes with the fix. Related-to: #70175, #70206

…ltstack#70175) ``salt.utils.error.fire_exception`` -- called from ``salt/minion.py:_thread_return`` (unhandled-exception path) and from ``salt/metaproxy/{proxy,deltaproxy}.py`` -- constructed a bare ``salt.utils.event.SaltEvent``, called ``fire_event()`` on it, and dropped the reference. With no context manager and no explicit ``destroy()``, cleanup ran only when GC eventually invoked ``SaltEvent.__del__`` and each finalisation emitted the three-warning triad from the underlying transport chain: unclosed publish server <PublishServer ...> unclosed publisher client <_TCPPubServerPublisher ...> unclosed SyncWrapper for cls=<class '..._TCPPubServerPublisher'> Fix: wrap the ``SaltEvent`` in a ``with`` block so ``__exit__`` calls ``destroy()`` -> ``close_pub()`` / ``close_pull()`` -> ``SyncWrapper.close()`` synchronously, tearing down the ``_TCPPubServerPublisher`` chain in the same call. Companion to PR saltstack#70206 (shutdown-time ``MinionManager`` destroy chain) and the sibling per-job caller fixes on branches ``dwoz/fix/70175-pubserver-perjob-leak`` (server-side ``PubServer._discard_on_close`` fix) and ``dwoz/fix/70175-saltevent-caller-close`` (``salt.modules.event.fire_master`` and ``salt.modules.mine._mine_send``). This PR closes the receive-path leg: the ``fire_exception`` helper on the minion's job-error return path. Regression test --------------- New unit test ``test_fire_exception_context_managed_no_unclosed_warnings`` in ``tests/pytests/unit/utils/test_error.py`` runs ``salt.utils.error.fire_exception`` 50 times and asserts no ``unclosed publish server`` / ``unclosed publisher client`` / ``unclosed SyncWrapper`` warnings surface after ``gc.collect()``. Pre-patch (verified by ``git stash push salt/utils/error.py`` and re-running):: AssertionError: fire_exception leaked 75 unclosed-resource warnings across 50 iterations (expected 0). Sample warnings: - unclosed publisher client <_TCPPubServerPublisher ...> - unclosed publisher client <_TCPPubServerPublisher ...> ... Post-patch: PASSED (0 warnings across 50 iterations).

…TCPPubServerPublisher (saltstack#70175) Extends the "warn + fall back to close()" pattern from commit 9955b89 (``salt.utils.event.SaltEvent.__del__``) to the three sub-classes ``SaltEvent`` composes with: * ``salt.utils.asynchronous.SyncWrapper.__del__`` * ``salt.transport.tcp.PublishServer.__del__`` * ``salt.transport.tcp._TCPPubServerPublisher.__del__`` Each ``__del__`` still emits the ``ResourceWarning`` via ``salt.utils.resource_warnings.warn_until_close`` (so leaky callers keep surfacing for pre-Potassium tracking), then falls back to ``close()`` wrapped in try/except so a finalizer never propagates. For ``PublishServer.close`` the individual sub-resource close steps (``pub_sock``, ``pub_server``, ``pull_sock``, ``io_loop.stop``, ``io_loop.close``) are additionally guarded, because they can raise during GC-time execution when the io_loop is in a partially torn-down state -- which is exactly the failure mode driving ~50 MB/hr RSS growth on the minion under sustained event traffic. Companion to sibling branches ``dwoz/fix/70175-pubserver-perjob-leak``, ``dwoz/fix/70175-saltevent-caller-close``, ``dwoz/fix/70175-receive-path-saltevent`` and to shutdown-path fix saltstack#70206. The ``master`` (Potassium) branch drops the ``close()`` fallback and requires explicit ``close()`` / context-manager use; the loud ``ResourceWarning`` here is the migration signal for that change. Regression tests: * tests/pytests/unit/utils/test_asynchronous.py::test_syncwrapper_del_safety_net_calls_close_70175 * tests/pytests/unit/transport/test_tcp.py::test_publish_server_del_safety_net_calls_close_70175 * tests/pytests/unit/transport/test_tcp.py::test_tcppubserverpublisher_del_safety_net_calls_close_70175 Each test: 1. Instantiates the class without ``with``, drops the reference, forces GC 2. Asserts the ``ResourceWarning`` still fires (behavior preserved) 3. Asserts a class-appropriate observable that only ``close()`` would set (asyncio_loop.is_closed() for SyncWrapper; sub-resource ``close()`` mock calls for PublishServer; stream+socket close for _TCPPubServerPublisher) All three tests fail on pre-patch origin/3008.x with only the ``ResourceWarning`` firing, and pass with the safety-net restored. Refs saltstack#70175, saltstack#70206.

…0175) ``salt.utils.error.fire_exception`` -- called from ``salt/minion.py:_thread_return`` (unhandled-exception path) and from ``salt/metaproxy/{proxy,deltaproxy}.py`` -- constructed a bare ``salt.utils.event.SaltEvent``, called ``fire_event()`` on it, and dropped the reference. With no context manager and no explicit ``destroy()``, cleanup ran only when GC eventually invoked ``SaltEvent.__del__`` and each finalisation emitted the three-warning triad from the underlying transport chain: unclosed publish server <PublishServer ...> unclosed publisher client <_TCPPubServerPublisher ...> unclosed SyncWrapper for cls=<class '..._TCPPubServerPublisher'> Fix: wrap the ``SaltEvent`` in a ``with`` block so ``__exit__`` calls ``destroy()`` -> ``close_pub()`` / ``close_pull()`` -> ``SyncWrapper.close()`` synchronously, tearing down the ``_TCPPubServerPublisher`` chain in the same call. Companion to PR #70206 (shutdown-time ``MinionManager`` destroy chain) and the sibling per-job caller fixes on branches ``dwoz/fix/70175-pubserver-perjob-leak`` (server-side ``PubServer._discard_on_close`` fix) and ``dwoz/fix/70175-saltevent-caller-close`` (``salt.modules.event.fire_master`` and ``salt.modules.mine._mine_send``). This PR closes the receive-path leg: the ``fire_exception`` helper on the minion's job-error return path. Regression test --------------- New unit test ``test_fire_exception_context_managed_no_unclosed_warnings`` in ``tests/pytests/unit/utils/test_error.py`` runs ``salt.utils.error.fire_exception`` 50 times and asserts no ``unclosed publish server`` / ``unclosed publisher client`` / ``unclosed SyncWrapper`` warnings surface after ``gc.collect()``. Pre-patch (verified by ``git stash push salt/utils/error.py`` and re-running):: AssertionError: fire_exception leaked 75 unclosed-resource warnings across 50 iterations (expected 0). Sample warnings: - unclosed publisher client <_TCPPubServerPublisher ...> - unclosed publisher client <_TCPPubServerPublisher ...> ... Post-patch: PASSED (0 warnings across 50 iterations).

…TCPPubServerPublisher (saltstack#70175) Extends the "warn + fall back to close()" pattern from commit 9955b89 (``salt.utils.event.SaltEvent.__del__``) to the three sub-classes ``SaltEvent`` composes with: * ``salt.utils.asynchronous.SyncWrapper.__del__`` * ``salt.transport.tcp.PublishServer.__del__`` * ``salt.transport.tcp._TCPPubServerPublisher.__del__`` Each ``__del__`` still emits the ``ResourceWarning`` via ``salt.utils.resource_warnings.warn_until_close`` (so leaky callers keep surfacing for pre-Potassium tracking), then falls back to ``close()`` wrapped in try/except so a finalizer never propagates. For ``PublishServer.close`` the individual sub-resource close steps (``pub_sock``, ``pub_server``, ``pull_sock``, ``io_loop.stop``, ``io_loop.close``) are additionally guarded, because they can raise during GC-time execution when the io_loop is in a partially torn-down state -- which is exactly the failure mode driving ~50 MB/hr RSS growth on the minion under sustained event traffic. Companion to sibling branches ``dwoz/fix/70175-pubserver-perjob-leak``, ``dwoz/fix/70175-saltevent-caller-close``, ``dwoz/fix/70175-receive-path-saltevent`` and to shutdown-path fix saltstack#70206. The ``master`` (Potassium) branch drops the ``close()`` fallback and requires explicit ``close()`` / context-manager use; the loud ``ResourceWarning`` here is the migration signal for that change. Regression tests: * tests/pytests/unit/utils/test_asynchronous.py::test_syncwrapper_del_safety_net_calls_close_70175 * tests/pytests/unit/transport/test_tcp.py::test_publish_server_del_safety_net_calls_close_70175 * tests/pytests/unit/transport/test_tcp.py::test_tcppubserverpublisher_del_safety_net_calls_close_70175 Each test: 1. Instantiates the class without ``with``, drops the reference, forces GC 2. Asserts the ``ResourceWarning`` still fires (behavior preserved) 3. Asserts a class-appropriate observable that only ``close()`` would set (asyncio_loop.is_closed() for SyncWrapper; sub-resource ``close()`` mock calls for PublishServer; stream+socket close for _TCPPubServerPublisher) All three tests fail on pre-patch origin/3008.x with only the ``ResourceWarning`` firing, and pass with the safety-net restored. Refs saltstack#70175, saltstack#70206.

dwoz requested a review from a team as a code owner September 1, 2026 22:56

dwoz added the test:full Run the full test suite label Sep 1, 2026

dwoz had a problem deploying to ci September 1, 2026 22:56 — with GitHub Actions Error

dwoz added this to the Argon v3008.3 milestone Sep 1, 2026

dwoz temporarily deployed to ci September 1, 2026 22:56 — with GitHub Actions Inactive

twangboy assigned dwoz Sep 1, 2026

twangboy previously approved these changes Sep 1, 2026

View reviewed changes

dwoz temporarily deployed to ci September 1, 2026 23:10 — with GitHub Actions Inactive

dwoz temporarily deployed to ci September 1, 2026 23:28 — with GitHub Actions Inactive

dwoz dismissed twangboy’s stale review via 668cfcd September 8, 2026 21:08

dwoz temporarily deployed to ci September 8, 2026 21:08 — with GitHub Actions Inactive

dwoz temporarily deployed to ci September 8, 2026 21:22 — with GitHub Actions Inactive

dwoz temporarily deployed to ci September 8, 2026 21:42 — with GitHub Actions Inactive

This was referenced Sep 10, 2026

Cancel PubServer read task on stream close to release per-job leak #70257

Merged

Wrap fire-and-forget MinionEvent callers to close SaltEvent chain (#7… #70258

Merged

dwoz merged commit e262256 into saltstack:3008.x Sep 10, 2026
2665 of 2726 checks passed

dwoz mentioned this pull request Sep 10, 2026

Wrap fire-and-forget SaltEvent in salt.utils.error.fire_exception #70259

Merged

dwoz mentioned this pull request Sep 10, 2026

Restore __del__ safety-net cleanup on SyncWrapper + PublishServer + _… #70261

Open

Navigation Menu

Uh oh!

[3008.x] Fix minion PublishServer / _TCPPubServerPublisher unclosed leaks (#70175) - #70206

[3008.x] Fix minion PublishServer / _TCPPubServerPublisher unclosed leaks (#70175)#70206
dwoz merged 2 commits into
saltstack:3008.xfrom
dwoz:dwoz/fix/70175-minion-unclosed-leaks

dwoz commented Sep 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dwoz commented Sep 1, 2026

Symptom

Root cause

Fix

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants