fix(core): keep dockerBuild layer cache on unrelated context changes - #13639
Conversation
Dockerfile COPY/ADD and RUN --mount=type=bind converted through llbtodagger take the entire build-context Directory as their source argument, so their cache identity changed whenever anything in the context changed — even files the step never selected (e.g. .git metadata rewritten by every CI checkout). The compat results carried no content-based identity, so one context change re-keyed every downstream RUN layer. BuildKit does not have this problem: its COPY cache keys are content checksums of just the selected paths. Fix by teaching both compat calls a content-based cache identity after execution: - __withDirectoryDockerfileCompat: H(parent content-preferred digest, dest path, content checksum of the dest subtree) - __withMountedPathDockerfileCompat: H(parent content-preferred digest, target, sourcePath, readOnly, content checksum of source@sourcePath) The copy still re-executes when the context changes, but byte-identical output merges back into the same cache equivalence class, so all downstream RUN layers keep hitting. Directory.filter is also wrapped with maintainContentHashing so dockerignore-filtered contexts stay content-addressed, letting the copy call itself cache-hit when only dockerignored files changed. Narrowing the copy source with filter(include: [src]) was tried first and rejected: static include patterns cannot express symlink-target closures (breaks copy-through-symlink-context). Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Marcos Nils <[email protected]>
|
heads up that this has been validated by the reporter that it fixes their issue. |
There was a problem hiding this comment.
One meaningful simplification comment, otherwise LGTM. I'll approve to unblock but would prefer to merge the simplification if it proves correct so we don't accumulate complication debt.
EDIT: codex found an issue after I posted this, also worth a fix before merge
| return inst.WithContentDigest(ctx, hashutil.HashStrings( | ||
| "__withMountedPathDockerfileCompat", | ||
| string(parentDgst), | ||
| target, | ||
| args.SourcePath, | ||
| strconv.FormatBool(args.ReadOnly), | ||
| srcHash, | ||
| )) |
There was a problem hiding this comment.
This doesn't seem very necessary. We can just get the content hash of the input as annotate that as the content digest. None of this other stuff seems necessary to mix in unless I am missing something.
Same comment for withDirectoryDockerfileCompatContentHashed
There was a problem hiding this comment.
Claude reply reviewed by Marcos:
Done for the directory case — now just annotates the resulting dir's content hash (like maintainContentHashing). Bonus: no dest stat, so empty-wildcard COPY optional-* /out/ stops erroring.
Kept mixing on the container mount, though, and I think it's needed: a taught content digest merges the eq-class globally (teachResultIdentityLocked), so equal digest ⟹ interchangeable everywhere. A
Directory's content hash is its whole value, so that's safe. A Container has no whole-value hash — keying purely on mounted content would collide two containers that share a mounted file but differ in parent
or target. So the mount keeps H(parent CPD, target, readOnly, checksum(source@path)): content-address the source (the actual fix), keep what distinguishes the container. Dropped sourcePath as redundant with
its checksum.
If there's a cleaner content-preferred identity for containers, happy to switch.
| return res, fmt.Errorf("failed to content hash dockerfile copy: directory path unset") | ||
| } | ||
| destPath := path.Join(dirPath, args.Path) | ||
| destDgst, err := core.GetContentHashFromFile(ctx, snapshot, destPath) |
There was a problem hiding this comment.
Quick note: this finding was identified by Codex and approved by Erik.
Handle allowed empty wildcard copies before hashing the destination
GetContentHashFromFile is unconditional here, but Dockerfile COPY/ADD permits empty wildcard matches. For example:
COPY optional-* /out/If nothing matches and /out/ does not exist, the compatibility copy successfully commits a no-op snapshot. This wrapper then checksums the absent destination and turns that valid no-op into a not found build error.
Since the detached result has already been materialized, this error can also bypass DagQL’s normal OnRelease installation and retain the committed snapshot ref.
Could we treat fs.ErrNotExist as a domain-separated “missing” destination digest and ensure the detached result is released on post-materialization errors? A regression test with an unmatched wildcard and absent destination would cover this case.
There was a problem hiding this comment.
this should be resolved Erik. LMK if you think there's anything missing here.
Follow-up to the dockerBuild layer-cache fix, addressing PR review: - __withDirectoryDockerfileCompat: annotate the content hash of the whole resulting directory (GetContentHashFromDirectory, same as maintainContentHashing) instead of a hand-rolled composite. This also tolerates COPY wildcards that match nothing (no destination to stat), and releases the materialized snapshot on post-materialization errors so the ref does not leak when dagql skips OnRelease installation. - __withMountedPathDockerfileCompat: drop sourcePath from the digest (redundant with its content checksum). Keep parent content-preferred digest, target, and readOnly: a taught content digest is a global alias for the result identity, and a Container has no whole-value content hash, so keying purely on mounted content would merge containers that differ only in parent or mount target. Add TestDockerBuildEmptyWildcardCopy covering an unmatched-wildcard COPY with an absent destination. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Marcos Nils <[email protected]>
2 participants
Dockerfile COPY/ADD and RUN --mount=type=bind converted through llbtodagger take the entire build-context Directory as their source argument, so their cache identity changed whenever anything in the context changed — even files the step never selected (e.g. .git metadata rewritten by every CI checkout). The compat results carried no content-based identity, so one context change re-keyed every downstream RUN layer. BuildKit does not have this problem: its COPY cache keys are content checksums of just the selected paths.
Fix by changing both compat calls to use a content-based cache identity after execution:
The copy still re-executes when the context changes, but byte-identical output merges back into the same cache equivalence class, so all downstream RUN layers keep hitting. Directory.filter is also wrapped with maintainContentHashing so dockerignore-filtered contexts stay content-addressed, letting the copy call itself cache-hit when only dockerignored files changed.
Narrowing the copy source with filter(include: [src]) was tried first and rejected: static include patterns cannot express symlink-target closures (breaks copy-through-symlink-context).