diff --git a/CHANGELOG.md b/CHANGELOG.md index 209c208..be8490f 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.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 diff --git a/pyproject.toml b/pyproject.toml index dfb00c6..48446ab 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.6" +version = "0.5.7" 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/uv.lock b/uv.lock index f301810..dde5cac 100644 --- a/uv.lock +++ b/uv.lock @@ -862,7 +862,7 @@ wheels = [ [[package]] name = "winnow" -version = "0.5.6" +version = "0.5.7" source = { editable = "." } dependencies = [ { name = "croniter" }, diff --git a/winnow/config.py b/winnow/config.py index c65f5dd..bc8abb0 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -123,14 +123,15 @@ class _Config: # 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" - if _data_cfg.exists() and _LEGACY_CONFIG_FILE.exists(): + _data_cfg_exists = _data_cfg.exists() + if _data_cfg_exists and _LEGACY_CONFIG_FILE.exists(): logging.warning( "Two config files found: %s and %s — using %s. Remove the legacy file to silence this.", _data_cfg, _LEGACY_CONFIG_FILE, _data_cfg, ) - config_file = _data_cfg if _data_cfg.exists() else _LEGACY_CONFIG_FILE + config_file = _data_cfg if _data_cfg_exists else _LEGACY_CONFIG_FILE if config_file.exists(): try: data = json.loads(config_file.read_text()) diff --git a/winnow/executor.py b/winnow/executor.py index ef77331..965d4ec 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -43,8 +43,13 @@ def _safe_person_dir(output_dir: str, person_name: str) -> str: 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. """ - candidate = os.path.realpath(os.path.join(output_dir, person_name)) + raw = os.path.join(output_dir, person_name) + if os.path.islink(raw): + raise ValueError(f"Person name {person_name!r} resolves to a symlink — skipping") + candidate = os.path.realpath(raw) base = os.path.realpath(output_dir) # Use the base path as its own prefix when it's the filesystem root ("/"), # otherwise append os.sep — avoids the false "//" double-slash when base == "/". @@ -100,9 +105,6 @@ def execute_jobs(jobs: list[dict]) -> None: logger.error(str(e)) continue # Face crops are transient (uploaded then discarded); wipe before each run. - if os.path.islink(person_dir): - logger.error("person_dir %s is a symlink — refusing to remove", person_dir) - continue 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 97dd0e9..9e38358 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -83,19 +83,25 @@ def merge_people(survivor_id: str, merge_ids: list[str]) -> bool: return False -def fetch_all_assets(person: dict) -> list[dict]: - """Fetch all assets for a person with pagination.""" +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. + """ name = person.get("name", "Unknown") person_id = person.get("id") if not person_id: logger.error("Person dict missing 'id' field for %s — skipping asset fetch", name) - return [] + return [], 0 url = f"{Config.IMMICH_URL}/api/search/metadata" page_size = 1000 logger.debug("Fetching assets for %s...", name) - assets = [] + assets: list[dict] = [] + total_raw = 0 # items seen across all pages before non-dict filtering for page in range(1, MAX_PAGES + 1): try: resp = requests.post( @@ -116,6 +122,7 @@ def fetch_all_assets(person: dict) -> list[dict]: 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 @@ -128,6 +135,11 @@ def fetch_all_assets(person: dict) -> list[dict]: logger.warning("%s: skipping %s non-dict item(s) in page %s", name, skipped_count, page) if not valid_assets: + if page_count > 0: + logger.warning( + "%s: page %s returned %s item(s) but none were valid dicts — stopping pagination", + name, page, page_count, + ) break assets.extend(valid_assets) @@ -140,7 +152,7 @@ def fetch_all_assets(person: dict) -> list[dict]: logger.error("Exception fetching assets for %s (page %s): %s", name, page, e) break - return assets + return assets, total_raw def fetch_face_data(asset_id: str, person_id: str | None = None) -> FaceData | None: diff --git a/winnow/jobs.py b/winnow/jobs.py index e9810d3..594ba13 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -162,10 +162,10 @@ def _configure_person(person: dict, people: list[dict]) -> dict | None: console.print(f"Scanning for {name}...") with console.status("[bold green]Fetching assets...[/bold green]"): - all_assets = fetch_all_assets(person) + all_assets, total_raw = fetch_all_assets(person) recent_assets = filter_recent_assets(all_assets, years=years) - rprint(f" Found [bold]{len(all_assets)}[/bold] total, [bold]{len(recent_assets)}[/bold] in range ({years} years).") + 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") @@ -262,16 +262,18 @@ def auto_configure(people: list[dict]) -> list[dict]: for person in valid_people: name = person["name"] - all_assets = fetch_all_assets(person) + all_assets, total_raw = fetch_all_assets(person) recent_assets = filter_recent_assets(all_assets, years=Config.YEARS_FILTER) - rprint(f" {name}: {len(all_assets)} total, {len(recent_assets)} recent") + 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. # 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 len(all_assets) < min_face_count: - rprint(f" [dim]Skipping {name} ({len(all_assets)} assets < MIN_FACE_COUNT={min_face_count}).[/dim]") + if min_face_count > 0 and total_raw < min_face_count: + rprint(f" [dim]Skipping {name} ({total_raw} assets < MIN_FACE_COUNT={min_face_count}).[/dim]") continue # Enforce MAX_AUTO_IMAGES against the tracked file count only.