fix(vk): admit only successful samplers across cache layers #1754

Merged

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

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

Arkex-ai commented

2026-11-10 15:39:36 +00:00

(Migrated from github.com)

Admit only successful handles through every cache layer and validate every combined-image-sampler array element before either pushed-state mutation or pooled-set allocation. Existing draw, compute and mesh-fallback callers already honor binding failure. Bound failed creation per parameter key/present and retry on later presents without a registration change. Owning insertion failure destroys only the new sampler; acceleration-cache allocation failure preserves registered parameters and globally owned handles. Failed failure-tracking allocation postpones new keys until the next present while valid hits remain available.

Filtering, LOD/compare/border/reduction/anisotropy policies, successful sampler lifetimes, registration checks, replay locking and API12 are unchanged. Storage images still accept their unused null sampler. No replacement sampler silently changes the requested filtering policy.

Validation at 10a2f7f6b25464fdfcc85ef4109ffd7e3f802e9f, based on main caaee0ba6aef97514132603c9e6fa9e58f99bf57:

108 execution cases run the actual complete factory, ID-cache consumer, policy helpers, descriptor binder and fingerprint helpers with authored Vulkan/registration/environment/hash/lock-boundary inputs. Windows LLVM/MinGW, Linux GCC and ASan/UBSan pass. 16 behavioral mutants are rejected. Cases cover ownership/acceleration/failure-metadata allocation faults, registration changes, threads, unchanged policies, cold/warm failure in an array's second element, both pushed-state layouts/full pushes, pooled sets, graphics/compute, recovery and storage images. Rejected writes produce no allocation/update/bind/push and leave the prior fingerprint intact.
Full local suite: 248 passed / 2 failed / 20 skipped, versus exact main 247 / 2 / 20. Both failures are unchanged local environment profiles: NUP symlink privilege and missing NGX SDK guard. The shared staging fixture repair is now accepted in #1741 and has been dropped from this diff. No assertion, warning or CI check is disabled.
Fresh official generation completed successfully with zero NULs in generated C/headers. Matching API12 host/backend build passed. Final-head RTX 5070 Ti healthy native validation passed with fixed FG 2x: serial and parallel/batch recording, a client-size roundtrip from 1444x720 to 1440x829 and back, six representative PNGs validated at 3440x1440, and both exits0. Original checkpoint hashes are unchanged. Earlier runs at cbe49eb remain historical; the tested-configuration row names that observed build, hardware, driver, checkpoint and limits.

Fresh actual-patch overlap review distinguishes #1654's scene LOD bias/additional biased ID table, #1541's writer/diagnostic refactor, #1366's filter controls, and #1653/#1008's provider-specific resources. This PR implements none of those features. If the sampler signature/table changes first, preserve positive-only admission in every added table and update the source fixture during rebase. No code was copied from those PRs.

The failure models do not establish native GPU OOM/device-loss recovery, image-quality or performance improvements, or campaign coverage. Only authored source, tests and documentation are included; no generated game code, dumps, captures or saves are published.

Current-head fork workflows are pending:

See docs/sampler-admission.md for the contract and model boundaries.

Current-head validation

Current head efe9837, 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: in_progress.
Game-free source regressions: queued.

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

