sboesen / remotely-sync

fork of remotely-save with security upgrades
Apache License 2.0
180 stars 8 forks source link

[Bug]: Sync on Remote running before sync complete #91

Closed kadisonm closed 6 months ago

kadisonm commented 6 months ago

What happened?

On sync the metadata is uploaded first, but if it is a large sync then the other device will detect it and try to sync before the first device is done syncing.

Additionally there is another small bug where sync on remote will try to run without auth causing an error.

What OS are you using?

Windows

What remote cloud services are you using?

Dropbox

Version of the plugin

No response

Version of Obsidian

No response

Using password or not

Ensure no sensitive information

kadisonm commented 6 months ago

Is there any reason metadata is uploaded before syncing? If not it's probably best to change it to after for the Sync History as well.

sboesen commented 6 months ago

Not that I'm aware of, we should upload after sync so this doesn't occur.

sboesen commented 6 months ago

Fixed in 37b2c97

sboesen commented 6 months ago

Fixed in 0.4.36

kadisonm commented 6 months ago

Thank you!

fyears commented 6 months ago

(author or remotely save here)

actually the plugin doesn’t provide a lock or a transaction which the real problem behind scene. moving meta data upload sequence doesn’t solve the problem at all i am afraid.

kadisonm commented 6 months ago

Sorry but what do you mean by lock and transaction?

Thanks for commenting by the way. I want to get this feature properly working as soon as possible.

sboesen commented 6 months ago

moving meta data upload sequence doesn’t solve the problem at all i am afraid.

It doesn't fix the race condition present with multiple clients syncing, but it does mitigate a bug with with "Sync on Remote Changes" (an update to Remotely Sync) which resulted in clients scanning the metadata file to see if it changed since last sync; this resulted in multiple clients quickly checking this metadata file resulting in unnecessary (endless?) syncing. Agreed the main race condition is not mitigated but that wasn't the goal for this issue.

Thankfully that race condition should only occur if multiple clients have changes they need to sync, which for my usage at least is an edge case.

Sorry but what do you mean by lock and transaction?

fyears@ is talking about something like mutual exclusion (mutex) or other forms of locking. Like we could check if a .lock file exists in the remote location and abort sync if so. At start of sync: create a .lock file when starting sync, etc.

Even this naive implementation has a race condition, because in theory two sync clients could check the file exists, think it doesn't, then both create it, like below:

Ideal (happy path): Client A: Does lock exist? -> No Client A: Create lock file -> Done Client B: Does lock exist? -> Yes, abort sync.

Race condition (unhappy path): Client A: Does lock exist? -> No Client B: Does lock exist? -> No [both clients start syncing, same problem]

Not all providers (including s3) support mechanisms to solve this like conditional write ("create this file only if it doesn't exist- if it exists, error") so we can't implement this (at least easily) for all providers. Some providers like webdav do support locking, but this is probably implementation dependent.

kadisonm commented 6 months ago

Ohhh thanks for the explanation. I hadn't thought about that before. Maybe when we fix the current sync issues (deletions, file hashes, etc) this could be implemented. If it's possible?

Not all providers (including s3) support mechanisms to solve this like conditional write ("create this file only if it doesn't exist- if it exists, error")

Can't the plugin check if the file exists, then upload the file?

sboesen commented 6 months ago

No worries!

Can't the plugin check if the file exists, then upload the file?

That's the problem - these operations are not guaranteed to happen in any particular order. Because the read and write are separate operations and could occur for two clients at the same time it's rare but possible that the unhappy path I described above happens - broken down more clearly:

Client A: Does lock exist? [takes time] Client B: Does lock exist? [takes time] Client A: Lock does not exist. Client B: Lock does not exist. Client A: Create lock file Client B: Create lock file <--- start of the problem.

Both clients think they created the lock file because they did not see a lock file, but unfortunately they checked at the same time (when a lock file didn't exist).

There are some workarounds but I don't think it's super reliable as a server-supported option. Like writing a number to the lock file, then reading it back to see if the number changed (like another client is trying to get a lock). It's possible this still results in a race condition, but less likely.

Worth discussing in a new (separate) issue! This is not super likely to happen compared to this issue because of how Sync on Remote [changes] was implemented.

kadisonm commented 6 months ago

Oh I see, thanks again. Makes more sense now.

A new issue sounds good, at least for further down the line. I assume this wouldn't be a priority right now, but might be an improvement later?

sboesen commented 6 months ago

Yeah! I agree. It would only happen if two clients have to sync at the same time - which should happen less often with a proper "Sync on Remote Changes" type feature, if it's working as intended. Worth opening as a nice to have but shouldn't be an issue in practice with out of the box obsidian (maybe some plugin that auto updates a file, and syncing config dirs... there are cases but for most people it should be fine).