fix: release discarded Vulkan resources and thread wake events #2358

Merged

SRhartty merged 2 commits from fix/resource-cleanup-leaks into main

2026-11-11 18:18:43 +00:00

SRhartty commented

2026-11-11 15:49:22 +00:00

(Migrated from github.com)

Changes:

Destroy stage shader modules before returning from a failed seed variant; retain successful cached variants and existing duplicate cleanup.
Destroy a newly built graphics pipeline when another entry already owns its cache key, preserving the existing pipeline and readiness state.
Close the wake event in hos_thread_destroy before freeing the joined thread.

Validation:

Added three game-free regression drivers executing extracted production code. They cover partial graphics/mesh translation failures, successful and duplicate variants, ready/pending/null pipeline collisions, a delayed worker losing publication, and 10,000 thread lifecycles with close ordering and null handles.
Each regression failed before its corresponding fix; all three pass with LLVM-MinGW clang++ and GCC on Windows.
Independent combined code review found no actionable issues. git diff --check and tracked Python byte-compilation pass.
Full local run: 337 passed, 9 failed, 31 skipped. The fpx_sticky check subsequently passed with HOOKSHOT_MINGW configured. draw_stream_visibility passes on isolated rerun.
An untouched source export of base 49f75dc8 reproduced the remaining local blockers: missing .NET SDK (updater_asset_digest, builder_details_hitbox, builder_dlss_update), missing zstandard (nup_read_name, bake_portability), Windows symlink privileges (nup_paths), missing MSVC-target windows.h (precompile_compute_guard), and the recorder_sync negative control accepting the old destructor. Its archive-only byte-compilation failure is due to lacking .git; that check passes in the actual checkout. The export also encountered a temporary executable deletion error in draw_stream_visibility.

Kept as a draft while CI and gameplay validation are pending. No game run, GPU/driver memory measurement, Linux runtime test, frame-rate improvement, or device-loss fix is claimed. No renderer API or generated shader changes, and no game files are included. There is no visible UI change.

Follow-up local validation:

Installed an isolated Python test environment with zstandard 0.25.0 and the .NET 8.0.428 SDK, plus the authorized Visual Studio 2022 Build Tools (MSVC, Clang 19.1.5 and Windows SDK 22621).
All nine originally failing checks now pass in targeted reruns: the two Python dependency checks, three .NET checks, nup_paths (24 cases, elevated because Windows requires symlink privileges), precompile_compute_guard, recorder_sync including all negative controls, and fpx_sticky.
The three new regression drivers also pass with Clang/MSVC. This is targeted revalidation; the complete local suite has not been rerun after environment setup.
Both Windows source-check shards and the Windows Vulkan build passed in CI. All three new regression drivers also passed in the Linux source-check logs. The Linux source jobs failed because DNS resolution of azure.archive.ubuntu.com prevented apt from installing dependencies (spirv-val/ccache and related tools); see https://github.com/NotRexed/Hookshot/actions/runs/43052172769 . A rerun request was rejected because it requires repository admin rights.
Gameplay/GPU memory validation is still pending.

Latest complete local validation:

Full game-free suite: 348 passed, 0 failed, 29 skipped in 341 seconds on Windows, 6 checks at once; process exit 0. This supersedes the earlier local failure counts.
The local environment uses GCC for C fixtures (which use GNU flags such as -lm) and Clang/MSVC for C++/SEH checks, with the .NET and Python dependencies above. The suite ran elevated for Windows symlink cases.
All three checks from the subsequent user run (program_cache_cleanup, hd_rumble, rumble_mapping) pass in this complete run. The temporary-executable deletion error did not recur.
This required only local environment setup; no additional project source changes. Gameplay validation and the Linux CI rerun after its DNS failure remain pending.

