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

[BUG] assets may become orphaned when moved by the storage template service #2877

Closed
1 of 3 tasks
uhthomas opened this issue Jun 20, 2023 · 16 comments · Fixed by #4361
Closed
1 of 3 tasks

[BUG] assets may become orphaned when moved by the storage template service #2877

uhthomas opened this issue Jun 20, 2023 · 16 comments · Fixed by #4361

Comments

@uhthomas
Copy link
Member

The bug

I uploaded 8.5k files to Immich with the CLI and waited for all the jobs to process. Things look mostly fine, except some assets are missing thumbnails.

image

A look at the jobs status page shows some did fail.

image

Any attempt to rerun those jobs fail, and the logs from the microservice show:

immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 3:37:51 PM   ERROR [JobService] Unable to run job handler: Error: Input file is missing: upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/acb63ba0-9693-46e4-a63c-5c58f8060def.ARW
immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 3:37:51 PM   ERROR [JobService] Error: Input file is missing: upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/acb63ba0-9693-46e4-a63c-5c58f8060def.ARW
immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 3:37:51 PM   ERROR [JobService] Object:
immich-microservice-bcb587fdb-wf4tr immich-microservice {
immich-microservice-bcb587fdb-wf4tr immich-microservice   "id": "75b70416-fd3c-414a-8957-5ad936ce4946"
immich-microservice-bcb587fdb-wf4tr immich-microservice }
immich-microservice-bcb587fdb-wf4tr immich-microservice
immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 3:37:51 PM   ERROR [JobService] Unable to run job handler: Error: Input file is missing: upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/4f26b42c-d230-4808-a338-a048f529c5c6.ARW
immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 3:37:51 PM   ERROR [JobService] Error: Input file is missing: upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/4f26b42c-d230-4808-a338-a048f529c5c6.ARW
immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 3:37:51 PM   ERROR [JobService] Object:
immich-microservice-bcb587fdb-wf4tr immich-microservice {
immich-microservice-bcb587fdb-wf4tr immich-microservice   "id": "a4aa15df-b5a2-4506-9762-d192a1449830"
immich-microservice-bcb587fdb-wf4tr immich-microservice }
immich-microservice-bcb587fdb-wf4tr immich-microservice

A manual storage template migration also shows similar logs:

immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 4:02:51 PM   ERROR [StorageTemplateService] Problem applying storage template
immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 4:02:51 PM   ERROR [StorageTemplateService] Error: ENOENT: no such file or directory, rename 'upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/ace1c10d-3a30-4c61-9f66-7461370cdb6f.ARW' -> 'upload/library/admin/2022/2022-11-24/_DSC5135+1.ARW'
immich-microservice-bcb587fdb-wf4tr immich-microservice [Nest] 7  - 06/20/2023, 4:02:51 PM   ERROR [StorageTemplateService] Object:
immich-microservice-bcb587fdb-wf4tr immich-microservice {
immich-microservice-bcb587fdb-wf4tr immich-microservice   "id": "7dbecbbe-2cdb-4ef8-aae6-8d780db4a452",
immich-microservice-bcb587fdb-wf4tr immich-microservice   "source": "upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/ace1c10d-3a30-4c61-9f66-7461370cdb6f.ARW",
immich-microservice-bcb587fdb-wf4tr immich-microservice   "destination": "upload/library/admin/2022/2022-11-24/_DSC5135+1.ARW"
immich-microservice-bcb587fdb-wf4tr immich-microservice }

Further, it's not possible to download the image. Though interestingly there is exif data.

image

Server logs from this action:

immich-server-688c95f58d-bs56b immich-server [Nest] 7  - 06/20/2023, 4:24:16 PM   ERROR [ExceptionsHandler] ENOENT: no such file or directory, stat 'upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/f1801892-ed57-47d9-bc57-f699f31ffaf8.ARW'
immich-server-688c95f58d-bs56b immich-server Error: ENOENT: no such file or directory, stat 'upload/upload/63a658d3-c110-42e1-8621-7e345cfda737/f1801892-ed57-47d9-bc57-f699f31ffaf8.ARW'

A manual inspection of the filesystem shows the image was successfully moved, but the change was not persisted to the database. The image was supposed to be moved back, but wasn't.

/usr/src/app $ find upload -name '_DSC4726*'
upload/library/admin/2022/2022-11-23/_DSC4726.ARW
/usr/src/app $ find upload -name '63a658d3-c110-42e1-8621-7e345cfda737/f1801892-ed57-47d9-bc57-f699f31ffaf8*'
/usr/src/app $

Loki shows there were some issues connecting to the database, which isn't Immich's fault but Immich should handle this better.

image

image

The OS that Immich Server is running on

Kubernetes

Version of Immich Server

v1.61.1

Version of Immich Mobile App

N/A

Platform with the issue

  • Server
  • Web
  • Mobile

Your docker-compose.yml content

N/A

Your .env content

N/A

Reproduction steps

N/A

Additional information

No response

@uhthomas
Copy link
Member Author

Following some internal discussion on Discord, we believe it may be beneficial to queue asset migrations individually rather a single job which migrates all assets at once. This has some advantages:

  • Proper error handling per asset.
  • Individual asset migrations can be retried.
  • More consistent code, as all the other job queues behave this way.
  • Migrations sometimes take a long time (5+ minutes in my case).
  • Parallel job execution could be beneficial.

The current all-in-one migration will be marked as successful, even if some assets weren't moved correctly which isn't ideal.

@uhthomas
Copy link
Member Author

Further, it looks like 163 files failed to move.

https://gist.github.com/uhthomas/650ff9f08f83f8f0ffb3008653df395d

@uhthomas
Copy link
Member Author

We had a long discussion about the right way to approach this and concluded the best thing to do in addition to individual jobs, is to move some logic into the database. We don't have specifics yet, but some psuedo code:

move asset
const dst = await calculateDestination(asset);
if (!checkFileExists(dst)) {
  await moveFile(src, dst);
}
await save({});
calculate destination
const tx = new Transaction(); // idk how this works with typeorm, this is how it works in Go
const results = tx.find({ dst });
if (!results.length) {
  tx.save({ dst });
  tx.commit();
  return;
}
tx.save({ dst, suffix: `-${results.length}` });
tx.commit();

This solution essentially moves the location of an asset from the existing asset table, to a new table and abstracts the location away. The schema of the table could look like:

CREATE TABLE asset_location (
  id SERIAL PRIMARY KEY,
  asset_id  INT,
  location VARCHAR(255) NOT NULL,
  suffix VARCHAR(255),
  CONSTRAINT fk_asset
      FOREIGN KEY(asset_id) 
	  REFERENCES assets(id)
);

@uhthomas
Copy link
Member Author

uhthomas commented Jun 20, 2023

I wrote this script to move assets back to the right place.

https://gist.github.com/uhthomas/87cd39d0bbed044800a982556617f8db

@uhthomas
Copy link
Member Author

So I've been thinking about this a bit and in addition to this, it would be really nice to support other file systems like S3. We can do this by dropping support for custom folders and switch to a flat hierarchy of unique directory names. Such <uuid>/<filename>. This is quite nice as upload destinations are guaranteed to be unique and do not require any move operations, and the original filename is preserved. In addition, this approach is fully compatible with S3. It would make the process a lot more simple and more reliable. The only potential issue I envision is that hundreds of thousands of objects in a single directory may not be compatible with most file systems. As such, it would probably have to work more similar to a tree with multiple directories to balance the number of files and distribute them evenly (a cryptographic random string would be fine).

@uhthomas
Copy link
Member Author

I'd also like to add that at current, Immich does not actually preserve the original filename if there are duplicate files on the same day. They are prefixed with a number, like ABC-1.jpg. The proposed solution would make this redundant and have the added benefit of not needing to mangle the original filename on disk.

@bo0tzz
Copy link
Member

bo0tzz commented Jul 27, 2023

We used to do that and got a lot of pushback on it, which prompted the current custom folder support.

@uhthomas
Copy link
Member Author

uhthomas commented Jul 27, 2023

We used to do that and got a lot of pushback on it, which prompted the current custom folder support.

Was the implementation identical, or was it slightly different? Links would be really helpful! I can understand pushback from files names <uuid> or <uuid>.jpg, but <uuid>/<filename>.jpg is a good compromise imo. I am afraid that if something doesn't change here, we won't be able to solve some of the issues we have with upload and storage reliability and support for S3 would be much more tricky.

@alex-phillips
Copy link
Contributor

How would this potentially conflict with the recently added support for external / read-only assets? I imagine the database entry would simply refer to the filesystem location that doesn't match this proposed convention, though it may not matter if no filesystem operations are taking place against those assets.

@bo0tzz
Copy link
Member

bo0tzz commented Jul 27, 2023

I don't see an issue with S3 support - we don't need to use the same scheme on every storage backend, so we can just take this approach only for S3 if we need.

#1098 is the PR that added storage templating, it has some (scattered) links to relevant issues/discussions.

@uhthomas
Copy link
Member Author

Thanks for the links @bo0tzz, definitely helpful for gathering context.

I think that diverging too much with different implementations will make things difficult. Whether it's because of the technical implementation like where file are initially uploaded and then what happens when the file upload is complete, or whether it's because storage migrations and this custom folder structure just wouldn't work with something like S3. It's probably wise to keep behaviour as similar as possible to avoid bugs and maintenance overhead, the only real way to do that imo is to work with the lowest common denominator where S3 does not support a move operation.

How would this potentially conflict with the recently added support for external / read-only assets? I imagine the database entry would simply refer to the filesystem location that doesn't match this proposed convention, though it may not matter if no filesystem operations are taking place against those assets.

If designed correctly, it shouldn't conflict at all. Ideally, it would actually make it easier as there would be more effort put into a proper abstraction for file systems.

@alex-phillips
Copy link
Contributor

I would have some concern with moving away from an available feature like storage template for a software-specific implementation of managing this type of personal media. I know that Immich came from managing the filesystem structure in the past, but moving back to that makes this type of media highly inaccessible except from within the app itself. Especially in its current state where Immich doesn't (and arguably shouldn't?) fill every role of interacting with these types of files.

Example scenario: Immich is used for asset backup from devices, browsing, sharing, etc. But current metadata management and photo editing is not possible in Immich. Moving the filesystem structure to something that is not human friendly makes interacting with and using these assets extremely difficult to navigate.

@uhthomas
Copy link
Member Author

I appreciate and understand that perspective, I can definitely see how a bunch of randomly generated directory names would be hard to work with, regardless of whether or not the original filename is preserved. There is a middle-ground, where immich could persist uploaded items to directories created with the current date, like 2022/12/12/<uuid>/<filename>.jpg but this clearly still isn't ideal.

I think we have to come to some sort of conclusion on what we do or don't want to support. I am not sure it's possible to have our cake and eat it too. Either we design Immich to be reliable and scalable without regard for some features, or the existing behaviour is preserved with the caveat of disparate storage implementations, complexity and issues like this one.

With respect to one point, I think that if Immich is a backup solution, then users should not be editing their files directly.

Further thinking, given the read-only feature, I think that may be a good way to sort of support both at once? The files uploaded to Immich can be in any form they need to be, whereas files that users may want to work with locally can be part of that read-only directory.

Would be really interested to hear your further thoughts.

@etnoy
Copy link
Contributor

etnoy commented Jul 27, 2023

For read only files and upcoming external libraries, that's not an issue with this proposal. They just stay put where they happened to be, the db points to their filename, and that's it. I think this only relates to uploaded (i.e. internal) assets

@jrasm91

This comment was marked as resolved.

@uhthomas
Copy link
Member Author

Closing for #4442 and #4445.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants