MagicBeanDev/Longshot
MagicBeanDev/Longshot
Watch
74
95
Fork
You've already forked Longshot
 98
Code Issues 231 Pull requests 275 Projects Releases 9 Packages Wiki Activity Actions 797

fix(saves): a copy that fails part way leaves the target's save as it was #277

Merged
HumanBean merged 2 commits from jarynclouatre/Longshot:fix/save-copy-staged into main 2026-11-14 00:43:00 +00:00
Conversation 3 Commits 2 Files changed 3 +255 -8
jarynclouatre commented 2026-11-13 19:08:13 +00:00
Contributor

Why

A save import or export that fails part way (a full disk, a file that can't be written) left the target's save broken. copy_saves in scripts/oot_saves.py took the backup, deleted the target's slot_* folders and then copied the new save in. When that copy failed, the old slots were already gone and half of the new save was left in their place; the game then saw the partial save, and the only way back was copying the backup in by hand.

What changed

  • saves (scripts/oot_saves.py, copy_saves): the backup is taken as before. The new save is then copied whole into a hidden folder inside the target (.<name>-incoming-*; beside it when the target doesn't exist yet). Only after that copy worked does the new swap_in move the target's old slots and loose files aside and the new ones in. These are renames on one disk; if one fails, the moves are undone, so the target has either all of its old save or all of the new one. Each move back is tried on its own, so one that fails too (a file in a new slot held open without delete sharing, on Windows) does not stop the others; the old entries that can't go back stay in the kept .<name>-old-* folder, and the error names them, that folder and the backup. Staging inside the target keeps the renames on one disk when the save folder links to another disk (a save moved to an SD card) and works when the folder above it can't be written, as the old code did. --keep-settings is unchanged: option.sav isn't in the copy and stays where it is.
  • saves (remove_tree): after a good swap the old entries' folder is deleted with a handler that clears a read-only bit and tries again (Windows refuses to delete a read-only file, which left the folder in the target, where save_problems then refuses the save). A folder that still can't be deleted is named after the copy, to delete by hand; the copy itself counts.
  • tests (tests/test_oot_saves.py): import and export, each with a slot copy that writes part of a slot and then fails with ENOSPC, an account.sav copy that fails, and the last rename into the target failing. Each time the target must be byte for byte as before with nothing left beside or in it, and the same copy must work afterwards. Another test copies into a save folder that links to a folder on another disk (/dev/shm, when it is another disk), one that links on the same disk, and one whose parent folder is read-only. A forward failure followed by a failing move back (mocked, and on Windows also a real file held open with Python's open, which doesn't share delete): every other old entry is back in the target, the old slot_00 is in the kept folder, and the error names it and the backup. A read-only file in an old slot (stat.S_IREAD; real on Windows), and deletes refused once and for good (mocked os.unlink).
  • docs (docs/saves-script.md): one line in "Every copy" for the staged copy.

Flags and settings

None.

Measurements

main: d9648863 (2026-11-13)
this PR: fix/save-copy-staged @ 5cde8d6e, rebased onto main 514591a3 as 2968265d after the runs (then 1fce28d5, the commit message reworded; 6ea909e4 adds the review's two fixes on top, measured below) (main's 11 changed files since d9648863 don't include the save script, its test or its doc; the save tests pass again at 2968265d)
Hardware: none used (game-free checks on a Linux x86-64 PC, Python 3.14.4)
Scene: none, the game was not run
Method: python3 -I -B tests/run_all.py, two runs each, interleaved (PR, main, PR, main); both runs of a side gave the same counts. This PR's tests/test_oot_saves.py also run against main's scripts/oot_saves.py.
How to reproduce: the commands above in a checkout of each commit; for the second, copy this PR's tests/test_oot_saves.py over main's

main @ d9648863 this PR @ 5cde8d6e
tests/run_all.py (run 1, run 2) 452 passed, 2 failed, 79 skipped (both) 452 passed, 2 failed, 79 skipped (both)
failing checks test_bake_portability.py, test_menu_i18n.py the same two
this PR's test_oot_saves.py 6 subtests fail: after each failed copy the target's slots are gone or partly new 49 tests pass
at 6ea909e4: tests/run_all.py 452 passed, 2 failed (the same two), 79 skipped
at 6ea909e4: test_oot_saves.py 53 tests pass, 1 skipped (the Windows-only held-open case)
Windows 11 (10.0.26320), python.org Python 3.14.10 embeddable: the four review cases at 1fce28d5: 2 errors (the move back raises, the held-open one with WinError 5), 3 failures (.<name>-old-* left in the target) at 6ea909e4: 4 pass

test_bake_portability.py fails here for a missing Python module (zstandard), on main as well. test_menu_i18n.py fails on main itself: a pt_BR hint (the testing tools' row) is no longer a text of vk_menu.cpp.

Testing

Tested: the game-free suite on a Linux PC, as in Measurements, including a real copy across two disks (/tmp and /dev/shm) through a linked save folder. On Windows 11 (native, Python 3.14.10): python -I -B tests/test_oot_saves.py -v Copy.test_rollback_that_fails_too Copy.test_rollback_that_fails_too_with_a_file_held_open Copy.test_read_only_file_in_the_old_save Copy.test_old_save_that_cannot_be_deleted, against 1fce28d5's script and this PR's.

Not tested: the rest of test_oot_saves.py on Windows (its folder discovery is the Linux one, RUN_ALL_SKIP), and a copy into a real game folder on Windows. Every rename stays inside the target folder, so on one volume; a running game is refused before any of it, as before.

Needs other testers: None.

Screenshots

Nothing on screen.

Risk and rollback

A copy now needs room for the new save on the target's disk before the old one is moved aside. Before, the old slots were deleted first, so a nearly full disk that held the new save only after that deletion now fails with the target unchanged. If the script is killed in the middle, a hidden .<name>-incoming-* or .<name>-old-* folder can be left in the save folder; the game reads only slot_* and the two .sav files, and the next copy's backup carries it along. The save checks (save_problems, run on the source of import hookshot and by the launcher's Validate) call any other name "not a save file", so an import from a Hookshot folder left that way is refused, naming the folder, until it is deleted. A move back that fails leaves the target partly new on purpose, with the old entries in the kept folder and the error saying where they and the backup are. Reverting the commits puts the old order back.

Reviewer notes

  • Found reading the save script's code, not from a player's report.

Issues

None.

Checklist

For everyone

  • One topic.
  • The title is a Conventional Commit (type(scope): description).
  • Rebased on current main.
  • python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
  • Tested in the game, or what wasn't is named under Testing.
  • Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
  • Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported.
  • No game files, dumps, build output or generated files.
  • third_party/ untouched (a new folder with a PROVENANCE.md instead).
  • The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

  • A game-free test added for the fix, failing on main and passing here.
  • A check under tests/ that reads changed source updated in this PR, not weakened.
  • Docs updated.
## Why A save import or export that fails part way (a full disk, a file that can't be written) left the target's save broken. `copy_saves` in `scripts/oot_saves.py` took the backup, deleted the target's `slot_*` folders and then copied the new save in. When that copy failed, the old slots were already gone and half of the new save was left in their place; the game then saw the partial save, and the only way back was copying the backup in by hand. ## What changed - saves (`scripts/oot_saves.py`, `copy_saves`): the backup is taken as before. The new save is then copied whole into a hidden folder inside the target (`.<name>-incoming-*`; beside it when the target doesn't exist yet). Only after that copy worked does the new `swap_in` move the target's old slots and loose files aside and the new ones in. These are renames on one disk; if one fails, the moves are undone, so the target has either all of its old save or all of the new one. Each move back is tried on its own, so one that fails too (a file in a new slot held open without delete sharing, on Windows) does not stop the others; the old entries that can't go back stay in the kept `.<name>-old-*` folder, and the error names them, that folder and the backup. Staging inside the target keeps the renames on one disk when the save folder links to another disk (a save moved to an SD card) and works when the folder above it can't be written, as the old code did. `--keep-settings` is unchanged: `option.sav` isn't in the copy and stays where it is. - saves (`remove_tree`): after a good swap the old entries' folder is deleted with a handler that clears a read-only bit and tries again (Windows refuses to delete a read-only file, which left the folder in the target, where `save_problems` then refuses the save). A folder that still can't be deleted is named after the copy, to delete by hand; the copy itself counts. - tests (`tests/test_oot_saves.py`): import and export, each with a slot copy that writes part of a slot and then fails with ENOSPC, an `account.sav` copy that fails, and the last rename into the target failing. Each time the target must be byte for byte as before with nothing left beside or in it, and the same copy must work afterwards. Another test copies into a save folder that links to a folder on another disk (`/dev/shm`, when it is another disk), one that links on the same disk, and one whose parent folder is read-only. A forward failure followed by a failing move back (mocked, and on Windows also a real file held open with Python's `open`, which doesn't share delete): every other old entry is back in the target, the old `slot_00` is in the kept folder, and the error names it and the backup. A read-only file in an old slot (`stat.S_IREAD`; real on Windows), and deletes refused once and for good (mocked `os.unlink`). - docs (`docs/saves-script.md`): one line in "Every copy" for the staged copy. ## Flags and settings None. ## Measurements main: d9648863 (2026-11-13) this PR: fix/save-copy-staged @ 5cde8d6e, rebased onto main 514591a3 as 2968265d after the runs (then 1fce28d5, the commit message reworded; 6ea909e4 adds the review's two fixes on top, measured below) (main's 11 changed files since d9648863 don't include the save script, its test or its doc; the save tests pass again at 2968265d) Hardware: none used (game-free checks on a Linux x86-64 PC, Python 3.14.4) Scene: none, the game was not run Method: `python3 -I -B tests/run_all.py`, two runs each, interleaved (PR, main, PR, main); both runs of a side gave the same counts. This PR's `tests/test_oot_saves.py` also run against main's `scripts/oot_saves.py`. How to reproduce: the commands above in a checkout of each commit; for the second, copy this PR's `tests/test_oot_saves.py` over main's | | main @ d9648863 | this PR @ 5cde8d6e | |---|---|---| | `tests/run_all.py` (run 1, run 2) | 452 passed, 2 failed, 79 skipped (both) | 452 passed, 2 failed, 79 skipped (both) | | failing checks | `test_bake_portability.py`, `test_menu_i18n.py` | the same two | | this PR's `test_oot_saves.py` | 6 subtests fail: after each failed copy the target's slots are gone or partly new | 49 tests pass | | at 6ea909e4: `tests/run_all.py` | | 452 passed, 2 failed (the same two), 79 skipped | | at 6ea909e4: `test_oot_saves.py` | | 53 tests pass, 1 skipped (the Windows-only held-open case) | | Windows 11 (10.0.26320), python.org Python 3.14.10 embeddable: the four review cases | at 1fce28d5: 2 errors (the move back raises, the held-open one with WinError 5), 3 failures (`.<name>-old-*` left in the target) | at 6ea909e4: 4 pass | `test_bake_portability.py` fails here for a missing Python module (zstandard), on main as well. `test_menu_i18n.py` fails on main itself: a pt_BR hint (the testing tools' row) is no longer a text of `vk_menu.cpp`. ## Testing **Tested:** the game-free suite on a Linux PC, as in Measurements, including a real copy across two disks (`/tmp` and `/dev/shm`) through a linked save folder. On Windows 11 (native, Python 3.14.10): `python -I -B tests/test_oot_saves.py -v Copy.test_rollback_that_fails_too Copy.test_rollback_that_fails_too_with_a_file_held_open Copy.test_read_only_file_in_the_old_save Copy.test_old_save_that_cannot_be_deleted`, against 1fce28d5's script and this PR's. **Not tested:** the rest of `test_oot_saves.py` on Windows (its folder discovery is the Linux one, `RUN_ALL_SKIP`), and a copy into a real game folder on Windows. Every rename stays inside the target folder, so on one volume; a running game is refused before any of it, as before. **Needs other testers:** None. ## Screenshots Nothing on screen. ## Risk and rollback A copy now needs room for the new save on the target's disk before the old one is moved aside. Before, the old slots were deleted first, so a nearly full disk that held the new save only after that deletion now fails with the target unchanged. If the script is killed in the middle, a hidden `.<name>-incoming-*` or `.<name>-old-*` folder can be left in the save folder; the game reads only `slot_*` and the two `.sav` files, and the next copy's backup carries it along. The save checks (`save_problems`, run on the source of `import hookshot` and by the launcher's Validate) call any other name "not a save file", so an import from a Hookshot folder left that way is refused, naming the folder, until it is deleted. A move back that fails leaves the target partly new on purpose, with the old entries in the kept folder and the error saying where they and the backup are. Reverting the commits puts the old order back. ## Reviewer notes - Found reading the save script's code, not from a player's report. ## Issues None. ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [x] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] A game-free test added for the fix, failing on `main` and passing here. - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [x] Docs updated.
jarynclouatre force-pushed fix/save-copy-staged from 2968265ddf
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
CI / Conventional Commit title (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
Details
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Details
Third-party parts / As their notes say (pull_request) Has been cancelled
Details
Third-party parts / The same as upstream (pull_request) Has been cancelled
Details
to 1fce28d5da
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
CI / Conventional Commit title (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
Details
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Details
Third-party parts / As their notes say (pull_request) Has been cancelled
Details
Third-party parts / The same as upstream (pull_request) Has been cancelled
Details
2026-11-13 19:09:46 +00:00 Compare
jarynclouatre changed title from WIP: fix(saves): a copy that fails part way leaves the target's save as it was to fix(saves): a copy that fails part way leaves the target's save as it was 2026-11-13 19:09:46 +00:00
monsterlag requested changes 2026-11-13 19:18:36 +00:00
Dismissed
monsterlag left a comment

Requesting changes at 1fce28d5da after native Windows Python 3.14.7 checks using authored files only.

The primary improvement works: two main/PR pairs of direct copy_saves checks cover normal replacement, keep-settings, a new target, partial-slot ENOSPC, loose-file ENOSPC, a final rename failure and a native Windows target handle without FILE_SHARE_DELETE. The PR passes all 14 scenarios. Main's partial slot/loose-file failures leave the target changed. No real saves, game artifacts, whole filesystem fill or ACL changes were used.

Two further Windows checks expose gaps:

  1. [P1] A rollback error prevents all remaining old entries from being restored (scripts/oot_saves.py:439). After slot_00 is placed, hold its progress.sav with CreateFileW sharing READ|WRITE but not DELETE; then inject a failure placing option.sav, as in the PR's forward-rename failure test. The reverse move of slot_00 raises WinError 5. Execution never reaches the loop restoring moved, so the active target contains only the new slot_00, with the old account/options/slots hidden in .target-old-*. The backup is intact, but the target is neither the whole old save nor the whole new one. A temporary lock should not stop restoration of every independent entry. Make rollback best-effort across all entries, preserve recovery material when it cannot finish, and report the rollback failure/recovery paths explicitly. Add a two-failure regression; the current test covers only a forward failure followed by fully successful rollback.

  2. [P2] Successful cleanup hides a read-only-file failure (scripts/oot_saves.py:444). Mark just old slot_02/progress.sav read-only, then copy normally. Backup and every rename work, but rmtree silently leaves .target-old-/slot_02/progress.sav inside the target while copy_saves prints Done. save_problems(target) then reports '.target-old- is not a save file', so that folder can no longer be imported as a Hookshot save without manual repair. Handle read-only cleanup or report an incomplete cleanup and its location; add a native Windows authored regression. Unlike the documented killed-process case, this occurs on an ordinary successful return.

I did not run the Linux-discovery suite on Windows, where it is declared unsupported. This review exercises the portable production copy function directly. No independent full-suite or power-loss claim is made.

Requesting changes at 1fce28d5dafd6efe160c176100d574fd0a606302 after native Windows Python 3.14.7 checks using authored files only. The primary improvement works: two main/PR pairs of direct copy_saves checks cover normal replacement, keep-settings, a new target, partial-slot ENOSPC, loose-file ENOSPC, a final rename failure and a native Windows target handle without FILE_SHARE_DELETE. The PR passes all 14 scenarios. Main's partial slot/loose-file failures leave the target changed. No real saves, game artifacts, whole filesystem fill or ACL changes were used. Two further Windows checks expose gaps: 1. [P1] A rollback error prevents all remaining old entries from being restored (scripts/oot_saves.py:439). After slot_00 is placed, hold its progress.sav with CreateFileW sharing READ|WRITE but not DELETE; then inject a failure placing option.sav, as in the PR's forward-rename failure test. The reverse move of slot_00 raises WinError 5. Execution never reaches the loop restoring moved, so the active target contains only the new slot_00, with the old account/options/slots hidden in .target-old-*. The backup is intact, but the target is neither the whole old save nor the whole new one. A temporary lock should not stop restoration of every independent entry. Make rollback best-effort across all entries, preserve recovery material when it cannot finish, and report the rollback failure/recovery paths explicitly. Add a two-failure regression; the current test covers only a forward failure followed by fully successful rollback. 2. [P2] Successful cleanup hides a read-only-file failure (scripts/oot_saves.py:444). Mark just old slot_02/progress.sav read-only, then copy normally. Backup and every rename work, but rmtree silently leaves .target-old-*/slot_02/progress.sav inside the target while copy_saves prints Done. save_problems(target) then reports '.target-old-* is not a save file', so that folder can no longer be imported as a Hookshot save without manual repair. Handle read-only cleanup or report an incomplete cleanup and its location; add a native Windows authored regression. Unlike the documented killed-process case, this occurs on an ordinary successful return. I did not run the Linux-discovery suite on Windows, where it is declared unsupported. This review exercises the portable production copy function directly. No independent full-suite or power-loss claim is made.
scripts/oot_saves.py Outdated
@ -416,0 +436,4 @@
placed.append(n)
except BaseException:
for n in reversed(placed):
os.replace(os.path.join(dst, n), os.path.join(incoming, n))
monsterlag commented 2026-11-13 19:18:36 +00:00
First-time contributor

[P1] A second failure here aborts rollback before any of moved is restored. Reproduced natively on Windows: after incoming slot_00 is placed, open its progress.sav without FILE_SHARE_DELETE; fail the later option.sav placement. This reverse rename gets WinError 5, leaving only new slot_00 active while the old account/options/slots remain hidden in aside. The backup is intact, but the target is partial. Attempt the independent restorations even after a rollback move fails, preserve recovery material, and report the incomplete rollback and recovery paths. Please add a forward-failure plus rollback-failure regression.

[P1] A second failure here aborts rollback before any of `moved` is restored. Reproduced natively on Windows: after incoming slot_00 is placed, open its progress.sav without FILE_SHARE_DELETE; fail the later option.sav placement. This reverse rename gets WinError 5, leaving only new slot_00 active while the old account/options/slots remain hidden in aside. The backup is intact, but the target is partial. Attempt the independent restorations even after a rollback move fails, preserve recovery material, and report the incomplete rollback and recovery paths. Please add a forward-failure plus rollback-failure regression.
scripts/oot_saves.py Outdated
@ -416,0 +441,4 @@
os.replace(os.path.join(aside, n), os.path.join(dst, n))
os.rmdir(aside)
raise
shutil.rmtree(aside, ignore_errors=True)
monsterlag commented 2026-11-13 19:18:36 +00:00
First-time contributor

[P2] ignore_errors=True makes an ordinary successful copy leave an invalid save directory on Windows. Reproduced with only old slot_02/progress.sav marked read-only: all renames succeed, but this cleanup leaves .target-old-*/slot_02/progress.sav and the script prints Done. save_problems(target) then rejects the hidden directory, blocking a later Hookshot import. Handle read-only-file removal or report cleanup failure and its location rather than silently claiming completion; please add an authored Windows case.

[P2] `ignore_errors=True` makes an ordinary successful copy leave an invalid save directory on Windows. Reproduced with only old slot_02/progress.sav marked read-only: all renames succeed, but this cleanup leaves `.target-old-*/slot_02/progress.sav` and the script prints Done. `save_problems(target)` then rejects the hidden directory, blocking a later Hookshot import. Handle read-only-file removal or report cleanup failure and its location rather than silently claiming completion; please add an authored Windows case.
fix(saves): a move back that fails leaves the rest put back, and read-only old files are deleted
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
Details
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Details
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Details
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
Details
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
Website / Website on Cloudflare (pull_request_target) Has been skipped
Details
6ea909e46d 
When a move into the target failed and then one of the moves back failed
too (a file held open without delete sharing on Windows), the rollback
stopped there and left the old save hidden in the old-entries folder. Every
entry is now tried on its own; the old ones that can't go back stay in that
folder, which is kept, and the error names them, the folder and the backup.

After a good swap the old entries' folder was deleted with errors ignored, so
a read-only file in the old save (Windows refuses to delete those) left it in
the target, where save_problems then refuses the save. A refused delete is now
tried again with the read-only bit cleared, and a folder that still can't be
deleted is named, to delete by hand.
jarynclouatre commented 2026-11-13 20:15:14 +00:00
Author
Contributor

Thanks, both reproduced and fixed in 6ea909e4.

  1. Rollback: each move back is now tried on its own, so a failed one no longer stops the rest. An old entry that can't go back (its name still taken by the new one that couldn't move out, or its own move failing) stays in the .<name>-old-* folder, which is then kept, and the script fails with the names, that folder and the backup's path. When everything went back the error is the original one, as before. Regression: test_rollback_that_fails_too (forward failure on option.sav, then the move back of the new slot_00 refused; mocked, runs everywhere) and test_rollback_that_fails_too_with_a_file_held_open (Windows only: the new slot_00/progress.sav opened with Python's open, which doesn't share delete, before the forward failure). Both check that account.sav, option.sav and slot_01 are the old ones again, the old slot_00 is in the kept folder, and the error names it and the backup.

  2. Cleanup: the old entries' folder is deleted by remove_tree, whose handler clears the read-only bit and tries again (onexc on 3.12+, onerror before). If something is still left, the script says the copy worked and names the folder to delete by hand instead of a plain Done. Regression: test_read_only_file_in_the_old_save (old slot_01/progress.sav set to stat.S_IREAD; afterwards the target holds only the new save and save_problems finds nothing) and test_old_save_that_cannot_be_deleted (deletes refused once, then always, through a mocked os.unlink).

Native Windows 11 (10.0.26320), python.org Python 3.14.10, those four tests only (the rest of the file uses the Linux discovery): at 1fce28d5, 2 errors (the held-open one raises WinError 5 from the move back) and 3 failures (.<name>-old-* left in the target); at 6ea909e4, all 4 pass. Linux: test_oot_saves.py 53 pass, 1 skipped (the Windows-only one); tests/run_all.py 452 passed, 2 failed (test_bake_portability.py, test_menu_i18n.py, both failing on main), 79 skipped. The description's What changed, Measurements, Testing and Risk are updated.

Thanks, both reproduced and fixed in 6ea909e4. 1. Rollback: each move back is now tried on its own, so a failed one no longer stops the rest. An old entry that can't go back (its name still taken by the new one that couldn't move out, or its own move failing) stays in the `.<name>-old-*` folder, which is then kept, and the script fails with the names, that folder and the backup's path. When everything went back the error is the original one, as before. Regression: `test_rollback_that_fails_too` (forward failure on `option.sav`, then the move back of the new `slot_00` refused; mocked, runs everywhere) and `test_rollback_that_fails_too_with_a_file_held_open` (Windows only: the new `slot_00/progress.sav` opened with Python's `open`, which doesn't share delete, before the forward failure). Both check that `account.sav`, `option.sav` and `slot_01` are the old ones again, the old `slot_00` is in the kept folder, and the error names it and the backup. 2. Cleanup: the old entries' folder is deleted by `remove_tree`, whose handler clears the read-only bit and tries again (`onexc` on 3.12+, `onerror` before). If something is still left, the script says the copy worked and names the folder to delete by hand instead of a plain Done. Regression: `test_read_only_file_in_the_old_save` (old `slot_01/progress.sav` set to `stat.S_IREAD`; afterwards the target holds only the new save and `save_problems` finds nothing) and `test_old_save_that_cannot_be_deleted` (deletes refused once, then always, through a mocked `os.unlink`). Native Windows 11 (10.0.26320), python.org Python 3.14.10, those four tests only (the rest of the file uses the Linux discovery): at 1fce28d5, 2 errors (the held-open one raises WinError 5 from the move back) and 3 failures (`.<name>-old-*` left in the target); at 6ea909e4, all 4 pass. Linux: `test_oot_saves.py` 53 pass, 1 skipped (the Windows-only one); `tests/run_all.py` 452 passed, 2 failed (`test_bake_portability.py`, `test_menu_i18n.py`, both failing on main), 79 skipped. The description's What changed, Measurements, Testing and Risk are updated.
monsterlag approved these changes 2026-11-13 20:47:07 +00:00
monsterlag left a comment

Approved follow-up6ea909e4 after inspecting the recovery/cleanup change and independently exercising the four portable follow-up regression methods on native Windows Python3.14.7.

Two sequential interleaved original-head1fce28d5 / follow-up6ea909e4 pairs, using the exact same new tests against each production module:

original1fce28d5 follow-up6ea909e4
four methods, run1 3 failures, 2 errors all4pass, no skips
four methods, run2 3 failures, 2 errors all4pass, no skips

This includes a real native open file without delete sharing, not only a mocked rollback error. The tests verify independent old entries are restored, the blocked old slot remains recoverable, the backup is intact and its recovery paths appear in the error. The read-only-file case now removes the stale folder and passes save_problems. A persistent cleanup failure is explicitly reported instead of being silently treated as a clean result.

My two prior blocking findings are resolved. Persistent I/O failure still prevents a wholly restored target, as expected; the new behavior makes recovery best-effort and reports what remains rather than claiming guaranteed atomicity. I did not run the Linux-discovery suite on Windows, perform a power-loss test or repeat the full Longshot suite. All data in this independent check was authored fixture content; no real save or installed player changed.

Approved follow-up6ea909e4 after inspecting the recovery/cleanup change and independently exercising the four portable follow-up regression methods on native Windows Python3.14.7. Two sequential interleaved original-head1fce28d5 / follow-up6ea909e4 pairs, using the exact same new tests against each production module: | | original1fce28d5 | follow-up6ea909e4 | |---|---|---| | four methods, run1 | 3 failures, 2 errors | all4pass, no skips | | four methods, run2 | 3 failures, 2 errors | all4pass, no skips | This includes a real native open file without delete sharing, not only a mocked rollback error. The tests verify independent old entries are restored, the blocked old slot remains recoverable, the backup is intact and its recovery paths appear in the error. The read-only-file case now removes the stale folder and passes save_problems. A persistent cleanup failure is explicitly reported instead of being silently treated as a clean result. My two prior blocking findings are resolved. Persistent I/O failure still prevents a wholly restored target, as expected; the new behavior makes recovery best-effort and reports what remains rather than claiming guaranteed atomicity. I did not run the Linux-discovery suite on Windows, perform a power-loss test or repeat the full Longshot suite. All data in this independent check was authored fixture content; no real save or installed player changed.
HumanBean deleted branch fix/save-copy-staged 2026-11-14 00:43:00 +00:00
HumanBean added this to the v3.0.0 milestone 2026-11-14 00:43:03 +00:00
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
Details
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Details
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Details
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
Details
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
Website / Website on Cloudflare (pull_request_target) Has been skipped
Details
Some workflows are waiting to be reviewed.
Sign in to join this conversation.
Reviewers
No reviewers
monsterlag
Labels
Clear labels   
area/accessibility
Barriers for people with disabilities

  
area/audio
Sound and music

  
area/input
Controllers, keyboard, mouse

  
area/install
Installing, updating, building from your game

  
area/menu
In-game menu and settings

  
area/renderer
Vulkan renderer, shaders, effects

  
area/saves
Save files

  
area/upscaling
DLSS, FSR, frame generation

  
beanstalk: approved @ 1697603
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ 2c5a62e
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ 4fd82b7
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ 64d6d09
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ 6f91fae
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ 70a78f9
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ 8d361dc
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ 9d8356e
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ abf2227
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ d82f202
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ e5f1e40
Set by Beanstalk for show; the farm's database is what counts

  
beanstalk: approved @ ee8ab5f
Set by Beanstalk for show; the farm's database is what counts

  
ci: approved
A maintainer approved CircleCI for this fork PR (sticky; removed after 14 quiet days)

  
good first issue
A good start for a new contributor

  
gpu/amd
AMD GPUs

  
gpu/igpu
Integrated GPUs

  
gpu/intel
Intel GPUs

  
gpu/nvidia
NVIDIA GPUs

  
help wanted
Extra hands or knowledge wanted

  
kind
bug
 Something doesn't work as it should

  
kind
chore
 Build, CI, dependencies, tidying

  
kind
docs
 Documentation

  
kind
feature
 A new feature or an improvement

  
kind
performance
 Frame rate, frame time, loading, memory

  
kind
question
 A question rather than a change

  
needs measurements
Waiting for before/after measurements

  
platform/linux
Affects Linux (a change for every system carries both Windows and Linux)

  
platform/macos
macOS (Apple silicon or Intel Macs)

  
platform/steam-deck
Steam Deck

  
platform/windows
Affects Windows (a change for every system carries both Windows and Linux)

  
preview
Maintainers: deploy a preview of this PR's website (read its changes first). Removed after the run.

  
priority
critical
 Crashes, data loss, or blocks most players

  
priority
high
 Important: next in line

  
priority
low
 Nice to have

  
regression
Worked in an earlier build, broken in a newer one: highest priority, above other bugs

  
status
blocked
 Waits on something else first

  
status
confirmed
 Reproduced or agreed on

  
status
duplicate
 Already reported or proposed

  
status
invalid
 Not a problem in this project

  
status
needs-info
 Waiting for more information from the reporter

  
status
wontfix
 Will not be worked on

  
testing wanted
Wanted: a test on other hardware (posts a test request in #test-requests)

  
ci: approved
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
beanstalk: approved @ 1697603
beanstalk: approved @ 2c5a62e
beanstalk: approved @ 4fd82b7
beanstalk: approved @ 64d6d09
beanstalk: approved @ 6f91fae
beanstalk: approved @ 70a78f9
beanstalk: approved @ 8d361dc
beanstalk: approved @ 9d8356e
beanstalk: approved @ abf2227
beanstalk: approved @ d82f202
beanstalk: approved @ e5f1e40
beanstalk: approved @ ee8ab5f
ci: approved
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/macos
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
regression
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
ci: approved
Milestone
Clear milestone
No items
No milestone
v3.0.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
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
MagicBeanDev/Longshot!277
No description provided.