Skip to content

transport/zeromq + channel + minion: async close for req channel reconnect - #70316

Merged
dwoz merged 2 commits into
saltstack:3008.xfrom
dwoz:vcops-90587/req-channel-close-async-3008.x
Sep 24, 2026
Merged

dwoz merged 2 commits into
saltstack:3008.xfrom
dwoz:vcops-90587/req-channel-close-async-3008.x

Conversation

@dwoz

@dwoz dwoz commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

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.

…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
twangboy previously approved these changes Sep 23, 2026
@twangboy twangboy added this to the Argon v3008.3 milestone 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
dwoz merged commit 68a5096 into saltstack:3008.x Sep 24, 2026
24 checks passed
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

1 active deployment
ci — 412af5c0 Deployed Sep 24, 2026 by dwoz via Build Onedir Packages / RPM (x86_64) #27225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants