Restore __del__ safety-net cleanup on SyncWrapper + PublishServer + _… - #70261
Open
dwoz wants to merge 2 commits into
Open
Restore __del__ safety-net cleanup on SyncWrapper + PublishServer + _…#70261dwoz wants to merge 2 commits into
dwoz wants to merge 2 commits into
Conversation
dwoz
force-pushed
the
dwoz/fix/70175-del-cleanup-safety-net
branch
from
September 10, 2026 08:22
d8c41b9 to
f5e3aad
Compare
dwoz
force-pushed
the
dwoz/fix/70175-del-cleanup-safety-net
branch
from
September 10, 2026 09:09
f5e3aad to
d26bbfe
Compare
…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.
The __del__ safety-net close() added in this PR broke transports on fork: EventSender (and every job child) inherits parent transports via copy-on-write, and on GC-time __del__ the child was calling close() on socket FDs the parent still owned, tearing down the parent's event bus. Reproduced deterministically on Python 3.14 as tests/pytests/unit/utils/event/test_event.py::test_event_no_timeout, which hangs 90s and times out. The EventSender child, on exit, GC'd the inherited MasterEvent.subscriber SyncWrapper and closed the shared ipc_publish_client FD; the parent's recv() then blocked forever. Also observed in the integration zeromq/tcp and scenarios clusters as 6-minute SIGTERMs on event-dependent cluster tests. Fix: apply the fork-safety pattern used by salt/transport/tcp.py::Subscriber / TCPPuller (and the sibling transport-fork-safety branch dwoz/fix/transport-fork-safety-warn-until- close-3008.x) to the three classes whose __del__ now calls close(): * salt.utils.asynchronous.SyncWrapper * salt.transport.tcp.PublishServer * salt.transport.tcp._TCPPubServerPublisher __init__ records self._creator_pid = os.getpid(). __del__ short- circuits (no warn, no close) when os.getpid() != self._creator_pid -- the parent still owns the wrapped obj / io_loop / asyncio_loop / socket FDs; the child must not touch them. SyncWrapper.__del__ uses self.__dict__.get(...) rather than getattr() for the probes: __getattr__ delegates to self.obj, so a partially- initialized instance would recurse infinitely through __getattr__ while the finalizer is running. Regression tests added, one per class: * test_syncwrapper_del_forked_child_does_not_touch_parent_resources_70175 * test_publish_server_del_forked_child_does_not_close_parent_fds_70175 * test_tcppubserverpublisher_del_forked_child_does_not_close_parent_fd_70175 Each simulates os.getpid() returning creator_pid + 1 (as it would in a fork() child) and asserts (a) no ResourceWarning fires and (b) no close() call reaches the sub-resources.
dwoz
force-pushed
the
dwoz/fix/70175-del-cleanup-safety-net
branch
from
September 10, 2026 21:02
3ab58da to
1f7613c
Compare
twangboy
approved these changes
Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…TCPPubServerPublisher (#70175)
Extends the "warn + fall back to close()" pattern from commit 9955b89 (
salt.utils.event.SaltEvent.__del__) to the three sub-classesSaltEventcomposes with:salt.utils.asynchronous.SyncWrapper.__del__salt.transport.tcp.PublishServer.__del__salt.transport.tcp._TCPPubServerPublisher.__del__Each
__del__still emits theResourceWarningviasalt.utils.resource_warnings.warn_until_close(so leaky callers keep surfacing for pre-Potassium tracking), then falls back toclose()wrapped in try/except so a finalizer never propagates. ForPublishServer.closethe 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-salteventand to shutdown-path fix #70206. Themaster(Potassium) branch drops theclose()fallback and requires explicitclose()/ context-manager use; the loudResourceWarninghere is the migration signal for that change.Regression tests:
Each test:
with, drops the reference, forces GCResourceWarningstill fires (behavior preserved)close()would set (asyncio_loop.is_closed() for SyncWrapper; sub-resourceclose()mock calls for PublishServer; stream+socket close for _TCPPubServerPublisher)All three tests fail on pre-patch origin/3008.x with only the
ResourceWarningfiring, and pass with the safety-net restored.Refs #70175, #70206.