Save robustness: no soft lock on a missing level, safe writes, backup fallback #17

Open

hiccup444 wants to merge 2 commits from hiccup444/da-game-hiccup:save-and-level-load-robustness into main

pull from: hiccup444/da-game-hiccup:save-and-level-load-robustness

merge into: dead-air:main

dead-air:main

Conversation 0 Commits 2 Files changed 6 +296 -43

hiccup444 commented 2026-10-25 08:46:08 +00:00

First-time contributor

Copy link

Summary

Two related robustness bugs in the run save and level loading.

A save naming a level that is not installed soft-locked the game. LevelLoader.load_level() unloaded the current level before it checked that the new one existed. Continue with such a save (the expected case once an addon that added a level is removed, since saved content is meant to stay "inert, never deleted") left the player in an empty world with no floor and no menu, while Game._level pointed at the freed level. Continue stayed enabled, so every launch repeated it. From the pause menu, Load unpaused first and then failed, leaving the player unpaused but still anchored, and pause could not be reopened because open_pause() sees the freed level as null.

The run save's backup did not protect much. save_state() was documented as backup-first, but nothing ever read run.json.bak. The backup step copied whatever was at the primary, so after a bad write the next save replaced the last good backup with the corrupt file. The write went straight into run.json and its result was not checked. has_state() only checked that the file existed, so Continue and Load stayed enabled for a save that could not load, and pressing them did nothing.

Changes

Level load (first commit):

• LevelLoader.load_level() loads the new scene, instantiates it and checks that its root is a Level, and only then unloads the current level. A scene that will not load, or whose root is not a Level (a broken addon), is freed and leaves the player where they were.
• Game.continue_game() checks LevelRegistry.has_level() before touching anything. For a missing level it keeps the save (inert, never deleted), logs a warning, leaves the player where they are and returns false. It now returns a bool so callers know whether it worked.
• Pause Load runs continue_game() while still paused, and only on success calls close_pause(), which also un-anchors the rig. On failure the game stays paused with the panel up.
• Continue and pause Load now mean "usable" (Game._save_usable()): a save that reads and names an installed level, meaning it is registered and its scene file is present. This goes one step beyond a readable file, so the button does not light up and then silently do nothing when an addon level is missing. Pause Save refreshes Load the same way instead of assuming the save worked.
• That check reads the file through a new SaveGame.peek_state(), which emits no signals, so lighting a button never fires load_failed or load_completed. Save files are parsed with a JSON instance, because JSON.parse_string() prints an engine error for every corrupt file.
• The main menu's comment on when Continue lights up now matches.

Save writes and backup (second commit):

• save_state() writes <path>.tmp, checks store_string() (a bool in 4.6) and get_error(), and reads the temp file back, which also catches a failure that only shows when close() flushes. It then rotates the previous save to .bak only if that file still loads, and renames the temp file into place. Any failure removes the temp file and emits save_failed.
• load_state() falls back to .bak when the save will not load, logs a warning, and emits load_completed with the file it actually used. When neither file loads, load_failed carries both reasons.
• has_state() and peek_state() consider both files and stay side-effect free.
• The doc comment at the top of the v1 section describes the new behavior.

Tests

tests/save_check.gd:

