From 88b32bf1b8304544d302a2ceffd91286f2bacca7 Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Fri, 9 Oct 2026 13:04:31 -0700 Subject: [PATCH 1/7] test: demonstrate double-counted send progress after merge The conflict resolution in sender.updateProgress kept both the fork's increment under s.mu and upstream's increment under progressCurrentMu, so every packet is added to progressCurrent twice whenever progressCb is set. The final total reported to progressCb is exactly double the STAT+DATA bytes the sender put on the wire. TestSendProgressTotal wraps the sender stream, sums the Size() of each non-terminal STAT packet and each non-empty DATA packet, and asserts the last=true total equals it. It fails on the merge (205440 vs 102720) and passes on both parents (644b083 and 91a3fc4). Co-Authored-By: Claude Opus 5.5 --- send_progress_test.go | 87 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 send_progress_test.go diff --git a/send_progress_test.go b/send_progress_test.go new file mode 100644 index 0000000..fb57303 --- /dev/null +++ b/send_progress_test.go @@ -0,0 +1,87 @@ +package fsutil + +import ( + "bytes" + "context" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/require" + "github.com/tonistiigi/fsutil/types" + "golang.org/x/sync/errgroup" +) + +// progressCountingStream wraps a Stream and sums the Size() of every packet +// that the sender is supposed to account for in its progress callback: each +// non-terminal STAT packet and each non-empty DATA packet. +type progressCountingStream struct { + Stream + mu sync.Mutex + total int +} + +func (c *progressCountingStream) SendMsg(m interface{}) error { + if p, ok := m.(*types.Packet); ok { + if (p.Type == types.PACKET_STAT && p.Stat != nil) || (p.Type == types.PACKET_DATA && len(p.Data) > 0) { + c.mu.Lock() + c.total += p.Size() + c.mu.Unlock() + } + } + return c.Stream.SendMsg(m) +} + +// TestSendProgressTotal checks that the cumulative size reported to the +// sender's progressCb matches the bytes the sender actually put on the wire. +func TestSendProgressTotal(t *testing.T) { + src := t.TempDir() + files := map[string][]byte{ + "small": []byte("data1"), + "dir/empty": nil, + // Larger than the 32KiB copy buffer so it is split into several + // DATA packets. + "dir/large": bytes.Repeat([]byte("x"), 100*1024+7), + } + for name, dt := range files { + p := filepath.Join(src, filepath.FromSlash(name)) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755)) + require.NoError(t, os.WriteFile(p, dt, 0o644)) + } + + fs, err := NewFS(src) + require.NoError(t, err) + + var ( + mu sync.Mutex + lastTotal = -1 + lastCalls int + ) + progressCb := func(total int, last bool) { + mu.Lock() + defer mu.Unlock() + if last { + lastCalls++ + lastTotal = total + } + } + + eg, ctx := errgroup.WithContext(context.Background()) + s1, s2 := sockPairProto(ctx) + counting := &progressCountingStream{Stream: s1} + + eg.Go(func() error { + defer s1.(*fakeConnProto).closeSend() + return Send(ctx, counting, fs, progressCb, nil) + }) + eg.Go(func() error { + return Receive(ctx, s2, t.TempDir(), ReceiveOpt{}) + }) + require.NoError(t, eg.Wait()) + + want := counting.total + require.Positive(t, want) + require.Equal(t, 1, lastCalls, "progressCb should be called exactly once with last=true") + require.Equal(t, want, lastTotal, "final progress total should equal the size of STAT and DATA packets sent") +} From 2cf36093b7cf6fb6b6d0adeeedf7647dbe85f0f7 Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Fri, 9 Oct 2026 13:05:55 -0700 Subject: [PATCH 2/7] test: verbose progress paths must be consistent per side (Windows) The merge put the fork's receiver StatusStat verboseProgressCb call before upstream's filepath.FromSlash normalisation, so on Windows the receiver reports StatusStat with the unix wire path ("dir/b") but StatusReceiving/StatusReceived with the native r.files key ("dir\b"). The sender has the mirror problem: StatusStat gets the native walk path while StatusSent gets the ToSlash'd wire path from s.files. TestVerboseProgressPathsConsistent asserts that every StatusSent path was also reported with StatusStat on the sender, and that the receiver reports the same native paths for StatusStat, StatusReceiving and StatusReceived as it does to NotifyHashed. FromSlash/ToSlash are no-ops on Unix, so this passes on macOS/Linux and is expected to fail only on Windows (cross-compiled with GOOS=windows; not executed). Co-Authored-By: Claude Opus 5.5 --- verbose_progress_test.go | 128 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 verbose_progress_test.go diff --git a/verbose_progress_test.go b/verbose_progress_test.go new file mode 100644 index 0000000..537274c --- /dev/null +++ b/verbose_progress_test.go @@ -0,0 +1,128 @@ +package fsutil + +import ( + "context" + "os" + "path/filepath" + "sort" + "sync" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" +) + +// verboseRecorder collects the paths passed to a VerboseProgressCB, keyed by +// status. +type verboseRecorder struct { + mu sync.Mutex + paths map[VerboseProgressStatus][]string +} + +func newVerboseRecorder() *verboseRecorder { + return &verboseRecorder{paths: map[VerboseProgressStatus][]string{}} +} + +func (r *verboseRecorder) cb(p string, status VerboseProgressStatus, _ int) { + r.mu.Lock() + defer r.mu.Unlock() + for _, existing := range r.paths[status] { + if existing == p { + return + } + } + r.paths[status] = append(r.paths[status], p) +} + +func (r *verboseRecorder) get(status VerboseProgressStatus) []string { + r.mu.Lock() + defer r.mu.Unlock() + out := append([]string(nil), r.paths[status]...) + sort.Strings(out) + return out +} + +// TestVerboseProgressPathsConsistent checks that, on each side of a transfer, +// the verbose progress callback reports a file under the same path for every +// status, and that the receiver reports the same (platform-native) path that +// it hands to NotifyHashed. +// +// On Unix filepath.FromSlash/ToSlash are no-ops so this passes. On Windows the +// merge of upstream's wire-path normalisation leaves: +// - sender: StatusStat gets the native walk path (`dir\b`) while +// StatusSent gets the wire path stored in s.files ("dir/b"); +// - receiver: StatusStat is called with p.Stat.Path before it is converted +// with filepath.FromSlash ("dir/b") while StatusReceiving/Received +// get the native r.files key via pipeNames (`dir\b`). It also fires +// before the "unrepresentable path" check, so a rejected path is still +// reported as stat'ed. +// +// This test cannot fail on macOS/Linux; it is red only on Windows. +func TestVerboseProgressPathsConsistent(t *testing.T) { + src := t.TempDir() + names := []string{"a", "dir/b", "dir/sub/c"} + for _, name := range names { + p := filepath.Join(src, filepath.FromSlash(name)) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755)) + require.NoError(t, os.WriteFile(p, []byte("data-"+name), 0o644)) + } + var wantFiles []string + for _, name := range names { + wantFiles = append(wantFiles, filepath.FromSlash(name)) + } + sort.Strings(wantFiles) + + fs, err := NewFS(src) + require.NoError(t, err) + + sendRec := newVerboseRecorder() + recvRec := newVerboseRecorder() + + var hashedMu sync.Mutex + var hashed []string + notifyHashed := func(kind ChangeKind, p string, fi os.FileInfo, err error) error { + if fi != nil && fi.Mode().IsRegular() { + hashedMu.Lock() + hashed = append(hashed, p) + hashedMu.Unlock() + } + return nil + } + + eg, ctx := errgroup.WithContext(context.Background()) + s1, s2 := sockPairProto(ctx) + eg.Go(func() error { + defer s1.(*fakeConnProto).closeSend() + return Send(ctx, s1, fs, nil, sendRec.cb) + }) + eg.Go(func() error { + return Receive(ctx, s2, t.TempDir(), ReceiveOpt{ + NotifyHashed: notifyHashed, + ContentHasher: simpleSHA256Hasher, + VerboseProgressCb: recvRec.cb, + }) + }) + require.NoError(t, eg.Wait()) + + sort.Strings(hashed) + require.Equal(t, wantFiles, hashed, "NotifyHashed paths") + + // Sender: every file reported as sent must have been reported as stat'ed + // under the identical path. + sendStat := sendRec.get(StatusStat) + sent := sendRec.get(StatusSent) + require.Len(t, sent, len(names), "sender StatusSent paths: %v", sent) + for _, p := range sent { + require.Contains(t, sendStat, p, "sender StatusSent path %q was not reported with StatusStat", p) + } + + // Receiver: the same, and they must match what NotifyHashed reported. + recvStat := recvRec.get(StatusStat) + received := recvRec.get(StatusReceived) + receiving := recvRec.get(StatusReceiving) + require.Equal(t, wantFiles, received, "receiver StatusReceived paths") + require.Equal(t, wantFiles, receiving, "receiver StatusReceiving paths") + for _, p := range received { + require.Contains(t, recvStat, p, "receiver StatusReceived path %q was not reported with StatusStat", p) + } +} From f2e55f54a795f34aa3e5dc34a8377c7f0df4b6fb Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Fri, 9 Oct 2026 13:37:06 -0700 Subject: [PATCH 3/7] ci: run CI for pushes to earthly-main and PRs into any branch The upstream merge took upstream's trigger block. That dropped the fork's push trigger (earthly-main, gh_test_ci) and limited pull_request to master/main. So CI no longer ran for PRs into earthly-main, including #3 itself. This brings back the push trigger from earthly-main (644b083). It also removes the branch filter from pull_request, so the workflow runs for every PR, whatever its base. A PR into earthly-main would have been enough, but stacked PRs (like this one, based on chore-merge-v0.14.1) need CI too. We keep upstream's workflow_dispatch and schedule triggers. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 283c4d4..75176d1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,10 +11,13 @@ on: workflow_dispatch: schedule: - cron: '0 8 */6 * *' # every 6 days - pull_request: + push: branches: - - master - - main + - earthly-main + - gh_test_ci + # No branch filter, so PRs stacked on other PR branches + # (not just PRs into earthly-main) get CI too. + pull_request: jobs: validate: From 59f0fd731998f3d8857a2c7bef618d3854c6b5f9 Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Fri, 9 Oct 2026 13:37:35 -0700 Subject: [PATCH 4/7] fix(send): count each packet once in send progress When the merge resolved the updateProgress conflict, it kept both sides. The fork increments progressCurrent under s.mu, and upstream increments it again under progressCurrentMu. So whenever progressCb is set, every STAT and DATA packet is counted twice. The callback also ran while s.mu was held. Upstream 216a08f added progressCurrentMu to avoid exactly that, because s.mu also guards s.files and the send queue. This takes upstream's version: a single increment under progressCurrentMu, and only when progressCb is set. Nothing else reads progressCurrent, so it no longer needs s.mu. Fixes TestSendProgressTotal. Co-Authored-By: Claude Opus 5.5 --- send.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/send.go b/send.go index 8e69422..ad3cc3e 100644 --- a/send.go +++ b/send.go @@ -114,9 +114,6 @@ func (s *sender) run(ctx context.Context) error { } func (s *sender) updateProgress(size int, last bool) { - s.mu.Lock() - defer s.mu.Unlock() - s.progressCurrent += size if s.progressCb != nil { s.progressCurrentMu.Lock() defer s.progressCurrentMu.Unlock() From cc66d5207ce9ecadc54aff47666336567ef3dc60 Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Fri, 9 Oct 2026 13:39:25 -0700 Subject: [PATCH 5/7] fix: report native paths in verbose progress on both sides Upstream now sends unix-style wire paths: the sender applies ToSlash and the receiver applies FromSlash. The merge put the fork's VerboseProgressCb calls around that change in a way that mixes the two forms on Windows. - Receiver: StatusStat was reported with p.Stat.Path before FromSlash, so it got the wire path ("dir/b"). StatusReceiving and StatusReceived get the native r.files key ("dir\b"). The call also ran before the "unrepresentable path" check, so a rejected path was still reported as stat'ed. - Sender: walk reports StatusStat with the native walk path, but sendFile reported StatusSent with h.path, which is the ToSlash'd value stored in s.files. Each side now reports native paths: - Receiver: StatusStat moves to after normalisation and the unrepresentable-path check, and reports the normalised path. - Sender: StatusSent reports filepath.FromSlash(h.path). Why native paths: - They match what the fork reported before the merge, when no conversion happened. - They match the receiver's r.files keys, pipeNames and NotifyHashed. - They match the paths the sender's walk callback gets. - EarthBuild's fsutilprogress keys per-file byte counts by this path across StatusReceiving and StatusReceived, so the two must agree. The receiver's StatusStat still fires before the order and hardlink validators, as it did before the merge. A validator error aborts the whole transfer anyway, so moving the call past them would add nothing useful. On Unix FromSlash and ToSlash do nothing, so behaviour there is unchanged. Fixes TestVerboseProgressPathsConsistent on Windows. Co-Authored-By: Claude Opus 5.5 --- receive.go | 10 ++++++---- send.go | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/receive.go b/receive.go index afb118b..dc73351 100644 --- a/receive.go +++ b/receive.go @@ -221,10 +221,6 @@ func (r *receiver) run(ctx context.Context) error { } break } - if r.verboseProgressCb != nil { - r.verboseProgressCb(p.Stat.Path, StatusStat, p.Size()) - } - // normalize unix wire-specific paths to platform-specific paths path := filepath.FromSlash(p.Stat.Path) if filepath.ToSlash(path) != p.Stat.Path { @@ -234,6 +230,12 @@ func (r *receiver) run(ctx context.Context) error { p.Stat.Path = path p.Stat.Linkname = filepath.FromSlash(p.Stat.Linkname) + // earthly-specific: report the native path, matching the + // StatusReceiving/StatusReceived paths (r.files keys) and NotifyHashed. + if r.verboseProgressCb != nil { + r.verboseProgressCb(path, StatusStat, p.Size()) + } + if fileCanRequestData(os.FileMode(p.Stat.Mode)) { r.mu.Lock() r.files[p.Stat.Path] = i diff --git a/send.go b/send.go index ad3cc3e..908ba64 100644 --- a/send.go +++ b/send.go @@ -146,7 +146,9 @@ func (s *sender) sendFile(h *sendHandle) error { return err } if s.verboseProgressCb != nil { - s.verboseProgressCb(h.path, StatusSent, fs.bytesWritten) + // h.path is the unix wire path from s.files; report the native + // path, matching the StatusStat path reported by walk. + s.verboseProgressCb(filepath.FromSlash(h.path), StatusSent, fs.bytesWritten) } } return s.conn.SendMsg(&types.Packet{ID: h.id, Type: types.PACKET_DATA}) From e66a226757f77ce33b857dccf7a9e8fb8ee7c2de Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Fri, 9 Oct 2026 13:47:40 -0700 Subject: [PATCH 6/7] ci: pin actions to full-length commit SHAs The EarthBuild org requires every action to be pinned to a full commit SHA; with tag pins every job failed at "Set up job" before running. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 75176d1..f061cf4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,13 +31,13 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 - name: Run - uses: docker/bake-action@v4 + uses: docker/bake-action@aefd381cbaa93c62a1e8b02194ae420cc36269d2 # v4.7.0 with: targets: ${{ matrix.target }} @@ -51,13 +51,13 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 - name: Run - uses: docker/bake-action@v4 + uses: docker/bake-action@aefd381cbaa93c62a1e8b02194ae420cc36269d2 # v4.7.0 with: targets: ${{ matrix.target }} @@ -76,18 +76,18 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 - name: Test - uses: docker/bake-action@v4 + uses: docker/bake-action@aefd381cbaa93c62a1e8b02194ae420cc36269d2 # v4.7.0 with: targets: test-${{ matrix.mode }} - name: Upload coverage - uses: codecov/codecov-action@v4 + uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4.6.0 with: directory: ${{ env.DESTDIR }}/coverage flags: unit,${{ matrix.mode }},go-${{ matrix.go }} @@ -107,10 +107,10 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Set up Go - uses: actions/setup-go@v5 + uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 with: go-version: ${{ matrix.go }} - @@ -121,7 +121,7 @@ jobs: - name: Upload coverage if: always() - uses: codecov/codecov-action@v4 + uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4.6.0 with: file: ./coverage.txt env_vars: RUNNER_OS @@ -135,10 +135,10 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Cache Vagrant boxes - uses: actions/cache@v4 + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 with: path: ~/.vagrant.d/boxes key: ${{ runner.os }}-vagrant-${{ hashFiles('hack/Vagrantfile.freebsd13') }} @@ -176,7 +176,7 @@ jobs: - name: Upload coverage if: always() - uses: codecov/codecov-action@v4 + uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4.6.0 with: file: ./coverage.txt flags: unit,freebsd From e2fbb01d11b911a227279f31b9c6c853704609a0 Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Fri, 9 Oct 2026 13:51:29 -0700 Subject: [PATCH 7/7] ci: bump docker/bake-action to v5 for buildx >= 0.20 bake-action < v5 refuses to run with the buildx now shipped on ubuntu-latest runners. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f061cf4..fb6598c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 - name: Run - uses: docker/bake-action@aefd381cbaa93c62a1e8b02194ae420cc36269d2 # v4.7.0 + uses: docker/bake-action@4a9a8d494466d37134e2bfca2d3a8de8fb2681ad # v5.13.0 with: targets: ${{ matrix.target }} @@ -57,7 +57,7 @@ jobs: uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 - name: Run - uses: docker/bake-action@aefd381cbaa93c62a1e8b02194ae420cc36269d2 # v4.7.0 + uses: docker/bake-action@4a9a8d494466d37134e2bfca2d3a8de8fb2681ad # v5.13.0 with: targets: ${{ matrix.target }} @@ -82,7 +82,7 @@ jobs: uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 - name: Test - uses: docker/bake-action@aefd381cbaa93c62a1e8b02194ae420cc36269d2 # v4.7.0 + uses: docker/bake-action@4a9a8d494466d37134e2bfca2d3a8de8fb2681ad # v5.13.0 with: targets: test-${{ matrix.mode }} -