fix(host): keep address regions reserved until discard completes

github-sync commented

2026-11-12 15:49:10 +00:00

Owner

@Arkex-ai opened this pull request on GitHub · 2026-11-12 15:33 UTC · original

Summary

Freeing an address region made its address available before the old pages finished being discarded. Another thread could allocate that address and then have its new payload cleared by the old discard. Freeing the lowest region also left a stale boundary that continued to restrict heap growth.

Fixes #2799. Draft: the source-level failures are reproduced; this head has not been run in the game. This does not claim a reduction in physical RAM or explain the reported multi-GiB memory pressure.

Technical changes

Keep a retiring range reserved until its discard callback returns, without holding the region lock through that callback; refind the entry after possible table relocation.
Exclude retiring entries from classification and enumeration, preserve enumeration's total-count/truncation contract, and recompute the remaining high-region boundary under the same lock as its readers.
Preserve empty and repeated finalization, low-hole allocation, existing page-discard/GPU-import policy, and the renderer interface.
Add a portable game-free regression using the actual allocator and reader functions with controlled commit/discard and native thread synchronization.

File overlaps reviewed: #2558 changes lazy heap commitment, not this lifecycle. #2478 adds another mapped-range reader; when combined, that reader must also exclude retiring entries. Neither implementation is duplicated here.

Validation

Current head: 45dfb5612c67c73613082838b5976137fea58e74. The review followup removes the unmeasured worklist row, leaving #2799 as the incidence record. It changes no host code or tests. The source/test measurements below were collected at c12c0242862beb2c4d10268265112d783438625b, based on 51da0464b4cf0c7f56b81ebd6bb309325835e175; those files remain byte-identical.

Windows, Clang 23.1.5 / llvm-mingw: the integrated regression passed 45 assertions in each of five runs. The unchanged base failed 14 behavioral assertions with the same fixture, not a compiler/setup failure. Cases include delayed discard/reallocation, callback reentry, table growth, live-range queries, holes below the heap, repeated and empty finalization, and 10,000 allocation/free cycles. The test also caught and rejected an earlier private candidate that stranded empty reservations. Strict compilation uses -Wall -Wextra -Werror.

The complete Windows game-free suite on these exact four file contents, one worker, finished 437 passed, 26 failed, 33 skipped. It is not green. Clean-base controls for those 26 failures are not yet complete, so they are not all attributed to the environment or described as pre-existing. The focused causal regression passes; hosted results are listed below, and the remaining local baseline controls are pending. Skips include platform, shader-tool and generated-game prerequisites.

The exact committed source and test files now pass three Linux GCC 14 runs, three Clang 17 runs and three Clang AddressSanitizer + UndefinedBehaviorSanitizer runs, with leak detection enabled. Each candidate run passes all 45 assertions; the same base control still fails 14 behavioral assertions. These are source-extracted CPU/native-thread fixtures, not Linux gameplay. The complete base and fixed hos_mem.c also pass strict syntax checking against the real host interface and retained local generic runtime headers; no linked game was produced. Full linked-game compilation and controlled AMD gameplay remain pending. No FPS or RAM-saving claim is made.

Historical hosted checks on c12c0242 (not the new documentation-only head): both Windows source shards and Linux source shard 1/2 passed, as did the Windows Vulkan build, Linux AppImage, SPIR-V fixtures, workflow lint, title and Builder package. Linux source shard 2/2 failed in test_release_scripts.py (test_nightly_two_runs_at_once_end_at_the_newer_commit, first=A): the fake release workflow reported HTTP 404 and mismatched checksums after retries. The address-region regression was not the failing check. An unchanged-base Linux control of that release-script test passed all 33 unit cases in one local run; that does not establish why the hosted failure occurred or turn the CI run green. Exact-head source CI, failed job.

Independent code review was received as COMMENTED, not approval. The requested in-game counters (frees, allocation skips while retirement is pending, and heap resizes enabled by a raised floor), followed by an uninstrumented run, remain unperformed; zero observed incidence must be reported as zero, not inferred from this fixture. Existing snapshot callers still need their own lifetime discipline, and the pre-existing heap-size check and subsequent heap mutation are not made a single transaction by this change.

Current-head hosted checks: Game-free source regressions, Linux AppImage, PR title, Windows builder. Their live conclusions are separate from local tests.

