Contain pipeline cache save allocation failures #1046

2026-11-09T07:57:16Z

monsterlag commented

2026-11-09 07:57:16 +00:00

(Migrated from github.com)

Pipeline cache saves can throw while allocating their byte buffer or temporary filename. A detached worker then terminates the process; a forced save propagates the exception and can leave the busy flag set.

Contain standard exceptions in the save worker, release the busy flag on every worker exit, and report thread-construction failures. Successful saves, thresholds and the existing forced-save behavior are preserved.

Validation on the refreshed main source:

Windows LLVM-MinGW and Linux GCC: all 11 focused tests pass, including actual container/thread-state allocation faults and six original-current-main exception controls.
Target-only strict automatic runner: 1 passed, 0 failed, 0 skipped.
Fault fixture and assertions are byte-identical to the original PR; the normal merge preserves all upstream registrations/workflows.
The production-only patch applies cleanly to official RC2 source c4ea255; that release tree was not edited or built by this refresh.

Refs #1022. These checks use real C++ containers/threads with authored Vulkan/file responses, without a GPU, game or real pipeline cache. The refresh does not add a new renderer/game run or prove an operating-system thread/detach failure. Concurrent forced callers and the cache format retain their existing behavior. Hosted Windows renderer, Builder packaging, Linux AppImage, native F2I, SPIR-V and workflow-lint checks pass. Both source jobs fail solely on the existing answer-domain fixture tracked in #1076; the new cache-save test passes in both. See platform builds, AppImage and source checks.

Pipeline cache saves can throw while allocating their byte buffer or temporary filename. A detached worker then terminates the process; a forced save propagates the exception and can leave the busy flag set. Contain standard exceptions in the save worker, release the busy flag on every worker exit, and report thread-construction failures. Successful saves, thresholds and the existing forced-save behavior are preserved. Validation on the refreshed main source: - Windows LLVM-MinGW and Linux GCC: all 11 focused tests pass, including actual container/thread-state allocation faults and six original-current-main exception controls. - Target-only strict automatic runner: 1 passed, 0 failed, 0 skipped. - Fault fixture and assertions are byte-identical to the original PR; the normal merge preserves all upstream registrations/workflows. - The production-only patch applies cleanly to official RC2 source c4ea255; that release tree was not edited or built by this refresh. Refs #1022. These checks use real C++ containers/threads with authored Vulkan/file responses, without a GPU, game or real pipeline cache. The refresh does not add a new renderer/game run or prove an operating-system thread/detach failure. Concurrent forced callers and the cache format retain their existing behavior. Hosted Windows renderer, Builder packaging, Linux AppImage, native F2I, SPIR-V and workflow-lint checks pass. Both source jobs fail solely on the existing answer-domain fixture tracked in #1076; the new cache-save test passes in both. See [platform builds](https://github.com/NotRexed/Hookshot/actions/runs/42609247438), [AppImage](https://github.com/NotRexed/Hookshot/actions/runs/42609247437) and [source checks](https://github.com/NotRexed/Hookshot/actions/runs/42609247460).

hckrman101 (Migrated from github.com) approved these changes

2026-11-09 18:50:49 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good. The Done guard releases busy on every worker exit, bad_alloc/length_error from the vector and the tmp path are caught in both the detached and forced paths, and a failed save never touches the existing cache (tmp file then rename). The thread-construction catch resets busy correctly. The new test is registered in run_all.py with a {cxx} ARGS entry, and it exercises the real function text with real containers/threads. The change doesn't touch pcache_init, so it doesn't conflict with the _fseeki64 change. I couldn't pull the job logs, so I'm relying on your note that the new test passes and the two red source jobs are the #1076 fixture.

Two small things, both pre-existing and fine as follow-ups (one inline comment below):

the fclose result isn't checked (disk full),
if the cache grows between the two vkGetPipelineCacheData calls the second returns VK_INCOMPLETE and the save is silently skipped.

Looks good. The Done guard releases busy on every worker exit, bad_alloc/length_error from the vector and the tmp path are caught in both the detached and forced paths, and a failed save never touches the existing cache (tmp file then rename). The thread-construction catch resets busy correctly. The new test is registered in run_all.py with a {cxx} ARGS entry, and it exercises the real function text with real containers/threads. The change doesn't touch pcache_init, so it doesn't conflict with the _fseeki64 change. I couldn't pull the job logs, so I'm relying on your note that the new test passes and the two red source jobs are the #1076 fixture. Two small things, both pre-existing and fine as follow-ups (one inline comment below): - the fclose result isn't checked (disk full), - if the cache grows between the two vkGetPipelineCacheData calls the second returns VK_INCOMPLETE and the save is silently skipped.

gpu/vk/vk_draw.cpp

					
				@ -6728,0 +6736,4 @@

				            busy = false;

				            logf("vk: could not start pipeline cache save: %s", e.what());

				        }

				    }

Nit, not new in this PR: with a full disk fwrite can succeed into the stdio buffer and the error only shows up in fclose, so ok stays true and the truncated tmp gets renamed over a good cache. Something like const bool ok = fwrite(...) == n; const bool closed = fclose(f) == 0; if (ok && closed) ... (and remove the tmp otherwise) would close that. The same call-2 path also drops the save on VK_INCOMPLETE if the cache grew after the size query; a retry or a small headroom on the buffer would cover it.

Nit, not new in this PR: with a full disk fwrite can succeed into the stdio buffer and the error only shows up in fclose, so ok stays true and the truncated tmp gets renamed over a good cache. Something like `const bool ok = fwrite(...) == n; const bool closed = fclose(f) == 0; if (ok && closed) ...` (and remove the tmp otherwise) would close that. The same call-2 path also drops the save on VK_INCOMPLETE if the cache grew after the size query; a retry or a small headroom on the buffer would cover it.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101