From dd3a5634a35d3716b3b40884ac7113264364a39b Mon Sep 17 00:00:00 2001 From: Jacob Gillespie Date: Tue, 26 May 2026 21:13:56 +0100 Subject: [PATCH 1/2] Avoid re-uploading existing Go cache entries --- pkg/cmd/gocache/gocache.go | 28 ++++++++----- pkg/cmd/gocache/gocache_test.go | 74 +++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 pkg/cmd/gocache/gocache_test.go diff --git a/pkg/cmd/gocache/gocache.go b/pkg/cmd/gocache/gocache.go index 4bb6e6da..157aea94 100644 --- a/pkg/cmd/gocache/gocache.go +++ b/pkg/cmd/gocache/gocache.go @@ -425,7 +425,7 @@ func (c *RemoteCache) Get(ctx context.Context, actionID string) (outputID, diskP return "", "", nil } - diskPath, err = c.Disk.Put(ctx, actionID, outputID, int64(size), bytes.NewReader(buf)) + diskPath, _, err = c.Disk.Put(ctx, actionID, outputID, int64(size), bytes.NewReader(buf)) if err != nil { if c.Verbose { log.Printf("unable to cache actionID %s to disk: %v", actionID, err) @@ -475,12 +475,12 @@ func (c *RemoteCache) Put(ctx context.Context, actionID, outputID string, size i } buf := b.Bytes() - diskPath, err = c.Disk.Put(ctx, actionID, outputID, size, bytes.NewReader(buf[headerSize:])) + diskPath, wrote, err := c.Disk.Put(ctx, actionID, outputID, size, bytes.NewReader(buf[headerSize:])) if err != nil { return "", fmt.Errorf("unable to write actionID %s to disk: %w", actionID, err) } - if len(outputID) == 0 { + if len(outputID) == 0 || !wrote { return diskPath, nil } @@ -586,16 +586,22 @@ func (dc *DiskCache) OutputFilename(objectID string) string { return fileNameOutput(dc.Dir, objectID) } -func (dc *DiskCache) Put(ctx context.Context, actionID, objectID string, size int64, body io.Reader) (diskPath string, _ error) { +func (dc *DiskCache) Put(ctx context.Context, actionID, objectID string, size int64, body io.Reader) (diskPath string, wrote bool, _ error) { file := fileNameOutput(dc.Dir, objectID) actionFile := fileNameAction(dc.Dir, actionID) + if err := os.MkdirAll(filepath.Dir(file), 0755); err != nil { + return "", false, err + } + if err := os.MkdirAll(filepath.Dir(actionFile), 0755); err != nil { + return "", false, err + } // Skip writing the file if it already exists and is the right size. stat, err := os.Stat(file) if err == nil && stat.Size() == size { _, err = os.Stat(actionFile) if err == nil { - return file, nil + return file, false, nil } } @@ -603,16 +609,16 @@ func (dc *DiskCache) Put(ctx context.Context, actionID, objectID string, size in if size == 0 { zf, err := os.OpenFile(file, os.O_CREATE|os.O_RDWR, 0644) if err != nil { - return "", err + return "", false, err } zf.Close() } else { wrote, err := writeAtomic(file, body) if err != nil { - return "", fmt.Errorf("unable to write to disk: %w", err) + return "", false, fmt.Errorf("unable to write to disk: %w", err) } if wrote != size { - return "", fmt.Errorf("wrote %d bytes, expected %d", wrote, size) + return "", false, fmt.Errorf("wrote %d bytes, expected %d", wrote, size) } } @@ -623,13 +629,13 @@ func (dc *DiskCache) Put(ctx context.Context, actionID, objectID string, size in TimeNanos: time.Now().UnixNano(), }) if err != nil { - return "", err + return "", false, err } if _, err := writeAtomic(actionFile, bytes.NewReader(ij)); err != nil { - return "", err + return "", false, err } - return file, nil + return file, true, nil } func writeAtomic(dest string, r io.Reader) (int64, error) { diff --git a/pkg/cmd/gocache/gocache_test.go b/pkg/cmd/gocache/gocache_test.go new file mode 100644 index 00000000..e5a77277 --- /dev/null +++ b/pkg/cmd/gocache/gocache_test.go @@ -0,0 +1,74 @@ +package gocache + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" +) + +func TestRemoteCachePutSkipsRemoteUploadWhenDiskAlreadyHasEntry(t *testing.T) { + var puts atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPut { + t.Fatalf("unexpected method %s", r.Method) + } + puts.Add(1) + w.WriteHeader(http.StatusCreated) + })) + defer server.Close() + + dir := t.TempDir() + newCache := func() (*RemoteCache, context.CancelFunc) { + ctx, cancel := context.WithCancel(context.Background()) + return &RemoteCache{ + BaseURL: server.URL, + Token: "token", + Disk: &DiskCache{Dir: dir}, + Ctx: ctx, + CtxCancel: cancel, + }, cancel + } + + cache, cancel := newCache() + _, err := cache.Put(context.Background(), "abc123", "def456", 4, bytes.NewReader([]byte("data"))) + if err != nil { + t.Fatalf("first Put returned error: %v", err) + } + _ = cache.Close() + cancel() + + cache, cancel = newCache() + _, err = cache.Put(context.Background(), "abc123", "def456", 4, bytes.NewReader([]byte("data"))) + if err != nil { + t.Fatalf("second Put returned error: %v", err) + } + _ = cache.Close() + cancel() + + if got := puts.Load(); got != 1 { + t.Fatalf("remote PUT count = %d, want 1", got) + } +} + +func TestDiskCachePutReportsNoWriteForExistingEntry(t *testing.T) { + dc := &DiskCache{Dir: t.TempDir()} + + _, wrote, err := dc.Put(context.Background(), "abc123", "def456", 4, bytes.NewReader([]byte("data"))) + if err != nil { + t.Fatalf("first Put returned error: %v", err) + } + if !wrote { + t.Fatal("first Put reported no write") + } + + _, wrote, err = dc.Put(context.Background(), "abc123", "def456", 4, bytes.NewReader([]byte("data"))) + if err != nil { + t.Fatalf("second Put returned error: %v", err) + } + if wrote { + t.Fatal("second Put reported write") + } +} From f51083f84d6a49e13ded1b048480e324b4db8706 Mon Sep 17 00:00:00 2001 From: Jacob Gillespie Date: Wed, 27 May 2026 12:19:13 +0100 Subject: [PATCH 2/2] Remove named returns --- pkg/cmd/gocache/gocache.go | 40 ++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/gocache/gocache.go b/pkg/cmd/gocache/gocache.go index 157aea94..f5a4a98b 100644 --- a/pkg/cmd/gocache/gocache.go +++ b/pkg/cmd/gocache/gocache.go @@ -208,8 +208,9 @@ func (p *Cache) handleRequest(ctx context.Context, req *wire.ProgRequest, res *w } } -func (p *Cache) handleGet(ctx context.Context, req *wire.ProgRequest, res *wire.ProgResponse) (retErr error) { +func (p *Cache) handleGet(ctx context.Context, req *wire.ProgRequest, res *wire.ProgResponse) error { p.Gets.Add(1) + var retErr error defer func() { if retErr != nil { log.Printf("get(action %x): %v", req.ActionID, retErr) @@ -222,18 +223,21 @@ func (p *Cache) handleGet(ctx context.Context, req *wire.ProgRequest, res *wire. }() outputID, diskPath, err := p.RemoteCache.Get(ctx, fmt.Sprintf("%x", req.ActionID)) if err != nil { - return err + retErr = err + return retErr } if outputID == "" && diskPath == "" { res.Miss = true return nil } if outputID == "" { - return errors.New("no outputID") + retErr = errors.New("no outputID") + return retErr } res.OutputID, err = hex.DecodeString(outputID) if err != nil { - return fmt.Errorf("invalid OutputID: %v", err) + retErr = fmt.Errorf("invalid OutputID: %v", err) + return retErr } fi, err := os.Stat(diskPath) if err != nil { @@ -241,10 +245,12 @@ func (p *Cache) handleGet(ctx context.Context, req *wire.ProgRequest, res *wire. res.Miss = true return nil } - return err + retErr = err + return retErr } if !fi.Mode().IsRegular() { - return fmt.Errorf("not a regular file") + retErr = fmt.Errorf("not a regular file") + return retErr } res.Size = fi.Size() time := fi.ModTime() @@ -253,7 +259,7 @@ func (p *Cache) handleGet(ctx context.Context, req *wire.ProgRequest, res *wire. return nil } -func (p *Cache) handlePut(ctx context.Context, req *wire.ProgRequest, res *wire.ProgResponse) (retErr error) { +func (p *Cache) handlePut(ctx context.Context, req *wire.ProgRequest, res *wire.ProgResponse) error { if req.OutputID == nil && req.ObjectID != nil { req.OutputID = req.ObjectID } @@ -267,6 +273,7 @@ func (p *Cache) handlePut(ctx context.Context, req *wire.ProgRequest, res *wire. actionID, objectID := fmt.Sprintf("%x", req.ActionID), fmt.Sprintf("%x", req.OutputID) p.Puts.Add(1) + var retErr error defer func() { if retErr != nil { p.PutErrors.Add(1) @@ -280,14 +287,17 @@ func (p *Cache) handlePut(ctx context.Context, req *wire.ProgRequest, res *wire. } diskPath, err := p.RemoteCache.Put(ctx, actionID, objectID, req.BodySize, body) if err != nil { - return err + retErr = err + return retErr } fi, err := os.Stat(diskPath) if err != nil { - return fmt.Errorf("stat after successful Put: %w", err) + retErr = fmt.Errorf("stat after successful Put: %w", err) + return retErr } if fi.Size() != req.BodySize { - return fmt.Errorf("failed to write file to disk with right size: disk=%v; wanted=%v", fi.Size(), req.BodySize) + retErr = fmt.Errorf("failed to write file to disk with right size: disk=%v; wanted=%v", fi.Size(), req.BodySize) + return retErr } res.DiskPath = diskPath return nil @@ -339,8 +349,8 @@ func (c *RemoteCache) httpClient() *http.Client { return http.DefaultClient } -func (c *RemoteCache) Get(ctx context.Context, actionID string) (outputID, diskPath string, err error) { - outputID, diskPath, err = c.Disk.Get(ctx, actionID) +func (c *RemoteCache) Get(ctx context.Context, actionID string) (string, string, error) { + outputID, diskPath, err := c.Disk.Get(ctx, actionID) if err == nil && outputID != "" { return outputID, diskPath, nil } @@ -440,7 +450,7 @@ func (c *RemoteCache) Get(ctx context.Context, actionID string) (outputID, diskP return outputID, diskPath, nil } -func (c *RemoteCache) Put(ctx context.Context, actionID, outputID string, size int64, body io.Reader) (diskPath string, _ error) { +func (c *RemoteCache) Put(ctx context.Context, actionID, outputID string, size int64, body io.Reader) (string, error) { if size < 0 { return "", fmt.Errorf("negative size %d", size) } @@ -544,7 +554,7 @@ type DiskCache struct { Verbose bool } -func (dc *DiskCache) Get(ctx context.Context, actionID string) (outputID, diskPath string, err error) { +func (dc *DiskCache) Get(ctx context.Context, actionID string) (string, string, error) { actionFile := fileNameAction(dc.Dir, actionID) ij, err := os.ReadFile(actionFile) if err != nil { @@ -586,7 +596,7 @@ func (dc *DiskCache) OutputFilename(objectID string) string { return fileNameOutput(dc.Dir, objectID) } -func (dc *DiskCache) Put(ctx context.Context, actionID, objectID string, size int64, body io.Reader) (diskPath string, wrote bool, _ error) { +func (dc *DiskCache) Put(ctx context.Context, actionID, objectID string, size int64, body io.Reader) (string, bool, error) { file := fileNameOutput(dc.Dir, objectID) actionFile := fileNameAction(dc.Dir, actionID) if err := os.MkdirAll(filepath.Dir(file), 0755); err != nil {