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.

docker/main/rootfs/usr/local/nginx/conf/nginx.conf

@@ -176,6 +176,12 @@ http {
             add_header Cache-Control "public";
             autoindex on;
             root /media/frigate;
+
+            # review thumbnails are rewritten in place while a review is active,
+            # so a cached file size truncates the response
+            location /clips/review/ {
+                open_file_cache off;
+            }
         }
 
         location /cache/ {

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
+            modes: set[RetainModeEnum] = set()
-            mode = None
+            # Now look for every reason to keep this recording segment
-            # Now look for a 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
+                # stop once no later review's pre capture can reach this segment
-                # and let this recording segment expire
+                if (
-                if review.start_time - pre_capture > recording.end_time:
+                    review.start_time - config.record.event_pre_capture
-                    keep = False
+                    > recording.end_time
+                ):
                     break
 
-                # if the review is in progress or ends after the recording starts, keep it
+                if review.start_time - pre_capture > recording.end_time:
-                # and stop looking at reviews
+                    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
+                    modes.add(
-                    mode = (
                         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.
+                # it on future segments too. reviews and recordings are sorted,
-                # since the review and recordings are sorted, we can skip review
+                # but an earlier review with a longer post capture may still
-                # that end before the previous recording segment started on future segments
+                # overlap, so only skip while nothing has overlapped yet
-                if review.end_time + post_capture < recording.start_time:
+                elif not modes:
                     review_start = idx
 
-            # Delete recordings outside of the retention window or based on the retention mode
+            # Delete recordings unless an overlapping review's retention mode keeps them
-            if (
+            if not any(
-                not keep
+                mode == RetainModeEnum.all
                 or (
                     mode == RetainModeEnum.motion
-                    and recording.motion == 0
+                    and (
-                    and recording.objects == 0
+                        recording.motion != 0
-                    and recording.dBFS == 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)

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
+            # stop once no later review item's pre capture can reach this segment
-            # and remove 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
+                continue
-                break
 
-            # if the review item is in progress or ends after the recording starts, keep it
+            # collect every review item that is in progress or ends after the
-            # and stop looking at review items
+            # 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
+                modes.add(
-                break
+                    alerts_retain_mode
-
+                    if review.severity == "alert"
-        if overlaps:
+                    else detections_retain_mode
-            record_mode = (
+                )
-                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,

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