lib/model: Newly created conflicting files can be silently overwritten

[mpx]

Syncthing overwrote a conflicting file during a sync without noticing the conflict.

I expected Syncthing to notice the file it was about to overwrite had not yet been seen through a local scan and either rename it, or create the new file with a "conflict" filename.

Syncthing Version: v0.4.11 both sides
OS Version: Windows 10 / FreeBSD 9.3 / Fedora 24

Steps:

1. Add a new file X on machine A (Windows 10).
2. Add a new file X on machine B (FreeBSD 9.3) with different content.
3. Rescan on machine A. Triggers sync to machine B.

Machine B overwrites file X without warning since it hasn't discovered it via a scan yet.
I've tested this between a few different OSes in different directions.
Other observations:

• Modifying existing files on both sides before a single rescan appears to work correctly.
• Deleting the test files then rescanning/recreating on both sides/rescanning appears to generate the expected conflict file.

It is slightly contrived I know, but it is more likely when using longer rescan intervals to conserve battery / IOPS. There is no safety net if multiple machines create a file with the same name within a rescan interval.

[AudriusButkevicius]

Yeah it causes a bunch of other issues too, I think we already have an issue for this (or atleast relating to this).

[calmh]

I think we check for unexpected conflicts on files we know exist, but not for files we don't know exist.

[Ferroin]

Just looking at this, a couple of thoughts come up:

• This is probably significantly less likely to impact people using file change notifications on the receiving system because syncthing-inotify (or whatever other tool) is likely to notify Syncthing of the new file pretty quickly (much smaller race window).
• Fixing this would require adding a check on the receiving end that the destination file doesn't already exist. Ideally, that check should happen just before we rename the temporary file into place after downloading everything. Such a check would slow down synchronization (you have to get a directory listing after each file downloaded.
• Because of both of the above factors, if this does get implemented, it should be done in a way that it can be disabled at runtime, as some users won't ever have this issue (I would actually expect most people to not have this issue).

[calmh]

I don't think it needs to be that bad. We need to to a stat before replacing, which we already do if the destination file is in the index (i.e., almost always).

There will still be a window between us checking and the rename actually happening, but that's at least down to the millisecond range.

[Ferroin]

Ah, I actually hadn't known Syncthing calls stat() just before replacing the destination file. If that's the case, then that pretty much negates my argument about performance (and thus the argument for this being configurable).