VFS wipe moved folder when conflict #3425

allexzander · 2021-06-10T13:46:57Z

No description provided.

mgallien · 2021-06-11T16:28:20Z

so if I understand correctly this is going to happen in those cases:

server has directory serverDir
client A rename serverDir to clientDir
before sync happen client B rename serverDir to conflictDir and is using CfApi VFS on Windows

what is not clear to me is what is being sent by server and client when sync happen for client B ?

why conflict detection is not being triggered ?
I would have believed that rename from server to client would lead to detection that we have a conflict like for files

back to your patch

are you sure the criteria is going to work because I would imagine that renaming a folder would also happen when you already had read some files inside it ?
in that case some of the files inside would be pure placeholders and some would be implicitly hydrated

allexzander · 2021-06-11T16:41:56Z

@mgallien

what is not clear to me is what is being sent by server and client when sync happen for client B ?

If client B had a folder serverDir hydrated, then the entire folder conflictDir will get uploaded, similar to how it will get uploaded if not using VFS. So, there will be 2 folders at the end: clientDir and conflictDir (in my TODO comment I suggest we do not upload a second rename and do not create a duplicate, but, this has been working for a while so I didn't want to introduce too much difference just now).

why conflict detection is not being triggered ?
I would have believed that rename from server to client would lead to detection that we have a conflict like for files

Conflict detection works only when the same file modified (the content of the file is changed) from 2 places simultaneously. Renaming the file does not trigger a conflict. With non-VFS mode, renaming is not resulting in a conflict, because both files are uploaded. You can try it yourself to confirm. I've tested this multiple times. Simultaneous renaming does not work when using WebUI from 2 different accounts. The second rename just does not work and there is an error that pops up. This wouldn't work the same way in the desktop client. We can rename files while we are offline too.

are you sure the criteria is going to work because I would imagine that renaming a folder would also happen when you already had read some files inside it ?
in that case some of the files inside would be pure placeholders and some would be implicitly hydrated

In case a client B had at least one file hydrated in the folder serverDir, then, we will not wipe this folder and one or more files that are hydrated. We will only wipe placeholders (because there is no way for us to re-upload them as they are removed from the source directory locally and remotely), and, we will re-upload a folder with it's new name conflictDir and all the files that were hydrated. This is identical to how it works with non-VFS mode. Both folders are then present, but, for the client B, only those files that were hydrated will get uploaded into a remote conflictDir.

This is the case for const bool isFolderPlaceholder = localEntry.isDirectory && *_discoveryData->_syncOptions._vfs->availability(path._local) == VfsItemAvailability::OnlineOnly;.

mgallien · 2021-06-14T10:15:42Z

I did a quick check on Linux side and renaming a directory would result in a MOVE request being sent to the server.
Is this also the case when running on Windows with and without CfApi virtual files ?

allexzander · 2021-06-14T11:51:50Z

I did a quick check on Linux side and renaming a directory would result in a MOVE request being sent to the server.
Is this also the case when running on Windows with and without CfApi virtual files ?

@mgallien Yes this happens in a similar fashion on Windows. With and without CfAPI. However, this will not work the same way if you rename/move the same directory by 2 different clients with 2 different accounts. The second client will not detect rename and will not issue a MOVE request. Have you tried this scenario as well?

mgallien · 2021-06-14T12:15:13Z

@allexzander So if I understand the PROPFIND that are called before deciding if it is a rename are making it decide something else ?

src/libsync/discovery.cpp

src/gui/tray/ActivityData.h

src/libsync/discovery.cpp

src/gui/tray/UserModel.h

allexzander · 2021-06-14T13:38:54Z

@allexzander So if I understand the PROPFIND that are called before deciding if it is a rename are making it decide something else ?

@mgallien
About this, I am not sure. Where is PROPFIND called?

mgallien · 2021-06-14T14:09:00Z

@allexzander So if I understand the PROPFIND that are called before deciding if it is a rename are making it decide something else ?

@mgallien
About this, I am not sure. Where is PROPFIND called?

so maybe we can conclude this one and then we try to fix logic of rename and conflict detection because from the log I am not very happy with current state

2021-06-14 14:35:53:972 [ info sync.discovery ]:       Processing "test rename 5" | valid: true/false/false | mtime: 1623672635/0/0 | size: 0/0/0 | etag: "60c7473b938af"//"" | checksum: ""//"" | perm: 0x55dbd48f7c58//0x7fa8af128978 | fileid: "00005083oc9y77xnu65f"//"" | inode: 13918312/0/ | type: CSyncEnums::ItemTypeDirectory/CSyncEnums::ItemTypeSkip/CSyncEnums::ItemTypeFile | e2ee: false/false | e2eeMangledName: ""/""                                                                                                                        │
│2021-06-14 14:35:53:972 [ info sync.discovery ]:       Stale DB entry                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::bindValue ]:   SQL bind 1 3589020237305718845                                                                                                                                                                                                                                                                                                                                                                                                                                 │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::exec ]:        SQL exec "DELETE FROM metadata WHERE phash=?1"                                                                                                                                                                                                                                                                                                                                                                                                                 │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::exec ]:        Last exec affected 1 rows.                                                                                                                                                                                                                                                                                                                                                                                                                                     │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::bindValue ]:   SQL bind 1 "test rename 5"                                                                                                                                                                                                                                                                                                                                                                                                                                     │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::exec ]:        SQL exec "DELETE FROM metadata WHERE (path > (?1||'/') AND path < (?1||'0'))"                                                                                                                                                                                                                                                                                                                                                                                  │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::exec ]:        Last exec affected 11 rows.                                                                                                                                                                                                                                                                                                                                                                                                                                    │
│2021-06-14 14:35:53:972 [ info sync.discovery ]:       Processing "test rename 6" | valid: false/false/true | mtime: 0/0/1623672635 | size: 0/0/0 | etag: ""//"60c7473b938af" | checksum: ""//"" | perm: 0x7fa8af128978//0x55dbd48c27e8 | fileid: ""//"00005083oc9y77xnu65f" | inode: 0/0/ | type: CSyncEnums::ItemTypeSkip/CSyncEnums::ItemTypeSkip/CSyncEnums::ItemTypeDirectory | e2ee: false/false | e2eeMangledName: ""/""                                                                                                                               │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::bindValue ]:   SQL bind 1 "00005083oc9y77xnu65f"                                                                                                                                                                                                                                                                                                                                                                                                                              │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::exec ]:        SQL exec "SELECT path, inode, modtime, type, md5, fileid, remotePerm, filesize,  ignoredChildrenRemote, contentchecksumtype.name || ':' || contentChecksum, e2eMangledName, isE2eEncrypted  FROM metadata  LEFT JOIN checksumtype as contentchecksumtype ON metadata.contentChecksumTypeId == contentchecksumtype.id WHERE fileid=?1"                                                                                                                          │
│2021-06-14 14:35:53:972 [ info nextcloud.sync.accessmanager ]: 6 "PROPFIND" "https://nextcloud.lan:8443/remote.php/dav/files/admin/test rename 6" has X-Request-ID "9a605bd6-f055-4963-98a0-792c07e6f7a2"                                                                                                                                                                                                                                                                                                                                                     │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.cookiejar ]     [ OCC::CookieJar::cookiesForUrl ]:      QUrl("https://nextcloud.lan:8443/remote.php/dav/files/admin/test rename 6") requests: (QNetworkCookie("__Host-nc_sameSiteCookielax=true; secure; HttpOnly; expires=Fri, 31-Dec-2100 23:59:59 GMT; domain=nextcloud.lan; path=/"), QNetworkCookie("__Host-nc_sameSiteCookiestrict=true; secure; HttpOnly; expires=Fri, 31-Dec-2100 23:59:59 GMT; domain=nextcloud.lan; path=/"), QNetworkCookie("oc_sessionPassphrase=lndjGHfgS96NfZNKmTyv%2BQvT1g8xm6Bz│
│2021-06-14 14:35:53:972 [ info nextcloud.sync.networkjob ]:    OCC::PropfindJob created for "https://nextcloud.lan" + "/test rename 6" "OCC::DiscoveryPhase"                                                                                                                                                                                                                                                                                                                                                                                                  │
│2021-06-14 14:35:53:972 [ info sync.discovery ]:       Processing "test rename 7" | valid: false/true/false | mtime: 0/1623672635/0 | size: 0/4096/0 | etag: ""//"" | checksum: ""//"" | perm: 0x7fa8af128978//0x7fa8af128978 | fileid: ""//"" | inode: 0/13918312/ | type: CSyncEnums::ItemTypeSkip/CSyncEnums::ItemTypeDirectory/CSyncEnums::ItemTypeFile | e2ee: false/false | e2eeMangledName: ""/""                                                                                                                                                      │
│2021-06-14 14:35:53:972 [ warning sync.discovery ]:    File "test rename 7" was modified before the last sync run and is not in the sync journal and server                                                                                                                                                                                                                                                                                                                                                                                                   │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::bindValue ]:   SQL bind 1 13918312                                                                                                                                                                                                                                                                                                                                                                                                                                            │
│2021-06-14 14:35:53:972 [ debug nextcloud.sync.database.sql ]  [ OCC::SqlQuery::exec ]:        SQL exec "SELECT path, inode, modtime, type, md5, fileid, remotePerm, filesize,  ignoredChildrenRemote, contentchecksumtype.name || ':' || contentChecksum, e2eMangledName, isE2eEncrypted  FROM metadata  LEFT JOIN checksumtype as contentchecksumtype ON metadata.contentChecksumTypeId == contentchecksumtype.id WHERE inode=?1"                                                                                                                           │
│

or

2021-06-14 14:52:46:709 [ warning nextcloud.sync.networkjob ]: QNetworkReply::ContentNotFoundError "Server replied \"404 Not Found\" to \"PROPFIND https://nextcloud.lan:8443/remote.php/dav/files/admin/test rename 7\"" QVariant(int, 404)                                                                                                                                                                                                                                                                                                                  │
│2021-06-14 14:52:46:709 [ warning nextcloud.sync.credentials.webflow ]:        QNetworkReply::ContentNotFoundError                                                                                                                                                                                                                                                                                                                                                                                                                                            │
│2021-06-14 14:52:46:709 [ warning nextcloud.sync.credentials.webflow ]:        "Error transferring https://nextcloud.lan:8443/remote.php/dav/files/admin/test rename 7 - server replied: Not Found"                                                                                                                                                                                                                                                                                                                                                           │
│2021-06-14 14:52:46:709 [ info nextcloud.sync.networkjob.etag ]:       Request Etag of QUrl("https://nextcloud.lan:8443/remote.php/dav/files/admin/test rename 7") FINISHED WITH STATUS "ContentNotFoundError Server replied \"404 Not Found\" to \"PROPFIND https://nextcloud.lan:8443/remote.php/dav/files/admin/test rename 7\""                                                                                                                                                                                                                           │
│2021-06-14 14:52:46:709 [ info sync.discovery ]:       Can't rename because the etag has changed or the directory is gone "test rename 7"                                                                                                                                                                                                                                                                                                                                                                                                                     │
│2021-06-14 14:52:46:709 [ info sync.discovery ]:       Discovered "test rename" CSyncEnums::CSYNC_INSTRUCTION_NEW OCC::SyncFileItem::Up CSyncEnums::ItemTypeDirectory                                                                                                                                                                                                                                                                                                                                                                                         ┤
│

allexzander · 2021-06-14T14:35:14Z

@mgallien

so maybe we can conclude this one and then we try to fix logic of rename and conflict detection because from the log I am not very happy with current state

Yes. Sounds like a plan.

src/gui/tray/UserModel.cpp

mgallien

Please clean history before merging

…re are hydrated items inside. Signed-off-by: allexzander <blackslayer4@gmail.com>

FlexW · 2021-06-15T11:11:53Z

src/gui/tray/UserModel.cpp

+{
+    const auto folderInstance = FolderMan::instance()->folder(folderAlias);
+    if (!folderInstance)
+        return;


Missing braces

FlexW · 2021-06-15T11:12:19Z

src/gui/tray/UserModel.cpp

+void User::slotCheckExpiredActivities()
+{
+    for (const Activity &activity : _activityModel->errorsList()) {
+        if (activity._expireAtMsecs > 0 && QDateTime::currentDateTime().toMSecsSinceEpoch() >= activity._expireAtMsecs) {


In some places you use ms and in other msecs. I would settle for one :)

Signed-off-by: allexzander <blackslayer4@gmail.com>

nextcloud-desktop-bot · 2021-06-15T11:38:06Z

AppImage file: Nextcloud-PR-3425-a3d12a616b589571e5189a97d061c8039484c87b-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

allexzander requested review from mgallien, camilasan and FlexW June 10, 2021 13:46

mgallien added the 2. to review label Jun 11, 2021

mgallien reviewed Jun 14, 2021

View reviewed changes

src/libsync/discovery.cpp Outdated Show resolved Hide resolved

src/gui/tray/ActivityData.h Show resolved Hide resolved

src/libsync/discovery.cpp Show resolved Hide resolved

src/gui/tray/UserModel.h Show resolved Hide resolved

allexzander force-pushed the bugfix/vfs-wipe-moved-folder-when-conflict branch from b0afaf0 to 3896ce8 Compare June 14, 2021 14:32

mgallien reviewed Jun 14, 2021

View reviewed changes

src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved

src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved

src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved

allexzander requested a review from mgallien June 14, 2021 16:00

mgallien approved these changes Jun 15, 2021

View reviewed changes

allexzander force-pushed the bugfix/vfs-wipe-moved-folder-when-conflict branch from d62c088 to 6450549 Compare June 15, 2021 10:18

Wipe empty folder when moving a VFS placeholder. Keep a folder if the…

6312f6d

…re are hydrated items inside. Signed-off-by: allexzander <blackslayer4@gmail.com>

allexzander changed the title ~~Bugfix/vfs wipe moved folder when conflict~~ VFS wipe moved folder when conflict Jun 15, 2021

allexzander force-pushed the bugfix/vfs-wipe-moved-folder-when-conflict branch from 6450549 to cf2560d Compare June 15, 2021 10:20

FlexW reviewed Jun 15, 2021

View reviewed changes

FlexW approved these changes Jun 15, 2021

View reviewed changes

Add error message to GUI.

a3d12a6

Signed-off-by: allexzander <blackslayer4@gmail.com>

allexzander force-pushed the bugfix/vfs-wipe-moved-folder-when-conflict branch from d7fd6d3 to a3d12a6 Compare June 15, 2021 11:34

allexzander merged commit ce54a98 into master Jun 15, 2021

allexzander deleted the bugfix/vfs-wipe-moved-folder-when-conflict branch June 15, 2021 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VFS wipe moved folder when conflict #3425

VFS wipe moved folder when conflict #3425

allexzander commented Jun 10, 2021

mgallien commented Jun 11, 2021

allexzander commented Jun 11, 2021 •

edited

Loading

mgallien commented Jun 14, 2021

allexzander commented Jun 14, 2021

mgallien commented Jun 14, 2021

allexzander commented Jun 14, 2021 •

edited

Loading

mgallien commented Jun 14, 2021

allexzander commented Jun 14, 2021

mgallien left a comment

FlexW Jun 15, 2021

FlexW Jun 15, 2021

nextcloud-desktop-bot commented Jun 15, 2021

VFS wipe moved folder when conflict #3425

VFS wipe moved folder when conflict #3425

Conversation

allexzander commented Jun 10, 2021

mgallien commented Jun 11, 2021

allexzander commented Jun 11, 2021 • edited Loading

mgallien commented Jun 14, 2021

allexzander commented Jun 14, 2021

mgallien commented Jun 14, 2021

allexzander commented Jun 14, 2021 • edited Loading

mgallien commented Jun 14, 2021

allexzander commented Jun 14, 2021

mgallien left a comment

Choose a reason for hiding this comment

FlexW Jun 15, 2021

Choose a reason for hiding this comment

FlexW Jun 15, 2021

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Jun 15, 2021

allexzander commented Jun 11, 2021 •

edited

Loading

allexzander commented Jun 14, 2021 •

edited

Loading