From cd4995c183fa16429a52ae5556a3050ed443e0f6 Mon Sep 17 00:00:00 2001 From: Vanshaj Poonia Date: Mon, 20 Jul 2026 22:39:48 +0530 Subject: [PATCH 1/3] fix(face-clusters): remove stale clusters after folder deletion (#1023) Face-cluster groups lingered in the UI after their source folder was removed, and clustering failed to start on a first AI-tagging pass. Four independent root causes: 1. Auto-clustering never created the first cluster. After onboarding writes user_preferences, metadata is non-empty, so with <=100 faces the 24h gate sent sync() down the incremental branch, which only assigns faces to existing cluster means and can't create the first one. Bootstrap a full pass when faces exist but no clusters do. 2. Empty clusters kept rendering. Deleting a folder cascades its faces away but leaves the now-empty face_clusters rows; the listing query had no HAVING filter. Exclude clusters with zero faces. 3. Recluster couldn't clear them. The zero-faces path returned a bare [] where callers unpack (results, skipped), raising ValueError; and even fixed, the early return skipped cluster deletion. Return the correct shape and clear all clusters when a full recluster yields none. 4. Deleting a folder never invalidated the ['clusters'] query, so a visible cluster list wouldn't refresh without a remount. Invalidate it explicitly (a combined ['folders','clusters'] tag would match neither query under React Query's prefix matching). Adds backend tests covering all four; Bug 3 is asserted at the DB layer since the new HAVING filter hides empty clusters from the API. --- backend/app/database/face_clusters.py | 19 ++- backend/app/utils/face_clusters.py | 22 ++- backend/tests/test_face_clusters.py | 169 +++++++++++++++++++++ frontend/src/hooks/useFolderOperations.tsx | 9 ++ 4 files changed, 217 insertions(+), 2 deletions(-) diff --git a/backend/app/database/face_clusters.py b/backend/app/database/face_clusters.py index 3c4ed5ded..8237a3783 100644 --- a/backend/app/database/face_clusters.py +++ b/backend/app/database/face_clusters.py @@ -188,6 +188,23 @@ def db_get_all_clusters() -> List[ClusterData]: conn.close() +def db_get_clusters_count() -> int: + """ + Count the clusters currently stored in the database. + + Returns: + Number of rows in the face_clusters table + """ + conn = sqlite3.connect(DATABASE_PATH) + cursor = conn.cursor() + + try: + cursor.execute("SELECT COUNT(*) FROM face_clusters") + return cursor.fetchone()[0] + finally: + conn.close() + + def db_update_cluster( cluster_id: ClusterId, cluster_name: Optional[ClusterName] = None, @@ -262,7 +279,7 @@ def db_get_all_clusters_with_face_counts() -> ( fc.face_image_base64, COALESCE(AVG(f.confidence), 0) as avg_confidence FROM face_clusters fc - LEFT JOIN faces f ON fc.cluster_id = f.cluster_id + INNER JOIN faces f ON fc.cluster_id = f.cluster_id GROUP BY fc.cluster_id, fc.cluster_name, fc.face_image_base64 """ ) diff --git a/backend/app/utils/face_clusters.py b/backend/app/utils/face_clusters.py index 4dc608ee5..f6965d67e 100644 --- a/backend/app/utils/face_clusters.py +++ b/backend/app/utils/face_clusters.py @@ -23,7 +23,11 @@ db_get_cluster_mean_embeddings, db_get_cluster_image_pairs, ) -from app.database.face_clusters import db_delete_all_clusters, db_insert_clusters_batch +from app.database.face_clusters import ( + db_delete_all_clusters, + db_insert_clusters_batch, + db_get_clusters_count, +) from app.database.metadata import ( db_get_metadata, db_update_metadata, @@ -71,6 +75,7 @@ def cluster_util_is_reclustering_needed(metadata) -> bool: Check if reclustering is needed based on: 1. Time since last clustering (24 hours) 2. Number of faces without cluster ID (> 100) + 3. No clusters existing yet while faces are waiting to be assigned Returns: bool: True if reclustering is needed, False otherwise @@ -96,6 +101,11 @@ def cluster_util_is_reclustering_needed(metadata) -> bool: if len(unassigned_faces) > 100: return True + # Incremental assignment matches faces against the means of existing clusters, so + # it can never create the first one. Bootstrap that case with a full pass. + if unassigned_faces and db_get_clusters_count() == 0: + return True + return False @@ -113,6 +123,16 @@ def cluster_util_face_clusters_sync(force_full_reclustering: bool = False): results, total_faces_skipped = cluster_util_cluster_all_face_embeddings() if not results: + # A full recluster rebuilds from scratch, so producing no clusters means + # none should remain. Without this the rows outlive the faces that + # justified them (e.g. after their folder is deleted) and keep surfacing. + with get_db_connection() as conn: + cursor = conn.cursor() + db_delete_all_clusters(cursor) + + current_metadata = metadata or {} + current_metadata["reclustering_time"] = datetime.now().timestamp() + db_update_metadata(current_metadata, cursor) return 0, total_faces_skipped results = [result.to_dict() for result in results] diff --git a/backend/tests/test_face_clusters.py b/backend/tests/test_face_clusters.py index 4f5d7d703..65fda9655 100644 --- a/backend/tests/test_face_clusters.py +++ b/backend/tests/test_face_clusters.py @@ -2,8 +2,15 @@ import numpy as np from unittest.mock import patch from fastapi import FastAPI +import app.database.face_clusters as face_clusters_db +import app.database.faces as faces_db +import app.database.images as images_db +import app.database.folders as folders_db +import app.database.yolo_mapping as yolo_db from app.utils.face_clusters import ( cluster_util_cluster_all_face_embeddings, + cluster_util_face_clusters_sync, + cluster_util_is_reclustering_needed, estimate_eps, ) from app.utils.face_quality import face_passes_quality_gate @@ -704,3 +711,165 @@ def test_quality_gate(self): ) is False ) + + +# ############################## +# Stale cluster cleanup (issue #1023) +# ############################## + + +@pytest.fixture +def isolated_cluster_db(tmp_path, monkeypatch): + """Point face_clusters/faces DB helpers at a disposable SQLite file. + + Each database module binds DATABASE_PATH at import time, so every module + that opens a connection needs its own attribute patched directly. + """ + db_path = str(tmp_path / "test_face_clusters.sqlite3") + monkeypatch.setattr(face_clusters_db, "DATABASE_PATH", db_path) + monkeypatch.setattr(faces_db, "DATABASE_PATH", db_path) + monkeypatch.setattr(images_db, "DATABASE_PATH", db_path) + monkeypatch.setattr(folders_db, "DATABASE_PATH", db_path) + monkeypatch.setattr(yolo_db, "DATABASE_PATH", db_path) + + yolo_db.db_create_YOLO_classes_table() + face_clusters_db.db_create_clusters_table() + faces_db.db_create_faces_table() + folders_db.db_create_folders_table() + images_db.db_create_images_table() + yield db_path + + +class TestEmptyClusterCleanup: + """Removing a folder cascades away its faces, but the clusters those faces + built are left behind. They must not keep surfacing in the UI.""" + + def test_listing_excludes_clusters_with_no_faces(self, isolated_cluster_db): + """A cluster whose faces are all gone is not returned to the caller.""" + face_clusters_db.db_insert_clusters_batch( + [ + { + "cluster_id": "populated", + "cluster_name": "Has Faces", + "face_image_base64": "b64", + }, + { + "cluster_id": "orphaned", + "cluster_name": "Folder Deleted", + "face_image_base64": "b64", + }, + ] + ) + faces_db.db_insert_face_embeddings( + image_id="img-1", + embeddings=np.ones(128), + confidence=0.9, + bbox={"x": 0, "y": 0, "width": 10, "height": 10}, + cluster_id="populated", + ) + + listed = face_clusters_db.db_get_all_clusters_with_face_counts() + + assert [c["cluster_id"] for c in listed] == ["populated"] + + def test_clusters_count_reflects_stored_rows(self, isolated_cluster_db): + assert face_clusters_db.db_get_clusters_count() == 0 + + face_clusters_db.db_insert_clusters_batch( + [{"cluster_id": "c1", "cluster_name": None, "face_image_base64": None}] + ) + + assert face_clusters_db.db_get_clusters_count() == 1 + + +class TestReclusterWithNoFaces: + """A forced recluster of an emptied library must clear stale clusters + instead of raising and leaving them in place.""" + + @patch("app.utils.face_clusters.db_get_all_faces_with_cluster_names") + def test_cluster_all_embeddings_returns_pair_when_no_faces(self, mock_faces): + """The no-faces path must return the same (results, skipped) shape as + every other path -- callers unpack it into two names.""" + mock_faces.return_value = [] + + assert cluster_util_cluster_all_face_embeddings() == ([], 0) + + @patch("app.utils.face_clusters.db_update_metadata") + @patch("app.utils.face_clusters.db_delete_all_clusters") + @patch("app.utils.face_clusters.get_db_connection") + @patch("app.utils.face_clusters.db_get_all_faces_with_cluster_names") + @patch("app.utils.face_clusters.db_get_metadata") + def test_forced_recluster_clears_clusters_when_no_faces_remain( + self, mock_metadata, mock_faces, mock_conn, mock_delete, mock_update_metadata + ): + mock_metadata.return_value = {"user_preferences": {}} + mock_faces.return_value = [] + + created, skipped = cluster_util_face_clusters_sync(force_full_reclustering=True) + + assert (created, skipped) == (0, 0) + mock_delete.assert_called_once() + + @patch("app.utils.face_clusters.db_delete_all_clusters") + @patch("app.utils.face_clusters.get_db_connection") + @patch("app.utils.face_clusters.db_update_face_cluster_ids_batch") + @patch( + "app.utils.face_clusters.cluster_util_assign_cluster_to_faces_without_clusterId" + ) + @patch("app.utils.face_clusters.cluster_util_is_reclustering_needed") + @patch("app.utils.face_clusters.db_get_metadata") + def test_incremental_pass_never_deletes_clusters( + self, + mock_metadata, + mock_needed, + mock_assign, + mock_update_batch, + mock_conn, + mock_delete, + ): + """Only a full recluster rebuilds from scratch; the incremental path + must leave existing clusters alone.""" + mock_metadata.return_value = {"user_preferences": {}} + mock_needed.return_value = False + mock_assign.return_value = ([{"face_id": 1, "cluster_id": "c1"}], 0) + + cluster_util_face_clusters_sync() + + mock_delete.assert_not_called() + + +class TestReclusteringNeededBootstrap: + """Incremental assignment matches faces against existing cluster means, so + it can never create the first cluster -- that case needs a full pass.""" + + @patch("app.utils.face_clusters.db_get_clusters_count") + @patch("app.utils.face_clusters.db_get_faces_unassigned_clusters") + def test_full_pass_forced_when_no_clusters_exist_yet( + self, mock_unassigned, mock_count + ): + mock_unassigned.return_value = [{"face_id": i} for i in range(50)] + mock_count.return_value = 0 + + assert cluster_util_is_reclustering_needed({"user_preferences": {}}) is True + + @patch("app.utils.face_clusters.db_get_clusters_count") + @patch("app.utils.face_clusters.db_get_faces_unassigned_clusters") + def test_incremental_still_used_once_clusters_exist( + self, mock_unassigned, mock_count + ): + """Guard against over-correcting into a full recluster every sync.""" + mock_unassigned.return_value = [{"face_id": i} for i in range(50)] + mock_count.return_value = 3 + + assert cluster_util_is_reclustering_needed({"user_preferences": {}}) is False + + @patch("app.utils.face_clusters.db_get_clusters_count") + @patch("app.utils.face_clusters.db_get_faces_unassigned_clusters") + def test_no_faces_and_no_clusters_does_not_force_a_pass( + self, mock_unassigned, mock_count + ): + """An empty library has nothing to bootstrap from.""" + mock_unassigned.return_value = [] + mock_count.return_value = 0 + + assert cluster_util_is_reclustering_needed({"user_preferences": {}}) is False diff --git a/frontend/src/hooks/useFolderOperations.tsx b/frontend/src/hooks/useFolderOperations.tsx index fe436b97d..c4e7081f6 100644 --- a/frontend/src/hooks/useFolderOperations.tsx +++ b/frontend/src/hooks/useFolderOperations.tsx @@ -1,5 +1,6 @@ import { useEffect } from 'react'; import { useDispatch, useSelector } from 'react-redux'; +import { useQueryClient } from '@tanstack/react-query'; import { usePictoMutation, usePictoQuery } from '@/hooks/useQueryExtension'; import { enableAITagging, @@ -19,6 +20,7 @@ import { getFoldersTaggingStatus } from '@/api/api-functions/folders'; */ export const useFolderOperations = () => { const dispatch = useDispatch(); + const queryClient = useQueryClient(); const folders = useSelector(selectAllFolders); // Query for folders @@ -134,6 +136,13 @@ export const useFolderOperations = () => { mutationFn: async (folder_id: string) => deleteFolders({ folder_ids: [folder_id] }), autoInvalidateTags: ['folders'], + // Deleting a folder cascades to its images and faces, so any cluster built from + // them is now stale. This has to be a separate call: autoInvalidateTags is passed + // through as a single queryKey and matches by prefix, so ['folders', 'clusters'] + // would match neither query. + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['clusters'] }); + }, }); // Apply feedback to the delete folder mutation From 69ee7ca0bcd8d2ab2474f1c513009e15e474d048 Mon Sep 17 00:00:00 2001 From: Vanshaj Poonia Date: Mon, 20 Jul 2026 23:48:59 +0530 Subject: [PATCH 2/3] refactor(face-clusters): address CodeRabbit review on #1392 Two review nitpicks on the #1023 fix: - Use INNER JOIN instead of LEFT JOIN + HAVING to exclude empty clusters from the listing query. Verified equivalent output (including the all-clusters-empty case) before applying; INNER JOIN filters non-matching rows during the join instead of generating and then discarding them post-aggregation. - Add regression coverage for the folder-delete cluster invalidation added in the previous commit: a successful delete invalidates ['clusters'], a failed one does not. Confirmed the test fails without that invalidation call before checking it in. --- .../__tests__/useFolderOperations.test.tsx | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 frontend/src/hooks/__tests__/useFolderOperations.test.tsx diff --git a/frontend/src/hooks/__tests__/useFolderOperations.test.tsx b/frontend/src/hooks/__tests__/useFolderOperations.test.tsx new file mode 100644 index 000000000..80c101b23 --- /dev/null +++ b/frontend/src/hooks/__tests__/useFolderOperations.test.tsx @@ -0,0 +1,93 @@ +import React from 'react'; +import { renderHook, waitFor } from '@testing-library/react'; +import { Provider } from 'react-redux'; +import { configureStore } from '@reduxjs/toolkit'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { rootReducer } from '@/app/store'; +import { useFolderOperations } from '@/hooks/useFolderOperations'; +import * as foldersApi from '@/api/api-functions/folders'; + +jest.mock('@/api/api-functions/folders', () => ({ + getAllFolders: jest + .fn() + .mockResolvedValue({ success: true, data: { folders: [] } }), + getFoldersTaggingStatus: jest + .fn() + .mockResolvedValue({ success: true, data: [] }), + addFolder: jest.fn(), + enableAITagging: jest.fn(), + disableAITagging: jest.fn(), + deleteFolders: jest.fn(), +})); + +const deleteFolders = foldersApi.deleteFolders as jest.Mock; + +function renderUseFolderOperations() { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: Infinity, staleTime: Infinity }, + mutations: { retry: false }, + }, + }); + const invalidateSpy = jest.spyOn(queryClient, 'invalidateQueries'); + const store = configureStore({ reducer: rootReducer }); + + const wrapper = ({ children }: { children: React.ReactNode }) => ( + + {children} + + ); + + const rendered = renderHook(() => useFolderOperations(), { wrapper }); + return { ...rendered, invalidateSpy }; +} + +const clustersKeyCalls = (spy: jest.SpyInstance) => + spy.mock.calls.filter( + ([arg]) => JSON.stringify(arg?.queryKey) === JSON.stringify(['clusters']), + ); + +describe('useFolderOperations - delete folder cache invalidation', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('invalidates the clusters query when folder deletion succeeds', async () => { + deleteFolders.mockResolvedValueOnce({ success: true, data: {} }); + const { result, invalidateSpy } = renderUseFolderOperations(); + + result.current.deleteFolder('folder-1'); + + await waitFor(() => { + expect(clustersKeyCalls(invalidateSpy).length).toBeGreaterThan(0); + }); + }); + + it('does not invalidate the clusters query when folder deletion fails', async () => { + // usePictoMutation hardcodes retry: 2, so every attempt (not just the + // first) must reject -- otherwise the retry falls through to the mock's + // default undefined return, which resolves as a false success. + deleteFolders.mockRejectedValue(new Error('delete failed')); + const { result, invalidateSpy } = renderUseFolderOperations(); + + result.current.deleteFolder('folder-1'); + + // autoInvalidateTags still fires ['folders'] on settle regardless of + // outcome, so wait for that instead of an arbitrary timeout to know the + // mutation has actually settled before asserting clusters was skipped. + // retry: 2 with a 500ms retryDelay means settling can take >1s. + await waitFor( + () => { + expect( + invalidateSpy.mock.calls.some( + ([arg]) => + JSON.stringify(arg?.queryKey) === JSON.stringify(['folders']), + ), + ).toBe(true); + }, + { timeout: 3000 }, + ); + + expect(clustersKeyCalls(invalidateSpy)).toHaveLength(0); + }); +}); From 76fbceac106eab558064c54ecd71d06a1212cd87 Mon Sep 17 00:00:00 2001 From: Vanshaj Poonia Date: Mon, 27 Jul 2026 16:45:09 +0530 Subject: [PATCH 3/3] fix(face-clusters): count only clusters that still have faces db_get_clusters_count() counted raw face_clusters rows, including orphans whose faces were already gone. The bootstrap check treats any nonzero count as proof that a usable cluster exists, so a library whose clusters were all orphaned by a folder deletion would skip the full pass -- and the incremental pass it fell back to matches faces against cluster means drawn from the faces table, of which an orphan has none. Newly imported faces were left unclustered until the 24-hour rule eventually forced a full pass. Counting distinct clusters that have at least one face makes this agree with the INNER JOIN already used by the listing query: both now mean "clusters a user can actually see and use". Regression test covers orphan rows alongside unassigned faces, plus the other side -- a cluster that still has faces must keep using the cheaper incremental path rather than being dragged into a full recluster. Raised by CodeRabbit on #1392. --- backend/app/database/face_clusters.py | 18 +++++-- backend/tests/test_face_clusters.py | 72 ++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/backend/app/database/face_clusters.py b/backend/app/database/face_clusters.py index 8237a3783..c172315ee 100644 --- a/backend/app/database/face_clusters.py +++ b/backend/app/database/face_clusters.py @@ -190,16 +190,28 @@ def db_get_all_clusters() -> List[ClusterData]: def db_get_clusters_count() -> int: """ - Count the clusters currently stored in the database. + Count the clusters that still have at least one face attached. + + Rows whose faces are all gone (e.g. after their folder was deleted) are + excluded, matching the INNER JOIN used by the cluster listing. Callers ask + this to decide whether any *usable* cluster exists: an orphan row cannot + seed incremental assignment, because that matches faces against cluster + means derived from the faces table. Returns: - Number of rows in the face_clusters table + Number of clusters with one or more faces """ conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() try: - cursor.execute("SELECT COUNT(*) FROM face_clusters") + cursor.execute( + """ + SELECT COUNT(DISTINCT fc.cluster_id) + FROM face_clusters fc + INNER JOIN faces f ON fc.cluster_id = f.cluster_id + """ + ) return cursor.fetchone()[0] finally: conn.close() diff --git a/backend/tests/test_face_clusters.py b/backend/tests/test_face_clusters.py index 65fda9655..7991585ae 100644 --- a/backend/tests/test_face_clusters.py +++ b/backend/tests/test_face_clusters.py @@ -772,13 +772,26 @@ def test_listing_excludes_clusters_with_no_faces(self, isolated_cluster_db): assert [c["cluster_id"] for c in listed] == ["populated"] - def test_clusters_count_reflects_stored_rows(self, isolated_cluster_db): + def test_clusters_count_only_counts_clusters_with_faces(self, isolated_cluster_db): + """The count drives the bootstrap decision, so it has to agree with the + listing: an orphan row is not a cluster anyone can use.""" assert face_clusters_db.db_get_clusters_count() == 0 face_clusters_db.db_insert_clusters_batch( [{"cluster_id": "c1", "cluster_name": None, "face_image_base64": None}] ) + # Still zero -- the row exists but has no faces behind it. + assert face_clusters_db.db_get_clusters_count() == 0 + + faces_db.db_insert_face_embeddings( + image_id="img-1", + embeddings=np.ones(128), + confidence=0.9, + bbox={"x": 0, "y": 0, "width": 10, "height": 10}, + cluster_id="c1", + ) + assert face_clusters_db.db_get_clusters_count() == 1 @@ -873,3 +886,60 @@ def test_no_faces_and_no_clusters_does_not_force_a_pass( mock_count.return_value = 0 assert cluster_util_is_reclustering_needed({"user_preferences": {}}) is False + + def test_orphan_rows_do_not_block_the_bootstrap_pass(self, isolated_cluster_db): + """Rows left behind by a deleted folder are not usable clusters. + + Incremental assignment matches against cluster *means*, which come from + the faces table -- an orphan row contributes none, so the incremental + pass has nothing to match and silently assigns nothing. Counting those + rows as existing clusters would suppress the bootstrap full pass and + leave the new faces unclustered. + """ + face_clusters_db.db_insert_clusters_batch( + [ + { + "cluster_id": "orphaned", + "cluster_name": "Folder Deleted", + "face_image_base64": "b64", + } + ] + ) + faces_db.db_insert_face_embeddings( + image_id="img-new", + embeddings=np.ones(128), + confidence=0.9, + bbox={"x": 0, "y": 0, "width": 10, "height": 10}, + cluster_id=None, + ) + + assert cluster_util_is_reclustering_needed({"user_preferences": {}}) is True + + def test_faces_still_attached_keep_the_incremental_path(self, isolated_cluster_db): + """Guard the other side: a cluster that still has faces can seed the + incremental pass, so it must not be dragged into a full recluster.""" + face_clusters_db.db_insert_clusters_batch( + [ + { + "cluster_id": "populated", + "cluster_name": "Still Here", + "face_image_base64": "b64", + } + ] + ) + faces_db.db_insert_face_embeddings( + image_id="img-1", + embeddings=np.ones(128), + confidence=0.9, + bbox={"x": 0, "y": 0, "width": 10, "height": 10}, + cluster_id="populated", + ) + faces_db.db_insert_face_embeddings( + image_id="img-new", + embeddings=np.ones(128), + confidence=0.9, + bbox={"x": 0, "y": 0, "width": 10, "height": 10}, + cluster_id=None, + ) + + assert cluster_util_is_reclustering_needed({"user_preferences": {}}) is False