From 29f08203cad68531c1130075f7af2f47977badb9 Mon Sep 17 00:00:00 2001 From: Josh Hawkins <32435876+hawkeye217@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:45:08 -0500 Subject: [PATCH] check every overlapping review item before dropping a recording Recording retention around review items applied only the first review item, in start order, whose pre and post capture window covered a segment. When an alert and a detection were close together, a quiet segment covered by both was dropped by the detection's motion mode without checking the alert's all mode. The loop also stopped at a review item that started too late to reach the segment, even when a later alert with a longer pre_capture did reach it. Both the recording maintainer and cleanup now check every review item whose window covers the segment and keep it if any of their retain modes would. --- frigate/record/cleanup.py | 50 +++---- frigate/record/maintainer.py | 35 ++--- frigate/test/test_maintainer.py | 111 +++++++++++++++ frigate/test/test_record_cleanup_reviews.py | 142 ++++++++++++++++++++ 4 files changed, 298 insertions(+), 40 deletions(-) create mode 100644 frigate/test/test_record_cleanup_reviews.py diff --git a/frigate/record/cleanup.py b/frigate/record/cleanup.py index 509c43ae14..01460f06f6 100644 --- a/frigate/record/cleanup.py +++ b/frigate/record/cleanup.py @@ -198,52 +198,56 @@ class RecordingCleanup(threading.Thread): deleted_recordings = set() kept_recordings: list[tuple[float, float]] = [] for recording in recordings: - keep = False - mode = None - # Now look for a reason to keep this recording segment + modes: set[RetainModeEnum] = set() + # Now look for every reason to keep this recording segment for idx in range(review_start, len(reviews)): review = reviews[idx] severity = review.severity pre_capture = config.record.get_review_pre_capture(severity) post_capture = config.record.get_review_post_capture(severity) - # if the review starts in the future, stop checking reviews - # and let this recording segment expire - if review.start_time - pre_capture > recording.end_time: - keep = False + # stop once no later review's pre capture can reach this segment + if ( + review.start_time - config.record.event_pre_capture + > recording.end_time + ): break - # if the review is in progress or ends after the recording starts, keep it - # and stop looking at reviews + if review.start_time - pre_capture > recording.end_time: + continue + + # collect every review that is in progress or ends after the + # recording starts, since their windows can overlap if ( review.end_time is None or review.end_time + post_capture >= recording.start_time ): - keep = True - mode = ( + modes.add( alerts_retain_mode if review.severity == "alert" else detections_retain_mode ) - break # if the review ends before this recording segment starts, skip - # this review and check the next review for an overlap. - # since the review and recordings are sorted, we can skip review - # that end before the previous recording segment started on future segments - if review.end_time + post_capture < recording.start_time: + # it on future segments too. reviews and recordings are sorted, + # but an earlier review with a longer post capture may still + # overlap, so only skip while nothing has overlapped yet + elif not modes: review_start = idx - # Delete recordings outside of the retention window or based on the retention mode - if ( - not keep + # Delete recordings unless an overlapping review's retention mode keeps them + if not any( + mode == RetainModeEnum.all or ( mode == RetainModeEnum.motion - and recording.motion == 0 - and recording.objects == 0 - and recording.dBFS == 0 + and ( + recording.motion != 0 + or recording.objects != 0 + or recording.dBFS != 0 + ) ) - or (mode == RetainModeEnum.active_objects and recording.objects == 0) + or (mode == RetainModeEnum.active_objects and recording.objects != 0) + for mode in modes ): recording_path = Path(recording.path) recording_path.unlink(missing_ok=True) diff --git a/frigate/record/maintainer.py b/frigate/record/maintainer.py index e024907343..4c3af64a8f 100644 --- a/frigate/record/maintainer.py +++ b/frigate/record/maintainer.py @@ -759,39 +759,40 @@ class RecordingMaintainer(threading.Thread): # we fell through the continuous / motion check, so we need to check the review items # if the cached segment overlaps with the review items: - overlaps = False + modes: set[RetainModeEnum] = set() for review in reviews: severity = SeverityEnum[review.severity] - # if the review item starts in the future, stop checking review items - # and remove this segment + # stop once no later review item's pre capture can reach this segment + if ( + review.start_time - record_config.event_pre_capture + ) > end_time.timestamp(): + break + if ( review.start_time - record_config.get_review_pre_capture(severity) ) > end_time.timestamp(): - overlaps = False - break + continue - # if the review item is in progress or ends after the recording starts, keep it - # and stop looking at review items + # collect every review item that is in progress or ends after the + # recording starts, since their windows can overlap if ( review.end_time is None or (review.end_time + record_config.get_review_post_capture(severity)) >= start_time.timestamp() ): - overlaps = True - break - - if overlaps: - record_mode = ( - alerts_retain_mode - if review.severity == "alert" - else detections_retain_mode - ) + modes.add( + alerts_retain_mode + if review.severity == "alert" + else detections_retain_mode + ) + if modes: if segment_stats is None: segment_stats = self.segment_stats(camera, start_time, end_time) - if not segment_stats.should_discard_segment(record_mode): + # keep the segment if any overlapping review item's mode keeps it + if any(not segment_stats.should_discard_segment(m) for m in modes): # move from cache to recordings immediately return await self.move_segment( camera, diff --git a/frigate/test/test_maintainer.py b/frigate/test/test_maintainer.py index 63e1ddbb34..e2fdfcf5bc 100644 --- a/frigate/test/test_maintainer.py +++ b/frigate/test/test_maintainer.py @@ -127,6 +127,116 @@ class TestMaintainer(unittest.IsolatedAsyncioTestCase): self.assertIsNone(result) maintainer.drop_segment.assert_called_once_with(cache_path) + async def test_quiet_segment_kept_by_later_overlapping_alert(self): + config = MagicMock(spec=FrigateConfig) + + camera_config = MagicMock() + camera_config.record.enabled = True + camera_config.record.continuous.days = 0 + camera_config.record.motion.days = 0 + camera_config.record.alerts.retain.mode = RetainModeEnum.all + camera_config.record.detections.retain.mode = RetainModeEnum.motion + camera_config.record.event_pre_capture = 60 + camera_config.record.get_review_pre_capture.return_value = 60 + camera_config.record.get_review_post_capture.return_value = 60 + config.cameras = {"test_cam": camera_config} + + maintainer = RecordingMaintainer(config, MagicMock()) + + now = datetime.datetime.now(datetime.UTC) + start_time = now - datetime.timedelta(seconds=20) + end_time = now - datetime.timedelta(seconds=10) + cache_path = "/tmp/cache/test_cam@20260417150000+0000.mp4" + + maintainer.end_time_cache = { + cache_path: (end_time, 10.0, False, None, None, None, []) + } + maintainer.object_recordings_info["test_cam"] = [(now.timestamp(), [], [], [])] + maintainer.audio_recordings_info["test_cam"] = [] + + maintainer.drop_segment = MagicMock() + maintainer.move_segment = AsyncMock(return_value=None) + maintainer.recordings_publisher = MagicMock() + + # the detection's post capture and the alert's window both cover the segment + detection = MagicMock() + detection.severity = "detection" + detection.start_time = start_time.timestamp() - 50 + detection.end_time = start_time.timestamp() - 40 + alert = MagicMock() + alert.severity = "alert" + alert.start_time = start_time.timestamp() - 10 + alert.end_time = start_time.timestamp() + + await maintainer.validate_and_move_segment( + "test_cam", + reviews=[detection, alert], + recording={ + "start_time": start_time, + "cache_path": cache_path, + "stream_type": "main", + }, + ) + + maintainer.move_segment.assert_awaited_once() + maintainer.drop_segment.assert_not_called() + + async def test_quiet_segment_kept_by_later_alert_with_longer_pre_capture(self): + config = MagicMock(spec=FrigateConfig) + + camera_config = MagicMock() + camera_config.record.enabled = True + camera_config.record.continuous.days = 0 + camera_config.record.motion.days = 0 + camera_config.record.alerts.retain.mode = RetainModeEnum.all + camera_config.record.detections.retain.mode = RetainModeEnum.all + camera_config.record.event_pre_capture = 30 + camera_config.record.get_review_pre_capture.side_effect = lambda s: ( + 30 if s == "alert" else 5 + ) + camera_config.record.get_review_post_capture.return_value = 5 + config.cameras = {"test_cam": camera_config} + + maintainer = RecordingMaintainer(config, MagicMock()) + + now = datetime.datetime.now(datetime.UTC) + start_time = now - datetime.timedelta(seconds=20) + end_time = now - datetime.timedelta(seconds=10) + cache_path = "/tmp/cache/test_cam@20260417150000+0000.mp4" + + maintainer.end_time_cache = { + cache_path: (end_time, 10.0, False, None, None, None, []) + } + maintainer.object_recordings_info["test_cam"] = [(now.timestamp(), [], [], [])] + maintainer.audio_recordings_info["test_cam"] = [] + + maintainer.drop_segment = MagicMock() + maintainer.move_segment = AsyncMock(return_value=None) + maintainer.recordings_publisher = MagicMock() + + # the detection's pre capture starts after the segment, the alert's reaches it + detection = MagicMock() + detection.severity = "detection" + detection.start_time = end_time.timestamp() + 10 + detection.end_time = end_time.timestamp() + 12 + alert = MagicMock() + alert.severity = "alert" + alert.start_time = end_time.timestamp() + 20 + alert.end_time = end_time.timestamp() + 25 + + await maintainer.validate_and_move_segment( + "test_cam", + reviews=[detection, alert], + recording={ + "start_time": start_time, + "cache_path": cache_path, + "stream_type": "main", + }, + ) + + maintainer.move_segment.assert_awaited_once() + maintainer.drop_segment.assert_not_called() + async def test_defers_review_overlap_segment_until_metadata_catches_up(self): # Regression: a segment overlapping an active_objects review must not # be dropped while detection metadata lags behind the segment end, @@ -139,6 +249,7 @@ class TestMaintainer(unittest.IsolatedAsyncioTestCase): camera_config.record.continuous.days = 7 camera_config.record.motion.days = 0 camera_config.record.alerts.retain.mode = RetainModeEnum.active_objects + camera_config.record.event_pre_capture = 5 camera_config.record.get_review_pre_capture.return_value = 5 camera_config.record.get_review_post_capture.return_value = 5 config.cameras = {"test_cam": camera_config} diff --git a/frigate/test/test_record_cleanup_reviews.py b/frigate/test/test_record_cleanup_reviews.py new file mode 100644 index 0000000000..41db0bd394 --- /dev/null +++ b/frigate/test/test_record_cleanup_reviews.py @@ -0,0 +1,142 @@ +"""Tests for review-based retention in recording cleanup.""" + +import datetime +import unittest +from unittest.mock import MagicMock + +from peewee import SqliteDatabase + +from frigate.config import FrigateConfig +from frigate.models import Previews, Recordings, ReviewSegment, UserReviewStatus +from frigate.record.cleanup import RecordingCleanup + + +class TestRecordingCleanupReviewOverlap(unittest.TestCase): + def setUp(self): + self.db = SqliteDatabase(":memory:") + models = [Previews, Recordings, ReviewSegment, UserReviewStatus] + self.db.bind(models) + self.db.create_tables(models) + self.base = datetime.datetime.now().timestamp() - 3600 + + def tearDown(self): + self.db.close() + + def _build_cleanup(self, alerts: dict, detections: dict) -> RecordingCleanup: + config = FrigateConfig( + **{ + "mqtt": {"host": "mqtt"}, + "cameras": { + "front_door": { + "ffmpeg": { + "inputs": [ + { + "path": "rtsp://10.0.0.1:554/video", + "roles": ["detect", "record"], + } + ] + }, + "record": { + "enabled": True, + "continuous": {"days": 0}, + "motion": {"days": 0}, + "alerts": alerts, + "detections": detections, + }, + } + }, + } + ) + return RecordingCleanup(config, MagicMock()) + + def _insert_review(self, id: str, severity: str, start: float, end: float): + ReviewSegment.create( + id=id, + camera="front_door", + start_time=self.base + start, + end_time=self.base + end, + severity=severity, + thumb_path=f"/media/frigate/clips/review/{id}.webp", + data={}, + ) + + def _insert_recording(self, id: str, start: float): + Recordings.create( + id=id, + camera="front_door", + path=f"/media/frigate/recordings/{id}.mp4", + start_time=self.base + start, + end_time=self.base + start + 10, + duration=10, + motion=0, + objects=0, + dBFS=0, + segment_size=0, + stream_type="main", + ) + + def test_quiet_segment_kept_by_later_overlapping_alert(self): + cleanup = self._build_cleanup( + alerts={"pre_capture": 60, "post_capture": 60, "retain": {"mode": "all"}}, + detections={"post_capture": 60, "retain": {"mode": "motion"}}, + ) + self._insert_review("detection", "detection", 90, 100) + self._insert_review("alert", "alert", 130, 140) + self._insert_recording("quiet", 140) + + cleanup.expire_recordings() + + assert Recordings.get_or_none(Recordings.id == "quiet") is not None + + def test_quiet_segment_dropped_when_no_overlapping_mode_keeps_it(self): + cleanup = self._build_cleanup( + alerts={ + "pre_capture": 60, + "post_capture": 60, + "retain": {"mode": "motion"}, + }, + detections={"post_capture": 60, "retain": {"mode": "motion"}}, + ) + self._insert_review("detection", "detection", 90, 100) + self._insert_review("alert", "alert", 130, 140) + self._insert_recording("quiet", 140) + + cleanup.expire_recordings() + + assert Recordings.get_or_none(Recordings.id == "quiet") is None + + def test_longer_pre_capture_survives_an_earlier_shorter_review(self): + # the detection starts after the segment even with its pre capture, + # but the later alert's longer pre capture still reaches it + cleanup = self._build_cleanup( + alerts={"pre_capture": 30, "retain": {"mode": "all"}}, + detections={"pre_capture": 5, "retain": {"mode": "all"}}, + ) + self._insert_review("detection", "detection", 10, 12) + self._insert_review("alert", "alert", 20, 25) + self._insert_recording("early", -6) + + cleanup.expire_recordings() + + assert Recordings.get_or_none(Recordings.id == "early") is not None + + def test_longer_post_capture_survives_a_later_shorter_review(self): + # the alert's window ends before the detection's, so skipping past + # it must not skip the detection for later segments + cleanup = self._build_cleanup( + alerts={"post_capture": 5, "retain": {"mode": "motion"}}, + detections={"post_capture": 60, "retain": {"mode": "all"}}, + ) + self._insert_review("detection", "detection", 90, 100) + self._insert_review("alert", "alert", 110, 115) + self._insert_recording("first", 125) + self._insert_recording("second", 135) + + cleanup.expire_recordings() + + assert Recordings.get_or_none(Recordings.id == "first") is not None + assert Recordings.get_or_none(Recordings.id == "second") is not None + + +if __name__ == "__main__": + unittest.main()