Fix four quality-replacement bugs found by code re-audit
C1/C4: Cap blur-score computation at 1440 px before calling assess_quality so scores are always on the same Laplacian scale as the embedding path (which operates on Immich preview thumbnails). Also converts the image to RGB before scoring and stores 0.0 on assess_quality failure so files uploaded without a score remain eligible for future quality replacement instead of occupying a slot permanently. C2: Fall back to the tracker's mapped-filename set as the pre-upload baseline when the Frigate GET /api/faces endpoint is unreachable at upload start. Previously, uploads that succeeded during a partial API outage were never mapped in frigate_files, leaving get_tracked_frigate_file_count permanently under-counting those files and allowing Frigate to exceed MAX_AUTO_IMAGES over time. C3: Track min_quality_score_for_slot when a quality-replacement delete succeeds but the subsequent upload fails. This ensures the freed slot can only be filled by a candidate that beats the deleted file's score, not just the next file in iteration order (which could be lower quality than what was deleted). Add get_tracked_frigate_filenames() to upload_tracker and expand tracker tests to cover the new function and exclude-parameter behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -156,3 +156,65 @@ def test_get_lowest_quality_mapped_file_skips_unscored():
|
||||
result = get_lowest_quality_mapped_file("Alice")
|
||||
assert result is not None
|
||||
assert result[1] == "asset-scored" # only scored file is a candidate
|
||||
|
||||
|
||||
# ── get_tracked_frigate_filenames ─────────────────────────────────────────────
|
||||
|
||||
def test_get_tracked_frigate_filenames_empty():
|
||||
from winnow.upload_tracker import get_tracked_frigate_filenames
|
||||
assert get_tracked_frigate_filenames("Alice") == set()
|
||||
|
||||
|
||||
def test_get_tracked_frigate_filenames_returns_mapped():
|
||||
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_file,
|
||||
remove_frigate_file,
|
||||
)
|
||||
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_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"}
|
||||
|
||||
|
||||
# ── get_lowest_quality_mapped_file with exclude ───────────────────────────────
|
||||
|
||||
def test_get_lowest_quality_exclude_skips_specified_file():
|
||||
from winnow.upload_tracker import (
|
||||
get_lowest_quality_mapped_file,
|
||||
mark_uploaded,
|
||||
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_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
|
||||
|
||||
|
||||
def test_get_lowest_quality_exclude_all_returns_none():
|
||||
from winnow.upload_tracker import (
|
||||
get_lowest_quality_mapped_file,
|
||||
mark_uploaded,
|
||||
record_frigate_file,
|
||||
)
|
||||
mark_uploaded("asset-a", person_name="Alice", score=0.50)
|
||||
record_frigate_file("Alice", "Alice-a.webp", "asset-a")
|
||||
assert get_lowest_quality_mapped_file("Alice", exclude={"Alice-a.webp"}) is None
|
||||
|
||||
+57
-7
@@ -17,9 +17,11 @@ from .frigate_api import delete_frigate_person_files, get_frigate_person_files
|
||||
from .image_processing import process_face_mode, process_full_mode, process_object_mode
|
||||
from .immich_api import fetch_face_data, fetch_full_image
|
||||
from .log_config import console
|
||||
from .quality import assess_quality
|
||||
from .upload_tracker import (
|
||||
get_lowest_quality_mapped_file,
|
||||
get_tracked_frigate_file_count,
|
||||
get_tracked_frigate_filenames,
|
||||
mark_rejected,
|
||||
mark_uploaded,
|
||||
record_frigate_file,
|
||||
@@ -205,7 +207,22 @@ def execute_jobs(jobs: list[dict]) -> None:
|
||||
# Record which asset produced which output file
|
||||
filename = f"{count}.jpg"
|
||||
asset_map[filename] = asset["id"]
|
||||
score_map[filename] = asset.get("quality_score") or asset.get("face_confidence")
|
||||
score_map[filename] = asset.get("quality_score")
|
||||
# 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 mode == "face" and 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(f"Quality score fallback for {asset['id']}: {exc}")
|
||||
score_map[filename] = 0.0 # unknown quality — treat as lowest
|
||||
# Also record object-mode variant filenames
|
||||
if mode == "object":
|
||||
for f in sorted(os.listdir(person_dir)):
|
||||
@@ -320,14 +337,41 @@ def upload_to_frigate(jobs: list[dict]) -> None:
|
||||
# Snapshot live Frigate files for post-upload reconciliation diff only.
|
||||
# effective_count is sourced from the tracker (mapped files) so that
|
||||
# manually-added Frigate files don't consume winnow's managed quota.
|
||||
known_frigate_files_at_start: set[str] = set(get_frigate_person_files(name) or [])
|
||||
_snapshot = 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).
|
||||
logger.warning(
|
||||
f"{name}: Frigate API unreachable at upload start"
|
||||
" — using tracker baseline for post-upload reconciliation"
|
||||
)
|
||||
known_frigate_files_at_start: set[str] = get_tracked_frigate_filenames(name)
|
||||
else:
|
||||
known_frigate_files_at_start: set[str] = set(_snapshot)
|
||||
effective_count = get_tracked_frigate_file_count(name)
|
||||
quality_replacement = job.get("config", {}).get("quality_replacement", False)
|
||||
actually_uploaded: list[tuple[str, str | None]] = []
|
||||
failed_deletes: set[str] = set()
|
||||
min_quality_score_for_slot: float | None = None
|
||||
|
||||
for fname in person_files:
|
||||
fpath = os.path.join(person_dir, fname)
|
||||
|
||||
# If a previous replacement delete succeeded but that upload failed,
|
||||
# require the next candidate to beat the deleted file's score so the
|
||||
# freed slot isn't filled with something worse than what we removed.
|
||||
if min_quality_score_for_slot is not None:
|
||||
file_score = score_map.get(fname)
|
||||
if file_score is None or file_score <= min_quality_score_for_slot:
|
||||
score_str = f"{file_score:.3f}" if file_score is not None else "N/A"
|
||||
progress.console.print(
|
||||
f" [dim]⏭ {fname}: score {score_str} ≤ freed slot floor"
|
||||
f" {min_quality_score_for_slot:.3f}, skipping[/dim]"
|
||||
)
|
||||
progress.advance(upload_task)
|
||||
continue
|
||||
|
||||
at_cap = effective_count >= Config.MAX_AUTO_IMAGES
|
||||
if at_cap:
|
||||
if not quality_replacement:
|
||||
@@ -339,7 +383,7 @@ def upload_to_frigate(jobs: list[dict]) -> None:
|
||||
progress.console.print(f" [dim]⏭ {fname}: no confidence score, skipping replacement[/dim]")
|
||||
progress.advance(upload_task)
|
||||
continue
|
||||
worst = get_lowest_quality_mapped_file(name)
|
||||
worst = get_lowest_quality_mapped_file(name, exclude=failed_deletes)
|
||||
if worst is None or new_score <= worst[2]:
|
||||
worst_score_str = f"{worst[2]:.3f}" if worst is not None else "N/A"
|
||||
progress.console.print(
|
||||
@@ -357,11 +401,10 @@ def upload_to_frigate(jobs: list[dict]) -> None:
|
||||
if delete_frigate_person_files(name, [worst_frigate_file]):
|
||||
remove_frigate_file(name, worst_frigate_file)
|
||||
effective_count -= 1
|
||||
min_quality_score_for_slot = worst_score
|
||||
else:
|
||||
logger.warning(f"Failed to delete {worst_frigate_file} for {name}, skipping replacement")
|
||||
# Remove from tracker so the next candidate targets a different file.
|
||||
# The file stays in Frigate (unmapped, like a manually-added file).
|
||||
remove_frigate_file(name, worst_frigate_file)
|
||||
failed_deletes.add(worst_frigate_file)
|
||||
progress.advance(upload_task)
|
||||
continue
|
||||
|
||||
@@ -377,6 +420,7 @@ def upload_to_frigate(jobs: list[dict]) -> None:
|
||||
uploaded += 1
|
||||
person_uploaded += 1
|
||||
effective_count += 1
|
||||
min_quality_score_for_slot = None
|
||||
|
||||
asset_id = asset_map.get(fname)
|
||||
if asset_id:
|
||||
@@ -438,7 +482,13 @@ def upload_to_frigate(jobs: list[dict]) -> None:
|
||||
|
||||
progress.advance(upload_task)
|
||||
|
||||
# Batch-map Frigate filenames to asset IDs now that all uploads are done
|
||||
if min_quality_score_for_slot is not None:
|
||||
logger.warning(
|
||||
f"{name}: freed replacement slot (floor {min_quality_score_for_slot:.3f})"
|
||||
" was not filled this run — will be available next run"
|
||||
)
|
||||
|
||||
# Batch-map Frigate filenames to asset IDs now that all uploads are done.
|
||||
if actually_uploaded:
|
||||
_reconcile_frigate_mappings(name, known_frigate_files_at_start, actually_uploaded)
|
||||
|
||||
|
||||
@@ -73,6 +73,11 @@ def delete_frigate_person_files(person_name: str, filenames: list[str]) -> bool:
|
||||
if resp.ok:
|
||||
logger.debug(f"Deleted {len(filenames)} Frigate file(s) for {person_name}")
|
||||
return True
|
||||
if resp.status_code == 404:
|
||||
# File already absent — stale tracker entry. Return True so the caller
|
||||
# removes it from the tracker and frees the slot cleanly.
|
||||
logger.warning(f"Frigate file(s) not found for {person_name} (stale tracker entry?): {filenames}")
|
||||
return True
|
||||
logger.warning(f"Frigate delete returned {resp.status_code} for {person_name}")
|
||||
return False
|
||||
except Exception as e:
|
||||
|
||||
@@ -12,7 +12,7 @@ Both are excluded from future candidate pools. To reset:
|
||||
by_person schema (frigate_uploaded_ids.json):
|
||||
{
|
||||
"asset_ids": ["immich-id-1", ...], # all assets we attempted to upload
|
||||
"scores": {"immich-id-1": 0.953}, # Immich face confidence at upload time
|
||||
"scores": {"immich-id-1": 450.3}, # Laplacian blur variance at upload time
|
||||
"frigate_files": {"PersonName-123.webp": "immich-id-1"}, # Frigate filename → asset ID
|
||||
"frigate_count": 42 # last known Frigate training image count
|
||||
}
|
||||
@@ -158,9 +158,26 @@ def get_tracked_frigate_file_count(person_name: str) -> int:
|
||||
return len(entry["frigate_files"])
|
||||
|
||||
|
||||
def get_lowest_quality_mapped_file(person_name: str) -> tuple[str, str, float] | None:
|
||||
def get_tracked_frigate_filenames(person_name: str) -> set[str]:
|
||||
"""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 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
|
||||
confidence score, or None if no mapped files with known scores exist."""
|
||||
quality score, or None if no mapped files with known scores exist.
|
||||
|
||||
Pass `exclude` to skip files that failed to delete this run without removing
|
||||
them from the tracker — they remain candidates on the next run.
|
||||
"""
|
||||
data = _load(UPLOAD_TRACKER_FILE)
|
||||
entry = _migrate_entry(data.get("by_person", {}).get(person_name, {}))
|
||||
frigate_files = entry.get("frigate_files", {})
|
||||
@@ -168,7 +185,7 @@ def get_lowest_quality_mapped_file(person_name: str) -> tuple[str, str, float] |
|
||||
candidates = [
|
||||
(frigate_filename, asset_id, scores[asset_id])
|
||||
for frigate_filename, asset_id in frigate_files.items()
|
||||
if asset_id in scores
|
||||
if asset_id in scores and (exclude is None or frigate_filename not in exclude)
|
||||
]
|
||||
if not candidates:
|
||||
return None
|
||||
|
||||
Reference in New Issue
Block a user