From 16fb1cd6b27bbffc9fe130168194016f5b7d2731 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Mon, 9 Nov 2026 21:14:58 -0500 Subject: [PATCH] fix(discover): an aborted run sends no batch; the watcher yields to Discover runs Live dev run: Discover remounted, the resume started an 11-file run, and StrictMode's simulated unmount aborted it during its 10 ms start-up pause. The run still sent its batch, and an aborted run's listeners ignore every progress and file-complete event, so the list stayed at '0 ready' with placeholders until the 5-minute inactivity lock cancelled the batch (the image in progress became a filename-only 'Limited analysis'). The wait helper now refuses to send for an aborted or superseded run, and the run returns without a completion notice; its finally starts the files the remount queued. The smart-folder watcher holds its queue while any user-started analysis is in flight (userAnalysisInFlight) and resumes on its own afterwards, so a startup catch-up of hundreds of smart-folder files no longer halves a Discover run. --- CHANGELOG.md | 10 ++ src/main/analysis/userAnalysisInFlight.js | 10 ++ src/main/services/SmartFolderWatcher.js | 38 ++++++- src/renderer/phases/discover/batchBusy.js | 5 + src/renderer/phases/discover/useAnalysis.js | 5 +- ...artFolderWatcher.userAnalysisDefer.test.js | 29 +++++ test/useAnalysis.test.js | 102 ++++++++++++++++++ 7 files changed, 197 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03bf0cc..578676c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -203,6 +203,16 @@ Changes you will notice: ### Fixed +- **Analyzed files show up again after Discover remounts mid-start.** A run that Discover's unmount + (or React StrictMode in development) aborted during its first moments still sent its batch, then + ignored every result: the list stayed on "0 ready" with loading placeholders, and five minutes + later the inactivity check cancelled the batch, turning the image in progress into a filename-only + "Limited analysis". An aborted run now sends nothing, reports no completion, and hands its files + to the run that replaces it. +- **The smart-folder watcher pauses while you analyze in Discover.** Its catch-up of files already + in your smart folders (for example the first launch over a library another app sorted) took turns + with the Discover run on the one GPU, halving its speed. The watcher now holds its queue until the + Discover run ends and then resumes on its own. - **Files are no longer analyzed two or three times at once.** Leaving Discover and coming back while a batch ran started a new run that the window's still-running batch refused; the new run then analyzed the same files again file by file, in parallel, and restarted itself when it ended. diff --git a/src/main/analysis/userAnalysisInFlight.js b/src/main/analysis/userAnalysisInFlight.js index 8f52138..e058f36 100644 --- a/src/main/analysis/userAnalysisInFlight.js +++ b/src/main/analysis/userAnalysisInFlight.js @@ -67,6 +67,15 @@ function isUserAnalysisInFlight(filePath) { return counts.has(keyOf(filePath)); } +/** + * Whether any user-started analysis is running. The smart-folder watcher + * holds its own queue meanwhile so the user's run gets the GPU. + * @returns {boolean} + */ +function hasUserAnalysisInFlight() { + return counts.size > 0; +} + /** * Wrap one analysis call so its file is marked for the call's duration. * @template T @@ -91,6 +100,7 @@ function _resetUserAnalysisInFlight() { module.exports = { claimUserAnalysis, isUserAnalysisInFlight, + hasUserAnalysisInFlight, withUserAnalysis, _resetUserAnalysisInFlight }; diff --git a/src/main/services/SmartFolderWatcher.js b/src/main/services/SmartFolderWatcher.js index 1093226..ec2b387 100644 --- a/src/main/services/SmartFolderWatcher.js +++ b/src/main/services/SmartFolderWatcher.js @@ -60,11 +60,16 @@ const { resolveRealPaths } = require('../../shared/pathSanitization'); const { isImagePath, isMediaPath } = require('../../shared/fileIdUtils'); -const { isUserAnalysisInFlight } = require('../analysis/userAnalysisInFlight'); +const { + isUserAnalysisInFlight, + hasUserAnalysisInFlight +} = require('../analysis/userAnalysisInFlight'); // How long a file Discover is analyzing right now waits before the watcher // checks it again. const USER_ANALYSIS_DEFER_MS = 15000; +// How often a queue held for a user-started analysis checks again. +const USER_ANALYSIS_HOLD_RECHECK_MS = 5000; // The queue keeps taking files of the kind it just ran (documents, images, // or audio/video) while any are waiting, so each model loads once per run of @@ -314,6 +319,8 @@ class SmartFolderWatcher { this._inFlightItems = new Map(); /** @type {'image' | 'media' | 'document' | null} */ this._lastLane = null; + /** @type {ReturnType | null} */ + this._userAnalysisHoldTimer = null; this._laneStreak = 0; // Abort controllers of the analyses running now, so stop() can cancel // them instead of waiting them out. @@ -863,6 +870,10 @@ class SmartFolderWatcher { this.queueTimer = null; } // And the disk-full recheck, so a held queue is not processed after stop. + if (this._userAnalysisHoldTimer) { + clearTimeout(this._userAnalysisHoldTimer); + this._userAnalysisHoldTimer = null; + } if (this._storageFullHoldTimer) { clearTimeout(this._storageFullHoldTimer); this._storageFullHoldTimer = null; @@ -2244,6 +2255,31 @@ class SmartFolderWatcher { return; } + // A Discover analysis is running: leave the GPU and the models to it. + // Both used to take turns file by file, so a startup catch-up of the + // smart folders halved the speed of the run the user was waiting on. + // The queue is durable; it resumes on its own once the run ends. + if (hasUserAnalysisInFlight()) { + if (!this._userAnalysisHoldTimer) { + logger.info('[SMART-FOLDER-WATCHER] Holding the queue while a Discover analysis runs', { + queued: this.analysisQueue.length + }); + const recheck = () => { + this._userAnalysisHoldTimer = null; + if (hasUserAnalysisInFlight()) { + this._userAnalysisHoldTimer = setTimeout(recheck, USER_ANALYSIS_HOLD_RECHECK_MS); + this._userAnalysisHoldTimer?.unref?.(); + return; + } + logger.info('[SMART-FOLDER-WATCHER] Discover analysis finished; resuming the queue'); + void this._processQueue(); + }; + this._userAnalysisHoldTimer = setTimeout(recheck, USER_ANALYSIS_HOLD_RECHECK_MS); + this._userAnalysisHoldTimer?.unref?.(); + } + return; + } + // Disk full: the vector store cannot save, so analyzing now would spend // model time on results that are lost on restart. Hold the queue (it is // durable) and look again shortly; the store clears the flag on its own diff --git a/src/renderer/phases/discover/batchBusy.js b/src/renderer/phases/discover/batchBusy.js index 53a6de9..2ad5d9e 100644 --- a/src/renderer/phases/discover/batchBusy.js +++ b/src/renderer/phases/discover/batchBusy.js @@ -84,6 +84,11 @@ export async function runBatchPathWhenFree({ /** @type {any[] | undefined} */ let files; for (;;) { + // A run aborted before its request went out (Discover unmounted during + // start-up, React StrictMode in development) must not send it: its + // listeners ignore every event of an aborted run, so the batch would run + // with nothing applying its results. + if (!isActiveRun() || abortSignal?.aborted) return 'superseded'; try { await runBatchPath(files); return 'ran'; diff --git a/src/renderer/phases/discover/useAnalysis.js b/src/renderer/phases/discover/useAnalysis.js index 0084924..13a7858 100644 --- a/src/renderer/phases/discover/useAnalysis.js +++ b/src/renderer/phases/discover/useAnalysis.js @@ -1688,7 +1688,10 @@ export function useAnalysis(options = {}) { await runWorkerPool(); } - if (!isActiveRun()) return; + // An aborted run (Discover unmounted) is not finished: no completion + // notice. The finally below releases the lock and starts any files a + // remount queued meanwhile. + if (!isActiveRun() || abortSignal.aborted) return; const finalProgress = { current: Math.min(completedCountRef.current, uniqueFiles.length), diff --git a/test/SmartFolderWatcher.userAnalysisDefer.test.js b/test/SmartFolderWatcher.userAnalysisDefer.test.js index b13174f..e391eaf 100644 --- a/test/SmartFolderWatcher.userAnalysisDefer.test.js +++ b/test/SmartFolderWatcher.userAnalysisDefer.test.js @@ -135,3 +135,32 @@ describe('SmartFolderWatcher defers files Discover is analyzing', () => { expect(watcher.analyzeDocumentFile).toHaveBeenCalled(); }); }); + +describe('SmartFolderWatcher holds its queue while a Discover analysis runs', () => { + // Live run: a startup catch-up of 879 smart-folder files took turns with + // the user's 11-file Discover batch, file by file, on one GPU. + afterEach(() => { + jest.useRealTimers(); + }); + + test('nothing is analyzed during the run, and the queue resumes after it', async () => { + jest.useFakeTimers(); + const watcher = makeWatcher(); + watcher._analyzeFile = jest.fn(async () => {}); + watcher.analysisQueue.push({ filePath: file, mtime: 1, eventType: 'add' }); + + const release = claimUserAnalysis(path.join(root, 'other-discover-file.pdf')); + await watcher._processQueue(); + expect(watcher._analyzeFile).not.toHaveBeenCalled(); + expect(watcher._userAnalysisHoldTimer).not.toBeNull(); + + // Still running at the next check: still held. + await jest.advanceTimersByTimeAsync(5000); + expect(watcher._analyzeFile).not.toHaveBeenCalled(); + + release(); + await jest.advanceTimersByTimeAsync(5000); + expect(watcher._analyzeFile).toHaveBeenCalledTimes(1); + expect(watcher._userAnalysisHoldTimer).toBeNull(); + }); +}); diff --git a/test/useAnalysis.test.js b/test/useAnalysis.test.js index d77f210..3c46c59 100644 --- a/test/useAnalysis.test.js +++ b/test/useAnalysis.test.js @@ -3,7 +3,11 @@ * Tests file analysis logic and state management */ +import * as React from 'react'; import { renderHook, act } from '@testing-library/react'; +// The same module instances the renderer uses; a later test resets the +// registry, after which a require() would load a second React. +import { useAnalysis as useAnalysisOriginal } from '../src/renderer/phases/discover/useAnalysis'; // Mock react-redux jest.mock('react-redux', () => ({ @@ -1046,3 +1050,101 @@ describe('applyAnalysisOutcome naming trust', () => { expect(name).toBe('timeline-1-2026-06-01.mp4'); }); }); + +describe('useAnalysis — a run aborted during start-up', () => { + // Live run (dev build, React StrictMode): Discover remounted, the resume + // started a run, StrictMode's simulated unmount aborted it during its 10 ms + // start-up pause, and the run still sent its batch. An aborted run's + // listeners ignore every event, so no result reached the list ("0 ready") + // until the 5-minute inactivity lock cancelled the batch. + const useAnalysis = (opts) => useAnalysisOriginal(opts); + const files = Array.from({ length: 40 }, (_, i) => ({ + path: `/batch/file-${i}.txt`, + name: `file-${i}.txt` + })); + const setters = () => ({ + setIsAnalyzing: jest.fn(), + setAnalysisProgress: jest.fn(), + setCurrentAnalysisFile: jest.fn(), + setAnalysisResults: jest.fn(), + setFileStates: jest.fn() + }); + const options = (overrides = {}) => ({ + selectedFiles: files, + fileStates: {}, + analysisResults: [], + isAnalyzing: false, + analysisProgress: { current: 0, total: 0 }, + namingSettings: { convention: 'original', separator: '-', dateFormat: 'YYYY-MM-DD' }, + setters: setters(), + updateFileState: jest.fn(), + addNotification: jest.fn(), + actions: { setPhaseData: jest.fn(), advancePhase: jest.fn() }, + ...overrides + }); + + beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers(); + global.window.electronAPI = mockElectronAPI; + mockElectronAPI.analysis.batch.mockResolvedValue({ + success: true, + results: files.map((file) => ({ + filePath: file.path, + success: true, + result: { category: 'documents', suggestedName: `${file.name}-done` } + })), + errors: [], + total: files.length, + successful: files.length + }); + mockElectronAPI.analysis.getReadyQueue.mockResolvedValue({ success: true, readyFiles: [] }); + mockElectronAPI.settings.get.mockResolvedValue({ maxConcurrentAnalysis: 3 }); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + test('an unmount during the start-up pause sends no batch', async () => { + const opts = options(); + const { result, unmount } = renderHook(() => useAnalysis(opts)); + let promise; + await act(async () => { + promise = result.current.analyzeFiles(files); + }); + unmount(); + await act(async () => { + jest.advanceTimersByTime(100); + await promise; + }); + expect(mockElectronAPI.analysis.batch).not.toHaveBeenCalled(); + expect(opts.addNotification).not.toHaveBeenCalledWith( + expect.stringMatching(/complete/i), + expect.anything(), + expect.anything(), + expect.anything() + ); + }); + + test('under StrictMode a resumed run sends one batch and its results reach the list', async () => { + const opts = options({ + isAnalyzing: true, + fileStates: Object.fromEntries(files.map((f) => [f.path, { state: 'pending' }])) + }); + renderHook(() => useAnalysis(opts), { wrapper: React.StrictMode }); + for (let i = 0; i < 10; i += 1) { + await act(async () => { + jest.advanceTimersByTime(500); + }); + } + expect(mockElectronAPI.analysis.batch).toHaveBeenCalledTimes(1); + const applied = opts.setters.setAnalysisResults.mock.calls + .map((call) => call[0]) + .filter(Array.isArray) + .pop(); + expect(applied).toEqual( + expect.arrayContaining([expect.objectContaining({ path: '/batch/file-0.txt' })]) + ); + }); +});