From 926a8e93515203eda99a4eb46c82eeb534b9c111 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 10 May 2026 12:08:06 -0700 Subject: [PATCH] fix(s3api): cap copy-chunk receive buffer to avoid append-grow blowup (#9420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(s3api): cap copy-chunk receive buffer to avoid append-grow blowup downloadChunkData accumulated the streamed chunk into a nil []byte via `chunkData = append(chunkData, data...)`. ReadUrlAsStream pumps in 256 KiB ticks, so a 64 MiB chunk grew the slice geometrically (256K → 512K → 1M → ... → 64M), allocating ~2x the chunk size for every transferred byte. Combined with the 4-way per-request concurrency and any number of in-flight UploadPartCopy calls (Harbor multipart assemble), this is what produces the runaway-RSS pattern reported in #6541. Pre-size the receive buffer to the known sizeInt so the callback fills in place. Add a regression test that downloads a 16 MiB chunk through httptest and asserts TotalAlloc stays under 1.5x the chunk size — the pre-fix code allocates ~5x and trips the bound. Local repro (weed 4.23, 6 parallel UploadPartCopy on a 512 MiB source): before: baseline 96 MiB → peak 3124 MiB, never reclaimed pprof: 650 MiB inuse in bytes.growSlice + 461 MiB in downloadChunkData.func1 * test(s3api): assert downloaded chunk content matches payload Address PR review feedback: the allocation-bound check alone would still pass if a future regression silently truncated or corrupted the chunk. Compare the returned bytes against the source payload (after the TotalAlloc measurement window so bytes.Equal doesn't pollute it). --- weed/s3api/s3api_object_handlers_copy.go | 8 +- .../s3api_object_handlers_copy_alloc_test.go | 106 ++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 weed/s3api/s3api_object_handlers_copy_alloc_test.go diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 2add4214f..ea728b067 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -1434,7 +1434,13 @@ func (s3a *S3ApiServer) downloadChunkData(srcUrl, fileId string, offset, size in return nil, fmt.Errorf("chunk size %d exceeds maximum int32 size", size) } sizeInt := int(size) - var chunkData []byte + // Pre-size the receive buffer to the known chunk size so the streaming + // callback below does not trigger geometric `append`-grow on a nil slice. + // Receiving a 64 MiB chunk through 256 KiB callback ticks would otherwise + // allocate ~2x the chunk size, and with concurrent UploadPartCopy requests + // (Harbor-style assemble loops) this caused the runaway-RSS pattern in + // https://github.com/seaweedfs/seaweedfs/issues/6541. + chunkData := make([]byte, 0, sizeInt) shouldRetry, err := util_http.ReadUrlAsStream(context.Background(), srcUrl, jwt, nil, false, false, offset, sizeInt, func(data []byte) { chunkData = append(chunkData, data...) }) diff --git a/weed/s3api/s3api_object_handlers_copy_alloc_test.go b/weed/s3api/s3api_object_handlers_copy_alloc_test.go new file mode 100644 index 000000000..c6d761248 --- /dev/null +++ b/weed/s3api/s3api_object_handlers_copy_alloc_test.go @@ -0,0 +1,106 @@ +package s3api + +import ( + "bytes" + "net/http" + "net/http/httptest" + "runtime" + "strconv" + "testing" + + util_http "github.com/seaweedfs/seaweedfs/weed/util/http" +) + +// TestDownloadChunkData_AllocationBound is a regression guard for +// https://github.com/seaweedfs/seaweedfs/issues/6541. +// +// downloadChunkData previously accumulated streamed bytes via +// +// var chunkData []byte +// ... fn := func(data []byte) { chunkData = append(chunkData, data...) } +// +// The callback fires once per ReadUrlAsStream pump (256 KiB), so a 16 MiB +// chunk grew the slice geometrically (256K -> 512K -> 1M -> ... -> 32M), +// allocating ~2x the chunk size for every transferred byte. Combined with +// the 4-way per-request concurrency in copyChunks/copyChunksForRange and +// any number of in-flight UploadPartCopy calls (Harbor multipart assemble), +// this produced the runaway RSS reported in the issue. +// +// We assert that downloading a chunkSize-byte chunk allocates at most +// 1.5 * chunkSize bytes on the heap. The pre-fix nil-append pattern blows +// past that bound; the fixed (`make([]byte, 0, sizeInt)`) version stays +// well under it. +func TestDownloadChunkData_AllocationBound(t *testing.T) { + // downloadChunkData drives the package-level global HTTP client. Tests + // don't go through the normal `weed` startup that does this, so call it + // here. Idempotent on subsequent test runs. + util_http.InitGlobalHttpClient() + + const chunkSize = 16 << 20 // 16 MiB + payload := make([]byte, chunkSize) + for i := range payload { + payload[i] = byte(i) + } + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", strconv.Itoa(len(payload))) + // Write in 64 KiB increments so the body is delivered to the client + // reader in many small reads, mirroring real volume-server streaming + // behavior. The exact step does not matter as long as the body comes + // through the client in multiple Read() calls. + const step = 64 * 1024 + for i := 0; i < len(payload); i += step { + end := i + step + if end > len(payload) { + end = len(payload) + } + if _, err := w.Write(payload[i:end]); err != nil { + return + } + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + } + })) + defer srv.Close() + + s3a := &S3ApiServer{} + + // Warm up the global HTTP client and any package-level pools so they + // don't show up in the measured allocation window. + if _, err := s3a.downloadChunkData(srv.URL, "1,0", 0, int64(chunkSize), nil); err != nil { + t.Fatalf("warm-up downloadChunkData: %v", err) + } + + runtime.GC() + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + + data, err := s3a.downloadChunkData(srv.URL, "1,0", 0, int64(chunkSize), nil) + if err != nil { + t.Fatalf("downloadChunkData: %v", err) + } + + runtime.ReadMemStats(&after) + allocated := after.TotalAlloc - before.TotalAlloc + + // Correctness check happens after the alloc measurement so bytes.Equal + // doesn't pollute the window. A pre-sized buffer that silently truncates + // or corrupts the chunk would still pass the bound — assert content too. + if !bytes.Equal(data, payload) { + t.Fatalf("downloaded data does not match payload (len got=%d want=%d)", + len(data), len(payload)) + } + + // The output slice itself is chunkSize bytes; the streaming pump uses a + // pooled 256 KiB read buffer and doesn't otherwise accumulate. A 1.5x + // bound is comfortably above the steady-state cost and well below the + // pre-fix geometric-grow cost (~2x). + maxAllowed := uint64(chunkSize) * 3 / 2 + if allocated > maxAllowed { + t.Fatalf("downloadChunkData allocated %d bytes for a %d-byte chunk; bound is %d "+ + "(regression: unbounded append in callback?)", + allocated, chunkSize, maxAllowed) + } + t.Logf("allocated=%d bytes, bound=%d, output=%d", allocated, maxAllowed, chunkSize) +}