Skip to content

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

Merged
hckrman101 merged 2 commits into
NotRexed:mainfrom
Arkex-ai:fix/build-cleanup-scope
Nov 11, 2026
Merged

hckrman101 merged 2 commits into
NotRexed:mainfrom
Arkex-ai:fix/build-cleanup-scope

Conversation

@Arkex-ai

@Arkex-ai Arkex-ai commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

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

@Joey-Magnus Joey-Magnus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Require parsed work/output ownership, preserve unknown orphans and ancestors, and avoid terminating unexamined child trees.

Assisted-by: OpenAI Codex
@Arkex-ai
Arkex-ai force-pushed the fix/build-cleanup-scope branch from 036ac8b to 40321e8 Compare November 11, 2026 20:24
@Arkex-ai

Copy link
Copy Markdown
Contributor Author

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 Joey-Magnus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hckrman101
hckrman101 merged commit 20f891e into NotRexed:main Nov 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows build cleanup can select processes outside the requested work folder

3 participants