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

server/src/domain/repositories/database.repository.ts

@@ -8,6 +8,7 @@ export enum DatabaseExtension {
 
 export enum DatabaseLock {
   GeodataImport = 100,
+  StorageTemplateMigration = 420,
   CLIPDimSize = 512,
 }
 

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<IStorageRepository>;
   let userMock: jest.Mocked<IUserRepository>;
   let cryptoMock: jest.Mocked<ICryptoRepository>;
+  let databaseRepository: jest.Mocked<IDatabaseRepository>;
 
   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 }]);

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;
+    return this.databaseRepository.withLock(DatabaseLock.StorageTemplateMigration, async () => {
-    const oldPath = originalPath;
+      const { id, sidecarPath, originalPath, exifInfo } = asset;
-    const newPath = await this.getTemplatePath(asset, metadata);
+      const oldPath = originalPath;
+      const newPath = await this.getTemplatePath(asset, metadata);
 
-    if (!exifInfo || !exifInfo.fileSizeInByte) {
+      if (!exifInfo || !exifInfo.fileSizeInByte) {
-      this.logger.error(`Asset ${id} missing exif info, skipping storage template migration`);
+        this.logger.error(`Asset ${id} missing exif info, skipping storage template migration`);
-      return;
+        return;
-    }
+      }
 
-    try {
+      try {
-      await this.storageCore.moveFile({
-        entityId: id,
-        pathType: AssetPathType.ORIGINAL,
-        oldPath,
-        newPath,
-        assetInfo: { sizeInBytes: exifInfo.fileSizeInByte, checksum: asset.checksum },
-      });
-      if (sidecarPath) {
         await this.storageCore.moveFile({
           entityId: id,
-          pathType: AssetPathType.SIDECAR,
+          pathType: AssetPathType.ORIGINAL,
-          oldPath: sidecarPath,
+          oldPath,
-          newPath: `${newPath}.xmp`,
+          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<string> {

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<R>(lock: DatabaseLock, callback: () => Promise<R>): Promise<R> {
     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<void> {
+  private async acquireLock(lock: DatabaseLock, queryRunner: QueryRunner): Promise<void> {
-    return this.dataSource.query('SELECT pg_advisory_lock($1)', [lock]);
+    return queryRunner.query('SELECT pg_advisory_lock($1)', [lock]);
   }
 
-  private async releaseLock(lock: DatabaseLock): Promise<void> {
+  private async releaseLock(lock: DatabaseLock, queryRunner: QueryRunner): Promise<void> {
-    return this.dataSource.query('SELECT pg_advisory_unlock($1)', [lock]);
+    return queryRunner.query('SELECT pg_advisory_unlock($1)', [lock]);
   }
 }