Fail fast on unresolvable master DNS in forked minion workers

changelog/69659.fixed.md

@@ -0,0 +1 @@
+Fixed a minion process leak where forked per-job worker processes would hang forever in the ``resolve_dns`` retry loop when the master hostname was unresolvable, accumulating over time and exhausting memory. Workers now fail fast on an unresolvable master (the ``retry_dns``/``retry_dns_count`` infinite-retry behavior of the main minion process is unchanged and still bounded opt-in via ``retry_dns_count``).

doc/ref/configuration/minion.rst

@@ -383,6 +383,12 @@ Set the number of attempts to perform when resolving
 the master hostname if name resolution fails.
 By default the minion will retry indefinitely.
 
+This bounds the retry loop of the main minion process. Forked per-job worker
+processes always fail fast on an unresolvable master (as if ``retry_dns_count``
+were ``0``) regardless of this setting, so a bad or removed master name cannot
+cause hung worker processes to accumulate. An explicit ``retry_dns_count`` is
+still honored inside workers.
+
 .. code-block:: yaml
 
     retry_dns_count: 3

salt/minion.py

@@ -114,6 +114,11 @@
 # signal handler sets this so that a SIGTERM arriving while the minion is
 # stuck retrying master DNS resolution can shut the io_loop down promptly
 # instead of waiting for ``retry_dns`` seconds * forever. See #69466.
+#
+# NOTE: this is a main-process mechanism only. A forked per-job worker holds
+# its own dead copy of this event that the parent can never set, so workers
+# must not rely on it to escape the retry loop -- they bound resolve_dns via
+# retry_dns_count instead (see Minion._target).
 _RESOLVE_DNS_ABORT = threading.Event()
 
 
@@ -2582,6 +2587,14 @@ def ctx(self):
     def _target(cls, minion_instance, opts, data, connected, creds_map):
         if creds_map:
             salt.crypt.AsyncAuth.creds_map = creds_map
+        # A forked per-job worker is short-lived and must never sit in the
+        # unbounded resolve_dns() retry loop when the master is unresolvable
+        # (the parent's _RESOLVE_DNS_ABORT event does not cross the fork
+        # boundary, so a stuck worker cannot be aborted short of SIGKILL and
+        # would leak). Fail fast so the worker raises, exits, and is reaped
+        # normally. An explicit operator-set retry_dns_count is honored.
+        if opts.get("retry_dns_count") is None:
+            opts["retry_dns_count"] = 0
         if not minion_instance:
             minion_instance = cls(opts, load_grains=False)
             minion_instance.connected = connected

tests/pytests/unit/test_minion.py

@@ -1037,6 +1037,73 @@ def trip_abort():
     )
 
 
+def test_resolve_dns_count_zero_fails_fast(minion_opts):
+    """
+    ``retry_dns_count: 0`` must make resolve_dns raise immediately on an
+    unresolvable master instead of entering the ``retry_dns`` sleep loop.
+    This is what lets forked workers fail fast rather than leak.
+    """
+    minion_opts.update(
+        {
+            "ipv6": False,
+            "master": "dummy",
+            "master_port": "4555",
+            # A large interval so that if the retry loop is (wrongly) entered
+            # the test would visibly stall instead of passing by luck.
+            "retry_dns": 90,
+            "retry_dns_count": 0,
+        },
+    )
+
+    dns_check = MagicMock(side_effect=SaltClientError("unresolvable"))
+    started = time.monotonic()
+    with patch("salt.utils.network.dns_check", dns_check):
+        with pytest.raises(SaltMasterUnresolvableError):
+            salt.minion.resolve_dns(minion_opts)
+    elapsed = time.monotonic() - started
+
+    # With retry_dns_count == 0 the loop raises on its first iteration before
+    # any retry sleep, so the initial lookup is the only dns_check call and
+    # the call returns effectively immediately.
+    assert dns_check.call_count == 1
+    assert elapsed < 5, (
+        f"resolve_dns slept before failing with retry_dns_count=0 "
+        f"(elapsed={elapsed:.2f}s); it should fail fast."
+    )
+
+
+def test_target_bounds_worker_dns_retry(minion_opts):
+    """
+    A forked per-job worker must not inherit an unbounded DNS retry loop.
+    Minion._target should force ``retry_dns_count`` to 0 when the operator
+    has not set it, and leave an explicit value untouched.
+    """
+    data = {"jid": "20240101000000000000", "fun": "test.ping", "arg": []}
+
+    # Stub out the actual job execution and the tornado StackContext wrapping
+    # so the test exercises only the opts mutation in _target's preamble,
+    # without requiring a fully initialized minion (functions/ctx).
+    def fake_stack_context(*args, **kwargs):
+        return contextlib.nullcontext()
+
+    with patch.object(salt.minion.Minion, "_thread_return", MagicMock()), patch(
+        "salt.ext.tornado.stack_context.StackContext", fake_stack_context
+    ):
+        # Operator did not set retry_dns_count -> worker fails fast.
+        unset_opts = copy.deepcopy(minion_opts)
+        unset_opts["retry_dns_count"] = None
+        minion = salt.minion.Minion(unset_opts, load_grains=False)
+        salt.minion.Minion._target(minion, unset_opts, data, True, None)
+        assert unset_opts["retry_dns_count"] == 0
+
+        # Operator set an explicit bound -> _target leaves it alone.
+        explicit_opts = copy.deepcopy(minion_opts)
+        explicit_opts["retry_dns_count"] = 5
+        minion = salt.minion.Minion(explicit_opts, load_grains=False)
+        salt.minion.Minion._target(minion, explicit_opts, data, True, None)
+        assert explicit_opts["retry_dns_count"] == 5
+
+
 def test_minion_manager_stop_unblocks_resolve_dns_69466(minion_opts):
     """
     Regression test for #69466.