fixes

frigate/analytics/reporter.py

@@ -19,6 +19,7 @@ from frigate.analytics.state import (
     save_state,
 )
 from frigate.analytics.transport import SendOutcome, send_report
+from frigate.config import FrigateConfig
 from frigate.config.holder import ConfigHolder
 from frigate.const import ANALYTICS_URL
 from frigate.notices import raise_notice, resolve_notice
@@ -37,7 +38,7 @@ PROMPT_KIND = "analytics_prompt"
 
 
 class AnalyticsReporter(threading.Thread):
-    """Reads the live config on every wake, so a settings save needs no restart."""
+    """Follows the live config, so a settings save needs no restart."""
 
     def __init__(
         self,
@@ -66,6 +67,9 @@ class AnalyticsReporter(threading.Thread):
         self.interval = self._next_interval()
         self.opted_in: bool | None = None
         self.warned_unwritable = False
+        # a settings save runs on an API thread while a wake may be mid-attempt
+        self._lock = threading.Lock()
+        config_holder.subscribe(self._on_config)
 
     def _next_interval(self) -> float:
         return INTERVAL_S + self.rng.uniform(-JITTER_S, JITTER_S)
@@ -80,52 +84,66 @@ class AnalyticsReporter(threading.Thread):
             if self.stop_event.wait(WAKE_S):
                 break
 
+    def _on_config(self, config: FrigateConfig) -> None:
+        # a save can turn sharing off and back on between two wakes, so an
+        # opt-out is handled when it's saved rather than at the next wake
+        with self._lock:
+            if not config.safe_mode:
+                self._apply_consent(config.telemetry.analytics)
+
+    def _apply_consent(self, opted_in: bool) -> None:
+        # called with the lock held
+        if opted_in == self.opted_in:
+            return
+
+        self.opted_in = opted_in
+
+        if opted_in:
+            resolve_notice(PROMPT_KIND)
+        else:
+            raise_notice(PROMPT_KIND)
+            delete_state(self.state_path)
+
     def tick(self) -> None:
-        config = self.config_holder.config
+        with self._lock:
+            config = self.config_holder.config
 
-        # safe mode parses a default config where analytics reads as off, and
-        # handling that as an opt-out would delete the install ID
-        if config.safe_mode:
-            return
+            # safe mode parses a default config where analytics reads as off,
+            # and handling that as an opt-out would delete the install ID
+            if config.safe_mode:
+                return
 
-        opted_in = config.telemetry.analytics
+            self._apply_consent(config.telemetry.analytics)
 
-        if opted_in != self.opted_in:
-            self.opted_in = opted_in
+            if not config.telemetry.analytics:
+                return
 
-            if opted_in:
-                resolve_notice(PROMPT_KIND)
-            else:
-                raise_notice(PROMPT_KIND)
-                delete_state(self.state_path)
+            now = self.clock()
 
-        if not opted_in:
-            return
+            if now < self.first_due:
+                return
 
-        now = self.clock()
+            state = load_state(self.state_path) or new_state()
 
-        if now < self.first_due:
-            return
+            if not self._due(state.last_attempt_at, now):
+                return
 
-        state = load_state(self.state_path) or new_state()
+            # saved before sending, so a failing endpoint or a crash mid-send
+            # still waits a full interval; without saved state every boot would
+            # send under a new install ID
+            if not save_state(AnalyticsState(state.install_id, now), self.state_path):
+                if not self.warned_unwritable:
+                    logger.warning(
+                        "Analytics is on, but %s isn't writable, so no report is sent",
+                        self.state_path,
+                    )
+                    self.warned_unwritable = True
 
-        if not self._due(state.last_attempt_at, now):
-            return
+                return
 
-        # saved before sending, so a failing endpoint or a crash mid-send still
-        # waits a full interval; without saved state every boot would send
-        # under a new install ID
-        if not save_state(AnalyticsState(state.install_id, now), self.state_path):
-            if not self.warned_unwritable:
-                logger.warning(
-                    "Analytics is on, but %s can't be written, so no report is sent",
-                    self.state_path,
-                )
-                self.warned_unwritable = True
+            self.interval = self._next_interval()
 
-            return
-
-        self.interval = self._next_interval()
+        # the lock stays free during the request, so a save never waits on it
         self._send(state.install_id, now)
 
     def _due(self, last_attempt_at: float, now: float) -> bool:
@@ -141,9 +159,19 @@ class AnalyticsReporter(threading.Thread):
             stats=self.stats_emitter.get_latest_stats(),
             notice_stats=notice_stats,
         )
