From 5ead92bb1211b017f449ba0e004d934dd35adf40 Mon Sep 17 00:00:00 2001 From: Jonathan Jogenfors Date: Fri, 20 Feb 2026 12:05:58 +0100 Subject: [PATCH] add error handling --- pnpm-lock.yaml | 10 +- server/package.json | 2 +- server/src/repositories/storage.repository.ts | 3 +- server/src/services/library.service.spec.ts | 6 +- server/src/services/library.service.ts | 18 +++- .../repositories/storage.repository.spec.ts | 94 ++++++++++++++++++- 6 files changed, 119 insertions(+), 14 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 105d6a2af0..0ffb892936 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -344,8 +344,8 @@ importers: specifier: 2.0.0-rc13 version: 2.0.0-rc13 '@immich/walkrs': - specifier: ^0.0.12 - version: 0.0.12 + specifier: ^0.0.13 + version: 0.0.13 '@nestjs/bullmq': specifier: ^11.0.1 version: 11.0.4(@nestjs/common@11.1.13(class-transformer@0.5.1)(class-validator@0.14.3)(reflect-metadata@0.2.2)(rxjs@7.8.2))(@nestjs/core@11.1.13)(bullmq@5.68.0) @@ -3023,8 +3023,8 @@ packages: peerDependencies: svelte: ^5.0.0 - '@immich/walkrs@0.0.12': - resolution: {integrity: sha512-I5ZhLJwYalf24bPX6rsSe6NQC9nKH/Sc/HQ2i7ZfD3dLnXZn7Cq7UQC7d0tVkMSM7rNQCjuL8r7ldUrWg4t2Vg==} + '@immich/walkrs@0.0.13': + resolution: {integrity: sha512-qKDoXFgy3d2Z7SIJBn25BcNyQnPLAp2zZEBcewpWxG5+qAXDPi3M3sweJ9qJ11Eha+YlmpUO3c8yd5CCBeq96A==} engines: {pnpm: '>=10.0.0'} '@inquirer/ansi@1.0.2': @@ -14929,7 +14929,7 @@ snapshots: transitivePeerDependencies: - '@sveltejs/kit' - '@immich/walkrs@0.0.12': {} + '@immich/walkrs@0.0.13': {} '@inquirer/ansi@1.0.2': {} diff --git a/server/package.json b/server/package.json index 814012c58f..ed7ef5996a 100644 --- a/server/package.json +++ b/server/package.json @@ -35,7 +35,7 @@ }, "dependencies": { "@extism/extism": "2.0.0-rc13", - "@immich/walkrs": "^0.0.12", + "@immich/walkrs": "^0.0.13", "@nestjs/bullmq": "^11.0.1", "@nestjs/common": "^11.0.4", "@nestjs/core": "^11.0.4", diff --git a/server/src/repositories/storage.repository.ts b/server/src/repositories/storage.repository.ts index a86d189d89..7d0dadfb62 100644 --- a/server/src/repositories/storage.repository.ts +++ b/server/src/repositories/storage.repository.ts @@ -1,3 +1,4 @@ +import type { WalkItem } from '@immich/walkrs' with { 'resolution-mode': 'import' }; import { Injectable } from '@nestjs/common'; import archiver from 'archiver'; import chokidar, { ChokidarOptions } from 'chokidar'; @@ -197,7 +198,7 @@ export class StorageRepository { }; } - async *walk(walkOptions: WalkOptionsDto): AsyncGenerator { + async *walk(walkOptions: WalkOptionsDto): AsyncGenerator { const { pathsToWalk, exclusionPatterns, includeHidden } = walkOptions; if (pathsToWalk.length === 0) { return; diff --git a/server/src/services/library.service.spec.ts b/server/src/services/library.service.spec.ts index 77aba02c5b..6cb9b1a869 100644 --- a/server/src/services/library.service.spec.ts +++ b/server/src/services/library.service.spec.ts @@ -162,7 +162,7 @@ describe(LibraryService.name, () => { mocks.library.get.mockResolvedValue(library); mocks.storage.walk.mockReturnValue( (async function* () { - yield await Promise.resolve(['/data/user1/photo.jpg']); + yield await Promise.resolve([{ type: 'entry', path: '/data/user1/photo.jpg' }]); })(), ); mocks.storage.stat.mockResolvedValue({ isDirectory: () => true } as Stats); @@ -202,7 +202,7 @@ describe(LibraryService.name, () => { mocks.storage.checkFileExists.mockResolvedValue(true); mocks.storage.walk.mockReturnValue( (async function* () { - yield await Promise.resolve(['/data/user1/photo.jpg']); + yield await Promise.resolve([{ type: 'entry', path: '/data/user1/photo.jpg' }]); })(), ); mocks.library.get.mockResolvedValue(library); @@ -225,7 +225,7 @@ describe(LibraryService.name, () => { mocks.library.get.mockResolvedValue(library); mocks.storage.walk.mockReturnValue( (async function* () { - yield await Promise.resolve(['/data/user1/photo.jpg']); + yield await Promise.resolve([{ type: 'entry', path: '/data/user1/photo.jpg' }]); })(), ); mocks.storage.stat.mockResolvedValue({ isDirectory: () => true } as Stats); diff --git a/server/src/services/library.service.ts b/server/src/services/library.service.ts index 4ca81fa93a..00b658f470 100644 --- a/server/src/services/library.service.ts +++ b/server/src/services/library.service.ts @@ -4,6 +4,7 @@ import { R_OK } from 'node:constants'; import { Stats } from 'node:fs'; import path, { basename, isAbsolute, parse } from 'node:path'; import picomatch from 'picomatch'; + import { JOBS_LIBRARY_PAGINATION_SIZE } from 'src/constants'; import { StorageCore } from 'src/cores/storage.core'; import { OnEvent, OnJob } from 'src/decorators'; @@ -639,7 +640,7 @@ export class LibraryService extends BaseService { this.logger.log(`Starting disk crawl of ${validImportPaths.length} import path(s) for library ${library.id}...`); - const fileGenerator = this.storageRepository.walk({ + const fileWalker = this.storageRepository.walk({ pathsToWalk: validImportPaths, includeHidden: false, // TODO: make this configurable? exclusionPatterns: library.exclusionPatterns, @@ -649,7 +650,20 @@ export class LibraryService extends BaseService { let progressCounter = 0; let lastLoggedMilestone = 0; - for await (const paths of fileGenerator) { + for await (const walkItems of fileWalker) { + const paths: string[] = []; + for (const item of walkItems) { + if (item.type === 'error') { + this.logger.warn(`Error walking ${item.path ?? 'unknown path'}: ${item.message}`); + } else { + paths.push(item.path); + } + } + + if (paths.length === 0) { + continue; + } + progressCounter += paths.length; await this.jobRepository.queue({ diff --git a/server/test/medium/specs/repositories/storage.repository.spec.ts b/server/test/medium/specs/repositories/storage.repository.spec.ts index a6b83ce080..2e4fbb63a6 100644 --- a/server/test/medium/specs/repositories/storage.repository.spec.ts +++ b/server/test/medium/specs/repositories/storage.repository.spec.ts @@ -1,7 +1,8 @@ +import type { WalkError, WalkItem } from '@immich/walkrs' with { 'resolution-mode': 'import' }; import { Kysely } from 'kysely'; import fs from 'node:fs/promises'; import os from 'node:os'; -import path from 'node:path'; +import path, { join } from 'node:path'; import { WalkOptionsDto } from 'src/dtos/library.dto'; import { LoggingRepository } from 'src/repositories/logging.repository'; import { StorageRepository } from 'src/repositories/storage.repository'; @@ -229,7 +230,11 @@ describe(StorageRepository.name, () => { const actual: string[] = []; for await (const batch of sut.walk(adjustedOptions)) { - actual.push(...batch); + for (const item of batch) { + if (item.type === 'entry') { + actual.push(item.path); + } + } } const expected = Object.entries(files) .filter((entry) => entry[1]) @@ -239,5 +244,90 @@ describe(StorageRepository.name, () => { }); }); } + + it('should handle access denied errors gracefully', async () => { + const testDir = await fs.mkdtemp(join(os.tmpdir(), 'immich-test-access-denied-')); + const restrictedDir = join(testDir, 'restricted'); + const restrictedFile = join(restrictedDir, 'file.jpg'); + const accessibleFile = join(testDir, 'accessible.jpg'); + + try { + // Create test directory structure + await fs.mkdir(restrictedDir, { recursive: true }); + await fs.writeFile(accessibleFile, 'accessible content'); + await fs.writeFile(restrictedFile, 'restricted content'); + + // Remove all permissions from restricted directory to simulate access denied + await fs.chmod(restrictedDir, 0o000); + + const actual: string[] = []; + const errors: WalkItem[] = []; + for await (const batch of sut.walk({ pathsToWalk: [testDir] })) { + for (const item of batch) { + if (item.type === 'entry') { + actual.push(item.path); + } else { + errors.push(item); + } + } + } + + // Should successfully walk accessible file but skip restricted directory + expect(actual).toContain(accessibleFile); + expect(actual).not.toContain(restrictedFile); + // Should have encountered an error for the restricted directory + expect(errors.length).toBe(1); + expect(errors.some((e) => e.type === 'error' && e.message?.includes('restricted'))).toBe(true); + } finally { + // Cleanup: restore permissions before deletion + try { + await fs.chmod(restrictedDir, 0o755); + } catch { + // Ignore errors if directory was already deleted or permissions cannot be restored + } + await fs.rm(testDir, { recursive: true, force: true }); + } + }); + + it('should return error details for access denied paths', async () => { + const testDir = await fs.mkdtemp(join(os.tmpdir(), 'immich-test-access-denied-')); + const restrictedDir = join(testDir, 'restricted'); + const restrictedFile = join(restrictedDir, 'file.jpg'); + const accessibleFile = join(testDir, 'accessible.jpg'); + + try { + // Create test directory structure + await fs.mkdir(restrictedDir, { recursive: true }); + await fs.writeFile(accessibleFile, 'accessible content'); + await fs.writeFile(restrictedFile, 'restricted content'); + + // Remove all permissions from restricted directory to simulate access denied + await fs.chmod(restrictedDir, 0o000); + + const errors: WalkError[] = []; + for await (const batch of sut.walk({ pathsToWalk: [testDir] })) { + for (const item of batch) { + if (item.type === 'error') { + errors.push(item); + } + } + } + + // Should have error details including path and message + expect(errors.length).toBe(1); + const restrictedError = errors.find((e) => e.type === 'error' && e.message?.includes('restricted')); + expect(restrictedError).toBeDefined(); + expect(restrictedError?.type).toBe('error'); + expect(restrictedError?.message).toBeDefined(); + } finally { + // Cleanup: restore permissions before deletion + try { + await fs.chmod(restrictedDir, 0o755); + } catch { + // Ignore errors if directory was already deleted or permissions cannot be restored + } + await fs.rm(testDir, { recursive: true, force: true }); + } + }); }); });