Fix beacon delete not closing beacon modules

When beacons.delete is called, the beacon is removed from opts but the
beacon module's close() function is never called. This leaves resources
like inotify file descriptors and notifier threads active, causing CPU
spin that never recovers.

Add _close_beacon() and close_beacons() methods to the Beacon class.
Call _close_beacon() from delete_beacon() before removing the beacon
from opts, and call close_beacons() from Minion.beacons_refresh()
before replacing the Beacon instance.

Fixes #66449

Author: dwoz

changelog/66449.fixed.md

@@ -0,0 +1,4 @@
+Fixed beacon delete not calling the beacon's close function, causing resource
+leaks (e.g. inotify file descriptors) and CPU spin after deleting beacons at
+runtime via ``beacons.delete``. Also fixed inotify file descriptor leak during
+beacon refresh when the Beacon instance is replaced.

salt/beacons/__init__.py

@@ -26,6 +26,55 @@ def __init__(self, opts, functions, interval_map=None):
         self.beacons = salt.loader.beacons(opts, functions)
         self.interval_map = interval_map or dict()
 
+    def _close_beacon(self, name):
+        """
+        Close a single beacon module if it has a close function.
+        This releases resources like inotify file descriptors.
+        """
+        beacon_config = self.opts["beacons"].get(name)
+        if beacon_config is None:
+            return
+
+        current_beacon_config = None
+        if isinstance(beacon_config, list):
+            current_beacon_config = {}
+            list(map(current_beacon_config.update, beacon_config))
+        elif isinstance(beacon_config, dict):
+            current_beacon_config = beacon_config
+
+        if current_beacon_config is None:
+            return
+
+        beacon_name = name
+        if self._determine_beacon_config(current_beacon_config, "beacon_module"):
+            beacon_name = current_beacon_config["beacon_module"]
+
+        close_str = f"{beacon_name}.close"
+        if close_str in self.beacons:
+            try:
+                config = copy.deepcopy(beacon_config)
+                if isinstance(config, list):
+                    config.append({"_beacon_name": name})
+                log.debug("Closing beacon %s", name)
+                self.beacons[close_str](config)
+            except Exception:  # pylint: disable=broad-except
+                log.debug("Failed to close beacon %s", name, exc_info=True)
+
+    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
+            self._close_beacon(mod)
+
     def process(self, config, grains):
         """
         Process the configured beacons
@@ -405,6 +454,9 @@ def delete_beacon(self, name):
             complete = False
         else:
             if name in self.opts["beacons"]:
+                # Close the beacon module to release resources (e.g. inotify fds)
+                # before removing it from the configuration.
+                self._close_beacon(name)
                 del self.opts["beacons"][name]
                 comment = f"Deleting beacon item: {name}"
             else:

salt/minion.py

@@ -3201,6 +3201,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,151 @@ 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_delete_beacon_calls_close(minion_opts):
+    """
+    Test that delete_beacon() calls the beacon's close function before
+    removing it, so resources like inotify file descriptors are released.
+    """
+    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
+
+    with patch("salt.utils.event.get_event"):
+        beacon.delete_beacon("inotify")
+
+    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
+    assert "inotify" not in minion_opts["beacons"]
+
+
+def test_delete_beacon_calls_close_with_beacon_module(minion_opts):
+    """
+    Test that delete_beacon() respects beacon_module and calls close
+    on the correct underlying module.
+    """
+    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
+
+    with patch("salt.utils.event.get_event"):
+        beacon.delete_beacon("watch_apache")
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert {"_beacon_name": "watch_apache"} in call_args
+    assert "watch_apache" not in minion_opts["beacons"]
+
+
+def test_delete_beacon_without_close(minion_opts):
+    """
+    Test that delete_beacon() works when the beacon module has no 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
+
+    with patch("salt.utils.event.get_event"):
+        beacon.delete_beacon("status")
+
+    assert "status" not in minion_opts["beacons"]