From ca9cad20bcec06bbb59b9a7c37f8cc16817bc36f Mon Sep 17 00:00:00 2001 From: Zack Pollard Date: Sat, 30 Dec 2023 15:09:33 +0000 Subject: [PATCH] feat: storage template locking + fix for database locks (#6054) * fix: locks need to be acquired and released using the same session * feat: only allow a single storage template operation at a single time this has been added to avoid possible rare race conditions where two files are moved at once to the same path with the same name, causing our duplicate iterator to not detect them and therefore both files will have the same name and overwrite eachother --- .../repositories/database.repository.ts | 1 + .../storage-template.service.spec.ts | 5 ++ .../storage-template.service.ts | 50 ++++++++++--------- .../infra/repositories/database.repository.ts | 19 ++++--- 4 files changed, 45 insertions(+), 30 deletions(-) diff --git a/server/src/domain/repositories/database.repository.ts b/server/src/domain/repositories/database.repository.ts index e69b3fc2fb..496081ddb2 100644 --- a/server/src/domain/repositories/database.repository.ts +++ b/server/src/domain/repositories/database.repository.ts @@ -8,6 +8,7 @@ export enum DatabaseExtension { export enum DatabaseLock { GeodataImport = 100, + StorageTemplateMigration = 420, CLIPDimSize = 512, } diff --git a/server/src/domain/storage-template/storage-template.service.spec.ts b/server/src/domain/storage-template/storage-template.service.spec.ts index 37c21827a2..68d0152345 100644 --- a/server/src/domain/storage-template/storage-template.service.spec.ts +++ b/server/src/domain/storage-template/storage-template.service.spec.ts @@ -2,6 +2,7 @@ import { IAlbumRepository, IAssetRepository, ICryptoRepository, + IDatabaseRepository, IMoveRepository, IPersonRepository, IStorageRepository, @@ -16,6 +17,7 @@ import { newAlbumRepositoryMock, newAssetRepositoryMock, newCryptoRepositoryMock, + newDatabaseRepositoryMock, newMoveRepositoryMock, newPersonRepositoryMock, newStorageRepositoryMock, @@ -36,6 +38,7 @@ describe(StorageTemplateService.name, () => { let storageMock: jest.Mocked; let userMock: jest.Mocked; let cryptoMock: jest.Mocked; + let databaseRepository: jest.Mocked; it('should work', () => { expect(sut).toBeDefined(); @@ -50,6 +53,7 @@ describe(StorageTemplateService.name, () => { storageMock = newStorageRepositoryMock(); userMock = newUserRepositoryMock(); cryptoMock = newCryptoRepositoryMock(); + databaseRepository = newDatabaseRepositoryMock(); sut = new StorageTemplateService( albumMock, @@ -61,6 +65,7 @@ describe(StorageTemplateService.name, () => { storageMock, userMock, cryptoMock, + databaseRepository, ); configMock.load.mockResolvedValue([{ key: SystemConfigKey.STORAGE_TEMPLATE_ENABLED, value: true }]); diff --git a/server/src/domain/storage-template/storage-template.service.ts b/server/src/domain/storage-template/storage-template.service.ts index 86ea0ae365..c8d2d3e875 100644 --- a/server/src/domain/storage-template/storage-template.service.ts +++ b/server/src/domain/storage-template/storage-template.service.ts @@ -8,9 +8,11 @@ import sanitize from 'sanitize-filename'; import { getLivePhotoMotionFilename, usePagination } from '../domain.util'; import { IEntityJob, JOBS_ASSET_PAGINATION_SIZE } from '../job'; import { + DatabaseLock, IAlbumRepository, IAssetRepository, ICryptoRepository, + IDatabaseRepository, IMoveRepository, IPersonRepository, IStorageRepository, @@ -63,6 +65,7 @@ export class StorageTemplateService { @Inject(IStorageRepository) private storageRepository: IStorageRepository, @Inject(IUserRepository) private userRepository: IUserRepository, @Inject(ICryptoRepository) private cryptoRepository: ICryptoRepository, + @Inject(IDatabaseRepository) private databaseRepository: IDatabaseRepository, ) { this.template = this.compile(config.storageTemplate.template); this.configCore = SystemConfigCore.create(configRepository); @@ -101,7 +104,6 @@ export class StorageTemplateService { const motionFilename = getLivePhotoMotionFilename(filename, livePhotoVideo.originalPath); await this.moveAsset(livePhotoVideo, { storageLabel, filename: motionFilename }); } - return true; } @@ -142,34 +144,36 @@ export class StorageTemplateService { return; } - const { id, sidecarPath, originalPath, exifInfo } = asset; - const oldPath = originalPath; - const newPath = await this.getTemplatePath(asset, metadata); + return this.databaseRepository.withLock(DatabaseLock.StorageTemplateMigration, async () => { + const { id, sidecarPath, originalPath, exifInfo } = asset; + const oldPath = originalPath; + const newPath = await this.getTemplatePath(asset, metadata); - if (!exifInfo || !exifInfo.fileSizeInByte) { - this.logger.error(`Asset ${id} missing exif info, skipping storage template migration`); - return; - } + if (!exifInfo || !exifInfo.fileSizeInByte) { + this.logger.error(`Asset ${id} missing exif info, skipping storage template migration`); + return; + } - try { - await this.storageCore.moveFile({ - entityId: id, - pathType: AssetPathType.ORIGINAL, - oldPath, - newPath, - assetInfo: { sizeInBytes: exifInfo.fileSizeInByte, checksum: asset.checksum }, - }); - if (sidecarPath) { + try { await this.storageCore.moveFile({ entityId: id, - pathType: AssetPathType.SIDECAR, - oldPath: sidecarPath, - newPath: `${newPath}.xmp`, + pathType: AssetPathType.ORIGINAL, + oldPath, + newPath, + assetInfo: { sizeInBytes: exifInfo.fileSizeInByte, checksum: asset.checksum }, }); + if (sidecarPath) { + await this.storageCore.moveFile({ + entityId: id, + pathType: AssetPathType.SIDECAR, + oldPath: sidecarPath, + newPath: `${newPath}.xmp`, + }); + } + } catch (error: any) { + this.logger.error(`Problem applying storage template`, error?.stack, { id: asset.id, oldPath, newPath }); } - } catch (error: any) { - this.logger.error(`Problem applying storage template`, error?.stack, { id: asset.id, oldPath, newPath }); - } + }); } private async getTemplatePath(asset: AssetEntity, metadata: MoveAssetMetadata): Promise { diff --git a/server/src/infra/repositories/database.repository.ts b/server/src/infra/repositories/database.repository.ts index 778d61bd44..af595057e2 100644 --- a/server/src/infra/repositories/database.repository.ts +++ b/server/src/infra/repositories/database.repository.ts @@ -2,7 +2,7 @@ import { DatabaseExtension, DatabaseLock, IDatabaseRepository, Version } from '@ import { Injectable } from '@nestjs/common'; import { InjectDataSource } from '@nestjs/typeorm'; import AsyncLock from 'async-lock'; -import { DataSource } from 'typeorm'; +import { DataSource, QueryRunner } from 'typeorm'; @Injectable() export class DatabaseRepository implements IDatabaseRepository { @@ -32,11 +32,16 @@ export class DatabaseRepository implements IDatabaseRepository { async withLock(lock: DatabaseLock, callback: () => Promise): Promise { let res; await this.asyncLock.acquire(DatabaseLock[lock], async () => { + const queryRunner = this.dataSource.createQueryRunner(); try { - await this.acquireLock(lock); + await this.acquireLock(lock, queryRunner); res = await callback(); } finally { - await this.releaseLock(lock); + try { + await this.releaseLock(lock, queryRunner); + } finally { + await queryRunner.release(); + } } }); @@ -51,11 +56,11 @@ export class DatabaseRepository implements IDatabaseRepository { await this.asyncLock.acquire(DatabaseLock[lock], () => {}); } - private async acquireLock(lock: DatabaseLock): Promise { - return this.dataSource.query('SELECT pg_advisory_lock($1)', [lock]); + private async acquireLock(lock: DatabaseLock, queryRunner: QueryRunner): Promise { + return queryRunner.query('SELECT pg_advisory_lock($1)', [lock]); } - private async releaseLock(lock: DatabaseLock): Promise { - return this.dataSource.query('SELECT pg_advisory_unlock($1)', [lock]); + private async releaseLock(lock: DatabaseLock, queryRunner: QueryRunner): Promise { + return queryRunner.query('SELECT pg_advisory_unlock($1)', [lock]); } }