fix(web-backup): prune candidate dictionary before writing BACKUP_STORAGE_KEY (issue #421)

[itsmiso-ai]

Closes #421

Prunes the candidate backup dictionary in memory before writing BACKUP_STORAGE_KEY so the aggregated web-backup payload never carries more than MAX_BACKUPS entries.

The old _backup_save_local_storage() order inserted the new snapshot, attempted to store the full MAX_BACKUPS + 1 set, and only then pruned. Once five snapshots were already retained, every rotation first required storage for six. When the browser's remaining localStorage capacity held the intended five-snapshot set but not the temporary six-snapshot set, the first _store_web_backups() call failed, the function returned empty, and main.gd::_on_startup_new_game() proceeded to reset the colony without a recoverable backup.

Fix:

• New _prune_backups_in_memory() helper uses the existing newest-first numeric ordering (same as _sorted_backup_names() / _prune_old_backups()).
• _backup_save_local_storage() calls it before _store_web_backups().
• If the final retained payload still cannot be written, backup_save() returns "" and leaves the previously stored backups byte-identical (no delete-some-entries-to-make-space fallback).
• _prune_old_backups() web branch reuses the helper for a single source of truth.

Tests added in tests/test_web_backup.gd:

• flow_backup_pruning_avoids_quota_blocked_rotation(): seeds five existing snapshots, swaps in a writer that rejects only the six-snapshot payload, asserts backup_save returns a retained key, the writer is never asked to store more than MAX_BACKUPS entries, list_backups contains exactly the newest five, and restore_backup recovers the new snapshot.
• flow_backup_save_preserves_existing_when_trimmed_payload_fails(): writer rejects every aggregated write; asserts the previous BACKUP_STORAGE_KEY payload is byte-identical, the snapshot count is unchanged, list_backups still enumerates the original five, and the live SAVE_KEY is untouched.

Verified locally with the headless matrix used in CI: all 32 tests/test_*.gd suites pass (44 assertions in test_web_backup.gd, including the 18 new ones). Reverting the in-memory prune before re-running the suite reproduces the original write-before-prune failures, confirming regression coverage.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M3.1-Flash-Preview (anthropic) — primary route · pr-reviewer-action v3.4.1

Findings (1 info)

Severity	Location	Finding
Info	scripts/game_state.gd:853	Since `_prune_backups_in_memory()` mutates and returns the same Dictionary reference, `pruned.size() == backups.size()` remains true after pruning and this web `_prune_old_backups()` path returns without persisting the trimmed set; no current repository caller reaches it, so the defect is non-blocki

Recommendation: approve. No blocking or major issues were found. The rotation path now persists at most MAX_BACKUPS snapshots, and the linked acceptance criteria are directly exercised. There is one non-blocking, currently unreachable aliasing defect in the refactored _prune_old_backups() web branch at scripts/game_state.gd:853.

Change-by-change findings

• scripts/game_state.gd

  • The complete cross-step path is consistent:
    • Producer: reads the current live state from SAVE_KEY.
    • Persisted representation: loads the parsed BACKUP_STORAGE_KEY dictionary, inserts the new backup, and trims it with the existing newest-first numeric ordering.
    • Transport/environment: _store_web_backups() serializes only that pruned candidate before writing through the existing localStorage writer.
    • Consumers: list_backups() and restore_backup() read the same aggregated key and use _sorted_backup_names().
    • Decision: a failed writer result returns "" without invoking any remover; a successful write is followed by the existing read-back check for the new key.
  • Exact call-site searches found only two _store_web_backups() callers, and both supply an already-pruned dictionary, so no sibling caller was missed.
  • Desktop backup behavior remains on its existing copy-and-prune path; localStorage key conventions and save schema are unchanged.

• tests/test_web_backup.gd

  • The quota-rotation regression seeds five backups, rejects any aggregated candidate larger than MAX_BACKUPS, verifies that no oversized write is attempted, and then verifies the retained key, five-entry payload, newest-first result, restore key, and restored tick.
  • The final-write-failure regression verifies the empty return, exactly one backup-key write attempt, byte-identical previous payload, unchanged backup count/list, and unchanged live save.
  • All new assertion calls include the required assertion name, and the reported exact-head CI execution succeeds.

Claims to falsify

1. Claim 1 — held: 2/2 persistence call sites. Both _store_web_backups() callers receive dictionaries capped at MAX_BACKUPS; no additional call sites were found.
2. Claim 2 — held: 3/3 historical stages. The removed implementation inserted the new snapshot, attempted the six-entry payload, and only then called the post-write pruning path.
3. Claim 3 — held: 3/3 failure behaviors. The candidate is a freshly parsed dictionary, a failed write returns immediately, and there is no deletion or fallback write that could alter the previously stored payload.
4. Claim 4 — held: 11/11 assertions. The deterministic quota test covers the retained return value, no oversized write, five retained entries, newest ordering, restoration of the new key, and restoration of tick 99.
5. Claim 5 — held: 7/7 assertions. The failure test covers the empty return, one write attempt, byte-identical backup payload, unchanged count/list, and unchanged live save.

Sources

• scripts/game_state.gd:748-907
• tests/test_web_backup.gd:174-264
• scripts/main.gd:269-284
• tests/test_case.gd
• .github/workflows/test.yml
• .github/actions/load-godot-toolchain/action.yml
• Linked issue PR 421 and the reported CI results for commit 2ec217efbf8bf17a8c798534f4e128fea9975a7a

Standards Compliance

• The change is focused on the two files named by the repository issue convention.
• It does not alter the save version or migration behavior, storage-key convention, desktop retention logic, release process, or UI direction.
• The tests avoid the documented two-argument assert_eq parse-error trap.
• The repository's headless test workflow enumerates tests/test_*.gd, and the exact-head script, macOS, smoke, and export checks all pass.
• No repository-standard violation was found.

Linked Issue Fit

• Ask: met. The candidate dictionary is pruned in memory before BACKUP_STORAGE_KEY is written.
• Deterministic regression test: met. Five snapshots are seeded and a hook writer accepts five but rejects six.
• Successful rotation behavior: met. backup_save retains the new key, listing returns five newest entries, and restore recovers tick 99.
• Final-write failure behavior: met. The function returns an empty string, performs no delete-to-make-space fallback, and preserves the previous payload exactly.
• Regression/scope constraints: met. Existing web, ordering, clear-game, and desktop backup coverage remains green, and neither desktop retention nor storage-key conventions changed.

Tool Harness Findings

Native reads and searches confirmed the save/list/restore implementation, the JavaScriptBridge writer's success handling, every aggregated-write caller, the desktop-only reachability of _prune_old_backups() under current repository callers, the New Game consumer, assertion signatures, and CI suite enumeration. They also exposed the non-blocking Dictionary-aliasing behavior documented in the info finding.