nik/frigate
1
0
Fork 0
mirror of https://github.com/blakeblackshear/frigate.git synced 2026-08-31 07:27:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix inconsistent export download filenames (#24111)

Browse Source 
* 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
This commit is contained in:
Josh Hawkins 2026-08-27 08:02:37 -05:00
parent 194e9bef69
commit a2085e27f6
4 changed files with 252 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
frigate/api/export.py
View File

@ -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=(
{

23
frigate/record/export.py
View File

@ -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 = {

85
frigate/test/http_api/test_http_export.py
View File

@ -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")

82
frigate/test/test_export.py
View File

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