diff --git a/CHANGELOG.md b/CHANGELOG.md index 670e6b6..d7f315b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.20] - 2026-06-15 + +### Fixed + +- **`_migrate_schema_v2` is now crash-safe**: the previous implementation used `conn.executescript()`, which issues an implicit `COMMIT` before executing — so a process kill between the `DROP TABLE` and the `ALTER TABLE RENAME` would permanently destroy `tracked_assets` with no rollback. Replaced with individual `conn.execute()` calls inside a `with conn:` transaction so the entire migration rolls back on failure. + +- **`FRIGATE_URL` with a trailing slash no longer produces double-slash upload paths**: `upload_to_frigate` in `executor.py` read `os.environ.get("FRIGATE_URL", "")` directly, bypassing the `.rstrip("/")` normalization in `frigate_api._get_frigate_url()`. A `FRIGATE_URL` ending in `/` produced paths like `/api/faces//Alice/register` for uploads while all other Frigate API calls used the cleaned URL. Both `executor.py` and `upload_tracker.reset_person` now call `_get_frigate_url()` instead of reading the env var inline. + +- **Corrupt thumbnail content now marks the asset rejected**: when `resp.ok=True` but `Image.open()` raises (corrupt JPEG bytes from Immich), the asset was silently skipped with no tracker entry, causing it to be re-selected and re-downloaded on every future run. The path now calls `mark_rejected()` so a permanently corrupt thumbnail doesn't cause an indefinite retry loop. + +- **`_save_jpeg` writes atomically**: the face crop JPEG was written directly to its final path — a disk-full or PIL encode error mid-write would leave a truncated file at the output path with no cleanup. The helper now writes to `{path}.tmp` and only calls `os.replace()` on success; on failure the temporary file is removed and the exception is re-raised. + +- **`_handle_duplicate_people` deduplicates even when all Immich merges fail**: when `MERGE_DUPLICATE_PEOPLE=true` and every `merge_people()` call returns `False`, the function previously returned the original unfiltered people list. Two jobs for the same person then ran sequentially, with the second job's `shutil.rmtree` wiping the first job's uploaded crops. The function now falls back to local deduplication (keep largest per name) whenever merging fails. + +- **`_get_frigate_url()` strips leading/trailing whitespace**: `os.environ.get("FRIGATE_URL", "").rstrip("/")` left whitespace-only values like `" "` as truthy, allowing them to reach API calls as malformed URLs. Added `.strip()` before `.rstrip("/")` so a whitespace-only value collapses to the empty string and is treated as unset. + +### Changed + +- **`_getenv_optional_float` now delegates to `_getenv_num`**: the function hand-rolled its own strip/cast/warn/None logic instead of calling `_getenv_num(name, None, float)` the way `_getenv_optional_int` does. Both optional helpers are now consistent and pick up any future changes to the shared `_getenv_num` implementation automatically. + +- **`reconcile._ts()` strips any file extension, not just `.webp`**: the Frigate timestamp extracted from training filenames used `.replace(".webp", "")`, which silently returns `0.0` for any non-`.webp` filename and produces undefined-order FIFO mappings if Frigate ever changes its training-file extension. Replaced with `.rsplit(".", 1)[0]` to strip the last extension generically. + ## [0.5.19] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 0e2488b..087bd4e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.19" +version = "0.5.20" description = "Selects diverse, high-quality photos from Immich as training data for Frigate face recognition." license = "AGPL-3.0-or-later" requires-python = ">=3.13" diff --git a/winnow/cli.py b/winnow/cli.py index 10f627b..2e338fa 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -127,7 +127,18 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: rprint(" [dim]Re-fetching people after merge...[/dim]") return get_people() - return people + # All merges failed — fall back to local deduplication (keep largest per name) so + # downstream job creation never runs two jobs for the same Frigate folder. + rprint( + " [yellow]All merges failed — applying local deduplication" + " to avoid overwriting output.[/yellow]" + ) + skip_ids = { + p["id"] + for ps in duplicates.values() + for p in sorted(ps, key=lambda x: x.get("assetCount", 0), reverse=True)[1:] + } + return [p for p in people if p["id"] not in skip_ids] _UNSUPPORTED_VARS = [ diff --git a/winnow/config.py b/winnow/config.py index d164d83..8bde0dc 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -35,14 +35,8 @@ def _getenv_float(name: str, default: float) -> float: def _getenv_optional_float(name: str) -> float | None: - raw = os.getenv(name, "").strip() - if not raw: - return None - try: - return float(raw) - except ValueError: - logging.warning("%s=%r is not a valid float — ignoring", name, raw) - return None + """Return float value of env var, or None if unset/empty. Warns and returns None on invalid.""" + return _getenv_num(name, None, float) def _getenv_optional_int(name: str) -> int | None: diff --git a/winnow/executor.py b/winnow/executor.py index 3059122..bb6f683 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -13,6 +13,7 @@ from rich.progress import BarColumn, Progress, SpinnerColumn, TaskProgressColumn from .config import Config, get_headers from .frigate_api import ( + _get_frigate_url, delete_frigate_person_files, get_all_frigate_person_files, get_frigate_person_files, @@ -158,7 +159,11 @@ def execute_jobs(jobs: list[dict]) -> None: try: img = Image.open(BytesIO(resp.content)) except Exception: - logger.warning("Invalid image data for asset %s", asset["id"]) + # resp.ok=True but content is unreadable — corrupt Immich + # thumbnail. Mark rejected so this asset isn't retried + # indefinitely on future runs. + logger.warning("Invalid image data for asset %s — marking rejected", asset["id"]) + mark_rejected(asset["id"], person_name=name) img = None else: img = None @@ -216,7 +221,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: rprint("[dim]No jobs to upload.[/dim]") return - frigate_url = os.environ.get("FRIGATE_URL", "") + frigate_url = _get_frigate_url() if not frigate_url: rprint("[yellow]⚠️ FRIGATE_URL not set, skipping upload.[/yellow]") return diff --git a/winnow/frigate_api.py b/winnow/frigate_api.py index 4f5cafc..3d15dd7 100644 --- a/winnow/frigate_api.py +++ b/winnow/frigate_api.py @@ -9,8 +9,8 @@ logger = logging.getLogger(__name__) def _get_frigate_url() -> str: - """Return normalized FRIGATE_URL with trailing slash stripped, or '' if unset.""" - return os.environ.get("FRIGATE_URL", "").rstrip("/") + """Return normalized FRIGATE_URL with whitespace and trailing slash stripped, or '' if unset.""" + return os.environ.get("FRIGATE_URL", "").strip().rstrip("/") def get_frigate_version() -> str | None: diff --git a/winnow/image_processing.py b/winnow/image_processing.py index 04d21af..7ded632 100644 --- a/winnow/image_processing.py +++ b/winnow/image_processing.py @@ -15,7 +15,16 @@ logger = logging.getLogger(__name__) def _save_jpeg(img: Image.Image, path: str) -> None: if img.mode != "RGB": img = img.convert("RGB") - img.save(path, format="JPEG") + tmp = path + ".tmp" + try: + img.save(tmp, format="JPEG") + os.replace(tmp, path) + except Exception: + try: + os.remove(tmp) + except OSError: + pass + raise def align_face(img: Image.Image, landmarks: list[list[float]] | np.ndarray) -> Image.Image | None: diff --git a/winnow/reconcile.py b/winnow/reconcile.py index 652097c..2a4f251 100644 --- a/winnow/reconcile.py +++ b/winnow/reconcile.py @@ -59,7 +59,7 @@ def reconcile_frigate_mappings( if len(new_files) == target: def _ts(fname: str) -> float: try: - return float(fname.rsplit("_", 1)[-1].replace(".webp", "")) + return float(fname.rsplit("_", 1)[-1].rsplit(".", 1)[0]) except (ValueError, IndexError): return 0.0 diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 655535b..22cf9d0 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -20,7 +20,7 @@ import os import sqlite3 from pathlib import Path -from .frigate_api import delete_frigate_person_files +from .frigate_api import _get_frigate_url, delete_frigate_person_files logger = logging.getLogger(__name__) @@ -76,23 +76,29 @@ def _migrate_schema_v2(conn: sqlite3.Connection) -> None: return # Already at new schema logger.info("Migrating tracked_assets: adding person_name to primary key") - conn.executescript(""" - CREATE TABLE tracked_assets_new ( - asset_id TEXT NOT NULL, - person_name TEXT, - status TEXT NOT NULL CHECK(status IN ('uploaded', 'rejected')), - blur_score REAL, - crop_width INTEGER, - crop_height INTEGER, - frigate_score REAL, - PRIMARY KEY (asset_id, person_name, status) - ); - INSERT OR IGNORE INTO tracked_assets_new - SELECT asset_id, person_name, status, blur_score, crop_width, crop_height, frigate_score - FROM tracked_assets; - DROP TABLE tracked_assets; - ALTER TABLE tracked_assets_new RENAME TO tracked_assets; - """) + # Use individual execute() calls inside a transaction — executescript() issues an + # implicit COMMIT before running, so a crash between DROP and RENAME would + # permanently destroy the table with no rollback path. + with conn: + conn.execute(""" + CREATE TABLE tracked_assets_new ( + asset_id TEXT NOT NULL, + person_name TEXT, + status TEXT NOT NULL CHECK(status IN ('uploaded', 'rejected')), + blur_score REAL, + crop_width INTEGER, + crop_height INTEGER, + frigate_score REAL, + PRIMARY KEY (asset_id, person_name, status) + ) + """) + conn.execute(""" + INSERT OR IGNORE INTO tracked_assets_new + SELECT asset_id, person_name, status, blur_score, crop_width, crop_height, frigate_score + FROM tracked_assets + """) + conn.execute("DROP TABLE tracked_assets") + conn.execute("ALTER TABLE tracked_assets_new RENAME TO tracked_assets") logger.info("tracked_assets schema migration complete") @@ -453,7 +459,7 @@ def reset_person(person_name: str) -> None: # Collect Frigate filenames before deleting frigate_filenames = list(get_tracked_frigate_filenames(person_name)) if frigate_filenames: - if not os.environ.get("FRIGATE_URL", "").strip(): + if not _get_frigate_url(): logger.info("FRIGATE_URL not set — skipping Frigate file deletion for %s", person_name) elif delete_frigate_person_files(person_name, frigate_filenames): logger.info("Deleted %s Frigate file(s) for %s", len(frigate_filenames), person_name)