MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(lift): write a split chunk's parts before the chunk itself #2751

Closed

github-sync wants to merge 3 commits from github/pr/2751 into main

github-sync commented

2026-11-12 11:35:43 +00:00

Owner

@corentinbonjour opened this pull request on GitHub · 2026-11-12 10:54 UTC · original
Merged on GitHub in 5a0c21cbfa

If a lift worker stops in the middle of cutting a big chunk (killed for memory, which is the case _each retries), the functions of that chunk's later parts are removed from the lifted C without a message, and the build then fails far from the cause.

Why it happens

_split_one (nx2/chunks.py, from #2324) writes the chunk itself first, cut down to its first part, then code_NNN_1.c, code_NNN_2.c and so on. When a worker stops between two of those writes:

_each sees the broken pool and runs the file again in the lift's own process, as its docstring says it may, because "each edit finds the places it made already";
that holds for the patches, but not for the split: the chunk on disk is now only its first part, under the budget, so the second run makes no parts;
split_big then removes every code_NNN_k.c that it did not make in this run, which deletes the parts the dead worker had written.

The functions that were in those parts are gone, and nothing says so.

What changed

_split_one writes the parts from the last to the first, so the chunk itself is written last. A worker stopped part way leaves the chunk whole, and the run again cuts it exactly as before. made keeps its order (part 1 first), so split_big's result and chunk_costs.txt are unchanged.

How it was tested

tests/test_chunk_split.py gets a case: whole_write.write_whole is made to stop _split_one after its first file, then split_big runs the chunk again. It checks that the chunk is cut again and that the parts hold every function in order. On main it fails ("after a stopped worker the chunk is cut again": 1 part instead of 4); with this change it passes, on Windows and on Linux (WSL).
The existing checks of that test (parts of equal cost, declarations in each part, stale parts removed, costs written) still pass.
python -I -B tests/run_all.py on Windows: the same failing set as untouched main, none new.

> [@corentinbonjour](https://github.com/corentinbonjour) opened this pull request on GitHub · 2026-11-12 10:54 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2751) > **Merged on GitHub** in https://git.magicbean.dev/MagicBeanDev/Hookshot/commit/5a0c21cbfa23abc041468113360a04d2e066a78e If a lift worker stops in the middle of cutting a big chunk (killed for memory, which is the case `_each` retries), the functions of that chunk's later parts are removed from the lifted C without a message, and the build then fails far from the cause. ## Why it happens `_split_one` (nx2/chunks.py, from #2324) writes the chunk itself first, cut down to its first part, then `code_NNN_1.c`, `code_NNN_2.c` and so on. When a worker stops between two of those writes: 1. `_each` sees the broken pool and runs the file again in the lift's own process, as its docstring says it may, because "each edit finds the places it made already"; 2. that holds for the patches, but not for the split: the chunk on disk is now only its first part, under the budget, so the second run makes no parts; 3. `split_big` then removes every `code_NNN_k.c` that it did not make in this run, which deletes the parts the dead worker had written. The functions that were in those parts are gone, and nothing says so. ## What changed `_split_one` writes the parts from the last to the first, so the chunk itself is written last. A worker stopped part way leaves the chunk whole, and the run again cuts it exactly as before. `made` keeps its order (part 1 first), so `split_big`'s result and `chunk_costs.txt` are unchanged. ## How it was tested - `tests/test_chunk_split.py` gets a case: `whole_write.write_whole` is made to stop `_split_one` after its first file, then `split_big` runs the chunk again. It checks that the chunk is cut again and that the parts hold every function in order. On `main` it fails ("after a stopped worker the chunk is cut again": 1 part instead of 4); with this change it passes, on Windows and on Linux (WSL). - The existing checks of that test (parts of equal cost, declarations in each part, stale parts removed, costs written) still pass. - `python -I -B tests/run_all.py` on Windows: the same failing set as untouched `main`, none new.

github-sync added 1 commit

2026-11-12 11:35:43 +00:00

fix(lift): write a split chunk's parts before the chunk itself e242d3c13f

_split_one rewrote the chunk as its first part, then wrote the others.
A worker stopped between two writes (out of memory) left the chunk cut
short: _each ran the file again in the lift's process, found it under
the budget, made no parts, and split_big removed the parts already
written, so their functions were gone from the C. The chunk is now
written last, so a run again finds it whole and cuts it the same way.

Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 <[email protected]>

github-sync commented

2026-11-12 14:45:22 +00:00

Author

Owner

@Joey-Magnus approved on GitHub · 2026-11-12 14:35 UTC · original

I merged this into current main and ran tests/test_chunk_split.py, including the new stopped_part_way case. It passes, and CI is green.

The analysis is right. The old order wrote code_NNN.c (cut down to its first part) first. A worker killed after that left a chunk under budget, so the in-process retry from _each made no parts, and split_big then deleted the code_NNN_k.c files the dead worker had already written. Writing the parts from last to first and the chunk itself last (nx2/chunks.py:414) closes that. made.insert(0, out) keeps the result order the same, so chunk_costs.txt and the build order don't change.

One small window is left. If the worker dies after the final write of code_NNN.c but before _each gets its result, the retry sees an already-cut chunk again. That's a much narrower window, since the whole chunk is no longer held in memory by then, so I'm fine leaving it.

> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2751#pullrequestreview-5876246188) I merged this into current main and ran tests/test_chunk_split.py, including the new stopped_part_way case. It passes, and CI is green. The analysis is right. The old order wrote code_NNN.c (cut down to its first part) first. A worker killed after that left a chunk under budget, so the in-process retry from `_each` made no parts, and split_big then deleted the code_NNN_k.c files the dead worker had already written. Writing the parts from last to first and the chunk itself last (nx2/chunks.py:414) closes that. `made.insert(0, out)` keeps the result order the same, so chunk_costs.txt and the build order don't change. One small window is left. If the worker dies after the final write of code_NNN.c but before `_each` gets its result, the retry sees an already-cut chunk again. That's a much narrower window, since the whole chunk is no longer held in memory by then, so I'm fine leaving it.

github-sync commented

2026-11-12 16:55:58 +00:00

Author

Owner

@hckrman101 reviewed on GitHub · 2026-11-12 16:31 UTC · original

Thanks, the analysis holds up. I read it against _split_one, split_big and _each.

The bug and the fix. Before, the chunk was rewritten as its first part before the other parts were written. A worker that stopped in between left a chunk under the budget. The in-process retry in _each then made no parts, and split_big removed every code_NNN_k.c it had not made in that run. Writing the parts from last to first and the chunk last fixes that. Each write goes through whole_write, so no file is ever half written, and a stop at any point leaves a whole chunk that the retry cuts again as before. The unfinished parts are overwritten, or removed as stale if they are not made again. It doesn't interact with the lift cache key (#2686, it only touches make_game.py) or the parallel emit (#2748, already merged), because this all happens after the emit, inside the split pass.

Byte-identical output. I think so. The cut points and the text of each file don't change. made.insert(0, out) keeps part 1 first, so split_big's return and the build order are as before. costs is now filled in reverse, but write_costs sorts it, so chunk_costs.txt is the same. The printed line is the same.

One window is left, and I think it is wider than Joey-Magnus's review says. He mentions a worker dying after its last write. The same state also occurs when this worker finished all its writes and its result was lost because a different worker was killed. When one worker is killed, ProcessPoolExecutor breaks the whole pool, and any future that has no result yet raises BrokenProcessPool. _each then runs that chunk again here, finds it already cut, makes no parts, and split_big deletes the parts the finished worker wrote. That is the same silent loss. It needs two things at once: a memory kill, and a finished worker whose result has not been collected. With 24 chunks in flight at the same time, that is not so narrow. I don't think it has to block this PR, since it is much better than before. A follow-up could make the second run recognise an already-cut chunk. For example, when _split_one sees a chunk under budget that already has code_NNN_k.c files, split_big keeps them instead of removing them.

Evidence (rule 3c). The new case in tests/test_chunk_split.py fails on main and passes here, which is a good unit-level proof. But this is a build fix, so I'd like to see it in a real build too:

a normal lift with and without this change, with a hash of the gen folder (every code_*.c and chunk_costs.txt) to show the output is identical;
ideally the failure on main and the fix, with a worker killed by hand during the split (kill one worker in split_big while it writes a big chunk) and the hardware named.

If the forced kill is hard to do, the hash comparison alone is enough for me. CI is green, and Joey-Magnus has approved. There are no conflicts with other open PRs: nothing else open touches nx2/chunks.py.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 16:31 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2751#pullrequestreview-5878185502) Thanks, the analysis holds up. I read it against `_split_one`, `split_big` and `_each`. **The bug and the fix.** Before, the chunk was rewritten as its first part before the other parts were written. A worker that stopped in between left a chunk under the budget. The in-process retry in `_each` then made no parts, and `split_big` removed every `code_NNN_k.c` it had not made in that run. Writing the parts from last to first and the chunk last fixes that. Each write goes through `whole_write`, so no file is ever half written, and a stop at any point leaves a whole chunk that the retry cuts again as before. The unfinished parts are overwritten, or removed as stale if they are not made again. It doesn't interact with the lift cache key (#2686, it only touches `make_game.py`) or the parallel emit (#2748, already merged), because this all happens after the emit, inside the split pass. **Byte-identical output.** I think so. The cut points and the text of each file don't change. `made.insert(0, out)` keeps part 1 first, so `split_big`'s return and the build order are as before. `costs` is now filled in reverse, but `write_costs` sorts it, so `chunk_costs.txt` is the same. The printed line is the same. **One window is left, and I think it is wider than Joey-Magnus's review says.** He mentions a worker dying after its last write. The same state also occurs when this worker finished all its writes and its result was lost because a different worker was killed. When one worker is killed, `ProcessPoolExecutor` breaks the whole pool, and any future that has no result yet raises `BrokenProcessPool`. `_each` then runs that chunk again here, finds it already cut, makes no parts, and `split_big` deletes the parts the finished worker wrote. That is the same silent loss. It needs two things at once: a memory kill, and a finished worker whose result has not been collected. With 24 chunks in flight at the same time, that is not so narrow. I don't think it has to block this PR, since it is much better than before. A follow-up could make the second run recognise an already-cut chunk. For example, when `_split_one` sees a chunk under budget that already has `code_NNN_k.c` files, `split_big` keeps them instead of removing them. **Evidence (rule 3c).** The new case in `tests/test_chunk_split.py` fails on `main` and passes here, which is a good unit-level proof. But this is a build fix, so I'd like to see it in a real build too: - a normal lift with and without this change, with a hash of the `gen` folder (every `code_*.c` and `chunk_costs.txt`) to show the output is identical; - ideally the failure on `main` and the fix, with a worker killed by hand during the split (kill one worker in `split_big` while it writes a big chunk) and the hardware named. If the forced kill is hard to do, the hash comparison alone is enough for me. CI is green, and Joey-Magnus has approved. There are no conflicts with other open PRs: nothing else open touches `nx2/chunks.py`.

github-sync referenced this pull request from a commit

2026-11-12 17:57:20 +00:00

fix(lift): write a split chunk's parts before the chunk itself (#2751)

github-sync added 2 commits

2026-11-12 17:57:33 +00:00

Merge branch 'main' into fix/split-big-worker-retry 3e4ca13215

fix(lift): keep the parts of a chunk cut already when split_big runs it again 38f3696145

A worker that finished the cut but whose result was lost when another
worker stopped (the pool breaks, _each runs the chunk again in the
lift's process) left a chunk already cut to its first part: the run
again made no parts and split_big removed the ones the worker wrote.
_split_one now joins the chunk and its parts on disk, cuts them again
and keeps the parts when they come out the same; an earlier lift's
parts beside a chunk that is small now still go. The cut moves into
_cut, unchanged, and a chunk the cut leaves in one part is no longer
rewritten.

github-sync closed this pull request

2026-11-12 17:58:55 +00:00

github-sync commented

2026-11-12 17:58:56 +00:00

Author

Owner

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

I merged main in and added one commit on top for the window from my last review: a worker that finished the cut, but whose result was lost when another worker stopped and broke the pool.

What I added. When _split_one finds a chunk under the budget that has code_NNN_k.c files beside it, it joins the chunk and those parts, cuts the result again, and keeps the parts if the cut gives back exactly the same files. So the second run in _each returns the parts instead of split_big deleting them. Parts from an earlier lift next to a chunk that is now small don't match, so they are still removed. The cut itself moved into _cut without changes. One small side effect: a chunk the cut leaves in a single part is no longer rewritten. tests/test_chunk_split.py gets a finished_but_lost case that covers both cases. It fails without the new commit ("a chunk cut already keeps its parts") and passes with it.

Same output. I didn't run a full lift here. Instead I rebuilt the real cut chunks from a cached lift's gen folder (the 8 chunks that had parts, plus 4 small ones) and split them with main's chunks.py and with this branch's. Both runs gave the same 30 files, byte for byte, with the same sha256 over all of them and the same chunk_costs.txt. Running split_big a second time over the already-cut files leaves 13 files and none of the 17 parts on main, while this branch keeps all 30 unchanged. test_chunk_split, test_parallel_emit, test_spawn_emit, test_lift_cache, test_chunks_save_folder, test_stale_hot_pieces and test_failure_reason all pass.

Thanks for tracking this down and for the clear write-up!

> [@hckrman101](https://github.com/hckrman101) approved on GitHub · 2026-11-12 17:11 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2751#pullrequestreview-5878793368) I merged main in and added one commit on top for the window from my last review: a worker that finished the cut, but whose result was lost when another worker stopped and broke the pool. **What I added.** When `_split_one` finds a chunk under the budget that has `code_NNN_k.c` files beside it, it joins the chunk and those parts, cuts the result again, and keeps the parts if the cut gives back exactly the same files. So the second run in `_each` returns the parts instead of `split_big` deleting them. Parts from an earlier lift next to a chunk that is now small don't match, so they are still removed. The cut itself moved into `_cut` without changes. One small side effect: a chunk the cut leaves in a single part is no longer rewritten. `tests/test_chunk_split.py` gets a `finished_but_lost` case that covers both cases. It fails without the new commit ("a chunk cut already keeps its parts") and passes with it. **Same output.** I didn't run a full lift here. Instead I rebuilt the real cut chunks from a cached lift's `gen` folder (the 8 chunks that had parts, plus 4 small ones) and split them with main's `chunks.py` and with this branch's. Both runs gave the same 30 files, byte for byte, with the same sha256 over all of them and the same `chunk_costs.txt`. Running `split_big` a second time over the already-cut files leaves 13 files and none of the 17 parts on main, while this branch keeps all 30 unchanged. `test_chunk_split`, `test_parallel_emit`, `test_spawn_emit`, `test_lift_cache`, `test_chunks_save_folder`, `test_stale_hot_pieces` and `test_failure_reason` all pass. Thanks for tracking this down and for the clear write-up!

Commenting is not possible because the repository is archived.

No reviewers