check every overlapping review item before dropping a recording (#24728)

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/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}

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()