fix(mobile): sync issue due to time mismatch #4659

fyfrey · 2023-10-26T16:07:13Z

Currently, the app can get sync issues because of a time mismatch between server and client. This can both occur due to timezone changes or simply wrong clocks.

With this PR, the client retrieves the time for the next sync request from the server, thus solving the issue.

vercel · 2023-10-26T16:07:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
immich	⬜️ Ignored (Inspect)	Visit Preview		Oct 26, 2023 4:37pm

jrasm91

Idk if this solves the problem either. The issue is the database may be in a different timezone than the server still. Really we should just send accurate timestamps with timezones

jrasm91 · 2023-10-26T19:24:28Z

server/src/domain/domain.constant.ts

@@ -4,6 +4,7 @@ import { extname } from 'node:path';
 import pkg from 'src/../../package.json';

 export const AUDIT_LOG_MAX_DURATION = Duration.fromObject({ days: 100 });
+export const AUDIT_LOG_CLEANUP_DURATION = Duration.fromObject({ days: 101 });


Why is this needed? Is this an attempt to resolve a potential issue with time differences between the server and the database?

yeah. it's simply being cautious. keep stuff for 1 day longer than needed if everything works perfect with the machine clocks and timezones

While that sounds reasonable, I'd much rather be accurate. If we were accurately requesting the change log, this would be unnecessary.

true. better safe than sorry for now?

fyfrey · 2023-10-26T19:40:15Z

Idk if this solves the problem either. The issue is the database may be in a different timezone than the server still. Really we should just send accurate timestamps with timezones

dates between app/server are now always in UTC; the server and datebase use timestampz which has timezones.. so it should be good?

jrasm91 · 2023-10-26T19:52:43Z

Idk if this solves the problem either. The issue is the database may be in a different timezone than the server still. Really we should just send accurate timestamps with timezones

dates between app/server are now always in UTC; the server and datebase use timestampz which has timezones.. so it should be good?

When we serialize a date and send it over json we're already using the ISO 8601 which is a timezone aware representation of the date and time. If this was passed along correctly to the database everything would be fine, so something is obviously not working correctly. My suspicion is TypeORM is adjusting the time, since JS Dates are only internally stored as numbers and have no sense of timezones. I believe it is offsetting it (incorrectly) to UTC.

https://github.com/typeorm/typeorm/blob/master/src/util/DateUtils.ts#L45-L80.

fyfrey · 2023-10-26T20:00:59Z

When we serialize a date and send it over json we're already using the ISO 8601 which is a timezone aware representation of the date and time. If this was passed along correctly to the database everything would be fine, so something is obviously not working correctly. My suspicion is TypeORM is adjusting the time, since JS Dates are only internally stored as numbers and have no sense of timezones. I believe it is offsetting it (incorrectly) to UTC.

https://github.com/typeorm/typeorm/blob/master/src/util/DateUtils.ts#L45-L80.

I see, typeORM doing random stuff again.
It could also use SystemTime instead of UTC (which might be different on the systems....)
I guess this would affect quite a lot of (our) code. Not sure how to fix this. Needs another PR

jrasm91 · 2023-10-26T20:45:01Z

A bigger change I am thinking about is switching to luxon date objects, to which you can do with custom type parsers. I wonder if we left it as a raw string if that would fix this endpoint at least.

fyfrey · 2023-10-27T08:53:45Z

We mount /etc/localtime:/etc/localtime:ro for server and microservices, but not for the database container. Maybe this leads to issues? Because I've never encountered any sync errors.. but I also run the postgres database on the host (thus it has the same /etc/localtime)

jrasm91 · 2023-10-27T11:13:22Z

Right if local time is the same it is never an issue. It should not be a requirement that the client, server, and database are on the same timezone though. I'm guessing this can be reproduced by having the client and server on different timezones.

alextran1502 · 2023-11-20T03:15:40Z

Hi Fynn, do you think we still need this PR?

fyfrey · 2023-11-20T17:58:45Z

Yeah, it's still an issue. I'll rebase and make some changes.

fyfrey · 2023-11-29T16:12:58Z

rebased and made sure the app always uses the server time when possible

