fix(server): deleting stacked assets (#25874)

* fix(server): deleting stacked assets

* fix: log a warning when removing an empty directory fails

server/src/queries/asset.job.repository.sql

@@ -430,30 +430,6 @@ select
   "asset"."originalPath",
   "asset"."isOffline",
   to_json("asset_exif") as "exifInfo",
-  (
-    select
-      coalesce(json_agg(agg), '[]')
-    from
-      (
-        select
-          "asset_face".*,
-          "person" as "person"
-        from
-          "asset_face"
-          left join lateral (
-            select
-              "person".*
-            from
-              "person"
-            where
-              "asset_face"."personId" = "person"."id"
-          ) as "person" on true
-        where
-          "asset_face"."assetId" = "asset"."id"
-          and "asset_face"."deletedAt" is null
-          and "asset_face"."isVisible" is true
-      ) as agg
-  ) as "faces",
   (
     select
       coalesce(json_agg(agg), '[]')
@@ -470,27 +446,37 @@ select
           "asset_file"."assetId" = "asset"."id"
       ) as agg
   ) as "files",
-  to_json("stacked_assets") as "stack"
+  to_json("stack_result") as "stack"
 from
   "asset"
   left join "asset_exif" on "asset"."id" = "asset_exif"."assetId"
-  left join "stack" on "stack"."id" = "asset"."stackId"
   left join lateral (
     select
       "stack"."id",
       "stack"."primaryAssetId",
-      array_agg("stacked") as "assets"
+      (
+        select
+          coalesce(json_agg(agg), '[]')
+        from
+          (
+            select
+              "stack_asset"."id"
+            from
+              "asset" as "stack_asset"
+            where
+              "stack_asset"."stackId" = "stack"."id"
+              and "stack_asset"."id" != "stack"."primaryAssetId"
+              and "stack_asset"."visibility" = $1
+              and "stack_asset"."status" != $2
+          ) as agg
+      ) as "assets"
     from
-      "asset" as "stacked"
+      "stack"
     where
-      "stacked"."deletedAt" is not null
+      "stack"."id" = "asset"."stackId"
-      and "stacked"."visibility" = $1
+  ) as "stack_result" on true
-      and "stacked"."stackId" = "stack"."id"
-    group by
-      "stack"."id"
-  ) as "stacked_assets" on "stack"."id" is not null
 where
-  "asset"."id" = $2
+  "asset"."id" = $3
 
 -- AssetJobRepository.streamForVideoConversion
 select

server/src/repositories/asset-job.repository.ts

@@ -1,10 +1,10 @@
 import { Injectable } from '@nestjs/common';
-import { Kysely } from 'kysely';
+import { Kysely, sql } from 'kysely';
 import { jsonArrayFrom } from 'kysely/helpers/postgres';
 import { InjectKysely } from 'nestjs-kysely';
-import { Asset, columns } from 'src/database';
+import { columns } from 'src/database';
 import { DummyValue, GenerateSql } from 'src/decorators';
-import { AssetFileType, AssetType, AssetVisibility } from 'src/enum';
+import { AssetFileType, AssetStatus, AssetType, AssetVisibility } from 'src/enum';
 import { DB } from 'src/schema';
 import {
   anyUuid,
@@ -15,7 +15,6 @@ import {
   withExif,
   withExifInner,
   withFaces,
-  withFacesAndPeople,
   withFilePath,
   withFiles,
 } from 'src/utils/database';
@@ -269,23 +268,29 @@ export class AssetJobRepository {
         'asset.isOffline',
       ])
       .$call(withExif)
-      .select(withFacesAndPeople)
       .select(withFiles)
-      .leftJoin('stack', 'stack.id', 'asset.stackId')
       .leftJoinLateral(
         (eb) =>
           eb
-            .selectFrom('asset as stacked')
+            .selectFrom('stack')
-            .select(['stack.id', 'stack.primaryAssetId'])
+            .whereRef('stack.id', '=', 'asset.stackId')
-            .select((eb) => eb.fn<Asset[]>('array_agg', [eb.table('stacked')]).as('assets'))
+            .select((eb) => [
-            .where('stacked.deletedAt', 'is not', null)
+              'stack.id',
-            .where('stacked.visibility', '=', AssetVisibility.Timeline)
+              'stack.primaryAssetId',
-            .whereRef('stacked.stackId', '=', 'stack.id')
+              jsonArrayFrom(
-            .groupBy('stack.id')
+                eb
-            .as('stacked_assets'),
+                  .selectFrom('asset as stack_asset')
-        (join) => join.on('stack.id', 'is not', null),
+                  .select(['stack_asset.id'])
+                  .whereRef('stack_asset.stackId', '=', 'stack.id')
+                  .whereRef('stack_asset.id', '!=', 'stack.primaryAssetId')
+                  .where('stack_asset.visibility', '=', sql.val(AssetVisibility.Timeline))
+                  .where('stack_asset.status', '!=', sql.val(AssetStatus.Deleted)),
+              ).as('assets'),
+            ])
+            .as('stack_result'),
+        (join) => join.onTrue(),
       )
