fix(vk): recover recording stream and descriptor allocation failures #1658

Merged

Arkex-ai merged 3 commits from fix/recording-admission into main

2026-11-10 17:05:51 +00:00

Arkex-ai commented

2026-11-10 12:37:29 +00:00

(Migrated from github.com)

This change publishes streams and extra descriptor pools only after allocation, mapping/set creation and container ownership succeed. Failures preserve prior resources and allow retry; reservations reject missing mappings and arithmetic overflow. Bank-cache metadata commits after a completed upload or a verified identical-byte hit. Successful growth, retention, push-descriptor and cache-visibility policies are preserved.

Validation:

The game-free regression covers 23 cases using the actual stream/reset, bank-cache and descriptor-allocation paths with authored failures and retries. These model allocation failures; they do not establish native driver-OOM or device-loss recovery.
Exact head 6a51fc2e82a8b1e86acb063b4d91b23d8269f7fa: Windows builder passed; source regressions and Linux AppImage are in progress. Fork runs do not bypass upstream approval.
Earlier local and healthy native-checkpoint results remain tied to their recorded source/artifact hashes; they are not exact-head runtime validation. No campaign or performance guarantee is claimed.

Implementation, test scope and references: docs/recording-admission.md. No game-derived files or build output are included.

Current-head validation

Current head 6a51fc2, based on main 7047756. Earlier local/runtime results describe their recorded source hashes.

The standalone contribution patch is unchanged after the audited main refresh; exact-head build/source checks are refreshed below. Earlier runtime evidence remains tied to its recorded hash.

Linux AppImage: in_progress.
Windows builder: success.
Game-free source regressions: failure.

Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

Failed recording-stream or descriptor allocations can leave incomplete resources usable, leak new resources, or prevent a clean retry. Overflowing reservations and premature bank-cache metadata can also authorize invalid writes or reuse stale bytes. This change publishes streams and extra descriptor pools only after allocation, mapping/set creation and container ownership succeed. Failures preserve prior resources and allow retry; reservations reject missing mappings and arithmetic overflow. Bank-cache metadata commits after a completed upload or a verified identical-byte hit. Successful growth, retention, push-descriptor and cache-visibility policies are preserved. Validation: - The game-free regression covers 23 cases using the actual stream/reset, bank-cache and descriptor-allocation paths with authored failures and retries. These model allocation failures; they do not establish native driver-OOM or device-loss recovery. - Exact head `6a51fc2e82a8b1e86acb063b4d91b23d8269f7fa`: [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42858312326) passed; [source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42858306868) and [Linux AppImage](https://github.com/Arkex-ai/Hookshot/actions/runs/42858317715) are in progress. Fork runs do not bypass upstream approval. - Earlier local and healthy native-checkpoint results remain tied to their recorded source/artifact hashes; they are not exact-head runtime validation. No campaign or performance guarantee is claimed. Implementation, test scope and references: [docs/recording-admission.md](https://github.com/Arkex-ai/Hookshot/blob/6a51fc2e82a8b1e86acb063b4d91b23d8269f7fa/docs/recording-admission.md). No game-derived files or build output are included.  ### Current-head validation Current head `6a51fc2`, based on main `7047756`. Earlier local/runtime results describe their recorded source hashes. The standalone contribution patch is unchanged after the audited main refresh; exact-head build/source checks are refreshed below. Earlier runtime evidence remains tied to its recorded hash. - [Linux AppImage](https://github.com/Arkex-ai/Hookshot/actions/runs/42858317715): in_progress. - [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42858312326): success. - [Game-free source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42858306868): failure. Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 16:40:17 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good. I read the vk_draw.cpp changes and they match the description:

Initial stream creation now goes through stream_insert, which only publishes the stream after buffer, allocation, mapping and the container insert all succeed. Failed growth no longer leaves an unmapped stream behind or advances si.
stream_reserve is overflow-safe and refuses to run with a missing stream or mapping. A recording whose first stream failed just gets no room, and the next draw_begin_recording retries it.
make_pool now checks the result. bind_descriptors no longer calls Vulkan with a null pool, and an extra pool is only kept once its first set allocation and the push_back both succeed.
The bank cache now sets bind_gen and grid only after a verified identical-byte hit or a completed upload. The growth size formula is equivalent to before.

The new test is registered in run_all.py with the same args as the framelog test, and CI is green.

Nit: the PR body is long and carries a few stale validation sections. It's worth trimming before merge.

Looks good. I read the vk_draw.cpp changes and they match the description: - Initial stream creation now goes through `stream_insert`, which only publishes the stream after buffer, allocation, mapping and the container insert all succeed. Failed growth no longer leaves an unmapped stream behind or advances `si`. - `stream_reserve` is overflow-safe and refuses to run with a missing stream or mapping. A recording whose first stream failed just gets no room, and the next `draw_begin_recording` retries it. - `make_pool` now checks the result. `bind_descriptors` no longer calls Vulkan with a null pool, and an extra pool is only kept once its first set allocation and the push_back both succeed. - The bank cache now sets `bind_gen` and `grid` only after a verified identical-byte hit or a completed upload. The growth size formula is equivalent to before. The new test is registered in run_all.py with the same args as the framelog test, and CI is green. Nit: the PR body is long and carries a few stale validation sections. It's worth trimming before merge.

Arkex-ai commented

2026-11-10 16:57:12 +00:00

(Migrated from github.com)

Trimmed the PR description and removed the stale validation sections. It now separates the 23-case modeled-failure coverage from earlier native results, which remain tied to their recorded hashes.

No code changes were needed for this nit; the head remains 6a51fc2e82a8b1e86acb063b4d91b23d8269f7fa. The exact-head Windows builder passed (42858312326); source regressions (42858306868) and AppImage (42858317715) are still in progress. Native driver-OOM/device-loss recovery is not claimed.

Trimmed the PR description and removed the stale validation sections. It now separates the 23-case modeled-failure coverage from earlier native results, which remain tied to their recorded hashes. No code changes were needed for this nit; the head remains `6a51fc2e82a8b1e86acb063b4d91b23d8269f7fa`. The exact-head Windows builder passed ([42858312326](https://github.com/Arkex-ai/Hookshot/actions/runs/42858312326)); source regressions ([42858306868](https://github.com/Arkex-ai/Hookshot/actions/runs/42858306868)) and AppImage ([42858317715](https://github.com/Arkex-ai/Hookshot/actions/runs/42858317715)) are still in progress. Native driver-OOM/device-loss recovery is not claimed.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101