fix(saves): the Linux saves' move leaves a linked folder and its saves alone #2282

Merged

jarynclouatre merged 1 commit from fix/linux-saves-move-links into main

2026-11-11 21:29:29 +00:00

jarynclouatre commented

2026-11-11 13:27:23 +00:00

(Migrated from github.com)

On Linux, the one-time move of the saves to ~/.local/share/hookshot (7eeb91a) can empty a folder outside the game folder and leave the game showing no saves, when user/ holds a symbolic link (saves kept in a synced folder or on another drive).

What happens

saves_dir moves each entry of user/ with a rename, or a copy and delete when the rename fails. A link is treated as the folder it names:

Game on another drive than the home folder (rename fails): the link is copied as a real folder, then delete_tree deletes everything in the folder the link names. That folder is the player's, not Hookshot's. This start still finds the saves, in the copy. The link itself is left in user/, so the next start sees save in both places, keeps the old place for good, and reads the folder it emptied: no saves.
Same drive: the link is renamed as it stands. One written relative to its place (../../sync) names nothing from ~/.local/share/hookshot: no saves, and none can be written.

Also in the same loop: when the new place already has one of the entries, the old place is kept, but the entries looked at before that one have already gone, and the game runs in the old place without them.

The change

A link is made again in the new place, to the folder it names in full, at any depth. delete_tree removes a link without entering it, as nn_fs.c's does since the DeleteDirectoryRecursively fix.
A link to an entry that moves too names it where it will be. A link that names nothing goes as written.
A folder with a link in it is copied rather than renamed, so the links in it are seen.
A user/ that is itself a link is the player's choice of place: used as it is, nothing moved.
The loop looks at every entry before it moves any: with an entry already in the new place, nothing moves.

