Restore __del__ safety-net cleanup on SyncWrapper + PublishServer + _… - #70261
Merged
dwoz merged 4 commits intoSep 11, 2026
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
twangboy
previously approved these changes
Sep 10, 2026
…rors
Under heavy master/minion connection churn (observed running the
4-master cluster tests on a 32-CPU Rocky 9 container, probabilistic in
CI), a tornado call inside ``TCPPuller.handle_stream`` intermittently
raises either:
* ``ValueError('fd %s added twice')`` from ``IOLoop.add_handler``
(called from tornado's ``IOStream._add_io_state`` when a stream
tries to register a fd already being tracked by another handler), or
* ``AssertionError('Already reading')`` from ``IOStream.read_bytes``
when a prior read on the same stream is still outstanding. (Older
tornado forks surfaced this as ``StreamAlreadyReadingError``; on
tornado 6.x it is an ``AssertionError``.)
Both were being swallowed by the historical broad-except on line 1957
of ``salt/transport/tcp.py`` -- so the outer ``while not stream.closed()``
loop immediately re-invoked ``stream.read_bytes`` on the same broken
fd, spinning the tornado io_loop at 77-119% CPU and growing the log
file to hundreds of MB in seconds until the CI step timed out. The
``EventPublisher`` pinned; cluster tests hung; CI killed them with
SIGTERM. Deterministic repro on the local Rocky 9 container.
Fix:
1. ``TCPPuller.handle_stream`` narrow-catches the two state errors
(``ValueError``, ``AssertionError``), logs at WARNING with full
traceback, calls ``stream.close()`` (guarded), and ``break``s out
of the reader loop. The prior generic broad-except is also now
break-out rather than continue -- a persistent unrecoverable error
at that level means the per-stream reader can no longer make
progress on the fd, so continuing would spin. The ``OSError``
branch is split: ``errno == 0`` continues (spurious, preserves
existing behavior), any other ``OSError`` closes and breaks
(EBADF / ECONNRESET / etc. mean the fd is unusable).
2. ``_TCPPubServerPublisher.close`` best-effort calls
``stream.io_loop.remove_handler(fd)`` before closing the stream
when the stream still holds a socket. Rationale: when
``stream.connect()`` raised ``fd added twice`` earlier, the fd
was partially registered but the stream's ``_state`` may not
have advanced -- so tornado's own ``stream.close()`` (which only
calls ``remove_handler`` when ``_state is not None``) does not
always deregister the handler, leaving a dangling selector entry
that resurfaces as another ``fd added twice`` the next time the
same fd is reused. Uses ``stream.io_loop`` (a tornado ``IOLoop``)
rather than ``self.io_loop`` (an asyncio loop) because the former
is what tornado registered the handler on.
Unit tests: three new tests in
``tests/pytests/unit/transport/test_tcp.py`` cover the two
handle_stream branches (mock stream whose ``read_bytes`` raises the
state error; assert the reader breaks and closes the stream instead of
retrying) and the publisher close hardening (mock stream + fake io_loop;
assert ``remove_handler(fd)`` is called before ``stream.close()``).
Bundled with PR saltstack#70261's safety-net cleanup work so CI can validate
the combination together.
``_stream_read`` retained a 1 MiB msgpack.Unpacker per accepted stream because the ``client -> _read_task -> coroutine frame -> client`` reference cycle only collected on the next cyclic-GC pass -- under sustained per-job subscriber churn (each state.apply child forks a fresh event-bus subscriber) tracemalloc showed +35 pinned Unpackers ~= +37 MiB retained after only 20 jobs on a live minion. ``_discard_on_close._cb()`` also leaked the per-subscriber ``self._writers`` entry -- an ``asyncio.Queue`` + drain-Task tuple, ~25 kB apiece. ``_discard_slow_client`` already popped this on the drain-timeout path, but the clean-close callback did not. This is the follow-up to PR saltstack#70260 (which cancelled the read task): that alone was not enough because the coroutine frame stayed pinned via the cycle above. Fixes: * ``_stream_read`` gains a ``try/finally`` that does ``del unpacker`` and clears ``client._read_task = None`` so refcount collection reclaims the coroutine frame (and its Unpacker) immediately on exit. * ``_discard_on_close._cb()`` also clears ``client._read_task`` (for the case where the coroutine has not yet resumed to observe the cancellation) and pops the ``self._writers`` entry, cancelling the drain task -- matching what ``_discard_slow_client`` already does. Combined with saltstack#70260 this drops retained per-job Python memory from ~85 kB/job to ~1.8 kB/job (measured with tracemalloc on a live 200-job RaaS-driven burst). Regression tests in ``tests/pytests/unit/transport/`` verify both invariants and were confirmed to fail on ``origin/3008.x`` without this patch. See issue saltstack#70175.
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.