refactor(storage): address PR #2246 review feedback

Addresses dobrac's review comments on the compression primitives PR:

  Serialization:
  - Split serialization.go into serialization_v3.go and serialization_v4.go
  - Each version's wire format is self-contained in one file
  - Move Metadata types/constants to metadata.go
  - Remove unused exported Serialize() wrapper

  V4 wire format:
  - Replace packed CompressionTypeNumFrames uint64 with separate
    CompressionType uint32 + NumFrames uint32 (no bit-shifting)
  - Remove MaxCompressedHeaderSize limit (uint32 prefix + LZ4 frame
    boundary are sufficient)

  Naming and reuse:
  - Rename AddFrames → SetFrames (replaces, not appends)
  - Use SetFrames in MergeMappings instead of inline Subset calls
  - Remove unnecessary maps.Clone in ToDiffHeader

  Formatting:
  - Use decimal (%d) instead of hex (%#x) in error messages and
    String() methods for consistency with the rest of the codebase

  Tests:
  - Add test for uploadPartSlices (MD5 hashing, body concatenation)

  Documentation:
  - Flag BuildFiles incompleteness: only contains builds from current
    upload session, not upstream dependencies

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: levb

packages/orchestrator/pkg/sandbox/block/chunk.go

@@ -213,7 +213,7 @@ func (c *Chunker) fetch(ctx context.Context, off int64, ft *storage.FrameTable)
 	if ft.IsCompressed() {
 		frameStarts, frameSize, err := ft.FrameFor(off)
 		if err != nil {
-			return fmt.Errorf("failed to get frame for offset %#x: %w", off, err)
+			return fmt.Errorf("failed to get frame for offset %d: %w", off, err)
 		}
 
 		chunkOff = frameStarts.U
@@ -302,7 +302,7 @@ func (c *Chunker) runFetch(ctx context.Context, session *fetchSession, offsetU i
 	_, err = file.GetFrame(ctx, offsetU, ft, compressed, mmapSlice[:session.chunkLen], readSize, onRead)
 	if err != nil {
 		timer.RecordRaw(ctx, session.chunkLen, attrs.remoteFailure)
-		session.setError(fmt.Errorf("failed to fetch data at %#x: %w", offsetU, err), false)
+		session.setError(fmt.Errorf("failed to fetch data at %d: %w", offsetU, err), false)
 
 		return
 	}

packages/orchestrator/pkg/sandbox/build_upload.go

@@ -390,8 +390,8 @@ func (p *PendingBuildInfo) applyToHeader(h *headers.Header, fileType string) err
 			continue
 		}
 
-		if err := mapping.AddFrames(info.ft); err != nil {
-			return fmt.Errorf("apply frames to mapping at offset %#x for build %s: %w",
+		if err := mapping.SetFrames(info.ft); err != nil {
+			return fmt.Errorf("apply frames to mapping at offset %d for build %s: %w",
 				mapping.Offset, mapping.BuildId.String(), err)
 		}
 	}

packages/shared/pkg/storage/compress_frame_table.go

@@ -53,7 +53,7 @@ type FrameOffset struct {
 }
 
 func (o *FrameOffset) String() string {
-	return fmt.Sprintf("U:%#x/C:%#x", o.U, o.C)
+	return fmt.Sprintf("U:%d/C:%d", o.U, o.C)
 }
 
 func (o *FrameOffset) Add(f FrameSize) {
@@ -67,7 +67,7 @@ type FrameSize struct {
 }
 
 func (s FrameSize) String() string {
-	return fmt.Sprintf("U:%#x/C:%#x", s.U, s.C)
+	return fmt.Sprintf("U:%d/C:%d", s.U, s.C)
 }
 
 type Range struct {
@@ -76,7 +76,7 @@ type Range struct {
 }
 
 func (r Range) String() string {
-	return fmt.Sprintf("%#x/%#x", r.Start, r.Length)
+	return fmt.Sprintf("%d/%d", r.Start, r.Length)
 }
 
 type FrameTable struct {
@@ -196,7 +196,7 @@ func (ft *FrameTable) FrameFor(offset int64) (starts FrameOffset, size FrameSize
 		currentOffset.Add(frame)
 	}
 
-	return FrameOffset{}, FrameSize{}, fmt.Errorf("offset %#x is beyond the end of the frame table", offset)
+	return FrameOffset{}, FrameSize{}, fmt.Errorf("offset %d is beyond the end of the frame table", offset)
 }
 
 // GetFetchRange translates a U-space range to C-space using the frame table.
@@ -205,12 +205,12 @@ func (ft *FrameTable) GetFetchRange(rangeU Range) (Range, error) {
 	if ft.IsCompressed() {
 		start, size, err := ft.FrameFor(rangeU.Start)
 		if err != nil {
-			return Range{}, fmt.Errorf("getting frame for offset %#x: %w", rangeU.Start, err)
+			return Range{}, fmt.Errorf("getting frame for offset %d: %w", rangeU.Start, err)
 		}
 		endOffset := rangeU.Start + int64(rangeU.Length)
 		frameEnd := start.U + int64(size.U)
 		if endOffset > frameEnd {
-			return Range{}, fmt.Errorf("range %v spans beyond frame ending at %#x", rangeU, frameEnd)
+			return Range{}, fmt.Errorf("range %v spans beyond frame ending at %d", rangeU, frameEnd)
 		}
 		fetchRange = Range{
 			Start:  start.C,

packages/shared/pkg/storage/compress_upload.go

@@ -14,11 +14,6 @@ import (
 	"golang.org/x/sync/errgroup"
 )
 
-// MaxCompressedHeaderSize is the maximum allowed decompressed header size (64 MiB).
-// Headers are typically a few hundred KiB (e.g., 100 layers × 256 frames × 32 bytes/frame ≈ 800 KB).
-// This is a safety bound to prevent unbounded allocation from corrupt data.
-const MaxCompressedHeaderSize = 64 << 20
-
 const (
 	// DefaultCompressFrameSize is the default uncompressed size of each compression
 	// frame (2 MiB). Overridable via CompressConfig.FrameSizeKB.

packages/shared/pkg/storage/gcp_multipart_test.go

@@ -1,6 +1,8 @@
 package storage
 
 import (
+	"crypto/md5"
+	"encoding/base64"
 	"encoding/xml"
 	"fmt"
 	"io"
@@ -115,6 +117,42 @@ func TestMultipartUploader_UploadPart_Success(t *testing.T) {
 	require.Equal(t, expectedETag, etag)
 }
 
+func TestMultipartUploader_UploadPartSlices_Success(t *testing.T) {
+	t.Parallel()
+	expectedETag := `"slice-etag"`
+	slices := [][]byte{[]byte("hello "), []byte("world"), []byte("!")}
+
+	// Compute expected MD5 over all slices.
+	h := md5.New()
+	for _, s := range slices {
+		h.Write(s)
+	}
+	expectedMD5 := base64.StdEncoding.EncodeToString(h.Sum(nil))
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		assert.Equal(t, "PUT", r.Method)
+		assert.Contains(t, r.URL.RawQuery, "partNumber=3")
+		assert.Contains(t, r.URL.RawQuery, "uploadId=test-upload-id")
+
+		// Verify MD5 matches the expected hash of all slices.
+		assert.Equal(t, expectedMD5, r.Header.Get("Content-MD5"))
+
+		// Verify body is the concatenation of all slices.
+		body, err := io.ReadAll(r.Body)
+		assert.NoError(t, err)
+		assert.Equal(t, []byte("hello world!"), body)
+
+		w.Header().Set("ETag", expectedETag)
+		w.WriteHeader(http.StatusOK)
+	})
+
+	uploader := createTestMultipartUploader(t, handler)
+	etag, err := uploader.uploadPartSlices(t.Context(), "test-upload-id", 3, slices)
+
+	require.NoError(t, err)
+	require.Equal(t, expectedETag, etag)
+}
+
 func TestMultipartUploader_UploadPart_MissingETag(t *testing.T) {
 	t.Parallel()
 	handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {

packages/shared/pkg/storage/header/header.go

@@ -25,8 +25,14 @@ type BuildFileInfo struct {
 const NormalizeFixVersion = 3
 
 type Header struct {
-	Metadata    *Metadata
-	BuildFiles  map[uuid.UUID]BuildFileInfo // V4 only: per-build file size + checksum
+	Metadata *Metadata
+	// BuildFiles maps build IDs to their file metadata (size + checksum).
+	// NOTE: This is currently incomplete — it only contains entries for builds
+	// uploaded within the same layered upload session. Upstream dependency builds
+	// (from parent templates) are missing, causing a Size() RPC fallback on first
+	// access. TODO: populate from the orchestrator's template cache at upload time
+	// so all builds referenced in Mapping have entries here.
+	BuildFiles  map[uuid.UUID]BuildFileInfo
 	blockStarts *bitset.BitSet
 	startMap    map[int64]*BuildMap
 
@@ -135,7 +141,7 @@ func (t *Header) Mappings(all bool) string {
 		if m.FrameTable != nil {
 			frames = len(m.FrameTable.Frames)
 		}
-		result += fmt.Sprintf("  - Offset: %#x, Length: %#x, BuildId: %s, BuildStorageOffset: %#x, numFrames: %d\n",
+		result += fmt.Sprintf("  - Offset: %d, Length: %d, BuildId: %s, BuildStorageOffset: %d, numFrames: %d\n",
 			m.Offset,
 			m.Length,
 			m.BuildId.String(),
@@ -272,7 +278,7 @@ func ValidateHeader(h *Header) error {
 
 	// Check that first mapping starts at 0
 	if sortedMappings[0].Offset != 0 {
-		return fmt.Errorf("mappings don't start at 0: first mapping starts at %#x for buildId %s",
+		return fmt.Errorf("mappings don't start at 0: first mapping starts at %d for buildId %s",
 			sortedMappings[0].Offset, h.Metadata.BuildId.String())
 	}
 
@@ -282,11 +288,11 @@ func ValidateHeader(h *Header) error {
 		nextStart := sortedMappings[i+1].Offset
 
 		if currentEnd < nextStart {
-			return fmt.Errorf("gap in mappings: mapping[%d] ends at %#x but mapping[%d] starts at %#x (gap=%d bytes) for buildId %s",
+			return fmt.Errorf("gap in mappings: mapping[%d] ends at %d but mapping[%d] starts at %d (gap=%d bytes) for buildId %s",
 				i, currentEnd, i+1, nextStart, nextStart-currentEnd, h.Metadata.BuildId.String())
 		}
 		if currentEnd > nextStart {
-			return fmt.Errorf("overlap in mappings: mapping[%d] ends at %#x but mapping[%d] starts at %#x (overlap=%d bytes) for buildId %s",
+			return fmt.Errorf("overlap in mappings: mapping[%d] ends at %d but mapping[%d] starts at %d (overlap=%d bytes) for buildId %s",
 				i, currentEnd, i+1, nextStart, currentEnd-nextStart, h.Metadata.BuildId.String())
 		}
 	}
@@ -295,42 +301,42 @@ func ValidateHeader(h *Header) error {
 	lastMapping := sortedMappings[len(sortedMappings)-1]
 	lastEnd := lastMapping.Offset + lastMapping.Length
 	if lastEnd < h.Metadata.Size {
-		return fmt.Errorf("mappings don't cover entire file: last mapping ends at %#x but file size is %#x (missing %d bytes) for buildId %s",
+		return fmt.Errorf("mappings don't cover entire file: last mapping ends at %d but file size is %d (missing %d bytes) for buildId %s",
 			lastEnd, h.Metadata.Size, h.Metadata.Size-lastEnd, h.Metadata.BuildId.String())
 	}
 
 	// Allow last mapping to extend up to one block past size (for alignment)
 	if lastEnd > h.Metadata.Size+h.Metadata.BlockSize {
-		return fmt.Errorf("last mapping extends too far: ends at %#x but file size is %#x (overhang=%d bytes, max allowed=%d) for buildId %s",
+		return fmt.Errorf("last mapping extends too far: ends at %d but file size is %d (overhang=%d bytes, max allowed=%d) for buildId %s",
 			lastEnd, h.Metadata.Size, lastEnd-h.Metadata.Size, h.Metadata.BlockSize, h.Metadata.BuildId.String())
 	}
 
 	// Validate individual mapping bounds
 	for i, m := range h.Mapping {
 		if m.Offset > h.Metadata.Size {
-			return fmt.Errorf("mapping[%d] has Offset %#x beyond header size %#x for buildId %s",
+			return fmt.Errorf("mapping[%d] has Offset %d beyond header size %d for buildId %s",
 				i, m.Offset, h.Metadata.Size, m.BuildId.String())
 		}
 		if m.Length == 0 {
-			return fmt.Errorf("mapping[%d] has zero length at offset %#x for buildId %s",
+			return fmt.Errorf("mapping[%d] has zero length at offset %d for buildId %s",
 				i, m.Offset, m.BuildId.String())
 		}
 	}
 
 	return nil
 }
 
-// AddFrames associates compression frame information with this header's mappings.
+// SetFrames associates compression frame information with this header's mappings.
 //
 // Only mappings matching this header's BuildId will be updated. Returns nil if frameTable is nil.
-func (t *Header) AddFrames(frameTable *storage.FrameTable) error {
+func (t *Header) SetFrames(frameTable *storage.FrameTable) error {
 	if frameTable == nil {
 		return nil
 	}
 
 	for _, mapping := range t.Mapping {
 		if mapping.BuildId == t.Metadata.BuildId {
-			if err := mapping.AddFrames(frameTable); err != nil {
+			if err := mapping.SetFrames(frameTable); err != nil {
 				return err
 			}
 		}

packages/shared/pkg/storage/header/mapping.go

@@ -32,7 +32,7 @@ func (mapping *BuildMap) Copy() *BuildMap {
 	}
 }
 
-// AddFrames associates compression frame information with this mapping.
+// SetFrames associates compression frame information with this mapping.
 //
 // When a file is uploaded with compression, the compressor produces a FrameTable
 // that describes how the compressed data is organized into frames. This method
@@ -41,7 +41,7 @@ func (mapping *BuildMap) Copy() *BuildMap {
 //
 // Returns nil if frameTable is nil. Returns an error if the mapping's range
 // cannot be found in the frame table.
-func (mapping *BuildMap) AddFrames(frameTable *storage.FrameTable) error {
+func (mapping *BuildMap) SetFrames(frameTable *storage.FrameTable) error {
 	if frameTable == nil {
 		return nil
 	}
@@ -53,7 +53,7 @@ func (mapping *BuildMap) AddFrames(frameTable *storage.FrameTable) error {
 
 	subset, err := frameTable.Subset(mappedRange)
 	if err != nil {
-		return fmt.Errorf("mapping at virtual offset %#x (storage offset %#x, length %#x): %w",
+		return fmt.Errorf("mapping at virtual offset %d (storage offset %d, length %d): %w",
 			mapping.Offset, mapping.BuildStorageOffset, mapping.Length, err)
 	}
 
@@ -131,7 +131,6 @@ func MergeMappings(
 
 	mappings := make([]*BuildMap, 0)
 
-	var err error
 	var baseIdx int
 	var diffIdx int
 
@@ -195,9 +194,8 @@ func MergeMappings(
 					// the build storage offset is the same as the base mapping
 					BuildStorageOffset: base.BuildStorageOffset,
 				}
-				leftBase.FrameTable, err = base.FrameTable.Subset(storage.Range{Start: int64(leftBase.BuildStorageOffset), Length: int(leftBase.Length)})
-				if err != nil {
-					return nil, fmt.Errorf("subset frame table for left split at offset %#x: %w", leftBase.Offset, err)
+				if err := leftBase.SetFrames(base.FrameTable); err != nil {
+					return nil, fmt.Errorf("set frames for left split at offset %d: %w", leftBase.Offset, err)
 				}
 
 				mappings = append(mappings, leftBase)
@@ -217,9 +215,8 @@ func MergeMappings(
 					BuildId:            base.BuildId,
 					BuildStorageOffset: base.BuildStorageOffset + uint64(rightBaseShift),
 				}
-				rightBase.FrameTable, err = base.FrameTable.Subset(storage.Range{Start: int64(rightBase.BuildStorageOffset), Length: int(rightBase.Length)})
-				if err != nil {
-					return nil, fmt.Errorf("subset frame table for right split at offset %#x: %w", rightBase.Offset, err)
+				if err := rightBase.SetFrames(base.FrameTable); err != nil {
+					return nil, fmt.Errorf("set frames for right split at offset %d: %w", rightBase.Offset, err)
 				}
 
 				baseMapping[baseIdx] = rightBase
@@ -248,9 +245,8 @@ func MergeMappings(
 					BuildId:            base.BuildId,
 					BuildStorageOffset: base.BuildStorageOffset + uint64(rightBaseShift),
 				}
-				rightBase.FrameTable, err = base.FrameTable.Subset(storage.Range{Start: int64(rightBase.BuildStorageOffset), Length: int(rightBase.Length)})
-				if err != nil {
-					return nil, fmt.Errorf("subset frame table for right split at offset %#x: %w", rightBase.Offset, err)
+				if err := rightBase.SetFrames(base.FrameTable); err != nil {
+					return nil, fmt.Errorf("set frames for right split at offset %d: %w", rightBase.Offset, err)
 				}
 
 				baseMapping[baseIdx] = rightBase
@@ -273,9 +269,8 @@ func MergeMappings(
 					BuildId:            base.BuildId,
 					BuildStorageOffset: base.BuildStorageOffset,
 				}
-				leftBase.FrameTable, err = base.FrameTable.Subset(storage.Range{Start: int64(leftBase.BuildStorageOffset), Length: int(leftBase.Length)})
-				if err != nil {
-					return nil, fmt.Errorf("subset frame table for left split at offset %#x: %w", leftBase.Offset, err)
+				if err := leftBase.SetFrames(base.FrameTable); err != nil {
+					return nil, fmt.Errorf("set frames for left split at offset %d: %w", leftBase.Offset, err)
 				}
 
 				mappings = append(mappings, leftBase)