Failed shader preparation, overlapping pipeline precompilation, and repeated thread creation can leave Vulkan objects or wake events allocated until the process exits. This releases those resources when their temporary owners are discarded. Changes: - Destroy stage shader modules before returning from a failed seed variant; retain successful cached variants and existing duplicate cleanup. - Destroy a newly built graphics pipeline when another entry already owns its cache key, preserving the existing pipeline and readiness state. - Close the wake event in hos_thread_destroy before freeing the joined thread. Validation: - Added three game-free regression drivers executing extracted production code. They cover partial graphics/mesh translation failures, successful and duplicate variants, ready/pending/null pipeline collisions, a delayed worker losing publication, and 10,000 thread lifecycles with close ordering and null handles. - Each regression failed before its corresponding fix; all three pass with LLVM-MinGW clang++ and GCC on Windows. - Independent combined code review found no actionable issues. git diff --check and tracked Python byte-compilation pass. - Full local run: 337 passed, 9 failed, 31 skipped. The fpx_sticky check subsequently passed with HOOKSHOT_MINGW configured. draw_stream_visibility passes on isolated rerun. - An untouched source export of base 49f75dc8 reproduced the remaining local blockers: missing .NET SDK (updater_asset_digest, builder_details_hitbox, builder_dlss_update), missing zstandard (nup_read_name, bake_portability), Windows symlink privileges (nup_paths), missing MSVC-target windows.h (precompile_compute_guard), and the recorder_sync negative control accepting the old destructor. Its archive-only byte-compilation failure is due to lacking .git; that check passes in the actual checkout. The export also encountered a temporary executable deletion error in draw_stream_visibility. Kept as a draft while CI and gameplay validation are pending. No game run, GPU/driver memory measurement, Linux runtime test, frame-rate improvement, or device-loss fix is claimed. No renderer API or generated shader changes, and no game files are included. There is no visible UI change. Follow-up local validation: - Installed an isolated Python test environment with zstandard 0.25.0 and the .NET 8.0.428 SDK, plus the authorized Visual Studio 2022 Build Tools (MSVC, Clang 19.1.5 and Windows SDK 22621). - All nine originally failing checks now pass in targeted reruns: the two Python dependency checks, three .NET checks, nup_paths (24 cases, elevated because Windows requires symlink privileges), precompile_compute_guard, recorder_sync including all negative controls, and fpx_sticky. - The three new regression drivers also pass with Clang/MSVC. This is targeted revalidation; the complete local suite has not been rerun after environment setup. - Both Windows source-check shards and the Windows Vulkan build passed in CI. All three new regression drivers also passed in the Linux source-check logs. The Linux source jobs failed because DNS resolution of azure.archive.ubuntu.com prevented apt from installing dependencies (spirv-val/ccache and related tools); see https://github.com/NotRexed/Hookshot/actions/runs/43052172769 . A rerun request was rejected because it requires repository admin rights. - Gameplay/GPU memory validation is still pending. Latest complete local validation: - Full game-free suite: 348 passed, 0 failed, 29 skipped in 341 seconds on Windows, 6 checks at once; process exit 0. This supersedes the earlier local failure counts. - The local environment uses GCC for C fixtures (which use GNU flags such as -lm) and Clang/MSVC for C++/SEH checks, with the .NET and Python dependencies above. The suite ran elevated for Windows symlink cases. - All three checks from the subsequent user run (program_cache_cleanup, hd_rumble, rumble_mapping) pass in this complete run. The temporary-executable deletion error did not recur. - This required only local environment setup; no additional project source changes. Gameplay validation and the Linux CI rerun after its DNS failure remain pending.

SRhartty (Migrated from github.com) reviewed

2026-11-11 17:24:31 +00:00

SRhartty (Migrated from github.com) left a comment

Local validation: 348 passed, 0 failed, 29 skipped. Windows CI passed. Linux CI needs a rerun after dependency downloads failed due to DNS resolution errors. Gameplay and GPU memory measurements are still pending.