-      .select((eb) => toJson(eb, 'stacked_assets').as('stack'))
+      .select((eb) => toJson(eb, 'stack_result').as('stack'))
       .where('asset.id', '=', id)
       .executeTakeFirst();
   }

server/src/repositories/storage.repository.ts

@@ -152,7 +152,7 @@ export class StorageRepository {
   }
 
   async unlinkDir(folder: string, options: { recursive?: boolean; force?: boolean }) {
-    await fs.rm(folder, options);
+    await fs.rm(folder, { ...options, maxRetries: 5, retryDelay: 100 });
   }
 
   async removeEmptyDirs(directory: string, self: boolean = false) {
@@ -168,7 +168,13 @@ export class StorageRepository {
     if (self) {
       const updated = await fs.readdir(directory);
       if (updated.length === 0) {
-        await fs.rmdir(directory);
+        try {
+          await fs.rmdir(directory);
+        } catch (error: Error | any) {
+          if (error.code !== 'ENOTEMPTY') {
+            this.logger.warn(`Attempted to remove directory, but failed: ${error}`);
+          }
+        }
       }
     }
   }

server/src/services/asset.service.spec.ts

@@ -8,7 +8,6 @@ import { AssetStats } from 'src/repositories/asset.repository';
 import { AssetService } from 'src/services/asset.service';
 import { assetStub } from 'test/fixtures/asset.stub';
 import { authStub } from 'test/fixtures/auth.stub';
-import { faceStub } from 'test/fixtures/face.stub';
 import { userStub } from 'test/fixtures/user.stub';
 import { factory } from 'test/small.factory';
 import { makeStream, newTestService, ServiceMocks } from 'test/utils';
@@ -565,12 +564,11 @@ describe(AssetService.name, () => {
   });
 
   describe('handleAssetDeletion', () => {
-    it('should remove faces', async () => {
+    it('should clean up files', async () => {
-      const assetWithFace = { ...assetStub.image, faces: [faceStub.face1, faceStub.mergeFace1] };
+      const asset = assetStub.image;
+      mocks.assetJob.getForAssetDeletion.mockResolvedValue(asset);
 
-      mocks.assetJob.getForAssetDeletion.mockResolvedValue(assetWithFace);
+      await sut.handleAssetDeletion({ id: asset.id, deleteOnDisk: true });
-
-      await sut.handleAssetDeletion({ id: assetWithFace.id, deleteOnDisk: true });
 
       expect(mocks.job.queue.mock.calls).toEqual([
         [
@@ -581,38 +579,29 @@ describe(AssetService.name, () => {
                 '/uploads/user-id/webp/path.ext',
                 '/uploads/user-id/thumbs/path.jpg',
                 '/uploads/user-id/fullsize/path.webp',
-                assetWithFace.originalPath,
+                asset.originalPath,
               ],
             },
           },
         ],
       ]);
-
+      expect(mocks.asset.remove).toHaveBeenCalledWith(asset);
-      expect(mocks.asset.remove).toHaveBeenCalledWith(assetWithFace);
-    });
-
-    it('should update stack primary asset if deleted asset was primary asset in a stack', async () => {
-      mocks.stack.update.mockResolvedValue(factory.stack() as any);
-      mocks.assetJob.getForAssetDeletion.mockResolvedValue(assetStub.primaryImage);
-
-      await sut.handleAssetDeletion({ id: assetStub.primaryImage.id, deleteOnDisk: true });
-
-      expect(mocks.stack.update).toHaveBeenCalledWith('stack-1', {
-        id: 'stack-1',
-        primaryAssetId: 'stack-child-asset-1',
-      });
     });
 
     it('should delete the entire stack if deleted asset was the primary asset and the stack would only contain one asset afterwards', async () => {
       mocks.stack.delete.mockResolvedValue();
       mocks.assetJob.getForAssetDeletion.mockResolvedValue({
         ...assetStub.primaryImage,
-        stack: { ...assetStub.primaryImage.stack, assets: assetStub.primaryImage.stack!.assets.slice(0, 2) },
+        stack: {
+          id: 'stack-id',
+          primaryAssetId: assetStub.primaryImage.id,
+          assets: [{ id: 'one-asset' }],
+        },
       });
 
       await sut.handleAssetDeletion({ id: assetStub.primaryImage.id, deleteOnDisk: true });
 
-      expect(mocks.stack.delete).toHaveBeenCalledWith('stack-1');
+      expect(mocks.stack.delete).toHaveBeenCalledWith('stack-id');
     });
 
     it('should delete a live photo', async () => {

server/src/services/asset.service.ts

@@ -327,10 +327,11 @@ export class AssetService extends BaseService {
       return JobStatus.Failed;
     }
 
-    // Replace the parent of the stack children with a new asset
+    // replace the parent of the stack children with a new asset
     if (asset.stack?.primaryAssetId === id) {
-      const stackAssetIds = asset.stack?.assets.map((a) => a.id) ?? [];
+      // this only includes timeline visible assets and excludes the primary asset
-      if (stackAssetIds.length > 2) {
+      const stackAssetIds = asset.stack.assets.map((a) => a.id);
+      if (stackAssetIds.length >= 2) {
         const newPrimaryAssetId = stackAssetIds.find((a) => a !== id)!;
         await this.stackRepository.update(asset.stack.id, {
           id: asset.stack.id,

server/test/medium/specs/services/asset.service.spec.ts

@@ -1,5 +1,5 @@
 import { Kysely } from 'kysely';
-import { AssetFileType, AssetMetadataKey, JobName, SharedLinkType } from 'src/enum';
+import { AssetFileType, AssetMetadataKey, AssetStatus, JobName, SharedLinkType } from 'src/enum';
 import { AccessRepository } from 'src/repositories/access.repository';
 import { AlbumRepository } from 'src/repositories/album.repository';
 import { AssetJobRepository } from 'src/repositories/asset-job.repository';
@@ -246,6 +246,66 @@ describe(AssetService.name, () => {
       });
     });
 
+    it('should delete a stacked primary asset (2 assets)', async () => {
+      const { sut, ctx } = setup();
+      ctx.getMock(EventRepository).emit.mockResolvedValue();
+      ctx.getMock(JobRepository).queue.mockResolvedValue();
+      const { user } = await ctx.newUser();
+      const { asset: asset1 } = await ctx.newAsset({ ownerId: user.id });
+      const { asset: asset2 } = await ctx.newAsset({ ownerId: user.id });
+      const { stack, result } = await ctx.newStack({ ownerId: user.id }, [asset1.id, asset2.id]);
+
+      const stackRepo = ctx.get(StackRepository);
+
+      expect(result).toMatchObject({ primaryAssetId: asset1.id });
+
+      await sut.handleAssetDeletion({ id: asset1.id, deleteOnDisk: true });
+
+      // stack is deleted as well
+      await expect(stackRepo.getById(stack.id)).resolves.toBe(undefined);
+    });
+
+    it('should delete a stacked primary asset (3 assets)', async () => {
+      const { sut, ctx } = setup();
+      ctx.getMock(EventRepository).emit.mockResolvedValue();
+      ctx.getMock(JobRepository).queue.mockResolvedValue();
+      const { user } = await ctx.newUser();
+      const { asset: asset1 } = await ctx.newAsset({ ownerId: user.id });
+      const { asset: asset2 } = await ctx.newAsset({ ownerId: user.id });
+      const { asset: asset3 } = await ctx.newAsset({ ownerId: user.id });
+      const { stack, result } = await ctx.newStack({ ownerId: user.id }, [asset1.id, asset2.id, asset3.id]);
+
+      expect(result).toMatchObject({ primaryAssetId: asset1.id });
+
+      await sut.handleAssetDeletion({ id: asset1.id, deleteOnDisk: true });
+
+      // new primary asset is picked
+      await expect(ctx.get(StackRepository).getById(stack.id)).resolves.toMatchObject({ primaryAssetId: asset2.id });
+    });
+
+    it('should delete a stacked primary asset (3 trashed assets)', async () => {
+      const { sut, ctx } = setup();
+      ctx.getMock(EventRepository).emit.mockResolvedValue();
+      ctx.getMock(JobRepository).queue.mockResolvedValue();
+      const { user } = await ctx.newUser();
+      const { asset: asset1 } = await ctx.newAsset({ ownerId: user.id });
+      const { asset: asset2 } = await ctx.newAsset({ ownerId: user.id });
+      const { asset: asset3 } = await ctx.newAsset({ ownerId: user.id });
+      const { stack, result } = await ctx.newStack({ ownerId: user.id }, [asset1.id, asset2.id, asset3.id]);
+
+      await ctx.get(AssetRepository).updateAll([asset1.id, asset2.id, asset3.id], {
+        deletedAt: new Date(),
+        status: AssetStatus.Deleted,
+      });
+
+      expect(result).toMatchObject({ primaryAssetId: asset1.id });
+
+      await sut.handleAssetDeletion({ id: asset1.id, deleteOnDisk: true });
+
+      // stack is deleted as well
+      await expect(ctx.get(StackRepository).getById(stack.id)).resolves.toBe(undefined);
+    });
+
     it('should not delete offline assets', async () => {
       const { sut, ctx } = setup();
       ctx.getMock(EventRepository).emit.mockResolvedValue();