feat(server): refresh external assets piecemeal #7934

etnoy · 2024-03-13T20:59:10Z

There have recently been improvements that help refreshing large external libraries. However, it is still capped at some limit, depending on available memory and the size of the library.

This PR lets us remove the upper limit of library refreshes. We previously kept a list of all assets in a library being refreshed because we would track which assets have been removed. This is the list that kept eating all memory. We no longer build a huge list from the filesystem crawl. Instead, we request them 5000 at a time and then discard when the corresponding refresh jobs have been queued.

This, however, means the library refresh jobs can't detect when a library asset goes offline, or when an offline asset goes back online. We move these actions to a separate job (but WIP maybe we can run it inline with normal refresh jobs??)

Quick testing indicated I was able to refresh millions of assets without a sweat on a VM with only 10G of ram. Even larger numbers are possible with even less memory; I just haven't bothered to test more ;)

cloudflare-pages · 2024-03-13T20:59:43Z

Deploying immich with Cloudflare Pages

Latest commit:	`b3bb5c5`
Status:	✅ Deploy successful!
Preview URL:	https://36ac0a6f.immich.pages.dev
Branch Preview URL:	https://feat-offline-files-job.immich.pages.dev

View logs

…/offline-files-job

server/src/domain/library/library.service.ts

server/e2e/jobs/specs/library.e2e-spec.ts

etnoy · 2024-03-20T21:05:26Z

Would it be better to still have the offline and online stuff as separate jobs, but just queue both of them? That approach was a lot faster and used less memory when I tested it.

No, because for a large refresh it can potentially take a long time before either offline or online starts running. By interweaving the queue you make it feel snappier to the user. In my opinion.

server/e2e/jobs/specs/library.e2e-spec.ts

server/src/domain/library/library.service.ts

danieldietzler · 2024-03-21T16:36:25Z

server/src/domain/library/library.service.ts

+    const checkIfOnlineAssetsAreOffline = async () => {
+      const existingAssetPage = await onlineAssets.next();
+      existingAssetsDone = existingAssetPage.done ?? true;
+
+      if (existingAssetPage.value) {
+        existingAssetCounter += existingAssetPage.value.length;
+        this.logger.log(
+          `Queuing online check of ${existingAssetPage.value.length} asset(s) in library ${library.id}...`,
+        );
+        await this.jobRepository.queueAll(
+          existingAssetPage.value.map((asset: AssetEntity) => ({
+            name: JobName.LIBRARY_CHECK_OFFLINE,
+            data: { id: asset.id, importPaths: validImportPaths },
+          })),
+        );
      }
-    }
+    };


If you already put it in its own function, why keep it in there and not move it out into its own private function?

It's simply because we need to use variables in the parent context

Can't you just pass the values? IMO arrow functions should be (as well as possible) independent and not have any side effects.

server/src/domain/library/library.service.ts

server/src/interfaces/asset.repository.ts

…/offline-files-job

server/src/services/library.service.ts

jrasm91 · 2024-03-22T23:11:02Z

server/src/services/library.service.ts