-        body = build_report(ctx, install_id, sent_at=int(now)).model_dump_json()
+        report = build_report(ctx, install_id, sent_at=int(now))
+        body = report.model_dump_json()
 
-        if self.send(self.url, body) is SendOutcome.accepted:
+        # consent can be withdrawn while the report builds
+        if not self.config_holder.config.telemetry.analytics:
+            return
+
+        if self.send(self.url, body) is not SendOutcome.accepted:
+            return
+
+        # a failed health section sent no notice counts, so they stay pending
+        if report.health is not None:
             self.notice_registry.mark_reported(notice_stats)
-            logger.info("Sent the daily analytics report")
-            logger.debug("Analytics report: %s", body)
+
+        logger.info("Sent the daily analytics report")
+        logger.debug("Analytics report: %s", body)

frigate/config/holder.py

@@ -1,9 +1,14 @@
 """Shared handle on the config object that is current for this instance."""
 
+import logging
+from collections.abc import Callable
+
 from .config import FrigateConfig
 
 __all__ = ["ConfigHolder"]
 
+logger = logging.getLogger(__name__)
+
 
 class ConfigHolder:
     """Indirection for the most recently parsed config.
@@ -23,12 +28,24 @@ class ConfigHolder:
 
     def __init__(self, config: FrigateConfig) -> None:
         self._config = config
+        self._listeners: list[Callable[[FrigateConfig], None]] = []
 
     @property
     def config(self) -> FrigateConfig:
         """The config as of the most recent successful save."""
         return self._config
 
+    def subscribe(self, listener: Callable[[FrigateConfig], None]) -> None:
+        """Call listener on the saving thread with each config installed later."""
+        self._listeners.append(listener)
+
     def set(self, config: FrigateConfig) -> None:
         """Install a freshly parsed config as the current one."""
         self._config = config
+
+        for listener in self._listeners:
+            try:
+                listener(config)
+            except Exception:
+                # a listener bug must not fail the save that has already applied
+                logger.exception("Config listener failed")

frigate/test/test_analytics_reporter.py

@@ -4,7 +4,6 @@ import os
 import tempfile
 import threading
 import unittest
-from types import SimpleNamespace
 from unittest.mock import Mock, patch
 
 from frigate.analytics import reporter as reporter_module
@@ -18,6 +17,7 @@ from frigate.analytics.reporter import (
 )
 from frigate.analytics.state import AnalyticsState, load_state, save_state
 from frigate.analytics.transport import SendOutcome
+from frigate.config.holder import ConfigHolder
 from frigate.test.analytics_helpers import make_config
 
 SNAPSHOT = [
@@ -42,18 +42,16 @@ class TestAnalyticsReporter(unittest.TestCase):
         self.registry.stats.return_value = SNAPSHOT
         self.stats = Mock()
         self.stats.get_latest_stats.return_value = {}
-        self.holder = SimpleNamespace(
-            config=make_config({"telemetry": {"analytics": True}})
-        )
+        self.holder = ConfigHolder(make_config({"telemetry": {"analytics": True}}))
 
         for name in ("raise_notice", "resolve_notice"):
             patcher = patch.object(reporter_module, name)
             setattr(self, name, patcher.start())
             self.addCleanup(patcher.stop)
 
-        built = Mock()
-        built.model_dump_json.return_value = '{"report": 1}'
-        patcher = patch.object(reporter_module, "build_report", return_value=built)
+        self.built = Mock()
+        self.built.model_dump_json.return_value = '{"report": 1}'
+        patcher = patch.object(reporter_module, "build_report", return_value=self.built)
         self.build_report = patcher.start()
         self.addCleanup(patcher.stop)
 
@@ -126,8 +124,45 @@ class TestAnalyticsReporter(unittest.TestCase):
         self.assertEqual(self.send.call_count, 1)
         self.registry.mark_reported.assert_not_called()
 
+    def test_an_accepted_report_without_health_keeps_the_watermarks(self):
+        self.built.health = None
+        reporter = self.reporter()
+        self.advance(FIRST_DELAY_S[0])
+
+        reporter.tick()
+
+        self.send.assert_called_once()
+        self.registry.mark_reported.assert_not_called()
+
+    def test_consent_withdrawn_while_the_report_builds_stops_the_send(self):
+        reporter = self.reporter()
+        self.advance(FIRST_DELAY_S[0])
+
+        def withdraw(*args, **kwargs):
+            self.holder.set(make_config())
+            return self.built
+
+        self.build_report.side_effect = withdraw
+        reporter.tick()
+
+        self.send.assert_not_called()
+        self.assertFalse(os.path.exists(self.path))
+
+    def test_turning_sharing_off_and_on_between_wakes_starts_a_fresh_identity(self):
+        save_state(AnalyticsState("a" * 32, 0.0), self.path)
+        reporter = self.reporter()
+        reporter.tick()
+
+        self.holder.set(make_config())
+        self.holder.set(make_config({"telemetry": {"analytics": True}}))
+        self.advance(FIRST_DELAY_S[0])
+        reporter.tick()
+
+        self.raise_notice.assert_called_once_with(PROMPT_KIND)
+        self.assertNotEqual(load_state(self.path).install_id, "a" * 32)
+
     def test_opting_out_raises_the_prompt_once_and_deletes_the_state(self):
-        self.holder.config = make_config()
+        self.holder.set(make_config())
         save_state(AnalyticsState("a" * 32, 0.0), self.path)
         reporter = self.reporter()
 
@@ -139,17 +174,17 @@ class TestAnalyticsReporter(unittest.TestCase):
         self.send.assert_not_called()
 
     def test_opting_in_resolves_the_prompt(self):
-        self.holder.config = make_config()
+        self.holder.set(make_config())
         reporter = self.reporter()
         reporter.tick()
 
-        self.holder.config = make_config({"telemetry": {"analytics": True}})
+        self.holder.set(make_config({"telemetry": {"analytics": True}}))
         reporter.tick()
 
         self.resolve_notice.assert_called_once_with(PROMPT_KIND)
 
     def test_safe_mode_touches_nothing(self):
-        self.holder.config = make_config({"safe_mode": True})
+        self.holder.set(make_config({"safe_mode": True}))
         save_state(AnalyticsState("a" * 32, 0.0), self.path)
         reporter = self.reporter()
 

frigate/test/test_config_util.py

@@ -82,5 +82,30 @@ class TestSwapRuntimeConfig(unittest.TestCase):
         app.genai_manager.update_config.assert_called_once_with(config)
 
 
+class TestConfigHolder(unittest.TestCase):
+    def test_set_passes_the_new_config_to_subscribers(self) -> None:
+        holder = ConfigHolder(MagicMock(name="boot_config"))
+        listener = MagicMock()
+        holder.subscribe(listener)
+        config = MagicMock(name="new_config")
+
+        holder.set(config)
+
+        listener.assert_called_once_with(config)
+
+    def test_a_failing_subscriber_does_not_block_the_swap(self) -> None:
+        holder = ConfigHolder(MagicMock(name="boot_config"))
+        after = MagicMock()
+        holder.subscribe(MagicMock(side_effect=RuntimeError("boom")))
+        holder.subscribe(after)
+        config = MagicMock(name="new_config")
+
+        with self.assertLogs("frigate.config.holder", "ERROR"):
+            holder.set(config)
+
+        self.assertIs(holder.config, config)
+        after.assert_called_once_with(config)
+
+
 if __name__ == "__main__":
     unittest.main()