From 7001623ba20ec2842e9f9b6ed50bc27ca8c53e06 Mon Sep 17 00:00:00 2001 From: Josh Hawkins <32435876+hawkeye217@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:15:33 -0500 Subject: [PATCH] fixes --- frigate/analytics/reporter.py | 106 +++++++++++++++--------- frigate/config/holder.py | 17 ++++ frigate/test/test_analytics_reporter.py | 57 ++++++++++--- frigate/test/test_config_util.py | 25 ++++++ 4 files changed, 155 insertions(+), 50 deletions(-) diff --git a/frigate/analytics/reporter.py b/frigate/analytics/reporter.py index fc4a3c93fb..a4c78d8c38 100644 --- a/frigate/analytics/reporter.py +++ b/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) diff --git a/frigate/config/holder.py b/frigate/config/holder.py index e5f3e7bd8f..26efec733f 100644 --- a/frigate/config/holder.py +++ b/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") diff --git a/frigate/test/test_analytics_reporter.py b/frigate/test/test_analytics_reporter.py index ca7a06ab01..3d12c3628d 100644 --- a/frigate/test/test_analytics_reporter.py +++ b/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() diff --git a/frigate/test/test_config_util.py b/frigate/test/test_config_util.py index 5888f26188..b4b23b9257 100644 --- a/frigate/test/test_config_util.py +++ b/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()