fix: clean up failed NUP extraction members #1412

Merged

JooaoPedro15 merged 1 commit from fix/clean-failed-nup-member into main

2026-11-10 04:26:38 +00:00

JooaoPedro15 commented

2026-11-10 01:01:43 +00:00

(Migrated from github.com)

When NUP extraction fails while writing a member or replacing its destination, the .part file remains on disk. This can consume space after a disk-full error or leave a complete staging file after a denied replacement.

Remove the staging file after a failed write or replacement. Preserve the previous destination and the extraction error. Completed members remain available for retry.

The game-free regression injects both a partial write followed by ENOSPC and a denied replacement. It verifies that the staging file is gone, the previous file is intact, and a retry succeeds. Both cases fail on the old code. The NUP path, missing NUP and Builder cache suites passed locally on macOS. The full game-free workflow passed on Windows and Ubuntu in the fork.

AI assistance from OpenAI Codex is recorded in the commit trailers.

When NUP extraction fails while writing a member or replacing its destination, the `.part` file remains on disk. This can consume space after a disk-full error or leave a complete staging file after a denied replacement. Remove the staging file after a failed write or replacement. Preserve the previous destination and the extraction error. Completed members remain available for retry. The game-free regression injects both a partial write followed by `ENOSPC` and a denied replacement. It verifies that the staging file is gone, the previous file is intact, and a retry succeeds. Both cases fail on the old code. The NUP path, missing NUP and Builder cache suites passed locally on macOS. The full game-free workflow passed on [Windows and Ubuntu in the fork](https://github.com/JooaoPedro15/Hookshot/actions/runs/42732316958). AI assistance from OpenAI Codex is recorded in the commit trailers.

NotRexed commented

2026-11-10 03:50:04 +00:00

(Migrated from github.com)

This landed in the v2.0.12 branch (merged there for testing; it reaches main with that release), so closing. Thanks!

This landed in the v2.0.12 branch (merged there for testing; it reaches `main` with that release), so closing. Thanks!

NotRexed commented

2026-11-10 03:55:18 +00:00

(Migrated from github.com)

Reopened: closing this was a mistake on my side. It was tested in the v2.0.12 branch, and it will be merged into main here, so it shows as merged. Sorry for the noise.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 04:18:39 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good, nothing blocking.

Containment: the unlink only touches dst + ".part", which extraction_path() has already lstat-checked (no link/reparse point, no extra hard links) and kept under <out>/<part>. os.unlink doesn't follow links, so cleanup can't reach outside the unpack folder.
Windows: the with g: block exits (handle closed) before the except runs, so we aren't unlinking our own open handle. The .part comes from a plain open(..., "wb"), so no inherited read-only bit (unlike #1395's copy2 case). If AV/indexer still holds it, suppress(OSError) leaves it and the next run truncates it with wb: no regression.
BaseException also covers the SystemExit from copy_to and Ctrl-C; the original error is re-raised unchanged. The previous destination is untouched since os.replace is the only writer. open() sits outside the try, correctly.
Tests: nx2/test_nup_paths.py is picked up by the nx2/test_*.py glob in tests/run_all.py, so no registration needed. CI: Windows source checks green, Ubuntu pending when I looked.

Overlap:

#1393 (bronze-ff) is the closest: it's in make_game.py and removes the whole .game staging dir when extraction/validation fails; #1412 is in nup.py and removes only the failing member's .part. Different files/hunks, they stack. For the Builder, #1393 is what actually gives the space back (a failed unpack leaves all completed members, which #1412 keeps on purpose for retry). #1412 still matters for direct nup.py --extract runs, where resume is a feature. I'd land both, #1393 being the one that fixes the user-visible problem.
#1391 edits main() in nup.py and a different hunk of the same test file: no conflict.
#1395 and #1397 are ROMFS-folder copy / cache path validation in make_game.py, not NUP member extraction. #1395 is a sibling (same stage-then-replace idea). #1393 and #1395 both touch make_game.py and tests/test_make_game_cache.py in different regions, so the second to land may need a trivial rebase.

Nit: g = open(...) then with g: works but reads oddly; with open(temporary, "wb") as g: inside the try is equivalent.

Looks good, nothing blocking. - Containment: the unlink only touches `dst + ".part"`, which `extraction_path()` has already lstat-checked (no link/reparse point, no extra hard links) and kept under `<out>/<part>`. `os.unlink` doesn't follow links, so cleanup can't reach outside the unpack folder. - Windows: the `with g:` block exits (handle closed) before the `except` runs, so we aren't unlinking our own open handle. The `.part` comes from a plain `open(..., "wb")`, so no inherited read-only bit (unlike #1395's copy2 case). If AV/indexer still holds it, `suppress(OSError)` leaves it and the next run truncates it with `wb`: no regression. - `BaseException` also covers the `SystemExit` from `copy_to` and Ctrl-C; the original error is re-raised unchanged. The previous destination is untouched since `os.replace` is the only writer. `open()` sits outside the `try`, correctly. - Tests: `nx2/test_nup_paths.py` is picked up by the `nx2/test_*.py` glob in `tests/run_all.py`, so no registration needed. CI: Windows source checks green, Ubuntu pending when I looked. Overlap: - #1393 (bronze-ff) is the closest: it's in `make_game.py` and removes the whole `.game` staging dir when extraction/validation fails; #1412 is in `nup.py` and removes only the failing member's `.part`. Different files/hunks, they stack. For the Builder, #1393 is what actually gives the space back (a failed unpack leaves all completed members, which #1412 keeps on purpose for retry). #1412 still matters for direct `nup.py --extract` runs, where resume is a feature. I'd land both, #1393 being the one that fixes the user-visible problem. - #1391 edits `main()` in `nup.py` and a different hunk of the same test file: no conflict. - #1395 and #1397 are ROMFS-folder copy / cache path validation in `make_game.py`, not NUP member extraction. #1395 is a sibling (same stage-then-replace idea). #1393 and #1395 both touch `make_game.py` and `tests/test_make_game_cache.py` in different regions, so the second to land may need a trivial rebase. Nit: `g = open(...)` then `with g:` works but reads oddly; `with open(temporary, "wb") as g:` inside the `try` is equivalent.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101