• A save whose temp file cannot be written (a directory sits in its place) reports failure and leaves both the save and its backup unchanged.
• No .tmp is left after a successful save (the unit save and the game's own saves).
• A truncated save: has_state() is true, peek_state() and load_state() return the backup, load_completed names the .bak, and has_state() and peek_state() emitted nothing.
• Saving over a corrupt save with a good backup: the save succeeds, .bak still holds the old good state, and the new state is in place.
• Save and backup both corrupt: has_state() is false and load_state() returns empty with one load_failed. In the game, Continue is disabled, continue_game() returns false and the mode stays MENU (after a check that a good save does light Continue).
• A save naming no_such_level, Continued from MENU: Continue is disabled, continue_game() returns false, _level is the same level and still in the tree, the mode is MENU, the menu panel is in the tree and visible, Continue is still disabled, and the save is kept.
• A save naming a registered but broken level. With its scene file missing, Continue is disabled. With a scene whose root is not a Level, Continue goes through the loader and returns false, with the same level still in the tree, the mode still MENU and the menu panel in the tree and visible.

tests/pause_check.gd:

• Pause Load with a good save also un-anchors the rig and hides the pause panel. These are regression guards: continue_game() already un-anchors the rig, so they cannot tell close_pause() from a plain unpause.
• The no_such_level save through pause Load: Load is disabled, and the action anyway stays paused with the panel visible, the same level still in the tree, the mode still GAME and the rig anchored. The save is kept.
• A save naming a level whose scene root is not a Level: Load is offered, since only loading shows the problem, and the action stays paused with the panel visible, the same level in the tree, the mode GAME and the rig anchored.
• A pause Save that cannot write leaves Load disabled.

The broken-level checks register a temporary entry in LevelRegistry and restore it with LevelRegistry.reload(). The checks remove every file they create, including .tmp and .bak.

Negative controls, each change reverted on its own against the final code:

• Unloading at the top of load_level(), as on main: 3 checks fail.
• Unloading after load() but before instantiating: 3 fail.
• Pause Load unpausing first, gated on the availability check: 2 fail.
• Writing straight into the save, with no temp file: 8 fail.
• Rotating the old save before writing the new one: 4 fail.
• Pause Save lighting Load unconditionally: 1 fails.
• The availability check without the scene-file check: 1 fails.

The read-back and the store_string()/get_error() checks have no test, because a script cannot force a short write portably.

Full suite list from the CI workflow: smoke_menu, arms_check, addon_check, save_check, rig_motion_check, pause_check and input_check all PASS, on the final commit and on the first commit alone (terrain_check is not on this branch). The local run was on Windows with Godot 4.6.3 and --headless --xr-mode off; CI covers Linux. The only new log lines are expected: the warnings for a save that needs a missing level and for a save loaded from its backup, and, in the broken-level checks, the loader's error that the scene is not a Level plus continue_game()'s error for an unloadable level.

Corrections to the review

• The review suggested removing run.json before renaming .tmp into place, because DirAccess.rename_absolute() replacing an existing target differs by platform. On 4.6.3 it already replaces an existing target on both. On Windows (tested) it returned OK and the target held the new content; the engine source deletes the target and then calls MoveFileW. On Linux it calls rename(), which replaces atomically. An explicit remove is not needed, and on Linux it would add a moment with no save file. One Windows quirk: a rename whose source is missing still deletes the target and returns FAILED, so the code only renames files it knows exist.
• "A rename within one directory is effectively atomic, so there is never a moment with no good file" holds on Linux only. On Windows the rename is delete then move, so run.json is briefly missing between the rotation and the final move. The load fallback to .bak is what covers that window.
• The review's _read() sketch used JSON.parse_string(), which on 4.6.3 prints ERROR: Parse JSON failed for each corrupt or empty file. Availability checks now read the file whenever the menu refreshes, so this uses a JSON instance, which reports the error code silently.
• For the pause path, the review said that reopening pause after the failed Load sets _level.process_mode on the freed node, a script error. On 4.6.3 a reference to a freed node compares equal to null (tested), so the _level == null guard in open_pause() returns early. There is no script error, but pause can no longer be opened at all, so the player cannot reach Resume and has to quit. The fix covers it the same way.
• The review's sketch for load_level() unloads after load() but before instantiate(). A scene that loads but whose root is not a Level (tested with a plain Node3D scene) then still strands the player: the menu level leaves the tree and Game._level keeps pointing at it. The loader now instantiates and checks the root first.

Overlap with open PRs

#11 changes the same spots in game.gd. It adds an in_traversal early return at the top of continue_game() and a _place_paths() call at its end, and adds not in_traversal to the Load availability line in open_pause() and to the pause Load branch. On rebase, the early return becomes return false, _place_paths() goes before return true, and the two conditions become mode == Level.ContentMode.GAME and not in_traversal and _save_usable() and mode == Level.ContentMode.GAME and not in_traversal and continue_game(). level_loader.gd merges cleanly: #11's adopt() and the unload_current() call in its begin_traversal() are unaffected, and every level it loads through load_level() is a Level. #14 touches none of these files.

Sibling PRs from the same review: #15, #16, #18 and #19. Their source changes merge cleanly with each other. Several of them add checks at the same spot in tests/pause_check.gd (and two in tests/save_check.gd), so whichever merges second may need a small rebase that keeps both blocks.

## Summary Two related robustness bugs in the run save and level loading. **A save naming a level that is not installed soft-locked the game.** `LevelLoader.load_level()` unloaded the current level before it checked that the new one existed. Continue with such a save (the expected case once an addon that added a level is removed, since saved content is meant to stay "inert, never deleted") left the player in an empty world with no floor and no menu, while `Game._level` pointed at the freed level. Continue stayed enabled, so every launch repeated it. From the pause menu, Load unpaused first and then failed, leaving the player unpaused but still anchored, and pause could not be reopened because `open_pause()` sees the freed level as null. **The run save's backup did not protect much.** `save_state()` was documented as backup-first, but nothing ever read `run.json.bak`. The backup step copied whatever was at the primary, so after a bad write the next save replaced the last good backup with the corrupt file. The write went straight into `run.json` and its result was not checked. `has_state()` only checked that the file existed, so Continue and Load stayed enabled for a save that could not load, and pressing them did nothing. ## Changes Level load (first commit): - `LevelLoader.load_level()` loads the new scene, instantiates it and checks that its root is a `Level`, and only then unloads the current level. A scene that will not load, or whose root is not a `Level` (a broken addon), is freed and leaves the player where they were. - `Game.continue_game()` checks `LevelRegistry.has_level()` before touching anything. For a missing level it keeps the save (inert, never deleted), logs a warning, leaves the player where they are and returns `false`. It now returns a bool so callers know whether it worked. - Pause Load runs `continue_game()` while still paused, and only on success calls `close_pause()`, which also un-anchors the rig. On failure the game stays paused with the panel up. - Continue and pause Load now mean "usable" (`Game._save_usable()`): a save that reads and names an installed level, meaning it is registered and its scene file is present. This goes one step beyond a readable file, so the button does not light up and then silently do nothing when an addon level is missing. Pause Save refreshes Load the same way instead of assuming the save worked. - That check reads the file through a new `SaveGame.peek_state()`, which emits no signals, so lighting a button never fires `load_failed` or `load_completed`. Save files are parsed with a `JSON` instance, because `JSON.parse_string()` prints an engine error for every corrupt file. - The main menu's comment on when Continue lights up now matches. Save writes and backup (second commit): - `save_state()` writes `<path>.tmp`, checks `store_string()` (a bool in 4.6) and `get_error()`, and reads the temp file back, which also catches a failure that only shows when `close()` flushes. It then rotates the previous save to `.bak` only if that file still loads, and renames the temp file into place. Any failure removes the temp file and emits `save_failed`. - `load_state()` falls back to `.bak` when the save will not load, logs a warning, and emits `load_completed` with the file it actually used. When neither file loads, `load_failed` carries both reasons. - `has_state()` and `peek_state()` consider both files and stay side-effect free. - The doc comment at the top of the v1 section describes the new behavior. ## Tests `tests/save_check.gd`: - A save whose temp file cannot be written (a directory sits in its place) reports failure and leaves both the save and its backup unchanged. - No `.tmp` is left after a successful save (the unit save and the game's own saves). - A truncated save: `has_state()` is true, `peek_state()` and `load_state()` return the backup, `load_completed` names the `.bak`, and `has_state()` and `peek_state()` emitted nothing. - Saving over a corrupt save with a good backup: the save succeeds, `.bak` still holds the old good state, and the new state is in place. - Save and backup both corrupt: `has_state()` is false and `load_state()` returns empty with one `load_failed`. In the game, Continue is disabled, `continue_game()` returns false and the mode stays MENU (after a check that a good save does light Continue). - A save naming `no_such_level`, Continued from MENU: Continue is disabled, `continue_game()` returns false, `_level` is the same level and still in the tree, the mode is MENU, the menu panel is in the tree and visible, Continue is still disabled, and the save is kept. - A save naming a registered but broken level. With its scene file missing, Continue is disabled. With a scene whose root is not a `Level`, Continue goes through the loader and returns false, with the same level still in the tree, the mode still MENU and the menu panel in the tree and visible. `tests/pause_check.gd`: - Pause Load with a good save also un-anchors the rig and hides the pause panel. These are regression guards: `continue_game()` already un-anchors the rig, so they cannot tell `close_pause()` from a plain unpause. - The `no_such_level` save through pause Load: Load is disabled, and the action anyway stays paused with the panel visible, the same level still in the tree, the mode still GAME and the rig anchored. The save is kept. - A save naming a level whose scene root is not a `Level`: Load is offered, since only loading shows the problem, and the action stays paused with the panel visible, the same level in the tree, the mode GAME and the rig anchored. - A pause Save that cannot write leaves Load disabled. The broken-level checks register a temporary entry in `LevelRegistry` and restore it with `LevelRegistry.reload()`. The checks remove every file they create, including `.tmp` and `.bak`. Negative controls, each change reverted on its own against the final code: - Unloading at the top of `load_level()`, as on `main`: 3 checks fail. - Unloading after `load()` but before instantiating: 3 fail. - Pause Load unpausing first, gated on the availability check: 2 fail. - Writing straight into the save, with no temp file: 8 fail. - Rotating the old save before writing the new one: 4 fail. - Pause Save lighting Load unconditionally: 1 fails. - The availability check without the scene-file check: 1 fails. The read-back and the `store_string()`/`get_error()` checks have no test, because a script cannot force a short write portably. Full suite list from the CI workflow: smoke_menu, arms_check, addon_check, save_check, rig_motion_check, pause_check and input_check all PASS, on the final commit and on the first commit alone (terrain_check is not on this branch). The local run was on Windows with Godot 4.6.3 and `--headless --xr-mode off`; CI covers Linux. The only new log lines are expected: the warnings for a save that needs a missing level and for a save loaded from its backup, and, in the broken-level checks, the loader's error that the scene is not a Level plus `continue_game()`'s error for an unloadable level. ## Corrections to the review - The review suggested removing `run.json` before renaming `.tmp` into place, because `DirAccess.rename_absolute()` replacing an existing target differs by platform. On 4.6.3 it already replaces an existing target on both. On Windows (tested) it returned OK and the target held the new content; the engine source deletes the target and then calls `MoveFileW`. On Linux it calls `rename()`, which replaces atomically. An explicit remove is not needed, and on Linux it would add a moment with no save file. One Windows quirk: a rename whose source is missing still deletes the target and returns FAILED, so the code only renames files it knows exist. - "A rename within one directory is effectively atomic, so there is never a moment with no good file" holds on Linux only. On Windows the rename is delete then move, so `run.json` is briefly missing between the rotation and the final move. The load fallback to `.bak` is what covers that window. - The review's `_read()` sketch used `JSON.parse_string()`, which on 4.6.3 prints `ERROR: Parse JSON failed` for each corrupt or empty file. Availability checks now read the file whenever the menu refreshes, so this uses a `JSON` instance, which reports the error code silently. - For the pause path, the review said that reopening pause after the failed Load sets `_level.process_mode` on the freed node, a script error. On 4.6.3 a reference to a freed node compares equal to `null` (tested), so the `_level == null` guard in `open_pause()` returns early. There is no script error, but pause can no longer be opened at all, so the player cannot reach Resume and has to quit. The fix covers it the same way. - The review's sketch for `load_level()` unloads after `load()` but before `instantiate()`. A scene that loads but whose root is not a `Level` (tested with a plain `Node3D` scene) then still strands the player: the menu level leaves the tree and `Game._level` keeps pointing at it. The loader now instantiates and checks the root first. ## Overlap with open PRs #11 changes the same spots in `game.gd`. It adds an `in_traversal` early return at the top of `continue_game()` and a `_place_paths()` call at its end, and adds `not in_traversal` to the Load availability line in `open_pause()` and to the pause Load branch. On rebase, the early return becomes `return false`, `_place_paths()` goes before `return true`, and the two conditions become `mode == Level.ContentMode.GAME and not in_traversal and _save_usable()` and `mode == Level.ContentMode.GAME and not in_traversal and continue_game()`. `level_loader.gd` merges cleanly: #11's `adopt()` and the `unload_current()` call in its `begin_traversal()` are unaffected, and every level it loads through `load_level()` is a `Level`. #14 touches none of these files. Sibling PRs from the same review: #15, #16, #18 and #19. Their source changes merge cleanly with each other. Several of them add checks at the same spot in `tests/pause_check.gd` (and two in `tests/save_check.gd`), so whichever merges second may need a small rebase that keeps both blocks.

hiccup444 added 2 commits 2026-10-25 08:46:08 +00:00

core: keep the current level when a save's level will not load ... b4ee0a24c6

LevelLoader.load_level unloaded the current level before it checked
that the new one existed. Continue with a save naming a missing level
(an addon level whose addon was removed) left the player in an empty
world with no menu, and Game holding a freed level. From the pause
menu, Load unpaused first, so a failed load left the player unpaused
but still anchored.

The loader now unloads only once the new scene is loaded and its root
is a Level, so a broken addon scene cannot strand the player either.
continue_game checks the save's level is installed before touching
anything, keeps the save (inert, never deleted), warns, and returns
whether it worked. Pause Load runs while still paused and closes pause
only on success. Continue and pause Load light up only for a save that
reads and names an installed level, registered with its scene present.
That check reads the file through a new side-effect free
SaveGame.peek_state, so lighting a button emits no load signals.

save: write through a temp file and fall back to the backup ... 8e69232bc9

The run save was documented as backup-first, but nothing ever read the
.bak, the backup step copied whatever was at the primary (a corrupt
file included, so the first save after a bad write replaced the last
good backup), the write went straight into the primary unchecked, and
has_state only checked that the file existed.

save_state now writes <path>.tmp, checks store_string and get_error,
and reads the file back before it replaces anything. The previous save
is rotated to .bak only if it still loads, then the temp file is
renamed into place. On 4.6.3 rename_absolute already replaces an
existing target (Windows deletes it first, Linux renames atomically),
so no explicit remove is needed. load_state falls back to .bak and
load_completed names the file actually used. has_state and peek_state
consider both files and never emit, so a truncated save with a good
backup still offers Continue, and two unreadable files offer nothing.

hiccup444 referenced this pull request 2026-10-25 08:46:34 +00:00

XR session setup: StartXR, pause on focus loss, XR shaders #15

hiccup444 referenced this pull request 2026-10-25 08:46:35 +00:00

Place the player, not the XR origin, at spawn, menu and save #16

hiccup444 referenced this pull request 2026-10-25 08:46:37 +00:00

Addon docs: imported assets need a Godot-exported pack #18

hiccup444 referenced this pull request 2026-10-25 08:46:37 +00:00

Hidden menu panels stop catching pointers and rendering #19

Some checks failed

Hide checks

tests / test (pull_request) Failing after 58s

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u save-and-level-load-robustness:hiccup444-save-and-level-load-robustness

git switch hiccup444-save-and-level-load-robustness

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff hiccup444-save-and-level-load-robustness

git switch hiccup444-save-and-level-load-robustness

git rebase main

git switch main

git merge --ff-only hiccup444-save-and-level-load-robustness

git switch hiccup444-save-and-level-load-robustness

git rebase main

git switch main

git merge --no-ff hiccup444-save-and-level-load-robustness

git switch main

git merge --squash hiccup444-save-and-level-load-robustness

git switch main

git merge --ff-only hiccup444-save-and-level-load-robustness

git switch main

git merge hiccup444-save-and-level-load-robustness

git push origin main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

dead-air/engine!17

No description provided.