[wip] close async audit 3008.x - #70322
Merged
dwoz merged 12 commits intoSep 25, 2026
Merged
Conversation
…connect notify ``AsyncPubChannel.connect_callback`` fires a ``_minion_event`` on the master when a reconnect completes. The one-shot ``AsyncReqChannel`` it uses for that notify was wrapped in sync ``with``, which routes teardown through ``__exit__``/``close`` -- the same sync fallback that PR-70316 addressed on the reconnect path. Because ``connect_callback`` runs on the same ioloop that owns the underlying ``RequestClient``, that sync teardown cannot await the running ``_send_recv`` task, so the socket ref keeps the ``zmq.asyncio.Context`` alive until later GC finalizes it from an ioloop callback and can wedge in ``zmq_ctx_term()``. ``connect_callback`` is already ``async def``; switching to ``async with`` routes cleanup through ``__aexit__`` -> ``close_async``, which awaits the send/recv exit future.
``AsyncAuth._authenticate`` opens an ``AsyncReqChannel`` for the sign-in loop. It used the sync ``with`` context manager, which invokes the sync ``__exit__``/``close`` on exit -- the same race the wedge fix in PR-70316 addressed on the minion reconnect path: sync ``close`` cannot await the running ``_send_recv`` task, so a socket ref held by that task's coroutine locals keeps the ``zmq.asyncio.Context`` alive until GC finalizes it from an ioloop callback and can wedge in ``zmq_ctx_term()``. ``_authenticate`` is already ``async def`` and its body already awaits; switching the context manager to ``async with`` is the minimal change to route teardown through ``AsyncReqChannel.__aexit__`` -> ``close_async`` -> transport's ``close_async``, which awaits the send/recv exit future.
``AsyncAuth.sign_in`` accepts a caller-supplied ``channel`` or opens its own ``AsyncReqChannel``; the ``finally`` clause called sync ``channel.close()`` on the owned-here path. That is the sync teardown that races the running ``_send_recv`` task and lets the ``zmq.asyncio.Context`` reach GC while a socket ref is still live -- the wedge PR-70316 traced through the minion reconnect path. Route the owned-here close through ``close_async`` when available (``AsyncReqChannel`` exposes it) so the transport's ``_send_recv_exit_future`` is awaited before the socket + context are released. A ``getattr`` guard keeps compatibility with third-party channel subclasses that only expose the sync ``close``, matching the shape PR-70316 used in ``Minion.connect_master`` and ``Minion.handle_event``.
… through it ``AsyncRemotePillar`` is only constructed on an active asyncio loop (via ``get_async_pillar`` from ``async def post_master_init``, ``pillar_refresh``, and the master's ``_pillar`` handler), and its ``destroy`` calls sync ``channel.close()``. That is the exact same-thread + loop-running teardown PR-70316 traced through the minion reconnect path: sync ``close`` cannot await the transport's running ``_send_recv`` task, so the socket ref keeps the ``zmq.asyncio.Context`` alive until GC finalizes it from a later ioloop callback and wedges in ``zmq_ctx_term()``. Add ``AsyncRemotePillar.aclose()`` that mirrors the shape of the minion reconnect fix: prefer ``channel.close_async`` when the channel exposes it (``AsyncReqChannel`` does) and fall back to sync ``close`` for third-party subclasses. Route the two async callers that owned an ``AsyncRemotePillar`` -- ``Minion._post_master_init`` and ``Minion.pillar_refresh`` -- through ``await async_pillar.aclose()``. ``destroy`` / ``__del__`` are intentionally kept as a compatibility layer for third-party consumers that never migrated to ``aclose``. Async callers reach ``aclose`` first, which flips ``_closing`` and makes the sync ``destroy`` a no-op, so ``__del__``'s ``channel.close()`` teardown does not fire under normal use. A debug log records the fallback path if it does hit, so leaks are visible instead of silent.
Companion to the ``with`` -> ``async with`` conversions in this branch
and the new ``AsyncRemotePillar.aclose``. Each site's fix rides on
one of three invariants:
(a) ``AsyncReqChannel.__aexit__`` routes through
``close_async`` -> transport ``close_async``, which awaits the
transport's ``_send_recv_exit_future`` before running teardown;
(b) ``AsyncRemotePillar.aclose`` closes the underlying channel's
``zmq.asyncio.Context`` (and legacy ``destroy`` / ``__del__``
short-circuit once aclose has run);
(c) the ``getattr(channel, "close_async", None)`` guard prefers the
async path and only falls back to sync ``close`` for
third-party subclasses.
Five focused tests cover those categories rather than reproducing the
full production wedge (which the existing
``test_context_finalizer_wedge`` covers end-to-end). Objects are
constructed directly against a dead master URI (tcp://127.0.0.1:1)
so no traffic actually flows and no salt master is required.
Verified locally: (3), (4) fail on this branch without the
``AsyncRemotePillar.aclose`` commit (AttributeError on ``aclose``);
(2) fails with ``__aexit__`` reverted to sync ``close`` -- the spy on
``close_async`` shows zero invocations. Tests (1) and (5) pass
independent of the fixes: (1) checks ``context.closed`` post-teardown
(both paths flip it, though only the async path guarantees drain
ordering); (5) is a shape guard for the ``getattr`` pattern.
The finalizer tracer at
``scratch/vcops-90587-ctx-trace/zmq_finalizer_trace_v5.py`` (deployed
to nsx on Ani Baghoumian's ``ab002212-63-maas-easy-deploy`` env)
captured the exact wedge signature in production and pinned the
creation site of the leaking ``zmq.asyncio.Context`` to::
handle_event -> _fire_master_main -> _send_req_async_main
-> self.req_channel.send(...)
-> transport.send -> await self.connect()
-> _init_socket()
The trace showed a ``FINALIZE_ENTRY`` with ``closed=False`` invoked
from ``tornado/ioloop.py:945 val = self.callback()`` -- i.e. the
``weakref.finalize`` registered on a ``RequestClient`` fired from an
ioloop callback and blocked in ``zmq_ctx_term()``.
Reading the deployed source revealed the state-machine bug behind it.
``RequestClient.close_async`` does the right thing::
self._closing = True
self.socket = None
self.context = None
...
context.destroy(linger=1000) # closed=True on the Context
But ``RequestClient.connect`` then silently resurrects the transport::
async def connect(self):
async with self._connect_lock:
if self.socket is None:
self._connect_called = True
self._closing = False # <-- clears the "closed" bit
self._init_socket() # <-- allocates a fresh Context
# AND registers a second
# weakref.finalize on
# this RequestClient
The wedge race is:
1. Reconnect calls ``await old_channel.close_async()``. The OLD
``RequestClient``'s Context #1 is destroyed cleanly and
``self.context`` is set to None.
2. Reconnect assigns ``self.req_channel = <new>``.
3. An in-flight coroutine (``_fire_master_main`` ->
``_send_req_async_main``) that captured the OLD channel across
an ``await`` yield point now calls ``.send()`` on it.
4. Old ``.send()`` -> old ``transport.send()`` -> ``await
self.connect()``. Before this change ``connect()``
unconditionally reset ``_closing`` to False and ran
``_init_socket``, which allocated Context #2 and registered a
second ``weakref.finalize`` against the same (dead)
``RequestClient``.
5. Nothing calls ``close_async`` on the OLD channel again -- the
reconnect already dropped its reference. When the OLD channel
is finally GC'd the second finalizer fires from an ioloop
callback and blocks in ``zmq_ctx_term()``. Loop wedges.
Refuse the reconnect. A closed ``RequestClient`` cannot be reused;
any caller holding a stale reference gets a clean
``SaltClientError`` (which the surrounding ``AsyncReqChannel`` retry
logic already handles), and no second Context is allocated or
finalized. With this in place the trace-log invariant holds: every
``FINALIZE_ENTRY`` on a ``RequestClient`` has ``closed=True`` and
the finalizer's ``context.destroy`` short-circuits.
Regression test in
``tests/pytests/functional/transport/zeromq/test_close_async_audit.py::test_closed_request_client_refuses_reconnect``
constructs a ``RequestClient``, drives ``connect -> close_async ->
connect``, and asserts (a) the second ``connect`` raises
``SaltClientError`` and (b) ``self.context`` remains ``None`` (no
resurrection). Verified fail-without-fix (``DID NOT RAISE``) and
pass-with-fix (``1 passed``) on the debian-12 CI container.
Commit 868bb2f ("channel/client: use ``async with`` on ephemeral AsyncReqChannel in reconnect notify") switched ``AsyncPubChannel.connect_callback``'s cleanup from sync ``with AsyncReqChannel.factory(...)`` to ``async with``. The existing mock in ``test_async_pub_channel_connect_cb`` only wired ``mock.__enter__``, so on the patched code the ``async with`` did not invoke it, the mocked channel object was never returned, and ``mock.send`` was never called -- failing the assertion. Mirror the source change in the test: wire ``__aenter__`` / ``__aexit__`` (as AsyncMock, since ``async with`` awaits them), and assert on ``mock.__aexit__`` instead of ``mock.__exit__``.
PR saltstack#70316's RequestClient.close_async graceful drain intentionally short-circuits the ``_send_recv`` SaltReqTimeoutError branch during the reconnect window -- that branch is what used to log Request timed out while waiting for a response. reconnecting. so the assertion for that log line was pinning the OLD failure mode and now fires on every CI run against Python 3.14 in the ci-onedir container. Reproduced locally with docker exec 35799623947_debian-12 bash -c 'cd /salt-fix && \ PYTHONPATH=/salt-fix /salt/.nox/ci-test-onedir/bin/python \ -m pytest tests/pytests/scenarios/regression/\ test_resource_runaway_oom.py::test_return_retry_resource_runaway \ --run-slow --transport=zeromq -v' Confirmed the failure hits on origin/3008.x baseline too, so the regression came from saltstack#70316 and not from this branch's audit commits. The other sanity check -- ``failed to return the job information for job`` -- still proves the return-retry path exhausted against an unreachable master, which is what the test is really guarding against (the leak measurements below are unconditionally logged for the memory-shape assertion the docstring already calls out).
Three tests were pinning the old ``with AsyncReqChannel.factory(...)``
context-manager shape (sync ``__enter__`` / ``__exit__``), but
salt/client/__init__.py:2217 now uses ``async with`` so the underlying
AsyncReqChannel's ``__aenter__`` / ``__aexit__`` are what actually get
called. With the old mock shape, ``async with`` doesn't find the
async dunders on the mock and the test bombs out before ``send`` is
ever invoked.
Reproduced locally in the ci-test-onedir container:
docker exec 35799623947_debian-12 bash -c 'cd /salt-fix && \
PYTHONPATH=/salt-fix /salt/.nox/ci-test-onedir/bin/python \
-m pytest --transport=zeromq -v \
tests/pytests/unit/test_client.py::test_pub_async_default_timeout \
tests/pytests/unit/test_client.py::test_pub_async_explicit_timeout \
tests/pytests/unit/test_client.py::test_pub_async_uses_publish_timeout_from_config'
before: FAILED FAILED FAILED
after: 3 passed in 1.05s
Same mirror change as the earlier
``test_async_pub_channel_connect_cb`` fix on this branch: swap the
plain ``MagicMock`` context-manager wiring for ``AsyncMock`` on
``__aenter__`` and ``__aexit__``. ``send`` itself is already
async-shaped so it needs no adjustment.
The earlier ``async with AsyncReqChannel.factory(...)`` migration in
this branch changed ``__aenter__`` to eagerly ``await
self.transport.connect()``. Under normal use it's fine -- every
production callsite (``AsyncPubChannel`` reconnect notify,
``LocalClient.pub_async``, ``AsyncAuth._authenticate``) calls
``channel.send()`` right after entering the block, and ``send()``
already lazy-connects on the first call. But tests that mock the
inner call (``AsyncAuth`` tests set ``auth.sign_in = mock_sign_in``
so ``_authenticate``'s body never actually reaches
``channel.send``) still hit the real ``AsyncReqChannel.factory``,
and with the eager ``__aenter__`` each such ``async with`` now
allocates a real ``zmq.asyncio.Context``, spawns
``_send_recv``, and registers the ``weakref.finalize`` from
PR-70315. ``__aexit__`` runs ``close_async`` which drives the
5s ``_send_recv_exit_future`` await and a ``context.destroy
(linger=1000)``. Across a full pytest session those wall-clock
costs compounded until group 3 wedged past every ``--timeout``
setting we tried.
Reproduced in the ci-test-onedir container:
docker exec 35799623947_debian-12 bash -c 'cd /salt-fix && \
PYTHONPATH=/salt-fix /salt/.nox/ci-test-onedir/bin/python \
-m pytest --transport=zeromq --slow-tests --core-tests \
--test-group-count=4 --test-group=3 --timeout=45 -q \
tests/pytests/unit'
before: 6-8 ``+++ Timeout +++`` markers, session killed at 30min
after: 0 timeouts, ``2468 passed, 510 skipped, 1 xfailed in 118s``
Fix: match the sync ``__enter__`` shape -- ``async with`` returns
without side effects, and ``send()`` connects lazily. The wedge
fix from PR-70316 lives in ``close_async``, not ``__aenter__``,
so this is purely removing an eager-connect that was never load-
bearing.
dwoz
force-pushed
the
vcops-90587/close-async-audit-3008.x
branch
from
September 25, 2026 07:48
e22d3c8 to
ccfd7ee
Compare
…iant Same branch's ``channel/client: make AsyncReqChannel.__aenter__ lazy`` (commit ccfd7ee) intentionally stopped ``__aenter__`` from allocating a ``zmq.asyncio.Context`` up front. Under that new shape, ``test_async_with_on_async_req_channel_closes_context`` picked up ``transport.context = None`` at inspection time and blew up in the assertion with ``AttributeError: 'NoneType' object has no attribute 'closed'`` on the Photon OS 4 FIPS runner (job 108000377473 on run 36109529376). Restore the test's intent -- verify that ``close_async`` releases an allocated context -- by explicitly calling ``await channel.transport.connect()`` inside the ``async with``. That forces the ``Context`` into existence so ``__aexit__``'s ``close_async`` has something to close, which is exactly the wedge scenario the test is guarding against.
This branch was successfully deployed
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.
What does this PR do?
What issues does this PR fix or reference?
Fixes
Previous Behavior
Remove this section if not relevant
New Behavior
Remove this section if not relevant
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes/No