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/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")