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] Rclone not supported because of one move command #2831

Closed
1 of 3 tasks
cycneuramus opened this issue Jun 17, 2023 · 6 comments · Fixed by #2832
Closed
1 of 3 tasks

[BUG] Rclone not supported because of one move command #2831

cycneuramus opened this issue Jun 17, 2023 · 6 comments · Fixed by #2832
Labels
enhancement New feature or request nice to have Nice to have feature 🗄️server

Comments

@cycneuramus
Copy link
Contributor

The bug

If I'm reading the source code correctly, the reason Immich currently doesn't support rclone mounted backends (see #1899, #1683) is simply because:

  • its way of moving files is by relying on the mv library, which:
    • works by calling fs.link (at least on clobber: false, which Immich is currently using), which:
      • creates a hard link of the source file at the destination path, at which:
        • mv will unlink the source file, thus having "moved" the file from source to destination.

Now, the problem is, rclone doesn't support hard links. Hence the move operation fails halfway with the ENOSYS: function not implemented, link ... error mentioned in the issues linked above.

I don't have a thorough understanding of the way Immich is supposed to work here, but if the point is to move the files and nothing more, it seems to me we could achieve full rclone compatibility—and with it support for countless storage backends—simply by reworking the move command (say, by using fs.rename(sourcePath, desPath); or something).

The OS that Immich Server is running on

Docker

Version of Immich Server

latest

Version of Immich Mobile App

latest

Platform with the issue

  • Server
  • Web
  • Mobile

Your docker-compose.yml content

not relevant

Your .env content

not relevant

Reproduction steps

Use an `rclone` remote as a storage backed for Immich and try e.g. to run the storage template migration job.

Additional information

No response

@cycneuramus cycneuramus added bug Something isn't working needs triage Bug that needs triage from maintainer labels Jun 17, 2023
@uhthomas
Copy link
Member

Hmm, so I believe Immich uses clobber to avoid overwriting existing files. The underlying package seems to assume that linking the file will implicitly not overwrite the destination, and is backed up by the documentation on link.

If newpath exists, it will not be overwritten.

Immich should probably just check if the file exists manually instead. This does introduce a potential race condition, though it may be behaviour which should just be accepted.

@cycneuramus
Copy link
Contributor Author

cycneuramus commented Jun 17, 2023

I wonder then what is the desired behavior. The clobber: false option seems to involve deleting the source file, so in moving away from hard links, would the same behavior therefore be achieved by checking manually for the destination file and, if found, simply delete the source...?

@uhthomas
Copy link
Member

uhthomas commented Jun 17, 2023

I believe link will error if the destination exists.

EEXIST newpath already exists.

@cycneuramus
Copy link
Contributor Author

Are we talking about the same thing here? The mv library (to which I was referring) calls the fs.unlink() method.

@uhthomas
Copy link
Member

I think so. If you read the source for doRename, it will only unlink if there was no error during linking. Therefore, if the destination exists then the source remains untouched and an error is returned.

@cycneuramus
Copy link
Contributor Author

Ah, I misunderstood the reference to link. In any case, I'll see if I can cook up a PR one of these days and get the ball rolling.

@jrasm91 jrasm91 added enhancement New feature or request nice to have Nice to have feature 🗄️server and removed bug Something isn't working needs triage Bug that needs triage from maintainer labels Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request nice to have Nice to have feature 🗄️server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants