From db27c2cf0e12786115c128b5b63017db854869e6 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Oct 2025 12:56:42 +0100 Subject: [PATCH] cachers,cmd/go-cacher-server: fix unexpected http 404s Running with a cold disk cache for go-cacher and a warm disk cache for go-cacher-server, I was getting no cache hits. This is due to a missing component in the output filename when serving up what go-cacher-server thinks is a cache hit; it was trying to serve from `/o-` instead of the real file location of `//o-` where o2 is the first 2 characters of the hex string. To help with consistency, I consolidated all places that need to calculate an action or output file path into 2 shared functions, but there is only one functional change. Tested via the benchmarks I included in #17, but it's probably worth investing in some unit tests as well as we start to use this more seriously. Signed-off-by: Tom Proctor --- cachers/disk.go | 39 +++++------ cmd/go-cacher-server/cacher-server.go | 38 +++++++++- cmd/go-cacher-server/cacher-server_test.go | 80 ++++++++++++++++++++++ 3 files changed, 134 insertions(+), 23 deletions(-) create mode 100644 cmd/go-cacher-server/cacher-server_test.go diff --git a/cachers/disk.go b/cachers/disk.go index 3d64e67..b78937f 100644 --- a/cachers/disk.go +++ b/cachers/disk.go @@ -39,9 +39,8 @@ func (dc *DiskCache) Get(ctx context.Context, actionID string) (outputID, diskPa if !validHex(actionID) { return "", "", fmt.Errorf("actionID must be valid hex strings") } - action2 := actionID[:2] - actionFile := filepath.Join(dc.Dir, action2, fmt.Sprintf("a-%s", actionID)) + actionFile := dc.ActionFilename(actionID) ij, err := os.ReadFile(actionFile) if err != nil { if os.IsNotExist(err) { @@ -61,30 +60,28 @@ func (dc *DiskCache) Get(ctx context.Context, actionID string) (outputID, diskPa // Protect against malicious non-hex OutputID on disk return "", "", nil } - output2 := ie.OutputID[:2] - return ie.OutputID, filepath.Join(dc.Dir, output2, fmt.Sprintf("o-%v", ie.OutputID)), nil + return ie.OutputID, dc.OutputFilename(ie.OutputID), nil } func (dc *DiskCache) OutputFilename(outputID string) string { - if len(outputID) < 4 || len(outputID) > 1000 { + if !validHex(outputID) { return "" } - for i := range outputID { - b := outputID[i] - if b >= '0' && b <= '9' || b >= 'a' && b <= 'f' { - continue - } + return filepath.Join(dc.Dir, outputID[:2], fmt.Sprintf("o-%s", outputID)) +} + +func (dc *DiskCache) ActionFilename(actionID string) string { + if !validHex(actionID) { return "" } - return filepath.Join(dc.Dir, fmt.Sprintf("o-%s", outputID)) + return filepath.Join(dc.Dir, actionID[:2], fmt.Sprintf("a-%s", actionID)) } func validHex(x string) bool { if len(x) < 4 || len(x) > 100 { return false } - for i := range len(x) { - b := x[i] + for _, b := range x { if b >= '0' && b <= '9' || b >= 'a' && b <= 'f' { continue } @@ -106,9 +103,10 @@ func (dc *DiskCache) Put(ctx context.Context, actionID, outputID string, size in return "", errors.New("outputID must be hex") } - action2, output2 := actionID[:2], outputID[:2] - outputDir := filepath.Join(dc.Dir, output2) - actionDir := filepath.Join(dc.Dir, action2) + actionFile := dc.ActionFilename(actionID) + outputFile := dc.OutputFilename(outputID) + actionDir := filepath.Dir(actionFile) + outputDir := filepath.Dir(outputFile) if err := os.MkdirAll(actionDir, 0755); err != nil { return "", fmt.Errorf("failed to create action directory: %w", err) @@ -117,17 +115,15 @@ func (dc *DiskCache) Put(ctx context.Context, actionID, outputID string, size in return "", fmt.Errorf("failed to create output directory: %w", err) } - file := filepath.Join(outputDir, fmt.Sprintf("o-%s", outputID)) - // Special case empty files; they're both common and easier to do race-free. if size == 0 { - zf, err := os.OpenFile(file, os.O_CREATE|os.O_RDWR, 0644) + zf, err := os.OpenFile(outputFile, os.O_CREATE|os.O_RDWR, 0644) if err != nil { return "", err } zf.Close() } else { - wrote, err := writeAtomic(file, body) + wrote, err := writeAtomic(outputFile, body) if err != nil { return "", err } @@ -145,11 +141,10 @@ func (dc *DiskCache) Put(ctx context.Context, actionID, outputID string, size in if err != nil { return "", err } - actionFile := filepath.Join(actionDir, fmt.Sprintf("a-%s", actionID)) if _, err := writeAtomic(actionFile, bytes.NewReader(ij)); err != nil { return "", err } - return file, nil + return outputFile, nil } func writeAtomic(dest string, r io.Reader) (int64, error) { diff --git a/cmd/go-cacher-server/cacher-server.go b/cmd/go-cacher-server/cacher-server.go index 9de26de..7298aff 100644 --- a/cmd/go-cacher-server/cacher-server.go +++ b/cmd/go-cacher-server/cacher-server.go @@ -76,7 +76,11 @@ type server struct { func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) { time.Sleep(s.latency) if s.verbose { - log.Printf("%s %s", r.Method, r.RequestURI) + lw := newLoggingResponseWriter(w) + w = lw + defer func() { + log.Printf("%d %s %s ", lw.StatusCode(), r.Method, r.RequestURI) + }() } if r.Method == "PUT" { s.handlePut(w, r) @@ -185,3 +189,35 @@ func (s *server) handlePut(w http.ResponseWriter, r *http.Request) { } w.WriteHeader(http.StatusNoContent) } + +// loggingResponseWriter is a trivial wrapper around http.ResponseWriter to +// capture the returned status code for logging. +type loggingResponseWriter struct { + http.ResponseWriter + + statusCodeSet bool + statusCode int +} + +func newLoggingResponseWriter(w http.ResponseWriter) *loggingResponseWriter { + return &loggingResponseWriter{ + ResponseWriter: w, + } +} + +func (w *loggingResponseWriter) WriteHeader(statusCode int) { + if !w.statusCodeSet { + w.statusCode = statusCode + w.statusCodeSet = true + } + + w.ResponseWriter.WriteHeader(statusCode) +} + +func (w *loggingResponseWriter) StatusCode() int { + if !w.statusCodeSet { + return http.StatusOK + } + + return w.statusCode +} diff --git a/cmd/go-cacher-server/cacher-server_test.go b/cmd/go-cacher-server/cacher-server_test.go new file mode 100644 index 0000000..4b67347 --- /dev/null +++ b/cmd/go-cacher-server/cacher-server_test.go @@ -0,0 +1,80 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "context" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/bradfitz/go-tool-cache/cachers" +) + +const ( + actionID = "aaaaaaaa" + outputID = "bbbbbbbb" + content = "hello" +) + +func TestCacheHits(t *testing.T) { + srvDir := t.TempDir() + srvDiskCache := &cachers.DiskCache{Dir: srvDir} + + srv := httptest.NewServer(&server{ + cache: srvDiskCache, + }) + t.Cleanup(srv.Close) + + clientDir := t.TempDir() + hc := &cachers.HTTPClient{ + BaseURL: srv.URL, + Disk: &cachers.DiskCache{Dir: clientDir}, + Verbose: *verbose, + } + + // Initially, expect cache miss. + expectCacheHit(t, hc, false) + + // Now populate the server cache, expect cache hit over HTTP. + srvPath, err := srvDiskCache.Put(context.Background(), actionID, outputID, int64(len(content)), strings.NewReader(content)) + if err != nil { + t.Fatal(err) + } + if srvPath == "" { + t.Fatal("got empty path from Put") + } + + expectCacheHit(t, hc, true) + + // Delete it from the server's cache, still expect cache hit due to local disk cache. + if err := os.Remove(srvPath); err != nil { + t.Fatal(err) + } + expectCacheHit(t, hc, true) +} + +func expectCacheHit(t *testing.T, hc *cachers.HTTPClient, hit bool) { + t.Helper() + + gotOutputID, clientPath, err := hc.Get(context.Background(), actionID) + if err != nil { + t.Fatal(err) + } + + if hit { + if gotOutputID != outputID { + t.Fatalf("got outputID %q; want %q", gotOutputID, outputID) + } + if gotContent, err := os.ReadFile(clientPath); err != nil || string(gotContent) != content { + t.Fatalf("read %q; content: %q, err: %v", clientPath, gotContent, err) + } + } else { + if gotOutputID != "" || clientPath != "" { + t.Fatalf("got %q, %q; want empty (cache miss)", gotOutputID, clientPath) + } + } +}