From c7b2a2c9264993d2fe6bc046da921070045f63cc Mon Sep 17 00:00:00 2001 From: Sam Crauwels Date: Wed, 4 Feb 2026 21:49:37 +0100 Subject: [PATCH] fix(server): handle duplicate asset checksums atomically with ON CONFLICT Replace the check-then-insert race condition with PostgreSQL ON CONFLICT DO NOTHING on the UQ_assets_owner_checksum constraint. - Split AssetRepository.create() into create() (ON CONFLICT DO NOTHING, returns undefined on conflict) and createStrict() (throws, used by createCopy for asset replacement) - Update AssetRepository.createAll() with the same ON CONFLICT clause - Handle the undefined return in uploadAsset() by cleaning up files and returning DUPLICATE status without logging noisy constraint violations - Simplify metadata.service motion photo extraction to use if/else instead of try/catch with isAssetChecksumConstraint - Update tests to match the new behavior Fixes #22742 --- server/src/repositories/asset.repository.ts | 32 +++- .../src/services/asset-media.service.spec.ts | 26 ++-- server/src/services/asset-media.service.ts | 33 ++++- server/src/services/metadata.service.ts | 43 +++--- .../repositories/asset.repository.spec.ts | 137 ++++++++++++++++++ .../repositories/asset.repository.mock.ts | 1 + 6 files changed, 230 insertions(+), 42 deletions(-) diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index 1a060c4715b7c..0a77755986d22 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -327,12 +327,42 @@ export class AssetRepository { }); } + /** + * Insert an asset, gracefully handling duplicate checksums via ON CONFLICT. + * Returns `undefined` when the asset already exists for this owner (same checksum, + * no library), rather than throwing a constraint violation. Callers must handle + * the `undefined` case to look up and return the existing duplicate. + */ create(asset: Insertable) { + return this.db + .insertInto('asset') + .values(asset) + .onConflict((oc) => oc.columns(['ownerId', 'checksum']).where('libraryId', 'is', null).doNothing()) + .returningAll() + .executeTakeFirst(); + } + + /** + * Insert an asset, throwing on any constraint violation (including duplicate checksum). + * Use this for operations where a conflict is a real error, such as creating backup + * copies during asset replacement. + */ + createStrict(asset: Insertable) { return this.db.insertInto('asset').values(asset).returningAll().executeTakeFirstOrThrow(); } + /** + * Batch-insert assets, skipping any that violate the owner+checksum uniqueness + * constraint. The returned array may be shorter than the input when duplicates + * are silently skipped. + */ createAll(assets: Insertable[]) { - return this.db.insertInto('asset').values(assets).returningAll().execute(); + return this.db + .insertInto('asset') + .values(assets) + .onConflict((oc) => oc.columns(['ownerId', 'checksum']).where('libraryId', 'is', null).doNothing()) + .returningAll() + .execute(); } @GenerateSql({ params: [DummyValue.UUID, { year: 2000, day: 1, month: 1 }] }) diff --git a/server/src/services/asset-media.service.spec.ts b/server/src/services/asset-media.service.spec.ts index 0bcb87e2f4431..8347f70c89cd5 100644 --- a/server/src/services/asset-media.service.spec.ts +++ b/server/src/services/asset-media.service.spec.ts @@ -373,7 +373,7 @@ describe(AssetMediaService.name, () => { ); }); - it('should handle a duplicate', async () => { + it('should handle a duplicate via ON CONFLICT', async () => { const file = { uuid: 'random-uuid', originalPath: 'fake_path/asset_1.jpeg', @@ -382,10 +382,9 @@ describe(AssetMediaService.name, () => { originalName: 'asset_1.jpeg', size: 0, }; - const error = new Error('unique key violation'); - (error as any).constraint_name = ASSET_CHECKSUM_CONSTRAINT; - mocks.asset.create.mockRejectedValue(error); + // eslint-disable-next-line unicorn/no-useless-undefined + mocks.asset.create.mockResolvedValue(undefined); mocks.asset.getUploadAssetIdByChecksum.mockResolvedValue(assetEntity.id); await expect(sut.uploadAsset(authStub.user1, createDto, file)).resolves.toEqual({ @@ -409,10 +408,11 @@ describe(AssetMediaService.name, () => { originalName: 'asset_1.jpeg', size: 0, }; - const error = new Error('unique key violation'); - (error as any).constraint_name = ASSET_CHECKSUM_CONSTRAINT; - mocks.asset.create.mockRejectedValue(error); + // eslint-disable-next-line unicorn/no-useless-undefined + mocks.asset.create.mockResolvedValue(undefined); + // eslint-disable-next-line unicorn/no-useless-undefined + mocks.asset.getUploadAssetIdByChecksum.mockResolvedValue(undefined); await expect(sut.uploadAsset(authStub.user1, createDto, file)).rejects.toBeInstanceOf( InternalServerErrorException, @@ -844,7 +844,7 @@ describe(AssetMediaService.name, () => { // this is the original file size mocks.storage.stat.mockResolvedValue({ size: 0 } as Stats); // this is for the clone call - mocks.asset.create.mockResolvedValue(copiedAsset); + mocks.asset.createStrict.mockResolvedValue(copiedAsset); await expect(sut.replaceAsset(authStub.user1, existingAsset.id, replaceDto, updatedFile)).resolves.toEqual({ status: AssetMediaStatus.REPLACED, @@ -858,7 +858,7 @@ describe(AssetMediaService.name, () => { originalPath: 'fake_path/photo1.jpeg', }), ); - expect(mocks.asset.create).toHaveBeenCalledWith( + expect(mocks.asset.createStrict).toHaveBeenCalledWith( expect.objectContaining({ originalFileName: 'existing-filename.jpeg', originalPath: 'fake_path/asset_1.jpeg', @@ -893,7 +893,7 @@ describe(AssetMediaService.name, () => { // this is the original file size mocks.storage.stat.mockResolvedValue({ size: 0 } as Stats); // this is for the clone call - mocks.asset.create.mockResolvedValue(copiedAsset); + mocks.asset.createStrict.mockResolvedValue(copiedAsset); await expect( sut.replaceAsset(authStub.user1, sidecarAsset.id, replaceDto, updatedFile, sidecarFile), @@ -931,7 +931,7 @@ describe(AssetMediaService.name, () => { // this is the original file size mocks.storage.stat.mockResolvedValue({ size: 0 } as Stats); // this is for the copy call - mocks.asset.create.mockResolvedValue(copiedAsset); + mocks.asset.createStrict.mockResolvedValue(copiedAsset); await expect(sut.replaceAsset(authStub.user1, existingAsset.id, replaceDto, updatedFile)).resolves.toEqual({ status: AssetMediaStatus.REPLACED, @@ -968,14 +968,14 @@ describe(AssetMediaService.name, () => { // this is the original file size mocks.storage.stat.mockResolvedValue({ size: 0 } as Stats); // this is for the clone call - mocks.asset.create.mockResolvedValue(copiedAsset); + mocks.asset.createStrict.mockResolvedValue(copiedAsset); await expect(sut.replaceAsset(authStub.user1, sidecarAsset.id, replaceDto, updatedFile)).resolves.toEqual({ status: AssetMediaStatus.DUPLICATE, id: sidecarAsset.id, }); - expect(mocks.asset.create).not.toHaveBeenCalled(); + expect(mocks.asset.createStrict).not.toHaveBeenCalled(); expect(mocks.asset.updateAll).not.toHaveBeenCalled(); expect(mocks.asset.upsertFile).not.toHaveBeenCalled(); expect(mocks.asset.deleteFile).not.toHaveBeenCalled(); diff --git a/server/src/services/asset-media.service.ts b/server/src/services/asset-media.service.ts index 020bda4df7b97..b9aec6e5ccdcf 100644 --- a/server/src/services/asset-media.service.ts +++ b/server/src/services/asset-media.service.ts @@ -149,11 +149,20 @@ export class AssetMediaService extends BaseService { { userId: auth.user.id, livePhotoVideoId: dto.livePhotoVideoId }, ); } - const asset = await this.create(auth.user.id, dto, file, sidecarFile); + + const result = await this.create(auth.user.id, dto, file, sidecarFile); + + if (result.duplicate) { + await this.jobRepository.queue({ + name: JobName.FileDelete, + data: { files: [file.originalPath, sidecarFile?.originalPath] }, + }); + return { id: result.id, status: AssetMediaStatus.DUPLICATE }; + } await this.userRepository.updateUsage(auth.user.id, file.size); - return { id: asset.id, status: AssetMediaStatus.CREATED }; + return { id: result.id, status: AssetMediaStatus.CREATED }; } catch (error: any) { return this.handleUploadError(error, auth, file, sidecarFile); } @@ -400,7 +409,7 @@ export class AssetMediaService extends BaseService { * and then queues a METADATA_EXTRACTION job. */ private async createCopy(asset: Omit) { - const created = await this.assetRepository.create({ + const created = await this.assetRepository.createStrict({ ownerId: asset.ownerId, originalPath: asset.originalPath, originalFileName: asset.originalFileName, @@ -424,7 +433,12 @@ export class AssetMediaService extends BaseService { return created; } - private async create(ownerId: string, dto: AssetMediaCreateDto, file: UploadFile, sidecarFile?: UploadFile) { + private async create( + ownerId: string, + dto: AssetMediaCreateDto, + file: UploadFile, + sidecarFile?: UploadFile, + ): Promise<{ id: string; duplicate: boolean }> { const asset = await this.assetRepository.create({ ownerId, libraryId: null, @@ -447,6 +461,15 @@ export class AssetMediaService extends BaseService { originalFileName: dto.filename || file.originalName, }); + if (!asset) { + const duplicateId = await this.assetRepository.getUploadAssetIdByChecksum(ownerId, file.checksum); + if (!duplicateId) { + this.logger.error(`Error locating duplicate for checksum constraint`); + throw new InternalServerErrorException(); + } + return { id: duplicateId, duplicate: true }; + } + if (dto.metadata?.length) { await this.assetRepository.upsertMetadata(asset.id, dto.metadata); } @@ -469,7 +492,7 @@ export class AssetMediaService extends BaseService { await this.jobRepository.queue({ name: JobName.AssetExtractMetadata, data: { id: asset.id, source: 'upload' } }); - return asset; + return { id: asset.id, duplicate: false }; } private requireQuota(auth: AuthDto, size: number) { diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index f5af444a22a1f..9ccb6237b7b69 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -31,7 +31,7 @@ import { PersonTable } from 'src/schema/tables/person.table'; import { BaseService } from 'src/services/base.service'; import { JobItem, JobOf } from 'src/types'; import { getAssetFiles } from 'src/utils/asset.util'; -import { isAssetChecksumConstraint } from 'src/utils/database'; + import { mimeTypes } from 'src/utils/mime-types'; import { isFaceImportEnabled } from 'src/utils/misc'; import { upsertTags } from 'src/utils/tag'; @@ -641,34 +641,31 @@ export class MetadataService extends BaseService { let isNewMotionAsset = false; if (!motionAsset) { - try { - const motionAssetId = this.cryptoRepository.randomUUID(); - motionAsset = await this.assetRepository.create({ - id: motionAssetId, - libraryId: asset.libraryId, - type: AssetType.Video, - fileCreatedAt: dates.dateTimeOriginal, - fileModifiedAt: stats.mtime, - localDateTime: dates.localDateTime, - checksum, - ownerId: asset.ownerId, - originalPath: StorageCore.getAndroidMotionPath(asset, motionAssetId), - originalFileName: `${parse(asset.originalFileName).name}.mp4`, - visibility: AssetVisibility.Hidden, - deviceAssetId: 'NONE', - deviceId: 'NONE', - }); + const motionAssetId = this.cryptoRepository.randomUUID(); + const created = await this.assetRepository.create({ + id: motionAssetId, + libraryId: asset.libraryId, + type: AssetType.Video, + fileCreatedAt: dates.dateTimeOriginal, + fileModifiedAt: stats.mtime, + localDateTime: dates.localDateTime, + checksum, + ownerId: asset.ownerId, + originalPath: StorageCore.getAndroidMotionPath(asset, motionAssetId), + originalFileName: `${parse(asset.originalFileName).name}.mp4`, + visibility: AssetVisibility.Hidden, + deviceAssetId: 'NONE', + deviceId: 'NONE', + }); + if (created) { + motionAsset = created; isNewMotionAsset = true; if (!asset.isExternal) { await this.userRepository.updateUsage(asset.ownerId, video.byteLength); } - } catch (error) { - if (!isAssetChecksumConstraint(error)) { - throw error; - } - + } else { motionAsset = await this.assetRepository.getByChecksum(checksumQuery); if (!motionAsset) { this.logger.warn(`Unable to find existing motion video asset for ${asset.id}: ${asset.originalPath}`); diff --git a/server/test/medium/specs/repositories/asset.repository.spec.ts b/server/test/medium/specs/repositories/asset.repository.spec.ts index 97f503e9edbc7..804df36b4b405 100644 --- a/server/test/medium/specs/repositories/asset.repository.spec.ts +++ b/server/test/medium/specs/repositories/asset.repository.spec.ts @@ -22,6 +22,143 @@ beforeAll(async () => { }); describe(AssetRepository.name, () => { + describe('create', () => { + it('should return the asset on successful insert', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const checksum = Buffer.from('duplicate-test-checksum'); + + const result = await sut.create({ + ownerId: user.id, + checksum, + originalPath: '/path/to/file.jpg', + originalFileName: 'file.jpg', + deviceAssetId: 'device-1', + deviceId: 'device', + type: 'IMAGE' as any, + fileCreatedAt: new Date().toISOString(), + fileModifiedAt: new Date().toISOString(), + localDateTime: new Date().toISOString(), + isFavorite: false, + }); + + expect(result).toBeDefined(); + expect(result!.ownerId).toBe(user.id); + }); + + it('should return undefined on duplicate checksum for the same owner', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const checksum = Buffer.from('duplicate-test-checksum-2'); + + const first = await sut.create({ + ownerId: user.id, + checksum, + originalPath: '/path/to/file1.jpg', + originalFileName: 'file1.jpg', + deviceAssetId: 'device-1', + deviceId: 'device', + type: 'IMAGE' as any, + fileCreatedAt: new Date().toISOString(), + fileModifiedAt: new Date().toISOString(), + localDateTime: new Date().toISOString(), + isFavorite: false, + }); + expect(first).toBeDefined(); + + const second = await sut.create({ + ownerId: user.id, + checksum, + originalPath: '/path/to/file2.jpg', + originalFileName: 'file2.jpg', + deviceAssetId: 'device-2', + deviceId: 'device', + type: 'IMAGE' as any, + fileCreatedAt: new Date().toISOString(), + fileModifiedAt: new Date().toISOString(), + localDateTime: new Date().toISOString(), + isFavorite: false, + }); + expect(second).toBeUndefined(); + }); + + it('should allow the same checksum for different owners', async () => { + const { ctx, sut } = setup(); + const { user: user1 } = await ctx.newUser(); + const { user: user2 } = await ctx.newUser(); + const checksum = Buffer.from('shared-checksum'); + + const first = await sut.create({ + ownerId: user1.id, + checksum, + originalPath: '/path/to/file1.jpg', + originalFileName: 'file1.jpg', + deviceAssetId: 'device-1', + deviceId: 'device', + type: 'IMAGE' as any, + fileCreatedAt: new Date().toISOString(), + fileModifiedAt: new Date().toISOString(), + localDateTime: new Date().toISOString(), + isFavorite: false, + }); + expect(first).toBeDefined(); + + const second = await sut.create({ + ownerId: user2.id, + checksum, + originalPath: '/path/to/file2.jpg', + originalFileName: 'file2.jpg', + deviceAssetId: 'device-2', + deviceId: 'device', + type: 'IMAGE' as any, + fileCreatedAt: new Date().toISOString(), + fileModifiedAt: new Date().toISOString(), + localDateTime: new Date().toISOString(), + isFavorite: false, + }); + expect(second).toBeDefined(); + expect(second!.ownerId).toBe(user2.id); + }); + }); + + describe('createStrict', () => { + it('should throw on duplicate checksum', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const checksum = Buffer.from('strict-duplicate-checksum'); + + await sut.createStrict({ + ownerId: user.id, + checksum, + originalPath: '/path/to/file1.jpg', + originalFileName: 'file1.jpg', + deviceAssetId: 'device-1', + deviceId: 'device', + type: 'IMAGE' as any, + fileCreatedAt: new Date().toISOString(), + fileModifiedAt: new Date().toISOString(), + localDateTime: new Date().toISOString(), + isFavorite: false, + }); + + await expect( + sut.createStrict({ + ownerId: user.id, + checksum, + originalPath: '/path/to/file2.jpg', + originalFileName: 'file2.jpg', + deviceAssetId: 'device-2', + deviceId: 'device', + type: 'IMAGE' as any, + fileCreatedAt: new Date().toISOString(), + fileModifiedAt: new Date().toISOString(), + localDateTime: new Date().toISOString(), + isFavorite: false, + }), + ).rejects.toThrow(); + }); + }); + describe('upsertExif', () => { it('should append to locked columns', async () => { const { ctx, sut } = setup(); diff --git a/server/test/repositories/asset.repository.mock.ts b/server/test/repositories/asset.repository.mock.ts index 55dcf6456fd2b..9c7f45a0187a9 100644 --- a/server/test/repositories/asset.repository.mock.ts +++ b/server/test/repositories/asset.repository.mock.ts @@ -5,6 +5,7 @@ import { Mocked, vitest } from 'vitest'; export const newAssetRepositoryMock = (): Mocked> => { return { create: vitest.fn(), + createStrict: vitest.fn(), createAll: vitest.fn(), upsertExif: vitest.fn(), updateAllExif: vitest.fn(),