Reject failed generator-cache output replacement - #9438
Open
gregcotten wants to merge 2 commits into
Open
Conversation
Check the fallback copy before reporting a hit, preserve its error, and clean pending temporary files. An output that cannot be replaced must fail the generator instead of leaving stale bytes behind a successful command. Extend the existing correctness test with a portable output obstruction and successful restoration after the obstruction is removed. Fixes halide#9437 Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> [skip ci]
gregcotten
marked this pull request as ready for review
September 10, 2026 19:15
This was referenced Sep 10, 2026
alexreinking
approved these changes
Sep 10, 2026
Request the normal pull-request checks now that the fix is submitted for upstream review. Preserve the reviewed patch and its existing history. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Author
|
@alexreinking, sorry these were committed with [skip ci] - I made a dummy commit to trigger workflows. Please approve! Needless to say this should be a squash merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9437.
A cache hit can currently leave an older pipeline's output on disk and still return success. When rename fails,
try_restoredoes not check whether its fallback copy succeeded.We investigated this after a Windows OpenCL AOT build appeared to keep an earlier implementation despite rebuilding the generator, prompting us to disable semantic caching. This reproducer demonstrates a mechanism that can produce that symptom; it does not establish the cause of the original incident.
Check that copy, preserve the error message, clean pending temporary files, and fail the generator before it can report a hit. Successful rename and copy-fallback restores retain their behavior; cache keys and the cache format are unchanged. The existing correctness test gains a nonempty-directory obstruction and verifies restoration after removing it.
The standalone CPU reproducer demonstrates the bug with Windows sharing locks. The same shared restore defect was also reproduced on macOS with an immutable archive.
Local Windows verification: a full shared Halide build at this PR's head passes
correctness_generator_cache,correctness_compile_to, andcorrectness_compile_to_multitarget. The normal CMaketest/integration/cacheproject passes both its generated-code execution test and cache-population check. The standalone reproducer against that full DLL reports nonzero status and no hit for locked objects/archives, then restores the requested bytes after unlocking. Successful control objects and archives are byte-identical to the original wheel's controls.The new regression also fails against the original
halide-bin==22.0.0.dev390wheel. Earlier focused-fixture checks cover temporary-file cleanup and successful copy fallback.clang-format21.1.8 andgit diff --checkpass. Full-build environment: Windows x64, C++20, MSVC 19.42 compiler, isolated 14.44 linker/CRT libraries, Windows SDK 10.0.22621.0, LLVM 22.1.8, serialization enabled. The newer link libraries are needed by the LLVM binary distribution; no installed toolchain was modified.Remaining validation: static Halide linkage and patched macOS/Linux runs have not been tested locally. Normal upstream CI has now been requested for the unchanged patch; the fork workflows are awaiting maintainer approval to run.
An output replacement failure is reported as a user error rather than a cache miss. Recompilation is not a reliable recovery in the presence of the separate Windows COFF archive writer defect in #9439, which ignores stream errors. Its producer-side fix is proposed in #9440. This PR handles restoration of an existing cache entry; #9440 handles fresh archive generation. The two fixes have been tested independently and together.