feat: storage template file move hardening by zackpollard · Pull Request #5917 · immich-app/immich

zackpollard · 2023-12-21T22:40:41Z

Storage template moving can sometimes fail and cause data corruption when the upload folder and library folder are not the same filesystem. This PR aims to address the existing problem and add validation of the file operations to ensure the moved file data matches the original asset data by the way of file size checks and optional hash checks.

TODO:

When file is copied rather than renamed, validate filesize in database matches filesize on disk
When file is copied rather than renamed, validate hash in database matches hash of copied asset
Change job concurrency to one and remove configuration options for this
Add setting to enable and disable storage template system entirely, default storage template system to off
~~Change structure of default upload folder to split GUIDs into seperate folders to improve performance~~ Will be done in seperate PR

Optional TODO:

~~Have the filename itself indicate if it is partial or not (.tmp suffix?)~~ This actually substantially increases complexity with the move table, not going ahead with this for now
~~Keep an audit log of moves/file operations in the database~~ Could be done in seperate PR
~~Use database locks to force template migration concurrency to one, avoiding race conditions where two processes move a file with the same name to the same location, not appending a +n~~ Done in feat: storage template locking + fix for database locks #6054

cloudflare-workers-and-pages · 2023-12-22T01:09:17Z

Deploying with Cloudflare Pages

Latest commit:	`b5d1617`
Status:	✅ Deploy successful!
Preview URL:	https://24067ebe.immich.pages.dev
Branch Preview URL:	https://feat-storage-template-harden.immich.pages.dev

View logs

docker/docker-compose.dev.yml

server/src/infra/database.config.ts

server/package.json

danieldietzler

Looking good so far! :)

server/src/domain/storage-template/storage-template.service.spec.ts

server/src/domain/storage/storage.core.ts

zackpollard · 2023-12-24T01:54:40Z

Ready for review and merge 😄

danieldietzler

Have you thought about nesting the files by default in a ab/cd/abcdxx-xxxx-xxxxx structure?

server/src/domain/storage-template/storage-template.service.ts

bo0tzz

Good stuff. Did you not end up adding the locking to this?

server/src/domain/storage/storage.core.ts

alextran1502 · 2023-12-24T17:09:15Z

I just tested the PR, it seems like the motion part of LivePhotos aren't moved to the library

zackpollard · 2023-12-24T18:27:04Z

I just tested the PR, it seems like the motion part of LivePhotos aren't moved to the library

We have explicit tests that check this behaviour so they should be. I haven't actually changed any of that logic, it's just two seperate calls to move each seperate part.

zackpollard · 2023-12-24T18:28:20Z

Good stuff. Did you not end up adding the locking to this?

No, I will only add locking if the PR refactoring the locking mechanism gets merged before this PR does.

zackpollard · 2023-12-24T18:28:51Z

Have you thought about nesting the files by default in a ab/cd/abcdxx-xxxx-xxxxx structure?

Forgot about that, can change it to that structure, yea

danieldietzler · 2023-12-24T18:40:18Z

Have you thought about nesting the files by default in a ab/cd/abcdxx-xxxx-xxxxx structure?

Forgot about that, can change it to that structure, yea

Ty :)

danieldietzler

Great stuff 🚀

alextran1502 · 2023-12-28T03:28:43Z

@zackpollard This condition prevents moving the video part of the LivePhotos upload

I tested with main on a small set of LivePhotos, and they are moved correctly.

If I remember correctly, we are not extracting EXIF and add a record in EXIF table for the motion part of LivePhotos

… data

…le for storage template job

…or new instances

…can cause race conditions above 1

…ate job

zackpollard · 2023-12-28T17:11:15Z

Changed exif extraction to run for all assets, including assets marked isVisible = false. This should resolve the issues with live photos not moving correctly.

alextran1502

LGTM

* fix: pgvecto.rs extension breaks typeorm schema:drop command * fix: parse postgres bigints to javascript number types when selecting data * feat: verify file size is the same as original asset after copying file for storage template job * feat: allow disabling of storage template job, defaults to disabled for new instances * fix: don't allow setting concurrency for storage template migration, can cause race conditions above 1 * feat: add checksum verification when file is copied for storage template job * fix: extract metadata for assets that aren't visible on timeline

