From cabdb9c0ebd7dea2eb3c9abc9c07990eca08d88d Mon Sep 17 00:00:00 2001 From: Holden Date: Sun, 14 Jun 2026 20:37:51 +0000 Subject: [PATCH] fix: address 9 code review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - upload_tracker: partial migration now rolls back atomically on failure; JSON renamed only after successful commit so failed runs retry cleanly - upload_tracker: allowlist score_col in _pick_mapped_file to close latent SQL injection surface - config: move load_dotenv() from module import into _load() so no I/O at import time and reset() fully resets env loading - config: use is None checks for IMMICH_URL/OUTPUT_DIR config-file fallback so explicitly empty env vars are not overridden by the file - executor: skip reconcile when Frigate API is unreachable at upload start — tracker baseline is incomplete and would mis-trigger the external-upload guard, permanently losing file mappings - reconcile: change polling break condition from >= to == target so transient overshoots don't prematurely exit the loop and trigger the external-upload guard - jobs: apply capacity cap as the selection limit rather than truncating post-selection by position, so the diversity algorithm works within the right budget from the start - Dockerfile: explicit gpu branch + exit 1 on unknown VARIANT instead of silent fallback --- Dockerfile | 5 ++++- winnow/config.py | 11 ++++++----- winnow/executor.py | 19 ++++++++++++------- winnow/jobs.py | 11 ++++------- winnow/reconcile.py | 2 +- winnow/upload_tracker.py | 34 +++++++++++++++++++--------------- 6 files changed, 46 insertions(+), 36 deletions(-) diff --git a/Dockerfile b/Dockerfile index 7d44dd0..dd01840 100644 --- a/Dockerfile +++ b/Dockerfile @@ -48,8 +48,11 @@ RUN if [ "$VARIANT" = "cpu" ]; then \ uv sync --frozen --no-dev --extra rocm; \ elif [ "$VARIANT" = "intel" ]; then \ uv sync --frozen --no-dev --extra intel; \ - else \ + elif [ "$VARIANT" = "gpu" ]; then \ uv sync --frozen --no-dev --extra gpu; \ + else \ + echo "Unknown VARIANT: '$VARIANT'. Must be one of: cpu, rocm, intel, gpu" >&2; \ + exit 1; \ fi && \ uv cache clean diff --git a/winnow/config.py b/winnow/config.py index 357e49b..85c8549 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -9,8 +9,6 @@ from typing import ClassVar from dotenv import load_dotenv from rich.prompt import Prompt -load_dotenv() - CONFIG_FILE = Path(".immich_config.json") @@ -81,6 +79,7 @@ class _Config: def _load(self) -> None: """Load configuration from environment and config file.""" + load_dotenv() # Load from environment (highest priority) self.IMMICH_URL = os.getenv("IMMICH_URL") self.API_KEY = os.getenv("API_KEY") @@ -102,12 +101,14 @@ class _Config: self.ENABLE_CACHE = os.getenv("ENABLE_CACHE", "true").lower() in ("true", "1", "yes") self.CACHE_DIR = os.getenv("CACHE_DIR", ".if_cache") - # Fall back to config file for non-sensitive values (API_KEY not stored here) + # 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. if CONFIG_FILE.exists(): try: data = json.loads(CONFIG_FILE.read_text()) - self.IMMICH_URL = self.IMMICH_URL or data.get("IMMICH_URL") - if not os.getenv("OUTPUT_DIR"): + if self.IMMICH_URL is None: + self.IMMICH_URL = data.get("IMMICH_URL") + if os.getenv("OUTPUT_DIR") is None: self.OUTPUT_DIR = data.get("OUTPUT_DIR", self.OUTPUT_DIR) except (json.JSONDecodeError, OSError) as e: logging.warning("Failed to load config file: %s", e) diff --git a/winnow/executor.py b/winnow/executor.py index a7b24ff..55eefb1 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -299,16 +299,21 @@ def upload_to_frigate(jobs: list[dict]) -> None: else get_frigate_person_files(name) ) if _snapshot is None: - # Frigate GET is down; fall back to the tracker's mapped filenames - # as the pre-upload baseline. reconciliation will still work unless - # there are concurrent manual uploads (handled by >target guard). + # Frigate GET is down. The tracker only knows files winnow mapped + # previously — it is blind to manually-added Frigate files. Using + # the tracker as the baseline would make those unmapped files look + # like new uploads in reconcile, triggering the >target guard and + # silently dropping all mappings. Skip reconciliation entirely when + # we can't get a reliable live snapshot. logger.warning( - f"{name}: Frigate API unreachable at upload start" - " — using tracker baseline for post-upload reconciliation" + "%s: Frigate API unreachable at upload start" + " — file mapping will be skipped for this batch", name ) - known_frigate_files_at_start: set[str] = get_tracked_frigate_filenames(name) + known_frigate_files_at_start: set[str] = set() + _skip_reconcile = True else: known_frigate_files_at_start: set[str] = set(_snapshot) + _skip_reconcile = False # Remove tracker mappings for files that no longer exist in Frigate # (manually deleted, or cleaned up outside winnow). This corrects the # effective_count so those slots are available for new uploads. @@ -548,7 +553,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: ) # Batch-map Frigate filenames to asset IDs now that all uploads are done. - if actually_uploaded: + if actually_uploaded and not _skip_reconcile: reconcile_frigate_mappings(name, known_frigate_files_at_start, actually_uploaded) # Per-person summary diff --git a/winnow/jobs.py b/winnow/jobs.py index 8e332fc..bd5db89 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -292,11 +292,13 @@ def auto_configure(people: list[dict]) -> list[dict]: # Cap selection to remaining capacity (no cap when replacement-only — executor # decides per-image whether to swap; any candidate could be an improvement). - auto_cap = None if not quality_replacement_only: if limit == "auto": + # Switch from open-ended auto to a fixed budget at remaining capacity + # so the diversity selector itself stops at the right count instead of + # selecting MAX_AUTO_IMAGES and then discarding the excess by position. if already_uploaded > 0: - auto_cap = capacity + limit = capacity else: limit = min(limit, capacity) @@ -320,11 +322,6 @@ def auto_configure(people: list[dict]) -> list[dict]: rprint(f" [dim]Skipping {name} (0 images selected).[/dim]") continue - # Apply auto_cap post-selection if needed - if auto_cap is not None and len(job["assets"]) > auto_cap: - job["assets"] = job["assets"][:auto_cap] - job["limit"] = len(job["assets"]) - rprint(f" [green]Queued {job['limit']} images for {name}.[/green]") jobs.append(job) diff --git a/winnow/reconcile.py b/winnow/reconcile.py index d3e9ab9..a6abdf4 100644 --- a/winnow/reconcile.py +++ b/winnow/reconcile.py @@ -45,7 +45,7 @@ def reconcile_frigate_mappings( ) return current_files = set(fresh) - if len(current_files - known_files_before) >= target: + if len(current_files - known_files_before) == target: break new_files = current_files - known_files_before diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index e721e6a..85ce303 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -112,22 +112,21 @@ def _maybe_migrate(cache_dir: str, conn: sqlite3.Connection) -> None: logger.info("Migrating JSON tracker files to SQLite in %s", cache_dir) - with conn: - if upload_json.exists(): - try: - data = json.loads(upload_json.read_text()) - _migrate_json_data(conn, data, "uploaded") - upload_json.rename(upload_json.with_suffix(".json.bak")) - except Exception as exc: - logger.warning("Migration of %s failed: %s", upload_json, exc) + 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 - if reject_json.exists(): - try: - data = json.loads(reject_json.read_text()) - _migrate_json_data(conn, data, "rejected") - reject_json.rename(reject_json.with_suffix(".json.bak")) - except Exception as exc: - logger.warning("Migration of %s failed: %s", reject_json, exc) + # Rename only after successful commit so a failed run retries cleanly next start. + if upload_json.exists(): + upload_json.rename(upload_json.with_suffix(".json.bak")) + if reject_json.exists(): + reject_json.rename(reject_json.with_suffix(".json.bak")) logger.info("JSON → SQLite migration complete") @@ -309,9 +308,14 @@ def has_frigate_scores(person_name: str) -> bool: return row[0] > 0 +_VALID_SCORE_COLS = frozenset({"blur_score", "frigate_score"}) + + def _pick_mapped_file( person_name: str, score_col: 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(