From 76c08fa230b8c9fc2fbdd99b839ed4a3d357df12 Mon Sep 17 00:00:00 2001 From: Jonathan Jogenfors Date: Mon, 28 Sep 2026 21:51:19 +0200 Subject: [PATCH] fix: library watcher should respect ignore patterns (#31418) * fix: chokidar should respect ignore patterns * fix: chokidar mock --------- Co-authored-by: Jason Rasmussen Co-authored-by: Daniel Dietzler --- .../repositories/storage.repository.spec.ts | 75 ++++++++++++++++ server/src/repositories/storage.repository.ts | 14 ++- server/src/services/library.service.spec.ts | 18 ++++ server/src/services/library.service.ts | 1 + .../repositories/storage.repository.spec.ts | 88 +++++++++++++++++++ .../specs/services/library.service.spec.ts | 32 ++++++- 6 files changed, 225 insertions(+), 3 deletions(-) create mode 100644 server/test/medium/specs/repositories/storage.repository.spec.ts diff --git a/server/src/repositories/storage.repository.spec.ts b/server/src/repositories/storage.repository.spec.ts index aca530835a..3d1e503ed4 100644 --- a/server/src/repositories/storage.repository.spec.ts +++ b/server/src/repositories/storage.repository.spec.ts @@ -1,9 +1,30 @@ import mockfs from 'mock-fs'; +import { vitest } from 'vitest'; import { CrawlOptionsDto } from 'src/dtos/library.dto.js'; import { LoggingRepository } from 'src/repositories/logging.repository.js'; import { StorageRepository } from 'src/repositories/storage.repository.js'; import { automock } from 'test/utils.js'; +const mocks = vitest.hoisted(() => { + const watcher = { + close: vitest.fn(), + on: vitest.fn(), + }; + watcher.on.mockReturnValue(watcher); + + return { watch: vitest.fn(() => watcher), watcher }; +}); + +vitest.mock('chokidar', () => ({ watch: mocks.watch })); + +const getHandler = (event: string) => { + const handler = mocks.watcher.on.mock.calls.find(([name]) => name === event)?.[1]; + if (!handler) { + throw new Error(`Missing ${event} handler`); + } + return handler; +}; + interface Test { test: string; options: CrawlOptionsDto; @@ -185,6 +206,9 @@ describe(StorageRepository.name, () => { beforeEach(() => { // eslint-disable-next-line no-sparse-arrays sut = new StorageRepository(automock(LoggingRepository, { args: [, { getEnv: () => ({}) }], strict: false })); + mocks.watch.mockReset().mockReturnValue(mocks.watcher); + mocks.watcher.on.mockReset().mockReturnValue(mocks.watcher); + mocks.watcher.close.mockReset().mockResolvedValue(undefined); }); afterEach(() => { @@ -206,4 +230,55 @@ describe(StorageRepository.name, () => { }); } }); + + describe('watch', () => { + it('should register event handlers and close the watcher', async () => { + const onReady = vitest.fn(); + const onAdd = vitest.fn(); + const onChange = vitest.fn(); + const onUnlink = vitest.fn(); + const onError = vitest.fn(); + const close = sut.watch(['/photos'], {}, { onAdd, onChange, onError, onReady, onUnlink }); + + expect(mocks.watch).toHaveBeenCalledWith(['/photos'], expect.objectContaining({ ignored: expect.any(Function) })); + + const error = new Error('watch error'); + getHandler('ready')(); + getHandler('add')('/photos/add.jpg'); + getHandler('change')('/photos/change.jpg'); + getHandler('unlink')('/photos/unlink.jpg'); + getHandler('error')(error); + + expect(onReady).toHaveBeenCalledWith(); + expect(onAdd).toHaveBeenCalledWith('/photos/add.jpg'); + expect(onChange).toHaveBeenCalledWith('/photos/change.jpg'); + expect(onUnlink).toHaveBeenCalledWith('/photos/unlink.jpg'); + expect(onError).toHaveBeenCalledWith(error); + + await close(); + expect(mocks.watcher.close).toHaveBeenCalledWith(); + }); + + it('should convert ignored glob arrays to a case-insensitive matcher', () => { + sut.watch(['/photos'], { ignored: ['**/excluded/**'] }, {}); + const [, options] = mocks.watch.mock.lastCall as unknown as [string[], { ignored?: unknown }]; + const ignored = options.ignored as (path: string) => boolean; + + expect(typeof ignored).toBe('function'); + expect(ignored('/photos/EXCLUDED/photo.jpg')).toBe(true); + expect(ignored('/photos/included/photo.jpg')).toBe(false); + }); + + it('should tolerate missing event callbacks', () => { + sut.watch(['/photos'], {}, {}); + + expect(() => { + getHandler('ready')(); + getHandler('add')('/photos/add.jpg'); + getHandler('change')('/photos/change.jpg'); + getHandler('unlink')('/photos/unlink.jpg'); + getHandler('error')(new Error('watch error')); + }).not.toThrow(); + }); + }); }); diff --git a/server/src/repositories/storage.repository.ts b/server/src/repositories/storage.repository.ts index 8464f19ec1..eafc3346bd 100644 --- a/server/src/repositories/storage.repository.ts +++ b/server/src/repositories/storage.repository.ts @@ -16,6 +16,7 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import { PassThrough, Readable, Writable } from 'node:stream'; import { createGunzip, createGzip } from 'node:zlib'; +import picomatch from 'picomatch'; import { CrawlOptionsDto, WalkOptionsDto } from 'src/dtos/library.dto.js'; import { LoggingRepository } from 'src/repositories/logging.repository.js'; import { mimeTypes } from 'src/utils/mime-types.js'; @@ -28,6 +29,8 @@ export interface WatchEvents { onError(error: Error): void; } +export type WatchOptions = Omit & { ignored?: string[] }; + export interface ImmichReadStream { stream: Readable; type?: string; @@ -273,8 +276,15 @@ export class StorageRepository { } } - watch(paths: string[], options: ChokidarOptions, events: Partial) { - const watcher = chokidarWatch(paths, options); + watch(paths: string[], options: WatchOptions, events: Partial) { + const matchesIgnoredPath = picomatch(options.ignored ?? [], { + dot: true, // Match the behavior of fast-glob's micromatch by using these settings + nocase: true, + posix: true, + strictSlashes: false, + }); + + const watcher = chokidarWatch(paths, { ...options, ignored: (path) => matchesIgnoredPath(path) }); watcher.on('ready', () => events.onReady?.()); watcher.on('add', (path) => events.onAdd?.(path)); diff --git a/server/src/services/library.service.spec.ts b/server/src/services/library.service.spec.ts index 83b1a123f2..53b3e7035c 100644 --- a/server/src/services/library.service.spec.ts +++ b/server/src/services/library.service.spec.ts @@ -942,6 +942,24 @@ describe(LibraryService.name, () => { expect(mocks.storage.watch).toHaveBeenCalledWith(library.importPaths, expect.anything(), expect.anything()); }); + it('should exclude paths from the watcher', async () => { + const library = factory.library({ + importPaths: ['/foo', '/bar'], + exclusionPatterns: ['**/excluded/**'], + }); + + mocks.library.get.mockResolvedValue(library); + mocks.library.getAll.mockResolvedValue([library]); + + await sut.watchAll(); + + expect(mocks.storage.watch).toHaveBeenCalledWith( + library.importPaths, + expect.objectContaining({ ignored: library.exclusionPatterns }), + expect.anything(), + ); + }); + it('should watch and unwatch library', async () => { const library = factory.library({ importPaths: ['/foo', '/bar'] }); diff --git a/server/src/services/library.service.ts b/server/src/services/library.service.ts index d86589a181..1a8fb35569 100644 --- a/server/src/services/library.service.ts +++ b/server/src/services/library.service.ts @@ -135,6 +135,7 @@ export class LibraryService extends BaseService { { usePolling: false, ignoreInitial: true, + ignored: library.exclusionPatterns, awaitWriteFinish: { stabilityThreshold: 5000, pollInterval: 1000, diff --git a/server/test/medium/specs/repositories/storage.repository.spec.ts b/server/test/medium/specs/repositories/storage.repository.spec.ts new file mode 100644 index 0000000000..bf34c5c256 --- /dev/null +++ b/server/test/medium/specs/repositories/storage.repository.spec.ts @@ -0,0 +1,88 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { LoggingRepository } from 'src/repositories/logging.repository.js'; +import { StorageRepository, type WatchOptions } from 'src/repositories/storage.repository.js'; +import { automock } from 'test/utils.js'; + +describe(StorageRepository.name, () => { + const annoyingExclusionPatterns = ['@', '#', '$', '%', '^', '&', '=']; + let sut: StorageRepository; + let tempDir: string; + let includedFile: string; + let excludedFile: string; + + beforeEach(async () => { + // eslint-disable-next-line no-sparse-arrays + sut = new StorageRepository(automock(LoggingRepository, { args: [, { getEnv: () => ({}) }], strict: false })); + tempDir = await mkdtemp(join(tmpdir(), 'immich-storage-watch-')); + includedFile = join(tempDir, 'included/photo.jpg'); + excludedFile = join(tempDir, 'excluded/photo.jpg'); + + await Promise.all([ + mkdir(join(tempDir, 'included'), { recursive: true }), + mkdir(join(tempDir, 'excluded'), { recursive: true }), + ]); + await Promise.all([writeFile(includedFile, 'included'), writeFile(excludedFile, 'excluded')]); + }); + + afterEach(async () => { + await rm(tempDir, { recursive: true, force: true }); + }); + + const startWatching = (options: WatchOptions) => { + const paths: string[] = []; + let resolveReady!: () => void; + + const ready = new Promise((resolve) => (resolveReady = resolve)); + const close = sut.watch([tempDir], options, { + onAdd: (path) => { + paths.push(path); + }, + onReady: resolveReady, + }); + + return { close, paths, ready }; + }; + + it('should emit add events for initial files', async () => { + const watcher = startWatching({ ignoreInitial: false }); + + try { + await watcher.ready; + + expect(watcher.paths.toSorted()).toEqual([includedFile, excludedFile].toSorted()); + } finally { + await watcher.close(); + } + }); + + it('should not emit add events for ignored initial files', async () => { + const watcher = startWatching({ ignoreInitial: false, ignored: ['**/excluded/**'] }); + + try { + await watcher.ready; + + expect(watcher.paths).toEqual([includedFile]); + } finally { + await watcher.close(); + } + }); + + it.each(annoyingExclusionPatterns)('should ignore folders with %s in their name', async (char) => { + const ignoredFolder = `${char}folder`; + const ignoredFile = join(tempDir, ignoredFolder, 'photo.jpg'); + await mkdir(join(tempDir, ignoredFolder), { recursive: true }); + await writeFile(ignoredFile, 'ignored'); + + const watcher = startWatching({ ignoreInitial: false, ignored: [`**/${ignoredFolder}/**`] }); + + try { + await watcher.ready; + + expect(watcher.paths.toSorted()).toEqual([includedFile, excludedFile].toSorted()); + } finally { + await watcher.close(); + } + }); +}); diff --git a/server/test/medium/specs/services/library.service.spec.ts b/server/test/medium/specs/services/library.service.spec.ts index 395ffc92ec..42730db2d3 100644 --- a/server/test/medium/specs/services/library.service.spec.ts +++ b/server/test/medium/specs/services/library.service.spec.ts @@ -3,11 +3,14 @@ import { existsSync } from 'node:fs'; import { copyFile, mkdir, mkdtemp, rm, utimes, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; +import type { SystemConfig } from 'src/dtos/config.dto.js'; import { StorageCore } from 'src/cores/storage.core.js'; import { AssetStatus, JobName, JobStatus } from 'src/enum.js'; import { AssetJobRepository } from 'src/repositories/asset-job.repository.js'; import { AssetRepository } from 'src/repositories/asset.repository.js'; +import { CronRepository } from 'src/repositories/cron.repository.js'; import { CryptoRepository } from 'src/repositories/crypto.repository.js'; +import { DatabaseRepository } from 'src/repositories/database.repository.js'; import { EventRepository } from 'src/repositories/event.repository.js'; import { JobRepository } from 'src/repositories/job.repository.js'; import { LibraryRepository } from 'src/repositories/library.repository.js'; @@ -15,6 +18,7 @@ import { LoggingRepository } from 'src/repositories/logging.repository.js'; import { StorageRepository } from 'src/repositories/storage.repository.js'; import { DB } from 'src/schema/index.js'; import { LibraryService } from 'src/services/library.service.js'; +import { systemConfigStub } from 'test/fixtures/system-config.stub.js'; import { MediumTestContext, testAssetsDir } from 'test/medium.factory.js'; import { newUuid } from 'test/small.factory.js'; import { getKyselyDB } from 'test/utils.js'; @@ -45,7 +49,7 @@ class LibraryTestContext extends MediumTestContext { super(LibraryService, { database, real: [AssetRepository, AssetJobRepository, CryptoRepository, LibraryRepository, StorageRepository], - mock: [EventRepository, JobRepository, LoggingRepository], + mock: [CronRepository, DatabaseRepository, EventRepository, JobRepository, LoggingRepository], }); const jobs = this.getMock(JobRepository); @@ -518,6 +522,32 @@ describe(LibraryService.name, () => { }); }); + describe('watch', () => { + it('should pass exclusion patterns to the library watcher', async () => { + const { sut, ctx } = setup(); + const library = await ctx.createLibrary({ + importPaths: [importRoot], + exclusionPatterns: ['**/excluded/**'], + }); + const storage = ctx.get(StorageRepository); + const watch = vitest.spyOn(storage, 'watch'); + + ctx.getMock(DatabaseRepository).tryLock.mockResolvedValue(true); + + try { + await sut.onConfigInit({ newConfig: systemConfigStub.libraryWatchEnabled as SystemConfig }); + + expect(watch).toHaveBeenCalledWith( + library.importPaths, + expect.objectContaining({ ignored: library.exclusionPatterns }), + expect.anything(), + ); + } finally { + await sut.onShutdown(); + } + }); + }); + describe('handleQueueSyncAssets', () => { it('should set an asset offline if its file is not in any import path', async () => { const { sut, ctx } = setup();