Windows is not touched (the code is under #ifndef _WIN32).

Tested

tests/test_linux_saves_move.py (new) builds saves_dir and what it calls from hos_main.c as it is, over host/posix/win32_posix.c, and starts it twice on folders of its own: a link written in full, one written relative, one deeper in (user/save/<user id>), one to an entry that moves too, one that names nothing, a linked user/, a new place that has an entry already, and a plain user/; with the game on another drive too (/dev/shm against the temp folder) where the PC has one.

On this change: passes, the other-drive cases included.
On main: 12 of its checks fail in 6 of the cases, among them "the linked folder lost its save" and "start 2 did not use the new place".
python -I -B tests/run_all.py: nothing new fails.

Not tested in a built game: I checked the function as the test builds it (gcc, -D_GNU_SOURCE as nx2/build.py passes), not a full Linux build of the game.

On Linux, the one-time move of the saves to `~/.local/share/hookshot` (7eeb91a) can empty a folder outside the game folder and leave the game showing no saves, when `user/` holds a symbolic link (saves kept in a synced folder or on another drive). ## What happens `saves_dir` moves each entry of `user/` with a rename, or a copy and delete when the rename fails. A link is treated as the folder it names: - **Game on another drive than the home folder** (rename fails): the link is copied as a real folder, then `delete_tree` deletes everything in the folder the link names. That folder is the player's, not Hookshot's. This start still finds the saves, in the copy. The link itself is left in `user/`, so the next start sees `save` in both places, keeps the old place for good, and reads the folder it emptied: no saves. - **Same drive**: the link is renamed as it stands. One written relative to its place (`../../sync`) names nothing from `~/.local/share/hookshot`: no saves, and none can be written. Also in the same loop: when the new place already has one of the entries, the old place is kept, but the entries looked at before that one have already gone, and the game runs in the old place without them. ## The change - A link is made again in the new place, to the folder it names in full, at any depth. `delete_tree` removes a link without entering it, as `nn_fs.c`'s does since the DeleteDirectoryRecursively fix. - A link to an entry that moves too names it where it will be. A link that names nothing goes as written. - A folder with a link in it is copied rather than renamed, so the links in it are seen. - A `user/` that is itself a link is the player's choice of place: used as it is, nothing moved. - The loop looks at every entry before it moves any: with an entry already in the new place, nothing moves. Windows is not touched (the code is under `#ifndef _WIN32`). ## Tested `tests/test_linux_saves_move.py` (new) builds `saves_dir` and what it calls from `hos_main.c` as it is, over `host/posix/win32_posix.c`, and starts it twice on folders of its own: a link written in full, one written relative, one deeper in (`user/save/<user id>`), one to an entry that moves too, one that names nothing, a linked `user/`, a new place that has an entry already, and a plain `user/`; with the game on another drive too (`/dev/shm` against the temp folder) where the PC has one. - On this change: passes, the other-drive cases included. - On `main`: 12 of its checks fail in 6 of the cases, among them "the linked folder lost its save" and "start 2 did not use the new place". - `python -I -B tests/run_all.py`: nothing new fails. Not tested in a built game: I checked the function as the test builds it (gcc, `-D_GNU_SOURCE` as `nx2/build.py` passes), not a full Linux build of the game.

Joey-Magnus (Migrated from github.com) approved these changes

2026-11-11 20:07:36 +00:00

Joey-Magnus (Migrated from github.com) left a comment

This fixes a real data-loss path, and the fix is careful. delete_tree no longer follows a link, so the cross-drive copy can't empty the player's synced folder any more. Links are recreated with their resolved target, and the look-then-move pass stops the half-moved state when the new place already has an entry. I read copy_link, has_link and the reworked saves_dir and found nothing blocking:

has_link and copy_tree test is_link before is_dir, so neither enters a linked folder (no loops, no walking into the player's data). Dangling links are handled too, since is_dir is false for them and delete_tree unlinks them.
The cleanup after a failed copy (delete_tree(to)) is now safe as well. Without the is_link check there, it would have walked into the links copy_link had just made.
realpath into char[MAX_PATH] is fine on Linux because host/posix/windows.h defines MAX_PATH as 4096.
Merged with current main (which has #2283's change to the same start-up code), test_linux_saves_move, test_linux_update_skip and test_oot_saves all pass.

minor host/hos_main.c:272-273,373: move_old is left pointing at saves_dir's stack buffer real after the function returns. Today only saves_dir reaches copy_tree/copy_link, so nothing reads it, but the next caller of copy_tree that meets a link would read a dead stack pointer. Resetting move_old = move_new = NULL before saves_dir returns (or passing them down as parameters) closes that off.

minor (it predates this PR, but this PR's comment now promises otherwise) host/hos_main.c:386-388: in the moving pass, a copy that fails does kept++ but the do … while keeps moving the remaining entries, and the function then returns the old place without them. That's the same split the new first pass was added to prevent, just reached through a failure (a full disk, a read-only file) instead of a collision. Stopping the inner loop once kept is set would at least stop the damage spreading. Fully undoing the earlier moves can wait for a later PR.

coordination: #2317 (the per-user data folder) moves saves_dir and its helpers into host/hos_data.c, and the two branches conflict. Whichever merges second has to carry the link handling over, together with test_linux_saves_move.py, which cuts the functions out of hos_main.c by text and would need to point at the new file. Worth agreeing on the order with @LNDF. The appimage failure on this run is unrelated: the mirror bootstrap timed out.

This fixes a real data-loss path, and the fix is careful. `delete_tree` no longer follows a link, so the cross-drive copy can't empty the player's synced folder any more. Links are recreated with their resolved target, and the look-then-move pass stops the half-moved state when the new place already has an entry. I read `copy_link`, `has_link` and the reworked `saves_dir` and found nothing blocking: - `has_link` and `copy_tree` test `is_link` before `is_dir`, so neither enters a linked folder (no loops, no walking into the player's data). Dangling links are handled too, since `is_dir` is false for them and `delete_tree` unlinks them. - The cleanup after a failed copy (`delete_tree(to)`) is now safe as well. Without the `is_link` check there, it would have walked into the links `copy_link` had just made. - `realpath` into `char[MAX_PATH]` is fine on Linux because `host/posix/windows.h` defines `MAX_PATH` as 4096. - Merged with current main (which has #2283's change to the same start-up code), `test_linux_saves_move`, `test_linux_update_skip` and `test_oot_saves` all pass. **minor** `host/hos_main.c:272-273,373`: `move_old` is left pointing at `saves_dir`'s stack buffer `real` after the function returns. Today only `saves_dir` reaches `copy_tree`/`copy_link`, so nothing reads it, but the next caller of `copy_tree` that meets a link would read a dead stack pointer. Resetting `move_old = move_new = NULL` before `saves_dir` returns (or passing them down as parameters) closes that off. **minor** (it predates this PR, but this PR's comment now promises otherwise) `host/hos_main.c:386-388`: in the moving pass, a copy that fails does `kept++` but the `do … while` keeps moving the remaining entries, and the function then returns the old place without them. That's the same split the new first pass was added to prevent, just reached through a failure (a full disk, a read-only file) instead of a collision. Stopping the inner loop once `kept` is set would at least stop the damage spreading. Fully undoing the earlier moves can wait for a later PR. **coordination**: #2317 (the per-user data folder) moves `saves_dir` and its helpers into `host/hos_data.c`, and the two branches conflict. Whichever merges second has to carry the link handling over, together with `test_linux_saves_move.py`, which cuts the functions out of `hos_main.c` by text and would need to point at the new file. Worth agreeing on the order with @LNDF. The `appimage` failure on this run is unrelated: the mirror bootstrap timed out.

Joey-Magnus (Migrated from github.com) approved these changes

2026-11-11 21:22:46 +00:00

Joey-Magnus (Migrated from github.com) left a comment

This fixes real data loss: delete_tree could empty a linked sync folder. is_link now comes before is_dir in copy_tree, has_link and delete_tree, and the test passes, including the other-drive case. The appimage failure is infrastructure.

Two minor points, both optional:

host/hos_main.c:373 leaves move_old pointing at saves_dir's stack buffer after return. It's harmless today, and a one-line reset fixes it.
:388, which predates this PR: a failed copy does kept++ but keeps moving the other entries, so on a full disk saves can still end up split between the two places.

This should merge before #2317, which moves saves_dir into hos_data.c and should carry the link handling with it.

This fixes real data loss: `delete_tree` could empty a linked sync folder. `is_link` now comes before `is_dir` in `copy_tree`, `has_link` and `delete_tree`, and the test passes, including the other-drive case. The appimage failure is infrastructure. Two minor points, both optional: - `host/hos_main.c:373` leaves `move_old` pointing at `saves_dir`'s stack buffer after return. It's harmless today, and a one-line reset fixes it. - `:388`, which predates this PR: a failed copy does `kept++` but keeps moving the other entries, so on a full disk saves can still end up split between the two places. This should merge before #2317, which moves `saves_dir` into `hos_data.c` and should carry the link handling with it.