fix(builder): scope stale process cleanup to the work folder #2250

Merged

Arkex-ai merged 2 commits from fix/build-cleanup-scope into main

2026-11-11 21:28:41 +00:00

Arkex-ai commented

2026-11-11 11:43:32 +00:00

(Migrated from github.com)

Summary

Starting a Windows build could stop Python or compiler processes belonging to another work folder. Cleanup now requires positive work-folder or output ownership and preserves processes whose ownership cannot be established. Fixes #2142.

Technical changes

Parse complete Windows arguments and compare normalized absolute paths at component boundaries. Unrelated orphans, sibling-prefix outputs and include/input paths no longer qualify.
Protect the full ancestor chain, including non-Python launchers. Recognize this installation's default work and a proven make_game parent's matching build.py child; preserve unknown orphans and unrelated descendants.
Stop eligible processes individually, without taskkill /T; report successful stops only and retain best-effort cleanup.

Validation

Current tested head 40321e87, rebased onto main 355453de. Default-work cleanup now recognizes previous packaged Builder versions under the same home, including their proven build child. Other homes, sibling prefixes, unexpected layouts and custom-work mismatches remain excluded. Eleven tests pass on Windows and Linux, including 500 quoted-argument controls; the previous production source fails the two new cross-version/query-diagnostic regressions on both systems. All process queries and termination calls are intercepted. No real process was stopped. Fresh hosted checks are separate exact-head evidence.

Earlier validation evidence and limits

Earlier tested head e14e3a8c, rebased onto main 6df80a8d. The selector, test and documentation patch is unchanged by range-diff; the worklist conflict preserves both main's existing row and this PR's row. Ten focused tests pass on Windows and Linux, including 500 deterministic quoted-argument round trips. All process queries and termination calls are intercepted; no real process was stopped. Fresh CI checks run on this head.

At 6a3af365 on base ebbba2ea, the unmodified helper selected unrelated synthetic processes as well as the intended control. Four causal mutations removing work ownership, path boundaries, ancestor protection or individual termination were rejected. Full Windows: 342 passed / 3 failed / 30 skipped, versus exact base 341 / 3 / 30, with the same failure names. Full Linux: 352 / 0 / 23; a separate release-script run with jq supplied its missing internal coverage (24 cases passed). These full/causal results refer to that earlier head, not the current rebase.

This is a synthetic selector regression, not a real termination or PID-reuse guarantee. Unknown orphan build.py processes and outputs known only through relative paths or response files remain untouched. Linux cleanup behavior is unchanged; gameplay does not exercise this Windows process selector.

Dev-work claim. Coordination with #2324 and #2373 preserves this engine fix independently of retained-lift and launcher UI changes.

Current-head workflow	Result
Game-free source regressions	Queued
Linux AppImage	Queued
PR title	Queued
Vulkan back end with NVIDIA DLSS	Queued
Windows builder	Queued

## Summary Starting a Windows build could stop Python or compiler processes belonging to another work folder. Cleanup now requires positive work-folder or output ownership and preserves processes whose ownership cannot be established. Fixes #2142. ## Technical changes - Parse complete Windows arguments and compare normalized absolute paths at component boundaries. Unrelated orphans, sibling-prefix outputs and include/input paths no longer qualify. - Protect the full ancestor chain, including non-Python launchers. Recognize this installation's default work and a proven make_game parent's matching build.py child; preserve unknown orphans and unrelated descendants. - Stop eligible processes individually, without `taskkill /T`; report successful stops only and retain best-effort cleanup. ## Validation Current tested head **`40321e87`**, rebased onto main **`355453de`**. Default-work cleanup now recognizes previous packaged Builder versions under the same home, including their proven build child. Other homes, sibling prefixes, unexpected layouts and custom-work mismatches remain excluded. Eleven tests pass on Windows and Linux, including 500 quoted-argument controls; the previous production source fails the two new cross-version/query-diagnostic regressions on both systems. All process queries and termination calls are intercepted. No real process was stopped. Fresh [hosted checks](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2250/checks) are separate exact-head evidence. <details> <summary>Earlier validation evidence and limits</summary> Earlier tested head **`e14e3a8c`**, rebased onto main **`6df80a8d`**. The selector, test and documentation patch is unchanged by range-diff; the worklist conflict preserves both main's existing row and this PR's row. **Ten focused tests pass on Windows and Linux**, including 500 deterministic quoted-argument round trips. All process queries and termination calls are intercepted; no real process was stopped. Fresh [CI checks](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2250/checks) run on this head. At `6a3af365` on base `ebbba2ea`, the unmodified helper selected unrelated synthetic processes as well as the intended control. Four causal mutations removing work ownership, path boundaries, ancestor protection or individual termination were rejected. Full Windows: **342 passed / 3 failed / 30 skipped**, versus exact base **341 / 3 / 30**, with the same failure names. Full Linux: **352 / 0 / 23**; a separate release-script run with jq supplied its missing internal coverage (**24 cases passed**). These full/causal results refer to that earlier head, not the current rebase. This is a synthetic selector regression, not a real termination or PID-reuse guarantee. Unknown orphan build.py processes and outputs known only through relative paths or response files remain untouched. Linux cleanup behavior is unchanged; gameplay does not exercise this Windows process selector. [Dev-work claim](https://discord.com/channels/1568810484818378804/1569019786766188665/1570037297234051113). Coordination with #2324 and #2373 preserves this engine fix independently of retained-lift and launcher UI changes. </details>  | Current-head workflow | Result | |---|---| | [Game-free source regressions](https://github.com/NotRexed/Hookshot/actions/runs/43095862413) | Queued | | [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/43095862343) | Queued | | [PR title](https://github.com/NotRexed/Hookshot/actions/runs/43095862104) | Queued | | [Vulkan back end with NVIDIA DLSS](https://github.com/NotRexed/Hookshot/actions/runs/43095862318) | Queued | | [Windows builder](https://github.com/NotRexed/Hookshot/actions/runs/43095862349) | Queued |

