[BUG][PLAYER][DATA LOSS] Replays are lost during Mass Replay Check when renaming

[WillLem]

Found a particularly nasty bug today when running tests on the SLX Replay Manager (which is essentially a re-skinned version of the existing NL Replay Renamer with a couple of extra features), and discovered that it exists in the engine as far back as NL 12.12.5.

To say the MRC deletes the files is somewhat incorrect. It does delete them, but this behaviour is actually correct. The bug is that the newly-generated replays are being saved over each other iff they have the same filename.

Steps to reproduce:

1) Unzip the attached Replay Check Batch to a new folder in "levels"
2) Run MRC on the replays within the folder, making sure to choose "move to" or "copy to" (choose Position + Timestamp to make it more likely that the buggy behaviour will occur, but it can occur for any renaming string)
3) Some of the replays won't have been saved (well, actually, they have been saved, but they overwrite each other because they have the same filename)

This is most likely occurring because the timestamp used for renaming is that of the replay check time rather than the timestamp on the original replay.

Suggestions for fixes:

1) Add an incrementing alphabet character to the timestamp iff the current timestamp has already been used (so, for example, __2024-06-05_15-26-51.  __2024-06-05_15-26-51A,  __2024-06-05_15-26-51B, etc)

2) Retain the timestamp of the original replay and use that instead (arguably, this should be happening anyway)

3) Check to see if the current "NewName" has already been used in the current replay check. If it has, add an alphanumeric character to the end of the replay name (so "NewName := NewName + i; Inc(i); or generate a random short string of digits) - essentially the same as suggestion (1), but we're taking the full replay name into account rather than just the timestamp

I'll likely go ahead and apply whichever of these fixes proves to be easiest in SLX, happy to do a PR if it tests working.

[WillLem]

Fixed in SLX commit beccdf927 and sent to NL via this pull request.

The fix generates a random 3-digit hex in the event of identical filenames when saving. For now, it's good enough to prevent data loss whilst we figure out whether we'd prefer to have replay files saved with the original timestamp tag (and, in the case of SLX, whilst I figure out how to do that! )

[Simon]

Swap lines 309 and 310, and you'll be able to remove the duplicated line 306.

Theoretically, your loop can be infinite. In practice, I doubt anybody will run into problems because nobody has found the original (serious) data loss behavior in years. It's up to you how much work you want to invest here.

It's good that the code (both yours and the original) first saves the replay, only then removes the replay. Reason: If the computer hangs in between the two, you haven't lost data.

It's surprisingly hard to make this 100 % robust. Here are some concerns and you probably don't want to fix them all these days. They're rabbit holes (compared to your fix candidate) and running into these bugs will be rare.

• Avoid the infinite loop.
  
• You can't expect to find a free filename for certain, regardless of how you generate the filenames. Keep the source replay file (instead of deleting it) after you couldn't find a free filename for renaming.
  
• Saving the replay can fail: Disk is full, you're renaming to a write-only media, you're renaming to a removed physical drive, you're renaming to a network drive and the connection dies, ... Keep the source replay file if the saving fails.
  
• You have a time-to-check-to-time-of-use bug: You test in NL that filename X is free, then another
  program creates file X, then NL saves the file to X, overwriting the other program's file. Or NL fails to save because the file is still open in the other program.
  

The time-to-check-to-time-of-use bug is unlikely (NL is not a security attack target) and I'm noting this mainly for completeness. I don't even know a good way to fix time-to-check-to-time-of-use in Delphi. In C, there is fopen("name.txt", "wx"), where the "wx" means that you want the file-opening call to succeed iff the file is nonexistant and writeable. The idea is that you both test and open in a single library call that relies on the operating system to ensure that nobody can do anything to the file in between.

-- Simon

[WillLem]

Swap lines 309 and 310, and you'll be able to remove the duplicated line 306
...
Avoid the infinite loop.

Good shout!

Applied these fixes and updated the PR.

You can't expect to find a free filename for certain, regardless of how you generate the filenames. Keep the source replay file (instead of deleting it) after you couldn't find a free filename for renaming.

Not so easy to see how to do this. If we exit the loop, does this also exit the main procedure? If so, then we can use the existing above fix, and exit if naming attempts have maxed out unsuccessfully (currently extremely unlikely). If not, I guess we could set a bool (tracked per-item) and then check & exit before deleting.

Keep the source replay file if the saving fails.

Another good shout, but again struggling to see how to do this one.

You have a time-to-check-to-time-of-use bug: You test in NL that filename X is free, then another
program creates file X, then NL saves the file to X, overwriting the other program's file

A) This is definitely pushing the effort I want to put into this, but also B) it shouldn't overwrite, because it will see the existing file and generate a new name, will it not? Besides, given normal circumstances this should only happen if performing an MRC on the same folder in 2 NL instances simultaneously... such use of the program needn't be supported anyway.

[Simon]

NewName := ChangeFileExt(NewName, '') + '_' + UniqueTag + '.nxrp';

Assuming we loop 4 times out of the maximum 1,000, this generates names such as:
blah_123_456_789_0AB.nxrp

Do you want this? Appending more and more random chars indeed has a higher chance of finding a free filename.

Probably, we'll stop before we run into Windows's limit of 255 wchars per filename without path. In particular, to run into OP's data loss and thereby into the loop here, you have to give a short prefix that didn't depend on replay names/level names/...

it shouldn't overwrite, because it will see the existing file and generate a new name, will it not?

Not if the OS takes processor time away from NL exactly after you've tested that X doesn't exist, exactly before you save to X.

I agree, skip it for this PR. The old code didn't even test for existence at all, you haven't made anything worse.

-- Simon

[WillLem]

Assuming we loop 4 times out of the maximum 1,000, this generates names such as:
blah_123_456_789_0AB.nxrp

Do you want this? Appending more and more random chars indeed has a higher chance of finding a free filename.

Circumstances that would lead to this happening:

We have > 1 replay file to test. All files are tested at the same timestamp, so result in the following name:

blah_timestamp
blah_timestamp
blah_timestamp
etc...

The loop sees that the filename blah_timestamp already exists, so saves the next file as blah_timestamp_123. Then, it generates the exact same tag for the next file (a 0.02% chance, probably reduced to even less when accounting for the files being saved during the same 1s time period), so needs to save it as 123_456.

In practice, this would basically never happen. Here's the results of a test I did just now. Note that those replays which have the same timestamp have a single 3-digit hex tag:



I mean, I'm happy to update it again if you think it's worth it. But, I'd say it's fixed.

[Simon]

Right, I wouldn't worry much about running out of allowed filename length. As you correctly describe, the chance of having even 2 or 3 extra loop iterations is tiny.

It's a good PR now that fixes the original data loss in the vast majority of cases.

-- Simon

[WillLem]

It's a good PR now that fixes the original data loss in the vast majority of cases.

Thanks for checking, you definitely picked up on some necessary improvements

[namida]

This definitely falls under worth fixing, and should be easy enough to achieve.

[namida]

Fixed in commit 996b2e1.

NL will now only overwrite the replay file if the file name generated for the output is exactly the same as the input filename (as in this case, it can be presumed the user is intentionally overwriting their existing ones). Otherwise, if the file already exists, a number will be appended to the name (before the extension). It starts with adding -0001, -0002 etc. In the extremely unlikely case 9999 is reached, it just adds on an extra digit (ie: the next one will have -10000 appended to the name).

Side effects may include an infinite hang if you have roughly 4 billion files with the same output name. I consider this a very unlikely scenario in practice.