fix(vk): publish recording stream writes before GPU submission #1568

Merged
Arkex-ai merged 3 commits from fix/draw-stream-visibility into main 2026-11-10 16:50:17 +00:00
Arkex-ai commented 2026-11-10 08:52:00 +00:00 (Migrated from github.com)

CPU-written recording streams may use noncoherent memory. Without flushing before queue submission, draws can see stale uniforms, vertex/index conversions or buffer copies, including earlier streams retained after growth.

Flush each recording slot's used streams through the existing VMA helper before serial/batched and successful parallel submission. Earlier used streams flush their capacity; the current stream flushes its used prefix. Coherent allocations remain a no-op. Allocation, completion/reuse and the renderer API are unchanged.

Production-code fixtures cover growth, retained-stream reuse, empty recordings, batching, worker slots and failed initialization/allocation in coherent and noncoherent modes. Main fails seven noncoherent consumers; six behavioral mutations fail. Windows LLVM and Linux GCC/ASan/UBSan pass. Tests splice complete production functions and therefore require maintenance if those markers change.

Historical integration at 5d9c8b1 / main be541fa: complete suite229 passed/3 failed/20 skipped vs baseline228/3/20 with identical local limitations; official matching API12 build; native FG2 serial and two-thread parallel checkpoint/resize runs exited0, with five decoded3440x1440 captures and preserved original saves. Current-head fork checks are below; those historical game runs are not relabeled as current runtime.

The NVIDIA run proves healthy compatibility, not native noncoherent behavior, stream growth, campaign coverage or performance. Global fallback buffers and mixed CPU/GPU argument rings are outside scope. See docs/draw-stream-visibility.md. No game data or build output is included.

Current-head validation

Current head d264ff7, 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.

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

CPU-written recording streams may use noncoherent memory. Without flushing before queue submission, draws can see stale uniforms, vertex/index conversions or buffer copies, including earlier streams retained after growth. Flush each recording slot's used streams through the existing VMA helper before serial/batched and successful parallel submission. Earlier used streams flush their capacity; the current stream flushes its used prefix. Coherent allocations remain a no-op. Allocation, completion/reuse and the renderer API are unchanged. Production-code fixtures cover growth, retained-stream reuse, empty recordings, batching, worker slots and failed initialization/allocation in coherent and noncoherent modes. Main fails seven noncoherent consumers; six behavioral mutations fail. Windows LLVM and Linux GCC/ASan/UBSan pass. Tests splice complete production functions and therefore require maintenance if those markers change. Historical integration at `5d9c8b1` / main `be541fa`: complete suite229 passed/3 failed/20 skipped vs baseline228/3/20 with identical local limitations; official matching API12 build; native FG2 serial and two-thread parallel checkpoint/resize runs exited0, with five decoded3440x1440 captures and preserved original saves. Current-head fork checks are below; those historical game runs are not relabeled as current runtime. The NVIDIA run proves healthy compatibility, not native noncoherent behavior, stream growth, campaign coverage or performance. Global fallback buffers and mixed CPU/GPU argument rings are outside scope. See `docs/draw-stream-visibility.md`. No game data or build output is included. <!-- goal15-current-validation --> ### Current-head validation Current head `d264ff7`, 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/42858214901): in_progress. - [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42858204051): in_progress. - [Game-free source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42858198825): in_progress. 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:38:41 +00:00
hckrman101 (Migrated from github.com) left a comment

Looks good. The fix is small and correct: submit_recording and the parallel segment submit both flush the slot's used streams (earlier streams in full, current stream up to used) through the existing flush_mapped helper before the queue sees them, and failed/lost-init segments are skipped. flush_mapped handles coherent memory already, so no cost on coherent hardware. The new fixture is registered in run_all.py, the parallel_init_order extension is sensible, and CI is green.

Nits only: the fixture splices production source by text markers (block(draw, "...")), so it will need touching if those functions get renamed; and the PR description is long and carries a few stale head sha notes, a short summary would age better.

Looks good. The fix is small and correct: submit_recording and the parallel segment submit both flush the slot's used streams (earlier streams in full, current stream up to `used`) through the existing flush_mapped helper before the queue sees them, and failed/lost-init segments are skipped. flush_mapped handles coherent memory already, so no cost on coherent hardware. The new fixture is registered in run_all.py, the parallel_init_order extension is sensible, and CI is green. Nits only: the fixture splices production source by text markers (`block(draw, "...")`), so it will need touching if those functions get renamed; and the PR description is long and carries a few stale head sha notes, a short summary would age better.
Commenting is not possible because the repository is archived.
No description provided.