Hosted integration status observed at 18:09 UTC: the source jobs check synthetic merge 0b70dcc373b32c6f69ce659fcbf0a3cf221733c6 (45dfb561 plus main 65a4d874), not standalone 45dfb561. Linux 1/2 fails test_readme_menu_rows.py because the menu has Skip opening but the README table lacks it; Linux 2/2 fails test_program_cache_one_variant.py with undeclared disk_zstd, kZstdHead and program_cache_payload. The address-region regression passes in the latter log. These are different failures from the older concurrent-nightly run. They remain unresolved integration failures; no green CI or clean-main reproduction is claimed. AppImage and Windows Vulkan builds passed; other checks were still pending.

> [@Arkex-ai](https://github.com/Arkex-ai) opened this pull request on GitHub · 2026-11-12 15:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2823) ## Summary Freeing an address region made its address available before the old pages finished being discarded. Another thread could allocate that address and then have its new payload cleared by the old discard. Freeing the lowest region also left a stale boundary that continued to restrict heap growth. Fixes #2799. **Draft: the source-level failures are reproduced; this head has not been run in the game.** This does not claim a reduction in physical RAM or explain the reported multi-GiB memory pressure. ## Technical changes - Keep a retiring range reserved until its discard callback returns, without holding the region lock through that callback; refind the entry after possible table relocation. - Exclude retiring entries from classification and enumeration, preserve enumeration's total-count/truncation contract, and recompute the remaining high-region boundary under the same lock as its readers. - Preserve empty and repeated finalization, low-hole allocation, existing page-discard/GPU-import policy, and the renderer interface. - Add a portable game-free regression using the actual allocator and reader functions with controlled commit/discard and native thread synchronization. File overlaps reviewed: #2558 changes lazy heap commitment, not this lifecycle. #2478 adds another mapped-range reader; when combined, that reader must also exclude retiring entries. Neither implementation is duplicated here. ## Validation Current head: **`45dfb5612c67c73613082838b5976137fea58e74`**. The review followup removes the unmeasured worklist row, leaving #2799 as the incidence record. It changes no host code or tests. The source/test measurements below were collected at **`c12c0242862beb2c4d10268265112d783438625b`**, based on `51da0464b4cf0c7f56b81ebd6bb309325835e175`; those files remain byte-identical. Windows, Clang 23.1.5 / llvm-mingw: the integrated regression passed **45 assertions in each of five runs**. The unchanged base failed **14 behavioral assertions** with the same fixture, not a compiler/setup failure. Cases include delayed discard/reallocation, callback reentry, table growth, live-range queries, holes below the heap, repeated and empty finalization, and 10,000 allocation/free cycles. The test also caught and rejected an earlier private candidate that stranded empty reservations. Strict compilation uses `-Wall -Wextra -Werror`. The complete Windows game-free suite on these exact four file contents, one worker, finished **437 passed, 26 failed, 33 skipped**. It is **not green**. Clean-base controls for those 26 failures are not yet complete, so they are not all attributed to the environment or described as pre-existing. The focused causal regression passes; hosted results are listed below, and the remaining local baseline controls are pending. Skips include platform, shader-tool and generated-game prerequisites. The exact committed source and test files now pass **three Linux GCC 14 runs, three Clang 17 runs and three Clang AddressSanitizer + UndefinedBehaviorSanitizer runs**, with leak detection enabled. Each candidate run passes all 45 assertions; the same base control still fails 14 behavioral assertions. These are source-extracted CPU/native-thread fixtures, not Linux gameplay. The complete base and fixed `hos_mem.c` also pass strict syntax checking against the real host interface and retained local generic runtime headers; no linked game was produced. Full linked-game compilation and controlled AMD gameplay remain pending. No FPS or RAM-saving claim is made. Historical hosted checks on `c12c0242` (not the new documentation-only head): both Windows source shards and Linux source shard 1/2 passed, as did the Windows Vulkan build, Linux AppImage, SPIR-V fixtures, workflow lint, title and Builder package. **Linux source shard 2/2 failed** in `test_release_scripts.py` (`test_nightly_two_runs_at_once_end_at_the_newer_commit`, first=A): the fake release workflow reported HTTP 404 and mismatched checksums after retries. The address-region regression was not the failing check. An unchanged-base Linux control of that release-script test passed all 33 unit cases in one local run; that does not establish why the hosted failure occurred or turn the CI run green. [Exact-head source CI](https://github.com/NotRexed/Hookshot/actions/runs/43249026786), [failed job](https://github.com/NotRexed/Hookshot/actions/runs/43249026786/job/131289446554). Independent code review was received as COMMENTED, not approval. The requested in-game counters (frees, allocation skips while retirement is pending, and heap resizes enabled by a raised floor), followed by an uninstrumented run, remain unperformed; zero observed incidence must be reported as zero, not inferred from this fixture. Existing snapshot callers still need their own lifetime discipline, and the pre-existing heap-size check and subsequent heap mutation are not made a single transaction by this change. Current-head hosted checks: [Game-free source regressions](https://github.com/NotRexed/Hookshot/actions/runs/43272879711), [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/43272879610), [PR title](https://github.com/NotRexed/Hookshot/actions/runs/43272876052), [Windows builder](https://github.com/NotRexed/Hookshot/actions/runs/43272879755). Their live conclusions are separate from local tests. Hosted integration status observed at 18:09 UTC: the source jobs check synthetic merge `0b70dcc373b32c6f69ce659fcbf0a3cf221733c6` (45dfb561 plus main `65a4d874`), not standalone 45dfb561. Linux 1/2 fails `test_readme_menu_rows.py` because the menu has `Skip opening` but the README table lacks it; Linux 2/2 fails `test_program_cache_one_variant.py` with undeclared `disk_zstd`, `kZstdHead` and `program_cache_payload`. The address-region regression passes in the latter log. These are different failures from the older concurrent-nightly run. They remain unresolved integration failures; no green CI or clean-main reproduction is claimed. AppImage and Windows Vulkan builds passed; other checks were still pending.

github-sync added 1 commit

2026-11-12 15:49:10 +00:00

fix(host): keep address regions reserved until discard completes c12c024286

Keep retiring regions unavailable until their old pages have been discarded,
exclude them from live-range readers, and restore the remaining heap bound.
Preserve empty reservation finalization and existing discard/import policy.

Fixes #2799

Assisted-by: GPT-6 Astra Pro
Co-Authored-By: GPT-6 Astra Pro <[email protected]>

github-sync commented

2026-11-12 16:56:26 +00:00

Author

Owner

@Arkex-ai commented on GitHub · 2026-11-12 16:55 UTC · original

The exact-head allocator regression now also passes GCC, Clang and ASan/UBSan on Linux. The remaining hosted failure is test_release_scripts.py's concurrent nightly-publish case (first=A), with a mock release HTTP 404/checksum mismatch; the same unchanged-base test passed all 33 cases in one local control. I have not changed or weakened that test. Could a maintainer rerun the failed Linux source shard to check whether this is reproducible? The PR stays draft pending gameplay and CI.

Failed job: https://github.com/NotRexed/Hookshot/actions/runs/43249026786/job/131289446554

> [@Arkex-ai](https://github.com/Arkex-ai) commented on GitHub · 2026-11-12 16:55 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2823#issuecomment-6830135879) The exact-head allocator regression now also passes GCC, Clang and ASan/UBSan on Linux. The remaining hosted failure is test_release_scripts.py's concurrent nightly-publish case (first=A), with a mock release HTTP 404/checksum mismatch; the same unchanged-base test passed all 33 cases in one local control. I have not changed or weakened that test. Could a maintainer rerun the failed Linux source shard to check whether this is reproducible? The PR stays draft pending gameplay and CI. Failed job: https://github.com/NotRexed/Hookshot/actions/runs/43249026786/job/131289446554

github-sync referenced this pull request

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

fix(host): retain linked synchronization objects for later reclamation #2858

github-sync referenced this pull request from a commit

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

docs: leave address-region incidence tracking in its issue

github-sync added 1 commit

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

docs: leave address-region incidence tracking in its issue 45dfb5612c

Address review on #2823. Keep gameplay and reach unmeasured until
local hardware validation; restore the unrelated worklist bytes.

Assisted-by: GPT-6 Astra Pro
Co-Authored-By: GPT-6 Astra Pro <[email protected]>

github-sync referenced this pull request

2026-11-12 17:59:26 +00:00

Address regions can be reused before their old pages finish discarding #2799

github-sync commented

2026-11-12 17:59:36 +00:00

Author

Owner

@hckrman101 commented on host/hos_mem.c line 226 on GitHub · 2026-11-12 17:25 UTC · original

This is the part the game can actually see. The floor used to only go down, so a heap resize after the lowest region was freed got refused. Now it can succeed. That's the intent, and it's correct (holes below the heap are left out, as in the allocator). Please log it in the in-game run, so we know whether the game ever does this and that it behaves when it does.

> [@hckrman101](https://github.com/hckrman101) commented on `host/hos_mem.c` line 226 on GitHub · 2026-11-12 17:25 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2823#discussion_r4553922130) This is the part the game can actually see. The floor used to only go down, so a heap resize after the lowest region was freed got refused. Now it can succeed. That's the intent, and it's correct (holes below the heap are left out, as in the allocator). Please log it in the in-game run, so we know whether the game ever does this and that it behaves when it does.

github-sync commented

2026-11-12 17:59:36 +00:00

Author

Owner

@hckrman101 commented on docs/worklist.md line 28 on GitHub · 2026-11-12 17:25 UTC · original

Nit: once this PR merges, this row describes something already fixed. Either leave the row out and keep #2799 open until the in-game check is done, or point the row at this PR as the candidate fix, the way the other rows list theirs. Also, if the in-game counter shows the race never happens, S0 x 0.4 is generous for a race nobody has seen in the game.

> [@hckrman101](https://github.com/hckrman101) commented on `docs/worklist.md` line 28 on GitHub · 2026-11-12 17:25 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2823#discussion_r4553922145) Nit: once this PR merges, this row describes something already fixed. Either leave the row out and keep #2799 open until the in-game check is done, or point the row at this PR as the candidate fix, the way the other rows list theirs. Also, if the in-game counter shows the race never happens, S0 x 0.4 is generous for a race nobody has seen in the game.

github-sync commented

2026-11-12 17:59:36 +00:00

Author

Owner

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

Thanks, this is a small, careful fix and the logic checks out. Here's what I went through:

Reuse while discarding: fixed. Allocation's first-fit still treats a releasing entry as taken, so nobody else can get that address until the discard returns. After relocking, the entry is looked up again by address, which handles the table being reallocated or shifted in the meantime. The discard still runs outside the lock. It has to, because the Linux path asks the renderer whether the pages are imported.
Released twice: can't happen. A second or concurrent free of the same address skips the releasing entry and returns, so only one discard runs.
Windows and Linux: the change is in the bookkeeping above the per-platform discard, so it covers both. On Windows, DiscardVirtualMemory/MEM_RESET leave the contents undefined, and on Linux, MADV_REMOVE or the zeroing of imported pages wipes them. Either way the new owner's data was at risk, and both are covered. The large-pages and HOS_KEEP_FREED_PAGES early returns are unchanged.
Address space: an entry is held only for the length of one discard call, so nothing builds up over a session. The 10,000-cycle churn case covers the table size too.
GPU imports: this path is unchanged; rx_guest_imported is still asked at discard time. #2795 changed when the Vulkan side gives back a retired pool's import and doesn't touch this code.
Test: it uses the real allocator and readers, with a discard stub that blocks, so it reproduces the real ordering at the bookkeeping level. The platform discard is stubbed out, which is fine for a check that runs without the game.

To take it out of draft (rule 3c, #2712): the bug needs to be shown in the game, not only in the test. A temporary log line is enough. Over a normal session, count the address-region frees, the allocations that had to skip a releasing entry (the race actually being hit), and any heap resize that succeeds only because the floor went back up. Then run this head with the log removed and report no regressions, with the GPU, driver and OS named and a row in tested-configurations. If the race counter stays at zero over a long session, please say so. The change is still worth having as hardening, but then the worklist score should drop (see the inline note).

CI: the only failure is the concurrent nightly case in test_release_scripts.py on ubuntu part 2/2. This diff doesn't touch the release scripts, and test_address_regions.py passes on both platforms, so I'd expect a rerun to clear it. There are no merge conflicts.

Overlaps: #2478 adds another reader of the region table, and it will need to skip releasing entries, as you noted. #2558 (lazy heap commit) now has to handle heap growth into space that used to belong to an address region. Since that space has already been discarded, it should just work. This PR is the smaller one, so I'd land it first and rebase both drafts onto it.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 17:25 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2823#pullrequestreview-5879009449) Thanks, this is a small, careful fix and the logic checks out. Here's what I went through: - **Reuse while discarding:** fixed. Allocation's first-fit still treats a `releasing` entry as taken, so nobody else can get that address until the discard returns. After relocking, the entry is looked up again by address, which handles the table being reallocated or shifted in the meantime. The discard still runs outside the lock. It has to, because the Linux path asks the renderer whether the pages are imported. - **Released twice:** can't happen. A second or concurrent free of the same address skips the `releasing` entry and returns, so only one discard runs. - **Windows and Linux:** the change is in the bookkeeping above the per-platform discard, so it covers both. On Windows, `DiscardVirtualMemory`/`MEM_RESET` leave the contents undefined, and on Linux, `MADV_REMOVE` or the zeroing of imported pages wipes them. Either way the new owner's data was at risk, and both are covered. The large-pages and `HOS_KEEP_FREED_PAGES` early returns are unchanged. - **Address space:** an entry is held only for the length of one discard call, so nothing builds up over a session. The 10,000-cycle churn case covers the table size too. - **GPU imports:** this path is unchanged; `rx_guest_imported` is still asked at discard time. #2795 changed when the Vulkan side gives back a retired pool's import and doesn't touch this code. - **Test:** it uses the real allocator and readers, with a discard stub that blocks, so it reproduces the real ordering at the bookkeeping level. The platform discard is stubbed out, which is fine for a check that runs without the game. **To take it out of draft (rule 3c, #2712):** the bug needs to be shown in the game, not only in the test. A temporary log line is enough. Over a normal session, count the address-region frees, the allocations that had to skip a `releasing` entry (the race actually being hit), and any heap resize that succeeds only because the floor went back up. Then run this head with the log removed and report no regressions, with the GPU, driver and OS named and a row in tested-configurations. If the race counter stays at zero over a long session, please say so. The change is still worth having as hardening, but then the worklist score should drop (see the inline note). **CI:** the only failure is the concurrent nightly case in `test_release_scripts.py` on ubuntu part 2/2. This diff doesn't touch the release scripts, and `test_address_regions.py` passes on both platforms, so I'd expect a rerun to clear it. There are no merge conflicts. **Overlaps:** #2478 adds another reader of the region table, and it will need to skip `releasing` entries, as you noted. #2558 (lazy heap commit) now has to handle heap growth into space that used to belong to an address region. Since that space has already been discarded, it should just work. This PR is the smaller one, so I'd land it first and rebase both drafts onto it.

github-sync commented

2026-11-12 17:59:36 +00:00

Author

Owner

@Arkex-ai commented on host/hos_mem.c line 226 on GitHub · 2026-11-12 17:57 UTC · original

The validation boundary remains explicit: no game run has established how often these paths are reached. The PR stays draft. The required temporary observations are frees, allocation skips while a range is retiring, and heap resizes admitted only by the raised floor, followed by an uninstrumented run. None of those counters has been measured yet, so the allocator fixture is not presented as a player fix or a RAM-saving result.

> [@Arkex-ai](https://github.com/Arkex-ai) commented on `host/hos_mem.c` line 226 on GitHub · 2026-11-12 17:57 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2823#discussion_r4554273553) The validation boundary remains explicit: no game run has established how often these paths are reached. The PR stays draft. The required temporary observations are frees, allocation skips while a range is retiring, and heap resizes admitted only by the raised floor, followed by an uninstrumented run. None of those counters has been measured yet, so the allocator fixture is not presented as a player fix or a RAM-saving result.

github-sync commented

2026-11-12 17:59:36 +00:00

Author

Owner

@Arkex-ai commented on docs/worklist.md line 28 on GitHub · 2026-11-12 17:57 UTC · original

Removed the speculative worklist row in 45dfb561. The file now matches the PR base byte-for-byte; #2799 remains the incidence record and is unranked until a game run establishes reach/impact. This followup changes no host code or tests.

> [@Arkex-ai](https://github.com/Arkex-ai) commented on `docs/worklist.md` line 28 on GitHub · 2026-11-12 17:57 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2823#discussion_r4554273803) Removed the speculative worklist row in 45dfb561. The file now matches the PR base byte-for-byte; #2799 remains the incidence record and is unranked until a game run establishes reach/impact. This followup changes no host code or tests.

Rows
Columns

fix(host): keep address regions reserved until discard completes #2823

Summary

Technical changes

Validation