perf(nup): write each extracted member in place and check each folder once #2416

Merged

corentinbonjour merged 1 commit from perf/nup-extract-in-place into main

2026-11-11 20:30:25 +00:00

corentinbonjour commented

2026-11-11 18:05:23 +00:00

(Migrated from github.com)

Unpacking a .nup spends most of its time on per-member filesystem work, not on data: each of the ~186,000 members gets a .part temporary, a rename, a realpath and an lstat of every folder on its path. Writing in place and checking each folder once makes the unpack 2.2 to 4.5 times faster on synthetic packages, about 2 minutes per unpack on a real one (estimated).

What changed

nx2/nup.py, extract() only:

New Destination class: it resolves the output folder once, and checks each folder with one lstat the first time it is reached: it creates the folder if it is missing, and refuses a link or reparse point.
One lstat on each member:
- a link or reparse point is refused;
- a regular file of the right size is kept as before, including the zstd head and digest check;
- any other existing file is unlinked rather than truncated, so another hard link to it keeps its data.
Writing: the member is created with os.open(O_WRONLY | O_CREAT | O_EXCL | O_BINARY) under its final name, so there is no .part and no os.replace. O_EXCL does not follow a link that appears at that name.
On any failure (ENOSPC, interrupt, the bounded zstd's SystemExit), the half-written member is unlinked, and the out-of-space message is built first, as before.
Leftover temporary: a .part left by an older builder can only sit beside the first member that still needs writing, so it is unlinked once per run.

The path validation moves into package_parts() and the link test into is_link(). heal and unpack still go through extraction_path() and keep their .part/.plain temporaries, unchanged.

Resume: no journal is needed. A member is written by sequential appends, so a run killed in the middle leaves it shorter than its listed size, and the existing size check writes it again. The builder never resumes anyway: it removes .game before every unpack and stamps only after success.

Measurements

The synthetic packages were made by a script (all members .zs with real SHA-256 digests, so the head and digest checks run in both versions). Each run is a fresh process, old (main) and new interleaved in shuffled order, extract(only="romfs") on NTFS with Defender on. Medians (min-max):

Package	main	this PR
20,000 x 16 KB, 5 runs	14.91 s (13.14-18.60)	6.65 s (5.82-7.32), 2.2x
10,000 x 130 KB raw, 5 runs	11.12 s (10.83-13.97)	4.06 s (4.00-4.31), 2.7x
10,000 x 130 KB, 25% zstd-packed, 5 runs	12.14 s (11.70-12.90)	4.62 s (4.37-4.86), 2.6x

On a loaded machine the gap was larger (4.5x on the zstd package). The quiet runs save about 0.7 ms per member, so roughly 2 minutes over 186,583 members. That figure is scaled from synthetic packages: I did not time a real unpack.

How it was tested

nx2/test_nup_paths.py:
- On Linux (WSL, Python 3.14), all 27 tests pass, including the ones that plant links to make extraction write outside its folder.
- On Windows the same 5 tests fail on this branch and on main. They need the symlink privilege (WinError 1314).
- The new tests (members written in place with folders checked once, a failed member removed and retried, a stale temporary hard link) fail on main's nup.py.
Other nup checks: nx2/test_nup_out_of_space.py, test_nup_zstd_heads.py and test_nup_stand_in.py pass.
Full suite: python -I -B tests/run_all.py on Windows gives the same failing set as untouched main, none new.

Notes

Folder checks: each folder is checked once per run, not once per member. A junction planted inside the staging folder during the extraction would not be caught. The old code's window was only narrower, not closed: it re-checked every few hundred microseconds. The member itself is still checked, and created with O_EXCL.
Durability: nothing is fsynced, same as before. The .part and rename gave no stronger guarantee on NTFS.
Overlap with #2326: that draft edits the same link check inside extraction_path(). This change moves the check into is_link(), so whichever lands second has a small conflict, and is_link() follows #2326's choice for OneDrive reparse points. This change keeps "the unpack refuses any".
#2391: no overlap with extract().

Unpacking a `.nup` spends most of its time on per-member filesystem work, not on data: each of the ~186,000 members gets a `.part` temporary, a rename, a `realpath` and an `lstat` of every folder on its path. Writing in place and checking each folder once makes the unpack 2.2 to 4.5 times faster on synthetic packages, about 2 minutes per unpack on a real one (estimated). ## What changed `nx2/nup.py`, `extract()` only: - **New `Destination` class:** it resolves the output folder once, and checks each folder with one `lstat` the first time it is reached: it creates the folder if it is missing, and refuses a link or reparse point. - **One `lstat` on each member:** - a link or reparse point is refused; - a regular file of the right size is kept as before, including the zstd head and digest check; - any other existing file is unlinked rather than truncated, so another hard link to it keeps its data. - **Writing:** the member is created with `os.open(O_WRONLY | O_CREAT | O_EXCL | O_BINARY)` under its final name, so there is no `.part` and no `os.replace`. `O_EXCL` does not follow a link that appears at that name. - **On any failure** (ENOSPC, interrupt, the bounded zstd's `SystemExit`), the half-written member is unlinked, and the out-of-space message is built first, as before. - **Leftover temporary:** a `.part` left by an older builder can only sit beside the first member that still needs writing, so it is unlinked once per run. The path validation moves into `package_parts()` and the link test into `is_link()`. `heal` and `unpack` still go through `extraction_path()` and keep their `.part`/`.plain` temporaries, unchanged. **Resume:** no journal is needed. A member is written by sequential appends, so a run killed in the middle leaves it shorter than its listed size, and the existing size check writes it again. The builder never resumes anyway: it removes `.game` before every unpack and stamps only after success. ## Measurements The synthetic packages were made by a script (all members `.zs` with real SHA-256 digests, so the head and digest checks run in both versions). Each run is a fresh process, old (`main`) and new interleaved in shuffled order, `extract(only="romfs")` on NTFS with Defender on. Medians (min-max): | Package | main | this PR | |---|---|---| | 20,000 x 16 KB, 5 runs | 14.91 s (13.14-18.60) | 6.65 s (5.82-7.32), 2.2x | | 10,000 x 130 KB raw, 5 runs | 11.12 s (10.83-13.97) | 4.06 s (4.00-4.31), 2.7x | | 10,000 x 130 KB, 25% zstd-packed, 5 runs | 12.14 s (11.70-12.90) | 4.62 s (4.37-4.86), 2.6x | On a loaded machine the gap was larger (4.5x on the zstd package). The quiet runs save about 0.7 ms per member, so roughly 2 minutes over 186,583 members. That figure is scaled from synthetic packages: I did not time a real unpack. ## How it was tested - **`nx2/test_nup_paths.py`:** - On Linux (WSL, Python 3.14), all 27 tests pass, including the ones that plant links to make extraction write outside its folder. - On Windows the same 5 tests fail on this branch and on `main`. They need the symlink privilege (WinError 1314). - The new tests (members written in place with folders checked once, a failed member removed and retried, a stale temporary hard link) fail on `main`'s `nup.py`. - **Other `nup` checks:** `nx2/test_nup_out_of_space.py`, `test_nup_zstd_heads.py` and `test_nup_stand_in.py` pass. - **Full suite:** `python -I -B tests/run_all.py` on Windows gives the same failing set as untouched `main`, none new. ## Notes - **Folder checks:** each folder is checked once per run, not once per member. A junction planted inside the staging folder during the extraction would not be caught. The old code's window was only narrower, not closed: it re-checked every few hundred microseconds. The member itself is still checked, and created with `O_EXCL`. - **Durability:** nothing is fsynced, same as before. The `.part` and rename gave no stronger guarantee on NTFS. - **Overlap with #2326:** that draft edits the same link check inside `extraction_path()`. This change moves the check into `is_link()`, so whichever lands second has a small conflict, and `is_link()` follows #2326's choice for OneDrive reparse points. This change keeps "the unpack refuses any". - **#2391:** no overlap with `extract()`.

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

2026-11-11 20:08:25 +00:00

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

This looks right to me. I read extract() and Destination at the PR's head with the link and hard-link attacks the old .part path defended against in mind, and the new path holds up:

The member's own name is lstated every time (nx2/nup.py:556-561), and the file is created with O_CREAT | O_EXCL (:584). A link at that name is refused, and one that appears between the check and the open makes the open fail instead of being followed. The failed open sits outside the try, so it doesn't unlink someone else's file either.
A wrong-sized existing file is unlinked, not truncated (:578-579), so a hard link that shares its data elsewhere keeps it. test_destination_hard_link_is_replaced_without_truncation covers this.
On resume, copy_to only reads forward from r[3] and writes sequentially (:254 onward), so a killed run leaves a short file, and the existing size check writes it again. Any exception, KeyboardInterrupt included, unlinks the half-written member (:591-595) after the out-of-space message has measured the drive. In any case, make_game.py removes .game (and the --check probe folder) before it calls --extract, so the builder never relies on resuming.
Dropping the commonpath check from the hot path is safe: package_parts already rejects empty, ./.., backslash, : and device names, so a rel can't climb out of its part.
heal and --unpack keep extraction_path with their temporaries unchanged.

The trade-off you describe in the notes, a folder checked once per run instead of before every member, seems reasonable. The old window was only narrower, and the member itself is still protected by O_EXCL.

The failed Python source checks (windows-2022, part 1/2) job is tests/test_native_astc_texture_count.py (the scanf deprecation warning under -Werror), which #2397 has since fixed on main. nx2/test_nup_paths.py and the other nup checks passed on both Windows shards and on Ubuntu. Your measurements come from synthetic packages only, which you say plainly. A timed real unpack before and after would be nice to have for the changelog, but it isn't needed to merge.

This looks right to me. I read `extract()` and `Destination` at the PR's head with the link and hard-link attacks the old `.part` path defended against in mind, and the new path holds up: - The member's own name is `lstat`ed every time (`nx2/nup.py:556-561`), and the file is created with `O_CREAT | O_EXCL` (`:584`). A link at that name is refused, and one that appears between the check and the `open` makes the `open` fail instead of being followed. The failed `open` sits outside the `try`, so it doesn't unlink someone else's file either. - A wrong-sized existing file is unlinked, not truncated (`:578-579`), so a hard link that shares its data elsewhere keeps it. `test_destination_hard_link_is_replaced_without_truncation` covers this. - On resume, `copy_to` only reads forward from `r[3]` and writes sequentially (`:254` onward), so a killed run leaves a short file, and the existing size check writes it again. Any exception, `KeyboardInterrupt` included, unlinks the half-written member (`:591-595`) after the out-of-space message has measured the drive. In any case, `make_game.py` removes `.game` (and the `--check` probe folder) before it calls `--extract`, so the builder never relies on resuming. - Dropping the `commonpath` check from the hot path is safe: `package_parts` already rejects empty, `.`/`..`, backslash, `:` and device names, so a `rel` can't climb out of its part. - `heal` and `--unpack` keep `extraction_path` with their temporaries unchanged. The trade-off you describe in the notes, a folder checked once per run instead of before every member, seems reasonable. The old window was only narrower, and the member itself is still protected by `O_EXCL`. The failed `Python source checks (windows-2022, part 1/2)` job is `tests/test_native_astc_texture_count.py` (the `scanf` deprecation warning under `-Werror`), which #2397 has since fixed on main. `nx2/test_nup_paths.py` and the other `nup` checks passed on both Windows shards and on Ubuntu. Your measurements come from synthetic packages only, which you say plainly. A timed real unpack before and after would be nice to have for the changelog, but it isn't needed to merge.