From 184f1a6d32d7d05ceb6d4e58fdb34b9b88fe1c59 Mon Sep 17 00:00:00 2001 From: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com> Date: Wed, 21 Jan 2026 09:30:45 -0600 Subject: [PATCH] fix: tag update race condition (#25371) --- server/src/database.ts | 2 ++ server/src/queries/asset.job.repository.sql | 14 --------- server/src/queries/stack.repository.sql | 2 ++ .../src/repositories/asset-job.repository.ts | 10 ------- server/src/repositories/asset.repository.ts | 1 + .../migrations/1768847456553-AddTagsToExif.ts | 9 ++++++ server/src/schema/tables/asset-exif.table.ts | 3 ++ server/src/services/metadata.service.spec.ts | 13 ++++++++ server/src/services/metadata.service.ts | 30 ++++++++++++------- server/src/services/tag.service.spec.ts | 22 ++++++++++++++ server/src/services/tag.service.ts | 11 +++++++ server/src/types.ts | 2 +- server/test/fixtures/shared-link.stub.ts | 1 + 13 files changed, 84 insertions(+), 36 deletions(-) create mode 100644 server/src/schema/migrations/1768847456553-AddTagsToExif.ts diff --git a/server/src/database.ts b/server/src/database.ts index 5f9d5aac29..60b6cc40aa 100644 --- a/server/src/database.ts +++ b/server/src/database.ts @@ -458,6 +458,7 @@ export const columns = { 'asset_exif.projectionType', 'asset_exif.rating', 'asset_exif.state', + 'asset_exif.tags', 'asset_exif.timeZone', ], plugin: [ @@ -481,4 +482,5 @@ export const lockableProperties = [ 'longitude', 'rating', 'timeZone', + 'tags', ] as const; diff --git a/server/src/queries/asset.job.repository.sql b/server/src/queries/asset.job.repository.sql index fa910eb352..bb9941a162 100644 --- a/server/src/queries/asset.job.repository.sql +++ b/server/src/queries/asset.job.repository.sql @@ -38,20 +38,6 @@ select and "asset_file"."type" = $1 ) as agg ) as "files", - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - "tag"."value" - from - "tag" - inner join "tag_asset" on "tag"."id" = "tag_asset"."tagId" - where - "asset"."id" = "tag_asset"."assetId" - ) as agg - ) as "tags", to_json("asset_exif") as "exifInfo" from "asset" diff --git a/server/src/queries/stack.repository.sql b/server/src/queries/stack.repository.sql index 64714e5665..b5f1dc7d18 100644 --- a/server/src/queries/stack.repository.sql +++ b/server/src/queries/stack.repository.sql @@ -43,6 +43,7 @@ select "asset_exif"."projectionType", "asset_exif"."rating", "asset_exif"."state", + "asset_exif"."tags", "asset_exif"."timeZone" from "asset_exif" @@ -127,6 +128,7 @@ select "asset_exif"."projectionType", "asset_exif"."rating", "asset_exif"."state", + "asset_exif"."tags", "asset_exif"."timeZone" from "asset_exif" diff --git a/server/src/repositories/asset-job.repository.ts b/server/src/repositories/asset-job.repository.ts index 39e658a5a8..b5cb4a537b 100644 --- a/server/src/repositories/asset-job.repository.ts +++ b/server/src/repositories/asset-job.repository.ts @@ -1,6 +1,5 @@ import { Injectable } from '@nestjs/common'; import { Kysely } from 'kysely'; -import { jsonArrayFrom } from 'kysely/helpers/postgres'; import { InjectKysely } from 'nestjs-kysely'; import { Asset, columns } from 'src/database'; import { DummyValue, GenerateSql } from 'src/decorators'; @@ -42,15 +41,6 @@ export class AssetJobRepository { .where('asset.id', '=', asUuid(id)) .select(['id', 'originalPath']) .select((eb) => withFiles(eb, AssetFileType.Sidecar)) - .select((eb) => - jsonArrayFrom( - eb - .selectFrom('tag') - .select(['tag.value']) - .innerJoin('tag_asset', 'tag.id', 'tag_asset.tagId') - .whereRef('asset.id', '=', 'tag_asset.assetId'), - ).as('tags'), - ) .$call(withExifInner) .limit(1) .executeTakeFirst(); diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index f8b3c8613a..944417c0ad 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -178,6 +178,7 @@ export class AssetRepository { bitsPerSample: ref('bitsPerSample'), rating: ref('rating'), fps: ref('fps'), + tags: ref('tags'), lockedProperties: lockedPropertiesBehavior === 'append' ? distinctLocked(eb, exif.lockedProperties ?? null) diff --git a/server/src/schema/migrations/1768847456553-AddTagsToExif.ts b/server/src/schema/migrations/1768847456553-AddTagsToExif.ts new file mode 100644 index 0000000000..6839468cae --- /dev/null +++ b/server/src/schema/migrations/1768847456553-AddTagsToExif.ts @@ -0,0 +1,9 @@ +import { Kysely, sql } from 'kysely'; + +export async function up(db: Kysely): Promise { + await sql`ALTER TABLE "asset_exif" ADD "tags" character varying[];`.execute(db); +} + +export async function down(db: Kysely): Promise { + await sql`ALTER TABLE "asset_exif" DROP COLUMN "tags";`.execute(db); +} diff --git a/server/src/schema/tables/asset-exif.table.ts b/server/src/schema/tables/asset-exif.table.ts index 346098a72c..9dacb547cf 100644 --- a/server/src/schema/tables/asset-exif.table.ts +++ b/server/src/schema/tables/asset-exif.table.ts @@ -93,6 +93,9 @@ export class AssetExifTable { @Column({ type: 'integer', nullable: true }) rating!: number | null; + @Column({ type: 'character varying', array: true, nullable: true }) + tags!: string[] | null; + @UpdateDateColumn({ default: () => 'clock_timestamp()' }) updatedAt!: Generated; diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index 09c8e673a6..6395e66e31 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -387,6 +387,7 @@ describe(MetadataService.name, () => { it('should extract tags from TagsList', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent'] } } as any); mockReadTags({ TagsList: ['Parent'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -397,6 +398,7 @@ describe(MetadataService.name, () => { it('should extract hierarchy from TagsList', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent/Child'] } } as any); mockReadTags({ TagsList: ['Parent/Child'] }); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.parentUpsert); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.childUpsert); @@ -417,6 +419,7 @@ describe(MetadataService.name, () => { it('should extract tags from Keywords as a string', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent'] } } as any); mockReadTags({ Keywords: 'Parent' }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -427,6 +430,7 @@ describe(MetadataService.name, () => { it('should extract tags from Keywords as a list', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent'] } } as any); mockReadTags({ Keywords: ['Parent'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -437,6 +441,7 @@ describe(MetadataService.name, () => { it('should extract tags from Keywords as a list with a number', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent', '2024'] } } as any); mockReadTags({ Keywords: ['Parent', 2024] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -448,6 +453,7 @@ describe(MetadataService.name, () => { it('should extract hierarchal tags from Keywords', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent/Child'] } } as any); mockReadTags({ Keywords: 'Parent/Child' }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -467,6 +473,7 @@ describe(MetadataService.name, () => { it('should ignore Keywords when TagsList is present', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent/Child', 'Child'] } } as any); mockReadTags({ Keywords: 'Child', TagsList: ['Parent/Child'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -486,6 +493,7 @@ describe(MetadataService.name, () => { it('should extract hierarchy from HierarchicalSubject', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent/Child', 'TagA'] } } as any); mockReadTags({ HierarchicalSubject: ['Parent|Child', 'TagA'] }); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.parentUpsert); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.childUpsert); @@ -507,6 +515,7 @@ describe(MetadataService.name, () => { it('should extract tags from HierarchicalSubject as a list with a number', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(removeNonSidecarFiles(assetStub.image)); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent', '2024'] } } as any); mockReadTags({ HierarchicalSubject: ['Parent', 2024] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -518,6 +527,7 @@ describe(MetadataService.name, () => { it('should extract ignore / characters in a HierarchicalSubject tag', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(assetStub.image); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Mom|Dad'] } } as any); mockReadTags({ HierarchicalSubject: ['Mom/Dad'] }); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.parentUpsert); @@ -532,6 +542,7 @@ describe(MetadataService.name, () => { it('should ignore HierarchicalSubject when TagsList is present', async () => { mocks.assetJob.getForMetadataExtraction.mockResolvedValue(assetStub.image); + mocks.asset.getById.mockResolvedValue({ exifInfo: { tags: ['Parent/Child', 'Parent2/Child2'] } } as any); mockReadTags({ HierarchicalSubject: ['Parent2|Child2'], TagsList: ['Parent/Child'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -896,6 +907,7 @@ describe(MetadataService.name, () => { ProfileDescription: 'extensive description', ProjectionType: 'equirectangular', tz: 'UTC-11:30', + TagsList: ['parent/child'], Rating: 3, }; @@ -935,6 +947,7 @@ describe(MetadataService.name, () => { country: null, state: null, city: null, + tags: ['parent/child'], }, { lockedPropertiesBehavior: 'skip' }, ); diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index e6cc15bc77..f74f9f4cec 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -254,6 +254,8 @@ export class MetadataService extends BaseService { } } + const tags = this.getTagList(exifTags); + const exifData: Insertable = { assetId: asset.id, @@ -296,6 +298,8 @@ export class MetadataService extends BaseService { // grouping livePhotoCID: (exifTags.ContentIdentifier || exifTags.MediaGroupUUID) ?? null, autoStackId: this.getAutoStackId(exifTags), + + tags: tags.length > 0 ? tags : null, }; const isSidewards = exifTags.Orientation && this.isOrientationSidewards(exifTags.Orientation); @@ -316,9 +320,10 @@ export class MetadataService extends BaseService { width: asset.width == null ? assetWidth : undefined, height: asset.height == null ? assetHeight : undefined, }), - this.applyTagList(asset, exifTags), ]; + await this.applyTagList(asset); + if (this.isMotionPhoto(asset, exifTags)) { promises.push(this.applyMotionPhotos(asset, exifTags, dates, stats)); } @@ -405,35 +410,35 @@ export class MetadataService extends BaseService { @OnEvent({ name: 'AssetTag' }) async handleTagAsset({ assetId }: ArgOf<'AssetTag'>) { - await this.jobRepository.queue({ name: JobName.SidecarWrite, data: { id: assetId, tags: true } }); + await this.jobRepository.queue({ name: JobName.SidecarWrite, data: { id: assetId } }); } @OnEvent({ name: 'AssetUntag' }) async handleUntagAsset({ assetId }: ArgOf<'AssetUntag'>) { - await this.jobRepository.queue({ name: JobName.SidecarWrite, data: { id: assetId, tags: true } }); + await this.jobRepository.queue({ name: JobName.SidecarWrite, data: { id: assetId } }); } @OnJob({ name: JobName.SidecarWrite, queue: QueueName.Sidecar }) async handleSidecarWrite(job: JobOf): Promise { - const { id, tags } = job; + const { id } = job; const asset = await this.assetJobRepository.getForSidecarWriteJob(id); if (!asset) { return JobStatus.Failed; } const lockedProperties = await this.assetJobRepository.getLockedPropertiesForMetadataExtraction(id); - const tagsList = (asset.tags || []).map((tag) => tag.value); const { sidecarFile } = getAssetFiles(asset.files); const sidecarPath = sidecarFile?.path || `${asset.originalPath}.xmp`; - const { description, dateTimeOriginal, latitude, longitude, rating } = _.pick( + const { description, dateTimeOriginal, latitude, longitude, rating, tags } = _.pick( { description: asset.exifInfo.description, dateTimeOriginal: asset.exifInfo.dateTimeOriginal, latitude: asset.exifInfo.latitude, longitude: asset.exifInfo.longitude, rating: asset.exifInfo.rating, + tags: asset.exifInfo.tags, }, lockedProperties, ); @@ -446,7 +451,7 @@ export class MetadataService extends BaseService { GPSLatitude: latitude, GPSLongitude: longitude, Rating: rating, - TagsList: tags ? tagsList : undefined, + TagsList: tags?.length ? tags : undefined, }, _.isUndefined, ); @@ -560,11 +565,14 @@ export class MetadataService extends BaseService { return tags; } - private async applyTagList(asset: { id: string; ownerId: string }, exifTags: ImmichTags) { - const tags = this.getTagList(exifTags); - const results = await upsertTags(this.tagRepository, { userId: asset.ownerId, tags }); + private async applyTagList({ id, ownerId }: { id: string; ownerId: string }) { + const asset = await this.assetRepository.getById(id, { exifInfo: true }); + const results = await upsertTags(this.tagRepository, { + userId: ownerId, + tags: asset?.exifInfo?.tags ?? [], + }); await this.tagRepository.replaceAssetTags( - asset.id, + id, results.map((tag) => tag.id), ); } diff --git a/server/src/services/tag.service.spec.ts b/server/src/services/tag.service.spec.ts index 6bb92abd8c..ff706552a9 100644 --- a/server/src/services/tag.service.spec.ts +++ b/server/src/services/tag.service.spec.ts @@ -191,6 +191,7 @@ describe(TagService.name, () => { it('should upsert records', async () => { mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set(['tag-1', 'tag-2'])); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3'])); + mocks.asset.getById.mockResolvedValue({ tags: [{ value: 'tag-1' }, { value: 'tag-2' }] } as any); mocks.tag.upsertAssetIds.mockResolvedValue([ { tagId: 'tag-1', assetId: 'asset-1' }, { tagId: 'tag-1', assetId: 'asset-2' }, @@ -204,6 +205,18 @@ describe(TagService.name, () => { ).resolves.toEqual({ count: 6, }); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + { assetId: 'asset-1', tags: ['tag-1', 'tag-2'] }, + { lockedPropertiesBehavior: 'append' }, + ); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + { assetId: 'asset-2', tags: ['tag-1', 'tag-2'] }, + { lockedPropertiesBehavior: 'append' }, + ); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + { assetId: 'asset-3', tags: ['tag-1', 'tag-2'] }, + { lockedPropertiesBehavior: 'append' }, + ); expect(mocks.tag.upsertAssetIds).toHaveBeenCalledWith([ { tagId: 'tag-1', assetId: 'asset-1' }, { tagId: 'tag-1', assetId: 'asset-2' }, @@ -229,6 +242,7 @@ describe(TagService.name, () => { mocks.tag.get.mockResolvedValue(tagStub.tag); mocks.tag.getAssetIds.mockResolvedValue(new Set(['asset-1'])); mocks.tag.addAssetIds.mockResolvedValue(); + mocks.asset.getById.mockResolvedValue({ tags: [{ value: 'tag-1' }] } as any); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-2'])); await expect( @@ -240,6 +254,14 @@ describe(TagService.name, () => { { id: 'asset-2', success: true }, ]); + expect(mocks.asset.upsertExif).not.toHaveBeenCalledWith( + { assetId: 'asset-1', tags: ['tag-1'] }, + { lockedPropertiesBehavior: 'append' }, + ); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + { assetId: 'asset-2', tags: ['tag-1'] }, + { lockedPropertiesBehavior: 'append' }, + ); expect(mocks.tag.getAssetIds).toHaveBeenCalledWith('tag-1', ['asset-1', 'asset-2']); expect(mocks.tag.addAssetIds).toHaveBeenCalledWith('tag-1', ['asset-2']); }); diff --git a/server/src/services/tag.service.ts b/server/src/services/tag.service.ts index 3ee5d29b75..3b3000f759 100644 --- a/server/src/services/tag.service.ts +++ b/server/src/services/tag.service.ts @@ -90,6 +90,7 @@ export class TagService extends BaseService { const results = await this.tagRepository.upsertAssetIds(items); for (const assetId of new Set(results.map((item) => item.assetId))) { + await this.updateTags(assetId); await this.eventRepository.emit('AssetTag', { assetId }); } @@ -107,6 +108,7 @@ export class TagService extends BaseService { for (const { id: assetId, success } of results) { if (success) { + await this.updateTags(assetId); await this.eventRepository.emit('AssetTag', { assetId }); } } @@ -125,6 +127,7 @@ export class TagService extends BaseService { for (const { id: assetId, success } of results) { if (success) { + await this.updateTags(assetId); await this.eventRepository.emit('AssetUntag', { assetId }); } } @@ -145,4 +148,12 @@ export class TagService extends BaseService { } return tag; } + + private async updateTags(assetId: string) { + const asset = await this.assetRepository.getById(assetId, { tags: true }); + await this.assetRepository.upsertExif( + { assetId, tags: asset?.tags?.map(({ value }) => value) ?? [] }, + { lockedPropertiesBehavior: 'append' }, + ); + } } diff --git a/server/src/types.ts b/server/src/types.ts index c28330a55e..f3b647f08f 100644 --- a/server/src/types.ts +++ b/server/src/types.ts @@ -318,7 +318,7 @@ export type JobItem = // Sidecar Scanning | { name: JobName.SidecarQueueAll; data: IBaseJob } | { name: JobName.SidecarCheck; data: IEntityJob } - | { name: JobName.SidecarWrite; data: ISidecarWriteJob } + | { name: JobName.SidecarWrite; data: IEntityJob } // Facial Recognition | { name: JobName.AssetDetectFacesQueueAll; data: IBaseJob } diff --git a/server/test/fixtures/shared-link.stub.ts b/server/test/fixtures/shared-link.stub.ts index 0f16057431..859b6b6ae2 100644 --- a/server/test/fixtures/shared-link.stub.ts +++ b/server/test/fixtures/shared-link.stub.ts @@ -147,6 +147,7 @@ export const sharedLinkStub = { visibility: AssetVisibility.Timeline, width: 500, height: 500, + tags: [], }, sharedLinks: [], faces: [],