helpy4/immich
1
0
Fork 0
mirror of https://github.com/immich-app/immich synced 2026-08-29 13:15:45 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix(server): let a second metadata extraction replace stored AV metadata (#30900)

Browse source 
`upsertExif` writes derived audio, video and keyframe metadata with an
`ON CONFLICT DO UPDATE`, but every column in those three update lists
assigns the stored row back to itself:

    bitrate: ref('asset_audio.bitrate')

`SET bitrate = asset_audio.bitrate` is a self-assignment, so once a row
exists nothing can change it. Re-running metadata extraction re-reads the
file, builds a fresh snapshot, and then quietly discards it. The rest of the
file already uses `excluded` for this, as does `plugin.repository.ts`.

`asset_video.frameCount` had a second problem: it is supplied on insert but
was missing from the update list, so it would have stayed stale even after
the reference was corrected.

Extraction is meant to be repeatable. Probing improves between releases, a
file can be repaired or replaced, and a fix to how a stream is chosen is
worthless if it cannot reach the assets that were already imported. An
`upsert` that silently degrades to insert-only defeats all of that.

Metadata extraction is the only caller that passes these three objects, and
it passes a complete snapshot or nothing: each object is built from a single
probe behind a guard, and `upsertExif` skips the branch entirely when the
object is absent. So taking the incoming row cannot write partial values
over good ones. The other three callers pass `exif` only, and every other
reference to these tables is a read.
This commit is contained in:
Lin, Chiang-Yu 2026-08-23 01:58:05 +08:00 • committed by GitHub
parent 2237b28813
commit b26b0cc806
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 150 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

51
server/src/repositories/asset.repository.ts
View file

@ -195,10 +195,10 @@ export class AssetRepository {
.values(audio)
.onConflict((oc) =>
oc.column('assetId').doUpdateSet(({ ref }) => ({
bitrate: ref('asset_audio.bitrate'),
index: ref('asset_audio.index'),
profile: ref('asset_audio.profile'),
codecName: ref('asset_audio.codecName'),
bitrate: ref('excluded.bitrate'),
index: ref('excluded.index'),
profile: ref('excluded.profile'),
codecName: ref('excluded.codecName'),
})),
),
);
@ -211,21 +211,22 @@ export class AssetRepository {
.values(video)
.onConflict((oc) =>
oc.column('assetId').doUpdateSet(({ ref }) => ({
bitrate: ref('asset_video.bitrate'),
timeBase: ref('asset_video.timeBase'),
index: ref('asset_video.index'),
profile: ref('asset_video.profile'),
level: ref('asset_video.level'),
colorPrimaries: ref('asset_video.colorPrimaries'),
colorTransfer: ref('asset_video.colorTransfer'),
colorMatrix: ref('asset_video.colorMatrix'),
dvProfile: ref('asset_video.dvProfile'),
dvLevel: ref('asset_video.dvLevel'),
dvBlSignalCompatibilityId: ref('asset_video.dvBlSignalCompatibilityId'),
codecName: ref('asset_video.codecName'),
formatName: ref('asset_video.formatName'),
formatLongName: ref('asset_video.formatLongName'),
pixelFormat: ref('asset_video.pixelFormat'),
bitrate: ref('excluded.bitrate'),
frameCount: ref('excluded.frameCount'),
timeBase: ref('excluded.timeBase'),
index: ref('excluded.index'),
profile: ref('excluded.profile'),
level: ref('excluded.level'),
colorPrimaries: ref('excluded.colorPrimaries'),
colorTransfer: ref('excluded.colorTransfer'),
colorMatrix: ref('excluded.colorMatrix'),
dvProfile: ref('excluded.dvProfile'),
dvLevel: ref('excluded.dvLevel'),
dvBlSignalCompatibilityId: ref('excluded.dvBlSignalCompatibilityId'),
codecName: ref('excluded.codecName'),
formatName: ref('excluded.formatName'),
formatLongName: ref('excluded.formatLongName'),
pixelFormat: ref('excluded.pixelFormat'),
})),
),
);
@ -238,12 +239,12 @@ export class AssetRepository {
.values(keyframes)
.onConflict((oc) =>
oc.column('assetId').doUpdateSet(({ ref }) => ({
pts: ref('asset_keyframe.pts'),
accDuration: ref('asset_keyframe.accDuration'),
ownDuration: ref('asset_keyframe.ownDuration'),
totalDuration: ref('asset_keyframe.totalDuration'),
packetCount: ref('asset_keyframe.packetCount'),
outputFrames: ref('asset_keyframe.outputFrames'),
pts: ref('excluded.pts'),
accDuration: ref('excluded.accDuration'),
ownDuration: ref('excluded.ownDuration'),
totalDuration: ref('excluded.totalDuration'),
packetCount: ref('excluded.packetCount'),
outputFrames: ref('excluded.outputFrames'),
})),
),
);

