Skip to content

session/filesync: byte-level output progress, no ContentHasher (fixes AS LOCAL panic) - #20

Merged
gilescope merged 8 commits into
merge-in-buildkit-all-at-oncefrom
giles-fix-synctarget-contenthasher
Jul 16, 2026
Merged

gilescope merged 8 commits into
merge-in-buildkit-all-at-oncefrom
giles-fix-synctarget-contenthasher

Conversation

@gilescope

Copy link
Copy Markdown

syncTargetDiffCopy set NotifyHashed without a ContentHasher. fsutil wraps every incoming file in a hashedWriter whenever NotifyCb is set and DEREFERENCES the hasher to do it (diskwriter.go, newHashWriter), so a nil hasher is a nil-pointer panic, not a no-op - and SAVE ARTIFACT ... AS LOCAL crashed outright:

panic: runtime error: invalid memory address or nil pointer dereference
fsutil.newHashWriter(0x0, ...) diskwriter.go:285

The session attachables run in the CLI, which is why it showed up there, not in buildkitd. Introduced by the verbose-progress rewire: the forked fsutil had a VerboseProgressCB hook that needed no hasher; stock fsutil offers only NotifyHashed, and the port adopted the hook without its precondition.

The real problem was the wrong hook. This is the OUTPUT path (writing built artifacts to the user's local disk); it computes no cache key and reads no digest - the callback was pure progress. But NotifyHashed is fsutil's HASHING per-file hook: it would MultiWriter every received byte through a hasher for a digest nobody reads, on every AS LOCAL, even once the nil-deref is fixed.

So switch to ReceiveOpt.ProgressCb: cumulative received bytes, per packet, NO hasher. It gives LIVE transfer feedback - which is what an output copy actually wants - and the panic is structurally impossible because no ContentHasher is ever constructed. The earthly-specific sync-target API carries a func(int, bool) progress callback instead of an fsutil.ChangeFunc (ExportEntry.OnReceiveFile -> OnReceiveProgress).

Not the Filter hook: it is an inclusion filter, applied in two places (receive.go destWalker + doubleWalkDiff) and sometimes with an empty stat, so it double-counts. Not a discard/no-op ContentHasher: it works, but it abuses the hashing hook for a non-hashing need. ProgressCb is the hook fsutil actually provides for this.

Trade-off: byte-level aggregate instead of per-file lines. On this path the per-file lines were VerbosePrintf (debug-only) anyway; the normal-level UX is a throttled byte summary, which ProgressCb feeds directly - and byte-level is the better signal for a slow network copy.

Verified: SAVE ARTIFACT AS LOCAL of a 60 MiB artifact succeeds, exact bytes on disk, live byte progress, no hashing. It panicked 100% of the time before.

Assisted-by: Claude:claude-opus-4.8 claude-code

… AS LOCAL panic)

syncTargetDiffCopy set NotifyHashed without a ContentHasher. fsutil wraps every
incoming file in a hashedWriter whenever NotifyCb is set and DEREFERENCES the
hasher to do it (diskwriter.go, newHashWriter), so a nil hasher is a nil-pointer
panic, not a no-op - and `SAVE ARTIFACT ... AS LOCAL` crashed outright:

  panic: runtime error: invalid memory address or nil pointer dereference
  fsutil.newHashWriter(0x0, ...)   diskwriter.go:285

The session attachables run in the CLI, which is why it showed up there, not in
buildkitd. Introduced by the verbose-progress rewire: the forked fsutil had a
VerboseProgressCB hook that needed no hasher; stock fsutil offers only
NotifyHashed, and the port adopted the hook without its precondition.

