From 043ebf85d78bc23fd21c8b66fc560936cb5b740b Mon Sep 17 00:00:00 2001
From: Holden <holden@arch.fyi>
Date: Sun, 14 Jun 2026 20:54:39 +0000
Subject: [PATCH] fix: prevent silent reject-ID data loss on partial migration
 rename
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two fixes found during post-refactor audit:

1. upload_tracker: remove COUNT(*) guard from _maybe_migrate. The guard
   blocked re-migration when a previous run successfully committed both
   JSON files but a PermissionError on the second rename() left it on
   disk. On the next startup COUNT > 0 → early return → rejected IDs
   permanently unimported. INSERT OR IGNORE is idempotent so re-running
   migration is always safe; guard not needed.

   Also wrap each rename() in its own try/except so a failure on one
   file is logged and does not propagate uncaught.

2. reconcile: break early when new_count > target is detected in the
   poll loop. Previously the loop ran all four delay intervals (1+2+4+8s)
   before the post-loop > target branch fired, wasting up to 15 seconds
   when a concurrent external upload was visible on the first poll.
---
 winnow/reconcile.py      |  5 ++++-
 winnow/upload_tracker.py | 21 ++++++++++++---------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/winnow/reconcile.py b/winnow/reconcile.py
index a6abdf4..20d1639 100644
--- a/winnow/reconcile.py
+++ b/winnow/reconcile.py
@@ -45,8 +45,11 @@ def reconcile_frigate_mappings(
             )
             return
         current_files = set(fresh)
-        if len(current_files - known_files_before) == target:
+        new_count = len(current_files - known_files_before)
+        if new_count == target:
             break
+        if new_count > target:
+            break  # external upload already visible — no point polling further
 
     new_files = current_files - known_files_before
 
diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py
index 85ce303..4e6e55a 100644
--- a/winnow/upload_tracker.py
+++ b/winnow/upload_tracker.py
@@ -105,10 +105,10 @@ def _maybe_migrate(cache_dir: str, conn: sqlite3.Connection) -> None:
     if not upload_json.exists() and not reject_json.exists():
         return
 
-    # Check if tables are already populated
-    row = conn.execute("SELECT COUNT(*) FROM tracked_assets").fetchone()
-    if row[0] > 0:
-        return  # already migrated
+    # 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", cache_dir)
 
@@ -122,11 +122,14 @@ def _maybe_migrate(cache_dir: str, conn: sqlite3.Connection) -> None:
         logger.warning("JSON migration failed, will retry next run: %s", exc)
         return
 
-    # 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"))
+    # 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")