Tests: Speichern beim Schließen und automatisches Speichern

Beides setzt Konzept 9 um („automatisch in festen Abständen“, Speichern
beim Schließen), und beides ließ sich entfernen, ohne dass ein Test
rot wurde.

- Schließen: Die Hauptszene bekommt NOTIFICATION_WM_CLOSE_REQUEST. Danach
  meldet ein SaveStore im selben Ordner einen Spielstand und genau eine
  Aufzeichnung.
- Automatisch: Die Hauptszene läuft per _run_tick bis zum Takt vor
  AUTOSAVE_TICKS, ohne Bilder abzuwarten. Bis dahin gibt es keinen
  Spielstand, einen Takt später gibt es einen.

Entfernt man das Speichern beim Schließen, schlägt der erste Test fehl;
entfernt man das automatische Speichern in _run_tick, der zweite. Ohne
das Schreiben der Aufzeichnung beim Schließen schlägt der erste ebenfalls
fehl.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

CLAUDE.md

@@ -27,11 +27,11 @@ Alle Spieler simulieren die Welt selbst und tauschen nur Befehle aus (Gleichschr
 - Keine Sammlung mit wechselnder Reihenfolge. Statt einer Hash-Tabelle nimmst du `Vec`, `BTreeMap` oder `BTreeSet`.
 - Die Welt ändert sich nur durch einen `Command` in `World::step`. Einzige Ausnahme ist das Laden eines Speicherstands mit `World::load`, das alle Spieler abmeldet.
 - Wer eine Sitzung aufzeichnet, ruft `Recording::step` statt `World::step` auf. So enthält die Aufzeichnung genau die ausgeführten Befehle.
-- Alles, was zum Zustand der Welt gehört, steht im Snapshot. Wer ein Feld ergänzt, ergänzt es in `snapshot.rs`, prüft es in `check_restorable` und erhöht `SNAPSHOT_VERSION`.
+- Alles, was zum Zustand der Welt gehört, steht im Snapshot. Wer ein Feld ergänzt, ergänzt es im Kodierer und im Dekodierer seiner Struktur (`snapshot.rs` für die Welt, `player.rs` für Spieler, `inventory.rs` für das Inventar, `map.rs` für die Karte), prüft es beim Dekodieren oder in `check_restorable` und erhöht `SNAPSHOT_VERSION`.
 - Wo eine Rechnung im Spiel überlaufen kann, steht eine ausdrückliche Wahl: `saturating_`, `wrapping_` oder `checked_`. Taktzähler laufen praktisch nie über und werden normal addiert: Die Welt zählt in `u64`, eine Aufzeichnung in `u32`, was bei 20 Takten pro Sekunde für über sechs Jahre reicht. Takte aus geladenen Dateien begrenzt `MAX_RESTORABLE_TICK`.
 - Keine fremden Crates im Kern.
 
-Drei Wächter prüfen das. `core/clippy.toml` sperrt Pfade wie `std::collections::HashMap` oder `std::fs::read`, auch über Aliase und gruppierte Importe. Der Test `core/tests/forbidden_apis.rs` durchsucht zusätzlich den Text nach verbotenen Begriffen; sie dürfen dort auch in Kommentaren nicht vorkommen. `core/tests/no_dependencies.rs` hält die Abhängigkeiten leer.
+Drei Wächter prüfen das. `core/clippy.toml` sperrt Pfade wie `std::collections::HashMap` oder `std::fs::read`, auch über Aliase und gruppierte Importe. Der Test `core/tests/forbidden_apis.rs` durchsucht zusätzlich den Text nach verbotenen Begriffen; sie dürfen dort auch in Kommentaren nicht vorkommen. Gruppierte Importe aus `std` (`use std::{…}`) sind verboten, weil sie dem Text die Modulnamen verbergen: Jeder Pfad steht in einer eigenen `use`-Zeile. `core/tests/no_dependencies.rs` hält die Abhängigkeiten leer.
 
 ## Regeln für Brücke und Hülle
 
@@ -39,7 +39,7 @@ Drei Wächter prüfen das. `core/clippy.toml` sperrt Pfade wie `std::collections
 - GDScript mit Typangaben schreiben (`var x := …`, Rückgabetypen an jeder Funktion).
 - Eingabeaktionen legt `game/scripts/input_setup.gd` an, nicht `project.godot`.
 - Alle Texte, die Spieler sehen, stehen in `game/i18n/texts.csv` (Deutsch und Englisch) und werden über `tr()` geholt.
-- Tests, die Dateien schreiben, nutzen `TestFiles.temporary_dir()`, nie den echten Benutzerordner.
+- Tests, die Dateien schreiben, nutzen `TestFiles.temporary_dir()` (Ordner unter `user://tests`), nie die echten Dateien des Spielers.
 - Godot erzeugt beim Import zu jedem Skript eine `.uid`-Datei. Sie gehört ins Archiv. Der Ordner `game/.godot/` gehört nicht hinein.
 
 ## Prüfen

core/clippy.toml

@@ -1,6 +1,9 @@
-# Paths the deterministic core must never use. Clippy resolves imports, so
-# aliases and grouped imports such as `use std::{fs, env}` are caught too.
-# The text search in core/tests/forbidden_apis.rs covers the rest.
+# Paths the deterministic core must never use. Clippy resolves paths, so a
+# listed path is caught under an alias or in a grouped import as well, for
+# example `fs::read` after `use std::{fs, env}`. Only the paths listed below
+# are checked. Everything else is the job of the text search in
+# core/tests/forbidden_apis.rs, which forbids grouped imports from `std`
+# because they hide module names from it.
 
 disallowed-types = [
     { path = "std::collections::HashMap", reason = "iteration order differs between runs; use BTreeMap or Vec" },

core/tests/forbidden_apis.rs

@@ -42,12 +42,20 @@ const FORBIDDEN: &[(&str, &str)] = &[
         "the core must not read or write streams; the shell does that",
     ),
     ("std::process", "the core must not start or end processes"),
+    (
+        "std::{",
+        "grouped std imports hide forbidden modules from this search; import each path on its own line",
+    ),
     ("static mut", "global mutable state can differ between runs"),
     (
         "thread_local",
         "global mutable state can differ between runs",
     ),
     ("Atomic", "global mutable state can differ between runs"),
+    ("Mutex", "global mutable state can differ between runs"),
+    ("RwLock", "global mutable state can differ between runs"),
+    ("OnceLock", "global mutable state can differ between runs"),
+    ("LazyLock", "global mutable state can differ between runs"),
     ("unsafe", "the core forbids unsafe code"),
 ];
 

core/tests/replay.rs

@@ -106,6 +106,13 @@ fn a_recording_can_start_in_the_middle_of_a_session() {
     }
     assert_eq!(recording.tick_count(), 70);
     assert_eq!(recording.end_tick(), 120);
+
+    // Decoding derives the start tick from the stored start world. The
+    // decoded recording must end at the same tick and equal the original.
+    let (decoded, _) = Recording::from_bytes(&recording.to_bytes(&world)).unwrap();
+    assert_eq!(decoded.end_tick(), 120);
+    assert_eq!(decoded, recording);
+
     assert_eq!(recording.replay(), Ok(world));
 }
 

