Reject failed generator-cache output replacement - #9438
Open
gregcotten wants to merge 2 commits into
Open
Conversation
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
Contributor
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 |
Member
|
@gregcotten — could you please rebase on |
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>
gregcotten
force-pushed
the
fix-generator-cache-output-replacement
branch
from
September 11, 2026 15:17
15a160c to
4d6a2ce
Compare
Catch CompileError in the test child and return a nonzero status, allowing its coverage profile to be written instead of losing it when the child aborts. Cache failure, cleanup, recovery, and reuse checks remain active. An instrumented Windows comparison confirms that the failed child's profile is absent before this change and present afterward. Production error handling is unchanged. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Contributor
Author
|
@alexreinking done! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9438 +/- ##
==========================================
+ Coverage 69.95% 70.17% +0.22%
==========================================
Files 261 261
Lines 79596 79621 +25
Branches 19400 19405 +5
==========================================
+ Hits 55678 55872 +194
+ Misses 18004 17955 -49
+ Partials 5914 5794 -120 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 before rebasing: a full shared Halide build passed
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. The branch includes #9436; CI and coverage results for the updated head are pending.
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.
The test child reports
CompileErroras a nonzero exit so expected failures retain coverage profiles. Instrumented Windows verification confirmed that the failing child's profile was previously lost and is now written, while all cache checks still pass. This changes only the test harness; production error handling is unchanged.