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/config/camera/record.py

@@ -224,14 +224,6 @@ class RecordConfig(FrigateBaseModel):
             self.detections.pre_capture,
         )
 
-    @property
-    def event_post_capture(self) -> int:
-        """Largest post-capture window used to fetch candidate reviews."""
-        return max(
-            self.alerts.post_capture,
-            self.detections.post_capture,
-        )
-
     def get_review_pre_capture(self, severity: SeverityEnum) -> int:
         if severity == SeverityEnum.alert:
             return self.alerts.pre_capture

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)

frigate/record/maintainer.py

@@ -471,14 +471,9 @@ class RecordingMaintainer(threading.Thread):
         for key, recordings in grouped_recordings.items():
             camera, stream_type = key
 
-            # get all reviews whose post capture reaches the oldest cache file
-            # or with end_time None; shared across stream types
+            # get all reviews with the end time after the start of the oldest
+            # cache file or with end_time None; shared across stream types
             if camera not in reviews_by_camera:
-                post_capture = (
-                    self.config.cameras[camera].record.event_post_capture
-                    if camera in self.config.cameras
-                    else 0
-                )
                 reviews_by_camera[camera] = (
                     ReviewSegment.select(
                         ReviewSegment.start_time,
@@ -489,10 +484,7 @@ class RecordingMaintainer(threading.Thread):
                     .where(
                         ReviewSegment.camera == camera,
                         (ReviewSegment.end_time == None)
-                        | (
-                            ReviewSegment.end_time
-                            >= min_start_per_camera[camera] - post_capture
-                        ),
+                        | (ReviewSegment.end_time >= min_start_per_camera[camera]),
                     )
                     .order_by(ReviewSegment.start_time)
                 )
@@ -767,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,

frigate/test/test_maintainer.py

@@ -6,8 +6,6 @@ import unittest
 from pathlib import Path
 from unittest.mock import AsyncMock, MagicMock, patch
 
-from peewee import SqliteDatabase
-
 # Mock complex imports before importing maintainer, saving originals so we can
 # restore them after import and avoid polluting sys.modules for other tests.
 _MOCKED_MODULES = [
@@ -22,7 +20,6 @@ for name in _MOCKED_MODULES:
 
 # Now import the class under test
 from frigate.config import FrigateConfig, RetainModeEnum  # noqa: E402
-from frigate.models import ReviewSegment  # noqa: E402
 from frigate.record.maintainer import RecordingMaintainer, SegmentInfo  # noqa: E402
 
 # Restore original modules (or remove mock if there was no original)
@@ -84,53 +81,6 @@ class TestMaintainer(unittest.IsolatedAsyncioTestCase):
                             f"Expected a single warning for unexpected files, got {len(matching)}",
                         )
 
-    async def test_ended_review_stays_eligible_through_post_capture(self):
-        config = MagicMock(spec=FrigateConfig)
-        camera_config = MagicMock()
-        camera_config.record.sub.enabled = False
-        camera_config.record.event_post_capture = 60
-        config.cameras = {"test_cam": camera_config}
-
-        maintainer = RecordingMaintainer(config, MagicMock())
-        maintainer.requestor = MagicMock()
-        maintainer.recordings_publisher = MagicMock()
-        maintainer.validate_and_move_segment = AsyncMock(return_value=None)
-
-        db = SqliteDatabase(":memory:")
-        with db.bind_ctx([ReviewSegment]):
-            db.create_tables([ReviewSegment])
-
-            # oldest cached segment starts 40s after the review ended
-            segment_start = datetime.datetime(
-                2026, 10, 19, 13, 1, 40, tzinfo=datetime.UTC
-            )
-            ReviewSegment.create(
-                id="review",
-                camera="test_cam",
-                start_time=segment_start.timestamp() - 50,
-                end_time=segment_start.timestamp() - 40,
-                severity="alert",
-                thumb_path="",
-                data={},
-            )
-
-            with tempfile.TemporaryDirectory() as cache_dir:
-                name = segment_start.strftime("%Y%m%d%H%M%S%z")
-                Path(cache_dir, f"test_cam@{name}.mp4").touch()
-
-                with (
-                    patch("frigate.record.maintainer.CACHE_DIR", cache_dir),
-                    patch(
-                        "frigate.record.maintainer.psutil.process_iter", return_value=[]
-                    ),
-                ):
-                    await maintainer.move_files()
-
-            reviews = maintainer.validate_and_move_segment.call_args.args[1]
-            self.assertEqual(
-                [segment_start.timestamp() - 40], [r.end_time for r in reviews]
-            )
-
     async def test_drops_quiet_segment_when_only_motion_retention(self):
         # Regression: when motion retention is enabled but a segment has no
         # motion and no review overlaps it, the segment must still be dropped.
@@ -177,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,
@@ -189,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()