core/tests/snapshot.rs

@@ -3,7 +3,7 @@ use grovewright_core::command::Command;
 use grovewright_core::hash::fnv1a64;
 use grovewright_core::map::{Map, Pos, Terrain};
 use grovewright_core::mapgen::outcrop_origin;
-use grovewright_core::player::{Player, PlayerId, MAX_ONLINE_PLAYERS};
+use grovewright_core::player::{Player, PlayerId, MAX_KNOWN_PLAYERS, MAX_ONLINE_PLAYERS};
 use grovewright_core::snapshot::{SnapshotError, MAX_RESTORABLE_TICK, SNAPSHOT_VERSION};
 use grovewright_core::world::{Event, World, DIG_COOLDOWN_TICKS};
 
@@ -256,3 +256,43 @@ fn states_right_at_the_limits_are_accepted() {
     let bytes = snapshot_of(1, MAX_RESTORABLE_TICK, &map, &players);
     assert!(World::from_snapshot(&bytes).is_ok());
 }
+
+/// A world that knows `MAX_KNOWN_PLAYERS` players, built through commands.
+/// They join in groups of `MAX_ONLINE_PLAYERS`, and each group but the last
+/// leaves before the next one joins, so only the last group is online.
+fn world_with_a_full_roster() -> World {
+    let mut world = World::new(7);
+    let groups = MAX_KNOWN_PLAYERS / MAX_ONLINE_PLAYERS;
+    for group in 0..groups {
+        let first = group * MAX_ONLINE_PLAYERS;
+        for index in first..first + MAX_ONLINE_PLAYERS {
+            world.step(&[Command::Join {
+                key: [index as u8; 16],
+                name: format!("P{index}"),
+            }]);
+        }
+        if group + 1 < groups {
+            for index in first..first + MAX_ONLINE_PLAYERS {
+                world.step(&[Command::Leave {
+                    player: PlayerId(index as u8),
+                }]);
+            }
+        }
+    }
+    world
+}
+
+#[test]
+fn a_full_roster_with_most_players_offline_is_accepted() {
+    let world = world_with_a_full_roster();
+    assert_eq!(world.players().len(), MAX_KNOWN_PLAYERS);
+    let online = world.players().iter().filter(|player| player.online);
+    assert_eq!(online.count(), MAX_ONLINE_PLAYERS);
+
+    let restored = World::from_snapshot(&world.to_snapshot()).unwrap();
+    assert_eq!(restored, world);
+
+    let loaded = World::load(&world.to_snapshot()).unwrap();
+    assert_eq!(loaded.players().len(), MAX_KNOWN_PLAYERS);
+    assert!(loaded.players().iter().all(|player| !player.online));
+}

