fix(s3api): cap copy-chunk receive buffer to avoid append-grow blowup (#9420)

* 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).
This commit is contained in:
Chris Lu
2026-05-10 12:08:06 -07:00
committed by GitHub
parent ca12934834
commit 926a8e9351
2 changed files with 113 additions and 1 deletions
+7 -1
View File
@@ -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...)
})
@@ -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)
}