Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cache timestamp comparisons #3974

Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Dec 22, 2023

Problem

If a user in the eastern hemisphere tries to install a version of a mod that was released in the past X hours, where X is the number of timezones east of Greenwich (so, 3 hours for UTC+3), it fails with this error:

About to upgrade:
 * Upgrade: Patch Manager 0.5.0 to 0.6.0 (spacedock.info, 247.0 KiB)
Downloading "https://spacedock.info/mod/3482/Patch%20Manager/download/0.6.0"
Finished downloading PatchManager 0.6.0, validating and storing to cache...
Trying to install PatchManager 0.6.0, but it's not downloaded or download is corrupted
Error during installation!
If the above message indicates a download error, please try again. Otherwise, please open an issue for us to investigate.
If you suspect a metadata problem: https://github.com/KSP-CKAN/NetKAN/issues/new/choose
If you suspect a bug in the client: https://github.com/KSP-CKAN/CKAN/issues/new/choose

Cause

CKAN thinks the freshly downloaded file is older than the release date, which ordinarily means it needs to re-download the file because it was replaced on the server:

21312 [4] DEBUG CKAN.NetFileCache (null) - Checking cache for https://spacedock.info/mod/3482/Patch Manager/download/0.6.0
21313 [4] DEBUG CKAN.NetFileCache (null) - Rebuilding cache index
21315 [4] DEBUG CKAN.NetFileCache (null) - Found file C:\Users\SunMachine\AppData\Local\CKAN\downloads\3AFBD5A2-PatchManager-0.6.0.zip
21316 [4] DEBUG CKAN.NetFileCache (null) - Found stale file, deleting it

CkanModule.release_date is being affected by two serious design defects in different components:

  • When Newtonsoft.Json (de-)serializes a UTC timestamp, such as CkanModule.release_date, it does not preserve the time zone as-is or convert it to UTC, as any reasonable programmer would expect. Instead, it converts to local time!! If you print it, it shows the correct time zone offset, so in theory it has all the info needed to recreate the original timestamp, except...
  • DateTime has two modes, local and UTC (it is unable to represent a timestamp in any of the ~22 other time zones), and < and > comparison operators. When it compares a local timestamp to a UTC timestamp, it does not convert them to the same time zone, even though it has all the information it would need to do so!! Instead it just compares the raw numbers as if they were already in the same time zone. This means it gives wrong answers unless the calling code uses a mechanism like .ToUniversalTime() carefully and strategically to ensure consistency.

Together, these two very surprising choices in the runtime and libraries break this simple line of code for people east of Greenwich:

if (remoteTimestamp == null
|| remoteTimestamp < File.GetLastWriteTime(file).ToUniversalTime())

remoteTimestamp is a local timestamp (despite Netkan generating and saving it in UTC originally), so it will be treated as greater than an actually-equivalent-or-slightly-less UTC timestamp returned by File.GetLastWriteTime(file).ToUniversalTime() if the local hour is greater than the UTC hour, i.e. if the time zone offset is negative, i.e. if you're in the eastern hemisphere.

The western hemisphere has the opposite, far less obvious problem. There, remote timestamps are treated as older than they really are by your distance from Greenwich, so if a mod author replaces a download, a user re-installing that mod will not get the latest changes if they first downloaded it just a few hours before the replacement.

Changes

Now we set JsonSerializerSettings.DateTimeZoneHandling = DateTimeZoneHandling.Utc to tell our JSON deserializers to convert timestamps to UTC so they can be used safely and sanely. The one in CkanModule.FromJson fixes loading freshly downloaded repo metadata, and the one in RepositoryData.FromJson handles loading cached repo metadata. The others are included just in case.

Fixes #3972.

@HebaruSan HebaruSan added the Bug label Dec 22, 2023
@HebaruSan HebaruSan added Core (ckan.dll) Issues affecting the core part of CKAN Network Issues affecting internet connections of CKAN labels Dec 22, 2023
@HebaruSan HebaruSan self-assigned this Dec 22, 2023
@HebaruSan HebaruSan merged commit 9ec1246 into KSP-CKAN:master Dec 22, 2023
8 checks passed
@HebaruSan HebaruSan deleted the fix/cache-timestamp-comparisons branch December 22, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Core (ckan.dll) Issues affecting the core part of CKAN Network Issues affecting internet connections of CKAN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Very recently updated mods fail to install with corrupted download message in eastern hemisphere
1 participant