diff --git a/backend/app/database/face_clusters.py b/backend/app/database/face_clusters.py index 3c4ed5ded..c172315ee 100644 --- a/backend/app/database/face_clusters.py +++ b/backend/app/database/face_clusters.py @@ -188,6 +188,35 @@ def db_get_all_clusters() -> List[ClusterData]: conn.close() +def db_get_clusters_count() -> int: + """ + 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 clusters with one or more faces + """ + conn = sqlite3.connect(DATABASE_PATH) + cursor = conn.cursor() + + try: + 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() + + def db_update_cluster( cluster_id: ClusterId, cluster_name: Optional[ClusterName] = None, @@ -262,7 +291,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..7991585ae 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,235 @@ 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_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 + + +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 + + 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 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); + }); +}); 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