From 6cdebdd3b3fc6c01b9971a37c59167f4b1cd400c Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 4 Feb 2026 12:33:37 -0500 Subject: [PATCH] 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 | 56 +++++++---------- .../src/repositories/asset-job.repository.ts | 37 ++++++----- server/src/repositories/storage.repository.ts | 10 ++- server/src/services/asset.service.spec.ts | 35 ++++------- server/src/services/asset.service.ts | 7 ++- .../specs/services/asset.service.spec.ts | 62 ++++++++++++++++++- 6 files changed, 127 insertions(+), 80 deletions(-) diff --git a/server/src/queries/asset.job.repository.sql b/server/src/queries/asset.job.repository.sql index 50f2c193fc..26f04bd9cb 100644 --- a/server/src/queries/asset.job.repository.sql +++ b/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 - and "stacked"."visibility" = $1 - and "stacked"."stackId" = "stack"."id" - group by - "stack"."id" - ) as "stacked_assets" on "stack"."id" is not null + "stack"."id" = "asset"."stackId" + ) as "stack_result" on true where - "asset"."id" = $2 + "asset"."id" = $3 -- AssetJobRepository.streamForVideoConversion select diff --git a/server/src/repositories/asset-job.repository.ts b/server/src/repositories/asset-job.repository.ts index 1608f7b6f6..6fec1b38a7 100644 --- a/server/src/repositories/asset-job.repository.ts +++ b/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') - .select(['stack.id', 'stack.primaryAssetId']) - .select((eb) => eb.fn('array_agg', [eb.table('stacked')]).as('assets')) - .where('stacked.deletedAt', 'is not', null) - .where('stacked.visibility', '=', AssetVisibility.Timeline) - .whereRef('stacked.stackId', '=', 'stack.id') - .groupBy('stack.id') - .as('stacked_assets'), - (join) => join.on('stack.id', 'is not', null), + .selectFrom('stack') + .whereRef('stack.id', '=', 'asset.stackId') + .select((eb) => [ + 'stack.id', + 'stack.primaryAssetId', + jsonArrayFrom( + eb + .selectFrom('asset as stack_asset') + .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(); } diff --git a/server/src/repositories/storage.repository.ts b/server/src/repositories/storage.repository.ts index 7345dfef5b..5a1a936e77 100644 --- a/server/src/repositories/storage.repository.ts +++ b/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}`); + } + } } } } diff --git a/server/src/services/asset.service.spec.ts b/server/src/services/asset.service.spec.ts index eca49bc14e..707faa326d 100755 --- a/server/src/services/asset.service.spec.ts +++ b/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 () => { - const assetWithFace = { ...assetStub.image, faces: [faceStub.face1, faceStub.mergeFace1] }; + it('should clean up files', async () => { + const asset = assetStub.image; + mocks.assetJob.getForAssetDeletion.mockResolvedValue(asset); - mocks.assetJob.getForAssetDeletion.mockResolvedValue(assetWithFace); - - await sut.handleAssetDeletion({ id: assetWithFace.id, deleteOnDisk: true }); + await sut.handleAssetDeletion({ id: asset.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(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', - }); + expect(mocks.asset.remove).toHaveBeenCalledWith(asset); }); 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 () => { diff --git a/server/src/services/asset.service.ts b/server/src/services/asset.service.ts index 066084ed45..23ba29e7b8 100644 --- a/server/src/services/asset.service.ts +++ b/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) ?? []; - if (stackAssetIds.length > 2) { + // this only includes timeline visible assets and excludes the primary asset + 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, diff --git a/server/test/medium/specs/services/asset.service.spec.ts b/server/test/medium/specs/services/asset.service.spec.ts index d0949c153c..1e34544e38 100644 --- a/server/test/medium/specs/services/asset.service.spec.ts +++ b/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();