From 14f759e960ae8b152512e74710db5eefd03877af Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 18:13:36 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20address=203=20quality=20review=20finding?= =?UTF-8?q?s=20=E2=80=94=20record=5Ffrigate=5Ffiles=5Fbatch=20cache=20muta?= =?UTF-8?q?tion,=20tracker=5Fok=20flag,=20LIMIT=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - record_frigate_files_batch: copy-before-mutate so a write failure doesn't leave cache ahead of disk (same fix as remove_frigate_files_batch) - executor: replace tracker_ok boolean with try/else - jobs: collapse duplicate custom_limit is not None checks into one guard Bump version to 0.6.3. --- CHANGELOG.md | 10 ++++++++++ pyproject.toml | 2 +- winnow/executor.py | 4 +--- winnow/jobs.py | 6 +++--- winnow/upload_tracker.py | 6 ++++-- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b80c65c..276c536 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.6.3] - 2026-06-16 + +### Fixed + +- **`record_frigate_files_batch` no longer mutates the tracker cache before write**: the function shared the same cache-corruption-on-write-failure bug that was fixed in `remove_frigate_files_batch` in v0.6.1 — `data.setdefault("by_person", {})` mutated the cached dict in-place, so a disk-full or permission error left the in-memory cache ahead of the on-disk file. Now uses the same copy-before-mutate pattern (shallow copies of the top-level dict and `by_person` sub-dict) so a failed write leaves cache and disk in sync. + +- **`tracker_ok` boolean flag replaced with try/else**: the intermediate boolean was a misleading placeholder — the `True` initial value suggested success before the operation ran. The control flow is now expressed directly with a try/except/else block. + +- **`LIMIT` env var guard simplified**: the two adjacent `if custom_limit is not None` checks in `_resolve_strategy` are collapsed into a single `if custom_limit is not None:` with nested branches, removing redundant evaluation. + ## [0.6.2] - 2026-06-16 ### Changed diff --git a/pyproject.toml b/pyproject.toml index fd6c77d..a4437a0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.6.2" +version = "0.6.3" 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/executor.py b/winnow/executor.py index ca159e5..c5fee48 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -510,7 +510,6 @@ def upload_to_frigate(jobs: list[dict]) -> None: asset_id = asset_map.get(fname) if asset_id: - tracker_ok = True try: mark_uploaded( asset_id, @@ -520,7 +519,6 @@ def upload_to_frigate(jobs: list[dict]) -> None: frigate_score=pre_fscore, ) except Exception as tracker_exc: - tracker_ok = False # Upload to Frigate succeeded — don't retry on tracker # failure or we'd upload a duplicate to Frigate. logger.error( @@ -528,7 +526,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: " but asset may be re-selected next run: %s", fname, tracker_exc, ) - if tracker_ok: + else: if pre_fscore is not None: person_has_fscores = True actually_uploaded.append((fname, asset_id)) diff --git a/winnow/jobs.py b/winnow/jobs.py index 941551e..b8243c7 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -69,9 +69,9 @@ def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, st return _getenv_int("LIMIT", 30), "time" custom_limit = _getenv_optional_int("LIMIT") - if custom_limit is not None and custom_limit > 0: - return custom_limit, "smart" - if custom_limit is not None and custom_limit <= 0: + if custom_limit is not None: + if custom_limit > 0: + return custom_limit, "smart" logger.warning("LIMIT=%s is invalid — ignoring and using auto strategy", custom_limit) strategy_map = { diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 6089938..3b56678 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -223,11 +223,13 @@ def record_frigate_files_batch(person_name: str, mappings: dict[str, str]) -> No """Record multiple Frigate filename → asset_id mappings in a single load/save.""" if not mappings: return - data = _load(UPLOAD_TRACKER_FILE) - by_person = data.setdefault("by_person", {}) + src = _load(UPLOAD_TRACKER_FILE) + by_person = dict(src.get("by_person", {})) entry = _migrate_entry(by_person.get(person_name, {})) entry["frigate_files"].update(mappings) by_person[person_name] = entry + data = dict(src) + data["by_person"] = by_person _save(UPLOAD_TRACKER_FILE, data) logger.debug(f"Batch-mapped {len(mappings)} Frigate file(s) for {person_name}")