So, this PR guards against sync issues originating from time divergence between client and server, e.g. due clock resets or timezone changes

jrasm91

I'm fine merging it as is. I think we still need to look at how dates get translated between the API body, server, and database, especially when the server and database are in different timezones, but this can be used in the meantime.

server/src/domain/audit/audit.service.ts

cloudflare-workers-and-pages · 2024-03-08T16:48:50Z

Deploying with Cloudflare Pages

Latest commit:	`d8413ea`
Status:	✅ Deploy successful!
Preview URL:	https://668752b9.immich.pages.dev
Branch Preview URL:	https://dev-fix-sync-time-mismatch.immich.pages.dev

View logs

jrasm91 · 2024-04-27T14:47:41Z

Superseded by #9100 and others

fyfrey added 📱mobile 🗄️server labels Oct 26, 2023

jrasm91 reviewed Oct 26, 2023

View reviewed changes

fyfrey force-pushed the dev/fix-sync-time-mismatch branch from f08c482 to eeaa67d Compare November 29, 2023 16:05

fix(mobile): sync issue due to time mismatch

4ce3676

fyfrey force-pushed the dev/fix-sync-time-mismatch branch from eeaa67d to 4ce3676 Compare November 29, 2023 16:08

prettier

061c567

jrasm91 approved these changes Nov 29, 2023

View reviewed changes

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

Fynn Petersen-Frey and others added 4 commits November 29, 2023 19:46

timeOfRequest -> requestedAt

e033e9a

fix tests

7f5d554

prettier

de7475f

Merge branch 'main' into dev/fix-sync-time-mismatch

6feac7d

vercel bot temporarily deployed to Preview December 11, 2023 16:54 Inactive

resolve conflict

81d6f69

fix

d8413ea

jrasm91 closed this Apr 27, 2024

alextran1502 deleted the dev/fix-sync-time-mismatch branch May 3, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mobile): sync issue due to time mismatch #4659

fix(mobile): sync issue due to time mismatch #4659

fyfrey commented Oct 26, 2023

vercel bot commented Oct 26, 2023 •

edited

Loading

jrasm91 left a comment

jrasm91 Oct 26, 2023

fyfrey Oct 26, 2023 •

edited

Loading

jrasm91 Oct 26, 2023

fyfrey Oct 26, 2023

fyfrey commented Oct 26, 2023

jrasm91 commented Oct 26, 2023

fyfrey commented Oct 26, 2023

jrasm91 commented Oct 26, 2023

fyfrey commented Oct 27, 2023 •

edited

Loading

jrasm91 commented Oct 27, 2023

alextran1502 commented Nov 20, 2023

fyfrey commented Nov 20, 2023

fyfrey commented Nov 29, 2023

jrasm91 left a comment

cloudflare-workers-and-pages bot commented Mar 8, 2024 •

edited

Loading

jrasm91 commented Apr 27, 2024

fix(mobile): sync issue due to time mismatch #4659

fix(mobile): sync issue due to time mismatch #4659

Conversation

fyfrey commented Oct 26, 2023

vercel bot commented Oct 26, 2023 • edited Loading

jrasm91 left a comment

Choose a reason for hiding this comment

jrasm91 Oct 26, 2023

Choose a reason for hiding this comment

fyfrey Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

jrasm91 Oct 26, 2023

Choose a reason for hiding this comment

fyfrey Oct 26, 2023

Choose a reason for hiding this comment

fyfrey commented Oct 26, 2023

jrasm91 commented Oct 26, 2023

fyfrey commented Oct 26, 2023

jrasm91 commented Oct 26, 2023

fyfrey commented Oct 27, 2023 • edited Loading

jrasm91 commented Oct 27, 2023

alextran1502 commented Nov 20, 2023

fyfrey commented Nov 20, 2023

fyfrey commented Nov 29, 2023

jrasm91 left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Mar 8, 2024 • edited Loading

Deploying with Cloudflare Pages

jrasm91 commented Apr 27, 2024

vercel bot commented Oct 26, 2023 •

edited

Loading

fyfrey Oct 26, 2023 •

edited

Loading

fyfrey commented Oct 27, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 8, 2024 •

edited

Loading