zackpollard added 🗄️server bug labels Dec 21, 2023

zackpollard force-pushed the feat/storage-template-hardening branch from 0c89f0f to 023f08f Compare December 22, 2023 01:35

jrasm91 reviewed Dec 22, 2023

View reviewed changes

docker/docker-compose.dev.yml Show resolved Hide resolved

server/src/infra/database.config.ts Show resolved Hide resolved

server/package.json Show resolved Hide resolved

danieldietzler approved these changes Dec 22, 2023

View reviewed changes

zackpollard force-pushed the feat/storage-template-hardening branch 4 times, most recently from 527720b to f99db77 Compare December 23, 2023 02:25

zackpollard marked this pull request as ready for review December 24, 2023 00:38

zackpollard requested review from danieldietzler and jrasm91 December 24, 2023 01:00

danieldietzler reviewed Dec 24, 2023

View reviewed changes

server/src/domain/storage-template/storage-template.service.ts Outdated Show resolved Hide resolved

server/src/domain/storage-template/storage-template.service.ts Outdated Show resolved Hide resolved

bo0tzz reviewed Dec 24, 2023

View reviewed changes

server/src/domain/storage/storage.core.ts Show resolved Hide resolved

server/src/domain/storage/storage.core.ts Show resolved Hide resolved

server/src/domain/storage/storage.core.ts Show resolved Hide resolved

zackpollard force-pushed the feat/storage-template-hardening branch 4 times, most recently from 459008c to 49389eb Compare December 24, 2023 23:19

danieldietzler approved these changes Dec 25, 2023

View reviewed changes

zackpollard added 3 commits December 28, 2023 12:40

fix: pgvecto.rs extension breaks typeorm schema:drop command

f65816c

fix: parse postgres bigints to javascript number types when selecting…

22aad78

… data

feat: verify file size is the same as original asset after copying fi…

1cc59a4

…le for storage template job

zackpollard added 3 commits December 28, 2023 12:40

feat: allow disabling of storage template job, defaults to disabled f…

428b536

…or new instances

fix: don't allow setting concurrency for storage template migration, …

45baca4

…can cause race conditions above 1

feat: add checksum verification when file is copied for storage templ…

ab750e1

…ate job

zackpollard force-pushed the feat/storage-template-hardening branch from 49389eb to ab750e1 Compare December 28, 2023 12:40

alextran1502 added the to-be-reviewed label Dec 29, 2023

uhthomas mentioned this pull request Dec 29, 2023

Object storage #4445

Closed

fix: extract metadata for assets that aren't visible on timeline

b5d1617

zackpollard force-pushed the feat/storage-template-hardening branch from 131e8a2 to b5d1617 Compare December 29, 2023 15:14

zackpollard mentioned this pull request Dec 29, 2023

feat: storage template locking + fix for database locks #6054

Merged

alextran1502 approved these changes Dec 29, 2023

View reviewed changes

zackpollard merged commit 2e38fa7 into main Dec 29, 2023

zackpollard deleted the feat/storage-template-hardening branch December 29, 2023 18:41

alextran1502 mentioned this pull request Jan 1, 2024

[BUG] UNTRACKS FILES #6086

Closed

3 tasks

jrasm91 mentioned this pull request Sep 5, 2025

Storage template migration uses wrong source storage label #14540

Closed

3 tasks

Uh oh!

Conversation

zackpollard commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danieldietzler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zackpollard commented Dec 24, 2023

Uh oh!

danieldietzler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bo0tzz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alextran1502 commented Dec 24, 2023

Uh oh!

zackpollard commented Dec 24, 2023

Uh oh!

zackpollard commented Dec 24, 2023

Uh oh!

zackpollard commented Dec 24, 2023

Uh oh!

danieldietzler commented Dec 24, 2023

Uh oh!

danieldietzler left a comment

Choose a reason for hiding this comment

Uh oh!

alextran1502 commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zackpollard commented Dec 28, 2023

Uh oh!

alextran1502 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zackpollard commented Dec 21, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 22, 2023 •

edited

Loading

alextran1502 commented Dec 28, 2023 •

edited

Loading