From c917110124979282e64462f7ce6c0fa20eccd111 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 10 May 2026 12:08:24 -0700 Subject: [PATCH] fix(volume): pre-size ParseUpload buffer to request ContentLength (#9421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(volume): pre-size ParseUpload buffer to request ContentLength The volume server's PostHandler reads the multipart upload body via bytes.Buffer.ReadFrom inside parseUpload. The buffer comes from a sync.Pool and may have cap=0 when the pool dropped the prior entry, which makes ReadFrom geometric-grow on each chunk: a 64 MiB upload allocates roughly 1+2+4+...+64 ≈ 128 MiB just to receive the body. Under concurrent uploads (every s3 chunk-copy lands here on the destination volume) this is one of the main contributors to the runaway-RSS pattern in #6541 — pprof shows ~458 MiB cum in parseUpload's bytes.Buffer.ReadFrom under Harbor-style assemble load. Grow the buffer once up front, bounded by the existing sizeLimit so a misreported Content-Length can't over-allocate. The receive then fills in place. Add a regression test that drives ParseUpload with a 16 MiB multipart body and bounds TotalAlloc at 1.5x the chunk size (pre-fix measures ~4x, so the bound trips deterministically). * fix(volume): guard ParseUpload pre-grow against int overflow on 32-bit Address PR review feedback: r.ContentLength is int64, and on 32-bit platforms int is 32 bits wide, so int(r.ContentLength) for a value above math.MaxInt32 wraps negative and bytes.Buffer.Grow panics with "bytes.Buffer.Grow: negative count". Skip the pre-grow optimization in that range; the existing geometric-grow path remains correct, just slightly more allocator pressure for that one call. 64-bit platforms (math.MaxInt == math.MaxInt64) are unaffected — the guard only kicks in for 32-bit builds with very large sizeLimit. * fix(volume): cap ParseUpload pre-grow at 4 MiB to bound DoS surface Address PR review: pre-growing the receive buffer to r.ContentLength trusts the header before any body bytes arrive. A bad header or slow / idle client could declare a large Content-Length up to sizeLimit (256 MiB by default for volume writes) and force per-request preallocation without sending data, turning many concurrent slow connections into avoidable memory pressure. Cap eager pre-grow at maxEagerPreGrow (4 MiB). Larger uploads still benefit from the higher starting cap and fall back to ReadFrom's grow path for the remainder. Per-request waste from a misreported Content-Length is now bounded at 4 MiB regardless of sizeLimit. Extract the policy as eagerPreGrow so the unit test can exercise the gates structurally — replaces the prior TotalAlloc bound (which became uninformative once savings were capped at 4 MiB). --- weed/storage/needle/needle_parse_upload.go | 26 +++++ .../needle/needle_parse_upload_alloc_test.go | 104 ++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100644 weed/storage/needle/needle_parse_upload_alloc_test.go diff --git a/weed/storage/needle/needle_parse_upload.go b/weed/storage/needle/needle_parse_upload.go index f7f62df11..aa54c3462 100644 --- a/weed/storage/needle/needle_parse_upload.go +++ b/weed/storage/needle/needle_parse_upload.go @@ -17,6 +17,31 @@ import ( "github.com/seaweedfs/seaweedfs/weed/util" ) +// maxEagerPreGrow caps how many bytes ParseUpload is willing to pre-allocate +// from a request's announced Content-Length. Large enough to skip a few +// rounds of bytes.Buffer.ReadFrom geometric grow on typical small uploads; +// small enough that a misreported Content-Length or a slow/idle client can +// only ever waste this much memory per request. Bigger uploads fall back to +// the standard ReadFrom-grow path for the remainder. +const maxEagerPreGrow = 4 * 1024 * 1024 + +// eagerPreGrow ensures bytesBuffer has at least min(contentLength, sizeLimit, +// maxEagerPreGrow) bytes of capacity, so the bytes.Buffer.ReadFrom pumps +// inside parseUpload below skip the first round(s) of geometric grow on the +// common upload sizes — see #6541. The cap policy is the load-bearing piece +// here; it's extracted so unit tests can exercise the policy directly +// without spinning a real http upload. +func eagerPreGrow(bytesBuffer *bytes.Buffer, contentLength, sizeLimit int64) { + if contentLength <= 0 || contentLength > sizeLimit { + return + } + grow := contentLength + if grow > maxEagerPreGrow { + grow = maxEagerPreGrow + } + bytesBuffer.Grow(int(grow)) +} + type ParsedUpload struct { FileName string Data []byte @@ -35,6 +60,7 @@ type ParsedUpload struct { func ParseUpload(r *http.Request, sizeLimit int64, bytesBuffer *bytes.Buffer) (pu *ParsedUpload, e error) { bytesBuffer.Reset() + eagerPreGrow(bytesBuffer, r.ContentLength, sizeLimit) pu = &ParsedUpload{bytesBuffer: bytesBuffer} pu.PairMap = make(map[string]string) for k, v := range r.Header { diff --git a/weed/storage/needle/needle_parse_upload_alloc_test.go b/weed/storage/needle/needle_parse_upload_alloc_test.go new file mode 100644 index 000000000..3b65c289a --- /dev/null +++ b/weed/storage/needle/needle_parse_upload_alloc_test.go @@ -0,0 +1,104 @@ +package needle + +import ( + "bytes" + "testing" +) + +// TestEagerPreGrow is a regression guard for the volume-side amplifier in +// https://github.com/seaweedfs/seaweedfs/issues/6541. +// +// ParseUpload reads the multipart part body via bytes.Buffer.ReadFrom; if the +// receive buffer arrives with cap=0 (sync.Pool dropped the prior buffer), +// ReadFrom doubles capacity on each grow and pays 2-4x the chunk size in +// cumulative allocations. eagerPreGrow shaves the first few rounds off that +// grow chain by pre-sizing from r.ContentLength. +// +// Two policy invariants the cases below pin: +// +// 1. The cap. We never grow more than maxEagerPreGrow per request, so a +// misreported Content-Length or a slow/idle client cannot force +// per-request preallocation up to the configured sizeLimit. This is the +// review concern that landed during PR review and is what changed the +// test from a TotalAlloc bound to this structural form. +// 2. The validity gates. Negative / zero / oversized Content-Length and +// buffers that already have enough capacity should not trigger a Grow. +func TestEagerPreGrow(t *testing.T) { + const sizeLimit = int64(256 * 1024 * 1024) + + cases := []struct { + name string + startCap int + cl int64 + wantMinCap int + wantNoop bool // post: cap stays at startCap + }{ + { + name: "small content-length grows exactly", + cl: 1 * 1024 * 1024, + wantMinCap: 1 * 1024 * 1024, + }, + { + name: "content-length at cap grows to cap", + cl: maxEagerPreGrow, + wantMinCap: maxEagerPreGrow, + }, + { + name: "content-length above cap is clamped to cap", + cl: 200 * 1024 * 1024, + wantMinCap: maxEagerPreGrow, + }, + { + name: "content-length above sizeLimit is rejected", + cl: sizeLimit + 1, + wantNoop: true, + }, + { + name: "zero content-length is a no-op", + cl: 0, + wantNoop: true, + }, + { + name: "negative content-length (chunked encoding) is a no-op", + cl: -1, + wantNoop: true, + }, + { + name: "buffer with sufficient cap is left alone", + startCap: maxEagerPreGrow, + cl: 1 * 1024 * 1024, + wantMinCap: maxEagerPreGrow, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + bb := &bytes.Buffer{} + if c.startCap > 0 { + bb.Grow(c.startCap) + } + startCap := bb.Cap() + + eagerPreGrow(bb, c.cl, sizeLimit) + + if c.wantNoop { + if got := bb.Cap(); got != startCap { + t.Fatalf("eagerPreGrow(cl=%d, sizeLimit=%d) should not have grown: "+ + "cap=%d, want %d (regression: validity gate removed?)", + c.cl, sizeLimit, got, startCap) + } + return + } + if got := bb.Cap(); got < c.wantMinCap { + t.Fatalf("after eagerPreGrow(cl=%d): cap=%d, want >=%d", c.cl, got, c.wantMinCap) + } + // Hard upper bound: never above the cap (regardless of cl/sizeLimit). + // Note bytes.Buffer.Grow may round capacity up modestly, so allow + // a generous overshoot but still well below sizeLimit. + if got := int64(bb.Cap()); got > 2*int64(maxEagerPreGrow) { + t.Fatalf("after eagerPreGrow(cl=%d): cap=%d exceeds 2*maxEagerPreGrow=%d "+ + "(regression: cap removed?)", c.cl, got, 2*maxEagerPreGrow) + } + }) + } +}