Skip to content

Reject failed generator-cache output replacement - #9438

Open
gregcotten wants to merge 2 commits into
halide:mainfrom
gregcotten:fix-generator-cache-output-replacement
Open

Reject failed generator-cache output replacement#9438
gregcotten wants to merge 2 commits into
halide:mainfrom
gregcotten:fix-generator-cache-output-replacement

Conversation

@gregcotten

@gregcotten gregcotten commented Sep 10, 2026

Copy link
Copy Markdown

Fixes #9437.

A cache hit can currently leave an older pipeline's output on disk and still return success. When rename fails, try_restore does 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, and correctness_compile_to_multitarget. The normal CMake test/integration/cache project 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.dev390 wheel. Earlier focused-fixture checks cover temporary-file cleanup and successful copy fallback. clang-format 21.1.8 and git diff --check pass. 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.

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]
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>
@gregcotten

Copy link
Copy Markdown
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

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.

Generator cache reports a hit after failing to replace an output

2 participants