Joey-Magnus (Migrated from github.com) reviewed

2026-11-11 20:07:28 +00:00

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

should fix — after a builder update, the earlier build on the default work folder survives (nx2/make_game.py:897-900): with no --work, a make_game.py is owned only when its script path equals this __file__. But the packaged builder runs make_game.py from its own version folder: Runner.cs:133 uses tools.Root, which is %LOCALAPPDATA%\HookshotBuilder\versions\<version>\HookshotBuilder\tools. Meanwhile the default work folder (%LOCALAPPDATA%\HookshotBuilder, line 1567) is shared by every version, and the builder only passes --work when the player chose a folder (MainWindow.xaml.cs:891). So if a build is still running from version N (the builder crashed, or was killed by an update) and version N+1 starts a build:

the old make_game.py isn't recognized, so it's preserved;
its build.py child isn't selected either, because owned only holds processes that are already doomed;
the old build's compilers are stopped, but its build.py keeps starting new ones in the same work\sa\build.

That's the "replaced by an update mid-build" case the old docstring named for #501/#530. A normal Cancel or Close still kills the whole tree (Runner.Cancel uses Kill(entireProcessTree: true)), so this is limited to crashes and killed builders. Still, it's the case this function exists for. Two possible fixes:

accept a default-work make_game.py whose absolute path matches <LOCALAPPDATA>\HookshotBuilder\versions\*\HookshotBuilder\tools\nx2\make_game.py, i.e. any version of this builder, not only this one; or
have the builder always pass --work Storage.Work, so ownership is always explicit.

Either way, please add a test row for "earlier version's make_game, default work".

minor — worklist row in the wrong table (docs/worklist.md:27): the row goes into Open, but docs/worklist.md:15 says a row moves to Fixed, waiting for a release when its PR merges. Since this PR is the fix, put the row there with this PR's link, or leave it out.

minor — silent return on a failed query (nx2/make_game.py:943-944): if r.returncode: return skips cleanup without a word. Before this change, a failed query reached the except and printed "could not look for an earlier build's processes". A one-line message here would keep that diagnostic for build-problem reports.

question: docs/build-cleanup-scope.md is a whole page for one helper, and the docstring it replaces lost the symptoms (OSError: [Errno 22] on a64_fpx.h, WinError 1224). Would a few lines of that in the docstring serve the next reader better than a separate page? Not blocking.