Ready for review. This PR fixes three resource leaks: shader modules left behind after failed seed translation, unpublished pipelines discarded after cache collisions, and thread wake events left open during destruction. Local validation: 348 passed, 0 failed, 29 skipped. Windows CI passed. Linux CI needs a rerun after dependency downloads failed due to DNS resolution errors. Gameplay and GPU memory measurements are still pending.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 17:36:19 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks, this is a clean, well-scoped fix. I went through all three changes:

Seed variant failure (vk_shader.cpp): on failure translate_into / translate_mesh_into leave the modules they made in p->stages (the catch only sets failed), and the Program dies here without having been published or used by a pipeline. So destroying them is correct, nothing else holds them, and there's no double destroy. It matches the duplicate-variant branch below and the drop lambda in the compute seeding.
Precompile collision (vk_draw.cpp): with part = -1, build_graphics returns a fresh pipeline that nothing else references, and it's destroyed under note_mu before any command buffer could see it, so there's no GPU-lifetime concern. Same pattern as the abandoned compute branch just above.
Wake event (hos_thread.c): the only caller is DestroyThread, which joins first, and the only SetEvent callers (hos_svc.c) wake threads taken from the waiter table under klock, which an exited thread can't be in. So once the join returns, nothing can wait on or signal t->wake. The posix shim's CloseHandle frees H_EVENT objects properly, so Linux behaves the same as Windows.

The touched code is unchanged on current main (the branch is 33 behind but has no conflicts).

Nits only, none blocking:

test_thread_wake_cleanup.py requires the wake event to be closed after the thread handle ("wake closed before thread handle"). That order doesn't matter for correctness, so the check would fail a harmless reorder. I'd drop that assert.
Same test: RUN_ALL_ARGS = ["--cc", "{cxx}"] builds C with the C++ driver through -x c. That works with clang++ and g++, but the name is a bit misleading.
Possible follow-up: when play's entry already exists with a null pipeline (its build failed), the precompiled pipeline that did build is thrown away rather than used. That isn't a regression, since before this PR it was simply leaked.

CI is re-running now (the earlier Linux jobs failed on apt DNS, and Windows passed). It should be green before merge.

Thanks, this is a clean, well-scoped fix. I went through all three changes: - **Seed variant failure** (`vk_shader.cpp`): on failure `translate_into` / `translate_mesh_into` leave the modules they made in `p->stages` (the catch only sets `failed`), and the `Program` dies here without having been published or used by a pipeline. So destroying them is correct, nothing else holds them, and there's no double destroy. It matches the duplicate-variant branch below and the `drop` lambda in the compute seeding. - **Precompile collision** (`vk_draw.cpp`): with `part = -1`, `build_graphics` returns a fresh pipeline that nothing else references, and it's destroyed under `note_mu` before any command buffer could see it, so there's no GPU-lifetime concern. Same pattern as the abandoned compute branch just above. - **Wake event** (`hos_thread.c`): the only caller is `DestroyThread`, which joins first, and the only `SetEvent` callers (`hos_svc.c`) wake threads taken from the waiter table under `klock`, which an exited thread can't be in. So once the join returns, nothing can wait on or signal `t->wake`. The posix shim's `CloseHandle` frees `H_EVENT` objects properly, so Linux behaves the same as Windows. The touched code is unchanged on current main (the branch is 33 behind but has no conflicts). Nits only, none blocking: - `test_thread_wake_cleanup.py` requires the wake event to be closed *after* the thread handle ("wake closed before thread handle"). That order doesn't matter for correctness, so the check would fail a harmless reorder. I'd drop that assert. - Same test: `RUN_ALL_ARGS = ["--cc", "{cxx}"]` builds C with the C++ driver through `-x c`. That works with clang++ and g++, but the name is a bit misleading. - Possible follow-up: when play's entry already exists with a null pipeline (its build failed), the precompiled pipeline that did build is thrown away rather than used. That isn't a regression, since before this PR it was simply leaked. CI is re-running now (the earlier Linux jobs failed on apt DNS, and Windows passed). It should be green before merge.