diff --git a/backend/shade/upload.go b/backend/shade/upload.go index e86cecc02..66660e836 100644 --- a/backend/shade/upload.go +++ b/backend/shade/upload.go @@ -3,7 +3,6 @@ package shade import ( - "bytes" "context" "fmt" "io" @@ -155,17 +154,15 @@ func (s *shadeChunkWriter) WriteChunk(ctx context.Context, chunkNumber int, read return 0, err } - // Read chunk - var chunk bytes.Buffer - n, err := io.Copy(&chunk, reader) + // Find the chunk size + n, err := reader.Seek(0, io.SeekEnd) + if err != nil { + return 0, fmt.Errorf("failed to read chunk: %w", err) + } if n == 0 { return 0, nil } - - if err != nil { - return 0, fmt.Errorf("failed to read chunk: %w", err) - } // Get presigned URL for this part var partURL api.PartURL @@ -201,14 +198,18 @@ func (s *shadeChunkWriter) WriteChunk(ctx context.Context, chunkNumber int, read } err = s.f.pacer.Call(func() (bool, error) { - // Use a fresh reader for each attempt so retries resend the whole chunk - opts.Body = bytes.NewReader(chunk.Bytes()) + // Rewind the chunk so each attempt sends it in full + _, err = reader.Seek(0, io.SeekStart) + if err != nil { + return false, err + } + opts.Body = reader uploadRes, err = s.f.srv.Call(ctx, &opts) return shouldRetry(ctx, uploadRes, err) }) if err != nil { - return 0, fmt.Errorf("failed to upload part %d: %w", chunk, err) + return 0, fmt.Errorf("failed to upload part %d: %w", chunkNumber+1, err) } if uploadRes.StatusCode != http.StatusOK && uploadRes.StatusCode != http.StatusCreated { diff --git a/backend/shade/upload_internal_test.go b/backend/shade/upload_internal_test.go new file mode 100644 index 000000000..899e8ac9c --- /dev/null +++ b/backend/shade/upload_internal_test.go @@ -0,0 +1,65 @@ +package shade + +import ( + "bytes" + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/rclone/rclone/fs" + "github.com/rclone/rclone/lib/pacer" + "github.com/rclone/rclone/lib/rest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTestChunkWriter returns a chunk writer pointed at server, which must +// answer the part URL request with a URL back to itself and the part PUT. +func newTestChunkWriter(t *testing.T, server *httptest.Server) *shadeChunkWriter { + ctx := context.Background() + f := &Fs{ + srv: rest.NewClient(server.Client()), + endpoint: server.URL, + drive: "drive", + pacer: fs.NewPacer(ctx, pacer.NewDefault(pacer.MinSleep(time.Millisecond), pacer.MaxSleep(time.Millisecond))), + token: "token", + tokenExp: time.Now().Add(time.Hour), + } + return &shadeChunkWriter{initToken: "init", f: f} +} + +// partServer returns a test server which hands out a part URL pointing at +// itself and passes part PUTs to put. +func partServer(t *testing.T, put func(w http.ResponseWriter, body []byte)) *httptest.Server { + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/upload/multipart/part/") { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `{"url":%q}`, server.URL+"/put") + return + } + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + put(w, body) + })) + return server +} + +// TestWriteChunkError checks that a part which keeps failing reports the +// part number in the error. +func TestWriteChunkError(t *testing.T) { + server := partServer(t, func(w http.ResponseWriter, body []byte) { + w.WriteHeader(http.StatusInternalServerError) + }) + defer server.Close() + + s := newTestChunkWriter(t, server) + _, err := s.WriteChunk(context.Background(), 2, bytes.NewReader([]byte("shade"))) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to upload part 3:") +}