The real problem was the wrong hook. This is the OUTPUT path (writing built
artifacts to the user's local disk); it computes no cache key and reads no
digest - the callback was pure progress. But NotifyHashed is fsutil's HASHING
per-file hook: it would MultiWriter every received byte through a hasher for a
digest nobody reads, on every AS LOCAL, even once the nil-deref is fixed.

So switch to ReceiveOpt.ProgressCb: cumulative received bytes, per packet, NO
hasher. It gives LIVE transfer feedback - which is what an output copy actually
wants - and the panic is structurally impossible because no ContentHasher is
ever constructed. The earthly-specific sync-target API carries a
`func(int, bool)` progress callback instead of an `fsutil.ChangeFunc`
(ExportEntry.OnReceiveFile -> OnReceiveProgress).

Not the Filter hook: it is an inclusion filter, applied in two places
(receive.go destWalker + doubleWalkDiff) and sometimes with an empty stat, so it
double-counts. Not a discard/no-op ContentHasher: it works, but it abuses the
hashing hook for a non-hashing need. ProgressCb is the hook fsutil actually
provides for this.

Trade-off: byte-level aggregate instead of per-file lines. On this path the
per-file lines were VerbosePrintf (debug-only) anyway; the normal-level UX is a
throttled byte summary, which ProgressCb feeds directly - and byte-level is the
better signal for a slow network copy.

Verified: SAVE ARTIFACT AS LOCAL of a 60 MiB artifact succeeds, exact bytes on
disk, live byte progress, no hashing. It panicked 100% of the time before.

Assisted-by: Claude:claude-opus-4.8 claude-code
@github-actions

Copy link
Copy Markdown

⚠️ Are we earthbuild yet?

Warning: "earthly" occurrences have increased by 29 (13.36%)

📈 Overall Progress

Branch Total Count
main 217
This PR 246
Difference +29 (13.36%)

📁 Changes by file type:

File Type Change
Go files (.go) ❌ +26
Documentation (.md) ➖ No change
Earthfiles ➖ No change

Keep up the great work migrating from Earthly to Earthbuild! 🚀

💡 Tips for finding more occurrences

Run locally to see detailed breakdown:

./.github/scripts/count-earthly.sh

Note that the goal is not to reach 0.
There is anticipated to be at least some occurences of earthly in the source code due to backwards compatibility with config files and language constructs.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces the per-file file-receive callback (fsutil.ChangeFunc) with a cumulative byte-level progress callback (func(int, bool)) across the file synchronization pipeline. This change avoids unnecessary hashing overhead and potential nil-pointer panics during artifact saving. The review feedback suggests naming the parameters of the anonymous progress callback function (e.g., func(bytes int, done bool)) across all signatures to improve code readability and self-documentation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread client/solve.go Outdated
Comment thread session/filesync/diffcopy.go Outdated
Comment thread session/filesync/filesync.go Outdated
Comment thread session/filesync/filesync.go Outdated
Comment thread session/filesync/filesync.go Outdated
gilescope and others added 7 commits July 15, 2026 08:21
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…n failure)

The repo ruleset requires all actions pinned to a full-length commit SHA, and
enforces it TRANSITIVELY. docker/bake-action@v7.0.0 is correctly pinned by us,
but its own subaction/matrix/action.yml uses `actions/github-script@v7`
UNPINNED internally - which we cannot pin (it is Docker's action, not ours).
Result: every workflow using bake-action failed at "Set up job":

  The action actions/github-script@v7 is not allowed in EarthBuild/buildkit
  because all actions must be pinned to a full-length commit SHA.

bake-action v7.3.0 pins its internal github-script to
3a2844b7e9c422d3c10d287c895573f7108da1b3 (v9.0.0), so bumping satisfies the
ruleset. All 8 refs across 4 workflows (.test, buildkit, test-os, validate),
including the /subaction/matrix variants that carry the offending reference.

This is why the validate/prepare gate was red on PRs against this branch (and
on the branch's own runs). Unrelated to any code change.

Assisted-by: Claude:claude-opus-4.8 claude-code
Signed-off-by: Giles Cope <gilescope@gmail.com>
(cherry picked from commit 1dd49bd)
Signed-off-by: Giles Cope <gilescope@gmail.com>
The prepare job's github-script does `yaml.load(core.getInput('includes'))`.
Current js-yaml throws `YAMLException: expected a document, but the input is
empty` on `yaml.load('')`, and empty includes is the NORMAL case - so prepare
failed every run, and since `run` has `needs: [prepare]` the whole test matrix
was blocked.

Masked until now by the transitive SHA-pin ruleset failure (the job died at
"Set up job" before reaching the script); unmasked once the bake-action pin
bump let it past setup.

Guard the empty input, restoring the prior undefined -> `[]` behaviour that the
existing `?? []` was already written for.

Assisted-by: Claude:claude-opus-4.8 claude-code
Signed-off-by: Giles Cope <gilescope@gmail.com>
@gilescope
gilescope merged commit 01153e9 into merge-in-buildkit-all-at-once Jul 16, 2026
112 of 114 checks passed
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.

1 participant