feat(mobile): use efficient sync

[fyfrey]

follow-up for #8755

enable the use of the more efficient endpoints to perform sync operations.

only merge after #9189 has been in a release for 1-2 weeks.

[cloudflare-pages]

Deploying immich with    Cloudflare Pages

Latest commit:	db6a66b
Status:	✅  Deploy successful!
Preview URL:	https://6dd0596e.immich.pages.dev
Branch Preview URL:	https://feat-app-efficient-sync.immich.pages.dev

View logs

[alextran1502]

while loop?

[fyfrey]

It's identical to while(true). I can change it but it does not really matter I'd say :)

[alextran1502]

I see this a lot in embedded system code, not used to seeing it in higher level language 😝

[fyfrey]

I personally use this for loops when I break out using other means.

While(true).. I use for endless tasks. Like a server listening indefinitely until it's killed etc.

[martyfuhry]

Can this be rewritten to a do while loop on the condition assets.length < chunkSize? I think that's the only break condition I see here. I concur that while (true) loops are odd and potentially dangerous. I also think for (;;) is less readable. I did not know what it meant until I read some of the other comments.

[fyfrey]

sadly, I can't put the break condition in the loop condition check: it would not even run the first pass of the loop - i can only to while(true) instead of for(;;). The loop has two break conditions that cannot be handled in the loop header

[martyfuhry]

I wish I could be more help, as I'm not tremendously familiar with the backup sync. But I'm happy to do any testing if you'd like me to, as well!

[alextran1502]

I ran into this error on the server when running the PR

Here is the mobile error log

[NicholasFlamy]

I ran into this error on the server when running the PR

Here is the mobile error log

I would guess this is caused by this:

immich/mobile/lib/shared/services/sync.service.dart

Line 219 in ce19a49

.filter()

[fyfrey]

I ran into this error on the server when running the PR

It seems some more server changes are required for stacked assets (my test instance probably had no stacked assets at the time...). I converted this PR back to a draft and will work on a new PR for the required server changes.

[jrasm91]

LGTM

[alextran1502]

Hi @fyfrey I tested this again and found a problem with rendering memory lane. Here is the error message

From my debugging, the API returned the correct data. Still, the call to get the asset from the database when building the model probably happened before the synchronization process finished, so it could not parse all data correctly, leading to the error.

Or, similar to previous symptom, not all assets are fetched

[fyfrey]

Hi @fyfrey I tested this again and found a problem with rendering memory lane. Here is the error message

Thanks @alextran1502 ! I've pushed a new commit to fix the bug in the memory lane. We were just lucky it did not occur previously..
Solution: Do not add a memory lane if assets are not yet in DB. Refresh memory lane provider after remote asset sync.

[alextran1502]

Hi @fyfrey, the change doesn't entirely fix the issue. I have multiple memories on a day but only the first memory gets rendered, so I checked the database. On main, I see 69000 assets in Isar database, while on this branch, I only see 64000 assets, so the fetching mechanism somehow doesn't fetch all assets

[fyfrey]

Hi @fyfrey, the change doesn't entirely fix the issue. I have multiple memories on a day but only the first memory gets rendered, so I checked the database. On main, I see 69000 assets in Isar database, while on this branch, I only see 64000 assets, so the fetching mechanism somehow doesn't fetch all assets

Thank you for testing again @alextran1502 . Very interesting. I did not notice any sync discrepancy (asset count) with my initial server changes. In the newer fix PR (#9100) we changed it to use the common asset query builder. It turns out it is incompatible with the app with stacked assets as the app expects all assets individually while the server returns them in an array with of the parent asset. This is probably the same issue you encountered a while ago and reverted server refactoring changes in #7752.

made another server PR to fix it: #9189

to test: run server with #9189 and mobile with this PR... or locally merge one into the other.. simply copy the few changes from #9189 into your local branch of this PR :)

[alextran1502]

I tried again with #9189 on my prod (FOR SCIENCE!!) and I still see missing memories. Checking the database again, now I see 68000 assets are fetched on this PR while still 69000 assets on main

[fyfrey]

Something must still be different between the old and the new server query.
We need some internal test instance that has all the edge cases like your prod instance does :-P

The old method also fetched archived assets for partners, maybe this accounts for the difference. But memories should not show archived assets.... I'll take another look at the code, or better the generated SQL.

Edit:
I've changed #9189 to also return archived assets for partners (just the legacy getAllAsset method). I hope this resolves the difference also for your prod instance.