transport/zeromq + channel + minion: async close for req channel reconnect - #70316
Merged
dwoz merged 2 commits intoSep 24, 2026
Merged
Conversation
…nnect
Fixes the root-cause race behind the pyzmq ``Context.__del__`` wedge
in the minion's reconnect path.
The wedge captured live on a production-shape minion had this shape:
RequestClient._init_socket -> self.context = zmq.asyncio.Context()
...
Minion.handle_event / connect_master reconnect fires:
self.req_channel.close() # runs on the ioloop thread
self.req_channel = None
...
~200 s later, Context.__del__ fires from an ioloop callback,
calls destroy() -> zmq_ctx_term(), and the loop wedges
indefinitely.
``RequestClient.close()``'s same-thread + loop-running branch cannot
await the running ``_send_recv`` task (blocking would deadlock the
loop it is on), so it runs ``_sync_teardown`` immediately and
returns. ``_send_recv`` may still be holding a reference to the
socket in its coroutine locals when that teardown runs; the socket
holds a reference to the context; and so the context refcount
does not drop to zero when ``close()`` returns. When
``_send_recv`` finally exits and drops its socket reference,
Python's GC finalizes the Context -- from an ioloop callback --
and pyzmq's default ``__del__`` walks the remaining sockets under
their native LINGER, wedging the loop in ``zmq_ctx_term()``.
Three coordinated changes:
- ``RequestClient.close_async()``: new coroutine that fires the
shutdown sentinel, awaits ``_send_recv_exit_future``, then
releases socket + context deterministically. Context is
destroyed with ``linger=1000`` -- bounded rather than open-ended,
but generous enough that any in-flight REQ traffic on the socket
the context still tracks can flush.
- ``AsyncReqChannel.close_async()``: new coroutine that awaits
``self.transport.close_async()`` when available and falls back
to sync ``close()`` for transports (e.g. TCP) without it.
``__aexit__`` switched over to the async variant so the same
race does not resurface in ``async with`` teardown.
- ``salt.minion`` reconnect sites (``connect_master`` and the
master-changed branch of ``handle_event``): both switched from
sync ``self.req_channel.close()`` to
``await self.req_channel.close_async()`` before the
``self.req_channel = None`` assignment. A ``getattr`` guard
keeps the reconnect code compatible with third-party channel
subclasses that only expose the sync ``close``.
Existing sync ``close()`` behaviour is left unchanged for callers
that cannot yield or are not on an ioloop -- the async path is
strictly additive. Local test verification: regression suite
(``test_context_finalizer_wedge``), ``tests/pytests/unit/transport/``
(256/2/3), and ``tests/pytests/unit/channel/`` (60/0/0) all pass.
twangboy
previously approved these changes
Sep 23, 2026
``test_reauth`` in this file terminates the salt-minion at end
(``stop_event.set()`` -> ``minion_func`` finally ->
``salt_minion.terminate()``). The ``salt_minion`` fixture is
package-scoped and does not auto-restart, so the second test
in the file, ``test_presence_events``, previously ran against
a minion that was no longer there. Every ping returned
``"Minion did not return. [No response]"`` and the master
event log showed ``salt/presence/present {'present': []}``.
Historically this masked because the sync ``req_channel.close()``
in the minion's shutdown path could wedge pyzmq's
``Context.__del__`` in ``zmq_ctx_term()``, keeping the minion
process alive as a zombie for long enough that
``test_presence_events`` still saw a responder.
The reconnect-path close switched from sync ``close()`` to
``await close_async()`` on this branch, so the minion now exits
cleanly and the pre-existing test-ordering bug is exposed on
every runner. Explicitly bring the minion back up at the top
of ``test_presence_events`` before pinging.
Local repro:
pytest tests/pytests/scenarios/reauth/test_reauth.py --run-slow
* before: 1 failed, 1 passed in 328s
* after: 2 passed in 109s
dwoz
added a commit
to dwoz/salt
that referenced
this pull request
Sep 25, 2026
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).
dwoz
added a commit
to dwoz/salt
that referenced
this pull request
Sep 25, 2026
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).
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
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.
Fixes the root-cause race behind the pyzmq
Context.__del__wedge in the minion's reconnect path.The wedge captured live on a production-shape minion had this shape:
RequestClient.close()'s same-thread + loop-running branch cannot await the running_send_recvtask (blocking would deadlock the loop it is on), so it runs_sync_teardownimmediately and returns._send_recvmay still be holding a reference to the socket in its coroutine locals when that teardown runs; the socket holds a reference to the context; and so the context refcount does not drop to zero whenclose()returns. When_send_recvfinally exits and drops its socket reference, Python's GC finalizes the Context -- from an ioloop callback -- and pyzmq's default__del__walks the remaining sockets under their native LINGER, wedging the loop inzmq_ctx_term().Three coordinated changes:
RequestClient.close_async(): new coroutine that fires the shutdown sentinel, awaits_send_recv_exit_future, then releases socket + context deterministically. Context is destroyed withlinger=1000-- bounded rather than open-ended, but generous enough that any in-flight REQ traffic on the socket the context still tracks can flush.AsyncReqChannel.close_async(): new coroutine that awaitsself.transport.close_async()when available and falls back to syncclose()for transports (e.g. TCP) without it.__aexit__switched over to the async variant so the same race does not resurface inasync withteardown.salt.minionreconnect sites (connect_masterand the master-changed branch ofhandle_event): both switched from syncself.req_channel.close()toawait self.req_channel.close_async()before theself.req_channel = Noneassignment. Agetattrguard keeps the reconnect code compatible with third-party channel subclasses that only expose the syncclose.Existing sync
close()behaviour is left unchanged for callers that cannot yield or are not on an ioloop -- the async path is strictly additive. Local test verification: regression suite (test_context_finalizer_wedge),tests/pytests/unit/transport/(256/2/3), andtests/pytests/unit/channel/(60/0/0) all pass.