Stop writing fileblob's .attrs sidecar

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 {
@@ -87,18 +99,53 @@ 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)
 	if err != nil {
 		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)

internal/storage/blob_test.go

@@ -6,11 +6,17 @@ import (
 	"encoding/hex"
 	"errors"
 	"io"
+	"io/fs"
+	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
+	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
+
+	"gocloud.dev/blob"
 )
 
 func TestOpenBucket(t *testing.T) {
@@ -293,3 +299,267 @@ 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)
+	}
+}
+
+// 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)
+	}
+}