Skip to content

fix(vk): a masked clear whose pipeline the driver did not make is tried again, not cached as null (#1787) - #1880

Merged
hckrman101 merged 3 commits into
NotRexed:mainfrom
oscarlosu-lab:fix/masked-clear-failed-pipeline
Nov 10, 2026
Merged

hckrman101 merged 3 commits into
NotRexed:mainfrom
oscarlosu-lab:fix/masked-clear-failed-pipeline

Conversation

@oscarlosu-lab

@oscarlosu-lab oscarlosu-lab commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

A failed layout, shader module or pipeline in clear_pipeline was used or cached as VK_NULL_HANDLE, so one transient driver failure removed that masked clear for the rest of the run. Fixes #1787.

Cause

clear_pipeline (gpu/vk/vk_draw.cpp) makes a masked clear's full-screen-triangle pipeline on its first use. It ignored what vkCreatePipelineLayout and vkCreateShaderModule returned, so a failed layout or module went on into the next calls, and it stored whatever cached_graphics gave back: a refused pipeline was cached as VK_NULL_HANDLE, and that clear never ran again.

Change

  • Each step's VkResult is checked; a layout is only kept in clear_layout once it was made.
  • A failure keeps nothing: the vertex module is destroyed when the fragment module fails, and a pipeline handle returned with an error is destroyed.
  • The key is tried again on its next clear, kClearTries (3) times, then once every kClearRetryEvery (600) of its clears, so a driver that always refuses it costs no compile every frame and a device that was short of memory for a while gets the clear back; the third failure logs one vk: masked clear: no <step> (VkResult n) line. A key that succeeds drops its count.

Test

tests/test_masked_clear_failure.py (game-free, reads the source like test_mesh_fallback_failed_once.py): every creation's result is checked after the call, the bound is checked before any work, a failed pipeline is counted and never cached, and the vertex module is freed on the fragment failure. It fails on main and passes here; python -I -B tests/run_all.py is otherwise unchanged on Windows.

Nothing changes on screen while the driver makes the pipeline, so no screenshots. Not run in the game: the path only matters when the driver refuses the pipeline.

🤖 Generated with Claude Code

oscarlosu-lab and others added 2 commits November 10, 2026 20:16
…ed again, not cached as null (NotRexed#1787)

clear_pipeline ignored what vkCreatePipelineLayout and vkCreateShaderModule returned and cached whatever
cached_graphics gave back, so one transient driver failure left that masked clear out for the rest of the run.
Each step's result is now checked; a failure keeps nothing (the vertex module is freed when the fragment one
fails) and the key is tried again on its next clear, at most kClearTries (3) times, with one log line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Arkex-ai

Copy link
Copy Markdown
Contributor

Thanks for picking this up. I already have an executed fault-injection fixture for #1787; I will hold my overlapping PR so we can consolidate the work here.

I checked the actual factory/cache functions at d2f8dd5 with authored failures. Both revisions pass the healthy control. Two additional paths need rollback:

  • If owning-cache insertion throws after pipeline creation, clear_pipes[key] = pl lets std::bad_alloc escape and leaves one pipeline live without a cache owner.
  • Fragment-word allocation failure also escapes clear_pipeline.

Is the three-attempt lifetime limit intended for ordinary transient allocation failures? After three failed calls, my probe makes allocation succeed on a later present, but this revision never calls the factory again. My candidate bounds attempts within a present and allows retry on a later present.

The reusable source-executed fixture covers owning insertion, fragment allocation, temporary ownership and retry. Its runner currently extracts my candidate's failure-tracking state; the comparison adapts that state declaration to this revision without changing the factory. These are game-free allocation models, not a native OOM report. Happy to bring the executed coverage and rollback into this PR rather than duplicate it.

…otRexed#1787)

After kClearTries failures a key was left without its clear for the rest of
the run, so a transient VK_ERROR_OUT_OF_DEVICE_MEMORY lost it for good. It now
backs off instead: one try every kClearRetryEvery (600) of its clears, which
keeps a driver that never makes the pipeline off the per-frame path and lets a
device that recovered get the clear back. The source test checks the back-off.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oscarlosu-lab

Copy link
Copy Markdown
Contributor Author

Thanks for checking it against the real functions.

Retry after the third failure: fair point, fixed in fa04b6f. The cap is there so a pipeline the driver can never make (the #1787 case) is not rebuilt every frame, but it also shut out a transient VK_ERROR_OUT_OF_DEVICE_MEMORY for good. A key that hits the cap now backs off instead: it is tried once every kClearRetryEvery (600) of its clears, so the per-frame cost stays bounded and a device that recovered gets its clear back. The source test checks the back-off.

std::bad_alloc on insertion / the fragment words: I'd leave these out of this PR. The rest of the renderer doesn't guard host allocations either (every cache and SPIR-V copy in vk_draw.cpp is a plain std:: container), so a host OOM here isn't something one function can recover from, and handling it only in clear_pipeline would be inconsistent rather than safer.

Happy to keep this as the one PR for #1787. If you'd rather your fixture land too, it could follow as a separate tests PR so this one stays small.

@Arkex-ai

Copy link
Copy Markdown
Contributor

Thanks, the back-off addresses the permanent lockout I reported. I checked the updated control flow at fa04b6f; I have not rerun the fault-injection probe on this revision yet.

Agreed to keep #1787 in this PR. I will keep my overlapping candidate unpublished and won't open a separate tests-only PR. The host-allocation cases remain recorded separately; any broader allocation-failure policy should be agreed with the maintainers before extending this scope.

@hckrman101
hckrman101 merged commit 9d94f4c into NotRexed:main Nov 10, 2026
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Masked-clear pipeline creation admits failed resources and caches null results

2 participants