From a2085e27f6a2a10ae8fbb309768776d45581b524 Mon Sep 17 00:00:00 2001 From: Josh Hawkins <32435876+hawkeye217@users.noreply.github.com> Date: Thu, 27 Aug 2026 08:02:37 -0500 Subject: [PATCH] Fix inconsistent export download filenames (#24111) * fix inconsistent export download filenames Zip entries in a case download were named from `Export.name`, the friendly display name, while an individual download uses the file name on disk. The two have always been formatted differently, so one export came out as `front_door_20260823_020615-20260823_020734_abc123.mp4` on its own and `front door 2026-08-23 020615 2026-08-23 020734.mp4` inside a zip. Zip entries now use the on-disk file name, and renaming an export renames its file, so there's only one name to download under. The rename is blocked while ffmpeg still holds the file. * cap filename length and catch duplicate names * fix export rename and stop blocking the event loop * move the rename rollback off the event loop * no awaits --- frigate/api/export.py | 71 +++++++++++++++++-- frigate/record/export.py | 23 +++++- frigate/test/http_api/test_http_export.py | 85 +++++++++++++++++++++++ frigate/test/test_export.py | 82 +++++++++++++++++++++- 4 files changed, 252 insertions(+), 9 deletions(-) diff --git a/frigate/api/export.py b/frigate/api/export.py index 6a4a6d5041..9e7b1e3735 100644 --- a/frigate/api/export.py +++ b/frigate/api/export.py @@ -1,7 +1,9 @@ """Export apis.""" +import contextlib import datetime import logging +import os import random import string import time @@ -15,7 +17,7 @@ import psutil from fastapi import APIRouter, Depends, Query, Request from fastapi.responses import JSONResponse, StreamingResponse from pathvalidate import sanitize_filename -from peewee import DoesNotExist +from peewee import DatabaseError, DoesNotExist, IntegrityError from playhouse.shortcuts import model_to_dict from frigate.api.auth import ( @@ -71,6 +73,7 @@ from frigate.record.export import ( DEFAULT_TIME_LAPSE_FFMPEG_ARGS, ChaptersEnum, PlaybackSourceEnum, + export_video_path, validate_ffmpeg_args, ) from frigate.util.path import sanitize_contained_path @@ -404,14 +407,17 @@ class _StreamingZipBuffer: def _unique_archive_name(export: Export, used: set[str]) -> str: - base = sanitize_filename(export.name) if export.name else None - if not base: - base = f"{export.camera}_{int(export.date)}" + """Zip entry name for an export, de-duplicated within the archive. + + The on-disk name is the one the user sees either way: renaming an export + renames its file, so a zip entry and an individual download can't drift. + """ + source = Path(export.video_path) + candidate = source.name - candidate = f"{base}.mp4" counter = 1 while candidate in used: - candidate = f"{base}_{counter}.mp4" + candidate = f"{source.stem}_{counter}{source.suffix}" counter += 1 used.add(candidate) @@ -927,8 +933,59 @@ async def export_rename(event_id: str, body: ExportRenameBody, request: Request) status_code=404, ) + if export.in_progress: + return JSONResponse( + content={ + "success": False, + "message": "Export is still being written and can't be renamed yet.", + }, + status_code=400, + ) + + new_path = export_video_path(body.name, export.id) + old_path = export.video_path + moved = new_path != old_path + + # move the file first so a rename that can't happen leaves the row alone + if moved: + try: + os.rename(old_path, new_path) + except OSError: + logger.exception("Failed to rename export file for %s", event_id) + return JSONResponse( + content={"success": False, "message": "Failed to rename export."}, + status_code=500, + ) + export.name = body.name - export.save() + export.video_path = new_path + + try: + export.save() + except DatabaseError as err: + # the queue database has no transactions, so undo the move by hand + if moved: + with contextlib.suppress(OSError): + os.rename(new_path, old_path) + + if isinstance(err, IntegrityError): + logger.warning( + "Export %s cannot be renamed, %s is taken", event_id, new_path + ) + return JSONResponse( + content={ + "success": False, + "message": "Another export already uses that name.", + }, + status_code=409, + ) + + logger.exception("Failed to save renamed export %s", event_id) + return JSONResponse( + content={"success": False, "message": "Failed to rename export."}, + status_code=500, + ) + return JSONResponse( content=( { diff --git a/frigate/record/export.py b/frigate/record/export.py index 3f68a7e1b1..0a84cd4bee 100644 --- a/frigate/record/export.py +++ b/frigate/record/export.py @@ -15,6 +15,7 @@ from pathlib import Path from typing import Any import pytz # type: ignore[import-untyped] +from pathvalidate import sanitize_filename from peewee import DoesNotExist from frigate.config import FfmpegConfig, FrigateConfig @@ -204,6 +205,22 @@ class PlaybackSourceEnum(str, Enum): preview = "preview" +EXPORT_FILE_NAME_MAX_BYTES = 255 + + +def export_video_path(name: str, export_id: str) -> str: + """Path an export's video is stored at once the user has named it. + + The id suffix keeps the path unique when two exports share a name, and + keeps the result a single path component whatever the user typed. + """ + suffix = f"_{export_id.split('_')[-1]}.mp4" + budget = EXPORT_FILE_NAME_MAX_BYTES - len(suffix.encode()) + stem = sanitize_filename(name).encode()[:budget].decode(errors="ignore") + + return os.path.join(EXPORT_DIR, f"{stem.strip('. ') or 'export'}{suffix}") + + class RecordingExporter(threading.Thread): """Exports a specific set of recordings for a camera to storage as a single file.""" @@ -917,7 +934,11 @@ class RecordingExporter(threading.Thread): "%Y%m%d_%H%M%S" ) cleaned_export_id = self.export_id.split("_")[-1] - video_path = f"{EXPORT_DIR}/{self.camera}_{filename_start_datetime}-{filename_end_datetime}_{cleaned_export_id}.mp4" + + if self.user_provided_name: + video_path = export_video_path(self.user_provided_name, self.export_id) + else: + video_path = f"{EXPORT_DIR}/{self.camera}_{filename_start_datetime}-{filename_end_datetime}_{cleaned_export_id}.mp4" thumb_path = self.save_thumbnail(self.export_id) export_values = { diff --git a/frigate/test/http_api/test_http_export.py b/frigate/test/http_api/test_http_export.py index 44eb0c2c4a..1469d80bb2 100644 --- a/frigate/test/http_api/test_http_export.py +++ b/frigate/test/http_api/test_http_export.py @@ -368,6 +368,91 @@ class TestHttpExport(BaseTestHttp): assert response.status_code == 200 assert response.json() == [queued_job.to_dict()] + def test_rename_export_moves_the_file(self): + with tempfile.TemporaryDirectory() as tmpdir: + video = os.path.join(tmpdir, "front_door_20260823_020615_abc123.mp4") + thumb = os.path.join(tmpdir, "front_door_abc123.webp") + for path, data in ((video, b"video"), (thumb, b"thumb")): + with open(path, "wb") as handle: + handle.write(data) + + Export.create( + id="front_door_abc123", + camera="front_door", + name="front door 2026-08-23 02:06:15 2026-08-23 02:07:34", + date=100, + video_path=video, + thumb_path=thumb, + in_progress=False, + ) + + with patch("frigate.record.export.EXPORT_DIR", tmpdir): + with AuthTestClient(self.app) as client: + response = client.patch( + "/export/front_door_abc123/rename", + json={"name": "Package thief"}, + ) + + assert response.status_code == 200 + + renamed = Export.get(Export.id == "front_door_abc123") + assert renamed.name == "Package thief" + assert os.path.basename(renamed.video_path) == "Package thief_abc123.mp4" + assert os.path.exists(renamed.video_path) + assert not os.path.exists(video) + + def test_rename_export_rejected_while_in_progress(self): + with tempfile.TemporaryDirectory() as tmpdir: + video = os.path.join(tmpdir, "front_door_abc123.mp4") + with open(video, "wb") as handle: + handle.write(b"video") + + Export.create( + id="front_door_running", + camera="front_door", + name="front door export", + date=100, + video_path=video, + thumb_path=os.path.join(tmpdir, "t.webp"), + in_progress=True, + ) + + with AuthTestClient(self.app) as client: + response = client.patch( + "/export/front_door_running/rename", + json={"name": "Package thief"}, + ) + + assert response.status_code == 400 + assert Export.get(Export.id == "front_door_running").video_path == video + + def test_rename_export_missing_file_leaves_the_row_alone(self): + with tempfile.TemporaryDirectory() as tmpdir: + video = os.path.join(tmpdir, "front_door_gone_abc123.mp4") + + Export.create( + id="front_door_gone", + camera="front_door", + name="front door export", + date=100, + video_path=video, + thumb_path=os.path.join(tmpdir, "t.webp"), + in_progress=False, + ) + + with patch("frigate.record.export.EXPORT_DIR", tmpdir): + with AuthTestClient(self.app) as client: + response = client.patch( + "/export/front_door_gone/rename", + json={"name": "Package thief"}, + ) + + assert response.status_code == 500 + + unchanged = Export.get(Export.id == "front_door_gone") + assert unchanged.name == "front door export" + assert unchanged.video_path == video + def test_reap_stale_exports_deletes_rows_with_no_file(self): with tempfile.TemporaryDirectory() as tmpdir: stale_video = os.path.join(tmpdir, "stale.mp4") diff --git a/frigate/test/test_export.py b/frigate/test/test_export.py index 7612a4144f..84c8c32f0c 100644 --- a/frigate/test/test_export.py +++ b/frigate/test/test_export.py @@ -1,6 +1,9 @@ import unittest +from pathlib import Path -from frigate.record.export import validate_ffmpeg_args +from frigate.api.export import _unique_archive_name +from frigate.models import Export +from frigate.record.export import export_video_path, validate_ffmpeg_args class TestValidateFfmpegArgs(unittest.TestCase): @@ -128,5 +131,82 @@ class TestValidateFfmpegArgs(unittest.TestCase): self.assertRejected("-metadata comment=x") +class TestExportVideoPath(unittest.TestCase): + """Tests for the file path an export takes once the user names it.""" + + EXPORT_ID = "front_door_abc123" + + def test_uses_the_name_the_user_gave(self): + self.assertEqual( + export_video_path("Package thief", self.EXPORT_ID), + "/media/frigate/exports/Package thief_abc123.mp4", + ) + + def test_id_suffix_keeps_shared_names_apart(self): + self.assertNotEqual( + export_video_path("clip", "front_door_abc123"), + export_video_path("clip", "front_door_def456"), + ) + + def test_long_names_fit_the_filesystem_limit(self): + # Names are capped in bytes, not characters: 244 CJK characters is + # under any character cap and still 732 bytes on disk. + for name in ("A" * 256, "\u76e3" * 256, "\U0001f3a5" * 100): + file_name = Path(export_video_path(name, self.EXPORT_ID)).name + self.assertLessEqual(len(file_name.encode()), 255) + + def test_truncation_keeps_the_name_decodable(self): + file_name = Path(export_video_path("\u76e3" * 256, self.EXPORT_ID)).name + self.assertTrue(file_name.endswith("_abc123.mp4")) + self.assertNotIn("\ufffd", file_name) + + def test_stays_inside_the_export_dir(self): + for name in ("../../etc/passwd", "..", "a/b", "...", ""): + path = Path(export_video_path(name, self.EXPORT_ID)) + self.assertEqual(str(path.parent), "/media/frigate/exports") + + +class TestUniqueArchiveName(unittest.TestCase): + """Tests for zip entry names in a case download. + + Entries use the on-disk file name, which is also what an individual + download produces, so the two can't drift. + """ + + def build_export(self, video_path: str) -> Export: + return Export( + id="front_door_abc123", + camera="front_door", + name="whatever the display name is", + date=1756000000.0, + video_path=video_path, + thumb_path=video_path.replace(".mp4", ".webp"), + in_progress=False, + ) + + def test_uses_the_on_disk_file_name(self): + export = self.build_export( + "/media/frigate/exports/front_door_20260823_020615-20260823_020734_abc123.mp4" + ) + self.assertEqual( + _unique_archive_name(export, set()), + "front_door_20260823_020615-20260823_020734_abc123.mp4", + ) + + def test_follows_a_renamed_file(self): + export = self.build_export("/media/frigate/exports/Package thief_abc123.mp4") + self.assertEqual( + _unique_archive_name(export, set()), "Package thief_abc123.mp4" + ) + + def test_entries_are_deduplicated(self): + export = self.build_export("/media/frigate/exports/Package thief_abc123.mp4") + used: set[str] = set() + self.assertEqual(_unique_archive_name(export, used), "Package thief_abc123.mp4") + self.assertEqual( + _unique_archive_name(export, used), "Package thief_abc123_1.mp4" + ) + + if __name__ == "__main__": unittest.main()