Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#3255 closed defect (fixed)

[PATCH] Prevent replay overwrites by using date and sequential ID

Reported by: elexis Owned by: elexis
Priority: Must Have Milestone: Alpha 20
Component: Core engine Keywords: patch
Cc: Patch:

Description (last modified by elexis)

Currently the commands.txt files (and ooslog output) are created in a subdirectory of the sim-log directory.

The commands.txt file contains all commands sent by players and is therefore the key file when filing bug reports and replaying previous games visually with #9.

The problem I want to address here is that the subdirectory that contains the commands.txt file has a bad name. It uses the process ID and a sequential number. There are three problems with that:

  1. Old files will be overwritten.
  2. It is not easy to identify the correct directory when filing bugreports.
  3. The visual replay function #9 will be accessible from the main menu at some point. It should then display the correct date and time without relying on a fragile timestamp.

Deeper analysis:

1. Why files will be overwritten

Quoting from: http://en.wikipedia.org/wiki/Process_identifier#Unix-like

Process IDs are usually allocated on a sequential basis, beginning at 0 and rising to a maximum value which varies from system to system. Once this limit is reached, allocation restarts at 300 and again increases.

Therefore pIDs are not random but have a very limited keyspace (4 to 5 figures), which makes it very possible that old files will be overwritten. The sequential number that will be added to the PID will only be added if you play multiple games with the same instance of 0ad. If you reboot and get the same PID, then you overwrite the commands.txt file.

2. Identifying the correct directory