docs/playtests/m1b.md

@@ -16,8 +16,8 @@ M1b ist noch kein Spiel, sondern das Fundament zum Anfassen: Du läufst über di
 
 1. Fühlt sich das Laufen gut an: Tempo, Halten an Wänden, Gleiten an Wänden entlang?
 2. Passt die Reichweite der Spitzhacke: zu kurz, zu weit oder genau richtig?
-3. Fühlt sich der Rhythmus des Schlagens gut an? Ein Fels braucht drei Schläge, höchstens vier Schläge gehen pro Sekunde.
+3. Fühlt sich der Rhythmus des Schlagens gut an? Weicher Fels braucht drei Schläge, höchstens vier Schläge gehen pro Sekunde. Harter Fels lässt sich noch nicht abbauen: Schläge darauf bewirken nichts, auch wenn das Feld grün umrandet ist.
 4. Ist auf einen Blick klar, was Boden, weicher Fels, harter Fels und der Rand der Karte ist?
-5. Ist dir etwas Merkwürdiges passiert? Die Aufzeichnungen deiner Sitzungen liegen unter `%APPDATA%\Godot\app_userdata\Grovewright\recordings`. Schick mir die neueste Datei, dann spiele ich die Sitzung exakt nach.
+5. Ist dir etwas Merkwürdiges passiert? Die Aufzeichnungen deiner Sitzungen liegen unter `%APPDATA%\Godot\app_userdata\Grovewright\recordings`. Eine Aufzeichnung entsteht nur, wenn du das Fenster schließt oder mit F9 einen Stand lädst. Schick mir die neueste Datei, dann spiele ich die Sitzung exakt nach. Nach einem Absturz oder nach der Meldung „Die Simulation wurde angehalten“ gibt es von dieser Sitzung keine Aufzeichnung. Schick mir dann stattdessen den Ordner `%APPDATA%\Godot\app_userdata\Grovewright\logs`, und zwar sofort, bevor du das Spiel erneut startest, denn jeder Start verdrängt ältere Logdateien.
 
 Antworte formlos im Chat oder als Kommentar in Gitea.

game/scripts/main.gd

@@ -8,7 +8,7 @@ const PLAYER_NAME := "Spieler"
 ## How far the pickaxe reaches, in tiles from the figure to the tile centre.
 const REACH_TILES := 2.5
 ## Ticks between two automatic saves: two minutes.
-const AUTOSAVE_TICKS := 2400
+const AUTOSAVE_TICKS := 2 * 60 * GwSession.TICKS_PER_SECOND
 const CAMERA_ZOOM := Vector2(3.0, 3.0)
 
 ## Folder for the player key, the save and the recordings. Tests use their
@@ -154,8 +154,10 @@ func _try_dig() -> void:
 		dig_at(marker.tile)
 
 
