Reap asyncio Tasks on SyncWrapper.run_sync to prevent per-call retention - #70171
Open
dwoz wants to merge 3 commits into
Open
Reap asyncio Tasks on SyncWrapper.run_sync to prevent per-call retention#70171dwoz wants to merge 3 commits into
dwoz wants to merge 3 commits into
Conversation
Refs saltstack#70169. SyncWrapper._target installs self.asyncio_loop as the current asyncio loop on the worker thread but drives the wrapped coroutine through tornado's io_loop.run_sync. Any asyncio.Task created inside the wrapped coroutine that doesn't complete within the run_sync window is left pending on self.asyncio_loop, pinning its coroutine + contextvars.Context until close() is called. Marked xfail(strict=True) until the fix commit lands.
Fixes saltstack#70169. SyncWrapper._target installs self.asyncio_loop as the current asyncio loop on the worker thread and drives the wrapped coroutine through tornado's io_loop.run_sync. Any asyncio.Task the wrapped coroutine schedules on the current asyncio loop and does not await -- pyzmq's future-based sockets and tornado's asyncio bridge do this internally -- survives past the run_sync window, pinning its coroutine and contextvars.Context until close(). Long-lived driver processes (EventReturn, BatchManager) don't call close() in steady state, so the retention accumulates for the process lifetime. A 26-hour observation on a live master saw ~18k retained Task/coroutine/Context triples on EventReturn (0.36 MB/hr). After run_sync returns, cancel all pending tasks on asyncio_loop and drive the loop until they finish, matching the pattern already used in close(). The regression test xfail marker added by the previous commit is removed.
Follow-up to the reap-on-run_sync fix. The previous commit blanket-cancelled every pending asyncio.Task on ``asyncio_loop`` after ``run_sync`` returned, which broke clients like ``salt.transport.tcp.PublishClient`` that intentionally keep a persistent ``_read_into_unpacker`` task in flight across multiple ``recv()`` calls. Cancelling that task mid-lifecycle left ``tornado.iostream.IOStream._read_future`` set, and the next ``recv()`` failed with ``AssertionError: Already reading``. Symptom on CI (test:full run 33132150880): uniform failures across all Linux distros in ``functional zeromq 1``, ``integration zeromq 1/5/7``, ``scenarios zeromq``, and ``unit zeromq 4``. Concrete minimal reproduction was ``tests/pytests/unit/test_minion.py:: test_minion_manager_async_stop`` which drives ``SaltEvent.subscriber`` (a ``SyncWrapper(ipc_publish_client)``) through two consecutive ``recv()`` calls -- the second raised ``Already reading``. Long-running integration jobs (``integration zeromq 5`` at 2h10m, ``scenarios zeromq`` at 1h30m) hit the same root cause but manifested as ``test.ping`` / ``mine.get`` / ``publish.publish`` timing out because the master's own event subscriber recv loop was silently torn down after each dispatch. Only cancel tasks that (a) did not exist before this ``run_sync`` call, and (b) have no strong user-object referrer -- i.e. weren't stored as an attribute on the wrapped object. Filter referrers by type: internal asyncio / GC machinery (``coroutine``, ``Task``, ``Future``, ``FrameType``, ``TaskStepMethWrapper``, containers) doesn't count as ownership, but a ``PublishClient`` / ``AsyncReqChannel`` / etc. holding the task as ``self._read_task`` does. The regression test from saltstack#70169 continues to pass -- its ``_AsyncioTaskScheduler.schedule_and_return`` fires ``loop.create_task(_child())`` without storing the returned task, so ``_child`` has no user-object referrer and is correctly reaped.
twangboy
approved these changes
Aug 31, 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.
Summary
SyncWrapper._targetatsalt/utils/asynchronous.py:270installsself.asyncio_loopas the current asyncio loop on the worker thread withasyncio.set_event_loop(asyncio_loop), then drives the wrapped coroutine through tornado'sio_loop.run_sync. Anyasyncio.Taskthe wrapped coroutine schedules on the current asyncio loop and does not await -- pyzmq's future-based sockets and tornado's asyncio bridge fire tasks on the current asyncio loop internally -- survives past therun_syncwindow and pins its coroutine +contextvars.Contextuntilclose()is called. Long-lived driver processes (EventReturn,BatchManager) don't callclose()in steady state, so the retention accumulates for the process lifetime.Evidence from a 26-hour observation on a live master (gdb-injected
PyRun_SimpleString(gc.get_objects())):EventReturnretained 18,107asyncio.Task+ 18,109coroutine+ 18,129contextvars.Contextat a 1:1:1 ratio.EventReturn; 0.30 MB/hr onBatchManager.EventPublishercontrol process (noSyncWrapper) had zero retention -- confirms the mechanism isSyncWrapper-specific.After
io_loop.run_syncreturns in_target, cancel all pending tasks onasyncio_loopand drive the loop until they finish, matching the pattern already used inclose(). Same set of tasks that would eventually be reaped atclose()are reaped after every dispatch instead. Gated on_loop_can_run_until_complete(asyncio_loop)so shutdown-race edges match theclose()path.Fixes #70169
Affected files
salt/utils/asynchronous.py--_targetnow reaps pending tasks in afinallyblock afterrun_syncreturns.tests/pytests/unit/utils/test_asynchronous.py-- new regression testtest_sync_wrapper_reaps_pending_tasks_after_run_sync; xfail marker added in the previous commit is removed by the fix commit.changelog/70169.fixed.md-- towncrier entry.Test plan
venv314/bin/pytest tests/pytests/unit/utils/test_asynchronous.py -v-- 7 passed (including the flipped xfail).venv314/bin/pytest tests/pytests/unit/utils/ --ignore=tests/pytests/unit/utils/test_vmware.py -q-- 2147 passed, 385 skipped. 2 unrelated pre-existing failures (test_pycrypto::test_gen_hash_passlib[blowfish-expected2],verify/test_verify.py::test_verify_socket) confirmed to fail on origin/3008.x too.venv314/bin/pre-commit run --files salt/utils/asynchronous.py tests/pytests/unit/utils/test_asynchronous.py changelog/70169.fixed.md-- all hooks pass.