Skip to content

fix: try to correct mtime on upsyncs - #8095

Merged
mgallien merged 2 commits into
masterfrom
bugfix/reset-mtime
Apr 8, 2025
Merged

fix: try to correct mtime on upsyncs#8095
mgallien merged 2 commits into
masterfrom
bugfix/reset-mtime

Conversation

@nilsding

@nilsding nilsding commented Mar 31, 2025

Copy link
Copy Markdown
Member

Files with a modification time of less than 0 do usually not make sense (and afaik the server doesn't accept them either).
--> attempt to update the modification time to Time.now while propagating

side note: I ran into this because KArchive/Ark(?) didn't consider the extra time attributes on entries for a certain zip file, so it instead used the standard time value of each zip entry which was set to <= 1980 for files and < 1970 for directories...

@nilsding nilsding added this to the 3.17.0 milestone Mar 31, 2025
@nilsding nilsding self-assigned this Mar 31, 2025
@nilsding
nilsding force-pushed the bugfix/reset-mtime branch from 261c7fa to 89487f4 Compare March 31, 2025 12:49

@mgallien mgallien left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comments

fileInfo = remoteRootFileInfo.create(fileName, onePartBody.size(), onePartBody.at(0).toLatin1());
}
fileInfo->lastModified = OCC::Utility::qDateTimeFromTime_t(request.rawHeader("x-oc-mtime").toLongLong());
fileInfo->lastModified = OCC::Utility::qDateTimeFromTime_t(modtime);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would differ from server behavior
modification time of the server side file should be what is in the header x-oc-mtime

@nilsding nilsding Apr 1, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really -- request.rawHeader only contains the headers of the HTTP request itself, not the multipart-part that's being uploaded (and modtime is set to the value from the multipart headers)

while cross-checking this I just saw that the header used for the modtime variable uses the x-file-mtime multipart header, which has a TODO set on the server side to be replaced with x-oc-mtime... https://github.com/nextcloud/server/blob/c029616ec05f363b28467bbdccd5ce806fd43212/apps/dav/lib/BulkUpload/BulkUploadPlugin.php#L61-L68

Comment thread test/syncenginetestutils.cpp Outdated
@nilsding
nilsding force-pushed the bugfix/reset-mtime branch from 89487f4 to a769926 Compare April 1, 2025 06:44

// the server only considers an mtime of 0-86400 (1d) as invalid, so this is fine
// see also: apps/dav/lib/Connector/Sabre/MtimeSanitizer.php
QCOMPARE(lastModified(barFileAaaSubFolder), INVALID_MTIME2);

@nilsding nilsding Apr 3, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course Windows doesn't like that change

lastModified(barFileAaaSubFolder) is -1, which is weird considering I'm running this on a 64-bit system (and Windows has been using a signed 64-bit time value internally since basically forever -- however it's 100ns intervals since Jan 1st 1601). time_t seems to be 64-bit on 64-bit Windows as well; my guess is that we need to update https://github.com/nextcloud/desktop/blob/master/src/csync/std/c_time.cpp#L45 to convert from a 64-bit time_t to a FILETIME type ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my, it's even worse than that

struct timeval, used within FileSystem::setModTime, is defined in the winsock.h header of all places and uses two longs for tv_sec and tv_usec -- and longs are 32-bit wide on Windows, even on x64 systems...

on x64 Linux a long is 64-bit wide, same as time_t and all other fields in a struct timeval

@nilsding
nilsding force-pushed the bugfix/reset-mtime branch from a769926 to e329208 Compare April 8, 2025 11:15
@nilsding
nilsding requested a review from mgallien April 8, 2025 14:10
nilsding added 2 commits April 8, 2025 23:18
Files with a modification time of less than 0 do usually not make sense
(and afaik the server doesn't accept them either).
--> attempt to update the modification time to _Time.now_ while
propagating

side note: I ran into this because KArchive/Ark(?) didn't consider the
extra time attributes on entries for a certain zip file, so it instead
used the standard time value of each zip entry which was set to <= 1980
for files and < 1970 for directories...

Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>
also removes some unused Utility methods

Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>
@mgallien
mgallien force-pushed the bugfix/reset-mtime branch from e329208 to 8cd1bee Compare April 8, 2025 21:18
@mgallien
mgallien enabled auto-merge April 8, 2025 21:18
@github-actions

github-actions Bot commented Apr 8, 2025

Copy link
Copy Markdown
Contributor

Artifact containing the AppImage: nextcloud-appimage-pr-8095.zip

SHA256 checksum: e96a6ae9006adfac918e8cbeb385573a284b9fdece957ce22a347e4288614990

To test this change/fix you can download the above artifact file, unzip it, and run it.

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

@sonarqubecloud

sonarqubecloud Bot commented Apr 8, 2025

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
68.0% Coverage on New Code (required ≥ 80%)
D Maintainability Rating on New Code (required ≥ A)
94 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@mgallien
mgallien merged commit d0ab91e into master Apr 8, 2025
@mgallien
mgallien deleted the bugfix/reset-mtime branch April 8, 2025 22:00
@nilsding nilsding linked an issue Apr 9, 2025 that may be closed by this pull request
@nilsding nilsding modified the milestones: 3.17.0, 3.16.3 Apr 9, 2025
@nilsding

nilsding commented Apr 9, 2025

Copy link
Copy Markdown
Member Author

/backport to stable-3.16

@github-actions

Copy link
Copy Markdown
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Cannot sync due to invalid modification time"

2 participants