-## Queues a pickaxe hit on `tile`, at most once per cooldown. The core checks
-## everything else.
+## Queues a pickaxe hit on `tile`, at most once per cooldown. The caller must
+## check the reach: figures move outside the lockstep, so the core does not
+## know where the figure is. The core checks the cooldown, the tile, its
+## terrain, the tool tier and the free space in the inventory.
 func dig_at(tile: Vector2i) -> void:
 	if session.tick() < _next_dig_tick:
 		return

game/scripts/save_store.gd

@@ -5,6 +5,8 @@ extends RefCounted
 
 const KEY_FILE := "player.key"
 const SAVE_FILE := "world.grw"
+## Holds the previous save while write_save replaces it.
+const BACKUP_FILE := SAVE_FILE + ".bak"
 const RECORDINGS_DIR := "recordings"
 ## Recordings beyond this number are deleted, oldest first.
 const KEEP_RECORDINGS := 10
@@ -13,9 +15,13 @@ const KEY_BYTES := 16
 var base_dir: String
 
 
+## Opens the folder and creates it if it is missing. If a crash interrupted a
+## save and left only the backup of the previous one (see write_save), the
+## backup becomes the save again.
 func _init(directory: String = "user://") -> void:
 	base_dir = directory
 	DirAccess.make_dir_recursive_absolute(base_dir)
+	_restore_backup()
 
 
 ## The key that identifies this installation in every world. It is created
@@ -41,15 +47,35 @@ func read_save() -> PackedByteArray:
 ## Writes the save through a temporary file, so that a crash or a failed
 ## write never destroys the previous save. Returns false if the new save was
 ## not stored completely; the previous save then stays as it was.
+##
+## The temporary file replaces the previous save in two moves: the previous
+## save becomes the backup, then the temporary file becomes the save. A single
+## move onto the previous save would not be safe, because on Windows Godot
+## deletes the target first and only then moves the file. If that move failed,
+## for example because a virus scanner holds the temporary file, the previous
+## save would already be gone. If the second move fails, the backup is moved
+## back. A crash between the two moves leaves only the backup; the next
+## SaveStore on the folder restores it.
 func write_save(bytes: PackedByteArray) -> bool:
 	if bytes.is_empty():
 		return false
 	var path := base_dir.path_join(SAVE_FILE)
 	var temporary := path + ".tmp"
+	var backup := base_dir.path_join(BACKUP_FILE)
 	if not _write(temporary, bytes) or FileAccess.get_file_as_bytes(temporary) != bytes:
 		DirAccess.remove_absolute(temporary)
 		return false
-	return DirAccess.rename_absolute(temporary, path) == OK
+	var has_previous := FileAccess.file_exists(path)
+	if has_previous and DirAccess.rename_absolute(path, backup) != OK:
+		DirAccess.remove_absolute(temporary)
+		return false
+	if DirAccess.rename_absolute(temporary, path) != OK:
+		if has_previous:
+			DirAccess.rename_absolute(backup, path)
+		return false
+	if has_previous:
+		DirAccess.remove_absolute(backup)
+	return true
 
 
 ## Moves an unusable save out of the way, so that the next save does not
@@ -89,6 +115,16 @@ func recordings() -> PackedStringArray:
 	return names
 
 
+## Moves the backup back to the save if there is no save, see write_save.
+func _restore_backup() -> void:
+	var path := base_dir.path_join(SAVE_FILE)
+	var backup := base_dir.path_join(BACKUP_FILE)
+	if FileAccess.file_exists(path) or not FileAccess.file_exists(backup):
+		return
+	if DirAccess.rename_absolute(backup, path) != OK:
+		push_warning("the previous save could not be restored from %s" % backup)
+
+
 func _write(path: String, bytes: PackedByteArray) -> bool:
 	var file := FileAccess.open(path, FileAccess.WRITE)
 	if file == null:

game/tests/run_tests.gd

@@ -2,7 +2,8 @@ extends SceneTree
 ## Runs every test script in res://tests whose file name starts with "test_".
 ## Every method whose name starts with "test_" is one test. A test fails if a
 ## check fails or if an error is reported while it runs, for example a call
