Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): transcodes failing due to storage migration happening simultaneously #3071

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Jul 1, 2023

Description

Schedules the video conversion job to be enqueued only after storage migration to avoid a File not found error while transcoding. Specifically, it's queued after JPEG thumbnail generation is complete. The reason why this particular job is chosen as a prerequisite is mainly because it already queries for the asset itself on completion, which means the result of this query can be used to check if the asset is a video and only queue for transcoding if so.

An earlier iteration queued all uploaded assets after the storage migration job, but this was misleading from a UX perspective as the number of queued assets for transcoding could be much higher than the number actually transcoded.

Fixes #3047

How Has This Been Tested?

I made a new account and uploaded about a thousand assets. Where before there would sometimes be a "file not found" error in the past, I never got this error with this PR; all videos were encoded successfully.

@vercel
Copy link

vercel bot commented Jul 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
immich ⬜️ Ignored (Inspect) Jul 1, 2023 10:28pm

@mertalev mertalev requested review from jrasm91 and alextran1502 July 1, 2023 23:08
@@ -163,9 +164,15 @@ export class JobService {
await this.jobRepository.queue({ name: JobName.CLASSIFY_IMAGE, data: item.data });
await this.jobRepository.queue({ name: JobName.ENCODE_CLIP, data: item.data });
await this.jobRepository.queue({ name: JobName.RECOGNIZE_FACES, data: item.data });
if (item.data.source !== 'upload') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Videos are currently only automatically queued for transcoding on upload, not when any other job is run. This check is to maintain that behavior. The asset check in this section also seems specific to uploading.


const [asset] = await this.assetRepository.getByIds([item.data.id]);
if (asset) {
if (asset.type === AssetType.VIDEO) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give a more explanation on why moving the job here would help with race conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently metadata extraction and transcoding are queued at the same time. Since metadata extraction is followed by migration, this can cause transcoding to fail. But since thumbnail generation is queued only after migration, there's no longer any risk of a race condition.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about just queuing all assets for video conversation on line 158 instead? It is more clear I think. The job now ignores non videos anyways.

@mertalev
Copy link
Contributor Author

mertalev commented Jul 4, 2023

I tried that, but it inflated the number of queued assets for transcoding in the dashboard. You could have 1000 assets waiting one second and 0 the next if the queue only had images.

@alextran1502 alextran1502 merged commit 71a2914 into main Jul 5, 2023
@alextran1502 alextran1502 deleted the fix-transcoding-before-migration branch July 5, 2023 05:36
@mertalev mertalev mentioned this pull request Jul 12, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] No such file or directory errors when uploading live photos
3 participants