124
server/test/medium/specs/repositories/asset.repository.spec.ts
View file

@ -23,6 +23,46 @@ beforeAll(async () => {
defaultDatabase = await getKyselyDB();
});
// Metadata extraction is repeatable: probing improves, files get repaired,
// and a re-run has to be able to correct what an earlier run stored.
const audioRow = (assetId: string, n: number) => ({
assetId,
bitrate: 100_000 + n,
index: n,
profile: n,
codecName: `codec-${n}`,
});
const videoRow = (assetId: string, n: number) => ({
assetId,
bitrate: 200_000 + n,
frameCount: 300 + n,
timeBase: 600 + n,
index: n,
profile: n,
level: n,
colorPrimaries: n,
colorTransfer: n,
colorMatrix: n,
dvProfile: n,
dvLevel: n,
dvBlSignalCompatibilityId: n,
codecName: `vcodec-${n}`,
formatName: `format-${n}`,
formatLongName: `format long ${n}`,
pixelFormat: `pixfmt-${n}`,
});
const keyframeRow = (assetId: string, n: number) => ({
assetId,
pts: [n],
accDuration: [n],
ownDuration: [n],
totalDuration: 1000 + n,
packetCount: 10 + n,
outputFrames: 20 + n,
});
describe(AssetRepository.name, () => {
describe('getTimeBucket', () => {
it('should order assets by local day first and fileCreatedAt within each day', async () => {
@ -80,6 +120,90 @@ describe(AssetRepository.name, () => {
});
describe('upsertExif', () => {
it('should replace stored audio metadata on a second extraction', async () => {
const { ctx, sut } = setup();
const { user } = await ctx.newUser();
const { asset } = await ctx.newAsset({ ownerId: user.id });
await sut.upsertExif({
exif: { assetId: asset.id, description: 'first' },
audio: audioRow(asset.id, 2),
lockedPropertiesBehavior: 'skip',
});
await sut.upsertExif({
exif: { assetId: asset.id, description: 'second' },
audio: audioRow(asset.id, 1),
lockedPropertiesBehavior: 'skip',
});
await expect(
ctx.database.selectFrom('asset_audio').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(),
).resolves.toEqual(audioRow(asset.id, 1));
});
it('should replace stored video metadata on a second extraction', async () => {
const { ctx, sut } = setup();
const { user } = await ctx.newUser();
const { asset } = await ctx.newAsset({ ownerId: user.id });
await sut.upsertExif({
exif: { assetId: asset.id, description: 'first' },
video: videoRow(asset.id, 2),
lockedPropertiesBehavior: 'skip',
});
await sut.upsertExif({
exif: { assetId: asset.id, description: 'second' },
video: videoRow(asset.id, 1),
lockedPropertiesBehavior: 'skip',
});
await expect(
ctx.database.selectFrom('asset_video').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(),
).resolves.toEqual(videoRow(asset.id, 1));
});
it('should replace stored keyframe metadata on a second extraction', async () => {
const { ctx, sut } = setup();
const { user } = await ctx.newUser();
const { asset } = await ctx.newAsset({ ownerId: user.id });
await sut.upsertExif({
exif: { assetId: asset.id, description: 'first' },
keyframes: keyframeRow(asset.id, 2),
lockedPropertiesBehavior: 'skip',
});
await sut.upsertExif({
exif: { assetId: asset.id, description: 'second' },
keyframes: keyframeRow(asset.id, 1),
lockedPropertiesBehavior: 'skip',
});
await expect(
ctx.database.selectFrom('asset_keyframe').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(),
).resolves.toEqual(keyframeRow(asset.id, 1));
});
// A probe that could not read a stream sends no object at all, and that must
// not be read as "delete what is already known".
it('should leave stored media metadata alone when an extraction omits it', async () => {
const { ctx, sut } = setup();
const { user } = await ctx.newUser();
const { asset } = await ctx.newAsset({ ownerId: user.id });
await sut.upsertExif({
exif: { assetId: asset.id, description: 'first' },
audio: audioRow(asset.id, 2),
lockedPropertiesBehavior: 'skip',
});
await sut.upsertExif({
exif: { assetId: asset.id, description: 'second' },
lockedPropertiesBehavior: 'skip',
});
await expect(
ctx.database.selectFrom('asset_audio').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(),
).resolves.toEqual(audioRow(asset.id, 2));
});
it('should append to locked columns', async () => {
const { ctx, sut } = setup();
const { user } = await ctx.newUser();