-## on a null value. Exits with code 1 if any test failed.
+## on a null value. Exits with code 1 if any test failed or if no test ran,
+## so that a renamed test folder or misnamed tests cannot pass as a clean run.
 ##
 ## From the repository root:
 ##   godot --headless --path game -s res://tests/run_tests.gd
@@ -43,6 +44,11 @@ func _initialize() -> void:
 
 
 func _run_all() -> void:
+	# Remove what runs that stopped early left behind, before the error
+	# counter starts. Only if the folder exists, because Godot reports an
+	# error when it lists a missing one.
+	if DirAccess.dir_exists_absolute(TestFiles.ROOT):
+		TestFiles.remove_dir(TestFiles.ROOT)
 	var counter := ErrorCounter.new()
 	OS.add_logger(counter)
 	var passed := 0
@@ -78,4 +84,4 @@ func _run_all() -> void:
 					print("        " + failure)
 	OS.remove_logger(counter)
 	print("%d passed, %d failed" % [passed, failed])
-	quit(1 if failed > 0 else 0)
+	quit(1 if failed > 0 or passed == 0 else 0)

game/tests/support/test_files.gd

@@ -1,18 +1,29 @@
 class_name TestFiles
 extends RefCounted
-## Temporary folders below user://tests for tests that write files, so that
-## tests never touch the player's real key, save or recordings.
+## Temporary folders below user://tests for tests that write files. That
+## folder lies inside the game's real user folder, next to the player's own
+## key, save and recordings, so nothing outside it may be deleted here.
+
+## The only folder the tests write to and remove_dir deletes from.
+const ROOT := "user://tests"
 
 
 static func temporary_dir(label: String) -> String:
-	var path := "user://tests".path_join("%s-%d" % [label, Time.get_ticks_usec()])
+	var path := ROOT.path_join("%s-%d" % [label, Time.get_ticks_usec()])
 	DirAccess.make_dir_recursive_absolute(path)
 	return path
 
 
+## Deletes the folder `path` with everything in it. Refuses every path that is
+## not ROOT or below it, with an error and without deleting anything: a wrong
+## argument must never delete the player's real files.
 static func remove_dir(path: String) -> void:
-	for file in DirAccess.get_files_at(path):
-		DirAccess.remove_absolute(path.path_join(file))
-	for directory in DirAccess.get_directories_at(path):
-		remove_dir(path.path_join(directory))
-	DirAccess.remove_absolute(path)
+	var clean := path.simplify_path()
+	if clean != ROOT and not clean.begins_with(ROOT + "/"):
+		push_error("TestFiles.remove_dir refuses %s: it is not %s or below it" % [path, ROOT])
+		return
+	for file in DirAccess.get_files_at(clean):
+		DirAccess.remove_absolute(clean.path_join(file))
+	for directory in DirAccess.get_directories_at(clean):
+		remove_dir(clean.path_join(directory))
+	DirAccess.remove_absolute(clean)

game/tests/test_main.gd

@@ -46,6 +46,33 @@ func test_a_saved_game_is_continued_on_the_next_start() -> void:
 	TestFiles.remove_dir(directory)
 
 
+func test_closing_the_window_saves_the_game_and_records_the_session() -> void:
+	var directory := TestFiles.temporary_dir("close")
+	var main := _start_main(directory)
+	await tree.process_frame
+	main.notification(Node.NOTIFICATION_WM_CLOSE_REQUEST)
+	var store := SaveStore.new(directory)
+	check(store.has_save(), "the game was saved")
+	check_eq(store.recordings().size(), 1, "the session was recorded")
+	_stop_main(main)
+	TestFiles.remove_dir(directory)
+
+
+func test_the_game_is_saved_automatically_every_two_minutes() -> void:
+	var directory := TestFiles.temporary_dir("autosave")
+	var main := _start_main(directory)
+	var store := SaveStore.new(directory)
+	check(not store.has_save(), "there is no save at the start")
+	# No frames are awaited, so only these ticks run.
+	while main.session.tick() < main.AUTOSAVE_TICKS - 1:
+		main._run_tick()
+	check(not store.has_save(), "still no save one tick before the interval ends")
+	main._run_tick()
+	check(store.has_save(), "saved when the interval ends")
+	_stop_main(main)
+	TestFiles.remove_dir(directory)
+
+
 func test_an_unusable_save_is_set_aside_and_a_new_world_starts() -> void:
 	var directory := TestFiles.temporary_dir("broken")
 	var store := SaveStore.new(directory)

