Fix inotify file descriptor leak during beacon refresh

changelog/66449.fixed.md

@@ -0,0 +1,4 @@
+Fixed inotify file descriptor leak in beacons. When beacons are refreshed
+(e.g. during module refresh or pillar refresh), the old beacon modules are now
+properly closed before creating new ones, preventing exhaustion of the inotify
+instance limit.

salt/beacons/__init__.py

@@ -26,6 +26,47 @@ def __init__(self, opts, functions, interval_map=None):
         self.beacons = salt.loader.beacons(opts, functions)
         self.interval_map = interval_map or dict()
 
+    def close_beacons(self):
+        """
+        Close all beacon modules that have a close function.
+        This ensures resources like inotify file descriptors are properly
+        released when beacons are refreshed or the Beacon instance is replaced.
+
+        See: https://github.com/saltstack/salt/issues/66449
+        See: https://github.com/saltstack/salt/issues/58907
+        """
+        beacons = self._get_beacons()
+        for mod in beacons:
+            if mod == "enabled":
+                continue
+
+            current_beacon_config = None
+            if isinstance(beacons[mod], list):
+                current_beacon_config = {}
+                list(map(current_beacon_config.update, beacons[mod]))
+            elif isinstance(beacons[mod], dict):
+                current_beacon_config = beacons[mod]
+
+            if current_beacon_config is None:
+                continue
+
+            beacon_name = None
+            if self._determine_beacon_config(current_beacon_config, "beacon_module"):
+                beacon_name = current_beacon_config["beacon_module"]
+            else:
+                beacon_name = mod
+
+            close_str = f"{beacon_name}.close"
+            if close_str in self.beacons:
+                try:
+                    config = copy.deepcopy(beacons[mod])
+                    if isinstance(config, list):
+                        config.append({"_beacon_name": mod})
+                    log.debug("Closing beacon %s", mod)
+                    self.beacons[close_str](config)
+                except Exception:  # pylint: disable=broad-except
+                    log.debug("Failed to close beacon %s", mod, exc_info=True)
+
     def process(self, config, grains):
         """
         Process the configured beacons

salt/minion.py

@@ -3202,6 +3202,10 @@ def beacons_refresh(self):
         prev_interval_map = {}
         if hasattr(self, "beacons") and hasattr(self.beacons, "interval_map"):
             prev_interval_map = self.beacons.interval_map
+        # Close existing beacon modules to release resources (e.g. inotify fds)
+        # before replacing the Beacon instance.
+        if hasattr(self, "beacons"):
+            self.beacons.close_beacons()
         self.beacons = salt.beacons.Beacon(
             self.opts, self.functions, interval_map=prev_interval_map
         )

tests/pytests/unit/test_beacons.py

@@ -121,3 +121,169 @@ def test_beacon_module(minion_opts):
     with patch.object(beacon, "beacons", mocked) as patched:
         beacon.process(minion_opts["beacons"], minion_opts["grains"])
         patched[name].assert_has_calls(calls)
+
+
+def test_close_beacons_calls_close_on_modules(minion_opts):
+    """
+    Test that close_beacons() calls the close function on each beacon
+    module that provides one, releasing resources like inotify fds.
+
+    See: https://github.com/saltstack/salt/issues/66449
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "inotify": [
+            {"files": {"/etc/fstab": {}}},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+
+    close_mock = MagicMock()
+    beacon.beacons["inotify.close"] = close_mock
+
+    beacon.close_beacons()
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert isinstance(call_args, list)
+    assert {"_beacon_name": "inotify"} in call_args
+
+
+def test_close_beacons_with_beacon_module_override(minion_opts):
+    """
+    Test that close_beacons() respects beacon_module and calls close
+    on the correct underlying module name.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "watch_apache": [
+            {"processes": {"apache2": "stopped"}},
+            {"beacon_module": "ps"},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+
+    close_mock = MagicMock()
+    beacon.beacons["ps.close"] = close_mock
+
+    beacon.close_beacons()
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert {"_beacon_name": "watch_apache"} in call_args
+
+
+def test_close_beacons_skips_modules_without_close(minion_opts):
+    """
+    Test that close_beacons() gracefully skips beacons that don't
+    have a close function.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "status": [
+            {"time": ["all"]},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+
+    assert "status.close" not in beacon.beacons
+    beacon.close_beacons()
+
+
+def test_close_beacons_handles_close_exception(minion_opts):
+    """
+    Test that close_beacons() does not propagate exceptions raised
+    by a beacon's close function.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "inotify": [
+            {"files": {"/etc/fstab": {}}},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+    beacon.beacons["inotify.close"] = MagicMock(side_effect=Exception("close failed"))
+
+    beacon.close_beacons()
+
+
+def test_close_beacons_multiple_beacons(minion_opts):
+    """
+    Test that close_beacons() calls close on all beacons that have
+    a close function.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "inotify": [
+            {"files": {"/etc/fstab": {}}},
+        ],
+        "watch_apache": [
+            {"processes": {"apache2": "stopped"}},
+            {"beacon_module": "ps"},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+
+    inotify_close = MagicMock()
+    ps_close = MagicMock()
+    beacon.beacons["inotify.close"] = inotify_close
+    beacon.beacons["ps.close"] = ps_close
+
+    beacon.close_beacons()
+
+    inotify_close.assert_called_once()
+    ps_close.assert_called_once()
+
+
+def test_close_beacons_skips_enabled_key(minion_opts):
+    """
+    Test that close_beacons() skips the 'enabled' key in the beacons config.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "enabled": True,
+        "inotify": [
+            {"files": {"/etc/fstab": {}}},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+    close_mock = MagicMock()
+    beacon.beacons["inotify.close"] = close_mock
+
+    beacon.close_beacons()
+
+    close_mock.assert_called_once()
+
+
+def test_close_beacons_dict_config(minion_opts):
+    """
+    Test that close_beacons() handles dict-style beacon configuration
+    (backwards-compatible format).
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "status": {"time": ["all"]},
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+    close_mock = MagicMock()
+    beacon.beacons["status.close"] = close_mock
+
+    beacon.close_beacons()
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert isinstance(call_args, dict)

tests/pytests/unit/test_minion.py

@@ -702,6 +702,46 @@ def test_beacons_refresh_preserves_interval_map(minion_opts):
             minion.destroy()
 
 
+def test_beacons_refresh_closes_old_beacons(minion_opts):
+    """
+    Tests that 'beacons_refresh' calls close_beacons() on the old Beacon
+    instance before replacing it, preventing inotify fd leaks.
+
+    See: https://github.com/saltstack/salt/issues/66449
+    See: https://github.com/saltstack/salt/issues/58907
+    """
+    with patch("salt.minion.Minion.ctx", MagicMock(return_value={})), patch(
+        "salt.utils.process.SignalHandlingProcess.start",
+        MagicMock(return_value=True),
+    ), patch(
+        "salt.utils.process.SignalHandlingProcess.join",
+        MagicMock(return_value=True),
+    ):
+        minion = None
+        try:
+            minion = salt.minion.Minion(
+                minion_opts,
+                io_loop=salt.ext.tornado.ioloop.IOLoop.current(),
+            )
+            minion.schedule = salt.utils.schedule.Schedule(
+                minion_opts, {}, returners={}
+            )
+
+            minion.module_refresh()
+            assert hasattr(minion, "beacons")
+
+            old_beacons = minion.beacons
+            with patch.object(old_beacons, "close_beacons") as close_mock:
+                minion.beacons_refresh()
+                close_mock.assert_called_once()
+
+            assert minion.beacons is not old_beacons
+
+        finally:
+            if minion is not None:
+                minion.destroy()
+
+
 @pytest.mark.slow_test
 async def test_when_ping_interval_is_set_the_callback_should_be_added_to_periodic_callbacks(
     minion_opts,