From 837c246c15b19a9296017e85893866a3acc6e952 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 4 Oct 2026 17:07:24 -0400 Subject: [PATCH] Refuse a non-finite cluster eps inside the resolver too The routers already 422 a non-finite cluster_eps (#375), but the resolver itself still trusted its input: _shrink_eps_to_pair_budget loops forever on inf once an album is over the pair budget, and nan slips past both of its comparisons into DBSCAN. Raise ValueError there, and in resolve_album_cluster_eps for the empty-album path that returns before the shrink. Also cover nan/inf/1e400 query values on /umap_data and /cluster_labels, which were not tested. Fixes #379 Co-Authored-By: Claude Opus 5.5 --- photomap/backend/cluster_eps.py | 19 +++++++++++++++++++ tests/backend/test_cluster_eps.py | 19 +++++++++++++++++++ tests/backend/test_umap_eps_validation.py | 5 ++++- 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/photomap/backend/cluster_eps.py b/photomap/backend/cluster_eps.py index 87281a73..b7033ec3 100644 --- a/photomap/backend/cluster_eps.py +++ b/photomap/backend/cluster_eps.py @@ -30,6 +30,7 @@ import hashlib import logging +import math from pathlib import Path import numpy as np @@ -160,6 +161,13 @@ def adaptive_cluster_eps( return best +def _require_finite(eps: float) -> None: + """Refuse an eps no clustering can use. ``None`` is not checked here — it + means "derive one" and is handled by the callers.""" + if not math.isfinite(eps): + raise ValueError(f"Cluster eps must be a finite number, got {eps!r}") + + def _shrink_eps_to_pair_budget(coords: np.ndarray, eps: float) -> float: """Shrink ``eps`` until DBSCAN's neighbor-pair count fits the budget. @@ -172,7 +180,14 @@ def _shrink_eps_to_pair_budget(coords: np.ndarray, eps: float) -> float: user ask for an eps far looser than an album can afford, and on a six-figure album that is an out-of-memory crash rather than a slow response. + + A non-finite ``eps`` is refused rather than shrunk: ``inf * 0.7`` is still + ``inf`` and every pair is in range, so the loop below would never exit, + and ``nan`` fails both of its comparisons and would slip through to + DBSCAN. The routers refuse both with a 422 before they get here; this + keeps the loop bounded for any other caller. """ + _require_finite(eps) if coords.shape[0] < 2: return eps @@ -245,6 +260,10 @@ def resolve_album_cluster_eps( unclamped and no cache entry is written. """ eps = requested if requested is not None else stored + if eps is not None: + # Checked here as well as in the pair-budget shrink because an empty + # album returns before reaching it, and would hand the value back. + _require_finite(eps) if coords is None or coords.shape[0] == 0: return eps if eps is not None else FALLBACK_CLUSTER_EPS if eps is None: diff --git a/tests/backend/test_cluster_eps.py b/tests/backend/test_cluster_eps.py index 87953ea8..2c019889 100644 --- a/tests/backend/test_cluster_eps.py +++ b/tests/backend/test_cluster_eps.py @@ -167,6 +167,25 @@ def test_pair_budget_shrinks_an_absurd_eps(): assert pairs <= MAX_NEIGHBOR_PAIRS +@pytest.mark.parametrize("eps", [float("inf"), float("nan")]) +def test_a_non_finite_eps_is_refused_not_resolved(eps): + """``inf`` never shrinks (``inf * 0.7`` is ``inf``), so on an album big + enough to be over the pair budget the shrink loop never exits; ``nan`` + fails every comparison and reaches DBSCAN, which raises. The routers 422 + both, but the resolver must not depend on that.""" + with pytest.raises(ValueError, match="finite"): + resolve_cluster_eps(blobs(), eps) + + +@pytest.mark.parametrize("requested, stored", [(float("inf"), None), (None, float("nan"))]) +@pytest.mark.parametrize("coords", [blobs(), np.empty((0, 2), dtype=np.float32), None]) +def test_album_resolver_refuses_a_non_finite_eps(tmp_path, coords, requested, stored): + """Including for an album with nothing to cluster, which returns before + the pair-budget shrink would have caught it.""" + with pytest.raises(ValueError, match="finite"): + resolve_album_cluster_eps(coords, tmp_path, requested=requested, stored=stored) + + def test_pair_budget_leaves_a_reasonable_eps_alone(): coords = blobs() assert resolve_cluster_eps(coords, 0.3) == pytest.approx(0.3) diff --git a/tests/backend/test_umap_eps_validation.py b/tests/backend/test_umap_eps_validation.py index dd1a0811..dcac06e0 100644 --- a/tests/backend/test_umap_eps_validation.py +++ b/tests/backend/test_umap_eps_validation.py @@ -73,7 +73,10 @@ def test_a_usable_eps_is_still_stored(client, new_album, eps): assert get_config_manager().get_album(key).umap_eps == pytest.approx(eps) -@pytest.mark.parametrize("eps", [0, -1, 0.005]) +# The non-finite spellings are the ones from issue #379: ``nan`` 500'd out of +# DBSCAN, and ``inf`` (or ``1e400``, which overflows to it) wedged a worker +# thread in the pair-budget shrink forever. +@pytest.mark.parametrize("eps", [0, -1, 0.005, "nan", "NaN", "inf", "Infinity", "-inf", "1e400"]) def test_an_unusable_query_parameter_is_refused(client, new_album, eps): """A 422 from the caller rather than a 500 out of sklearn — or, for the value under the floor, rather than a map clustered at something the