Failed sampler creation can permanently poison the ID, thread-local and global caches, or return an undefined error-output handle. Returning null alone is insufficient: the final descriptor consumer must also decline the operation instead of passing a failed sampler to Vulkan. Admit only successful handles through every cache layer and validate every combined-image-sampler array element before either pushed-state mutation or pooled-set allocation. Existing draw, compute and mesh-fallback callers already honor binding failure. Bound failed creation per parameter key/present and retry on later presents without a registration change. Owning insertion failure destroys only the new sampler; acceleration-cache allocation failure preserves registered parameters and globally owned handles. Failed failure-tracking allocation postpones new keys until the next present while valid hits remain available. Filtering, LOD/compare/border/reduction/anisotropy policies, successful sampler lifetimes, registration checks, replay locking and API12 are unchanged. Storage images still accept their unused null sampler. No replacement sampler silently changes the requested filtering policy. Validation at `10a2f7f6b25464fdfcc85ef4109ffd7e3f802e9f`, based on main `caaee0ba6aef97514132603c9e6fa9e58f99bf57`: - **108 execution cases** run the actual complete factory, ID-cache consumer, policy helpers, descriptor binder and fingerprint helpers with authored Vulkan/registration/environment/hash/lock-boundary inputs. Windows LLVM/MinGW, Linux GCC and ASan/UBSan pass. **16 behavioral mutants** are rejected. Cases cover ownership/acceleration/failure-metadata allocation faults, registration changes, threads, unchanged policies, cold/warm failure in an array's second element, both pushed-state layouts/full pushes, pooled sets, graphics/compute, recovery and storage images. Rejected writes produce no allocation/update/bind/push and leave the prior fingerprint intact. - Full local suite: **248 passed / 2 failed / 20 skipped**, versus exact main **247 / 2 / 20**. Both failures are unchanged local environment profiles: NUP symlink privilege and missing NGX SDK guard. The shared staging fixture repair is now accepted in #1741 and has been dropped from this diff. No assertion, warning or CI check is disabled. - Fresh official generation completed successfully with zero NULs in generated C/headers. Matching API12 host/backend build passed. Final-head RTX 5070 Ti healthy native validation passed with fixed FG 2x: serial and parallel/batch recording, a client-size roundtrip from 1444x720 to 1440x829 and back, six representative PNGs validated at 3440x1440, and both exits0. Original checkpoint hashes are unchanged. Earlier runs at `cbe49eb` remain historical; the tested-configuration row names that observed build, hardware, driver, checkpoint and limits. Fresh actual-patch overlap review distinguishes #1654's scene LOD bias/additional biased ID table, #1541's writer/diagnostic refactor, #1366's filter controls, and #1653/#1008's provider-specific resources. This PR implements none of those features. If the sampler signature/table changes first, preserve positive-only admission in every added table and update the source fixture during rebase. No code was copied from those PRs. The failure models do not establish native GPU OOM/device-loss recovery, image-quality or performance improvements, or campaign coverage. Only authored source, tests and documentation are included; no generated game code, dumps, captures or saves are published. Current-head fork workflows are pending: - [Game-free source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42845353381) - [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42845360360) - [Linux AppImage](https://github.com/Arkex-ai/Hookshot/actions/runs/42845366090) See `docs/sampler-admission.md` for the contract and model boundaries.  ### Current-head validation Current head `efe9837`, 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/42858447739): in_progress. - [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42858442997): in_progress. - [Game-free source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42858437819): queued. 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:41:25 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good. Checked the vk_draw.cpp changes against the head: sampler_for only caches non-null handles, sampler_make no longer emplaces a failed/undefined handle into the global map, the thread-local and seen caches are best-effort (the try/catch only drops an acceleration entry), and a failed insert destroys just the new sampler. The per-present failure set is read/written under ParLock, so the retry-on-next-present behaviour is safe across threads. bind_descriptors checks every combined-image-sampler array element before touching the pushed-state cache or allocating a set, and all three callers (graphics, compute, mesh fallback) already handle a false return. Storage images are untouched. The new test is registered in tests/run_all.py and CI is green.

Nit: a draw whose sampler fails is skipped for that present, which is the intended trade-off and is documented, so nothing to change.

Merging order: no direct overlap found with #1654/#1541/#1366; whoever lands second just needs to keep the positive-only admission in any added table.

Looks good. Checked the vk_draw.cpp changes against the head: sampler_for only caches non-null handles, sampler_make no longer emplaces a failed/undefined handle into the global map, the thread-local and `seen` caches are best-effort (the try/catch only drops an acceleration entry), and a failed insert destroys just the new sampler. The per-present failure set is read/written under ParLock, so the retry-on-next-present behaviour is safe across threads. bind_descriptors checks every combined-image-sampler array element before touching the pushed-state cache or allocating a set, and all three callers (graphics, compute, mesh fallback) already handle a false return. Storage images are untouched. The new test is registered in tests/run_all.py and CI is green. Nit: a draw whose sampler fails is skipped for that present, which is the intended trade-off and is documented, so nothing to change. Merging order: no direct overlap found with #1654/#1541/#1366; whoever lands second just needs to keep the positive-only admission in any added table.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101