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

Implement a "Recovery Mode" for recovering crashing projects during initialization #92563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rsubtil
Copy link
Contributor

@rsubtil rsubtil commented May 30, 2024

Implements godotengine/godot-proposals#2628

Note

This description is updated whenever any significant changes are made, and should always reflect the current state of this PR

Overview

This adds a new special "Recovery Mode" that, as described, disables some engine features that can make a project fail to launch during startup. When this mode is launched, the following features are completely disabled:

  • Tool scripts (GDScript and C#)
  • Editor plugins
  • GDExtension addons
  • Scene restoring

This is displayed to the user as soon as the project is opened in this mode, through both a popup, and a notification:

image
image

Because some of these features may be necessary for a project to work properly, this mode may put the project in an unusable/unworkable state. Since this is a temporary mode meant to allow only for some basic troubleshooting, a few other changes were done as well to reflect this:

  • All launching functionality is disabled, with the run bar modified to reflect it
    image
  • The Asset Library and Game View plugins are disabled, since they can't really work during this mode
    image
  • Tool scripts are displayed differently to clarify that they're not being run during this mode
    image
  • The plugin configuration panel has a warning to clarify plugins do not run during this mode
    image

Usage

Automatic detection

When launching a project, Godot creates a lock file at user://.recovery_mode_lock. This file persists during the engine initialization, and is removed when the editor starts scanning the project files, after one second.

If any crash/hang happens during initialization, this file is not removed. So, if this file exists the next time Godot attempts to open the project, then the engine knows the project failed to open last time, thus prompting the user to open it in this mode.

CLI

This mode can be manually triggered by opening a project with the new --recovery-mode flag:

$ godot -e -path <...> --recovery-mode
$ godot --help

Godot Engine v4.4.dev.mono.custom_build.f7a0552d5 (2024-12-06 14:59:43 UTC) - https://godotengine.org
...
Run options:
...
  --recovery-mode                   E  Start the editor in recovery mode, which disables features that can typically cause startup crashes, such as tool scripts, editor plugins, GDExtension addons, and others.
...

UI

When a project is opened, the project manager searches for any existing lock file; if that's the case, it opens a popup to suggest using this mode, explaining what it does and its intended usage.

image

It is also possible to use this mode manually from the new options button next to the Edit button:

image

Test projects

To test this functionality, I've made a test suite of projects that crash the engine on initialization with typical user scenarios:

  • GDScript tool script: - Project with a bad tool script, which is attached to a node on the main scene.
  • C# tool script: - Same as above, but with a C# script, which requires rebuilding the project when the script is changed.
  • Editor plugin: - Project with a bad editor plugin, which internally has bad GDScript code.
  • GDExtension addon: - Project with an external error in the GDExtension addon.

All of these test cases fail to open on a normal Godot session and require manual intervention to be recovered. But under recovery mode, every test must open successfully.

GDScript tool script C# tool script Editor plugin GDExtension addon
Crash tool_gdscript_crash.zip tool_csharp_crash.zip plugin_crash.zip gdextension_crash.zip
Hang tool_gdscript_hang.zip tool_csharp_hang.zip plugin_hang.zip gdextension_hang.zip
  • Crash: This project has some bad code that immediately crashes the engine when opened.
  • Hang: This project has some bad code that loops infinitely, which hangs the engine and leaves it frozen until forcibly closed.

Future work

This is a basic implementation so far, and the usefulness of this mode should be improved with other additional features:

  • An automatic mechanism to detect failing scenarios and automatically prompt users to enable this mode, as described in Add a way to disable all plugins when opening a project (safe mode) godot-proposals#2628 (edit: implemented in this PR too)
  • Being able to open/edit scenes when some dependencies are missing, since these can originate from temporarily disabled editor/GDExtension plugins.
  • Adding other problematic features for recovery mode that I didn't cover (e.g. importing assets)

@YeldhamDev
Copy link
Member

Is the yellow border for the safe mode dialog necessary? It won't even be seen by most since windows aren't embedded by default in the editor.

@rsubtil
Copy link
Contributor Author

rsubtil commented May 30, 2024

Is the yellow border for the safe mode dialog necessary? It won't even be seen by most since windows aren't embedded by default in the editor.

Good point, I don't use the default setting of separated windows 😅

It was more of an experiment, but I believe the yellow banner at the run bar is already noticeable enough 👍

@rsubtil rsubtil force-pushed the feature-oopsie_woopsie branch from 7311717 to 88f7e17 Compare May 30, 2024 20:08
editor/gui/editor_run_bar.cpp Outdated Show resolved Hide resolved
editor/gui/editor_run_bar.cpp Outdated Show resolved Hide resolved
editor/gui/editor_run_bar.cpp Outdated Show resolved Hide resolved
@Calinou
Copy link
Member

Calinou commented Jun 1, 2024

Note that we could automatically prompt the user to edit a project in safe mode after an editor (not project) crash. To do so, we need to write a file in the project's .godot/ folder when the editor crashes. This can be accomplished in the crash handler when Engine::get_singleton()->is_editor_hint() is true.

When opening any project, the editor checks for the presence of this file. If the file is present, a dialog is spawned to prompt the user to open the project in safe mode. Regardless of the user's answer, the file is removed afterwards when the editor is done initializing and rescanning things.

When opening a project from the command line, the project opens as usual, but a warning message is printed to warn the user that they can use --safe-mode if desired (this is done to avoid breaking automation). The file is then removed after the editor is done initializing and rescanning things.

After doing this, we probably won't need to have the Edit (Safe Mode) button anymore in the project manager (which I feel is too prominent right now). We could make it manually accessible by shift-clicking, but I think the automatic prompt will suit the 90% use case.

A similar approach could be used to write a "lock file" in .godot/ to warn the user when opening the Godot editor multiple times at once on the same project. (This is sometimes desired, but it comes with some edge cases so it probably makes sense to warn about it.)

@rsubtil rsubtil force-pushed the feature-oopsie_woopsie branch 2 times, most recently from ea34c06 to 5e79eab Compare June 1, 2024 11:29
@rsubtil
Copy link
Contributor Author

rsubtil commented Jun 1, 2024

@Calinou yes, I think this an important mechanism to complement this feature. I hadn't included it yet because, actually, I wanted to confirm this detail that you've mentioned, before proceeding:

This can be accomplished in the crash handler when Engine::get_singleton()->is_editor_hint() is true.

Do we really want this mode to be triggered on any editor crash? I think it would generate more false positives (e.g. internal engine crashes) and generate user confusion because this mode would implicitly be triggered for all types of crashes, when it can only help in troubleshooting a subset of them.

I'm more concerned in ensuring we avoid false positives, and limiting this mode to only trigger on initialization crashes helps to reduce that. The usefulness of this mode, in my opinion, is tied to situations where a user lost the ability to edit the project at all and requires some form of external intervention to recover them.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 1, 2024

Maybe we could create some "init" file when starting editor and delete it once the editor loads. If it doesn't load it means it crashed.
Though I suppose there are some edge cases like exporting from CLI etc. so not sure if it can be done reliably.

editor/editor_node.h Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
@rsubtil rsubtil force-pushed the feature-oopsie_woopsie branch from 5e79eab to c292649 Compare June 6, 2024 17:27
@rsubtil
Copy link
Contributor Author

rsubtil commented Jun 7, 2024

@KoBeWi should I add the auto-detect crash functionality we discussed already in this PR, or do it in another PR after this one is merged?

I think @Calinou brings a valid point in doing it right now; if this functionality is implemented, there isn't much need to add the new "Edit in Safe Mode" button to the project manager.

I don't expect this functionality to take much effort to implement, but considering my current availability, it might still take around one week to implement.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 7, 2024

Well, this is not going to be merged before 4.4, so you have some time to improve it.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2024

You renamed _open_selected_projects_with_migration method

@rsubtil
Copy link
Contributor Author

rsubtil commented Sep 26, 2024

You renamed _open_selected_projects_with_migration method

Yes, that was intended; I renamed _open_selected_project_ask to _open_selected_project_check_warnings to not only make it's functionality clearer, but also because there's now multiple possible steps when opening a project (check warnings, check safe mode).

Since you're doing something similar, I saw fit to also rename it to _open_selected_project_check_migration.

editor/project_manager.h Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2024

Since you're doing something similar, I saw fit to also rename it to _open_selected_project_check_migration.

But the function performs the migration. The checks are technically done before the info dialog appears.

@rsubtil rsubtil force-pushed the feature-oopsie_woopsie branch from 8990cbd to 8faa6f2 Compare September 27, 2024 17:17
@rsubtil
Copy link
Contributor Author

rsubtil commented Sep 27, 2024

But the function performs the migration. The checks are technically done before the info dialog appears.

Ok yeah, you're right, I've reverted this name change.

@rsubtil rsubtil force-pushed the feature-oopsie_woopsie branch 2 times, most recently from 0111dd7 to 015a1e7 Compare October 3, 2024 18:27
@akien-mga akien-mga force-pushed the feature-oopsie_woopsie branch from 015a1e7 to 47721c4 Compare November 13, 2024 10:20
@akien-mga akien-mga requested a review from a team as a code owner November 13, 2024 10:20
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

editor/project_manager.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga force-pushed the feature-oopsie_woopsie branch from 47721c4 to 23ecaab Compare November 13, 2024 10:41
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Nov 13, 2024
@akien-mga
Copy link
Member

I rebased to solve merge conflicts. I'll try to find a bit of time to finally test the feature and approve.
Also CC @bruvzg if you want to do another review pass.

return Item(project_name, description, project_version, tags, p_path, icon, main_scene, unsupported_features, last_edited, p_favorite, grayed, missing, config_version);
// We can't use globalize_path because there is no loaded project. So we replicate the behavior of accessing user://
if (!cf_project_name.is_empty()) {
String safe_mode_lock_file = OS::get_singleton()->get_user_data_dir(cf_project_name).path_join(".safe_mode_lock");
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it do get_safe_dir_name on a name, like it's done in the actualt project, also seems like it won't handle application/config/use_custom_user_dir/application/config/custom_user_dir_name override (get_user_data_dir is reading it from currently loaded config, but in this case it should be selected project config).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't it do get_safe_dir_name on a name, like it's done in the actualt project,

Yup, I missed that. I also don't remember why I used the raw cf_project_name variable instead of the unescaped project_name, so I'll revisit this.

also seems like it won't handle application/config/use_custom_user_dir/application/config/custom_user_dir_name override

This is indeed a problem, because this query is inside the get_user_dir. I already had to move the query for application/config/name outside to be able to set it from the context of the project manager. My best idea so far is to extract all project specific logic from this function, and move this logic to the generic os.cpp::get_user_data_dir() function, with the OS-specific overrides now becoming quite simple:

// os.cpp
String OS::get_user_data_dir() const {
	String appname = get_safe_dir_name(GLOBAL_GET("application/config/name"));
	if (!appname.is_empty()) {
		bool use_custom_dir = GLOBAL_GET("application/config/use_custom_user_dir");
		if (use_custom_dir) {
			String custom_dir = get_safe_dir_name(GLOBAL_GET("application/config/custom_user_dir_name"), true);
			if (custom_dir.is_empty()) {
				custom_dir = appname;
			}
			return get_user_data_dir(custom_dir);
		} else {
			return get_user_data_dir(get_godot_dir_name().path_join("app_userdata").path_join(appname));
		}
	}

	return get_user_data_dir(get_godot_dir_name().path_join("app_userdata").path_join("[unnamed project]"));
}
...

// os_unix.cpp
String OS_Unix::get_user_data_dir(const String &p_dirname) const {
	return get_data_path().path_join(p_dirname);
}
...

// os_windows.cpp
String OS_Unix::get_user_data_dir(const String &p_dirname) const {
	return get_data_path().path_join(p_dirname).replace("\\", "/");
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up extracting this logic to os.cpp, which has the benefit of unifying 3 implementations (Unix, Windows and Web).

@akien-mga
Copy link
Member

akien-mga commented Nov 13, 2024

Tested briefly with the MRP of #99001 which causes a crash on start from a tool script, and with this PR it's properly flagged as requiring Safe Mode, which works well to open the project and then manually fix the tool script.

Also tested with a 4.1 project where I'd install godot-jolt 0.8.0 (known to crash on 4.2+), which crashes when opened with master / this PR, and Safe Mode properly works it around.

One minor nitpick, when you manually select Edit in Safe Mode from the Project Manager for any project, it shows the same dialog as if that project did crash previously, so it starts with:
image

This isn't accurate if selecting Safe Mode manually on projects that didn't crash.

@rsubtil
Copy link
Contributor Author

rsubtil commented Nov 13, 2024

One minor nitpick, when you manually select Edit in Safe Mode from the Project Manager for any project, it shows the same dialog as if that project did crash previously, so it starts with:

Good catch! I'll hide that part of the message when this is triggered manually then 👍

@rsubtil
Copy link
Contributor Author

rsubtil commented Nov 16, 2024

So, in order to remove the first part of the text when the dialog is manually triggered, I created two separate nodes to simply hide the first one depending on that.

And, since now I'm using nodes instead of popup's set_text, I experimented with using a RichTextLabel for the details part, in order to properly render the bullet points:
image

What do you think?

@Faless
Copy link
Collaborator

Faless commented Nov 30, 2024

Sorry for being very late to the party, I think the term safe mode is very misleading, and gives a false sense of security.

I suggest to rename the option as recovery mode since the goal is to make "crashing projects" recoverable, and not to create a secure environment.

@rsubtil
Copy link
Contributor Author

rsubtil commented Dec 1, 2024

I suggest to rename the option as recovery mode since the goal is to make "crashing projects" recoverable, and not to create a secure environment.

Huum I agree, it does a better job at conveying that this mode is mostly intended as a "rescue" mode, not as a slightly less functional but still usable mode for development. I'll change the naming and fix the merge conflict that appeared in the meantime 👍

@rsubtil rsubtil force-pushed the feature-oopsie_woopsie branch 2 times, most recently from 1161b2f to 1fce89c Compare December 7, 2024 10:32
@rsubtil
Copy link
Contributor Author

rsubtil commented Dec 7, 2024

I've made some significant changes this time, so here's a summary (the original description will be updated shortly too):

And some minor changes I took the liberty of doing as well:

  • Disabled the newly added "Game" tab as well during this mode, since it is not possible to run the project.
  • Changed the error codes for GDExtension and EditorNode from OK to invalid ones.

@rsubtil rsubtil changed the title Implement a "Safe Mode" for recovering crashing projects during initialization Implement a "Recovery Mode" for recovering crashing projects during initialization Dec 7, 2024
@akien-mga akien-mga requested a review from bruvzg December 12, 2024 10:43
@rsubtil rsubtil force-pushed the feature-oopsie_woopsie branch from 1fce89c to f87f54c Compare December 15, 2024 12:59
@rsubtil rsubtil requested a review from a team as a code owner December 15, 2024 12:59
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.