Let salt-proxy finish its graceful shutdown (#70217)

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.

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):

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:

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

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