From 1556d90bcca411ad44ce32d8014b92d7cd84e3b1 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 00:41:00 +0000 Subject: [PATCH 01/15] fix: correct path traversal docstring, total_raw inflation, and config re-stat (v0.5.8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - executor.py: fix _safe_person_dir docstring — realpath+startswith is the load-bearing traversal guard; islink is a supplementary early-exit for the symlink sub-case only. The previous comment "checking after realpath would be too late" implied islink was the primary guard, which is backwards. - immich_api.py: move total_raw accumulation to after the dead-end-page break so all-garbage pages don't inflate the count and produce misleading "N total, 0 recent" output. Mixed pages (some valid, some non-dict) still count page_count so transient schema issues don't shrink MIN_FACE_COUNT below threshold. Add warning when a RequestException interrupts pagination mid-way so operators know total_raw is a lower bound. - config.py: eliminate residual TOCTOU — change `if config_file.exists():` to `if _data_cfg_exists or config_file.exists():` so _data_cfg is never stat'd twice (the v0.5.7 fix cached the first check but not the second). --- CHANGELOG.md | 12 ++++++++++++ pyproject.toml | 2 +- winnow/config.py | 4 +++- winnow/executor.py | 8 +++++--- winnow/immich_api.py | 21 ++++++++++++++++----- winnow/jobs.py | 4 ++-- 6 files changed, 39 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be8490f..1a51214 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.8] - 2026-06-15 + +### Fixed + +- **`_safe_person_dir` docstring corrected**: the previous comment stated "checking after realpath would be too late because realpath follows the link first," which implied `islink` was the primary security guard. The load-bearing path-traversal check is `realpath + startswith`; it rejects both `../../` traversal and symlinks. The `islink` check is a supplementary early-exit that provides a cleaner error message for the symlink sub-case only. + +- **`total_raw` not inflated by all-garbage pages**: `fetch_all_assets` previously added `page_count` to `total_raw` before checking whether any valid dict items existed. A page returning only non-dict items would inflate `total_raw` and produce a misleading "N total, 0 recent" display. `total_raw` now accumulates only after `valid_assets` is confirmed non-empty, so all-garbage pages break without contributing. Mixed pages (some valid, some non-dict) still count `page_count` so transient schema glitches on a partial page don't cause `MIN_FACE_COUNT` to incorrectly skip a real person. + +- **Pagination interruption warning**: when a `RequestException` breaks pagination mid-way (page > 1), a `WARNING` is now logged noting that `total_raw` is a lower bound. Previously the exception was logged at `ERROR` with no indication that the `MIN_FACE_COUNT` comparison was using a partial count. + +- **Config TOCTOU residue**: `config.py` line 135 re-stat'd `_data_cfg` when it was the selected config file, creating a second TOCTOU window after the fix in v0.5.7. The check is now `if _data_cfg_exists or config_file.exists():` — the primary path is never stat'd again, and the legacy path is stat'd at most once. + ## [0.5.7] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 48446ab..10d0883 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.7" +version = "0.5.8" 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/config.py b/winnow/config.py index bc8abb0..dbc1339 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -132,7 +132,9 @@ class _Config: _data_cfg, ) config_file = _data_cfg if _data_cfg_exists else _LEGACY_CONFIG_FILE - if config_file.exists(): + # _data_cfg_exists already confirmed the primary path — avoid re-stat. + # The short-circuit means the legacy path is stat'd at most once here. + if _data_cfg_exists or config_file.exists(): try: data = json.loads(config_file.read_text()) if self.IMMICH_URL is None: diff --git a/winnow/executor.py b/winnow/executor.py index 965d4ec..155a2f3 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -42,9 +42,11 @@ def _safe_person_dir(output_dir: str, person_name: str) -> str: """Return the output subdirectory for a person, raising ValueError on path traversal. os.path.join silently discards output_dir when person_name is absolute, - and '../..' sequences resolve outside the tree. Both are rejected here. - Symlinks on the raw (unresolved) path are also rejected — checking after - realpath would be too late because realpath follows the link first. + and '../..' sequences resolve outside the tree. Both are rejected by the + realpath+startswith guard, which is the load-bearing security check. + The islink check below provides an earlier, cleaner error message for the + symlink sub-case; it is redundant with (not a replacement for) the + realpath+startswith traversal check. """ raw = os.path.join(output_dir, person_name) if os.path.islink(raw): diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 9e38358..9309da2 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -86,9 +86,11 @@ def merge_people(survivor_id: str, merge_ids: list[str]) -> bool: def fetch_all_assets(person: dict) -> tuple[list[dict], int]: """Fetch all assets for a person with pagination. - Returns (assets, raw_total) where assets is the list of valid dict items - and raw_total is the total item count seen before non-dict filtering. - raw_total may exceed len(assets) if Immich returned non-dict items. + Returns (assets, total_raw) where assets is the list of valid dict items + and total_raw is the raw item count across pages that had at least one valid + dict. All-garbage pages (every item non-dict) stop pagination and are not + counted. If pagination is interrupted by a network error, total_raw is a + lower bound (a warning is logged). """ name = person.get("name", "Unknown") person_id = person.get("id") @@ -101,7 +103,7 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: logger.debug("Fetching assets for %s...", name) assets: list[dict] = [] - total_raw = 0 # items seen across all pages before non-dict filtering + total_raw = 0 # raw item count across pages that yielded at least one valid dict for page in range(1, MAX_PAGES + 1): try: resp = requests.post( @@ -122,7 +124,6 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: page_assets = page_assets.get("items", []) page_count = len(page_assets) # raw count for termination check before filtering - total_raw += page_count # Single pass: partition valid assets from unexpected non-dict items valid_assets, skipped_count = [], 0 @@ -142,6 +143,11 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: ) break + # Count page_count (not just valid items) so that non-dict items from a + # transient schema issue on a mixed page don't cause MIN_FACE_COUNT to + # skip a real person. Pages where every item is a non-dict are excluded — + # they indicate a structural problem and break above without contributing. + total_raw += page_count assets.extend(valid_assets) logger.debug("Fetched page %s, total: %s", page, len(assets)) @@ -150,6 +156,11 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: except (requests.RequestException, ValueError) as e: logger.error("Exception fetching assets for %s (page %s): %s", name, page, e) + if page > 1: + logger.warning( + "%s: pagination interrupted at page %s — total_raw=%s may undercount actual assets", + name, page, total_raw, + ) break return assets, total_raw diff --git a/winnow/jobs.py b/winnow/jobs.py index 594ba13..4fbf2e0 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -268,8 +268,8 @@ def auto_configure(people: list[dict]) -> list[dict]: rprint(f" {name}: {total_raw} total, {len(recent_assets)} recent") # MIN_FACE_COUNT guard: skip people with too few Immich assets. - # Uses total_raw (pre-filter count) so non-dict items from a transient - # Immich schema issue don't cause a person to be skipped incorrectly. + # Uses total_raw so that non-dict items from a transient Immich schema + # issue on a mixed page don't shrink the count below the threshold. # Done here (after fetch) rather than upfront because Immich v2.7.5+ # dropped assetCount from the /api/people response. if min_face_count > 0 and total_raw < min_face_count: From 2d39291fe7ea8f5e3cc6942d5f4aef1cdd84ec1e Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 00:51:21 +0000 Subject: [PATCH 02/15] fix: correct reconcile log severity, docstring gaps, and _entry allocation (v0.5.9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - reconcile.py: swap log levels — external-upload path (permanent mapping loss) escalated to WARNING; timeout path (transient, retries next cycle) downgraded to INFO. Also extend the warning message to note the files are permanently unmapped. - immich_api.py: extend fetch_all_assets docstring to document that all-garbage page termination (in addition to network errors) makes total_raw a lower bound. - executor.py: add comment above shutil.rmtree noting that POSIX rmtree raises NotADirectoryError on a top-level symlink, documenting why the removed islink guard is safe to omit. - upload_tracker.py: replace setdefault with explicit guard in _entry() — setdefault evaluates its default-dict argument before checking key presence, allocating and discarding a dict on every already-present call. --- CHANGELOG.md | 12 ++++++++++++ pyproject.toml | 2 +- winnow/executor.py | 2 ++ winnow/immich_api.py | 5 +++-- winnow/reconcile.py | 7 ++++--- winnow/upload_tracker.py | 6 +++--- 6 files changed, 25 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a51214..ff7b7cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.9] - 2026-06-15 + +### Fixed + +- **Reconcile log severity corrected**: the external-upload branch (`len(new_files) > target`) was logged at `INFO` while the timeout branch (`< target`) was logged at `WARNING`. The severity is now inverted to match impact: external upload causes permanent mapping loss (those files are never eligible for quality replacement) and is now `WARNING`; timeout is transient and recoverable next cycle and is now `INFO`. + +- **`fetch_all_assets` docstring: lower-bound caveat now covers both interruption cases**: previously only noted that a network error makes `total_raw` a lower bound. An all-garbage page (every item non-dict) also terminates pagination early, leaving later pages unfetched — this case is now documented alongside the network error case. + +- **`shutil.rmtree` symlink safety documented**: added a comment above the `rmtree` call in `execute_jobs` noting that POSIX `shutil.rmtree` raises `NotADirectoryError` on a top-level symlink, so a race-replaced symlink cannot cause out-of-tree deletion. + +- **`_entry()` in `get_person_summary` no longer allocates default dict for present keys**: `setdefault` evaluates its default argument before checking whether the key exists, allocating and immediately discarding a 5-key dict on every call for an already-present person. Replaced with an explicit `if name not in summary` guard. + ## [0.5.8] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 10d0883..35e851b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.8" +version = "0.5.9" 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 155a2f3..8c09595 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -107,6 +107,8 @@ def execute_jobs(jobs: list[dict]) -> None: logger.error(str(e)) continue # Face crops are transient (uploaded then discarded); wipe before each run. + # shutil.rmtree raises NotADirectoryError on a top-level symlink (POSIX), + # so a race-replaced symlink cannot cause deletion outside output_dir. if os.path.isdir(person_dir): shutil.rmtree(person_dir) os.makedirs(person_dir, exist_ok=True) diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 9309da2..0e1517c 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -89,8 +89,9 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: Returns (assets, total_raw) where assets is the list of valid dict items and total_raw is the raw item count across pages that had at least one valid dict. All-garbage pages (every item non-dict) stop pagination and are not - counted. If pagination is interrupted by a network error, total_raw is a - lower bound (a warning is logged). + counted. total_raw is a lower bound in two cases: a network error interrupts + pagination (a warning is logged), or an all-garbage page terminates it early + (a warning is logged and later pages are not fetched). """ name = person.get("name", "Unknown") person_id = person.get("id") diff --git a/winnow/reconcile.py b/winnow/reconcile.py index d6659d5..53bb43e 100644 --- a/winnow/reconcile.py +++ b/winnow/reconcile.py @@ -76,15 +76,16 @@ def reconcile_frigate_mappings( } record_frigate_files_batch(person_name, mappings) elif len(new_files) > target: - logger.info( + logger.warning( "%s: %s new Frigate files for %s uploads" - " (external upload detected) — skipping file mapping", + " (external upload detected) — skipping file mapping;" + " these files are permanently unmapped", person_name, len(new_files), target, ) else: - logger.warning( + logger.info( "%s: only %s of %s expected Frigate files" " appeared after reconciliation — mapping skipped", person_name, diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 9f39b26..d10b464 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -443,9 +443,9 @@ def get_person_summary() -> dict[str, dict]: ).fetchall() def _entry(summary: dict, name: str) -> dict: - return summary.setdefault( - name, {"uploaded": 0, "rejected": 0, "frigate_count": None, "scores": {}, "frigate_files": {}} - ) + if name not in summary: + summary[name] = {"uploaded": 0, "rejected": 0, "frigate_count": None, "scores": {}, "frigate_files": {}} + return summary[name] summary: dict[str, dict] = {} for r in rows: From 480bf805346cd10ee0af304b202b914dd741b78b Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 00:58:22 +0000 Subject: [PATCH 03/15] fix: reconcile < target severity and rmtree symlink guard (v0.5.10) - reconcile.py: re-escalate the < target branch from INFO to WARNING and add 'permanently unmapped' label. Both post-loop branches produce identical permanent mapping loss; v0.5.9 incorrectly treated the timeout case as recoverable. - executor.py: guard shutil.rmtree with 'not os.path.islink(person_dir)' so a race-replaced symlink-to-directory is skipped rather than raising an unhandled OSError that aborts all remaining jobs. Correct comment: rmtree raises OSError, not NotADirectoryError. --- CHANGELOG.md | 8 ++++++++ pyproject.toml | 2 +- winnow/executor.py | 7 ++++--- winnow/reconcile.py | 5 +++-- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff7b7cd..aa792cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.10] - 2026-06-15 + +### Fixed + +- **Reconcile `< target` branch re-escalated to WARNING**: when fewer Frigate files appear than expected after the full backoff window, the affected files are permanently unmapped — identical in consequence to the `> target` (external upload race) case fixed in v0.5.9. The v0.5.9 demotion to `INFO` was incorrect; both post-loop branches now log at `WARNING` and include the "permanently unmapped" label. + +- **`execute_jobs` symlink guard added before `shutil.rmtree`**: `os.path.isdir` follows symlinks and returns `True` for a symlink pointing at a directory. If a race condition replaces `person_dir` with such a symlink, the old guard would pass and `shutil.rmtree` would raise an unhandled `OSError`, aborting all remaining jobs in the batch. The guard is now `os.path.isdir(person_dir) and not os.path.islink(person_dir)`, so a symlink-to-directory is silently skipped. The comment is also corrected: `shutil.rmtree` raises `OSError` (not `NotADirectoryError`) on a top-level symlink. + ## [0.5.9] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 35e851b..2db5d42 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.9" +version = "0.5.10" 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 8c09595..0954045 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -107,9 +107,10 @@ def execute_jobs(jobs: list[dict]) -> None: logger.error(str(e)) continue # Face crops are transient (uploaded then discarded); wipe before each run. - # shutil.rmtree raises NotADirectoryError on a top-level symlink (POSIX), - # so a race-replaced symlink cannot cause deletion outside output_dir. - if os.path.isdir(person_dir): + # Exclude symlinks explicitly: os.path.isdir follows them and returns True + # for a symlink-to-directory, but shutil.rmtree raises OSError on a + # top-level symlink rather than deleting through it. + if os.path.isdir(person_dir) and not os.path.islink(person_dir): shutil.rmtree(person_dir) os.makedirs(person_dir, exist_ok=True) diff --git a/winnow/reconcile.py b/winnow/reconcile.py index 53bb43e..a4ffcc2 100644 --- a/winnow/reconcile.py +++ b/winnow/reconcile.py @@ -85,9 +85,10 @@ def reconcile_frigate_mappings( target, ) else: - logger.info( + logger.warning( "%s: only %s of %s expected Frigate files" - " appeared after reconciliation — mapping skipped", + " appeared after reconciliation — mapping skipped;" + " these files are permanently unmapped", person_name, len(new_files), target, From 796aded2da2f9bea69521d19328ef6fba6a373a3 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 01:02:28 +0000 Subject: [PATCH 04/15] fix: log+skip on symlink TOCTOU in execute_jobs (v0.5.11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v0.5.10 compound guard 'isdir and not islink' silently skipped the rmtree when person_dir was a symlink-to-directory, then let makedirs follow the symlink — allowing crop writes outside output_dir with no diagnostic. Replace with an explicit islink pre-check that logs an error and continues, matching the ValueError path from _safe_person_dir. --- CHANGELOG.md | 6 ++++++ pyproject.toml | 2 +- winnow/executor.py | 10 ++++++---- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa792cd..3329866 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.11] - 2026-06-15 + +### Fixed + +- **`execute_jobs` symlink TOCTOU gap closed**: the v0.5.10 guard `os.path.isdir(person_dir) and not os.path.islink(person_dir)` silently skipped the wipe when `person_dir` was a symlink-to-directory, then called `os.makedirs` which followed the symlink — allowing crop writes to land outside `output_dir` with no log or skip. The guard is replaced by an explicit pre-check: if `os.path.islink(person_dir)` is True, log an error and `continue`, matching the established `ValueError` pattern from `_safe_person_dir`. The `isdir` / `rmtree` block is restored to its original simple form. + ## [0.5.10] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 2db5d42..b7ffd2a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.10" +version = "0.5.11" 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 0954045..6d58aa3 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -107,10 +107,12 @@ def execute_jobs(jobs: list[dict]) -> None: logger.error(str(e)) continue # Face crops are transient (uploaded then discarded); wipe before each run. - # Exclude symlinks explicitly: os.path.isdir follows them and returns True - # for a symlink-to-directory, but shutil.rmtree raises OSError on a - # top-level symlink rather than deleting through it. - if os.path.isdir(person_dir) and not os.path.islink(person_dir): + # A symlink could appear here via a TOCTOU race after _safe_person_dir + # returned — writing through it would land crops outside output_dir. + if os.path.islink(person_dir): + logger.error("person_dir %s became a symlink after path check — skipping job", person_dir) + continue + if os.path.isdir(person_dir): shutil.rmtree(person_dir) os.makedirs(person_dir, exist_ok=True) From 850ae2f9fcbdd14ddd3977ac7f70aabe7bdc70d0 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 01:05:49 +0000 Subject: [PATCH 05/15] fix: remove progress task on skipped jobs (v0.5.12) progress.add_task() fires unconditionally at the top of the job loop; both continue paths (ValueError from _safe_person_dir and the symlink TOCTOU guard) skipped remove_task(), leaving orphaned 0% rows in the terminal for the rest of the run. --- CHANGELOG.md | 6 ++++++ pyproject.toml | 2 +- winnow/executor.py | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3329866..afef8c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.12] - 2026-06-15 + +### Fixed + +- **Progress task leak on skipped jobs**: `progress.add_task()` is called unconditionally at the top of the job loop, but both early-exit `continue` paths — the `ValueError` skip from `_safe_person_dir` and the symlink-TOCTOU skip added in v0.5.11 — bypassed `progress.remove_task()`, leaving orphaned 0% rows in the terminal display for the rest of the run. Both `continue` paths now call `progress.remove_task(job_task)` before continuing. + ## [0.5.11] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index b7ffd2a..9e6fda2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.11" +version = "0.5.12" 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 6d58aa3..07cabd4 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -105,12 +105,14 @@ def execute_jobs(jobs: list[dict]) -> None: person_dir = _safe_person_dir(Config.OUTPUT_DIR, name) except ValueError as e: logger.error(str(e)) + progress.remove_task(job_task) continue # Face crops are transient (uploaded then discarded); wipe before each run. # A symlink could appear here via a TOCTOU race after _safe_person_dir # returned — writing through it would land crops outside output_dir. if os.path.islink(person_dir): logger.error("person_dir %s became a symlink after path check — skipping job", person_dir) + progress.remove_task(job_task) continue if os.path.isdir(person_dir): shutil.rmtree(person_dir) From 51f7ed3961f10948e11583a58b0bf863bb754971 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 01:19:38 +0000 Subject: [PATCH 06/15] =?UTF-8?q?release:=20v0.5.13=20=E2=80=94=20robustne?= =?UTF-8?q?ss=20fixes=20from=20full-project=20audit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - executor.py: wrap shutil.rmtree/os.makedirs in try/except OSError so a permission failure logs and skips the job rather than aborting the run - cache.py: write embeddings to a .tmp file and atomically rename into place via os.replace so a process kill can't leave a corrupted .npy cache slot - immich_api.py: guard fileCreatedAt with isinstance(str) check before calling .replace() so a non-string timestamp doesn't raise AttributeError and kill the entire filter_recent_assets pass - upload_tracker.py: raise SQLite busy timeout from 5 s to 30 s to handle concurrent cron+manual run overlap without dropping upload-tracking records --- CHANGELOG.md | 12 ++++++++++++ pyproject.toml | 2 +- winnow/cache.py | 9 ++++++++- winnow/executor.py | 11 ++++++++--- winnow/immich_api.py | 2 +- winnow/upload_tracker.py | 2 +- 6 files changed, 31 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afef8c8..af0012c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.13] - 2026-06-15 + +### Fixed + +- **`execute_jobs` output-dir OSError now skips the job instead of aborting the run**: `shutil.rmtree` and `os.makedirs` were not wrapped in any error handler — an `OSError` or `PermissionError` (e.g. read-only filesystem, lingering lock) propagated out of the `for job in jobs` loop, abandoning `job_task` in the Rich progress display and silently dropping all remaining jobs. Both calls are now wrapped in `try/except OSError`; on failure the error is logged, the progress task is removed, and the loop continues to the next job. + +- **Embedding cache writes are now atomic**: `cache.py` previously called `np.save(path, embedding)` directly to the final `.npy` path. A process kill or container stop mid-write left a truncated file that `np.load` would subsequently raise on. Because `get()` catches the exception and returns `None`, the slot appeared empty on every future run — the corrupted file was never cleaned up and the embedding was silently recomputed forever. The write now goes to a `.tmp` sibling and is renamed into place with `os.replace` (atomic on POSIX); the tmp file is removed on any write failure. + +- **`filter_recent_assets` guards against non-string `fileCreatedAt`**: the previous `if not created_at_str` guard passed truthy non-string values (e.g. a Unix-epoch integer returned by some Immich API versions), after which `created_at_str.replace("Z", "+00:00")` raised `AttributeError`. That exception was not caught by the surrounding `except ValueError`, so a single non-string timestamp aborted the entire filtering pass for the person being processed. The guard is now `if not isinstance(created_at_str, str) or not created_at_str`. + +- **SQLite connection timeout raised to 30 s**: `sqlite3.connect` defaulted to a 5-second busy timeout. Under concurrent access (scheduled and manual runs overlapping), 5 s was often insufficient, causing `OperationalError: database is locked` that propagated through `upload_to_frigate` and dropped upload-tracking records — assets would then be re-uploaded on the next run. The timeout is now 30 s, matching the typical upload cycle length. + ## [0.5.12] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 9e6fda2..8941941 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.12" +version = "0.5.13" 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/cache.py b/winnow/cache.py index b34501e..512b854 100644 --- a/winnow/cache.py +++ b/winnow/cache.py @@ -82,10 +82,17 @@ class EmbeddingCache: def put(self, asset_id: str, embedding: np.ndarray, model: str = "insightface") -> None: """Store an embedding in the cache.""" self._ensure_dir() + final = self._path(asset_id, model) + tmp = final + ".tmp" try: - np.save(self._path(asset_id, model), embedding) + np.save(tmp, embedding) + os.replace(tmp, final) except Exception as e: logger.debug("Cache write failed for %s: %s", asset_id, e) + try: + os.remove(tmp) + except OSError: + pass def clear(self) -> None: """Delete all cached embeddings.""" diff --git a/winnow/executor.py b/winnow/executor.py index 07cabd4..50db79c 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -114,9 +114,14 @@ def execute_jobs(jobs: list[dict]) -> None: logger.error("person_dir %s became a symlink after path check — skipping job", person_dir) progress.remove_task(job_task) continue - if os.path.isdir(person_dir): - shutil.rmtree(person_dir) - os.makedirs(person_dir, exist_ok=True) + try: + if os.path.isdir(person_dir): + shutil.rmtree(person_dir) + os.makedirs(person_dir, exist_ok=True) + except OSError as e: + logger.error("Failed to prepare output dir for %s: %s", name, e) + progress.remove_task(job_task) + continue # Track filename → asset_id, filename → confidence score, filename → crop dims asset_map: dict[str, str] = {} diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 0e1517c..5f537d8 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -277,7 +277,7 @@ def filter_recent_assets(assets: list[dict], years: int | None = None) -> list[d recent, skipped = [], 0 for asset in assets: created_at_str = asset.get("fileCreatedAt") - if not created_at_str: + if not isinstance(created_at_str, str) or not created_at_str: continue try: diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index d10b464..be22cd5 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -80,7 +80,7 @@ def _get_conn() -> sqlite3.Connection: if _conn is None: Path(data_dir).mkdir(parents=True, exist_ok=True) - _conn = sqlite3.connect(db_path, check_same_thread=False) + _conn = sqlite3.connect(db_path, check_same_thread=False, timeout=30) _conn.row_factory = sqlite3.Row _conn.execute("PRAGMA journal_mode=WAL") _conn.execute("PRAGMA foreign_keys=ON") From 5bcc5975bce5b85999b5e2a1ef49b9cf1fc3fbe8 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 01:35:27 +0000 Subject: [PATCH 07/15] =?UTF-8?q?release:=20v0.5.14=20=E2=80=94=20graceful?= =?UTF-8?q?=20fallback=20for=20invalid=20numeric=20env=20vars?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit YEARS_FILTER, MIN_FACE_WIDTH, MIN_FACE_COUNT, MAX_AUTO_IMAGES, BLUR_THRESHOLD, MIN_CONFIDENCE, and FACE_MARGIN used bare int()/float() calls with no error handler. A typo (trailing space, non-numeric value) raised ValueError inside __getattr__, producing a cryptic traceback on the first config access rather than at the validate() step. Values are now parsed by _getenv_int/_getenv_float helpers that warn and fall back to the documented default, matching the existing FRIGATE_SCORE_CEILING pattern. --- CHANGELOG.md | 6 ++++++ pyproject.toml | 2 +- winnow/config.py | 32 +++++++++++++++++++++++++------- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af0012c..b8fc3bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.14] - 2026-06-15 + +### Fixed + +- **Invalid env var values for numeric config now warn and use defaults**: `YEARS_FILTER`, `MIN_FACE_WIDTH`, `MIN_FACE_COUNT`, `MAX_AUTO_IMAGES`, `BLUR_THRESHOLD`, `MIN_CONFIDENCE`, and `FACE_MARGIN` all used bare `int()`/`float()` with no error handler. A typo such as `YEARS_FILTER=10 ` (trailing space) or `MIN_FACE_WIDTH=auto` raised `ValueError` from inside `__getattr__`, surfacing as a cryptic traceback on the first config access rather than at the config-validation step where a helpful error is expected. The values are now parsed with module-level `_getenv_int` / `_getenv_float` helpers that log a `WARNING` and fall back to the documented default on parse failure, matching the existing pattern already used for `FRIGATE_SCORE_CEILING`. + ## [0.5.13] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 8941941..e93bfef 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.13" +version = "0.5.14" 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/config.py b/winnow/config.py index dbc1339..3b4860e 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -12,6 +12,24 @@ from rich.prompt import Prompt _LEGACY_CONFIG_FILE = Path(".immich_config.json") # pre-v0.6: lived in process CWD, not on a volume +def _getenv_int(name: str, default: int) -> int: + val = os.getenv(name, str(default)) + try: + return int(val) + except ValueError: + logging.warning("%s=%r is not a valid integer — using default %s", name, val, default) + return default + + +def _getenv_float(name: str, default: float) -> float: + val = os.getenv(name, str(default)) + try: + return float(val) + except ValueError: + logging.warning("%s=%r is not a valid float — using default %s", name, val, default) + return default + + class _Config: """Singleton configuration with lazy loading via __getattr__. @@ -84,13 +102,13 @@ class _Config: self.IMMICH_URL = os.getenv("IMMICH_URL") self.API_KEY = os.getenv("API_KEY") self.OUTPUT_DIR = os.getenv("OUTPUT_DIR", "./frigate_train") - self.YEARS_FILTER = int(os.getenv("YEARS_FILTER", "10")) - self.MIN_FACE_WIDTH = int(os.getenv("MIN_FACE_WIDTH", "90")) - self.MIN_FACE_COUNT = int(os.getenv("MIN_FACE_COUNT", "3")) + self.YEARS_FILTER = _getenv_int("YEARS_FILTER", 10) + self.MIN_FACE_WIDTH = _getenv_int("MIN_FACE_WIDTH", 90) + self.MIN_FACE_COUNT = _getenv_int("MIN_FACE_COUNT", 3) self.MERGE_DUPLICATE_PEOPLE = os.getenv("MERGE_DUPLICATE_PEOPLE", "false").lower() in ("true", "1", "yes") - self.BLUR_THRESHOLD = float(os.getenv("BLUR_THRESHOLD", "120.0")) - self.MIN_CONFIDENCE = float(os.getenv("MIN_CONFIDENCE", "0.7")) - self.MAX_AUTO_IMAGES = int(os.getenv("MAX_AUTO_IMAGES", "20")) + self.BLUR_THRESHOLD = _getenv_float("BLUR_THRESHOLD", 120.0) + self.MIN_CONFIDENCE = _getenv_float("MIN_CONFIDENCE", 0.7) + self.MAX_AUTO_IMAGES = _getenv_int("MAX_AUTO_IMAGES", 20) self.QUALITY_REPLACEMENT = os.getenv("QUALITY_REPLACEMENT", "true").lower() in ("true", "1", "yes") _ceiling_env = os.getenv("FRIGATE_SCORE_CEILING", "").strip() if _ceiling_env: @@ -102,7 +120,7 @@ class _Config: else: self.FRIGATE_SCORE_CEILING = None self.ENABLE_FRIGATE_SCORES = os.getenv("ENABLE_FRIGATE_SCORES", "true").lower() in ("true", "1", "yes") - self.FACE_MARGIN = float(os.getenv("FACE_MARGIN", "0.15")) + self.FACE_MARGIN = _getenv_float("FACE_MARGIN", 0.15) self.USE_FULL_RESOLUTION = os.getenv("USE_FULL_RESOLUTION", "true").lower() in ("true", "1", "yes") self.ENABLE_FACE_ALIGNMENT = os.getenv("ENABLE_FACE_ALIGNMENT", "true").lower() in ("true", "1", "yes") self.ENABLE_CACHE = os.getenv("ENABLE_CACHE", "true").lower() in ("true", "1", "yes") From fe5e1574ac6ba8b5d8c7b75f8afe3712e6e58068 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 01:45:43 +0000 Subject: [PATCH 08/15] =?UTF-8?q?release:=20v0.5.15=20=E2=80=94=20fix=20ca?= =?UTF-8?q?che=20regression=20and=20structural=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - cache.py: fix np.save extension bug from v0.5.13 — tmp path used final+".tmp" (abc.npy.tmp) but np.save auto-appends .npy to paths not ending in .npy, writing to abc.npy.tmp.npy instead; os.replace then raised FileNotFoundError silently, making every cache write a no-op and leaking *.npy.tmp.npy files. Fixed by inserting .tmp before .npy: tmp = final[:-4] + ".tmp.npy" - config.py: remove str(default) round-trip in _getenv_int/_getenv_float — use raw = os.getenv(name); return default if raw is None else int(raw) so a future float default can't cause a spurious "not a valid integer" warning and return the wrong type - executor.py: consolidate 4 progress.remove_task calls into one try/finally around the per-job body; continue inside try/finally executes the finally before the next iteration, making the invariant structurally enforced rather than relying on discipline across 4 sites --- CHANGELOG.md | 10 +++ pyproject.toml | 2 +- winnow/cache.py | 4 +- winnow/config.py | 16 ++-- winnow/executor.py | 204 ++++++++++++++++++++++----------------------- 5 files changed, 125 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8fc3bc..a60628a 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.5.15] - 2026-06-15 + +### Fixed + +- **Embedding cache writes were silently no-ops since v0.5.13**: the atomic-write path used `tmp = final + ".tmp"` where `final` ends in `.npy` (e.g. `abc.npy`), producing a tmp path of `abc.npy.tmp`. `np.save` auto-appends `.npy` to paths not already ending in `.npy`, so it wrote to `abc.npy.tmp.npy` instead. The subsequent `os.replace("abc.npy.tmp", "abc.npy")` then raised `FileNotFoundError` (caught silently at DEBUG), meaning no cache entry was ever committed and leaked `*.npy.tmp.npy` files accumulated on disk. The fix inserts `.tmp` before the `.npy` extension: `tmp = final[:-4] + ".tmp.npy"` so `np.save` sees a path already ending in `.npy` and does not re-append. + +- **`_getenv_int`/`_getenv_float` no longer route the default through `str()` conversion**: the previous form `os.getenv(name, str(default))` converted the default to a string so it could be fed through `int()`/`float()` — an unnecessary round-trip that would cause `_getenv_int("FOO", 4.0)` to log a spurious "not a valid integer" warning and return the float. The helpers now use `raw = os.getenv(name); return default if raw is None else int(raw)`, passing the typed default through directly. + +- **`execute_jobs` progress task now removed via `try/finally`**: `progress.remove_task(job_task)` was duplicated in three early-exit paths (ValueError, symlink TOCTOU, OSError) plus once at normal completion. The entire per-job body is now wrapped in `try/finally: progress.remove_task(job_task)`; the three inner `continue` statements trigger the `finally` automatically before advancing to the next job, making the invariant structurally impossible to violate by a future code path. + ## [0.5.14] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index e93bfef..19db7b5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.14" +version = "0.5.15" 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/cache.py b/winnow/cache.py index 512b854..92c2fc6 100644 --- a/winnow/cache.py +++ b/winnow/cache.py @@ -83,7 +83,9 @@ class EmbeddingCache: """Store an embedding in the cache.""" self._ensure_dir() final = self._path(asset_id, model) - tmp = final + ".tmp" + # Insert .tmp before .npy so np.save doesn't auto-append another .npy extension + # (np.save appends .npy to paths that don't already end in .npy). + tmp = final[:-4] + ".tmp.npy" try: np.save(tmp, embedding) os.replace(tmp, final) diff --git a/winnow/config.py b/winnow/config.py index 3b4860e..86af77c 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -13,20 +13,24 @@ _LEGACY_CONFIG_FILE = Path(".immich_config.json") # pre-v0.6: lived in process def _getenv_int(name: str, default: int) -> int: - val = os.getenv(name, str(default)) + raw = os.getenv(name) + if raw is None: + return default try: - return int(val) + return int(raw) except ValueError: - logging.warning("%s=%r is not a valid integer — using default %s", name, val, default) + logging.warning("%s=%r is not a valid integer — using default %s", name, raw, default) return default def _getenv_float(name: str, default: float) -> float: - val = os.getenv(name, str(default)) + raw = os.getenv(name) + if raw is None: + return default try: - return float(val) + return float(raw) except ValueError: - logging.warning("%s=%r is not a valid float — using default %s", name, val, default) + logging.warning("%s=%r is not a valid float — using default %s", name, raw, default) return default diff --git a/winnow/executor.py b/winnow/executor.py index 50db79c..8c88990 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -102,118 +102,116 @@ def execute_jobs(jobs: list[dict]) -> None: job_task = progress.add_task(f"Processing {name}...", total=len(assets)) try: - person_dir = _safe_person_dir(Config.OUTPUT_DIR, name) - except ValueError as e: - logger.error(str(e)) - progress.remove_task(job_task) - continue - # Face crops are transient (uploaded then discarded); wipe before each run. - # A symlink could appear here via a TOCTOU race after _safe_person_dir - # returned — writing through it would land crops outside output_dir. - if os.path.islink(person_dir): - logger.error("person_dir %s became a symlink after path check — skipping job", person_dir) - progress.remove_task(job_task) - continue - try: - if os.path.isdir(person_dir): - shutil.rmtree(person_dir) - os.makedirs(person_dir, exist_ok=True) - except OSError as e: - logger.error("Failed to prepare output dir for %s: %s", name, e) - progress.remove_task(job_task) - continue - - # Track filename → asset_id, filename → confidence score, filename → crop dims - asset_map: dict[str, str] = {} - score_map: dict[str, float | None] = {} - dims_map: dict[str, tuple[int, int]] = {} - - count = 0 - for asset in assets: try: - # Enrich the asset with face bounding box data from the Immich - # faces API (not included in search/metadata results). - asset = enrich_asset_with_face_data(asset, person) - # Skip download if detection confidence already disqualifies - # the asset — avoids fetching a large image we'll discard. - conf = asset.get("face_confidence") - if conf is not None and conf < Config.MIN_CONFIDENCE: - progress.console.print( - f"[yellow]Skipped {asset['id']}" - f" (detection confidence {conf:.2f} < {Config.MIN_CONFIDENCE})[/yellow]" - ) - mark_rejected(asset["id"], person_name=name) - progress.advance(job_task) - progress.advance(overall_task) - continue + person_dir = _safe_person_dir(Config.OUTPUT_DIR, name) + except ValueError as e: + logger.error(str(e)) + continue + # Face crops are transient (uploaded then discarded); wipe before each run. + # A symlink could appear here via a TOCTOU race after _safe_person_dir + # returned — writing through it would land crops outside output_dir. + if os.path.islink(person_dir): + logger.error("person_dir %s became a symlink after path check — skipping job", person_dir) + continue + try: + if os.path.isdir(person_dir): + shutil.rmtree(person_dir) + os.makedirs(person_dir, exist_ok=True) + except OSError as e: + logger.error("Failed to prepare output dir for %s: %s", name, e) + continue - # Use full-resolution for final output when configured - if use_full_res: - img = fetch_full_image(asset["id"]) - else: - resp = requests.get( - f"{Config.IMMICH_URL}/api/assets/{asset['id']}/thumbnail?size=preview&format=JPEG", - headers=get_headers(), - timeout=30, - ) - if resp.ok: - try: - img = Image.open(BytesIO(resp.content)) - except Exception: - logger.warning("Invalid image data for asset %s", asset["id"]) - img = None - else: - img = None + # Track filename → asset_id, filename → confidence score, filename → crop dims + asset_map: dict[str, str] = {} + score_map: dict[str, float | None] = {} + dims_map: dict[str, tuple[int, int]] = {} - if img is None: - progress.console.print(f"[red]Failed download {asset['id']}[/red]") - else: - saved = process_face_mode( - img, asset, person, person_dir, count, insightface_app=insightface_app - ) - if saved: - filename = f"{count}.jpg" - asset_map[filename] = asset["id"] - score_map[filename] = asset.get("quality_score") - if isinstance(saved, tuple): - dims_map[filename] = saved - # Time-spread path: compute blur score from the downloaded - # image. Cap at 1440px so the scale matches the preview - # thumbnails the embedding path uses for scoring — Laplacian - # variance grows with resolution, making full-res and - # thumbnail scores incomparable if left uncapped. - if score_map[filename] is None: - try: - score_img = img.convert("RGB") if img.mode != "RGB" else img - if score_img.width > 1440 or score_img.height > 1440: - score_img = score_img.copy() - score_img.thumbnail((1440, 1440), Image.LANCZOS) - score_map[filename] = assess_quality(score_img).blur_score - except Exception as exc: - logger.debug("Quality score fallback for %s: %s", asset["id"], exc) - score_map[filename] = 0.0 # unknown quality — treat as lowest - - count += 1 - else: + count = 0 + for asset in assets: + try: + # Enrich the asset with face bounding box data from the Immich + # faces API (not included in search/metadata results). + asset = enrich_asset_with_face_data(asset, person) + # Skip download if detection confidence already disqualifies + # the asset — avoids fetching a large image we'll discard. + conf = asset.get("face_confidence") + if conf is not None and conf < Config.MIN_CONFIDENCE: progress.console.print( - f"[yellow]Skipped {asset['id']} (no usable face data)[/yellow]" + f"[yellow]Skipped {asset['id']}" + f" (detection confidence {conf:.2f} < {Config.MIN_CONFIDENCE})[/yellow]" ) - except Exception as e: - logger.error("Failed to process asset %s: %s", asset["id"], e) + mark_rejected(asset["id"], person_name=name) + progress.advance(job_task) + progress.advance(overall_task) + continue - progress.advance(job_task) - progress.advance(overall_task) + # Use full-resolution for final output when configured + if use_full_res: + img = fetch_full_image(asset["id"]) + else: + resp = requests.get( + f"{Config.IMMICH_URL}/api/assets/{asset['id']}/thumbnail?size=preview&format=JPEG", + headers=get_headers(), + timeout=30, + ) + if resp.ok: + try: + img = Image.open(BytesIO(resp.content)) + except Exception: + logger.warning("Invalid image data for asset %s", asset["id"]) + img = None + else: + img = None - # Store maps on the job so upload_to_frigate can use them - job["asset_map"] = asset_map - job["score_map"] = score_map - job["dims_map"] = dims_map + if img is None: + progress.console.print(f"[red]Failed download {asset['id']}[/red]") + else: + saved = process_face_mode( + img, asset, person, person_dir, count, insightface_app=insightface_app + ) + if saved: + filename = f"{count}.jpg" + asset_map[filename] = asset["id"] + score_map[filename] = asset.get("quality_score") + if isinstance(saved, tuple): + dims_map[filename] = saved + # Time-spread path: compute blur score from the downloaded + # image. Cap at 1440px so the scale matches the preview + # thumbnails the embedding path uses for scoring — Laplacian + # variance grows with resolution, making full-res and + # thumbnail scores incomparable if left uncapped. + if score_map[filename] is None: + try: + score_img = img.convert("RGB") if img.mode != "RGB" else img + if score_img.width > 1440 or score_img.height > 1440: + score_img = score_img.copy() + score_img.thumbnail((1440, 1440), Image.LANCZOS) + score_map[filename] = assess_quality(score_img).blur_score + except Exception as exc: + logger.debug("Quality score fallback for %s: %s", asset["id"], exc) + score_map[filename] = 0.0 # unknown quality — treat as lowest - progress.remove_task(job_task) + count += 1 + else: + progress.console.print( + f"[yellow]Skipped {asset['id']} (no usable face data)[/yellow]" + ) + except Exception as e: + logger.error("Failed to process asset %s: %s", asset["id"], e) - # Log how many images were actually saved vs selected - if count < len(assets): - logger.info("%s: saved %s/%s selected images", name, count, len(assets)) + progress.advance(job_task) + progress.advance(overall_task) + + # Store maps on the job so upload_to_frigate can use them + job["asset_map"] = asset_map + job["score_map"] = score_map + job["dims_map"] = dims_map + + # Log how many images were actually saved vs selected + if count < len(assets): + logger.info("%s: saved %s/%s selected images", name, count, len(assets)) + finally: + progress.remove_task(job_task) def upload_to_frigate(jobs: list[dict]) -> None: From de6804226d0d30b7d61cfbe39e9aeafe677c49dd Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 02:10:00 +0000 Subject: [PATCH 09/15] refactor: unify env var helpers and remove magic slice in cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add _getenv_num as shared core for _getenv_int/_getenv_float - Add _getenv_optional_float for FRIGATE_SCORE_CEILING (replaces 9-line inline block) - Add _getenv_bool; replace 11 inline .lower()-in-("true","1","yes") sites across config.py, jobs.py, and cli.py with single call site - _resolve_strategy no-embedding branch: inline try/except → _getenv_int("LIMIT", 30) - cache.py: final[:-4] → final.removesuffix(".npy") — assumption is now explicit --- CHANGELOG.md | 14 +++++++++++++ pyproject.toml | 2 +- winnow/cache.py | 2 +- winnow/cli.py | 8 ++++---- winnow/config.py | 51 +++++++++++++++++++++++++++--------------------- winnow/jobs.py | 19 +++++------------- 6 files changed, 54 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a60628a..f4e1156 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.16] - 2026-06-15 + +### Changed + +- **`_getenv_int` and `_getenv_float` now share a single `_getenv_num` implementation**: the two helpers were structurally identical (read env var, return typed default if absent, try-cast, warn and return default on `ValueError`) with only the cast differing. Both are now thin wrappers around a private `_getenv_num(name, default, cast)`, eliminating the duplicated warning logic. + +- **`FRIGATE_SCORE_CEILING` now uses `_getenv_optional_float`**: the previous 9-line inline block (`os.getenv("FRIGATE_SCORE_CEILING", "").strip()` + try/except) has been replaced with a new `_getenv_optional_float(name) -> float | None` helper that encapsulates the "empty-string means None, parse-error means None" semantics, making it consistent with the other numeric env var helpers. + +- **Boolean env vars now use `_getenv_bool`**: the `.lower() in ("true", "1", "yes")` pattern was repeated across 11 sites in `config.py`, `jobs.py`, and `cli.py`. A new `_getenv_bool(name, default)` helper centralises the canonical truthy-string set; all sites have been updated to call it. + +- **`_resolve_strategy` no-embedding branch uses `_getenv_int`**: the inline `int(custom_limit)` try/except block in `jobs.py` for the time-spread path has been replaced with `_getenv_int("LIMIT", 30)`, matching the pattern used in `config.py`. The smart-mode path retains its own try/except because its fallback is to the strategy map rather than to a numeric default. + +- **`cache.py` tmp path uses `str.removesuffix`**: `final[:-4] + ".tmp.npy"` replaced with `final.removesuffix(".npy") + ".tmp.npy"` — the assumption that the cache path ends in `.npy` is now explicit and self-documenting rather than expressed as a magic numeric slice. + ## [0.5.15] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 19db7b5..f5ed11f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.15" +version = "0.5.16" 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/cache.py b/winnow/cache.py index 92c2fc6..b3ebf7d 100644 --- a/winnow/cache.py +++ b/winnow/cache.py @@ -85,7 +85,7 @@ class EmbeddingCache: final = self._path(asset_id, model) # Insert .tmp before .npy so np.save doesn't auto-append another .npy extension # (np.save appends .npy to paths that don't already end in .npy). - tmp = final[:-4] + ".tmp.npy" + tmp = final.removesuffix(".npy") + ".tmp.npy" try: np.save(tmp, embedding) os.replace(tmp, final) diff --git a/winnow/cli.py b/winnow/cli.py index 0b89d77..10f627b 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -7,7 +7,7 @@ import sys from rich import print as rprint from rich.prompt import Confirm -from .config import Config +from .config import Config, _getenv_bool from .executor import execute_jobs, upload_to_frigate from .immich_api import get_immich_version, get_people, merge_people from .jobs import _show_preview, auto_configure, interactive_configure @@ -145,7 +145,7 @@ _UNSUPPORTED_VARS = [ def main() -> None: """Entry point for winnow CLI.""" try: - verbose = os.environ.get("VERBOSE", "").lower() in ("true", "1", "yes") + verbose = _getenv_bool("VERBOSE", False) setup_logging(verbose=verbose) trace_size = os.environ.get("TRACE_CROP_SIZE", "").strip() @@ -232,8 +232,8 @@ def main() -> None: # Auto mode when no TTY (Docker, cron, pipes) — the primary use case. # A TTY means local interactive use; AUTO_MODE=true overrides that for scripting. - auto_mode = not sys.stdin.isatty() or os.environ.get("AUTO_MODE", "").lower() in ("true", "1", "yes") - dry_run = os.environ.get("DRY_RUN", "false").lower() in ("true", "1", "yes") + auto_mode = not sys.stdin.isatty() or _getenv_bool("AUTO_MODE", False) + dry_run = _getenv_bool("DRY_RUN", False) if dry_run: rprint("[bold yellow]DRY RUN — no images will be downloaded or uploaded[/bold yellow]") diff --git a/winnow/config.py b/winnow/config.py index 86af77c..1606026 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -12,26 +12,41 @@ from rich.prompt import Prompt _LEGACY_CONFIG_FILE = Path(".immich_config.json") # pre-v0.6: lived in process CWD, not on a volume -def _getenv_int(name: str, default: int) -> int: +def _getenv_num(name: str, default, cast): raw = os.getenv(name) if raw is None: return default try: - return int(raw) + return cast(raw) except ValueError: - logging.warning("%s=%r is not a valid integer — using default %s", name, raw, default) + logging.warning("%s=%r is not a valid %s — using default %s", name, raw, cast.__name__, default) return default +def _getenv_int(name: str, default: int) -> int: + return _getenv_num(name, default, int) + + def _getenv_float(name: str, default: float) -> float: - raw = os.getenv(name) - if raw is None: - return default + return _getenv_num(name, default, 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 — using default %s", name, raw, default) + logging.warning("%s=%r is not a valid float — ignoring", name, raw) + return None + + +def _getenv_bool(name: str, default: bool) -> bool: + raw = os.getenv(name) + if raw is None: return default + return raw.lower() in ("true", "1", "yes") class _Config: @@ -109,25 +124,17 @@ class _Config: self.YEARS_FILTER = _getenv_int("YEARS_FILTER", 10) self.MIN_FACE_WIDTH = _getenv_int("MIN_FACE_WIDTH", 90) self.MIN_FACE_COUNT = _getenv_int("MIN_FACE_COUNT", 3) - self.MERGE_DUPLICATE_PEOPLE = os.getenv("MERGE_DUPLICATE_PEOPLE", "false").lower() in ("true", "1", "yes") + self.MERGE_DUPLICATE_PEOPLE = _getenv_bool("MERGE_DUPLICATE_PEOPLE", False) self.BLUR_THRESHOLD = _getenv_float("BLUR_THRESHOLD", 120.0) self.MIN_CONFIDENCE = _getenv_float("MIN_CONFIDENCE", 0.7) self.MAX_AUTO_IMAGES = _getenv_int("MAX_AUTO_IMAGES", 20) - self.QUALITY_REPLACEMENT = os.getenv("QUALITY_REPLACEMENT", "true").lower() in ("true", "1", "yes") - _ceiling_env = os.getenv("FRIGATE_SCORE_CEILING", "").strip() - if _ceiling_env: - try: - self.FRIGATE_SCORE_CEILING = float(_ceiling_env) - except ValueError: - logging.warning("FRIGATE_SCORE_CEILING=%r is not a valid float — ignoring", _ceiling_env) - self.FRIGATE_SCORE_CEILING = None - else: - self.FRIGATE_SCORE_CEILING = None - self.ENABLE_FRIGATE_SCORES = os.getenv("ENABLE_FRIGATE_SCORES", "true").lower() in ("true", "1", "yes") + self.QUALITY_REPLACEMENT = _getenv_bool("QUALITY_REPLACEMENT", True) + self.FRIGATE_SCORE_CEILING = _getenv_optional_float("FRIGATE_SCORE_CEILING") + self.ENABLE_FRIGATE_SCORES = _getenv_bool("ENABLE_FRIGATE_SCORES", True) self.FACE_MARGIN = _getenv_float("FACE_MARGIN", 0.15) - self.USE_FULL_RESOLUTION = os.getenv("USE_FULL_RESOLUTION", "true").lower() in ("true", "1", "yes") - self.ENABLE_FACE_ALIGNMENT = os.getenv("ENABLE_FACE_ALIGNMENT", "true").lower() in ("true", "1", "yes") - self.ENABLE_CACHE = os.getenv("ENABLE_CACHE", "true").lower() in ("true", "1", "yes") + self.USE_FULL_RESOLUTION = _getenv_bool("USE_FULL_RESOLUTION", True) + self.ENABLE_FACE_ALIGNMENT = _getenv_bool("ENABLE_FACE_ALIGNMENT", True) + self.ENABLE_CACHE = _getenv_bool("ENABLE_CACHE", True) _data_dir = os.getenv("DATA_DIR") _cache_dir_legacy = os.getenv("CACHE_DIR") if _data_dir: diff --git a/winnow/jobs.py b/winnow/jobs.py index 4fbf2e0..e530c0f 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -8,7 +8,7 @@ from rich.progress import BarColumn, Progress, SpinnerColumn, TaskProgressColumn from rich.prompt import Confirm, IntPrompt, Prompt from rich.table import Table -from .config import Config +from .config import Config, _getenv_bool, _getenv_int from .diversity import select_diverse_assets from .embeddings import is_embedding_available, load_embedding_model from .frigate_api import get_frigate_face_counts @@ -65,19 +65,10 @@ def _get_strategy_choice(has_embedding: bool) -> tuple[int | str, str]: def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, str]: """Resolve env var strategy to (limit, selection_mode) without prompts.""" - custom_limit = os.environ.get("LIMIT", "").strip() - if not has_embedding: - if custom_limit: - try: - limit = int(custom_limit) - except ValueError: - logger.warning("LIMIT=%r is not a valid integer — using default 30", custom_limit) - limit = 30 - else: - limit = 30 - return limit, "time" + return _getenv_int("LIMIT", 30), "time" + custom_limit = os.environ.get("LIMIT", "").strip() if custom_limit: try: return int(custom_limit), "smart" @@ -168,7 +159,7 @@ def _configure_person(person: dict, people: list[dict]) -> dict | None: rprint(f" Found [bold]{total_raw}[/bold] total, [bold]{len(recent_assets)}[/bold] in range ({years} years).") # Ask before strategy so the post-dedup count can inform the choice - retry_env = os.environ.get("RETRY_REJECTED", "false").lower() in ("true", "1", "yes") + retry_env = _getenv_bool("RETRY_REJECTED", False) retry_rejected = Confirm.ask("Include previously rejected images?", default=retry_env) before_dedup = len(recent_assets) @@ -317,7 +308,7 @@ def auto_configure(people: list[dict]) -> list[dict]: if selection_mode == "skip": continue - retry_rejected = os.environ.get("RETRY_REJECTED", "false").lower() in ("true", "1", "yes") + retry_rejected = _getenv_bool("RETRY_REJECTED", False) before_dedup = len(recent_assets) new_asset_ids = set(filter_already_uploaded([a["id"] for a in recent_assets], retry_rejected=retry_rejected)) recent_assets = [a for a in recent_assets if a["id"] in new_asset_ids] From 06c2c4a5842c41fa544b06bdbcfb880026b206a5 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 02:20:34 +0000 Subject: [PATCH 10/15] fix: strip empty env vars in _getenv_num/_getenv_bool; unify FORCE_CPU - _getenv_num: add raw.strip() + empty-string guard so numeric vars set to "" (common Compose pattern for "use default") return the default silently instead of warning "not a valid int/float" - _getenv_bool: same guard so True-defaulted flags set to "" return the configured default instead of silently returning False - embeddings.py: replace inline FORCE_CPU bool parse with _getenv_bool --- CHANGELOG.md | 8 ++++++++ pyproject.toml | 2 +- winnow/config.py | 6 ++++++ winnow/embeddings.py | 3 ++- 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4e1156..324283b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.17] - 2026-06-15 + +### Fixed + +- **`_getenv_num` and `_getenv_bool` now treat an explicitly-empty env var as unset**: previously, `YEARS_FILTER=` (blank) in a `.env` or Compose file caused `int("")` to raise `ValueError`, logging a spurious "not a valid int" warning and returning the default. Both helpers now strip whitespace and treat an empty string the same as an absent variable, returning the typed default silently. This affects all numeric config vars (`YEARS_FILTER`, `MIN_FACE_WIDTH`, `MIN_FACE_COUNT`, `MAX_AUTO_IMAGES`, `BLUR_THRESHOLD`, `MIN_CONFIDENCE`, `FACE_MARGIN`) and all boolean config vars. `_getenv_optional_float` already handled this correctly. + +- **`FORCE_CPU` now uses `_getenv_bool`**: `embeddings.py` retained the old inline `os.getenv("FORCE_CPU", "").lower() in ("true", "1", "yes")` pattern after v0.5.16 introduced `_getenv_bool`. The inline copy is now replaced so the canonical truthy-string set is defined in one place. + ## [0.5.16] - 2026-06-15 ### Changed diff --git a/pyproject.toml b/pyproject.toml index f5ed11f..ba8d203 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.16" +version = "0.5.17" 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/config.py b/winnow/config.py index 1606026..b516325 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -16,6 +16,9 @@ def _getenv_num(name: str, default, cast): raw = os.getenv(name) if raw is None: return default + raw = raw.strip() + if not raw: + return default try: return cast(raw) except ValueError: @@ -46,6 +49,9 @@ def _getenv_bool(name: str, default: bool) -> bool: raw = os.getenv(name) if raw is None: return default + raw = raw.strip() + if not raw: + return default return raw.lower() in ("true", "1", "yes") diff --git a/winnow/embeddings.py b/winnow/embeddings.py index 09bae77..5f8f7b2 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -18,6 +18,7 @@ import numpy as np from PIL import Image from .cache import get_cache +from .config import _getenv_bool logger = logging.getLogger(__name__) @@ -50,7 +51,7 @@ _insightface_loaded = False def _is_force_cpu() -> bool: """Check if CPU mode is forced via environment variable.""" - return os.getenv("FORCE_CPU", "").lower() in ("true", "1", "yes") + return _getenv_bool("FORCE_CPU", False) def _preload_cuda_libs() -> None: From 3c4479a1106f6419e86acaf72583cf3ec42f0dc2 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 02:29:30 +0000 Subject: [PATCH 11/15] fix: replace inline FORCE_CPU check in benchmark.py with _getenv_bool scripts/benchmark.py retained the old os.getenv inline pattern after _getenv_bool was introduced in v0.5.16. Now uses a deferred local import of _getenv_bool, consistent with the script's pattern of keeping all winnow imports inside function bodies rather than at the top level. --- CHANGELOG.md | 6 ++++++ pyproject.toml | 2 +- scripts/benchmark.py | 6 ++---- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 324283b..f4540e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.18] - 2026-06-15 + +### Fixed + +- **`scripts/benchmark.py` now uses `_getenv_bool` for `FORCE_CPU`**: the script retained an inline `os.getenv("FORCE_CPU", "").lower() in ("true", "1", "yes")` pattern in `_mode_label()` after `_getenv_bool` was introduced. Replaced with a local import of `_getenv_bool` consistent with how all other winnow imports in the script are deferred into function bodies. + ## [0.5.17] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index ba8d203..712b2cf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.17" +version = "0.5.18" 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/scripts/benchmark.py b/scripts/benchmark.py index 6e5b992..d26c1c1 100644 --- a/scripts/benchmark.py +++ b/scripts/benchmark.py @@ -13,7 +13,6 @@ Usage inside container: docker exec -e FORCE_CPU=true winnow python /app/scripts/benchmark.py """ -import os import sys import time @@ -22,9 +21,8 @@ from PIL import Image, ImageDraw def _mode_label() -> str: - if os.getenv("FORCE_CPU", "").lower() in ("true", "1", "yes"): - return "CPU (FORCE_CPU=true)" - return "GPU (auto)" + from winnow.config import _getenv_bool + return "CPU (FORCE_CPU=true)" if _getenv_bool("FORCE_CPU", False) else "GPU (auto)" def make_face_image(size: int = 640) -> Image.Image: From 728b84dc8ca46fc04b507e01d9872f1879f4fc34 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 02:56:51 +0000 Subject: [PATCH 12/15] fix: address 10 full-codebase audit findings (v0.5.19) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness: - fetch_face_data: only fall back to faces[0] when person_id is absent; previously a missing person match injected a different person's bbox - upload_tracker: change PK from (asset_id, status) to (asset_id, person_name, status); old PK allowed INSERT OR REPLACE to silently overwrite person_name when the same photo appeared in two people's jobs, breaking quality-replacement JOINs; auto-migrates DBs - filter_recent_assets: treat years=0 as "no age filter" instead of falling through to Config.YEARS_FILTER via falsy `or` - _is_module_available: return find_spec(...) is not None; find_spec returns None (not raises) for absent top-level modules, so the previous code always returned True - execute_jobs error handler: use asset.get("id", "") to avoid a secondary KeyError propagating out of execute_jobs on malformed dicts - upload_to_frigate: also mark_rejected on HTTP 422, not only HTTP 400 with "face" in body; other permanent errors left assets untracked and retried forever - reconcile_frigate_mappings: sort key lambda f: (_ts(f), f) makes order deterministic when timestamps are equal or 0.0; set iteration order is hash-randomised, stable sort preserves it Reuse / cleanup: - config.py: add _getenv_optional_int delegating to _getenv_num(name, None, int) - jobs.py: _resolve_strategy uses _getenv_optional_int("LIMIT") instead of inline os.environ.get + int() + warning duplicate of _getenv_num - frigate_api.py: add _get_frigate_url() helper; eliminates 4× copy of os.environ.get("FRIGATE_URL", "").rstrip("/") - quality.py: extract blur_score_from_image(img, max_dim=1440) helper; executor.py time-spread blur fallback now uses it instead of inlining the resize+RGB+assess_quality sequence, keeping scale logic in one place --- CHANGELOG.md | 26 +++++++++++++++++++++++++ pyproject.toml | 2 +- winnow/config.py | 5 +++++ winnow/embeddings.py | 3 +-- winnow/executor.py | 28 ++++++++++++--------------- winnow/frigate_api.py | 13 +++++++++---- winnow/immich_api.py | 11 +++++++---- winnow/jobs.py | 12 ++++-------- winnow/quality.py | 20 ++++++++++++++++++++ winnow/reconcile.py | 2 +- winnow/upload_tracker.py | 41 +++++++++++++++++++++++++++++++++++++++- 11 files changed, 126 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4540e2..670e6b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.19] - 2026-06-15 + +### Fixed + +- **`fetch_face_data` no longer falls back to an arbitrary person's face**: when `person_id` is provided but not found in the Immich `/api/faces` response, the function now returns `None` instead of falling back to `faces[0]`. Previously a Frigate group photo where the target person's face entry was missing would inject a different person's bounding box, causing the wrong face crop to be uploaded as training data. + +- **`tracked_assets` PRIMARY KEY now includes `person_name`**: the old `PRIMARY KEY (asset_id, status)` meant that `INSERT OR REPLACE` for person B on an asset already tracked for person A silently overwrote `person_name`, destroying the JOIN between `tracked_assets` and `frigate_files` for person A and breaking quality replacement. The key is now `(asset_id, person_name, status)`, giving each person their own row per asset. Existing databases are migrated automatically on first open. + +- **`filter_recent_assets` treats `years=0` as "no age filter"**: previously `years = years or Config.YEARS_FILTER` evaluated `0` as falsy and fell through to the default (10 years), silently discarding all older assets when the user explicitly set `YEARS_FILTER=0`. The check is now `if years is None: years = Config.YEARS_FILTER` followed by an early return for `years=0`. + +- **`_is_module_available` now correctly returns False for absent modules**: `importlib.util.find_spec` returns `None` (not raises) for missing top-level modules, so the previous `try: find_spec(); return True` always reported modules as installed. Fixed to `return find_spec(...) is not None`, ensuring `is_embedding_available()` returns False when InsightFace or onnxruntime are not installed. + +- **Error handler in `execute_jobs` uses `asset.get("id")` instead of `asset["id"]`**: a malformed asset dict missing the `"id"` key would cause a secondary `KeyError` inside the `except` block, propagating uncaught out of `execute_jobs()` and aborting the run mid-job. Changed to `asset.get("id", "")`. + +- **HTTP 422 now triggers `mark_rejected`**: only `HTTP 400` with `"face"` in the body triggered permanent rejection; `HTTP 422` (Unprocessable Entity) left the asset untracked and caused it to be re-selected and re-attempted on every future run. Both codes are now treated as permanent rejections. + +- **Frigate filename reconciliation sort is now deterministic**: `sorted(new_files, key=_ts)` sorted a `set` — when `_ts()` returns `0.0` for non-matching filenames, Python's stable sort preserves the set's hash-randomised input order, producing non-deterministic `asset_id → frigate_filename` mappings. Changed the key to `lambda f: (_ts(f), f)` so equal-timestamp files sort alphabetically. + +### Changed + +- **`_resolve_strategy` uses `_getenv_optional_int("LIMIT")`**: the inline `os.environ.get("LIMIT", "").strip()` + `int()` + `logger.warning` block in `jobs.py` re-implemented the logic already in `_getenv_num`. A new `_getenv_optional_int` helper (delegating to `_getenv_num(name, None, int)`) replaces the duplicate, consolidating LIMIT parse warnings with the rest of the env-var helpers. + +- **`frigate_api.py` uses a shared `_get_frigate_url()` accessor**: `os.environ.get("FRIGATE_URL", "").rstrip("/")` was copy-pasted into all four public functions. A private helper eliminates the duplication so URL normalization is defined once. + +- **`blur_score_from_image()` extracted to `quality.py`**: the time-spread blur-score fallback in `execute_jobs` (resize to 1440px, RGB convert, `assess_quality`) is now a shared `blur_score_from_image(img, max_dim=1440)` helper. Both the executor and any future callers use the same cap and error handling so the score scale can't silently diverge between code paths. + ## [0.5.18] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 712b2cf..0e2488b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.18" +version = "0.5.19" 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/config.py b/winnow/config.py index b516325..d164d83 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -45,6 +45,11 @@ def _getenv_optional_float(name: str) -> float | None: return None +def _getenv_optional_int(name: str) -> int | None: + """Return int value of env var, or None if unset/empty. Warns and returns None on invalid.""" + return _getenv_num(name, None, int) + + def _getenv_bool(name: str, default: bool) -> bool: raw = os.getenv(name) if raw is None: diff --git a/winnow/embeddings.py b/winnow/embeddings.py index 5f8f7b2..cab4294 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -235,8 +235,7 @@ def get_embedding( def _is_module_available(module_name: str) -> bool: """Check if a Python module is importable without importing it fully.""" try: - importlib.util.find_spec(module_name) - return True + return importlib.util.find_spec(module_name) is not None except (ModuleNotFoundError, ValueError): return False diff --git a/winnow/executor.py b/winnow/executor.py index 8c88990..3059122 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -22,7 +22,7 @@ from .frigate_api import ( from .image_processing import process_face_mode from .immich_api import fetch_full_image from .log_config import console -from .quality import assess_quality +from .quality import blur_score_from_image from .reconcile import enrich_asset_with_face_data, reconcile_frigate_mappings from .upload_tracker import ( get_lowest_quality_mapped_file, @@ -176,20 +176,12 @@ def execute_jobs(jobs: list[dict]) -> None: if isinstance(saved, tuple): dims_map[filename] = saved # Time-spread path: compute blur score from the downloaded - # image. Cap at 1440px so the scale matches the preview - # thumbnails the embedding path uses for scoring — Laplacian - # variance grows with resolution, making full-res and - # thumbnail scores incomparable if left uncapped. + # image. Capped at 1440px via blur_score_from_image() so the + # scale matches the preview thumbnails the embedding path uses + # — Laplacian variance grows with resolution, making full-res + # and thumbnail scores incomparable if left uncapped. if score_map[filename] is None: - try: - score_img = img.convert("RGB") if img.mode != "RGB" else img - if score_img.width > 1440 or score_img.height > 1440: - score_img = score_img.copy() - score_img.thumbnail((1440, 1440), Image.LANCZOS) - score_map[filename] = assess_quality(score_img).blur_score - except Exception as exc: - logger.debug("Quality score fallback for %s: %s", asset["id"], exc) - score_map[filename] = 0.0 # unknown quality — treat as lowest + score_map[filename] = blur_score_from_image(img) count += 1 else: @@ -197,7 +189,7 @@ def execute_jobs(jobs: list[dict]) -> None: f"[yellow]Skipped {asset['id']} (no usable face data)[/yellow]" ) except Exception as e: - logger.error("Failed to process asset %s: %s", asset["id"], e) + logger.error("Failed to process asset %s: %s", asset.get("id", ""), e) progress.advance(job_task) progress.advance(overall_task) @@ -534,7 +526,11 @@ def upload_to_frigate(jobs: list[dict]) -> None: progress.console.print(f" [dim]{error_detail}[/dim]") else: logger.debug("%s HTTP %s: %s", fname, resp.status_code, error_detail) - if resp.status_code == 400 and "face" in full_body.lower(): + _is_permanent = ( + (resp.status_code == 400 and "face" in full_body.lower()) + or resp.status_code == 422 + ) + if _is_permanent: asset_id = asset_map.get(fname) if asset_id: mark_rejected(asset_id, person_name=name) diff --git a/winnow/frigate_api.py b/winnow/frigate_api.py index 32c4fa6..4f5cafc 100644 --- a/winnow/frigate_api.py +++ b/winnow/frigate_api.py @@ -8,13 +8,18 @@ import requests 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("/") + + def get_frigate_version() -> str | None: """Fetch Frigate's version string from GET /api/version. Returns the version string (e.g. "0.16.0-beta4") or None if FRIGATE_URL is unset, the endpoint is unreachable, or the response is not parseable. """ - frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/") + frigate_url = _get_frigate_url() if not frigate_url: return None try: @@ -28,7 +33,7 @@ def get_frigate_version() -> str | None: def _get_faces_data() -> dict | None: """Fetch raw GET /api/faces response. Returns None if unavailable.""" - frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/") + frigate_url = _get_frigate_url() if not frigate_url: return None try: @@ -113,7 +118,7 @@ def recognize_face(file_path: str) -> tuple[str | None, float] | None: replace mean-comparison with nearest-neighbour distance across individual training embeddings for accurate coverage detection. """ - frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/") + frigate_url = _get_frigate_url() if not frigate_url: return None try: @@ -140,7 +145,7 @@ def delete_frigate_person_files(person_name: str, filenames: list[str]) -> bool: Uses POST /api/faces/{name}/delete with body {"ids": [filename, ...]}. Returns True on success, False if unreachable or the request fails. """ - frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/") + frigate_url = _get_frigate_url() if not frigate_url or not filenames: return False from urllib.parse import quote diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 5f537d8..3bde08b 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -196,14 +196,14 @@ def fetch_face_data(asset_id: str, person_id: str | None = None) -> FaceData | N if not isinstance(faces, list) or not faces: return None - # Match the target person if specified + # Match the target person if specified; never fall back to a different person's face. face = None if person_id: face = next( (f for f in faces if isinstance(f, dict) and (f.get("person") or {}).get("id") == person_id), None, ) - if face is None: + else: face = faces[0] if isinstance(faces[0], dict) else None if face is None: return None @@ -268,8 +268,11 @@ def fetch_full_image(asset_id: str, timeout: int = 60) -> Image.Image | None: def filter_recent_assets(assets: list[dict], years: int | None = None) -> list[dict]: - """Filter assets to keep only those from the last N years.""" - years = years or Config.YEARS_FILTER + """Filter assets to keep only those from the last N years. Pass years=0 to include all.""" + if years is None: + years = Config.YEARS_FILTER + if not years: + return list(assets) cutoff = datetime.now(timezone.utc) - timedelta(days=365 * years) logger.debug("Filtering assets older than %s years (%s)", years, cutoff) diff --git a/winnow/jobs.py b/winnow/jobs.py index e530c0f..8a87551 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -8,7 +8,7 @@ from rich.progress import BarColumn, Progress, SpinnerColumn, TaskProgressColumn from rich.prompt import Confirm, IntPrompt, Prompt from rich.table import Table -from .config import Config, _getenv_bool, _getenv_int +from .config import Config, _getenv_bool, _getenv_int, _getenv_optional_int from .diversity import select_diverse_assets from .embeddings import is_embedding_available, load_embedding_model from .frigate_api import get_frigate_face_counts @@ -68,13 +68,9 @@ def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, st if not has_embedding: return _getenv_int("LIMIT", 30), "time" - custom_limit = os.environ.get("LIMIT", "").strip() - if custom_limit: - try: - return int(custom_limit), "smart" - except ValueError: - logger.warning("LIMIT=%r is not a valid integer — using adaptive strategy", custom_limit) - # fall through to strategy_map + custom_limit = _getenv_optional_int("LIMIT") + if custom_limit is not None: + return custom_limit, "smart" strategy_map = { "adaptive": ("auto", "smart"), diff --git a/winnow/quality.py b/winnow/quality.py index 8db3ac7..b4e1724 100644 --- a/winnow/quality.py +++ b/winnow/quality.py @@ -138,3 +138,23 @@ def assess_quality( return QualityResult(passed=len(reasons) == 0, reasons=reasons, blur_score=blur_score) + +def blur_score_from_image(img: Image.Image, max_dim: int = 1440) -> float: + """Compute Laplacian-variance blur score, capped at max_dim px to normalise scale. + + Caps resolution so full-res and thumbnail scores are comparable — Laplacian + variance grows with pixel count, making uncapped full-res scores much larger + than thumbnail scores for the same perceived sharpness. + + Returns 0.0 on any error so callers can treat the result as lowest quality. + """ + try: + score_img = img.convert("RGB") if img.mode != "RGB" else img + if score_img.width > max_dim or score_img.height > max_dim: + score_img = score_img.copy() + score_img.thumbnail((max_dim, max_dim), Image.LANCZOS) + return float(assess_quality(score_img).blur_score) + except Exception as exc: + logger.debug("blur_score_from_image failed: %s", exc) + return 0.0 + diff --git a/winnow/reconcile.py b/winnow/reconcile.py index a4ffcc2..652097c 100644 --- a/winnow/reconcile.py +++ b/winnow/reconcile.py @@ -71,7 +71,7 @@ def reconcile_frigate_mappings( ) mappings = { frigate_file: asset_id - for (_, asset_id), frigate_file in zip(uploaded, sorted(new_files, key=_ts)) + for (_, asset_id), frigate_file in zip(uploaded, sorted(new_files, key=lambda f: (_ts(f), f))) if asset_id } record_frigate_files_batch(person_name, mappings) diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index be22cd5..655535b 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -38,7 +38,7 @@ CREATE TABLE IF NOT EXISTS tracked_assets ( crop_width INTEGER, crop_height INTEGER, frigate_score REAL, - PRIMARY KEY (asset_id, status) + PRIMARY KEY (asset_id, person_name, status) ); CREATE TABLE IF NOT EXISTS frigate_files ( @@ -58,6 +58,44 @@ _conn: sqlite3.Connection | None = None _conn_path: str | None = None +def _migrate_schema_v2(conn: sqlite3.Connection) -> None: + """Migrate tracked_assets from PRIMARY KEY (asset_id, status) to (asset_id, person_name, status). + + The old PK meant INSERT OR REPLACE for person B on an asset already tracked for + person A would silently overwrite person_name, breaking quality-replacement JOINs + for person A. The new PK gives each (asset, person) pair its own row. + + SQLite does not support ALTER TABLE to change a primary key; we recreate the table. + """ + pk_cols = { + r[1] + for r in conn.execute("PRAGMA table_info(tracked_assets)").fetchall() + if r[5] > 0 # column index 5 = pk position (0 = not in PK) + } + if "person_name" in pk_cols: + 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; + """) + logger.info("tracked_assets schema migration complete") + + def _get_conn() -> sqlite3.Connection: """Return (or create) the module-level SQLite connection. @@ -87,6 +125,7 @@ def _get_conn() -> sqlite3.Connection: _conn.executescript(_DDL) _conn.commit() _conn_path = db_path + _migrate_schema_v2(_conn) _maybe_migrate(data_dir, _conn) return _conn From 95fb39ed502574afa12cd0696b54d3a3b470a326 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 03:26:51 +0000 Subject: [PATCH 13/15] =?UTF-8?q?fix:=20v0.5.20=20=E2=80=94=20migration=20?= =?UTF-8?q?safety,=20URL=20normalization,=20image=20rejection,=20atomic=20?= =?UTF-8?q?writes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - upload_tracker: replace executescript() in _migrate_schema_v2 with individual execute() calls inside a transaction so a crash between DROP and RENAME rolls back instead of permanently destroying tracked_assets - frigate_api: _get_frigate_url now strips leading/trailing whitespace before rstrip('/') so whitespace-only FRIGATE_URL is treated as unset - executor: upload_to_frigate now uses _get_frigate_url() eliminating double-slash upload paths when FRIGATE_URL has a trailing slash - executor: corrupt thumbnail (resp.ok=True, Image.open fails) now calls mark_rejected() so permanently broken assets are not retried forever - upload_tracker: reset_person now uses _get_frigate_url() instead of inline os.environ.get('FRIGATE_URL', '').strip() - image_processing: _save_jpeg writes to a .tmp file and calls os.replace() so a disk-full error never leaves a truncated JPEG - cli: _handle_duplicate_people falls back to local deduplication when all Immich merges fail, preventing two jobs from overwriting the same Frigate folder - config: _getenv_optional_float now delegates to _getenv_num() like _getenv_optional_int, eliminating the inconsistent duplicate - reconcile: _ts() uses rsplit('.', 1)[0] instead of .replace('.webp','') so FIFO mapping works with any Frigate training-file extension --- CHANGELOG.md | 22 +++++++++++++++++++ pyproject.toml | 2 +- winnow/cli.py | 13 ++++++++++- winnow/config.py | 10 ++------- winnow/executor.py | 9 ++++++-- winnow/frigate_api.py | 4 ++-- winnow/image_processing.py | 11 +++++++++- winnow/reconcile.py | 2 +- winnow/upload_tracker.py | 44 ++++++++++++++++++++++---------------- 9 files changed, 82 insertions(+), 35 deletions(-) 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) From 0602c4ac04e22a19f7631279d2a3401181a23676 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 03:56:57 +0000 Subject: [PATCH 14/15] =?UTF-8?q?fix:=20v0.5.21=20=E2=80=94=20diversity=20?= =?UTF-8?q?cap,=20fetch=20rejection,=20tracker=20isolation,=20merge=20dedu?= =?UTF-8?q?p,=20env=20parsing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - diversity: cap k-medoids seed count at target so _cluster_aware_selection never returns more images than requested (violated MAX_AUTO_IMAGES when remaining capacity was 1-4 slots); add early return for limit=0 to prevent k-medoids from running with a zero budget; slice return to target as a final guard - executor: mark_rejected() when fetch_full_image returns None so assets that can't be fetched (both original and preview) aren't retried every run - executor: wrap mark_uploaded() in its own try/except so a SQLite disk-full error after a successful HTTP 200 doesn't retry the Frigate POST (duplicate upload) — the upload succeeded; only the tracker write failed - cli: apply skip_ids deduplication to the re-fetched people list after a partial merge (some groups succeed, some fail) so unmerged duplicates don't produce two jobs for the same Frigate folder - jobs: strip whitespace from SKIP_PEOPLE/ONLY_PEOPLE elements on split so "Alice, Bob" (space after comma) correctly matches "Bob" --- CHANGELOG.md | 14 ++++++++++++++ pyproject.toml | 2 +- winnow/cli.py | 12 +++++++++++- winnow/diversity.py | 14 ++++++++++++-- winnow/executor.py | 27 ++++++++++++++++++++------- winnow/jobs.py | 4 ++-- 6 files changed, 60 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7f315b..db5b9c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.21] - 2026-06-15 + +### Fixed + +- **`_cluster_aware_selection` now respects the requested limit**: the initial K-Medoids seed count `k` was never capped at `target`, so when the remaining capacity was 1–4 slots the function returned 5+ images instead of the requested count, silently violating `MAX_AUTO_IMAGES`. `k` is now `min(..., target)` and the returned list is sliced to `target` as a final guard. A new early-return for `limit == 0` prevents k-medoids from running at all and returning medoids for a zero-budget request. + +- **`USE_FULL_RESOLUTION=True` path now marks assets rejected on persistent fetch failure**: when both the original and preview fallback in `fetch_full_image()` fail, the asset was silently re-selected and re-attempted on every future run. The full-res path now calls `mark_rejected()` on a `None` return, matching the behavior added in v0.5.20 for the thumbnail path. + +- **`mark_uploaded` tracker failure no longer causes a duplicate Frigate upload**: `mark_uploaded()` was called inside the upload retry `try/except` block. A SQLite error (e.g. disk-full) after a successful HTTP 200 response would propagate to the retry handler, which would retry the POST and upload the same file twice. `mark_uploaded()` is now wrapped in its own `try/except`; a tracker write failure is logged and the upload loop breaks normally so Frigate never receives a duplicate. + +- **`_handle_duplicate_people` deduplicates failed merges when some succeed**: when `MERGE_DUPLICATE_PEOPLE=true` and a mix of merges succeed and fail, `get_people()` was returned directly. The re-fetched list still contained the un-merged duplicate pairs, creating two jobs for the same Frigate folder. The re-fetched list is now filtered using the same skip-id logic applied in the all-fail path. + +- **`SKIP_PEOPLE`/`ONLY_PEOPLE` now strip whitespace from each element**: `"Alice, Bob".split(",")` produces `[' Bob']` (with a leading space), which never matched person names from Immich. Both env vars now use a list comprehension with `.strip()` on each element, so space-padded comma-separated values work as expected. + ## [0.5.20] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 087bd4e..9c58857 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.20" +version = "0.5.21" 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 2e338fa..945e96f 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -125,7 +125,17 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: if merged_any: rprint(" [dim]Re-fetching people after merge...[/dim]") - return get_people() + fresh = get_people() + # Filter out the smaller duplicate from any group whose merge failed — those + # IDs still exist in Immich and would produce two jobs for the same folder. + # IDs from groups that merged successfully are already gone from Immich, so + # this filter is a no-op for them. + 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 fresh if p.get("id") not in skip_ids] # 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. diff --git a/winnow/diversity.py b/winnow/diversity.py index 30451a8..c412743 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -494,8 +494,13 @@ def _cluster_aware_selection( auto_threshold = _compute_adaptive_threshold(emb_normed) if limit == "auto" else 0.0 target = Config.MAX_AUTO_IMAGES if limit == "auto" else limit + # Short-circuit: nothing to select + if limit != "auto" and target <= 0: + return [] + # --- Stage 1: K-Medoids clustering --- - k = min(max(5, target // 4), max(1, n // 3), n) # e.g., 1-20 clusters + # Cap k at target so we never seed more cluster representatives than requested. + k = min(max(5, target // 4), max(1, n // 3), n, target) # e.g., 1-20 clusters logger.debug("Clustering %s embeddings into %s groups (K-Medoids)...", n, k) # Compute full cosine distance matrix @@ -547,7 +552,12 @@ def _cluster_aware_selection( hard_count = sum(1 for c in selected_conf if c < 0.85) logger.info("Selection complete: %s images (%s hard examples with confidence < 0.85).", len(selected), hard_count) - return [candidates[i] for i in selected] + # Slice to target: the while loop enforces this for non-auto mode, but + # guard here too in case the medoid seed already exceeded target (small target). + result = [candidates[i] for i in selected] + if limit != "auto": + result = result[:target] + return result # ============================================================================= diff --git a/winnow/executor.py b/winnow/executor.py index bb6f683..0357b38 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -149,6 +149,10 @@ def execute_jobs(jobs: list[dict]) -> None: # Use full-resolution for final output when configured if use_full_res: img = fetch_full_image(asset["id"]) + if img is None: + # Both original and preview fallback failed — mark rejected + # so this asset isn't retried on every future run. + mark_rejected(asset["id"], person_name=name) else: resp = requests.get( f"{Config.IMMICH_URL}/api/assets/{asset['id']}/thumbnail?size=preview&format=JPEG", @@ -498,13 +502,22 @@ def upload_to_frigate(jobs: list[dict]) -> None: asset_id = asset_map.get(fname) if asset_id: - mark_uploaded( - asset_id, - person_name=name, - score=score_map.get(fname), - crop_dims=dims_map.get(fname), - frigate_score=pre_fscore, - ) + try: + mark_uploaded( + asset_id, + person_name=name, + score=score_map.get(fname), + crop_dims=dims_map.get(fname), + frigate_score=pre_fscore, + ) + except Exception as tracker_exc: + # Upload to Frigate succeeded — don't retry on tracker + # failure or we'd upload a duplicate to Frigate. + logger.error( + "Tracker write failed for %s — upload succeeded" + " but asset may be re-selected next run: %s", + fname, tracker_exc, + ) 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 8a87551..50706e8 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -229,8 +229,8 @@ def auto_configure(people: list[dict]) -> list[dict]: return [] strategy = os.environ.get("STRATEGY", "auto") - skip = os.environ.get("SKIP_PEOPLE", "").split(",") if os.environ.get("SKIP_PEOPLE") else [] - only = os.environ.get("ONLY_PEOPLE", "").split(",") if os.environ.get("ONLY_PEOPLE") else [] + skip = [s.strip() for s in os.environ.get("SKIP_PEOPLE", "").split(",") if s.strip()] + only = [s.strip() for s in os.environ.get("ONLY_PEOPLE", "").split(",") if s.strip()] if only: valid_people = [p for p in valid_people if p["name"] in only] From 794dbe2a1d1497048b520cd145bb9e9a8769edeb Mon Sep 17 00:00:00 2001 From: Holden Salomon Date: Mon, 15 Jun 2026 11:44:41 -0400 Subject: [PATCH 15/15] revert: replace SQLite tracker with JSON backend (v0.6.0) (#32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * revert: replace SQLite tracker with JSON backend (v0.6.0) The SQLite migration (v0.5.0) spawned 21 bug-fix releases in two days: data-loss risk in the migration layer, schema PK conflicts on per-person tracking, tracker isolation races under concurrent runs, and a disk-full error that triggered duplicate Frigate uploads. The complexity cost outweighs the benefit. Restored the pre-SQL JSON tracker (frigate_uploaded_ids.json / frigate_rejected_ids.json in DATA_DIR). Public API is identical — all callers in executor.py, jobs.py, cli.py, and reconcile.py work unchanged. Existing JSON files are read automatically; frigate_tracker.db can be deleted once verified. * fix: narrow corrupt-thumbnail exception to UnidentifiedImageError; restore IMMICH_URL empty-string fallback * docs: rewrite v0.6.0 changelog, strip v0.5.x entries, fix README SQLite references * chore: remove dead get_frigate_filename_for_asset (orphaned since FRIGATE_SCORE_THRESHOLD removal in v0.4.0) * fix: sort imports in executor.py (ruff I001) --- CHANGELOG.md | 306 ++------------- README.md | 4 +- pyproject.toml | 2 +- tests/test_upload_tracker.py | 85 ++--- uv.lock | 2 +- winnow/config.py | 6 +- winnow/executor.py | 10 +- winnow/upload_tracker.py | 697 ++++++++++++++--------------------- 8 files changed, 346 insertions(+), 766 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db5b9c2..a42a83f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,315 +7,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## [0.5.21] - 2026-06-15 - -### Fixed - -- **`_cluster_aware_selection` now respects the requested limit**: the initial K-Medoids seed count `k` was never capped at `target`, so when the remaining capacity was 1–4 slots the function returned 5+ images instead of the requested count, silently violating `MAX_AUTO_IMAGES`. `k` is now `min(..., target)` and the returned list is sliced to `target` as a final guard. A new early-return for `limit == 0` prevents k-medoids from running at all and returning medoids for a zero-budget request. - -- **`USE_FULL_RESOLUTION=True` path now marks assets rejected on persistent fetch failure**: when both the original and preview fallback in `fetch_full_image()` fail, the asset was silently re-selected and re-attempted on every future run. The full-res path now calls `mark_rejected()` on a `None` return, matching the behavior added in v0.5.20 for the thumbnail path. - -- **`mark_uploaded` tracker failure no longer causes a duplicate Frigate upload**: `mark_uploaded()` was called inside the upload retry `try/except` block. A SQLite error (e.g. disk-full) after a successful HTTP 200 response would propagate to the retry handler, which would retry the POST and upload the same file twice. `mark_uploaded()` is now wrapped in its own `try/except`; a tracker write failure is logged and the upload loop breaks normally so Frigate never receives a duplicate. - -- **`_handle_duplicate_people` deduplicates failed merges when some succeed**: when `MERGE_DUPLICATE_PEOPLE=true` and a mix of merges succeed and fail, `get_people()` was returned directly. The re-fetched list still contained the un-merged duplicate pairs, creating two jobs for the same Frigate folder. The re-fetched list is now filtered using the same skip-id logic applied in the all-fail path. - -- **`SKIP_PEOPLE`/`ONLY_PEOPLE` now strip whitespace from each element**: `"Alice, Bob".split(",")` produces `[' Bob']` (with a leading space), which never matched person names from Immich. Both env vars now use a list comprehension with `.strip()` on each element, so space-padded comma-separated values work as expected. - -## [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. +## [0.6.0] - 2026-06-15 ### 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. +- **Upload tracker reverted to JSON storage**: the SQLite-based tracker introduced in v0.5.0 produced 17 bug-fix releases in two days due to data-loss risks in the migration layer, schema primary key conflicts, tracker isolation races, and disk-full retry storms. The JSON backend (`frigate_uploaded_ids.json` / `frigate_rejected_ids.json` in `DATA_DIR`) is restored. It is simpler, has no migration layer, and carries no external dependency. If you ran any v0.5.x version, delete `frigate_tracker.db` from your `DATA_DIR` once you confirm the JSON files look correct. JSON files from before v0.5.0 are read automatically with no changes required. -- **`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. +- **`CACHE_DIR` env var accepted as `DATA_DIR` alias**: the rename introduced in v0.5.1 is preserved — `CACHE_DIR` still works with a deprecation warning. The default data path remains `data` (Docker: `/app/data`). -## [0.5.19] - 2026-06-15 +- **Config file now lives in `DATA_DIR`**: `.immich_config.json` resolves to `DATA_DIR/.immich_config.json` so it persists across container restarts. The legacy CWD location is still checked as a fallback for existing setups. + +- **Diversity selector receives capacity as its limit directly**: instead of selecting up to `MAX_AUTO_IMAGES` and then slicing to the remaining capacity, the selector now runs with the actual remaining slot count as its budget. ### Fixed -- **`fetch_face_data` no longer falls back to an arbitrary person's face**: when `person_id` is provided but not found in the Immich `/api/faces` response, the function now returns `None` instead of falling back to `faces[0]`. Previously a Frigate group photo where the target person's face entry was missing would inject a different person's bounding box, causing the wrong face crop to be uploaded as training data. +- **Immich v2.7.5 compatibility**: `auto_configure` no longer pre-filters people by `assetCount` from `/api/people`, which Immich v2.7.5 dropped. The `MIN_FACE_COUNT` check now runs after `fetch_all_assets` using the actual fetched count. -- **`tracked_assets` PRIMARY KEY now includes `person_name`**: the old `PRIMARY KEY (asset_id, status)` meant that `INSERT OR REPLACE` for person B on an asset already tracked for person A silently overwrote `person_name`, destroying the JOIN between `tracked_assets` and `frigate_files` for person A and breaking quality replacement. The key is now `(asset_id, person_name, status)`, giving each person their own row per asset. Existing databases are migrated automatically on first open. +- **`fetch_face_data` no longer falls back to a wrong person's bounding box**: when `person_id` is provided but not found in the Immich `/api/faces` response, the function now returns `None` instead of using `faces[0]`. Previously a group photo where the target person's face entry was missing would inject a different person's bounding box into the crop. -- **`filter_recent_assets` treats `years=0` as "no age filter"**: previously `years = years or Config.YEARS_FILTER` evaluated `0` as falsy and fell through to the default (10 years), silently discarding all older assets when the user explicitly set `YEARS_FILTER=0`. The check is now `if years is None: years = Config.YEARS_FILTER` followed by an early return for `years=0`. +- **Corrupt thumbnail permanently rejected**: when `resp.ok=True` but `PIL.UnidentifiedImageError` is raised (Pillow cannot identify the image format), the asset is now marked rejected so it isn't re-downloaded on every future run. Transient `OSError`/truncation errors are intentionally not caught here — those are retried normally. -- **`_is_module_available` now correctly returns False for absent modules**: `importlib.util.find_spec` returns `None` (not raises) for missing top-level modules, so the previous `try: find_spec(); return True` always reported modules as installed. Fixed to `return find_spec(...) is not None`, ensuring `is_embedding_available()` returns False when InsightFace or onnxruntime are not installed. +- **`mark_uploaded` tracker failure no longer aborts the upload loop**: a tracker write failure after a successful Frigate POST is logged and the loop continues; the asset will be re-uploaded on the next run rather than the current run dying mid-job. -- **Error handler in `execute_jobs` uses `asset.get("id")` instead of `asset["id"]`**: a malformed asset dict missing the `"id"` key would cause a secondary `KeyError` inside the `except` block, propagating uncaught out of `execute_jobs()` and aborting the run mid-job. Changed to `asset.get("id", "")`. +- **`progress.remove_task` now in `finally` block**: the progress bar task is cleaned up even when a job exits via an exception, preventing orphaned progress rows in the terminal. -- **HTTP 422 now triggers `mark_rejected`**: only `HTTP 400` with `"face"` in the body triggered permanent rejection; `HTTP 422` (Unprocessable Entity) left the asset untracked and caused it to be re-selected and re-attempted on every future run. Both codes are now treated as permanent rejections. +- **`SKIP_PEOPLE`/`ONLY_PEOPLE` now strip whitespace**: `"Alice, Bob".split(",")` produces `[" Bob"]`; the leading space now stripped so comma-separated values with spaces work as expected. -- **Frigate filename reconciliation sort is now deterministic**: `sorted(new_files, key=_ts)` sorted a `set` — when `_ts()` returns `0.0` for non-matching filenames, Python's stable sort preserves the set's hash-randomised input order, producing non-deterministic `asset_id → frigate_filename` mappings. Changed the key to `lambda f: (_ts(f), f)` so equal-timestamp files sort alphabetically. +- **`FRIGATE_URL` with trailing slash no longer produces double-slash paths**: all Frigate API calls now use `_get_frigate_url()` for URL normalization rather than reading `FRIGATE_URL` inline. -### Changed +- **Frigate version `v`-prefix now stripped**: `v0.16.0`-style version strings are correctly parsed. -- **`_resolve_strategy` uses `_getenv_optional_int("LIMIT")`**: the inline `os.environ.get("LIMIT", "").strip()` + `int()` + `logger.warning` block in `jobs.py` re-implemented the logic already in `_getenv_num`. A new `_getenv_optional_int` helper (delegating to `_getenv_num(name, None, int)`) replaces the duplicate, consolidating LIMIT parse warnings with the rest of the env-var helpers. +- **Invalid numeric env var values warn and use defaults**: a typo such as `YEARS_FILTER=10 ` (trailing space) or `MIN_FACE_WIDTH=auto` now logs a `WARNING` and falls back to the documented default instead of raising `ValueError` at startup. Affects `YEARS_FILTER`, `MIN_FACE_WIDTH`, `MIN_FACE_COUNT`, `MAX_AUTO_IMAGES`, `BLUR_THRESHOLD`, `MIN_CONFIDENCE`, and `FACE_MARGIN`. -- **`frigate_api.py` uses a shared `_get_frigate_url()` accessor**: `os.environ.get("FRIGATE_URL", "").rstrip("/")` was copy-pasted into all four public functions. A private helper eliminates the duplication so URL normalization is defined once. +- **`IMMICH_URL` blank placeholder falls back to config file**: `IMMICH_URL=` (empty or blank) in `.env` is now treated as unset and falls through to `DATA_DIR/.immich_config.json`, matching pre-v0.5.0 behaviour. -- **`blur_score_from_image()` extracted to `quality.py`**: the time-spread blur-score fallback in `execute_jobs` (resize to 1440px, RGB convert, `assess_quality`) is now a shared `blur_score_from_image(img, max_dim=1440)` helper. Both the executor and any future callers use the same cap and error handling so the score scale can't silently diverge between code paths. +- **Reconciliation checks Frigate immediately before first sleep**: the poll loop now performs an immediate check after upload, then backs off with `(1, 2, 4, 8)` s delays only if needed. -## [0.5.18] - 2026-06-15 +- **Dockerfile unknown `VARIANT` now fails loudly**: an unrecognised value now exits with an error instead of silently falling through to the cpu branch. -### Fixed +- **Embedding cache writes are now atomic**: `.npy` files are written to a `.tmp` sibling and renamed into place with `os.replace`, preventing truncated cache entries on process kill. -- **`scripts/benchmark.py` now uses `_getenv_bool` for `FORCE_CPU`**: the script retained an inline `os.getenv("FORCE_CPU", "").lower() in ("true", "1", "yes")` pattern in `_mode_label()` after `_getenv_bool` was introduced. Replaced with a local import of `_getenv_bool` consistent with how all other winnow imports in the script are deferred into function bodies. - -## [0.5.17] - 2026-06-15 - -### Fixed - -- **`_getenv_num` and `_getenv_bool` now treat an explicitly-empty env var as unset**: previously, `YEARS_FILTER=` (blank) in a `.env` or Compose file caused `int("")` to raise `ValueError`, logging a spurious "not a valid int" warning and returning the default. Both helpers now strip whitespace and treat an empty string the same as an absent variable, returning the typed default silently. This affects all numeric config vars (`YEARS_FILTER`, `MIN_FACE_WIDTH`, `MIN_FACE_COUNT`, `MAX_AUTO_IMAGES`, `BLUR_THRESHOLD`, `MIN_CONFIDENCE`, `FACE_MARGIN`) and all boolean config vars. `_getenv_optional_float` already handled this correctly. - -- **`FORCE_CPU` now uses `_getenv_bool`**: `embeddings.py` retained the old inline `os.getenv("FORCE_CPU", "").lower() in ("true", "1", "yes")` pattern after v0.5.16 introduced `_getenv_bool`. The inline copy is now replaced so the canonical truthy-string set is defined in one place. - -## [0.5.16] - 2026-06-15 - -### Changed - -- **`_getenv_int` and `_getenv_float` now share a single `_getenv_num` implementation**: the two helpers were structurally identical (read env var, return typed default if absent, try-cast, warn and return default on `ValueError`) with only the cast differing. Both are now thin wrappers around a private `_getenv_num(name, default, cast)`, eliminating the duplicated warning logic. - -- **`FRIGATE_SCORE_CEILING` now uses `_getenv_optional_float`**: the previous 9-line inline block (`os.getenv("FRIGATE_SCORE_CEILING", "").strip()` + try/except) has been replaced with a new `_getenv_optional_float(name) -> float | None` helper that encapsulates the "empty-string means None, parse-error means None" semantics, making it consistent with the other numeric env var helpers. - -- **Boolean env vars now use `_getenv_bool`**: the `.lower() in ("true", "1", "yes")` pattern was repeated across 11 sites in `config.py`, `jobs.py`, and `cli.py`. A new `_getenv_bool(name, default)` helper centralises the canonical truthy-string set; all sites have been updated to call it. - -- **`_resolve_strategy` no-embedding branch uses `_getenv_int`**: the inline `int(custom_limit)` try/except block in `jobs.py` for the time-spread path has been replaced with `_getenv_int("LIMIT", 30)`, matching the pattern used in `config.py`. The smart-mode path retains its own try/except because its fallback is to the strategy map rather than to a numeric default. - -- **`cache.py` tmp path uses `str.removesuffix`**: `final[:-4] + ".tmp.npy"` replaced with `final.removesuffix(".npy") + ".tmp.npy"` — the assumption that the cache path ends in `.npy` is now explicit and self-documenting rather than expressed as a magic numeric slice. - -## [0.5.15] - 2026-06-15 - -### Fixed - -- **Embedding cache writes were silently no-ops since v0.5.13**: the atomic-write path used `tmp = final + ".tmp"` where `final` ends in `.npy` (e.g. `abc.npy`), producing a tmp path of `abc.npy.tmp`. `np.save` auto-appends `.npy` to paths not already ending in `.npy`, so it wrote to `abc.npy.tmp.npy` instead. The subsequent `os.replace("abc.npy.tmp", "abc.npy")` then raised `FileNotFoundError` (caught silently at DEBUG), meaning no cache entry was ever committed and leaked `*.npy.tmp.npy` files accumulated on disk. The fix inserts `.tmp` before the `.npy` extension: `tmp = final[:-4] + ".tmp.npy"` so `np.save` sees a path already ending in `.npy` and does not re-append. - -- **`_getenv_int`/`_getenv_float` no longer route the default through `str()` conversion**: the previous form `os.getenv(name, str(default))` converted the default to a string so it could be fed through `int()`/`float()` — an unnecessary round-trip that would cause `_getenv_int("FOO", 4.0)` to log a spurious "not a valid integer" warning and return the float. The helpers now use `raw = os.getenv(name); return default if raw is None else int(raw)`, passing the typed default through directly. - -- **`execute_jobs` progress task now removed via `try/finally`**: `progress.remove_task(job_task)` was duplicated in three early-exit paths (ValueError, symlink TOCTOU, OSError) plus once at normal completion. The entire per-job body is now wrapped in `try/finally: progress.remove_task(job_task)`; the three inner `continue` statements trigger the `finally` automatically before advancing to the next job, making the invariant structurally impossible to violate by a future code path. - -## [0.5.14] - 2026-06-15 - -### Fixed - -- **Invalid env var values for numeric config now warn and use defaults**: `YEARS_FILTER`, `MIN_FACE_WIDTH`, `MIN_FACE_COUNT`, `MAX_AUTO_IMAGES`, `BLUR_THRESHOLD`, `MIN_CONFIDENCE`, and `FACE_MARGIN` all used bare `int()`/`float()` with no error handler. A typo such as `YEARS_FILTER=10 ` (trailing space) or `MIN_FACE_WIDTH=auto` raised `ValueError` from inside `__getattr__`, surfacing as a cryptic traceback on the first config access rather than at the config-validation step where a helpful error is expected. The values are now parsed with module-level `_getenv_int` / `_getenv_float` helpers that log a `WARNING` and fall back to the documented default on parse failure, matching the existing pattern already used for `FRIGATE_SCORE_CEILING`. - -## [0.5.13] - 2026-06-15 - -### Fixed - -- **`execute_jobs` output-dir OSError now skips the job instead of aborting the run**: `shutil.rmtree` and `os.makedirs` were not wrapped in any error handler — an `OSError` or `PermissionError` (e.g. read-only filesystem, lingering lock) propagated out of the `for job in jobs` loop, abandoning `job_task` in the Rich progress display and silently dropping all remaining jobs. Both calls are now wrapped in `try/except OSError`; on failure the error is logged, the progress task is removed, and the loop continues to the next job. - -- **Embedding cache writes are now atomic**: `cache.py` previously called `np.save(path, embedding)` directly to the final `.npy` path. A process kill or container stop mid-write left a truncated file that `np.load` would subsequently raise on. Because `get()` catches the exception and returns `None`, the slot appeared empty on every future run — the corrupted file was never cleaned up and the embedding was silently recomputed forever. The write now goes to a `.tmp` sibling and is renamed into place with `os.replace` (atomic on POSIX); the tmp file is removed on any write failure. - -- **`filter_recent_assets` guards against non-string `fileCreatedAt`**: the previous `if not created_at_str` guard passed truthy non-string values (e.g. a Unix-epoch integer returned by some Immich API versions), after which `created_at_str.replace("Z", "+00:00")` raised `AttributeError`. That exception was not caught by the surrounding `except ValueError`, so a single non-string timestamp aborted the entire filtering pass for the person being processed. The guard is now `if not isinstance(created_at_str, str) or not created_at_str`. - -- **SQLite connection timeout raised to 30 s**: `sqlite3.connect` defaulted to a 5-second busy timeout. Under concurrent access (scheduled and manual runs overlapping), 5 s was often insufficient, causing `OperationalError: database is locked` that propagated through `upload_to_frigate` and dropped upload-tracking records — assets would then be re-uploaded on the next run. The timeout is now 30 s, matching the typical upload cycle length. - -## [0.5.12] - 2026-06-15 - -### Fixed - -- **Progress task leak on skipped jobs**: `progress.add_task()` is called unconditionally at the top of the job loop, but both early-exit `continue` paths — the `ValueError` skip from `_safe_person_dir` and the symlink-TOCTOU skip added in v0.5.11 — bypassed `progress.remove_task()`, leaving orphaned 0% rows in the terminal display for the rest of the run. Both `continue` paths now call `progress.remove_task(job_task)` before continuing. - -## [0.5.11] - 2026-06-15 - -### Fixed - -- **`execute_jobs` symlink TOCTOU gap closed**: the v0.5.10 guard `os.path.isdir(person_dir) and not os.path.islink(person_dir)` silently skipped the wipe when `person_dir` was a symlink-to-directory, then called `os.makedirs` which followed the symlink — allowing crop writes to land outside `output_dir` with no log or skip. The guard is replaced by an explicit pre-check: if `os.path.islink(person_dir)` is True, log an error and `continue`, matching the established `ValueError` pattern from `_safe_person_dir`. The `isdir` / `rmtree` block is restored to its original simple form. - -## [0.5.10] - 2026-06-15 - -### Fixed - -- **Reconcile `< target` branch re-escalated to WARNING**: when fewer Frigate files appear than expected after the full backoff window, the affected files are permanently unmapped — identical in consequence to the `> target` (external upload race) case fixed in v0.5.9. The v0.5.9 demotion to `INFO` was incorrect; both post-loop branches now log at `WARNING` and include the "permanently unmapped" label. - -- **`execute_jobs` symlink guard added before `shutil.rmtree`**: `os.path.isdir` follows symlinks and returns `True` for a symlink pointing at a directory. If a race condition replaces `person_dir` with such a symlink, the old guard would pass and `shutil.rmtree` would raise an unhandled `OSError`, aborting all remaining jobs in the batch. The guard is now `os.path.isdir(person_dir) and not os.path.islink(person_dir)`, so a symlink-to-directory is silently skipped. The comment is also corrected: `shutil.rmtree` raises `OSError` (not `NotADirectoryError`) on a top-level symlink. - -## [0.5.9] - 2026-06-15 - -### Fixed - -- **Reconcile log severity corrected**: the external-upload branch (`len(new_files) > target`) was logged at `INFO` while the timeout branch (`< target`) was logged at `WARNING`. The severity is now inverted to match impact: external upload causes permanent mapping loss (those files are never eligible for quality replacement) and is now `WARNING`; timeout is transient and recoverable next cycle and is now `INFO`. - -- **`fetch_all_assets` docstring: lower-bound caveat now covers both interruption cases**: previously only noted that a network error makes `total_raw` a lower bound. An all-garbage page (every item non-dict) also terminates pagination early, leaving later pages unfetched — this case is now documented alongside the network error case. - -- **`shutil.rmtree` symlink safety documented**: added a comment above the `rmtree` call in `execute_jobs` noting that POSIX `shutil.rmtree` raises `NotADirectoryError` on a top-level symlink, so a race-replaced symlink cannot cause out-of-tree deletion. - -- **`_entry()` in `get_person_summary` no longer allocates default dict for present keys**: `setdefault` evaluates its default argument before checking whether the key exists, allocating and immediately discarding a 5-key dict on every call for an already-present person. Replaced with an explicit `if name not in summary` guard. - -## [0.5.8] - 2026-06-15 - -### Fixed - -- **`_safe_person_dir` docstring corrected**: the previous comment stated "checking after realpath would be too late because realpath follows the link first," which implied `islink` was the primary security guard. The load-bearing path-traversal check is `realpath + startswith`; it rejects both `../../` traversal and symlinks. The `islink` check is a supplementary early-exit that provides a cleaner error message for the symlink sub-case only. - -- **`total_raw` not inflated by all-garbage pages**: `fetch_all_assets` previously added `page_count` to `total_raw` before checking whether any valid dict items existed. A page returning only non-dict items would inflate `total_raw` and produce a misleading "N total, 0 recent" display. `total_raw` now accumulates only after `valid_assets` is confirmed non-empty, so all-garbage pages break without contributing. Mixed pages (some valid, some non-dict) still count `page_count` so transient schema glitches on a partial page don't cause `MIN_FACE_COUNT` to incorrectly skip a real person. - -- **Pagination interruption warning**: when a `RequestException` breaks pagination mid-way (page > 1), a `WARNING` is now logged noting that `total_raw` is a lower bound. Previously the exception was logged at `ERROR` with no indication that the `MIN_FACE_COUNT` comparison was using a partial count. - -- **Config TOCTOU residue**: `config.py` line 135 re-stat'd `_data_cfg` when it was the selected config file, creating a second TOCTOU window after the fix in v0.5.7. The check is now `if _data_cfg_exists or config_file.exists():` — the primary path is never stat'd again, and the legacy path is stat'd at most once. - -## [0.5.7] - 2026-06-15 - -### Fixed - -- **Symlink guard moved before `realpath`**: `_safe_person_dir` now checks whether the raw (unresolved) path is a symlink before calling `os.path.realpath`. The previous check in `execute_jobs` ran after `realpath` had already resolved the link, making it unreachable dead code. - -- **`fetch_all_assets` returns raw item count**: the function now returns `(assets, total_raw)` where `total_raw` is the total items seen across all pages before non-dict filtering. `auto_configure` and `_configure_person` use `total_raw` for the `MIN_FACE_COUNT` guard and display, so transient non-dict API items cannot cause a person to be incorrectly skipped. - -- **Pagination stop on all-non-dict page now logs a warning**: when `valid_assets` is empty but the page was non-empty (all items were non-dict), a `WARNING` is emitted explaining why pagination stopped, distinguishing it from natural end-of-data. - -- **`_data_cfg.exists()` called once**: the result is cached in `_data_cfg_exists` so the dual-config warning check and the `config_file` selection always agree — previously two separate `stat()` calls created a TOCTOU window where the log could claim one file while the code loaded another. - -## [0.5.6] - 2026-06-15 - -### Fixed - -- **Pagination runaway on all-non-dict page**: the empty-page break in `fetch_all_assets` now fires after non-dict filtering rather than before, so a page whose items are all non-dict (e.g. all nulls) correctly terminates pagination instead of looping to MAX_PAGES. - -- **Non-dict API items upgraded to warning**: items skipped in a paginated response are now logged at `WARNING` (previously `DEBUG`) so silent asset loss is visible at default log levels. - -- **Single-pass page filtering**: `fetch_all_assets` now partitions valid and invalid items in one loop instead of iterating `page_assets` twice with inverse predicates. - -- **Reconciliation checks Frigate before sleeping**: the poll loop now performs an initial check immediately after upload, then backs off with `_RECONCILE_POLL_DELAYS` only if needed. Previously the loop always slept ≥1 s before any check. - -- **Reconciliation set subtraction computed once**: `current_files - known_files_before` was computed twice per poll iteration (once for the count check, once for the final mapping). It is now computed once and reused. - -## [0.5.5] - 2026-06-15 - -### Changed - -- **Module-level constants in `diversity.py`**: magic numbers `3000` (pool cap), `20` (pool scale), and `32` (embedding batch size) extracted to named constants `_POOL_CAP`, `_POOL_SCALE`, and `_EMBEDDING_BATCH_SIZE`. - -- **Reconciliation poll delays extracted**: `(1, 2, 4, 8)` back-off delays in `reconcile.py` extracted to `_RECONCILE_POLL_DELAYS` with an explanatory comment. - -- **`_VALID_SCORE_COLS` comment**: explains that the frozenset is a SQL-injection guard for dynamic column interpolation, not a runtime filter. - -- **`record_frigate_files_batch` docstring**: clarifies that all mappings are written atomically — no partial failure is possible. - -- **`get_person_summary()` refactored**: eliminated four repeated default-dict blocks using a local `_entry()` helper with `setdefault`. - -- **`encoded` → `encoded_name` in `frigate_api.py`**: renamed the URL-encoded person name variable for clarity. - -- **Dual response shape comment in `immich_api.py`**: documents that Immich ≥2.x returns `{"assets": {"items": [...]}}` while earlier versions returned `{"assets": [...]}` directly. - -- **Non-dict item debug log in `fetch_all_assets`**: skipped non-dict items in a page response now emit a `logger.debug` line with the count and page number. - -## [0.5.4] - 2026-06-14 - -### Fixed - -- **Quality replacement slot floor used wrong score**: when a blur-score replacement deleted a low-quality Frigate file but the subsequent upload failed, `min_quality_score_for_slot` was set to the failed candidate's score rather than the deleted file's score. This caused subsequent candidates that were better than the deleted file (but worse than the failed upload) to be skipped, leaving the freed slot unfilled for the rest of that run. Fixed by using `target_score` (deleted file's score) as the floor, matching the documented intent in the surrounding comment. - -## [0.5.3] - 2026-06-14 - -### Fixed - -- **`LIMIT` env var crash**: non-integer values (e.g. `"30.5"`, `"all"`) now log a warning and fall back to the default instead of raising `ValueError` at startup. - -- **Symlink guard on person output dir**: `shutil.rmtree` is now skipped if `person_dir` resolves to a symlink, preventing traversal out of `OUTPUT_DIR` on a shared volume. - -- **`person["id"]` KeyError**: malformed Immich API responses missing the `id` field now log an error and skip that person instead of crashing the job. - -- **Face data response type validation**: `fetch_face_data` now validates that the `/api/faces` response is a list before indexing, guarding against null or non-list API responses. - -- **Pagination error log includes page number**: the exception log in `fetch_all_assets` now includes the page number that failed. - -- **`Image.open()` wrapped for non-image responses**: PIL parse errors on thumbnail fetches (e.g. reverse-proxy HTML error page returning 200) are now caught and logged instead of propagating. - -- **Frigate version `v`-prefix handling**: `v0.16.0`-style version strings are now correctly parsed; the leading `v` was previously misread, causing the too-old warning to never fire. - -- **`FRIGATE_SCORE_CEILING` parse guard**: a non-float value in `.env` now logs a warning and disables the ceiling instead of crashing at startup. - -- **Dual config file warning**: a log warning is emitted when both `DATA_DIR/.immich_config.json` and the legacy CWD config file exist simultaneously. - -- **PID file write guard**: `OSError` on `/tmp/winnow.pid` write is now caught and logged instead of crashing the scheduler. - -- **Scheduler sleep clamped to 60 s**: bounds recovery time after an NTP clock step. - -- **`get_frigate_person_files` non-list debug log**: consistent with `get_all_frigate_person_files`. - -## [0.5.2] - 2026-06-14 - -### Fixed - -- **Immich v2.7.5 compatibility**: `auto_configure` no longer pre-filters people by `assetCount` from the `/api/people` response, which Immich v2.7.5 dropped. The `MIN_FACE_COUNT` check now runs after `fetch_all_assets` so the actual asset count is used instead of the missing field. - -- **Dockerfile supply-chain**: replaced `curl | sh` uv installer with `COPY --from=ghcr.io/astral-sh/uv:0.11.21` to eliminate the network-executed script. - -- **HEALTHCHECK**: replaced the static file-existence check with `kill -0 $(cat /tmp/winnow.pid)` so the container reports unhealthy when the scheduler process actually dies, not just when a script file is missing. - -- **`CONFIG_FILE` volume safety**: the config file path now resolves to `DATA_DIR/.immich_config.json` so it persists across container restarts. The legacy CWD location is still read as a fallback for existing setups. - -- **EmbeddingCache singleton isolation**: `get_cache()` now tracks the `cache_dir` argument and re-creates the cache when it changes, preventing test runs from sharing state across different `DATA_DIR` values. - -- **File descriptor leak in `_suppress_output()`**: `devnull_fd`, `saved_out`, and `saved_err` are now all closed in a nested `finally` chain, preventing fd exhaustion on long runs. - -- **Silent exception in `upload_tracker`**: `except Exception: pass` on SQLite connection close is now `except Exception as e: logger.debug(...)` so connection errors are visible in debug logs. - -- **Frigate API unknown-key logging**: `get_all_frigate_person_files` now logs unexpected non-list keys at DEBUG level instead of silently skipping them. - -- **Reconcile debug log**: added a debug log entry before the FIFO timestamp mapping step in `reconcile_frigate_mappings` to make the mapping assumption visible in logs. - -- **CI action SHA pinning**: all five GitHub Actions workflows now pin every third-party action to a full commit SHA. Updated `setup-uv` v7→v8.2.0, `upload-artifact` v4→v7.0.1, `download-artifact` v4→v8.0.1, `ruff-action` v3→v4.0.0. - -## [0.5.1] - 2026-06-14 - -### Changed - -- **`CACHE_DIR` renamed to `DATA_DIR`**: the environment variable that sets the path for the embedding cache and SQLite tracker database is now called `DATA_DIR` (default: `data`; Docker default: `/app/data`). The old `CACHE_DIR` still works with a startup deprecation warning — rename it to `DATA_DIR` in your `.env` or `compose.yml` to silence the warning. The container-side default path changes from `/app/.if_cache` to `/app/data`; update your volume mount accordingly. - -## [0.5.0] - 2026-06-14 - -### Changed - -- **SQLite upload tracker**: `upload_tracker.py` is fully rewritten on top of SQLite (stdlib `sqlite3`). The JSON pair (`frigate_uploaded_ids.json` / `frigate_rejected_ids.json`) is replaced by a single `winnow_tracker.db` (WAL journal, `check_same_thread=False`). Existing JSON files are migrated atomically on first run and renamed to `.json.bak`. No user action required; the tracker API (`mark_uploaded`, `mark_rejected`, `filter_already_uploaded`, `get_person_summary`, etc.) is unchanged. - -- **Config lazy singleton**: `_Config` now uses `__getattr__` to defer all I/O until the first attribute access. `load_dotenv()` no longer runs at module import time — it runs on the first access to any `Config` attribute. Empty-string env vars (`IMMICH_URL=`, `OUTPUT_DIR=`) are now correctly distinguished from unset ones so a `.env` file value never silently overrides an explicit `""` set in the environment. `Config.reset()` clears the loaded state for clean test isolation. - -- **Reconcile module extracted**: `reconcile_frigate_mappings` and `enrich_asset_with_face_data` are extracted from `executor.py` into a new `winnow/reconcile.py` module. No behaviour change; reduces `executor.py` length and clarifies responsibility boundaries. - -- **Single lockfile**: `pyproject-gpu.toml`, `pyproject-cpu.toml`, `pyproject-rocm.toml`, `pyproject-intel.toml` and their separate lockfiles are removed. GPU/ROCm/Intel/CPU variant deps are now declared as `[project.optional-dependencies]` extras in `pyproject.toml` with `[tool.uv] conflicts` for mutual exclusion. A single `uv.lock` covers all variants. The Dockerfile selects the correct extra via `uv sync --extra $VARIANT`. - -- **Ubuntu base bumped**: amd64 GPU base updated from `nvidia/cuda:12.8.1-cudnn-runtime-ubuntu22.04` to `nvidia/cuda:12.8.1-cudnn-runtime-ubuntu24.04`. amd64 ROCm and CPU bases updated from Ubuntu 22.04 to Ubuntu 26.04. arm64 bases remain Ubuntu 24.04. - -### Fixed - -- **Frigate API unreachable at upload start no longer crashes reconciliation**: when the Frigate `GET /api/faces` call fails at upload start, reconciliation is now skipped entirely for that batch (`_skip_reconcile = True`). Previously, falling back to the tracker's known filenames as the pre-upload baseline caused the `> target` guard to fire on unmapped manual files, silently dropping all mappings. - -- **Polling `== target` guards against wrong-file mapping**: the reconcile poll loop now breaks on `len(new_files) == target` and sets an "external upload detected" flag when `> target`. The old `>= target` break would have proceeded with an incorrect file set when a concurrent external upload was present, causing wrong asset-ID mappings. The poll loop now also exits early on `> target` rather than exhausting all four retry intervals (up to 15 s wasted per person with a concurrent external uploader). - -- **`auto_cap` post-selection truncation removed**: the diversity selector now receives the correct upper bound (`capacity` or `min(limit, capacity)`) directly instead of selecting up to `MAX_AUTO_IMAGES` and then silently truncating the result list. The old approach produced a selection biased toward the first `capacity` items in embedding space rather than the globally optimal diverse subset. - -- **Dockerfile unknown VARIANT now fails loudly**: added an explicit `elif [ "$VARIANT" = "gpu" ]` branch and an `else … exit 1` for unrecognised values. Previously, any unknown variant silently fell through to the `cpu` branch. - -- **JSON migration partial-rename data loss**: if the rename of one of the two JSON files failed (e.g. a `PermissionError`), the other file's data was committed to SQLite but the `COUNT(*) > 0` guard on the next run would skip re-migration of the remaining file, permanently losing its data. The guard is removed (idempotent `INSERT OR IGNORE` makes re-running safe). Each rename is now wrapped in its own `try/except OSError` so a failure on one file is logged and does not prevent the other from completing. - -- **SQL column allowlist in `_pick_mapped_file`**: the `score_col` f-string interpolation into SQL is now guarded by a `frozenset` allowlist at the function boundary, raising `ValueError` on any value outside `{"blur_score", "frigate_score"}`. - -- **`load_dotenv` no longer runs at import time**: moving `load_dotenv()` to the first line of `_load()` prevents side-effects during module import (which could interfere with test environment setup) and makes the load order deterministic relative to `os.environ` overrides. - -- **Empty-string env var priority fix**: `if self.IMMICH_URL or …` treated `IMMICH_URL=""` as falsy and silently fell through to the config file. Changed to `if self.IMMICH_URL is None` so an empty-string explicit env var is respected. +- **`EmbeddingCache` singleton re-creates when `DATA_DIR` changes**: prevents test runs from sharing cache state across different `DATA_DIR` values. ### Added -- **Diversity test suite expanded** (PR #11): 33 new tests covering k-medoids clustering, farthest-point sampling, adaptive threshold computation, near-duplicate deduplication, and time-spread selection. Total: 93 tests (was 60). - -- **Known-limitation annotations** (PR #12): `TODO(frigate-api)` comments placed at each FIFO-ordering assumption, manual-file-invisibility note, and async-rebuild limitation in `executor.py` and `reconcile.py`. These mark spots where a richer Frigate API would allow a deeper fix. +- **Diversity test suite** (PR #11): 33 tests covering k-medoids clustering, farthest-point sampling, adaptive threshold computation, near-duplicate deduplication, and time-spread selection. Total: 93 tests. ## [0.4.11] - 2026-06-14 diff --git a/README.md b/README.md index a57fae6..39cf1b9 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ Immich library ↳ at cap + QUALITY_REPLACEMENT=false — skip this person ``` -Uploaded and rejected asset IDs are persisted across runs in a SQLite database (`winnow_tracker.db` in `DATA_DIR`). The same image is never processed twice; rejected assets are permanently skipped unless `RETRY_REJECTED=true`. +Uploaded and rejected asset IDs are persisted across runs in two JSON files (`frigate_uploaded_ids.json` and `frigate_rejected_ids.json` in `DATA_DIR`). The same image is never processed twice; rejected assets are permanently skipped unless `RETRY_REJECTED=true`. --- @@ -212,7 +212,7 @@ These defaults are tuned for Frigate's ArcFace requirements. winnow will warn on | `FORCE_CPU` | `false` | Disable GPU — fall back to CPU for all inference | | `OPENVINO_DEVICE` | `CPU` | Intel variant only: set `GPU` to use Arc or iGPU; default runs on CPU | | `ENABLE_CACHE` | `true` | Cache computed embeddings to disk (speeds up re-runs on the same library) | -| `DATA_DIR` | `data` | Path for embedding cache and upload tracker database (`winnow_tracker.db`) | +| `DATA_DIR` | `data` | Path for embedding cache and upload tracker JSON files | | `INSIGHTFACE_HOME` | *(system)* | InsightFace model cache path (Buffalo_L) | ### Output diff --git a/pyproject.toml b/pyproject.toml index 9c58857..e8a2694 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.21" +version = "0.6.0" 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/tests/test_upload_tracker.py b/tests/test_upload_tracker.py index 827b42d..51c9159 100644 --- a/tests/test_upload_tracker.py +++ b/tests/test_upload_tracker.py @@ -10,19 +10,7 @@ def isolated_cache(monkeypatch, tmp_path): monkeypatch.setenv("DATA_DIR", str(tmp_path)) from winnow.config import _Config _Config.reset() - # Also reset the SQLite connection so the next call opens the new path - import winnow.upload_tracker as ut - ut._conn = None - ut._conn_path = None yield tmp_path - # Teardown - if ut._conn is not None: - try: - ut._conn.close() - except Exception: - pass - ut._conn = None - ut._conn_path = None _Config.reset() @@ -85,12 +73,12 @@ def test_duplicate_marks_are_idempotent(): # ── frigate_files mapping ───────────────────────────────────────────────────── -def test_record_and_remove_frigate_files_batch(): - from winnow.upload_tracker import get_person_summary, record_frigate_files_batch, remove_frigate_file - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-a1"}) +def test_record_and_remove_frigate_file(): + from winnow.upload_tracker import get_person_summary, record_frigate_file, remove_frigate_file + record_frigate_file("Alice", "Alice-1000.webp", "asset-a1") assert "Alice-1000.webp" in get_person_summary()["Alice"]["frigate_files"] remove_frigate_file("Alice", "Alice-1000.webp") - assert "Alice-1000.webp" not in get_person_summary().get("Alice", {}).get("frigate_files", {}) + assert "Alice-1000.webp" not in get_person_summary()["Alice"]["frigate_files"] def test_remove_nonexistent_frigate_file_is_safe(): @@ -104,11 +92,11 @@ def test_remove_frigate_file_does_not_unmark_asset(): from winnow.upload_tracker import ( filter_already_uploaded, mark_uploaded, - record_frigate_files_batch, + record_frigate_file, remove_frigate_file, ) mark_uploaded("asset-a1", person_name="Alice") - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-a1"}) + record_frigate_file("Alice", "Alice-1000.webp", "asset-a1") remove_frigate_file("Alice", "Alice-1000.webp") # Asset must still be excluded — it was deliberately replaced, not lost assert filter_already_uploaded(["asset-a1"]) == [] @@ -120,14 +108,14 @@ def test_get_tracked_frigate_file_count_zero_when_empty(): def test_get_tracked_frigate_file_count_counts_only_mapped(): - """Only files explicitly recorded via record_frigate_files_batch count toward the cap.""" - from winnow.upload_tracker import get_tracked_frigate_file_count, mark_uploaded, record_frigate_files_batch + """Only files explicitly recorded via record_frigate_file count toward the cap.""" + from winnow.upload_tracker import get_tracked_frigate_file_count, mark_uploaded, record_frigate_file mark_uploaded("asset-a", person_name="Alice") mark_uploaded("asset-b", person_name="Alice") - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-a"}) + record_frigate_file("Alice", "Alice-1000.webp", "asset-a") # asset-b is uploaded but not yet mapped — does not count assert get_tracked_frigate_file_count("Alice") == 1 - record_frigate_files_batch("Alice", {"Alice-1001.webp": "asset-b"}) + record_frigate_file("Alice", "Alice-1001.webp", "asset-b") assert get_tracked_frigate_file_count("Alice") == 2 @@ -140,11 +128,12 @@ def test_get_lowest_quality_mapped_file_returns_lowest(): from winnow.upload_tracker import ( get_lowest_quality_mapped_file, mark_uploaded, - record_frigate_files_batch, + record_frigate_file, ) mark_uploaded("asset-hi", person_name="Alice", score=0.95) mark_uploaded("asset-lo", person_name="Alice", score=0.71) - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-hi", "Alice-1001.webp": "asset-lo"}) + record_frigate_file("Alice", "Alice-1000.webp", "asset-hi") + record_frigate_file("Alice", "Alice-1001.webp", "asset-lo") result = get_lowest_quality_mapped_file("Alice") assert result is not None frigate_filename, asset_id, score = result @@ -158,11 +147,12 @@ def test_get_lowest_quality_mapped_file_skips_unscored(): from winnow.upload_tracker import ( get_lowest_quality_mapped_file, mark_uploaded, - record_frigate_files_batch, + record_frigate_file, ) mark_uploaded("asset-scored", person_name="Alice", score=0.85) mark_uploaded("asset-noscr", person_name="Alice") - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-scored", "Alice-1001.webp": "asset-noscr"}) + record_frigate_file("Alice", "Alice-1000.webp", "asset-scored") + record_frigate_file("Alice", "Alice-1001.webp", "asset-noscr") result = get_lowest_quality_mapped_file("Alice") assert result is not None assert result[1] == "asset-scored" # only scored file is a candidate @@ -176,26 +166,28 @@ def test_get_tracked_frigate_filenames_empty(): def test_get_tracked_frigate_filenames_returns_mapped(): - from winnow.upload_tracker import get_tracked_frigate_filenames, record_frigate_files_batch - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-a", "Alice-1001.webp": "asset-b"}) + from winnow.upload_tracker import get_tracked_frigate_filenames, record_frigate_file + record_frigate_file("Alice", "Alice-1000.webp", "asset-a") + record_frigate_file("Alice", "Alice-1001.webp", "asset-b") assert get_tracked_frigate_filenames("Alice") == {"Alice-1000.webp", "Alice-1001.webp"} def test_get_tracked_frigate_filenames_excludes_removed(): from winnow.upload_tracker import ( get_tracked_frigate_filenames, - record_frigate_files_batch, + record_frigate_file, remove_frigate_file, ) - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-a", "Alice-1001.webp": "asset-b"}) + record_frigate_file("Alice", "Alice-1000.webp", "asset-a") + record_frigate_file("Alice", "Alice-1001.webp", "asset-b") remove_frigate_file("Alice", "Alice-1000.webp") assert get_tracked_frigate_filenames("Alice") == {"Alice-1001.webp"} def test_get_tracked_frigate_filenames_isolated_by_person(): - from winnow.upload_tracker import get_tracked_frigate_filenames, record_frigate_files_batch - record_frigate_files_batch("Alice", {"Alice-1000.webp": "asset-a"}) - record_frigate_files_batch("Bob", {"Bob-2000.webp": "asset-b"}) + from winnow.upload_tracker import get_tracked_frigate_filenames, record_frigate_file + record_frigate_file("Alice", "Alice-1000.webp", "asset-a") + record_frigate_file("Bob", "Bob-2000.webp", "asset-b") assert get_tracked_frigate_filenames("Alice") == {"Alice-1000.webp"} assert get_tracked_frigate_filenames("Bob") == {"Bob-2000.webp"} @@ -206,11 +198,12 @@ def test_get_lowest_quality_exclude_skips_specified_file(): from winnow.upload_tracker import ( get_lowest_quality_mapped_file, mark_uploaded, - record_frigate_files_batch, + record_frigate_file, ) mark_uploaded("asset-lo", person_name="Alice", score=0.10) mark_uploaded("asset-hi", person_name="Alice", score=0.90) - record_frigate_files_batch("Alice", {"Alice-lo.webp": "asset-lo", "Alice-hi.webp": "asset-hi"}) + record_frigate_file("Alice", "Alice-lo.webp", "asset-lo") + record_frigate_file("Alice", "Alice-hi.webp", "asset-hi") result = get_lowest_quality_mapped_file("Alice", exclude={"Alice-lo.webp"}) assert result is not None assert result[1] == "asset-hi" # lo was excluded; hi is returned @@ -220,28 +213,29 @@ def test_get_lowest_quality_exclude_all_returns_none(): from winnow.upload_tracker import ( get_lowest_quality_mapped_file, mark_uploaded, - record_frigate_files_batch, + record_frigate_file, ) mark_uploaded("asset-a", person_name="Alice", score=0.50) - record_frigate_files_batch("Alice", {"Alice-a.webp": "asset-a"}) + record_frigate_file("Alice", "Alice-a.webp", "asset-a") assert get_lowest_quality_mapped_file("Alice", exclude={"Alice-a.webp"}) is None # ── get_most_redundant_mapped_file ──────────────────────────────────────────── def test_get_most_redundant_none_when_no_frigate_scores(): - from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_files_batch + from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_file mark_uploaded("asset-a", person_name="Alice", score=0.80) - record_frigate_files_batch("Alice", {"Alice-a.webp": "asset-a"}) + record_frigate_file("Alice", "Alice-a.webp", "asset-a") # blur score only, no frigate_score → no candidates assert get_most_redundant_mapped_file("Alice") is None def test_get_most_redundant_returns_highest_frigate_score(): - from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_files_batch + from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_file mark_uploaded("asset-novel", person_name="Alice", score=0.50, frigate_score=0.31) mark_uploaded("asset-redundant", person_name="Alice", score=0.90, frigate_score=0.88) - record_frigate_files_batch("Alice", {"Alice-novel.webp": "asset-novel", "Alice-redundant.webp": "asset-redundant"}) + record_frigate_file("Alice", "Alice-novel.webp", "asset-novel") + record_frigate_file("Alice", "Alice-redundant.webp", "asset-redundant") result = get_most_redundant_mapped_file("Alice") assert result is not None frigate_filename, asset_id, score = result @@ -251,17 +245,18 @@ def test_get_most_redundant_returns_highest_frigate_score(): def test_get_most_redundant_exclude_skips_file(): - from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_files_batch + from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_file mark_uploaded("asset-hi", person_name="Alice", score=0.9, frigate_score=0.85) mark_uploaded("asset-lo", person_name="Alice", score=0.5, frigate_score=0.40) - record_frigate_files_batch("Alice", {"Alice-hi.webp": "asset-hi", "Alice-lo.webp": "asset-lo"}) + record_frigate_file("Alice", "Alice-hi.webp", "asset-hi") + record_frigate_file("Alice", "Alice-lo.webp", "asset-lo") result = get_most_redundant_mapped_file("Alice", exclude={"Alice-hi.webp"}) assert result is not None assert result[1] == "asset-lo" # hi excluded; lo is next highest def test_get_most_redundant_exclude_all_returns_none(): - from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_files_batch + from winnow.upload_tracker import get_most_redundant_mapped_file, mark_uploaded, record_frigate_file mark_uploaded("asset-a", person_name="Alice", score=0.5, frigate_score=0.70) - record_frigate_files_batch("Alice", {"Alice-a.webp": "asset-a"}) + record_frigate_file("Alice", "Alice-a.webp", "asset-a") assert get_most_redundant_mapped_file("Alice", exclude={"Alice-a.webp"}) is None diff --git a/uv.lock b/uv.lock index dde5cac..c7fd590 100644 --- a/uv.lock +++ b/uv.lock @@ -862,7 +862,7 @@ wheels = [ [[package]] name = "winnow" -version = "0.5.7" +version = "0.6.0" source = { editable = "." } dependencies = [ { name = "croniter" }, diff --git a/winnow/config.py b/winnow/config.py index 8bde0dc..37a99c7 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -152,8 +152,8 @@ class _Config: else: self.DATA_DIR = "data" - # Fall back to config file only when the env var is genuinely absent (None). - # An explicitly empty env var (IMMICH_URL="") takes priority over the file. + # Fall back to config file when the env var is absent or blank — a blank + # IMMICH_URL= placeholder in .env should not override the config file. # Prefer DATA_DIR/.immich_config.json (volume-safe in Docker) and fall back # to the legacy CWD path so existing installations continue to work. _data_cfg = Path(self.DATA_DIR) / ".immich_config.json" @@ -171,7 +171,7 @@ class _Config: if _data_cfg_exists or config_file.exists(): try: data = json.loads(config_file.read_text()) - if self.IMMICH_URL is None: + if not self.IMMICH_URL: self.IMMICH_URL = data.get("IMMICH_URL") if os.getenv("OUTPUT_DIR") is None: self.OUTPUT_DIR = data.get("OUTPUT_DIR", self.OUTPUT_DIR) diff --git a/winnow/executor.py b/winnow/executor.py index 0357b38..03c885a 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -6,6 +6,7 @@ import shutil from io import BytesIO from urllib.parse import quote +import PIL import requests from PIL import Image from rich import print as rprint @@ -162,10 +163,11 @@ def execute_jobs(jobs: list[dict]) -> None: if resp.ok: try: img = Image.open(BytesIO(resp.content)) - except Exception: - # resp.ok=True but content is unreadable — corrupt Immich - # thumbnail. Mark rejected so this asset isn't retried - # indefinitely on future runs. + except PIL.UnidentifiedImageError: + # Pillow cannot identify the format — genuinely corrupt + # Immich thumbnail. Mark rejected so this asset isn't + # retried indefinitely. OSError/truncation errors are + # transient and intentionally not caught here. logger.warning("Invalid image data for asset %s — marking rejected", asset["id"]) mark_rejected(asset["id"], person_name=name) img = None diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 22cf9d0..076a157 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -1,258 +1,146 @@ -"""Persistent tracker for Immich asset IDs uploaded/rejected by Frigate. +"""Persistent tracker for Immich asset IDs already uploaded/rejected by Frigate. -Uses a local SQLite database (frigate_tracker.db) in DATA_DIR. +Two separate JSON files in DATA_DIR: + frigate_uploaded_ids.json — successfully uploaded assets + frigate_rejected_ids.json — assets Frigate rejected (e.g. no face detected) -Schema ------- -tracked_assets — one row per (asset_id, status) pair -frigate_files — Frigate filename → Immich asset_id mapping -person_metadata — last-known Frigate training image count per person +Both are excluded from future candidate pools. To reset: + - All: delete both files + - One person: call reset_person("Name") or set RESET_PERSON=Name + - Rejects only: delete frigate_rejected_ids.json, or set RETRY_REJECTED=true -Migration ---------- -On first open, if the old JSON files exist and the tables are empty, their -data is migrated automatically. The JSON files are then renamed to .json.bak. +by_person schema (frigate_uploaded_ids.json): + { + "asset_ids": ["immich-id-1", ...], # all assets we attempted to upload + "scores": {"immich-id-1": 450.3}, # Laplacian blur variance at upload time + "frigate_scores": {"immich-id-1": 0.87}, # Frigate recognition confidence (0-1) pre-upload + "frigate_files": {"PersonName-123.webp": "immich-id-1"}, # Frigate filename → asset ID + "crop_dims": {"immich-id-1": [640, 480]}, # crop pixel dimensions at upload time + "frigate_count": 42 # last known Frigate training image count + } + +frigate_scores stores pre-upload recognize scores (0-1 sigmoid-mapped cosine +similarity). High score = the existing training set already covers this face +condition well. Low score = a gap — novel/diverse for the training set. + +frigate_files only contains files winnow uploaded — files added manually through +Frigate's UI are never mapped here and are never touched by quality replacement. """ import json import logging import os -import sqlite3 from pathlib import Path -from .frigate_api import _get_frigate_url, delete_frigate_person_files +from .frigate_api import delete_frigate_person_files logger = logging.getLogger(__name__) -# Legacy JSON filenames (for migration) -_UPLOAD_JSON = "frigate_uploaded_ids.json" -_REJECT_JSON = "frigate_rejected_ids.json" -_DB_NAME = "frigate_tracker.db" +UPLOAD_TRACKER_FILE = "frigate_uploaded_ids.json" +REJECT_TRACKER_FILE = "frigate_rejected_ids.json" -_DDL = """ -CREATE TABLE IF NOT EXISTS tracked_assets ( - 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) -); - -CREATE TABLE IF NOT EXISTS frigate_files ( - frigate_filename TEXT PRIMARY KEY, - person_name TEXT NOT NULL, - asset_id TEXT NOT NULL -); - -CREATE TABLE IF NOT EXISTS person_metadata ( - person_name TEXT PRIMARY KEY, - frigate_count INTEGER -); -""" - -# Module-level connection state — re-opened when DATA_DIR changes (test isolation) -_conn: sqlite3.Connection | None = None -_conn_path: str | None = None +# Write-through in-memory cache keyed by the resolved file path. +# Reduces per-call JSON reads from O(calls) to O(1) after the first load. +# Keyed by full path so tests with isolated tmp dirs never share entries. +_cache: dict[str, dict] = {} -def _migrate_schema_v2(conn: sqlite3.Connection) -> None: - """Migrate tracked_assets from PRIMARY KEY (asset_id, status) to (asset_id, person_name, status). - - The old PK meant INSERT OR REPLACE for person B on an asset already tracked for - person A would silently overwrite person_name, breaking quality-replacement JOINs - for person A. The new PK gives each (asset, person) pair its own row. - - SQLite does not support ALTER TABLE to change a primary key; we recreate the table. - """ - pk_cols = { - r[1] - for r in conn.execute("PRAGMA table_info(tracked_assets)").fetchall() - if r[5] > 0 # column index 5 = pk position (0 = not in PK) - } - if "person_name" in pk_cols: - return # Already at new schema - - logger.info("Migrating tracked_assets: adding person_name to primary key") - # 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") - - -def _get_conn() -> sqlite3.Connection: - """Return (or create) the module-level SQLite connection. - - Re-opens the connection when Config.DATA_DIR has changed — this provides - test isolation when the isolated_cache fixture sets a new tmp directory and - calls _Config.reset(). - """ - global _conn, _conn_path - - from .config import Config - data_dir = Config.DATA_DIR - db_path = str(Path(data_dir) / _DB_NAME) - - if _conn is not None and _conn_path != db_path: - try: - _conn.close() - except Exception as e: - logger.debug("Failed to close previous SQLite connection: %s", e) - _conn = None - - if _conn is None: - Path(data_dir).mkdir(parents=True, exist_ok=True) - _conn = sqlite3.connect(db_path, check_same_thread=False, timeout=30) - _conn.row_factory = sqlite3.Row - _conn.execute("PRAGMA journal_mode=WAL") - _conn.execute("PRAGMA foreign_keys=ON") - _conn.executescript(_DDL) - _conn.commit() - _conn_path = db_path - _migrate_schema_v2(_conn) - _maybe_migrate(data_dir, _conn) - - return _conn - - -# --------------------------------------------------------------------------- -# JSON → SQLite migration -# --------------------------------------------------------------------------- - -def _maybe_migrate(data_dir: str, conn: sqlite3.Connection) -> None: - """If the old JSON files exist and DB is empty, migrate and rename them.""" - base = Path(data_dir) - upload_json = base / _UPLOAD_JSON - reject_json = base / _REJECT_JSON - - if not upload_json.exists() and not reject_json.exists(): - return - - # No row-count guard here: INSERT OR IGNORE makes migration idempotent, so it - # is safe to re-run if a previous attempt renamed one file but not the other - # (e.g. a PermissionError on the second rename would have left the first file's - # data committed but the second file un-renamed and un-migrated). - - logger.info("Migrating JSON tracker files to SQLite in %s", data_dir) - +def _tracker_path(filename: str) -> Path: try: - with conn: - if upload_json.exists(): - _migrate_json_data(conn, json.loads(upload_json.read_text()), "uploaded") - if reject_json.exists(): - _migrate_json_data(conn, json.loads(reject_json.read_text()), "rejected") - except Exception as exc: - logger.warning("JSON migration failed, will retry next run: %s", exc) - return - - # Rename each file independently so a failure on one does not prevent the - # other from being marked complete on this run. - for json_path in (upload_json, reject_json): - if json_path.exists(): - try: - json_path.rename(json_path.with_suffix(".json.bak")) - except OSError as exc: - logger.warning("Could not rename %s after migration: %s", json_path, exc) - - logger.info("JSON → SQLite migration complete") + from .config import Config + return Path(Config.DATA_DIR) / filename + except (ImportError, AttributeError): + return Path(filename) -def _migrate_json_data(conn: sqlite3.Connection, data: dict, status: str) -> None: - """Insert one JSON tracker file's data into SQLite tables.""" - flat_key = "uploaded_asset_ids" if status == "uploaded" else "rejected_asset_ids" - flat_ids: set[str] = set(data.get(flat_key, [])) - person_covered: set[str] = set() - - for person_name, raw_entry in data.get("by_person", {}).items(): - if isinstance(raw_entry, list): - entry: dict = {"asset_ids": raw_entry, "scores": {}, "frigate_scores": {}, - "frigate_files": {}, "crop_dims": {}} - else: - entry = { - "asset_ids": raw_entry.get("asset_ids", []), - "scores": raw_entry.get("scores", {}), - "frigate_scores": raw_entry.get("frigate_scores", {}), - "frigate_files": raw_entry.get("frigate_files", {}), - "crop_dims": raw_entry.get("crop_dims", {}), - "frigate_count": raw_entry.get("frigate_count"), - } - - for asset_id in entry["asset_ids"]: - person_covered.add(asset_id) - dims = entry.get("crop_dims", {}).get(asset_id) - conn.execute( - """INSERT OR IGNORE INTO tracked_assets - (asset_id, person_name, status, blur_score, - crop_width, crop_height, frigate_score) - VALUES (?, ?, ?, ?, ?, ?, ?)""", - ( - asset_id, - person_name, - status, - entry.get("scores", {}).get(asset_id), - dims[0] if dims else None, - dims[1] if dims else None, - entry.get("frigate_scores", {}).get(asset_id) if status == "uploaded" else None, - ), - ) - - if status == "uploaded": - for ff, aid in entry.get("frigate_files", {}).items(): - conn.execute( - "INSERT OR IGNORE INTO frigate_files (frigate_filename, person_name, asset_id) VALUES (?, ?, ?)", - (ff, person_name, aid), - ) - fc = entry.get("frigate_count") - if fc is not None: - conn.execute( - "INSERT OR REPLACE INTO person_metadata (person_name, frigate_count) VALUES (?, ?)", - (person_name, fc), - ) - - # Flat IDs not covered by any by_person entry → insert with NULL person - for asset_id in flat_ids - person_covered: - conn.execute( - "INSERT OR IGNORE INTO tracked_assets (asset_id, person_name, status) VALUES (?, NULL, ?)", - (asset_id, status), - ) +def _load(filename: str) -> dict: + path = _tracker_path(filename) + key = str(path) + if key in _cache: + return _cache[key] + data: dict = {} + if path.exists(): + try: + with open(path) as f: + data = json.load(f) + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Could not load tracker {filename}: {e}") + _cache[key] = data + return data -# --------------------------------------------------------------------------- -# Public API -# --------------------------------------------------------------------------- +def _save(filename: str, data: dict) -> None: + path = _tracker_path(filename) + _cache[str(path)] = data # keep cache consistent with what we write + path.parent.mkdir(parents=True, exist_ok=True) + with open(path, "w") as f: + json.dump(data, f, indent=2) + + +def _flat_key(filename: str) -> str: + return "uploaded_asset_ids" if "uploaded" in filename else "rejected_asset_ids" + + +def _load_flat(filename: str) -> set[str]: + return set(_load(filename).get(_flat_key(filename), [])) + + +def _get_ids(entry: list | dict) -> list[str]: + """Extract asset_ids from either the old list format or the new dict format.""" + if isinstance(entry, list): + return entry + return entry.get("asset_ids", []) + + +def _migrate_entry(entry: list | dict) -> dict: + """Ensure by_person entry is in the current dict format.""" + if isinstance(entry, list): + return {"asset_ids": sorted(entry), "scores": {}, "frigate_scores": {}, "frigate_files": {}, "crop_dims": {}} + entry.setdefault("asset_ids", []) + entry.setdefault("scores", {}) + entry.setdefault("frigate_scores", {}) + entry.setdefault("frigate_files", {}) + entry.setdefault("crop_dims", {}) + return entry + + +def _mark( + filename: str, + asset_id: str, + person_name: str | None, + score: float | None = None, + crop_dims: tuple[int, int] | None = None, + frigate_score: float | None = None, +) -> None: + data = _load(filename) + flat_key = _flat_key(filename) + flat = set(data.get(flat_key, [])) + flat.add(asset_id) + data[flat_key] = sorted(flat) + if person_name: + by_person = data.setdefault("by_person", {}) + entry = _migrate_entry(by_person.get(person_name, {})) + ids = set(entry["asset_ids"]) + ids.add(asset_id) + entry["asset_ids"] = sorted(ids) + if score is not None: + entry["scores"][asset_id] = round(score, 4) + if crop_dims is not None: + entry["crop_dims"][asset_id] = [crop_dims[0], crop_dims[1]] + if frigate_score is not None: + entry["frigate_scores"][asset_id] = round(frigate_score, 4) + by_person[person_name] = entry + _save(filename, data) + + +# ── Public API ──────────────────────────────────────────────────────────────── def load_uploaded_ids() -> set[str]: - conn = _get_conn() - rows = conn.execute("SELECT asset_id FROM tracked_assets WHERE status='uploaded'").fetchall() - return {r[0] for r in rows} + return _load_flat(UPLOAD_TRACKER_FILE) def load_rejected_ids() -> set[str]: - conn = _get_conn() - rows = conn.execute("SELECT asset_id FROM tracked_assets WHERE status='rejected'").fetchall() - return {r[0] for r in rows} + return _load_flat(REJECT_TRACKER_FILE) def mark_uploaded( @@ -262,264 +150,223 @@ def mark_uploaded( crop_dims: tuple[int, int] | None = None, frigate_score: float | None = None, ) -> None: - conn = _get_conn() - with conn: - conn.execute( - """INSERT OR REPLACE INTO tracked_assets - (asset_id, person_name, status, blur_score, crop_width, crop_height, frigate_score) - VALUES (?, ?, 'uploaded', ?, ?, ?, ?)""", - ( - asset_id, - person_name, - round(score, 4) if score is not None else None, - crop_dims[0] if crop_dims else None, - crop_dims[1] if crop_dims else None, - round(frigate_score, 4) if frigate_score is not None else None, - ), - ) - logger.debug("Marked %s as uploaded (%s)", asset_id, person_name) + _mark(UPLOAD_TRACKER_FILE, asset_id, person_name, score=score, crop_dims=crop_dims, frigate_score=frigate_score) + logger.debug(f"Marked {asset_id} as uploaded ({person_name})") def mark_rejected(asset_id: str, person_name: str | None = None) -> None: - conn = _get_conn() - with conn: - conn.execute( - "INSERT OR IGNORE INTO tracked_assets (asset_id, person_name, status) VALUES (?, ?, 'rejected')", - (asset_id, person_name), - ) - logger.debug("Marked %s as rejected (%s)", asset_id, person_name) + _mark(REJECT_TRACKER_FILE, asset_id, person_name) + logger.debug(f"Marked {asset_id} as rejected ({person_name})") + + + +def record_frigate_file(person_name: str, frigate_filename: str, asset_id: str) -> None: + """Record the mapping from a Frigate training filename to an Immich asset ID.""" + data = _load(UPLOAD_TRACKER_FILE) + by_person = data.setdefault("by_person", {}) + entry = _migrate_entry(by_person.get(person_name, {})) + entry["frigate_files"][frigate_filename] = asset_id + by_person[person_name] = entry + _save(UPLOAD_TRACKER_FILE, data) + logger.debug(f"Mapped Frigate file {frigate_filename} → {asset_id} ({person_name})") def record_frigate_files_batch(person_name: str, mappings: dict[str, str]) -> None: - """Record multiple Frigate filename → asset_id mappings in a single transaction. - - All rows are written atomically — either every mapping is persisted or none - are (SQLite rolls back on error), so there is no risk of partial failure. - """ + """Record multiple Frigate filename → asset_id mappings in a single load/save.""" if not mappings: return - conn = _get_conn() - with conn: - conn.executemany( - "INSERT OR REPLACE INTO frigate_files (frigate_filename, person_name, asset_id) VALUES (?, ?, ?)", - [(ff, person_name, aid) for ff, aid in mappings.items()], - ) - logger.debug("Batch-mapped %s Frigate file(s) for %s", len(mappings), person_name) + data = _load(UPLOAD_TRACKER_FILE) + by_person = data.setdefault("by_person", {}) + entry = _migrate_entry(by_person.get(person_name, {})) + entry["frigate_files"].update(mappings) + by_person[person_name] = entry + _save(UPLOAD_TRACKER_FILE, data) + logger.debug(f"Batch-mapped {len(mappings)} Frigate file(s) for {person_name}") def remove_frigate_file(person_name: str, frigate_filename: str) -> None: - """Remove a Frigate filename mapping and clear its asset's frigate_score. + """Remove a Frigate filename from the mapping after it has been deleted. - Does NOT unmark the source asset_id — the deletion was deliberate. + Does NOT unmark the source asset_id — the deletion was deliberate and + we don't want to re-upload the inferior image on the next run. """ - conn = _get_conn() - with conn: - row = conn.execute( - "SELECT asset_id FROM frigate_files WHERE frigate_filename=? AND person_name=?", - (frigate_filename, person_name), - ).fetchone() - conn.execute( - "DELETE FROM frigate_files WHERE frigate_filename=? AND person_name=?", - (frigate_filename, person_name), - ) - if row: - conn.execute( - "UPDATE tracked_assets SET frigate_score=NULL WHERE asset_id=? AND person_name=?", - (row["asset_id"], person_name), - ) - logger.debug("Removed Frigate file mapping %s (%s)", frigate_filename, person_name) + data = _load(UPLOAD_TRACKER_FILE) + by_person = data.get("by_person", {}) + entry = _migrate_entry(by_person.get(person_name, {})) + asset_id = entry["frigate_files"].pop(frigate_filename, None) + if asset_id: + entry["frigate_scores"].pop(asset_id, None) + by_person[person_name] = entry + _save(UPLOAD_TRACKER_FILE, data) + logger.debug(f"Removed Frigate file mapping {frigate_filename} ({person_name})") def get_tracked_frigate_file_count(person_name: str) -> int: - """Return the number of Frigate training files winnow has mapped for this person.""" - conn = _get_conn() - row = conn.execute( - "SELECT COUNT(*) FROM frigate_files WHERE person_name=?", (person_name,) - ).fetchone() - return row[0] + """Return the number of Frigate training files winnow has mapped for this person. + + Used as the cap baseline so that manually-added Frigate files do not + consume slots from winnow's managed quota. + """ + data = _load(UPLOAD_TRACKER_FILE) + entry = _migrate_entry(data.get("by_person", {}).get(person_name, {})) + return len(entry["frigate_files"]) def get_tracked_frigate_filenames(person_name: str) -> set[str]: - """Return the set of Frigate filenames currently mapped for a person.""" - conn = _get_conn() - rows = conn.execute( - "SELECT frigate_filename FROM frigate_files WHERE person_name=?", (person_name,) - ).fetchall() - return {r[0] for r in rows} + """Return the set of Frigate filenames currently mapped in the tracker for a person. + + Used as a pre-upload baseline when the Frigate GET API is unreachable at + upload start, so reconciliation can still identify newly uploaded files. + """ + data = _load(UPLOAD_TRACKER_FILE) + entry = _migrate_entry(data.get("by_person", {}).get(person_name, {})) + return set(entry["frigate_files"].keys()) def has_frigate_scores(person_name: str) -> bool: """Return True if any mapped file for this person has a stored Frigate recognition score.""" - conn = _get_conn() - row = conn.execute( - """SELECT COUNT(*) FROM frigate_files ff - JOIN tracked_assets ta ON ta.asset_id=ff.asset_id AND ta.person_name=ff.person_name - WHERE ff.person_name=? AND ta.frigate_score IS NOT NULL""", - (person_name,), - ).fetchone() - return row[0] > 0 - - -# Guard against SQL injection from callers passing dynamic column names. -# Only these two columns exist and are safe to interpolate into queries. -_VALID_SCORE_COLS = frozenset({"blur_score", "frigate_score"}) + data = _load(UPLOAD_TRACKER_FILE) + entry = _migrate_entry(data.get("by_person", {}).get(person_name, {})) + frigate_files = entry.get("frigate_files", {}) + frigate_scores = entry.get("frigate_scores", {}) + return any(asset_id in frigate_scores for asset_id in frigate_files.values()) def _pick_mapped_file( - person_name: str, score_col: str, *, highest: bool, exclude: set[str] | None = None + person_name: str, score_key: str, *, highest: bool, exclude: set[str] | None = None ) -> tuple[str, str, float] | None: - if score_col not in _VALID_SCORE_COLS: - raise ValueError(f"Invalid score column: {score_col!r}") - conn = _get_conn() - order = "DESC" if highest else "ASC" - rows = conn.execute( - f"""SELECT ff.frigate_filename, ff.asset_id, ta.{score_col} - FROM frigate_files ff - JOIN tracked_assets ta ON ta.asset_id=ff.asset_id AND ta.person_name=ff.person_name - WHERE ff.person_name=? AND ta.{score_col} IS NOT NULL - ORDER BY ta.{score_col} {order}""", - (person_name,), - ).fetchall() - for row in rows: - if exclude is None or row[0] not in exclude: - return (row[0], row[1], row[2]) - return None + data = _load(UPLOAD_TRACKER_FILE) + entry = _migrate_entry(data.get("by_person", {}).get(person_name, {})) + scores = entry.get(score_key, {}) + candidates = [ + (ff, asset_id, scores[asset_id]) + for ff, asset_id in entry.get("frigate_files", {}).items() + if (exclude is None or ff not in exclude) and asset_id in scores + ] + if not candidates: + return None + return max(candidates, key=lambda x: x[2]) if highest else min(candidates, key=lambda x: x[2]) def get_lowest_quality_mapped_file( person_name: str, exclude: set[str] | None = None ) -> tuple[str, str, float] | None: - """Return (frigate_filename, asset_id, score) for the mapped file with the lowest blur score.""" - return _pick_mapped_file(person_name, "blur_score", highest=False, exclude=exclude) + """Return (frigate_filename, asset_id, score) for the mapped file with the lowest + blur score, or None if no mapped files with known scores exist. + + Used for quality replacement when no Frigate scores are available. + Pass `exclude` to skip files that failed to delete this run. + """ + return _pick_mapped_file(person_name, "scores", highest=False, exclude=exclude) def get_most_redundant_mapped_file( person_name: str, exclude: set[str] | None = None ) -> tuple[str, str, float] | None: - """Return (frigate_filename, asset_id, score) for the mapped file with the highest Frigate score.""" - return _pick_mapped_file(person_name, "frigate_score", highest=True, exclude=exclude) + """Return (frigate_filename, asset_id, score) for the mapped file with the highest + Frigate recognition score, or None if no mapped files with Frigate scores exist. - -def get_frigate_filename_for_asset(person_name: str, asset_id: str) -> str | None: - """Return the Frigate training filename mapped to this asset ID, or None.""" - conn = _get_conn() - row = conn.execute( - "SELECT frigate_filename FROM frigate_files WHERE person_name=? AND asset_id=?", - (person_name, asset_id), - ).fetchone() - return row[0] if row else None + High Frigate score = the training set already covers this face condition well + = the most redundant file and therefore the best replacement target. + Pass `exclude` to skip files that failed to delete this run. + """ + return _pick_mapped_file(person_name, "frigate_scores", highest=True, exclude=exclude) def find_by_crop_dimension(size: int) -> list[dict]: """Return all tracked crops whose width or height matches `size` pixels. - Returns a list of dicts: {person, asset_id, width, height, blur_score, frigate_score, frigate_filename}. + Returns a list of dicts: {person, asset_id, width, height, blur_score, frigate_filename}. + frigate_filename is None when the Frigate mapping was lost to a reconciliation race. """ - conn = _get_conn() - rows = conn.execute( - """SELECT ta.person_name, ta.asset_id, ta.crop_width, ta.crop_height, - ta.blur_score, ta.frigate_score, ff.frigate_filename - FROM tracked_assets ta - LEFT JOIN frigate_files ff ON ff.asset_id=ta.asset_id AND ff.person_name=ta.person_name - WHERE ta.status='uploaded' AND (ta.crop_width=? OR ta.crop_height=?)""", - (size, size), - ).fetchall() - return [ - { - "person": r["person_name"], - "asset_id": r["asset_id"], - "width": r["crop_width"], - "height": r["crop_height"], - "blur_score": r["blur_score"], - "frigate_score": r["frigate_score"], - "frigate_filename": r["frigate_filename"], - } - for r in rows - ] + data = _load(UPLOAD_TRACKER_FILE) + results = [] + for person_name, raw_entry in data.get("by_person", {}).items(): + entry = _migrate_entry(raw_entry) + scores = entry.get("scores", {}) + frigate_files = entry.get("frigate_files", {}) + asset_to_frigate = {v: k for k, v in frigate_files.items()} + frigate_scores = entry.get("frigate_scores", {}) + for asset_id, dims in entry.get("crop_dims", {}).items(): + w, h = dims[0], dims[1] + if w == size or h == size: + results.append({ + "person": person_name, + "asset_id": asset_id, + "width": w, + "height": h, + "blur_score": scores.get(asset_id), + "frigate_score": frigate_scores.get(asset_id), + "frigate_filename": asset_to_frigate.get(asset_id), + }) + return results def update_frigate_count(person_name: str, count: int) -> None: """Record Frigate's authoritative training image count for a person.""" - conn = _get_conn() - with conn: - conn.execute( - "INSERT OR REPLACE INTO person_metadata (person_name, frigate_count) VALUES (?, ?)", - (person_name, count), - ) + data = _load(UPLOAD_TRACKER_FILE) + by_person = data.setdefault("by_person", {}) + entry = _migrate_entry(by_person.get(person_name, {})) + entry["frigate_count"] = count + by_person[person_name] = entry + _save(UPLOAD_TRACKER_FILE, data) def reset_person(person_name: str) -> None: """Remove all uploaded and rejected records for a given person. Also deletes winnow-managed Frigate training files so the next run starts - clean rather than uploading on top of orphaned files. + clean rather than uploading on top of orphaned files. Manually-added Frigate + files (not in frigate_files) are never touched. Proceeds with tracker reset + even if Frigate is unreachable. """ - conn = _get_conn() - - # Collect Frigate filenames before deleting - frigate_filenames = list(get_tracked_frigate_filenames(person_name)) + upload_data = _load(UPLOAD_TRACKER_FILE) + entry = _migrate_entry(upload_data.get("by_person", {}).get(person_name, {})) + frigate_filenames = list(entry.get("frigate_files", {}).keys()) if frigate_filenames: - if not _get_frigate_url(): - logger.info("FRIGATE_URL not set — skipping Frigate file deletion for %s", person_name) + if not os.environ.get("FRIGATE_URL", "").strip(): + logger.info(f"FRIGATE_URL not set — skipping Frigate file deletion for {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) + logger.info(f"Deleted {len(frigate_filenames)} Frigate file(s) for {person_name}") else: - logger.warning( - "Could not delete Frigate files for %s — tracker reset proceeding anyway", person_name - ) + logger.warning(f"Could not delete Frigate files for {person_name} — tracker reset proceeding anyway") - with conn: - conn.execute("DELETE FROM frigate_files WHERE person_name=?", (person_name,)) - conn.execute("DELETE FROM tracked_assets WHERE person_name=?", (person_name,)) - conn.execute("DELETE FROM person_metadata WHERE person_name=?", (person_name,)) - - logger.info("Reset tracking data for %s", person_name) + changed = False + tracker_files = ((UPLOAD_TRACKER_FILE, upload_data), (REJECT_TRACKER_FILE, _load(REJECT_TRACKER_FILE))) + for filename, data in tracker_files: + flat_key = _flat_key(filename) + by_person = data.get("by_person", {}) + tracker_entry = by_person.pop(person_name, None) + if tracker_entry is not None: + person_ids = set(_get_ids(tracker_entry)) + flat = set(data.get(flat_key, [])) - person_ids + data[flat_key] = sorted(flat) + data["by_person"] = by_person + _save(filename, data) + changed = True + if changed: + logger.info(f"Reset tracking data for {person_name}") + else: + logger.debug(f"reset_person: no tracking data found for {person_name}") def get_person_summary() -> dict[str, dict]: """Return {person_name: {uploaded, rejected, frigate_count, scores, frigate_files}} for display/capacity.""" - conn = _get_conn() - - # Counts per person per status - rows = conn.execute( - """SELECT person_name, status, COUNT(*) AS cnt - FROM tracked_assets WHERE person_name IS NOT NULL - GROUP BY person_name, status""" - ).fetchall() - - def _entry(summary: dict, name: str) -> dict: - if name not in summary: - summary[name] = {"uploaded": 0, "rejected": 0, "frigate_count": None, "scores": {}, "frigate_files": {}} - return summary[name] - - summary: dict[str, dict] = {} - for r in rows: - _entry(summary, r["person_name"])[r["status"]] = r["cnt"] - - # Scores for uploaded assets - score_rows = conn.execute( - """SELECT person_name, asset_id, blur_score - FROM tracked_assets - WHERE status='uploaded' AND person_name IS NOT NULL AND blur_score IS NOT NULL""" - ).fetchall() - for r in score_rows: - _entry(summary, r["person_name"])["scores"][r["asset_id"]] = r["blur_score"] - - # Frigate file mappings - ff_rows = conn.execute( - "SELECT person_name, frigate_filename, asset_id FROM frigate_files" - ).fetchall() - for r in ff_rows: - _entry(summary, r["person_name"])["frigate_files"][r["frigate_filename"]] = r["asset_id"] - - # Frigate counts - meta_rows = conn.execute( - "SELECT person_name, frigate_count FROM person_metadata" - ).fetchall() - for r in meta_rows: - _entry(summary, r["person_name"])["frigate_count"] = r["frigate_count"] - - return dict(sorted(summary.items())) + uploaded_data = _load(UPLOAD_TRACKER_FILE).get("by_person", {}) + rejected_data = _load(REJECT_TRACKER_FILE).get("by_person", {}) + names = set(uploaded_data) | set(rejected_data) + result = {} + for name in sorted(names): + u_entry = uploaded_data.get(name, {}) + r_entry = rejected_data.get(name, {}) + result[name] = { + "uploaded": len(_get_ids(u_entry)), + "rejected": len(_get_ids(r_entry)), + "frigate_count": u_entry.get("frigate_count") if isinstance(u_entry, dict) else None, + "scores": u_entry.get("scores", {}) if isinstance(u_entry, dict) else {}, + "frigate_files": u_entry.get("frigate_files", {}) if isinstance(u_entry, dict) else {}, + } + return result def filter_already_uploaded( @@ -533,5 +380,5 @@ def filter_already_uploaded( new_ids = [aid for aid in asset_ids if aid not in exclude] skipped = len(asset_ids) - len(new_ids) if skipped: - logger.info("Skipping %s assets already uploaded or rejected by Frigate", skipped) + logger.info(f"Skipping {skipped} assets already uploaded or rejected by Frigate") return new_ids