From 9f7aa705d5eda127f759c98bafbccf69faf9c135 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Wed, 2 Sep 2026 14:37:49 -0400 Subject: [PATCH] Let salt-proxy finish its graceful shutdown (#70217) ProxyMinion._handle_signals asked MinionManager to stop and then immediately called the parent signal handler as well. stop() does not shut anything down; it schedules stop_async on the io_loop and hands it the parent handler to invoke once the graceful shutdown has finished. So the process exited before the io_loop could run the task, and Python reported "coroutine 'MinionManager.stop_async' was never awaited" on every SIGTERM. The 5 second grace period that lets the minion flush its final return messages to the master lives in stop_async, so those were lost too. Minion._handle_signals already only calls the parent handler when there is no stop method to defer to; do the same here. _terminate_subprocess_list also read proc.pid inside an except OSError. With multiprocessing disabled the entries are threading.Thread objects, which have no pid, and AttributeError is not an OSError, so it escaped and aborted the teardown before destroy() ran. Skip entries that cannot be signalled and carry on with the rest. --- changelog/70217.fixed.md | 1 + salt/cli/daemons.py | 13 +++- salt/minion.py | 12 +++- .../pytests/unit/cli/test_daemons_signals.py | 55 ++++++++++++++++ tests/pytests/unit/test_minion.py | 62 +++++++++++++++++++ 5 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 changelog/70217.fixed.md create mode 100644 tests/pytests/unit/cli/test_daemons_signals.py diff --git a/changelog/70217.fixed.md b/changelog/70217.fixed.md new file mode 100644 index 000000000000..f4ee7a2a1c36 --- /dev/null +++ b/changelog/70217.fixed.md @@ -0,0 +1 @@ +Fixed ``salt-proxy`` exiting before the graceful shutdown it had just scheduled could run. ``MinionManager.stop()`` only schedules ``stop_async`` on the io_loop and hands it the parent signal handler to invoke when it is done, but the proxy daemon also called the parent handler itself, so the process exited first and Python reported ``coroutine 'MinionManager.stop_async' was never awaited``. Final job returns were never flushed. Also stopped a ``threading.Thread`` entry in ``SubprocessList``, which is what jobs are when ``multiprocessing`` is disabled, from aborting the teardown with ``AttributeError`` before ``destroy()`` ran. diff --git a/salt/cli/daemons.py b/salt/cli/daemons.py index 9e2124cb8f7b..b21bd5d19aea 100644 --- a/salt/cli/daemons.py +++ b/salt/cli/daemons.py @@ -434,8 +434,17 @@ class ProxyMinion( def _handle_signals(self, signum, sigframe): # pylint: disable=unused-argument # escalate signal to the process manager processes - self.minion.stop(signum, super()._handle_signals) - super()._handle_signals(signum, sigframe) + if hasattr(self.minion, "stop"): + # ``stop`` only *schedules* ``stop_async`` on the io_loop and hands + # it the parent handler to run once the graceful shutdown is done. + # Calling the parent handler here as well exits the process + # immediately, so the io_loop never gets to run ``stop_async`` -- + # Python reports it as "coroutine 'MinionManager.stop_async' was + # never awaited" -- and nothing is torn down or flushed. This + # mirrors what ``Minion._handle_signals`` above already does. + self.minion.stop(signum, super()._handle_signals) + else: + super()._handle_signals(signum, sigframe) # pylint: disable=no-member def prepare(self): diff --git a/salt/minion.py b/salt/minion.py index 8292806508a9..cf529dd730c7 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -464,11 +464,19 @@ def _terminate_subprocess_list(subprocess_list, signum, grace_seconds=2.0): if not salt.utils.platform.is_windows(): for proc in procs: + # With ``multiprocessing: False`` the entries are + # ``threading.Thread`` objects, which have no ``pid`` and cannot be + # signalled. Reading ``.pid`` there raises ``AttributeError``, + # which is not an ``OSError``, so it escaped and aborted the whole + # teardown before ``destroy()`` ever ran. + pid = getattr(proc, "pid", None) + if pid is None: + continue try: - os.kill(proc.pid, signum) + os.kill(pid, signum) except OSError as exc: if exc.errno not in (errno.ESRCH, errno.EACCES): - log.warning("Failed to signal job child pid %s: %s", proc.pid, exc) + log.warning("Failed to signal job child pid %s: %s", pid, exc) deadline = time.time() + grace_seconds for proc in procs: diff --git a/tests/pytests/unit/cli/test_daemons_signals.py b/tests/pytests/unit/cli/test_daemons_signals.py new file mode 100644 index 000000000000..7b8f3f8ca156 --- /dev/null +++ b/tests/pytests/unit/cli/test_daemons_signals.py @@ -0,0 +1,55 @@ +""" +Signal handling for the salt-proxy daemon. +""" + +import signal + +import salt.cli.daemons +import salt.utils.parsers +from tests.support.mock import MagicMock, patch + + +def _proxy_daemon(minion): + """ + A ProxyMinion daemon instance without running the option-parser __init__. + """ + daemon = salt.cli.daemons.ProxyMinion.__new__(salt.cli.daemons.ProxyMinion) + daemon.minion = minion + return daemon + + +def test_proxy_minion_signal_leaves_exit_to_the_graceful_stop(): + """ + ``MinionManager.stop()`` only *schedules* ``stop_async`` on the io_loop and + hands it the parent signal handler to run once the graceful shutdown has + finished. Calling the parent handler here as well exits the process + immediately, so the io_loop never runs ``stop_async`` -- Python reports it + as "coroutine 'MinionManager.stop_async' was never awaited" -- and nothing + is flushed or torn down. ``Minion._handle_signals`` already gets this + right; the proxy daemon did not. + """ + minion = MagicMock() + daemon = _proxy_daemon(minion) + + with patch.object(salt.utils.parsers.DaemonMixIn, "_handle_signals") as parent: + daemon._handle_signals(signal.SIGTERM, None) + + # The graceful stop is asked for ... + assert minion.stop.called + # ... and the process is NOT torn down out from under it. + assert not parent.called + + +def test_proxy_minion_signal_still_exits_without_a_stop_method(): + """ + Inverse: when the minion has no ``stop`` there is no graceful path to wait + for, so the parent handler must still run -- the guard must not leave the + daemon unable to exit on a signal. + """ + minion = MagicMock(spec=[]) + daemon = _proxy_daemon(minion) + + with patch.object(salt.utils.parsers.DaemonMixIn, "_handle_signals") as parent: + daemon._handle_signals(signal.SIGTERM, None) + + assert parent.called diff --git a/tests/pytests/unit/test_minion.py b/tests/pytests/unit/test_minion.py index de28616b4165..1bc363fc88e0 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -2441,3 +2441,65 @@ async def _instant_sleep(_): # code path; the .destroy() call would try to tear down channels # we never created. A best-effort close is enough. pass + + +def test_terminate_subprocess_list_tolerates_thread_entries(): + """ + With ``multiprocessing: False`` the entries in ``SubprocessList`` are + ``threading.Thread`` objects, which have no ``pid`` and cannot be + signalled. Reading ``.pid`` raised ``AttributeError``, which is not an + ``OSError``, so it escaped this helper and aborted the graceful shutdown + before ``destroy()`` ever ran. + """ + import threading + + started = threading.Event() + release = threading.Event() + + def _worker(): + started.set() + release.wait(5) + + thread = threading.Thread(target=_worker) + thread.start() + started.wait(5) + + class _SubprocessList: + processes = [thread] + + try: + # Must not raise. Before the fix this blew up with AttributeError. + salt.minion._terminate_subprocess_list( + _SubprocessList(), signal.SIGTERM, grace_seconds=0.1 + ) + finally: + release.set() + thread.join(5) + + +def test_terminate_subprocess_list_still_signals_real_processes(): + """ + Inverse of the above: an entry that really is a process must still be + signalled, so skipping the pid-less thread entries cannot turn the whole + helper into a no-op. + """ + + class _FakeProc: + pid = 4242 + + def is_alive(self): + return True + + def join(self, timeout=None): + return None + + class _SubprocessList: + processes = [_FakeProc()] + + with patch("os.kill") as kill_mock: + salt.minion._terminate_subprocess_list( + _SubprocessList(), signal.SIGTERM, grace_seconds=0.1 + ) + + assert kill_mock.called + assert kill_mock.call_args[0][0] == 4242