-      this.logger.debug(`Found ${assetIdsToMarkOffline.length} offline asset(s) previously marked as online`);
-      await this.assetRepository.updateAll(assetIdsToMarkOffline, { isOffline: true });
-    }
+    while (!crawlDone) {


This is a bit weird. We should be able to use a normal (async) for loop here instead.

Normally a for loop would be preferable. I switched to a while loop because I wanted to do this in batches of 1000 at a time

I don't understand. Why can't you do

for await (const crawlResult of crawledAssets) { ... }

?

The code needs to do two tasks, and to make the system feel more responsive I don't want to wait for the first (potentially very long) job to be worked on before the next jobs start. We therefore interleave the two jobs in between

This reply does not make any sense to me. Doesn't this semantically do the exact same thing?

jrasm91 · 2024-03-22T23:12:42Z

server/src/services/library.service.ts

-          await this.scanAssets(job.id, batch, library.ownerId, job.refreshAllFiles ?? false);
-          batch = [];
+        if (!existingAssetsDone) {
+          // Interweave the queuing of offline checks with the asset scanning (if any)


What is the benefit of this? If anything it seems like it would be less efficient.

We have two things happening in parallel. One queue checks the list of crawled assets on the file system. The other queue checks if files in the db are still online. In order to speed up the percieved quickness of the system I am picking 1000 from each queue at a time. This makes the system feel snappier because it doesn't wait for all files to finish scanning before noticing an existing file being offline, and vice versa. This final loop checks if there are more things to pick up from the "is this asset still online?" queue.

Hope this makes sense

server/src/services/library.service.ts

…/offline-files-job

server/src/services/library.service.ts

…/offline-files-job

danieldietzler

This has already gotten much more readable, thanks!
And sorry that PR got stale lol

server/e2e/jobs/specs/library.e2e-spec.ts

server/src/services/library.service.spec.ts

danieldietzler · 2024-04-05T13:57:06Z

server/src/services/library.service.ts

-      this.logger.debug(`Found ${assetIdsToMarkOffline.length} offline asset(s) previously marked as online`);
-      await this.assetRepository.updateAll(assetIdsToMarkOffline, { isOffline: true });
-    }
+    while (!crawlDone) {


I don't understand. Why can't you do

for await (const crawlResult of crawledAssets) { ... }

?

danieldietzler · 2024-04-05T13:58:28Z

server/src/services/library.service.ts

-      let batch = [];
-      for (const assetPath of crawledAssetPaths) {
-        batch.push(assetPath);
+      if (crawledAssetPaths.length % LIBRARY_SCAN_BATCH_SIZE === 0 || crawlDone) {


Can't we move that whole if outside the loop? Once we quit that loop we inherently are done with the scanning, no?

No, we aren't done with the scanning after the loop, it also needs to check if any existing assets need to be checked.

I don't understand this. You're implying that batching does not give you all results?

Maybe I am also just very confused... This whole interleaving those two jobs may make it more performant but it definitely adds a lot of complexity my brain apparently can't comprehend.

I didn't mean to approve this yet oops

…/offline-files-job

add job to check for offline files

5e497e5

etnoy added 🗄️server external-library Issues related to external libraries labels Mar 13, 2024

etnoy added 13 commits March 14, 2024 00:19

fix lint

8bb73d6

only check for offline when using checkForOffline

247429c

improve tests

0803458

Merge branch 'main' of https://github.com/immich-app/immich into feat…

f68bcf0

…/offline-files-job

remove old test

d09d4d3

wip

5b581ce

remove trie

3b0d993

refactor batches

4ba95bb

also check offline status

95b57f0

Merge branch 'main' of https://github.com/immich-app/immich into feat…

68a4925

…/offline-files-job

Merge branch 'main' of https://github.com/immich-app/immich into feat…

d8dd1fb

…/offline-files-job

fix spelling

311d7d5

don't do offline scan

d7a78e5

etnoy changed the title ~~feat(server): add job to check for offline files~~ feat(server): refresh external assets piecemeal Mar 15, 2024

etnoy added 8 commits March 15, 2024 23:33

rename scan to check

fa3a70a

Merge branch 'main' of https://github.com/immich-app/immich into feat…

8bcee7f

…/offline-files-job

fix job statuses

ff47d55

fix lint

380ae35

cleanup

f7f30a5

add test

e26f8b4

open-api

5ae4fb8

fix test

f8039a7

etnoy marked this pull request as ready for review March 15, 2024 23:43

etnoy requested review from jrasm91 and mertalev March 15, 2024 23:43

mertalev reviewed Mar 16, 2024

View reviewed changes

server/src/domain/library/library.service.ts Outdated Show resolved Hide resolved

server/src/domain/library/library.service.ts Outdated Show resolved Hide resolved

server/e2e/jobs/specs/library.e2e-spec.ts Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin' into feat/offline-files-job

f48992b

etnoy added 3 commits March 20, 2024 22:18

fix merge

38e2cde

Merge remote-tracking branch 'origin' into feat/offline-files-job

7043f2f

fix lint

6fd511e

danieldietzler reviewed Mar 21, 2024

View reviewed changes

Merge branch 'main' of https://github.com/immich-app/immich into feat…

fbee95b

…/offline-files-job

jrasm91 reviewed Mar 22, 2024

View reviewed changes

etnoy added 10 commits March 25, 2024 22:40

Merge branch 'main' of https://github.com/immich-app/immich into feat…

17216e1

…/offline-files-job

add library job back

2398edf

add offline job to correct queue

2c4b174

library spec compiles now

f2c7235

move one test to new e2e

5745010

fix comments

52a19b6

fix comments

2d40c85

fix lint

cfbad58

Merge branch 'main' of https://github.com/immich-app/immich into feat…

37ad05f

…/offline-files-job

refactor path validation

586bf19

jrasm91 reviewed Mar 27, 2024

View reviewed changes

server/src/services/library.service.ts Show resolved Hide resolved

etnoy added 5 commits March 30, 2024 21:52

Merge branch 'main' of https://github.com/immich-app/immich into feat…

5515f57

…/offline-files-job

Merge branch 'main' of https://github.com/immich-app/immich into feat…

ee0fedf

…/offline-files-job

fix loop bug

17f2adb

remove logging

0348372

Merge branch 'main' of https://github.com/immich-app/immich into feat…

a352f7e

…/offline-files-job

danieldietzler previously approved these changes Apr 5, 2024

View reviewed changes

Merge branch 'main' of https://github.com/immich-app/immich into feat…

b218e0d

…/offline-files-job

etnoy mentioned this pull request Apr 7, 2024

"Error: EMFILE: too many open files" when adding large external library #8592

Open

3 tasks

etnoy added 2 commits April 10, 2024 00:29

Merge branch 'main' of https://github.com/immich-app/immich into feat…

2a9f615

…/offline-files-job

expect responses

b3bb5c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): refresh external assets piecemeal #7934

feat(server): refresh external assets piecemeal #7934

etnoy commented Mar 13, 2024 •

edited

cloudflare-pages bot commented Mar 13, 2024 •

edited

etnoy commented Mar 20, 2024

danieldietzler Mar 21, 2024

etnoy Mar 26, 2024

danieldietzler Apr 5, 2024

jrasm91 Mar 22, 2024

etnoy Mar 25, 2024 •

edited

danieldietzler Apr 5, 2024

etnoy Apr 9, 2024

danieldietzler Apr 10, 2024

jrasm91 Mar 22, 2024

etnoy Mar 25, 2024

danieldietzler left a comment

danieldietzler Apr 5, 2024

danieldietzler Apr 5, 2024

etnoy Apr 9, 2024

danieldietzler Apr 10, 2024

danieldietzler Apr 10, 2024 •

edited

feat(server): refresh external assets piecemeal #7934

Are you sure you want to change the base?

feat(server): refresh external assets piecemeal #7934

Conversation

etnoy commented Mar 13, 2024 • edited

cloudflare-pages bot commented Mar 13, 2024 • edited

Deploying immich with Cloudflare Pages

etnoy commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etnoy Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietzler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietzler Apr 10, 2024 • edited

Choose a reason for hiding this comment

etnoy commented Mar 13, 2024 •

edited

cloudflare-pages bot commented Mar 13, 2024 •

edited

etnoy Mar 25, 2024 •

edited

danieldietzler Apr 10, 2024 •

edited