Apply data check to all cases when validating a segment (#24515)

frigate/record/maintainer.py

@@ -388,6 +388,22 @@ class RecordingMaintainer(threading.Thread):
                 RecordingsDataTypeEnum.valid.value,
             )
 
+        # assume that empty means the relevant recording info has not been received yet
+        camera_info = self.object_recordings_info[camera]
+        most_recently_processed_frame_time = (
+            camera_info[-1][0] if len(camera_info) > 0 else 0
+        )
+
+        # ensure delayed segment info does not lead to lost segments, every
+        # retention decision below depends on complete stats for the segment
+        if (
+            datetime.datetime.fromtimestamp(
+                most_recently_processed_frame_time
+            ).astimezone(datetime.UTC)
+            < end_time
+        ):
+            return None
+
         record_config = self.config.cameras[camera].record
         segment_stats: SegmentInfo | None = None
         highest = None
@@ -401,37 +417,22 @@ class RecordingMaintainer(threading.Thread):
         # we should first just check if this segment matches that
         # and avoid any DB calls
         if highest is not None:
-            # assume that empty means the relevant recording info has not been received yet
-            camera_info = self.object_recordings_info[camera]
-            most_recently_processed_frame_time = (
-                camera_info[-1][0] if len(camera_info) > 0 else 0
+            record_mode = (
+                RetainModeEnum.all if highest == "continuous" else RetainModeEnum.motion
             )
+            segment_stats = self.segment_stats(camera, start_time, end_time)
 
-            # ensure delayed segment info does not lead to lost segments
-            if (
-                datetime.datetime.fromtimestamp(
-                    most_recently_processed_frame_time
-                ).astimezone(datetime.UTC)
-                >= end_time
-            ):
-                record_mode = (
-                    RetainModeEnum.all
-                    if highest == "continuous"
-                    else RetainModeEnum.motion
+            # Here we only check if we should move the segment based on non-object recording retention
+            # we will always want to check for overlapping review items below before dropping the segment
+            if not segment_stats.should_discard_segment(record_mode):
+                return await self.move_segment(
+                    camera,
+                    start_time,
+                    end_time,
+                    duration,
+                    cache_path,
+                    segment_stats,
                 )
-                segment_stats = self.segment_stats(camera, start_time, end_time)
-
-                # Here we only check if we should move the segment based on non-object recording retention
-                # we will always want to check for overlapping review items below before dropping the segment
-                if not segment_stats.should_discard_segment(record_mode):
-                    return await self.move_segment(
-                        camera,
-                        start_time,
-                        end_time,
-                        duration,
-                        cache_path,
-                        segment_stats,
-                    )
 
         # we fell through the continuous / motion check, so we need to check the review items
         # if the cached segment overlaps with the review items:
@@ -487,10 +488,6 @@ class RecordingMaintainer(threading.Thread):
         # continuous/motion retention (either disabled or segment_stats said
         # discard), so waiting longer just fills the cache.
         else:
-            camera_info = self.object_recordings_info[camera]
-            most_recently_processed_frame_time = (
-                camera_info[-1][0] if len(camera_info) > 0 else 0
-            )
             retain_cutoff = datetime.datetime.fromtimestamp(
                 most_recently_processed_frame_time - record_config.event_pre_capture
             ).astimezone(datetime.UTC)

frigate/test/test_maintainer.py

@@ -1,7 +1,7 @@
 import datetime
 import sys
 import unittest
-from unittest.mock import MagicMock, patch
+from unittest.mock import AsyncMock, MagicMock, patch
 
 # Mock complex imports before importing maintainer, saving originals so we can
 # restore them after import and avoid polluting sys.modules for other tests.
@@ -16,7 +16,7 @@ for name in _MOCKED_MODULES:
     sys.modules[name] = MagicMock()
 
 # Now import the class under test
-from frigate.config import FrigateConfig  # noqa: E402
+from frigate.config import FrigateConfig, RetainModeEnum  # noqa: E402
 from frigate.record.maintainer import RecordingMaintainer  # noqa: E402
 
 # Restore original modules (or remove mock if there was no original)
@@ -115,6 +115,70 @@ class TestMaintainer(unittest.IsolatedAsyncioTestCase):
         self.assertIsNone(result)
         maintainer.drop_segment.assert_called_once_with(cache_path)
 
+    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,
+        # the missing frames may hold the active objects (or continuous
+        # retention would keep it anyway).
+        config = MagicMock(spec=FrigateConfig)
+
+        camera_config = MagicMock()
+        camera_config.record.enabled = True
+        camera_config.record.continuous.days = 7
+        camera_config.record.motion.days = 0
+        camera_config.record.alerts.retain.mode = RetainModeEnum.active_objects
+        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}
+
+        stop_event = MagicMock()
+        maintainer = RecordingMaintainer(config, stop_event)
+
+        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)}
+        # Metadata has only reached partway into the segment.
+        maintainer.object_recordings_info["test_cam"] = [
+            (end_time.timestamp() - 8, [], [], [])
+        ]
+        maintainer.audio_recordings_info["test_cam"] = []
+
+        maintainer.drop_segment = MagicMock()
+        maintainer.move_segment = AsyncMock(return_value=None)
+        maintainer.recordings_publisher = MagicMock()
+
+        review = MagicMock()
+        review.severity = "alert"
+        review.start_time = start_time.timestamp() - 30
+        review.end_time = None
+
+        result = await maintainer.validate_and_move_segment(
+            "test_cam",
+            reviews=[review],
+            recording={"start_time": start_time, "cache_path": cache_path},
+        )
+
+        self.assertIsNone(result)
+        maintainer.drop_segment.assert_not_called()
+        maintainer.move_segment.assert_not_awaited()
+
+        # Once metadata passes the segment end, continuous retention keeps it.
+        maintainer.object_recordings_info["test_cam"].append(
+            (now.timestamp(), [], [], [])
+        )
+
+        await maintainer.validate_and_move_segment(
+            "test_cam",
+            reviews=[review],
+            recording={"start_time": start_time, "cache_path": cache_path},
+        )
+
+        maintainer.drop_segment.assert_not_called()
+        maintainer.move_segment.assert_awaited_once()
+
     async def test_expire_stale_recordings_info_drops_only_absent_cameras(self):
         config = MagicMock(spec=FrigateConfig)
         config.cameras = {}