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): get assetFiles when retrieving assets WithoutProperty.THUMBNAIL #12225

Merged

Conversation

mPyKen
Copy link
Contributor

@mPyKen mPyKen commented Sep 2, 2024

const { previewFile, thumbnailFile } = getAssetFiles(asset.files);

asset.files is being accessed but files are not being retrieved here. This patch adds the files to the relation when retrieving assets during thumbnail generation job.

Background:
Many users won't notice this as there is usually already an asset_job_status row for each asset in the database, thus, the thumbnail generation job won't even retrieve and paginate through these assets. The core devs probably neither, as they probably have been re-running jobs on all assets since asset_job_status has been introduced in #4854. But, for those who have been running immich for a long time, there might be assets which do not have a asset_job_status yet, thus, missing one important migration introduced in https://github.com/immich-app/immich/pull/11908/files#diff-99da4a5a2a767c94716e167a5d49f089778c22365cf403e92808711beb6125b9.
There is another migration in https://github.com/immich-app/immich/pull/11861/files#diff-09097609a560037cd9fc82b6d5de63f184101d01c8d14a53182de0d5e649d4e0 which moves the assets' file information to a separate table. As such, there are now assets for which I have valid asset_files but no asset_job_status. Without this PR's fix, the thumbnail generation job re-creates previews for all assets including those which already have previews. If one cancels and reruns the job with force=false, it still re-creates many previews as both thumbnailAt and previewAt fields have to be filled to be treated as 'not missing' in the handleQueueGenerateThumbnails function. (This is especially a problem for those who are running immich with an s3 storage backend, paying for egress)

This PR won't fix the issue that there is still no asset_job_status for all assets. Hence, running the job for missing thumbnails still iterates through all assets and checks if asset_files.path is null.
To fix the above issue correctly, one has to manually run a migration and upsert into asset_job_status:
EDIT: included below migration to this PR to add asset_job_status for all assets for which we already have at least one asset_file.

-- create job status row for all assets
INSERT INTO "asset_job_status" ("assetId", "facesRecognizedAt", "metadataExtractedAt", "duplicatesDetectedAt", "previewAt", "thumbnailAt")
SELECT "assetId", NULL, NULL, NULL, NULL, NULL
FROM "asset_files" f
WHERE "f"."path" IS NOT NULL
ON CONFLICT DO NOTHING

-- set previewAt
UPDATE "asset_job_status"
SET "previewAt" = NOW()
FROM "asset_files" f
WHERE "previewAt" IS NULL
AND "asset_job_status"."assetId" = "f"."assetId"
AND "f"."type" = 'preview'
AND "f"."path" IS NOT NULL

-- set thumbnailAt
UPDATE "asset_job_status"
SET "thumbnailAt" = NOW()
FROM "asset_files" f
WHERE "thumbnailAt" IS NULL
AND "asset_job_status"."assetId" = "f"."assetId"
AND "f"."type" = 'thumbnail'
AND "f"."path" IS NOT NULL

Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

Nice catch! You could make a migration for the commands to add older assets to the job status table.

@mPyKen
Copy link
Contributor Author

mPyKen commented Sep 2, 2024

I'm not used to typeorm - aren't migrations auto generated? Can I just copy one of the files under server/src/migrations and paste my queries?

@mertalev
Copy link
Contributor

mertalev commented Sep 2, 2024

You can follow this to make a new migration, except with create instead of generate. It'll make a stub for you to fill in with the actual queries.

@mPyKen
Copy link
Contributor Author

mPyKen commented Sep 2, 2024

Oh I see, thanks! Will follow the guide using create and add the migrations

@mPyKen
Copy link
Contributor Author

mPyKen commented Sep 2, 2024

Added migration. Not sure if we need existing timestamps for the other fields "facesRecognizedAt", "metadataExtractedAt", and "duplicatesDetectedAt", but that would be outside the scope of this PR, I guess

@mertalev mertalev merged commit 438344f into immich-app:main Sep 2, 2024
25 checks passed
@mPyKen mPyKen deleted the fix-asset-files-during-thumbnail-job branch September 3, 2024 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants