Free the space left by a failed NUP unpack #1393

Merged

bronze-ff merged 1 commit from fix/builder-discard-failed-unpack into main

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

bronze-ff commented

2026-11-10 00:16:49 +00:00

(Migrated from github.com)

When a fresh NUP unpack fails, the Builder leaves its partially written .game directory in the output folder. The next attempt deletes that staging directory before unpacking anyway.

Remove the staging directory when extraction or validation fails, before the previous game cache is replaced. Keep the original build error and log the staging path if cleanup also fails.

The game-free regression simulates the Builder error from a disk-full unpack and checks that the partial data is gone and the previous cache and save are intact. It also covers a missing ROMFS and an unsupported build. The three cleanup assertions fail on main and pass with this change. The cache, missing-input, and output regression suites passed on Windows and Linux, and the NUP path suite passed on Linux.

Related to #1388, which was closed with disk-space guidance. #1391 measures free space on the destination volume. This change frees the Builder staging space after an unsuccessful unpack.

Compatibility check: tested a clean combined tree with #1391 (destination free-space reporting) and #1395 (interrupted ROMFS copies). The cache, missing-input and output suites passed on Windows and Linux. The NUP path suite also passed on Linux.

When a fresh NUP unpack fails, the Builder leaves its partially written `.game` directory in the output folder. The next attempt deletes that staging directory before unpacking anyway. Remove the staging directory when extraction or validation fails, before the previous game cache is replaced. Keep the original build error and log the staging path if cleanup also fails. The game-free regression simulates the Builder error from a disk-full unpack and checks that the partial data is gone and the previous cache and save are intact. It also covers a missing ROMFS and an unsupported build. The three cleanup assertions fail on `main` and pass with this change. The cache, missing-input, and output regression suites passed on Windows and Linux, and the NUP path suite passed on Linux. Related to #1388, which was closed with disk-space guidance. #1391 measures free space on the destination volume. This change frees the Builder staging space after an unsuccessful unpack. Compatibility check: tested a clean combined tree with #1391 (destination free-space reporting) and #1395 (interrupted ROMFS copies). The cache, missing-input and output suites passed on Windows and Linux. The NUP path suite also passed on Linux.

NotRexed commented

2026-11-10 03:49:58 +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:54:53 +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:16 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good to me. Cleanup is scoped to the staging dir and the original error is preserved.

What I checked:

Path safety: the only thing removed is game (<out>/.game), which check_cache_paths already resolved and rejected if it was a link/reparse point before this branch runs. shutil.rmtree doesn't follow links inside the tree, so a planted symlink or junction in the extraction is unlinked, not followed. Same target the old rmtree(game, ignore_errors=True) at the start already used, so no new reach. The previous romfs/.hookshot_romfs/kept cache is untouched because the cleanup only happens before the swap.
Windows locks: run() waits for nup.py to exit before we get to except, so there's no handle of ours left open. If AV/indexer still holds a file, rmtree raises OSError, which you log with the path and then re-raise the original error, so the real failure is never masked and the next run's start-of-run rmtree retries. Good. (Ctrl-C isn't caught, so that case still relies on the start-of-run cleanup; fine.)
Tests: the new assertions are in the existing tests/test_make_game_cache.py, no new file, so nothing to register in run_all.py. Faking the failure with make_game.fail(...) plus a stray .part file exercises the real SystemExit path.
Option ids: n/a.

Overlap with the siblings: #1412 (clean up the .part of a failed member inside nup.py) and this PR touch different files and don't conflict textually. They're complementary rather than competing: for the Builder, this PR is the one that actually frees the space (it drops the whole .game, which includes the .part and any completed members), so #1412's .part unlink is redundant on that path. #1412 still has value for running nup.py --extract directly and for retry-in-place, and it keeps the previous destination file intact. I'd merge both; if only one, this one fixes the reported disk-space symptom. #1391 and #1395 are different areas (reporting free space, ROMFS copy), and the PR notes a clean combined tree.

Nits, none blocking:

check() (the --only exefs probe) has the same shape: check_modules failing leaves work/probe behind. It's small (exefs only), so optional.
The except OSError half isn't covered by a test (only SystemExit is). Fine to leave.

CI was still pending when I looked (Python source checks on Windows and the Windows Vulkan link check already pass).

Looks good to me. Cleanup is scoped to the staging dir and the original error is preserved. What I checked: - Path safety: the only thing removed is `game` (`<out>/.game`), which `check_cache_paths` already resolved and rejected if it was a link/reparse point before this branch runs. `shutil.rmtree` doesn't follow links inside the tree, so a planted symlink or junction in the extraction is unlinked, not followed. Same target the old `rmtree(game, ignore_errors=True)` at the start already used, so no new reach. The previous `romfs`/`.hookshot_romfs`/`kept` cache is untouched because the cleanup only happens before the swap. - Windows locks: `run()` waits for nup.py to exit before we get to `except`, so there's no handle of ours left open. If AV/indexer still holds a file, `rmtree` raises `OSError`, which you log with the path and then re-raise the original error, so the real failure is never masked and the next run's start-of-run `rmtree` retries. Good. (Ctrl-C isn't caught, so that case still relies on the start-of-run cleanup; fine.) - Tests: the new assertions are in the existing `tests/test_make_game_cache.py`, no new file, so nothing to register in `run_all.py`. Faking the failure with `make_game.fail(...)` plus a stray `.part` file exercises the real `SystemExit` path. - Option ids: n/a. Overlap with the siblings: #1412 (clean up the `.part` of a failed member inside nup.py) and this PR touch different files and don't conflict textually. They're complementary rather than competing: for the Builder, this PR is the one that actually frees the space (it drops the whole `.game`, which includes the `.part` and any completed members), so #1412's `.part` unlink is redundant on that path. #1412 still has value for running `nup.py --extract` directly and for retry-in-place, and it keeps the previous destination file intact. I'd merge both; if only one, this one fixes the reported disk-space symptom. #1391 and #1395 are different areas (reporting free space, ROMFS copy), and the PR notes a clean combined tree. Nits, none blocking: - `check()` (the `--only exefs` probe) has the same shape: `check_modules` failing leaves `work/probe` behind. It's small (exefs only), so optional. - The `except OSError` half isn't covered by a test (only `SystemExit` is). Fine to leave. CI was still pending when I looked (Python source checks on Windows and the Windows Vulkan link check already pass).

Commenting is not possible because the repository is archived.

No reviewers

hckrman101