This is a real safety fix, and the selector reads carefully. The new rules are sound: ownership has to be positive, paths are compared at component boundaries, the ancestor chain is walked across non-Python launchers now that the query lists every process, and `taskkill` no longer uses `/T`. I traced the rules against how `make_game.py` actually launches its steps. `lift.py` gets an absolute `--work`, so it's still matched. `build.py` is matched as `nx2/build.py --standalone` under a proven parent. Its compilers write absolute `-o` paths under `OOT_WORK` (`nx2/kit.py:20`), so the clang/lld rule matches them. `PY`'s `-X utf8` is handled by `_cleanup_python`. CI is green. **should fix — after a builder update, the earlier build on the default work folder survives** (`nx2/make_game.py:897-900`): with no `--work`, a `make_game.py` is owned only when its script path equals this `__file__`. But the packaged builder runs `make_game.py` from its own version folder: `Runner.cs:133` uses `tools.Root`, which is `%LOCALAPPDATA%\HookshotBuilder\versions\<version>\HookshotBuilder\tools`. Meanwhile the default work folder (`%LOCALAPPDATA%\HookshotBuilder`, line 1567) is shared by every version, and the builder only passes `--work` when the player chose a folder (`MainWindow.xaml.cs:891`). So if a build is still running from version N (the builder crashed, or was killed by an update) and version N+1 starts a build: - the old `make_game.py` isn't recognized, so it's preserved; - its `build.py` child isn't selected either, because `owned` only holds processes that are already doomed; - the old build's compilers are stopped, but its `build.py` keeps starting new ones in the same `work\sa\build`. That's the "replaced by an update mid-build" case the old docstring named for #501/#530. A normal Cancel or Close still kills the whole tree (`Runner.Cancel` uses `Kill(entireProcessTree: true)`), so this is limited to crashes and killed builders. Still, it's the case this function exists for. Two possible fixes: - accept a default-work `make_game.py` whose absolute path matches `<LOCALAPPDATA>\HookshotBuilder\versions\*\HookshotBuilder\tools\nx2\make_game.py`, i.e. any version of this builder, not only this one; or - have the builder always pass `--work Storage.Work`, so ownership is always explicit. Either way, please add a test row for "earlier version's make_game, default work". **minor — worklist row in the wrong table** (`docs/worklist.md:27`): the row goes into **Open**, but `docs/worklist.md:15` says a row moves to **Fixed, waiting for a release** when its PR merges. Since this PR is the fix, put the row there with this PR's link, or leave it out. **minor — silent return on a failed query** (`nx2/make_game.py:943-944`): `if r.returncode: return` skips cleanup without a word. Before this change, a failed query reached the `except` and printed "could not look for an earlier build's processes". A one-line message here would keep that diagnostic for build-problem reports. **question:** `docs/build-cleanup-scope.md` is a whole page for one helper, and the docstring it replaces lost the symptoms (`OSError: [Errno 22]` on `a64_fpx.h`, WinError 1224). Would a few lines of that in the docstring serve the next reader better than a separate page? Not blocking.

Arkex-ai commented

2026-11-11 20:24:26 +00:00

(Migrated from github.com)

Thanks, the cross-version case was missing. Fixed in 40321e87 on main 355453de: default-work ownership accepts the exact packaged versions/<version>/HookshotBuilder/tools/nx2/make_game.py shape under that same home, then follows only its proven build child. Other homes, sibling prefixes, extra path components and custom-work mismatches stay excluded.

Eleven intercepted-process tests pass on Windows and Linux; the previous source fails the new cross-version and failed-query diagnostic cases. I restored the query diagnostic and original lock symptoms in the docstring, and removed the worklist row as suggested. Fresh CI is running.

Thanks, the cross-version case was missing. Fixed in `40321e87` on main `355453de`: default-work ownership accepts the exact packaged `versions/<version>/HookshotBuilder/tools/nx2/make_game.py` shape under that same home, then follows only its proven build child. Other homes, sibling prefixes, extra path components and custom-work mismatches stay excluded. Eleven intercepted-process tests pass on Windows and Linux; the previous source fails the new cross-version and failed-query diagnostic cases. I restored the query diagnostic and original lock symptoms in the docstring, and removed the worklist row as suggested. Fresh CI is running.

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

2026-11-11 21:23:09 +00:00

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

The cross-version case is fixed: only <home>\versions\<v>\HookshotBuilder\tools\nx2\make_game.py under the same home is accepted, and only a proven build.py child is followed. All 11 tests pass.

Trade-off to note: an orphaned build.py whose make_game is gone is now left running (make_game.py:904, :974). Its compilers still get stopped by their -o path, but it can start new ones. The process query also lists every process now, which is slower but stays inside the 90 s timeout.

The cross-version case is fixed: only `<home>\versions\<v>\HookshotBuilder\tools\nx2\make_game.py` under the same home is accepted, and only a proven `build.py` child is followed. All 11 tests pass. Trade-off to note: an orphaned `build.py` whose `make_game` is gone is now left running (`make_game.py:904`, `:974`). Its compilers still get stopped by their `-o` path, but it can start new ones. The process query also lists every process now, which is slower but stays inside the 90 s timeout.