When people want to file a bugreport and the error appeared on the last game they played, then they only have to identify the file with the most recent filedate. Some have already problems with that (Mac users in particular, since that information isn't displayed by default). If the match was some time ago, the timestamp definitely helps identifying the correct file.


Open question: Should we make a subdirectory for the revision, so that the directory won't contain replay logs of different revisions? I.e. .../sim-log/alpha18/2015-05-20_03-38-38/commands.txt? No - that info should be saved to the commands.txt file.

Attachments (7)

t3255_use_datetime_in_simlog_directory.patch (1.8 KB ) - added by elexis 5 years ago.
t3255_use_datetime_in_simlog_directory_v2.patch (2.0 KB ) - added by elexis 5 years ago.
Now uses sim-log/YYYY-MM-DD_id/commands.txt. Additional info like timestamp should be saved into the commands.txt file (see #3258).
t3255_use_timestamp_and_pid_for_oosdump_filename.patch (1.3 KB ) - added by elexis 5 years ago.
If you run two instances of 0ad, rejoin with one player and if there is an oos, the oos_dump.txt files will have different filenames instead of being overwritten. This way you can diff the two oos-dumps. Should be included in the final patch of this ticket as well. The file should probably be moved to the sim-log subdirectory that also contains the commands.txt file.
t3255_prevent_replay_overwrites_v3.patch (3.5 KB ) - added by elexis 5 years ago.
t3255_prevent_replay_overwrites_v4.patch (4.4 KB ) - added by elexis 5 years ago.
t3255_prevent_replay_overwrites_v5.patch (6.5 KB ) - added by elexis 5 years ago.
t3255_prevent_replay_overwrites_v6.patch (8.4 KB ) - added by elexis 5 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 by trompetin17, 5 years ago

if you have 3 or more 0ad open in your machine?

in reply to:  1 comment:2 by elexis, 5 years ago

Replying to trompetin17:

if you have 3 or more 0ad open in your machine?

I think you mean point (1). It can happen that the commands.txt files are overwritten after rebooting when 0ad gets the same PID as some time earlier. The more files are in that directory, the more probable it becomes, since the PID only has 4 to 5 letters.

If the directory is empty, then the chance is 1/99999 = 0,001%. But if you test a lot then you have many files, for example 5000 on my machine. The probability that files will be overwritten is then 5000/99999 = 5% already. This means every 20th file will be overwritten.

It should be fixed for the other two reasons as well.

comment:3 by leper, 5 years ago

Keywords: review removed

Broken in case of multiple instances, incomplete header changes, see discussion on irc.

by elexis, 5 years ago

Now uses sim-log/YYYY-MM-DD_id/commands.txt. Additional info like timestamp should be saved into the commands.txt file (see #3258).

comment:4 by elexis, 5 years ago

Priority: Should HaveNice to Have
Summary: [PATCH] Use date + time in sim-log directory instead of pid[PATCH] Use date in commands.txt directory

comment:5 by elexis, 5 years ago

Description: modified (diff)

comment:6 by elexis, 5 years ago

TODO: CSimulation2Impl::DumpState in Simulation2.cpp also uses the old path for saving the oos_dump (-ooslog option).

TODO: CSimulation2Impl::ReportSerializationFailure in Simulation2.cpp should not use psLogDir() / "oos_log" but that directory above to save the oos_log, so that the file won't be overwritten with each oos error. This would allow one to create two oos_log files with two 0ad instances and diff them.

by elexis, 5 years ago

If you run two instances of 0ad, rejoin with one player and if there is an oos, the oos_dump.txt files will have different filenames instead of being overwritten. This way you can diff the two oos-dumps. Should be included in the final patch of this ticket as well. The file should probably be moved to the sim-log subdirectory that also contains the commands.txt file.

comment:7 by elexis, 5 years ago

Priority: Nice to HaveShould Have

another TODO: the constructor of CReplayLogger creates an empty commands.txt and the parent directory as soon as one enters the gamesetup. If one leaves the gamesetup, the empty commands.txt file remains. Since the file is useless and replays are being listed / processed in #3258, the commands.txt file should be created only if the game actually starts. To accomplish that move the contents of the CReplayLogger constructor to the StartGame function.

comment:8 by stanislas69, 5 years ago

Any updates ?

comment:9 by elexis, 5 years ago

Will upload an updated patch soon.

  • The TODOs in comment 6 will be addressed by just saving both oosdumps and the serializationtest-oosdumps to psLogDir() / "oos_log".
  • #3339 replaces the patch between comment 6 and 7
  • #3408 replaces the TODO in comment 7

comment:10 by Itms, 5 years ago

Milestone: Alpha 19Alpha 20

comment:11 by elexis, 5 years ago

Keywords: review added
Milestone: Alpha 20Alpha 19
Priority: Should HaveMust Have
Summary: [PATCH] Use date in commands.txt directory[PATCH] Prevent replay overwrites by using date and sequential ID

Since now everyone has the ability to replay games, it would be quite sad if we let the game overwrite some replays every now and then.

(and yes, the patch works too if you start two games on one computer at the same millisecond.)

comment:12 by stanislas69, 5 years ago

Owner: set to elexis

comment:13 by Itms, 5 years ago

Milestone: Alpha 19Alpha 20

No, this is not urgent (it is easy to sort files by modification date, and overwriting won't happen frequently) and it is a change too important to get in after FF. So this stays in A20.

Actually, nothing at all should be brought back to A19 now. If they were pushed at some point there is a good reason.

comment:14 by elexis, 5 years ago

The ticket was pushed to a20 because we didn't have a reviewable patch.

comment:15 by elexis, 5 years ago

Milestone: Alpha 20Alpha 19

The new patch addresses 3 issues:

  1. prevents accidental overwriting of replays
  2. uses replays_v0.0.19/2015-11-02_007 instead of sim_log/4863
    • no more useless disk I/O (initially 1min+ loading time for outdated replays, #3433)
    • user-friendly file-management
  3. defines the replay directory only in one place of the code

Notice the newly introduced replay root directory will be created in InitVfs() of GameSetup.cpp.

After commiting we need to replace sim_log with replays_v0.0.19 in wiki:ReportingErrors, wiki:Debugging, wiki:EngineProfiling, stating that the last part is the version number of 0 A.D.


Replying to Itms:

overwriting won't happen frequently

Every replay one wants to keep needs to be renamed immediately after the match.

Replying to Itms:

not urgent

  • The addressed problems in 1. and 2. are not only persistent but aggregating with every match
  • Maybe one of the heroes of the 24 hotfixes in the last 2 days (r17147-r17171) finds time to commit this (Itms counts as a hero too)
  • Would be nice to not push this before commit freeze otherwise

comment:16 by wraitii, 5 years ago

My comments:

I think this can be committed as is, but I really dislike the for loop on a conceptual basis. I think it'd be much better to just get all files in the directory, get the highest existing number, and add 1.

comment:17 by leper, 5 years ago

Milestone: Alpha 19Alpha 20

See comments on irc.

comment:18 by wraitii, 5 years ago

In 17177:

Prevent accidental overwriting of replays in very rare cases while we wait for a better solution.

Refs #3255

comment:19 by elexis, 5 years ago

Updated patch:

  • Moves replays to user documents (where also screenshots and savegames reside)
  • Creates one directory per engine version to avoid mixing replay versions
  • Supports alphanumerical sorting for 10k replays per day
  • Prevents overwrites by using more than 4 digts for the index if needed
  • Shows the directory of new replays to stdout

comment:20 by elexis, 5 years ago

  • remove the pid also from the serializationtest and ooslog directory path, since that is not really useful
  • move the directory-index code to a shared function and use it for serializationtest and ooslog, to ease access for developers

comment:21 by elexis, 5 years ago

Resolution: fixed
Status: newclosed

In 17761:

Use date and sequential ID for replay-directorynames, fixes #3255.
Save replays in userdata (screenshots, savegames) and create one subdirectory for every release.

comment:22 by elexis, 5 years ago

Keywords: review removed

comment:23 by elexis, 5 years ago

In 17776:

Fix a race-condition when two games have been started simultaneously and attempt to create the same replay directory, refs #3255.
Instead of triggering a debug-breakpoint, print a warning to stdout and succeed in the N'th retry when having started N processes simultaneously.
Previously the problem had been addressed by using the processID in the directory name.

Note: See TracTickets for help on using tickets.