From 2ec217efbf8bf17a8c798534f4e128fea9975a7a Mon Sep 17 00:00:00 2001 From: Courier Date: Thu, 15 Oct 2026 02:31:46 +0000 Subject: [PATCH] fix(web-backup): prune candidate dictionary before writing BACKUP_STORAGE_KEY (issue #421) The web backup writer stored the full MAX_BACKUPS+1 candidate first and pruned after, so once the backup cap filled every rotation needed storage for the temporary sixth snapshot. If the browser's remaining localStorage capacity only fit the final five-snapshot set, the first write failed and _prune_old_backups() never ran, leaving New Game callers with no recoverable backup. Prune the candidate dictionary in memory using the existing newest-first numeric ordering BEFORE calling _store_web_backups, so the aggregated BACKUP_STORAGE_KEY value never carries more than MAX_BACKUPS entries. If the trimmed payload itself cannot be written, return empty and leave the previously stored backups byte-identical. Adds _prune_backups_in_memory() helper used by both the write path and _prune_old_backups(), plus deterministic hook-based regression tests for the quota-blocked rotation and the preservation of existing backups under a persistent write failure. --- scripts/game_state.gd | 34 ++++++++++++--- tests/test_web_backup.gd | 91 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 6 deletions(-) diff --git a/scripts/game_state.gd b/scripts/game_state.gd index 3fae9cb..17beba0 100644 --- a/scripts/game_state.gd +++ b/scripts/game_state.gd @@ -775,9 +775,20 @@ func _backup_save_local_storage() -> String: var backups := _load_web_backups() var backup_key := _backup_filename() backups[backup_key] = current + # Prune the candidate BEFORE writing — the BACKUP_STORAGE_KEY value + # is one aggregated JSON payload, so writing the full + # MAX_BACKUPS+1 set first and then pruning would briefly require + # storage for MAX_BACKUPS+1 entries. If the browser's remaining + # capacity only fits the intended MAX_BACKUPS retained set, the + # old write-before-prune order failed before any pruning happened + # and the New Game caller reset the colony with no recoverable + # backup (issue #421). + backups = _prune_backups_in_memory(backups) if not _store_web_backups(backups): + # The final retained payload did not fit either. Do not delete + # stored backups first to make space — preserve the previously + # stored backups unchanged (issue #421). return "" - _prune_old_backups() # Same defensive check as backup_save() — see issue #411. if not _load_web_backups().has(backup_key): return "" @@ -838,12 +849,10 @@ func _prune_old_backups() -> void: """Delete oldest backups beyond MAX_BACKUPS. list_backups() is newest-first.""" if use_local_storage: var backups := _load_web_backups() - var names := _sorted_backup_names(backups.keys()) - if names.size() <= MAX_BACKUPS: + var pruned := _prune_backups_in_memory(backups) + if pruned.size() == backups.size(): return - for old_name in names.slice(MAX_BACKUPS): - backups.erase(old_name) - _store_web_backups(backups) + _store_web_backups(pruned) return var backups := list_backups() if backups.size() <= MAX_BACKUPS: @@ -851,6 +860,19 @@ func _prune_old_backups() -> void: for old_path in backups.slice(MAX_BACKUPS): DirAccess.remove_absolute(ProjectSettings.globalize_path(String(old_path))) +# Returns a copy of `backups` trimmed to at most MAX_BACKUPS entries +# using the same newest-first numeric ordering the public list/prune +# logic uses (issue #411). The web backup write path calls this BEFORE +# writing BACKUP_STORAGE_KEY so the on-disk payload never has to store +# more than MAX_BACKUPS entries — see issue #421. +func _prune_backups_in_memory(backups: Dictionary) -> Dictionary: + var names := _sorted_backup_names(backups.keys()) + if names.size() <= MAX_BACKUPS: + return backups + for old_name in names.slice(MAX_BACKUPS): + backups.erase(old_name) + return backups + func list_backups() -> Array[String]: """Return sorted (newest-first) list of backup paths (desktop) or keys (web).""" if use_local_storage: diff --git a/tests/test_web_backup.gd b/tests/test_web_backup.gd index 46bccfb..7f417d8 100644 --- a/tests/test_web_backup.gd +++ b/tests/test_web_backup.gd @@ -34,6 +34,8 @@ func run_tests() -> void: flow_backup_skips_when_no_live_save() flow_backup_write_failure_returns_empty() flow_restore_write_failure_returns_empty() + flow_backup_pruning_avoids_quota_blocked_rotation() + flow_backup_save_preserves_existing_when_trimmed_payload_fails() teardown() func setup() -> void: @@ -172,6 +174,95 @@ func flow_restore_write_failure_returns_empty() -> void: _fail_write_keys.clear() assert_eq(int(_gs.load_game().get("tick", -1)), 80, "failed web restore leaves the live save untouched") +# 9) Pruning happens BEFORE the BACKUP_STORAGE_KEY write. With five +# existing snapshots stored and a writer that rejects only the +# six-snapshot payload, the new snapshot rotates in instead of failing. +# This is the deterministic regression for the write-before-prune order +# fixed in issue #421. +func flow_backup_pruning_avoids_quota_blocked_rotation() -> void: + _ls.clear() + _gs._backup_counter = 0 + # Seed five snapshots with the original (always-succeed) writer. + for i in 5: + _gs.save_game(_valid_state(i + 1)) + _gs.backup_save() + assert_eq(_parsed_backups().size(), 5, "seeded five existing web backups") + var pre_snapshot_payload: String = String(_ls.get(GameState.BACKUP_STORAGE_KEY, "")) + # Swap in a writer that rejects aggregated payloads larger than the + # MAX_BACKUPS-trimmed set, simulating a browser whose remaining + # localStorage capacity holds the final five but not the temporary + # six. Only the BACKUP_STORAGE_KEY aggregation falls under this + # rule — the live SAVE_KEY write still uses the unchanged stub. + # Counter lives in a Dictionary so the lambda's write propagates out + # under GDScript's closure capture semantics. + var prior_writer: Callable = _gs._local_storage_writer + var counters := {"rejected_oversized_writes": 0} + _gs._local_storage_writer = func(key: String, payload: String) -> bool: + if key == GameState.BACKUP_STORAGE_KEY: + var parsed: Variant = JSON.parse_string(payload) + if parsed is Dictionary and (parsed as Dictionary).size() > int(GameState.MAX_BACKUPS): + counters["rejected_oversized_writes"] += 1 + return false + _ls[key] = payload + return true + _gs.save_game(_valid_state(99)) + var new_key: String = _gs.backup_save() + _gs._local_storage_writer = prior_writer + assert_true(not new_key.is_empty(), "backup_save returns a retained key after the in-memory prune") + assert_eq(int(counters["rejected_oversized_writes"]), 0, "the writer is never asked to store more than MAX_BACKUPS entries") + assert_true(_ls.has(GameState.BACKUP_STORAGE_KEY), "the writer accepted the trimmed payload and stored it") + var stored_payload: String = String(_ls[GameState.BACKUP_STORAGE_KEY]) + var pruned_backups := _parsed_backups() + assert_eq(pruned_backups.size(), int(GameState.MAX_BACKUPS), "BACKUP_STORAGE_KEY holds exactly MAX_BACKUPS entries") + assert_true(pruned_backups.has(new_key), "the newly created backup is retained after pruning") + assert_false(stored_payload == pre_snapshot_payload, "the trimmed payload differs from the prior five-snapshot payload") + var names: Array[String] = _gs.list_backups() + if not assert_eq(names.size(), int(GameState.MAX_BACKUPS), "list_backups contains exactly the newest five snapshots"): + return + assert_eq(names[0], new_key, "list_backups places the new snapshot first (newest-first)") + var restored: String = _gs.restore_backup() + assert_eq(restored, new_key, "restore_backup recovers the new snapshot") + var loaded: Dictionary = _gs.load_game() + assert_eq(int(loaded.get("tick", -1)), 99, "restored save carries the new tick") + +# 10) If the final retained payload itself cannot be written, the +# previously stored backups must remain unchanged — no "delete some +# entries first to make space" fallback (issue #421). +func flow_backup_save_preserves_existing_when_trimmed_payload_fails() -> void: + _ls.clear() + _gs._backup_counter = 0 + for i in 5: + _gs.save_game(_valid_state(i + 1)) + _gs.backup_save() + var pre_count: int = _parsed_backups().size() + var pre_payload: String = String(_ls.get(GameState.BACKUP_STORAGE_KEY, "")) + # Writer that rejects every aggregated backup write, simulating a + # persistent quota failure even for the trimmed five-snapshot + # payload. The live SAVE_KEY write still uses the unchanged stub + # so save_game() can run to completion. + var prior_writer: Callable = _gs._local_storage_writer + var counters := {"store_attempts": 0} + _gs._local_storage_writer = func(key: String, payload: String) -> bool: + if key == GameState.BACKUP_STORAGE_KEY: + counters["store_attempts"] += 1 + return false + _ls[key] = payload + return true + _gs.save_game(_valid_state(123)) + var new_key: String = _gs.backup_save() + _gs._local_storage_writer = prior_writer + assert_eq(new_key, "", "backup_save returns empty when the trimmed payload cannot be written") + assert_eq(int(counters["store_attempts"]), 1, "backup_save attempts exactly one BACKUP_STORAGE_KEY write per call") + assert_true(_ls.has(GameState.BACKUP_STORAGE_KEY), "previously stored backups remain under BACKUP_STORAGE_KEY") + assert_eq(String(_ls[GameState.BACKUP_STORAGE_KEY]), pre_payload, "previously stored backup payload is byte-identical") + assert_eq(_parsed_backups().size(), pre_count, "previously stored backup count is unchanged") + assert_eq(int(_gs.list_backups().size()), pre_count, "list_backups still enumerates the original five snapshots") + # The post-failure save is untouched by backup_save; load_game + # reads SAVE_KEY and returns the just-saved (trimmed-write-failure) + # state, not any backup. + var loaded: Dictionary = _gs.load_game() + assert_eq(int(loaded.get("tick", -1)), 123, "SAVE_KEY still holds the post-failure live save") + # ── Helpers ────────────────────────────────────────────────────────────────── func _valid_state(tick: int) -> Dictionary: