From f70f338e7f5a55ac02fe7fa6665ff1962ff785c9 Mon Sep 17 00:00:00 2001
From: Chris Lu <chris.lu@gmail.com>
Date: Sun, 10 May 2026 13:30:22 -0700
Subject: [PATCH] review: surface shouldRetry, add int32 guard, drop redundant
 drains
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Address review on PR 9424:

* coderabbit (HIGH, line 122): ReadUrlAsStream can set shouldRetry=true
  with readErr=nil. Before this fix, that fell through to mw.Close()
  and the destination POST succeeded against a possibly-truncated
  multipart body. Mirror downloadChunkData's explicit check and
  surface shouldRetry as a producer error so the dst POST aborts.
* gemini (line 98): chunk size is int64 but ReadUrlAsStream takes int.
  Reject sizes above MaxInt32 up front so the int(size) cast can't
  truncate negative on 32-bit platforms — same guard downloadChunkData
  uses.
* gemini (line 151): util_http.CloseResponse already drains the body
  (io.Copy(io.Discard, ...) inside the helper) before closing, so the
  manual io.Copy drains we added are redundant. Drop them.
---
 .../s3api_object_handlers_copy_stream.go      | 22 +++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/weed/s3api/s3api_object_handlers_copy_stream.go b/weed/s3api/s3api_object_handlers_copy_stream.go
index ef3a9cba6..76cec9da1 100644
--- a/weed/s3api/s3api_object_handlers_copy_stream.go
+++ b/weed/s3api/s3api_object_handlers_copy_stream.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"math"
 	"mime"
 	"mime/multipart"
 	"net/http"
@@ -54,6 +55,11 @@ func (s3a *S3ApiServer) streamCopyChunkRange(
 	assignResult *filer_pb.AssignVolumeResponse,
 	isCompressed bool,
 ) error {
+	// ReadUrlAsStream takes int for size; reject anything that would
+	// truncate negative on 32-bit. Mirrors the guard in downloadChunkData.
+	if size > int64(math.MaxInt32) {
+		return fmt.Errorf("chunk size %d exceeds maximum int32 size", size)
+	}
 	dstUrl := fmt.Sprintf("http://%s/%s", assignResult.Location.Url, assignResult.FileId)
 	dstJwt := security.EncodedJwt(assignResult.Auth)
 	srcJwt := filer.JwtForVolumeServer(srcFileId)
@@ -115,6 +121,16 @@ func (s3a *S3ApiServer) streamCopyChunkRange(
 			producerErr = fmt.Errorf("stream read: %w", readErr)
 			return
 		}
+		// shouldRetry can be set without an error (e.g. ReadUrlAsStream
+		// surfacing a partial-read condition that the buffered path
+		// re-fetches). Treat it as a failed copy here too — otherwise we
+		// would close the multipart cleanly and let the destination POST
+		// succeed against a possibly-truncated body. Mirrors the explicit
+		// check downloadChunkData makes after ReadUrlAsStream returns.
+		if shouldRetry {
+			producerErr = fmt.Errorf("stream read %s offset=%d size=%d: retry needed", srcUrl, offset, size)
+			return
+		}
 
 		if err := mw.Close(); err != nil {
 			producerErr = fmt.Errorf("multipart close: %w", err)
@@ -140,14 +156,12 @@ func (s3a *S3ApiServer) streamCopyChunkRange(
 		pipeReader.CloseWithError(err)
 		return fmt.Errorf("POST: %w", err)
 	}
+	// CloseResponse drains and closes resp.Body for us; no manual io.Copy
+	// drain needed for keepalive.
 	defer util_http.CloseResponse(resp)
 
 	if resp.StatusCode >= 400 {
-		// Drain the body so the connection can be reused.
-		_, _ = io.Copy(io.Discard, resp.Body)
 		return fmt.Errorf("POST %s: %s", dstUrl, resp.Status)
 	}
-	// Drain the body even on success — http.Client's keepalive depends on it.
-	_, _ = io.Copy(io.Discard, resp.Body)
 	return nil
 }