From 5a1a841365a842eab345a70c420380cc00606e2e Mon Sep 17 00:00:00 2001 From: Zack Pollard Date: Sat, 21 Sep 2024 00:16:53 +0100 Subject: [PATCH] fix: rework file handling so we always explicitly create, overwrite or both (#12812) --- server/src/interfaces/storage.interface.ts | 4 +++- server/src/repositories/storage.repository.ts | 12 +++++++++-- server/src/services/metadata.service.spec.ts | 10 ++++----- server/src/services/metadata.service.ts | 2 +- server/src/services/storage.service.spec.ts | 9 ++++++-- server/src/services/storage.service.ts | 21 +++++++++++++++---- .../repositories/storage.repository.mock.ts | 4 +++- 7 files changed, 46 insertions(+), 16 deletions(-) diff --git a/server/src/interfaces/storage.interface.ts b/server/src/interfaces/storage.interface.ts index fec3d66dd5..321f7b8367 100644 --- a/server/src/interfaces/storage.interface.ts +++ b/server/src/interfaces/storage.interface.ts @@ -35,7 +35,9 @@ export interface IStorageRepository { createZipStream(): ImmichZipStream; createReadStream(filepath: string, mimeType?: string | null): Promise; readFile(filepath: string, options?: FileReadOptions): Promise; - writeFile(filepath: string, buffer: Buffer): Promise; + createFile(filepath: string, buffer: Buffer): Promise; + createOrOverwriteFile(filepath: string, buffer: Buffer): Promise; + overwriteFile(filepath: string, buffer: Buffer): Promise; realpath(filepath: string): Promise; unlink(filepath: string): Promise; unlinkDir(folder: string, options?: { recursive?: boolean; force?: boolean }): Promise; diff --git a/server/src/repositories/storage.repository.ts b/server/src/repositories/storage.repository.ts index c699047ce1..6fd9bb8b04 100644 --- a/server/src/repositories/storage.repository.ts +++ b/server/src/repositories/storage.repository.ts @@ -40,8 +40,16 @@ export class StorageRepository implements IStorageRepository { return fs.stat(filepath); } - writeFile(filepath: string, buffer: Buffer) { - return fs.writeFile(filepath, buffer); + createFile(filepath: string, buffer: Buffer) { + return fs.writeFile(filepath, buffer, { flag: 'wx' }); + } + + createOrOverwriteFile(filepath: string, buffer: Buffer) { + return fs.writeFile(filepath, buffer, { flag: 'w' }); + } + + overwriteFile(filepath: string, buffer: Buffer) { + return fs.writeFile(filepath, buffer, { flag: 'r+' }); } rename(source: string, target: string) { diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index 19aaa2ea1a..4eac4a4cf9 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -511,7 +511,7 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.livePhotoMotionAsset.id }); expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoMotionAsset.id]); - expect(storageMock.writeFile).not.toHaveBeenCalled(); + expect(storageMock.createOrOverwriteFile).not.toHaveBeenCalled(); expect(jobMock.queue).not.toHaveBeenCalled(); expect(jobMock.queueAll).not.toHaveBeenCalled(); expect(assetMock.update).not.toHaveBeenCalledWith( @@ -581,7 +581,7 @@ describe(MetadataService.name, () => { type: AssetType.VIDEO, }); expect(userMock.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); - expect(storageMock.writeFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); + expect(storageMock.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); expect(assetMock.update).toHaveBeenNthCalledWith(1, { id: assetStub.livePhotoWithOriginalFileName.id, livePhotoVideoId: fileStub.livePhotoMotion.uuid, @@ -624,7 +624,7 @@ describe(MetadataService.name, () => { type: AssetType.VIDEO, }); expect(userMock.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); - expect(storageMock.writeFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); + expect(storageMock.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); expect(assetMock.update).toHaveBeenNthCalledWith(1, { id: assetStub.livePhotoWithOriginalFileName.id, livePhotoVideoId: fileStub.livePhotoMotion.uuid, @@ -668,7 +668,7 @@ describe(MetadataService.name, () => { type: AssetType.VIDEO, }); expect(userMock.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); - expect(storageMock.writeFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); + expect(storageMock.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); expect(assetMock.update).toHaveBeenNthCalledWith(1, { id: assetStub.livePhotoWithOriginalFileName.id, livePhotoVideoId: fileStub.livePhotoMotion.uuid, @@ -716,7 +716,7 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.livePhotoStillAsset.id }); expect(assetMock.create).toHaveBeenCalledTimes(0); - expect(storageMock.writeFile).toHaveBeenCalledTimes(0); + expect(storageMock.createOrOverwriteFile).toHaveBeenCalledTimes(0); // The still asset gets saved by handleMetadataExtraction, but not the video expect(assetMock.update).toHaveBeenCalledTimes(1); expect(jobMock.queue).toHaveBeenCalledTimes(0); diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index eaa491c3ee..60a1e12a5a 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -529,7 +529,7 @@ export class MetadataService { const existsOnDisk = await this.storageRepository.checkFileExists(motionAsset.originalPath); if (!existsOnDisk) { this.storageCore.ensureFolders(motionAsset.originalPath); - await this.storageRepository.writeFile(motionAsset.originalPath, video); + await this.storageRepository.createFile(motionAsset.originalPath, video); this.logger.log(`Wrote motion photo video to ${motionAsset.originalPath}`); await this.jobRepository.queue({ name: JobName.METADATA_EXTRACTION, data: { id: motionAsset.id } }); } diff --git a/server/src/services/storage.service.spec.ts b/server/src/services/storage.service.spec.ts index b0f38554cb..930fb3c726 100644 --- a/server/src/services/storage.service.spec.ts +++ b/server/src/services/storage.service.spec.ts @@ -41,6 +41,11 @@ describe(StorageService.name, () => { expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/library'); expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/profile'); expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/thumbs'); + expect(storageMock.createFile).toHaveBeenCalledWith('upload/encoded-video/.immich', expect.any(Buffer)); + expect(storageMock.createFile).toHaveBeenCalledWith('upload/library/.immich', expect.any(Buffer)); + expect(storageMock.createFile).toHaveBeenCalledWith('upload/profile/.immich', expect.any(Buffer)); + expect(storageMock.createFile).toHaveBeenCalledWith('upload/thumbs/.immich', expect.any(Buffer)); + expect(storageMock.createFile).toHaveBeenCalledWith('upload/upload/.immich', expect.any(Buffer)); }); it('should throw an error if .immich is missing', async () => { @@ -49,13 +54,13 @@ describe(StorageService.name, () => { await expect(sut.onBootstrap()).rejects.toThrow('Failed to validate folder mount'); - expect(storageMock.writeFile).not.toHaveBeenCalled(); + expect(storageMock.createOrOverwriteFile).not.toHaveBeenCalled(); expect(systemMock.set).not.toHaveBeenCalled(); }); it('should throw an error if .immich is present but read-only', async () => { systemMock.get.mockResolvedValue({ mountFiles: true }); - storageMock.writeFile.mockRejectedValue(new Error("ENOENT: no such file or directory, open '/app/.immich'")); + storageMock.overwriteFile.mockRejectedValue(new Error("ENOENT: no such file or directory, open '/app/.immich'")); await expect(sut.onBootstrap()).rejects.toThrow('Failed to validate folder mount'); diff --git a/server/src/services/storage.service.ts b/server/src/services/storage.service.ts index a8f6a76e74..15328b0c21 100644 --- a/server/src/services/storage.service.ts +++ b/server/src/services/storage.service.ts @@ -32,7 +32,7 @@ export class StorageService { for (const folder of Object.values(StorageFolder)) { if (!flags.mountFiles) { this.logger.log(`Writing initial mount file for the ${folder} folder`); - await this.verifyWriteAccess(folder); + await this.createMountFile(folder); } await this.verifyReadAccess(folder); @@ -81,17 +81,30 @@ export class StorageService { } } - private async verifyWriteAccess(folder: StorageFolder) { + private async createMountFile(folder: StorageFolder) { const { folderPath, filePath } = this.getMountFilePaths(folder); try { this.storageRepository.mkdirSync(folderPath); - await this.storageRepository.writeFile(filePath, Buffer.from(`${Date.now()}`)); + await this.storageRepository.createFile(filePath, Buffer.from(`${Date.now()}`)); + } catch (error) { + this.logger.error(`Failed to create ${filePath}: ${error}`); + this.logger.error( + `The "${folder}" folder cannot be written to, please make sure the volume is mounted with the correct permissions`, + ); + throw new ImmichStartupError(`Failed to validate folder mount (write to "/${folder}")`); + } + } + + private async verifyWriteAccess(folder: StorageFolder) { + const { filePath } = this.getMountFilePaths(folder); + try { + await this.storageRepository.overwriteFile(filePath, Buffer.from(`${Date.now()}`)); } catch (error) { this.logger.error(`Failed to write ${filePath}: ${error}`); this.logger.error( `The "${folder}" folder cannot be written to, please make sure the volume is mounted with the correct permissions`, ); - throw new ImmichStartupError(`Failed to validate folder mount (write to "/${folder}")`); + throw new ImmichStartupError(`Failed to validate folder mount (write to "/${folder}")`); } } diff --git a/server/test/repositories/storage.repository.mock.ts b/server/test/repositories/storage.repository.mock.ts index 5c2951e097..5226e0bb1e 100644 --- a/server/test/repositories/storage.repository.mock.ts +++ b/server/test/repositories/storage.repository.mock.ts @@ -48,7 +48,9 @@ export const newStorageRepositoryMock = (reset = true): Mocked