game/tests/test_save_store.gd

@@ -19,6 +19,7 @@ func test_a_save_is_written_and_read_back() -> void:
 	check(store.has_save(), "a save exists")
 	check_eq(store.read_save(), PackedByteArray([4, 5]), "the newest save is read")
 	check(not FileAccess.file_exists(directory.path_join(SaveStore.SAVE_FILE + ".tmp")), "no temporary file is left")
+	check(not FileAccess.file_exists(directory.path_join(SaveStore.BACKUP_FILE)), "no backup is left")
 	TestFiles.remove_dir(directory)
 
 
@@ -43,6 +44,58 @@ func test_a_save_that_cannot_be_written_keeps_the_previous_one() -> void:
 	TestFiles.remove_dir(directory)
 
 
+func test_a_save_that_cannot_be_moved_into_place_keeps_the_previous_one() -> void:
+	var directory := TestFiles.temporary_dir("held")
+	var store := SaveStore.new(directory)
+	store.write_save(PackedByteArray([1, 2, 3]))
+	# The open handle stands for an outside program, for example a virus
+	# scanner. The new save can be written, but Windows refuses to move a file
+	# that is held open.
+	var holder := FileAccess.open(directory.path_join(SaveStore.SAVE_FILE + ".tmp"), FileAccess.WRITE)
+	check(not store.write_save(PackedByteArray([4, 5])), "the failed save is reported")
+	check_eq(store.read_save(), PackedByteArray([1, 2, 3]), "the previous save is untouched")
+	check(not FileAccess.file_exists(directory.path_join(SaveStore.BACKUP_FILE)), "no backup is left")
+	holder.close()
+	TestFiles.remove_dir(directory)
+
+
+func test_a_save_that_cannot_move_the_previous_one_aside_keeps_it() -> void:
+	var directory := TestFiles.temporary_dir("aside-blocked")
+	var store := SaveStore.new(directory)
+	store.write_save(PackedByteArray([1, 2, 3]))
+	# A folder under the backup name keeps the previous save from moving.
+	DirAccess.make_dir_absolute(directory.path_join(SaveStore.BACKUP_FILE))
+	check(not store.write_save(PackedByteArray([4, 5])), "the failed save is reported")
+	check_eq(store.read_save(), PackedByteArray([1, 2, 3]), "the previous save is untouched")
+	check(not FileAccess.file_exists(directory.path_join(SaveStore.SAVE_FILE + ".tmp")), "no temporary file is left")
+	TestFiles.remove_dir(directory)
+
+
+func test_an_interrupted_save_is_recovered_from_the_backup() -> void:
+	var directory := TestFiles.temporary_dir("recover")
+	# A crash between the two moves of write_save leaves only the backup.
+	var backup := directory.path_join(SaveStore.BACKUP_FILE)
+	var file := FileAccess.open(backup, FileAccess.WRITE)
+	file.store_buffer(PackedByteArray([7, 8, 9]))
+	file.close()
+	var store := SaveStore.new(directory)
+	check(store.has_save(), "the previous save is back")
+	check_eq(store.read_save(), PackedByteArray([7, 8, 9]), "with its content")
+	check(not FileAccess.file_exists(backup), "the backup is moved, not copied")
+	TestFiles.remove_dir(directory)
+
+
+func test_a_backup_never_replaces_an_existing_save() -> void:
+	var directory := TestFiles.temporary_dir("keep")
+	SaveStore.new(directory).write_save(PackedByteArray([1, 2, 3]))
+	var file := FileAccess.open(directory.path_join(SaveStore.BACKUP_FILE), FileAccess.WRITE)
+	file.store_buffer(PackedByteArray([9, 9]))
+	file.close()
+	var store := SaveStore.new(directory)
+	check_eq(store.read_save(), PackedByteArray([1, 2, 3]), "the save is not replaced by the older backup")
+	TestFiles.remove_dir(directory)
+
+
 func test_setting_aside_without_a_save_reports_nothing_moved() -> void:
 	var directory := TestFiles.temporary_dir("nothing")
 	check_eq(SaveStore.new(directory).set_aside_save(), "", "no save, nothing moved")