MagicBeanDev/Hookshot
Archived
Watch
2
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

fix(saves): a link to an entry that moved first follows it to the new place #2832

Closed
github-sync wants to merge 2 commits from github/pr/2832 into main
pull from: github/pr/2832
merge into: MagicBeanDev:main
Conversation 4 Commits 2 Files changed 3 +24 -10
github-sync commented 2026-11-12 15:49:15 +00:00
Owner

@nullifyshot opened this pull request on GitHub · 2026-11-12 15:48 UTC · original
Merged on GitHub in 9e7764271a

test_linux_saves_move fails now and then on CI's ubuntu part 1/2 ("a link to an entry that moves too: last names another folder", e.g. on #2822, which only touches release scripts), and the same bug can point a player's saves link at a folder that no longer exists after the first start on Linux.

Cause. When user/ moves to $XDG_DATA_HOME/hookshot, copy_link (host/hos_main.c) makes each link again. A link to an entry that moves too is pointed at that entry's new place, but only when realpath resolves it. An absolute link (user/last -> <game>/user/save/<id>) listed after save/ names nothing once save/ has gone. realpath then fails, the link's text is copied unchanged, and the new link names the old place, which is removed at the end of the move. Which entry comes first depends on the file system's directory order, so the check passes on most runners and fails on some.

Fix. The link's text is checked against the folder on the move both as realpath names it and as given, after readlink too. Links written from where they stand and links that name nothing are unchanged.

Test. The case "a link to an entry that moves too" now makes 36 more absolute links to save/<id> (last-a ... last-9), so on any directory order some are listed after save/. Without the fix, those name the old place. The check is Linux only (it skips on Windows), so the run on this PR's ubuntu job is the proof. python -I -B tests/run_all.py on Windows: 419 passed; the 5 that fail (fpx_sticky, bake_portability, input_background, lift_cache, spawn_emit) fail on main on this PC too.

Overlap. #2317 moves this code (copy_link, move_old/move_new) into the per-user data folder work and has the same realpath-only check. Whichever merges second should carry the change across.

Not tested in the game. The Linux move runs once, at a first start with a user/ beside the game, which can't be run on this PC (Windows). Wanted: someone on Linux with a user/ folder holding an absolute symlink to a file in user/save/ to start the game once and check that the link in ~/.local/share/hookshot points inside that folder (readlink).

🤖 Generated with Claude Code

> [@nullifyshot](https://github.com/nullifyshot) opened this pull request on GitHub · 2026-11-12 15:48 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2832) > **Merged on GitHub** in https://git.magicbean.dev/MagicBeanDev/Hookshot/commit/9e7764271a87c8c4cfd4be16250b59dfbe10d62d `test_linux_saves_move` fails now and then on CI's ubuntu part 1/2 ("a link to an entry that moves too: last names another folder", e.g. on #2822, which only touches release scripts), and the same bug can point a player's saves link at a folder that no longer exists after the first start on Linux. **Cause.** When `user/` moves to `$XDG_DATA_HOME/hookshot`, `copy_link` (host/hos_main.c) makes each link again. A link to an entry that moves too is pointed at that entry's new place, but only when `realpath` resolves it. An absolute link (`user/last -> <game>/user/save/<id>`) listed after `save/` names nothing once `save/` has gone. `realpath` then fails, the link's text is copied unchanged, and the new link names the old place, which is removed at the end of the move. Which entry comes first depends on the file system's directory order, so the check passes on most runners and fails on some. **Fix.** The link's text is checked against the folder on the move both as `realpath` names it and as given, after `readlink` too. Links written from where they stand and links that name nothing are unchanged. **Test.** The case "a link to an entry that moves too" now makes 36 more absolute links to `save/<id>` (`last-a` ... `last-9`), so on any directory order some are listed after `save/`. Without the fix, those name the old place. The check is Linux only (it skips on Windows), so the run on this PR's ubuntu job is the proof. `python -I -B tests/run_all.py` on Windows: 419 passed; the 5 that fail (fpx_sticky, bake_portability, input_background, lift_cache, spawn_emit) fail on `main` on this PC too. **Overlap.** #2317 moves this code (`copy_link`, `move_old`/`move_new`) into the per-user data folder work and has the same `realpath`-only check. Whichever merges second should carry the change across. **Not tested in the game.** The Linux move runs once, at a first start with a `user/` beside the game, which can't be run on this PC (Windows). Wanted: someone on Linux with a `user/` folder holding an absolute symlink to a file in `user/save/` to start the game once and check that the link in `~/.local/share/hookshot` points inside that folder (`readlink`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- github-sync:item:2832 -->
copy_link re-pointed a link to an entry of the moving user/ folder only when realpath resolved it. An absolute link listed
after that entry named nothing by then, so its text was copied as it was and the new link named the removed old place.
The directory order is the file system's, so test_linux_saves_move failed on some CI runners. The link's text is now
checked against the folder as given and as realpath names it; the test makes 36 more such links so some come after save/.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
The bug #2832 fixes was found while fixing another one and had no row; the
work list asks for a scored row for every new problem, owned in the PR that
does the work.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
github-sync closed this pull request 2026-11-12 17:59:45 +00:00
github-sync commented 2026-11-12 17:59:46 +00:00
Author
Owner

@hckrman101 reviewed on GitHub · 2026-11-12 17:14 UTC · original

Thanks, this is a real bug and the fix is the right shape. I read it with "can this lose or duplicate a save" in mind, and I think it can't. This is a code read only: I did not build or run anything. Since it's a draft, this is a COMMENT.

The scenario, as I read it. saves_dir moves user/ entry by entry. An entry holding a link is copied and then deleted (copy_tree, delete_tree), and copy_link re-makes each link. Before: an absolute link listed after the entry it names got realpath failing (the entry had already gone), so the link text was copied as written and the new link named the old user/ that is removed at the end. The saves themselves were safe in the new place; the player's link just dangled. After: the text is also checked against move_old and move_from (both need to start with /, and the next character must be a separator, so user2/ isn't matched), and rewritten under move_new. That covers a link to a link too, since readlink gives the text of the first hop. I see no way for the old or new code to lose or overwrite a save from this: the saves are never touched by copy_link, only the link text.

Edge cases, checked against the code:

  • Link pointing outside the folder: left as it is (not under either prefix). Fine.
  • Link loop: realpath fails, readlink gives the text, it is copied as written; nothing follows it, so no endless loop.
  • Dangling link: as written, except that an absolute dangling link that names a place under the old user/ is now pointed under the new place. That is better than before, but the description says "links that name nothing are unchanged", which isn't quite true for that case. The test only has the relative ../gone one.
  • Target already in the new place: the first pass counts it as kept, nothing moves, the old folder stays. Not touched by this PR.
  • Interrupted move and resume: an entry copied but not yet deleted exists in both places, so the next start sees kept and keeps the old folder (nothing lost). Not touched by this PR either.
  • Two links to the same entry: no shared state, each is rewritten alone (the 36 test links cover it).
  • Permissions: if readlink fails, copy_link returns 0, the copy is undone and the old place is kept. Safe.
  • Small gap (not new): a link to user/ itself (target equal to the folder, no trailing separator) isn't rewritten, because of target[n] != SEP. I doubt anyone has one; mention only.

The test. The extra links are the right idea, and CI on this head is green (ubuntu 1/2 and 2/2). But it still depends on the file system's order, so a green run is not proof the fix works: on a file system that lists newest-first (tmpfs, for instance), all 36 links come before save/ and the old code would pass too. It would be sturdier to not depend on the order: for example a case that removes or moves save/ first on purpose, or a small check of the link-text path on its own. Not a blocker, but the reason for this PR is that the old test flaked, so it is worth a try. The edge cases above (a link outside, a loop, an absolute dangling link) are not covered either; one each would be cheap.

Evidence (rule 3c). There is no real run, and the PR says so: the move runs once, on Linux, and the author is on Windows. For a fix that touches players' saves I would like a before/after listing from a Linux machine: a user/ with an absolute link into user/save/<id> (and some names that sort both sides of save), the readlink of each link in ~/.local/share/hookshot before the fix (names the old place) and after (names the new one), plus the saves still there. Anyone on Linux or SteamOS with a copy of that user/ can do it in a few minutes.

Overlap. #2317 (not a draft) deletes move_old/move_new/copy_link from this part of host/hos_main.c and does the move elsewhere, so the two will conflict in hos_main.c. If #2317 goes first, this fix needs porting to its version; if this goes first, #2317 needs rebasing and the new check carried across. The PR already says this. #2453, #1958 and #2842 don't touch these functions. The worklist row is fine.

To take it out of draft: a Linux before/after listing as above (or a Linux tester's result), and ideally the order-independent test. The CI run is green and no conflicts with main today. With that I'd be happy to approve.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 17:14 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2832#pullrequestreview-5878841643) Thanks, this is a real bug and the fix is the right shape. I read it with "can this lose or duplicate a save" in mind, and I think it can't. This is a code read only: I did not build or run anything. Since it's a draft, this is a COMMENT. **The scenario, as I read it.** `saves_dir` moves `user/` entry by entry. An entry holding a link is copied and then deleted (`copy_tree`, `delete_tree`), and `copy_link` re-makes each link. Before: an absolute link listed after the entry it names got `realpath` failing (the entry had already gone), so the link text was copied as written and the new link named the old `user/` that is removed at the end. The saves themselves were safe in the new place; the player's link just dangled. After: the text is also checked against `move_old` and `move_from` (both need to start with `/`, and the next character must be a separator, so `user2/` isn't matched), and rewritten under `move_new`. That covers a link to a link too, since `readlink` gives the text of the first hop. I see no way for the old or new code to lose or overwrite a save from this: the saves are never touched by `copy_link`, only the link text. **Edge cases, checked against the code:** - Link pointing outside the folder: left as it is (not under either prefix). Fine. - Link loop: `realpath` fails, `readlink` gives the text, it is copied as written; nothing follows it, so no endless loop. - Dangling link: as written, except that an absolute dangling link that names a place under the old `user/` is now pointed under the new place. That is better than before, but the description says "links that name nothing are unchanged", which isn't quite true for that case. The test only has the relative `../gone` one. - Target already in the new place: the first pass counts it as `kept`, nothing moves, the old folder stays. Not touched by this PR. - Interrupted move and resume: an entry copied but not yet deleted exists in both places, so the next start sees `kept` and keeps the old folder (nothing lost). Not touched by this PR either. - Two links to the same entry: no shared state, each is rewritten alone (the 36 test links cover it). - Permissions: if `readlink` fails, `copy_link` returns 0, the copy is undone and the old place is kept. Safe. - Small gap (not new): a link to `user/` itself (target equal to the folder, no trailing separator) isn't rewritten, because of `target[n] != SEP`. I doubt anyone has one; mention only. **The test.** The extra links are the right idea, and CI on this head is green (ubuntu 1/2 and 2/2). But it still depends on the file system's order, so a green run is not proof the fix works: on a file system that lists newest-first (tmpfs, for instance), all 36 links come before `save/` and the old code would pass too. It would be sturdier to not depend on the order: for example a case that removes or moves `save/` first on purpose, or a small check of the link-text path on its own. Not a blocker, but the reason for this PR is that the old test flaked, so it is worth a try. The edge cases above (a link outside, a loop, an absolute dangling link) are not covered either; one each would be cheap. **Evidence (rule 3c).** There is no real run, and the PR says so: the move runs once, on Linux, and the author is on Windows. For a fix that touches players' saves I would like a before/after listing from a Linux machine: a `user/` with an absolute link into `user/save/<id>` (and some names that sort both sides of `save`), the `readlink` of each link in `~/.local/share/hookshot` before the fix (names the old place) and after (names the new one), plus the saves still there. Anyone on Linux or SteamOS with a copy of that `user/` can do it in a few minutes. **Overlap.** #2317 (not a draft) deletes `move_old`/`move_new`/`copy_link` from this part of `host/hos_main.c` and does the move elsewhere, so the two will conflict in `hos_main.c`. If #2317 goes first, this fix needs porting to its version; if this goes first, #2317 needs rebasing and the new check carried across. The PR already says this. #2453, #1958 and #2842 don't touch these functions. The worklist row is fine. **To take it out of draft:** a Linux before/after listing as above (or a Linux tester's result), and ideally the order-independent test. The CI run is green and no conflicts with main today. With that I'd be happy to approve. <!-- github-sync:r:5878841643 -->
github-sync commented 2026-11-12 17:59:46 +00:00
Author
Owner

@mooki000 commented on GitHub · 2026-11-12 17:22 UTC · original

The Linux run asked for in T-BHEP, finished just after the merge: the fix holds on both file systems I could reach, and the old code fails on one of them every time. Tested at the head aea86182 (the merge commit itself was not built).

Arch Linux, GCC 16.2 and Clang 23.1; tests/test_linux_saves_move.py, 30 runs a cell, passes/failures:

Source btrfs, GCC btrfs, Clang tmpfs, GCC tmpfs, Clang
Base 51da0464 with only the new test 0/30 0/30 30/0 30/0
Head 30/0 30/0 30/0 30/0

Why the base passes on tmpfs, which matters for the test: the directory order decides it, as the description says. Listed unsorted, btrfs gives save before all 36 new links (creation order), so every link is read after its entry has gone and the old code fails each time. tmpfs gives the newest first, so all 36 links come before save and the old code passes. So on a runner whose temporary folder lists newest-first, the 36 links do not exercise the bug. Making some of the links before save/ is created and some after would put a late link on both orders; I can send that as a small test-only follow-up if you want it.

Measured on the edge cases the review read from the code (same behaviour on the base unless said):

  • An absolute link that was already dangling and names a place under the old user/ is now rewritten under the new folder (still dangling): the one behaviour change, as the review noted.
  • A link that reaches save through another spelling of the parent (a symlinked parent folder, or game//user/save) still names the removed folder when save went first.
  • A link to the old user folder itself ends up naming the removed folder.

Complete game-free suite on the head: 474 passed, 1 failed, 20 skipped; the one failure (the updater digest check, a missing .NET reference pack on this PC) fails on the base too.

Not tested: a real first start of the game that moves user/, ext4, xfs, Windows.

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 17:22 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2832#issuecomment-6830796802) The Linux run asked for in T-BHEP, finished just after the merge: **the fix holds on both file systems I could reach, and the old code fails on one of them every time.** Tested at the head `aea86182` (the merge commit itself was not built). Arch Linux, GCC 16.2 and Clang 23.1; `tests/test_linux_saves_move.py`, 30 runs a cell, passes/failures: | Source | btrfs, GCC | btrfs, Clang | tmpfs, GCC | tmpfs, Clang | |---|---|---|---|---| | Base `51da0464` with only the new test | 0/30 | 0/30 | 30/0 | 30/0 | | Head | 30/0 | 30/0 | 30/0 | 30/0 | **Why the base passes on tmpfs, which matters for the test:** the directory order decides it, as the description says. Listed unsorted, btrfs gives `save` before all 36 new links (creation order), so every link is read after its entry has gone and the old code fails each time. tmpfs gives the newest first, so all 36 links come before `save` and the old code passes. So on a runner whose temporary folder lists newest-first, the 36 links do not exercise the bug. Making some of the links before `save/` is created and some after would put a late link on both orders; I can send that as a small test-only follow-up if you want it. Measured on the edge cases the review read from the code (same behaviour on the base unless said): - An absolute link that was already dangling and names a place under the old `user/` is now rewritten under the new folder (still dangling): the one behaviour change, as the review noted. - A link that reaches `save` through another spelling of the parent (a symlinked parent folder, or `game//user/save`) still names the removed folder when `save` went first. - A link to the old `user` folder itself ends up naming the removed folder. Complete game-free suite on the head: 474 passed, 1 failed, 20 skipped; the one failure (the updater digest check, a missing .NET reference pack on this PC) fails on the base too. Not tested: a real first start of the game that moves `user/`, ext4, xfs, Windows. <!-- github-sync:c:6830796802 -->
github-sync commented 2026-11-12 17:59:46 +00:00
Author
Owner

@nullifyshot commented on GitHub · 2026-11-12 17:28 UTC · original

Thanks @mooki000 for the Linux run (T-BHEP) and @hckrman101 for the review. Both of you found the same weak spot: the test leans on the directory order, so a newest-first listing (tmpfs) passes on the old code. mooki000, yes please, send the test-only follow-up with some links made before save/ and some after; I won't start one, so we don't do it twice. A correction to the merged description, as the review says: an absolute link that already named nothing under the old user/ is now pointed under the new place (still dangling), so "links that name nothing are unchanged" holds only for the others. The link to user/ itself (no trailing separator) is left as it was before; noted, not changed.

> [@nullifyshot](https://github.com/nullifyshot) commented on GitHub · 2026-11-12 17:28 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2832#issuecomment-6830933118) Thanks @mooki000 for the Linux run (T-BHEP) and @hckrman101 for the review. Both of you found the same weak spot: the test leans on the directory order, so a newest-first listing (tmpfs) passes on the old code. mooki000, yes please, send the test-only follow-up with some links made before `save/` and some after; I won't start one, so we don't do it twice. A correction to the merged description, as the review says: an absolute link that already named nothing under the old `user/` is now pointed under the new place (still dangling), so "links that name nothing are unchanged" holds only for the others. The link to `user/` itself (no trailing separator) is left as it was before; noted, not changed. <!-- github-sync:c:6830933118 -->
github-sync commented 2026-11-12 19:02:39 +00:00
Author
Owner

@mooki000 commented on GitHub · 2026-11-12 18:32 UTC · original

Opened as #2888: half the links are made before save/ and half after, so the old code now fails on tmpfs as well as btrfs (30 of 30 in every cell); main passes everywhere.

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 18:32 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2832#issuecomment-6832446006) Opened as #2888: half the links are made before `save/` and half after, so the old code now fails on tmpfs as well as btrfs (30 of 30 in every cell); main passes everywhere. <!-- github-sync:c:6832446006 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
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/Hookshot!2832
No description provided.