"Quit on Close Content" + "Auto Save State" writes state twice (causing duplicate files, corrupt thumbnails)

[bcat]

Description

I've noticed that when the "Quit on Close Content" option is used together with "Auto Save State", RetroArch writes the save state twice when "Close Content" is selected, rather than once when "Quit RetroArch" is used.

Additionally, the second write appears to happen too late (and/or race with other parts of the exit flow), which leads to buggy interactions with various other options.

• When the "Save State Thumbnails" option is also enabled, the thumbnail seems to be written twice as well, but the second thumbnail ends up corrupt or empty. (Maybe the game's framebuffer is already gone by that point?)
• When a core override exists (even if it's just an empty core.cfg file), the core override is unloaded before the second auto save is written, causing the second auto save to go to the wrong directory (e.g., ignoring options like "Sort Save States into Folders by Core Name" and "Sort Save States into Folders by Content Directory").

These issues appear to be side effects of the auto save being performed twice rather than issues with those additional options themselves.

Expected behavior

When "Quit on Close Content" is enabled, "Quit RetroArch" and "Close Content" should behave identically. This includes writing the auto save state (and thumbnail, if enabled) exactly once.

Actual behavior

"Quit RetroArch" writes the auto save state once, as described above; however, "Close Content" writes it twice, leading to the issues described above.

Steps to reproduce the bug

1. Create a new RetroArch install with default settings.
2. Configure RetroArch settings "Quit on Close Content: ON" and "Auto Save State: ON".
3. Optionally set "Save State Thumbnails: ON" to see the buggy interaction with thumbnail saving.
4. Optionally create a core override file (core.cfg) for the core you want to test to see how this causes an incorrect path to be used by the duplicate auto save. (The file can be empty.)
5. Load a game.
6. Quit RetroArch by selecting "Close Content" in the quick menu.

The issues described here seems to be independent of which core is used. At least I was able to repro the same issue with Gambette, mGBA, Mesen, and Snes9x.

I've attached three log files:

1. quit.log: Correct behavior when exiting with "Quit". Auto save state is written only once, in the correct location. Thumbnail is also written correctly.
2. close_content.log: Incorrect behavior when exiting with "Close Content". Auto save state is written twice. Both writes go to the correct location, but thumbnail is empty due to second write.
3. close_content_with_core_override.log: Incorrect behavior when exiting with "Close Content" when a core override is in use. Auto save state is written twice. Second write goes to incorrect location (:/saves rather than :/saves/core), and thumbnail is still empty.

Here is the relevant portions of the log file for the correct behavior (quit.log):

[INFO] [State]: Saving state "C:\Users\bcat\Documents\Temp\RetroArch-Win64\states\Gambatte\144p Test Suite (0.23).state.auto", 51464 bytes.
[INFO] Auto save state to "C:\Users\bcat\Documents\Temp\RetroArch-Win64\states\Gambatte\144p Test Suite (0.23).state.auto" succeeded.
[INFO] [Core]: Content ran for a total of: 00 hours, 00 minutes, 04 seconds.
[INFO] [Runtime]: Saving runtime log file: "C:\Users\bcat\Documents\Temp\RetroArch-Win64\playlists\logs\Gambatte\144p Test Suite (0.23).lrtl".
[INFO] [Core]: Unloading game..
[DEBUG] [Audio]: Stopped audio driver "xaudio"
[INFO] [Core]: Unloading core..
[INFO] [Core]: Unloading core symbols..
[INFO] [Core]: Saved core options file to "C:\Users\bcat\Documents\Temp\RetroArch-Win64\config\Gambatte\Gambatte.opt".

Now compare that to the worst case behavior (close_content_with_core_override.log):

[INFO] [Core]: Content ran for a total of: 00 hours, 00 minutes, 04 seconds.
[INFO] [Runtime]: Saving runtime log file: "C:\Users\bcat\Documents\Temp\RetroArch-Win64\playlists\logs\Gambatte\144p Test Suite (0.23).lrtl".
[INFO] [State]: Saving state "C:\Users\bcat\Documents\Temp\RetroArch-Win64\states\Gambatte\144p Test Suite (0.23).state.auto", 51464 bytes.
[INFO] Auto save state to "C:\Users\bcat\Documents\Temp\RetroArch-Win64\states\Gambatte\144p Test Suite (0.23).state.auto" succeeded.
[INFO] [Config]: Loading config: "C:\Users\bcat\Documents\Temp\RetroArch-Win64\retroarch.cfg".
[INFO] [Overrides]: Configuration overrides unloaded, original configuration restored.
[INFO] [Config]: Saved new config to "C:\Users\bcat\Documents\Temp\RetroArch-Win64\retroarch.cfg".
[INFO] [State]: Saving state "C:\Users\bcat\Documents\Temp\RetroArch-Win64\states\144p Test Suite (0.23).state.auto", 51464 bytes.
[INFO] Auto save state to "C:\Users\bcat\Documents\Temp\RetroArch-Win64\states\144p Test Suite (0.23).state.auto" succeeded.
[INFO] [Core]: No content, starting dummy core.
[INFO] [Core]: Content ran for a total of: 00 hours, 00 minutes, 00 seconds.
[INFO] [Core]: Unloading game..
[DEBUG] [Audio]: Stopped audio driver "xaudio"
[INFO] [Core]: Unloading core..
[INFO] [Core]: Unloading core symbols..
[INFO] [Core]: Saved core options file to "C:\Users\bcat\Documents\Temp\RetroArch-Win64\config\Gambatte\Gambatte.opt".

In both cases, an auto save is first written to :/states/Gambatte/144p Test Suite (0.23).state.auto as expected. But when exiting using "Close Content", a second auto save is written to :/states/Gambatte/144p Test Suite (0.23).state.auto. The wrong path appears to be a result of the incorrect second save happening after the core override is unloaded.

Bisect Results

I'm not aware of this being a regression, so I didn't try and bisect yet. Can do so later if needed.

Version/Commit

• RetroArch: 1.16.0 (041ae30)

Environment information

• OS: Windows 10 Version 22H2 (OS Build 19045.3570)
• Compiler: MinGW (10.2.0) 64-bit

[bcat]

I think I understand part of the problem. It looks like this is how quitting normally vs. via "Close Content" differ:

• "Quit RetroArch": CMD_EVENT_QUIT => retroarch_main_quit (calls command_event_save_auto_state)
• "Close Content" (with "Quit on Close Content" enabled): CMD_EVENT_CLOSE_CONTENT => CMD_EVENT_UNLOAD_CORE (calls command_event_save_auto_state) => CMD_EVENT_QUIT => retroarch_main_quit (also calls command_event_save_auto_state)

So that explains where the second auto save is coming from (and, more generally, why some cleanup tasks seem to be happen twice when using "Quit on Close Content").

But what I don't understand is why "Quit on Close Content" doesn't simply cause CMD_EVENT_CLOSE_CONTENT to fire CMD_EVENT_QUIT directly. It seems to me that would resolve this issue and match the name of the option better (by simply making "Close Content" act the same as "Quit RetroArch"). Maybe there's something I'm missing, though....

[bcat]

A partial answer... it seems that despite the name, CMD_EVENT_CLOSE_CONTENT isn't fired when selecting "Close Content" in the menu. It appears to only be used for the "Close Content" hotkey instead.

Making CMD_EVENT_CLOSE_CONTENT fire CMD_EVENT_QUIT resolves the issue when closing content via hotkey, but further breaks closing content via quick menu.

[bcat]

I wonder if #16014 incidentally fixed this. Will update to 1.17.0 and retest soon.

[bcat]

Okay, I (finally) retested this in 1.19.1, and unfortunately the bug still exists. With "Quit on Close Content" enabled, the autosave state is still written twice on exit: once when the core is unloaded and again when the actual quit happens. Additionally, the second autosave writes to the wrong location when core overrides change the state directory path (since the second write happens after the core override resets the core-specific to the global one).

But I do still think #16014 might have made this easier to fix. Specifically, now that Quit on Close is handled uniformly for both menu items and hotkeys, I wonder if we can just do the following:

1. Update check_quit_on_close to return a boolean stating whether or not it ended up firing CMD_EVENT_QUIT.
2. Make sure the two existing Close Content call sites call check_quit_on_close before firing CMD_EVENT_UNLOAD_CORE.
3. If check_quit_on_close returns true, skip firing CMD_EVENT_UNLOAD_CORE entirely (the important bit).

It appears to be fine to quit without unloading the core, because that's the same thing the quit hotkey does. And this avoids the problematic double autosave write.

@davidgfnet, do you have thoughts on this since you changed the Quit on Close impl pretty recently? If the changes I suggested seem sane to you, I could take a stab at a PR, but I wanted to see if you saw anything silly I was missing....

[hizzlekizzle]

I think @sonninnos (?) was looking at this awhile back but didn't find any easy solution. Your bool flag idea sounds pretty reasonable to me, at least, and is worth checking out and testing, I'd say.