From c13b09630ed22fe91be2eb6804670d8311f29aaa Mon Sep 17 00:00:00 2001 From: montehurd Date: Mon, 7 Sep 2026 12:36:48 -0700 Subject: [PATCH 1/2] Stop writing fileblob's .attrs sidecar fileblob stores blob metadata in an ".attrs" file per object and rewrites it with os.Create, truncating in place outside the atomic rename that protects the blob. A read overlapping a write decodes a partial file and fails with "opening reader: EOF", served as a 502. One writer against four readers on a single key failed 408 of 2000 reads. cacheMetadataBlob is most exposed to it, rewriting a key on every refresh while readers are served from it. Nothing in the proxy reads what the sidecar holds. gocloud.dev/blob is imported only by internal/storage, Store sets no ContentType, and Attributes is used only for Size, which comes from os.Stat. A missing sidecar already defaults cleanly, so "metadata=skip" removes the hazard rather than locking around it, and saves a write per store. --- internal/storage/blob.go | 9 +++- internal/storage/blob_test.go | 93 +++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 97e50f3f..4a1fd56f 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -87,7 +87,14 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { // This avoids "invalid cross-device link" errors from os.Rename when // the bucket directory and os.TempDir are on different filesystems // (e.g. Docker volume mounts). - urlStr += "?no_tmp_dir=true" + // + // Do not write fileblob's ".attrs" sidecar. It is rewritten with + // os.Create, truncating in place outside the atomic rename that + // protects the blob, so a read overlapping a write can decode a + // partial file; a missing one defaults cleanly, a truncated one does + // not. Nothing in the proxy needs it: Store sets no ContentType, and + // Size reads os.Stat via Attributes. + urlStr += "?no_tmp_dir=true&metadata=skip" } bucket, err := blob.OpenBucket(ctx, urlStr) diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index 3e5bf65b..b2877030 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -9,6 +9,8 @@ import ( "path/filepath" "runtime" "strings" + "sync" + "sync/atomic" "testing" "time" ) @@ -293,3 +295,94 @@ func fileURLFromPath(path string) string { } return "file://" + path } + +func TestOpenBucketWritesNoAttrsSidecar(t *testing.T) { + dir := t.TempDir() + ctx := context.Background() + + b, err := OpenBucket(ctx, fileURLFromPath(dir)) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + defer func() { _ = b.Close() }() + + if _, _, err := b.Store(ctx, "pkg/thing-1.0.0.tgz", strings.NewReader("content")); err != nil { + t.Fatalf("Store failed: %v", err) + } + + sidecars, err := filepath.Glob(filepath.Join(dir, "*", "*.attrs")) + if err != nil { + t.Fatalf("Glob failed: %v", err) + } + if len(sidecars) != 0 { + t.Errorf("got sidecar files %v, want none: a truncated sidecar fails reads that overlap a write", sidecars) + } +} + +// A read overlapping a write to the same key must not fail. fileblob rewrote +// its ".attrs" sidecar in place, so a reader decoding it mid-write saw a +// partial file, which the proxy served as a 502 on an artifact it held. +func TestConcurrentReadsSurviveWritesToSameKey(t *testing.T) { + const ( + key = "pkg/thing-1.0.0.tgz" + readers = 4 + readsPerRead = 500 + ) + dir := t.TempDir() + ctx := context.Background() + + b, err := OpenBucket(ctx, fileURLFromPath(dir)) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + defer func() { _ = b.Close() }() + + payload := strings.Repeat("x", 4096) + if _, _, err := b.Store(ctx, key, strings.NewReader(payload)); err != nil { + t.Fatalf("seeding Store failed: %v", err) + } + + done := make(chan struct{}) + var writers sync.WaitGroup + writers.Add(1) + go func() { + defer writers.Done() + for { + select { + case <-done: + return + default: + } + if _, _, err := b.Store(ctx, key, strings.NewReader(payload)); err != nil { + return + } + } + }() + + var failures atomic.Int64 + var reading sync.WaitGroup + for range readers { + reading.Add(1) + go func() { + defer reading.Done() + for range readsPerRead { + r, err := b.Open(ctx, key) + if err != nil { + failures.Add(1) + continue + } + if _, err := io.Copy(io.Discard, r); err != nil { + failures.Add(1) + } + _ = r.Close() + } + }() + } + reading.Wait() + close(done) + writers.Wait() + + if got := failures.Load(); got != 0 { + t.Errorf("%d of %d reads failed while one writer rewrote the same key, want 0", got, readers*readsPerRead) + } +} From 0695a3292e411b661fa7a8305c553c6c54599e7c Mon Sep 17 00:00:00 2001 From: montehurd Date: Mon, 7 Sep 2026 12:36:48 -0700 Subject: [PATCH 2/2] Clear .attrs sidecars left by earlier versions metadata=skip stops fileblob rewriting sidecars but does not delete ones already on disk, so a sidecar left partial by an interrupted write now fails every read of its key for good. Before, a later store repaired it by rewriting. Store therefore removes the sidecar for the key it writes. Removal is atomic where the rewrite was not, so a concurrent reader gets the whole old file or nothing. Delete already removes sidecars, so the two paths drain a cache between them. Deriving that path is necessary because fileblob's key escaping is unexported. It is the identity for a plain key and parts from one only for keys that are not valid local paths, which is what filepath.Localize rejects. That also keeps the removal inside the cache directory: without it a key holding ".." resolves outside. The clearing test runs one key per storage path the proxy builds, seeded through a bucket that still writes sidecars so the path under test is fileblob's own. --- internal/storage/blob.go | 42 +++++++- internal/storage/blob_test.go | 177 ++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+), 1 deletion(-) diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 4a1fd56f..4a4bbc52 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -22,11 +22,19 @@ import ( const osWindows = "windows" +// attrsExt is fileblob's sidecar suffix, kept only to clear sidecars an +// earlier version wrote. +const attrsExt = ".attrs" + // Blob implements Storage using gocloud.dev/blob. // Supports local filesystem (file://) and S3 (s3://) URLs. type Blob struct { bucket *blob.Bucket url string + + // fileRoot is the directory backing a file:// bucket, empty for cloud + // backends. Used only to clear sidecars an earlier version wrote. + fileRoot string } // OpenBucket opens a blob bucket from a URL. @@ -47,6 +55,8 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { return OpenGCS(ctx, urlStr) } + var fileRoot string + // Handle file:// URLs specially to create the directory if strings.HasPrefix(urlStr, "file://") { path := strings.TrimPrefix(urlStr, "file://") @@ -74,6 +84,8 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { return nil, fmt.Errorf("resolving path: %w", err) } + fileRoot = absPath + // Convert back to URL format with forward slashes urlPath := filepath.ToSlash(absPath) if runtime.GOOS == osWindows { @@ -102,10 +114,38 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { return nil, fmt.Errorf("opening bucket: %w", err) } - return &Blob{bucket: bucket, url: urlStr}, nil + return &Blob{bucket: bucket, url: urlStr, fileRoot: fileRoot}, nil +} + +// legacySidecarPath gives the ".attrs" path an earlier version wrote for key, +// or "" when the mapping is not certain. +// +// fileblob maps keys with an unexported escapeKey, so this derives it. +// escapeKey is the identity for a plain key and parts from one only for keys +// that are not valid local paths, which is what filepath.Localize rejects. +// Declining those keeps the removal inside fileRoot too: a key holding ".." +// would otherwise resolve outside the cache. A control character is the one +// case Localize accepts and escapeKey does not, where removal simply misses. +func (b *Blob) legacySidecarPath(key string) string { + if b.fileRoot == "" { + return "" + } + rel, err := filepath.Localize(key) + if err != nil { + return "" + } + return filepath.Join(b.fileRoot, rel) + attrsExt } func (b *Blob) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) { + // Drop any sidecar an earlier version left for this key. Nothing rewrites + // one now, so a partial sidecar from an interrupted write would fail every + // read of the key for good. Removal is atomic where the rewrite was not, + // so a concurrent reader gets the whole old file or nothing. + if sidecar := b.legacySidecarPath(path); sidecar != "" { + _ = os.Remove(sidecar) + } + // Compute hash while writing h := sha256.New() tee := io.TeeReader(r, h) diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index b2877030..bef61d4d 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -6,6 +6,8 @@ import ( "encoding/hex" "errors" "io" + "io/fs" + "os" "path/filepath" "runtime" "strings" @@ -13,6 +15,8 @@ import ( "sync/atomic" "testing" "time" + + "gocloud.dev/blob" ) func TestOpenBucket(t *testing.T) { @@ -386,3 +390,176 @@ func TestConcurrentReadsSurviveWritesToSameKey(t *testing.T) { t.Errorf("%d of %d reads failed while one writer rewrote the same key, want 0", got, readers*readsPerRead) } } + +// seedLegacySidecar stores key through a bucket that still writes sidecars, as +// an earlier version did, and returns the path fileblob actually used. It is +// discovered rather than assumed, so callers test the real mapping. +func seedLegacySidecar(t *testing.T, dir, key, payload string) string { + t.Helper() + ctx := context.Background() + + legacy, err := blob.OpenBucket(ctx, fileURLFromPath(dir)+"?no_tmp_dir=true") + if err != nil { + t.Fatalf("opening legacy bucket: %v", err) + } + if err := legacy.WriteAll(ctx, key, []byte(payload), nil); err != nil { + t.Fatalf("legacy WriteAll: %v", err) + } + if err := legacy.Close(); err != nil { + t.Fatalf("closing legacy bucket: %v", err) + } + + var found []string + walkErr := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() && strings.HasSuffix(path, ".attrs") { + found = append(found, path) + } + return nil + }) + if walkErr != nil { + t.Fatalf("walking %s: %v", dir, walkErr) + } + if len(found) != 1 { + t.Fatalf("got sidecars %v, want exactly one", found) + } + return found[0] +} + +// An interrupted setAttrs leaves a partial sidecar that fails every read of the +// key, and nothing rewrites one now, so a store has to clear it. +// +// One key per storage path the proxy builds: ArtifactPath across ecosystems, +// metadata blobs, and the Gradle build cache. Scoped npm names, Go's "!" case +// escaping and the ":" in OCI digests and Debian epochs are the characters +// most likely to part fileblob's mapping from a plain path join. +func TestStoreClearsLegacyAttrsSidecar(t *testing.T) { + keys := []string{ + "npm/@babel/core/7.24.0/core-7.24.0.tgz", + "maven/org.apache.commons/commons-lang3/3.14.0/commons-lang3-3.14.0.jar", + "golang/github.com/!burnt!sushi/toml/v1.3.2/v1.3.2.zip", + "oci/library/nginx/sha256:abc123def456/manifest", + "debian/tzdata/1:2024a-1/tzdata_2024a-1_all.deb", + "pypi/requests/2.31.0/requests-2.31.0-py3-none-any.whl", + "cargo/serde/1.0.197/serde-1.0.197.crate", + "julia/Example/a1b2c3/a1b2c3.tar.gz", + "conda/numpy/1.26.4/numpy-1.26.4-py311.conda", + "_metadata/npm/@babel/core/metadata", + "_gradle/http-build-cache/0a1b2c3d4e5f", + } + + for _, key := range keys { + t.Run(key, func(t *testing.T) { + assertStoreClearsSidecar(t, key) + }) + } +} + +func assertStoreClearsSidecar(t *testing.T, key string) { + t.Helper() + const payload = "payload" + ctx := context.Background() + dir := t.TempDir() + + sidecar := seedLegacySidecar(t, dir, key, payload) + if err := os.WriteFile(sidecar, []byte(`{"user.content_type":"appl`), 0o600); err != nil { + t.Fatalf("corrupting sidecar: %v", err) + } + + b := openFileBlob(t, dir) + if _, err := b.Open(ctx, key); err == nil { + t.Fatal("corrupt sidecar did not fail the read, so it is not the file fileblob reads for this key") + } + + derived := b.legacySidecarPath(key) + if _, _, err := b.Store(ctx, key, strings.NewReader(payload)); err != nil { + t.Fatalf("Store failed: %v", err) + } + + if derived == "" { + assertSidecarKept(t, sidecar) + return + } + if derived != sidecar { + t.Fatalf("derived %q, but fileblob wrote %q", derived, sidecar) + } + if _, err := os.Stat(sidecar); !os.IsNotExist(err) { + t.Errorf("sidecar still present after Store, stat err = %v", err) + } + assertReadsBack(t, b, key, payload) +} + +// Windows rejects ":" in a local path and fileblob escapes it, so for those +// keys the mapping is not certain and the sidecar is left alone. +func assertSidecarKept(t *testing.T, sidecar string) { + t.Helper() + if runtime.GOOS != osWindows { + t.Fatalf("declined a key that is a plain local path on %s", runtime.GOOS) + } + if _, err := os.Stat(sidecar); err != nil { + t.Errorf("declined key should keep its sidecar, stat err = %v", err) + } +} + +func assertReadsBack(t *testing.T, b *Blob, key, want string) { + t.Helper() + r, err := b.Open(context.Background(), key) + if err != nil { + t.Fatalf("read still failing after Store cleared the sidecar: %v", err) + } + defer func() { _ = r.Close() }() + got, err := io.ReadAll(r) + if err != nil { + t.Fatalf("ReadAll failed: %v", err) + } + if string(got) != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func openFileBlob(t *testing.T, dir string) *Blob { + t.Helper() + s, err := OpenBucket(context.Background(), fileURLFromPath(dir)) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + t.Cleanup(func() { _ = s.Close() }) + b, ok := s.(*Blob) + if !ok { + t.Fatalf("got %T, want *Blob", s) + } + return b +} + +// fileblob escapes a non-local key in a way this cannot reproduce, and one +// holding ".." resolves outside the cache directory. Removal declines both +// rather than delete the wrong file. +func TestLegacySidecarPathDeclinesNonLocalKeys(t *testing.T) { + b := &Blob{fileRoot: filepath.FromSlash("/var/cache/proxy")} + + for _, key := range []string{ + "npm/pkg//1.0.0/x.tgz", + "npm/pkg/../../../../etc/passwd", + "npm/pkg/1.0.0/", + "/etc/passwd", + "", + } { + if got := b.legacySidecarPath(key); got != "" { + t.Errorf("legacySidecarPath(%q) = %q, want \"\"", key, got) + } + } + + if got := b.legacySidecarPath("npm/pkg/1.0.0/x.tgz"); got == "" { + t.Error("a plain key must still map to a sidecar path") + } +} + +// Cloud backends have no local directory, so nothing is removed for them. +func TestLegacySidecarPathEmptyForCloudBackends(t *testing.T) { + b := &Blob{} + if got := b.legacySidecarPath("npm/pkg/1.0.0/x.tgz"); got != "" { + t.Errorf("legacySidecarPath = %q, want \"\" when there is no file root", got) + } +}