Repository navigation
fix(build): fingerprint injected Go source patch bodies - #2527
Merged
xushiwei merged 4 commits intoSep 8, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: source patch cache fix
The goal — folding source-patch bodies into the package fingerprint so a patch-body edit invalidates the build cache — is the right fix. But as written the change does not take effect in production because of a path-key mismatch, and the added test masks it. Details inline.
Summary of findings
- [P0]
packageGoSourceInputsmatchesoverlay[file]against the_patch/paths inCompiledGoFiles, but the overlay is keyed byz_llgo_patch_*paths — the lookup never succeeds for real builds, so patch bodies still don't enter the fingerprint. - [P1] The new test uses identical synthetic paths for
CompiledGoFilesand the overlay key, so it passes without exercising the real path relationship — giving false confidence. - [P3] The doc comment overstates that "source patches ... are not present in GoFiles"; only appended patch files fit that description.
Comment on lines
+43
to
+46
| pkg := &aPackage{Package: &packages.Package{ | ||
| ID: "example.test", PkgPath: "example.test", | ||
| GoFiles: []string{original}, CompiledGoFiles: []string{original, patch}, | ||
| }} |
Contributor
There was a problem hiding this comment.
[P1] Test uses identical synthetic paths, masking the P0 mismatch
This test constructs CompiledGoFiles: []string{original, patch} and keys the overlay by the same patch path, so overlay[file] succeeds and the test passes. In production these are distinct paths (a _patch/ source path in CompiledGoFiles vs. a z_llgo_patch_* overlay key), so the guarded branch never fires. As written the test asserts the intended property on an unrealistic path relationship and therefore does not catch the P0 defect.
Recommend modeling production: set the CompiledGoFiles entry to the _patch/ path and key the overlay by the injected z_llgo_patch_* path, so the test reproduces the real mismatch and fails until the helper is corrected.
Collaborator
Author
There was a problem hiding this comment.
Fixed in f2387a8. The regression now models distinct production paths: _patch/llgo_patch.go is the selected source file, while z_llgo_patch_llgo_patch.go is the injected overlay key. It mutates only the selected source body and still verifies that an unselected overlay does not change the manifest.
Comment on lines
+194
to
+195
| // Source patches are appended to CompiledGoFiles after go list; they are not | ||
| // present in GoFiles. Their overlaid bodies must participate in cache keys too. |
Contributor
There was a problem hiding this comment.
[P3] Comment overstates that source patches are absent from GoFiles
The comment says source patches "are not present in GoFiles." That holds only for the appended patch files (z_llgo_patch_*). The source-patch machinery also overlays files that are in GoFiles under their original names (asm stub replacement, packageStubSource skip-all, filterSourcePatchFile) — those bodies are captured via the slices.Clone(p.GoFiles) + digestFilesWithOverlay path, not this loop. Consider narrowing the wording to "appended source-patch files" to avoid implying this loop covers all patch kinds.
Collaborator
Author
There was a problem hiding this comment.
Updated in f2387a8. The comment now says specifically that appended source-patch files are absent from GoFiles; it no longer describes all source-patch forms that way.
Comment on lines
+198
to
+202
| for _, file := range p.CompiledGoFiles { | ||
| if _, ok := overlay[file]; ok && !slices.Contains(files, file) { | ||
| files = append(files, file) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[P0] Overlay key mismatch: patch bodies excluded from fingerprint
Overlay key mismatch: patch bodies never enter the fingerprint in production.
In production the appended CompiledGoFiles entries and the overlay keys are different paths, so overlay[file] never matches and no patch file is added here.
- The slice appended to
CompiledGoFiles(load.go:345) comes fromapplySourcePatchForPkg, which appendssrc.filename— the_patch/source pathfilepath.Join(patchDir, name)(source_patch.go:132,:261). - The overlay is keyed by the injected GOROOT target
filepath.Join(srcDir, "z_llgo_patch_"+name)(source_patch.go:258). There is noout[src.filename]entry.
Since a _patch/ path is never a key in the overlay, the if _, ok := overlay[file]; ok guard is false for every real source-patch file, so it is never appended and its body never reaches the fingerprint — the same stale-cache defect this PR intends to fix: editing a registered source-patch body will not invalidate the cached archive for the affected stdlib package.
Suggested fix: fingerprint by the content actually keyed in the overlay — e.g. match/append the injected z_llgo_patch_* target path (or digest the _patch/ file recorded in CompiledGoFiles), and update the test to use the real distinct paths so it reproduces the mismatch.
Collaborator
Author
There was a problem hiding this comment.
Fixed in f2387a8. Fingerprinting now consumes the exact selected source-patch file list from context.patchFiles[p.PkgPath], so it no longer relies on matching _patch/... CompiledGoFiles paths against z_llgo_patch_... overlay keys. The selected source body is digested directly.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cpunion
force-pushed
the
codex/fix-source-patch-cache-20260908
branch
from
September 8, 2026 01:22
b1082dd to
f2387a8
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
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
2 participants
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.
Problem
Injected Go source patches are added to
CompiledGoFilesaftergo list, but package cache fingerprints previously considered onlyGoFiles. Changing an injected function body could silently reuse a stale archive. This common build-cache defect was found while validating a wasm syscall timestamp fix.Fix
CompiledGoFilesin Go source inputs without counting original files twice.This is independent of the WASM R4 runtime changes; it changes only build fingerprinting and its tests.
Validation
be23e488a.b1082dd3590a25c86efd4606cfd91da408713d20, with no failed or pending checks before this upstream contribution.3dce98b91do not overlap this patch.Why earlier CI did not expose this
A clean build is unaffected: the injected source is compiled correctly on the first cache miss. The stale result requires two builds sharing one package cache where only the selected injected patch body changes. Earlier clean-checkout and ordinary cache-hit lanes did not exercise that transition. The new regression performs that exact second build, so prior green clean-build results are not evidence against this fix.