diff --git a/b2/b2_test.go b/b2/b2_test.go index 32e33a3..2e5a3bd 100644 --- a/b2/b2_test.go +++ b/b2/b2_test.go @@ -751,6 +751,92 @@ func TestReuploadFile(t *testing.T) { } } +// TestFailedUploadDoesNotPoolURL verifies that when simpleWriteFile hard-fails +// (retries exhausted), the upload URL from the last, failed attempt is not +// returned to the bucket's urlPool. Since urlPool has no health check, pooling +// it would let an unrelated, subsequent Writer on the same bucket pick up a +// potentially broken upload URL/connection. +func TestFailedUploadDoesNotPoolURL(t *testing.T) { + ctx := context.Background() + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + ch := make(chan time.Time) + close(ch) + after = func(d time.Duration) <-chan time.Time { + return ch + } + + root := &testRoot{ + bucketMap: make(map[string]map[string]string), + errs: &errCont{ + errMap: map[string]map[int]error{ + "uploadFile": { + 0: testError{reupload: true, maxReuploads: 0}, + }, + }, + }, + } + client := &Client{ + backend: &beRoot{ + b2i: root, + }, + } + b, err := client.NewBucket(ctx, "fun", &BucketAttrs{Type: Private}) + if err != nil { + t.Fatal(err) + } + o := b.Object("foo") + w := o.NewWriter(ctx) + r := io.LimitReader(zReader{}, 1e4) + if _, err := io.Copy(w, r); err != nil { + t.Fatal(err) + } + if err := w.Close(); err == nil { + t.Fatalf("writer should have returned an error") + } + + if u := b.urlPool.get(); u != nil { + t.Fatalf("upload URL from a failed attempt was pooled; want nil") + } +} + +// TestSuccessfulUploadPoolsURL confirms the happy path still pools the upload +// URL after simpleWriteFile succeeds, i.e. that the fix in +// TestFailedUploadDoesNotPoolURL didn't break normal URL reuse. +func TestSuccessfulUploadPoolsURL(t *testing.T) { + ctx := context.Background() + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + root := &testRoot{ + bucketMap: make(map[string]map[string]string), + errs: &errCont{}, + } + client := &Client{ + backend: &beRoot{ + b2i: root, + }, + } + b, err := client.NewBucket(ctx, "fun", &BucketAttrs{Type: Private}) + if err != nil { + t.Fatal(err) + } + o := b.Object("foo") + w := o.NewWriter(ctx) + r := io.LimitReader(zReader{}, 1e4) + if _, err := io.Copy(w, r); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatalf("writer should not have returned an error: %v", err) + } + + if u := b.urlPool.get(); u == nil { + t.Fatalf("upload URL from a successful attempt was not pooled; want non-nil") + } +} + func TestReuploadFileWithoutReuploadAfter(t *testing.T) { ctx := context.Background() ctx, cancel := context.WithTimeout(ctx, 10*time.Second) diff --git a/b2/writer.go b/b2/writer.go index cf15313..519f64a 100644 --- a/b2/writer.go +++ b/b2/writer.go @@ -303,8 +303,16 @@ func (w *Writer) simpleWriteFile() error { return err } // This defer needs to be in a func() so that we put whatever the value of ue - // is at function exit. - defer func() { w.o.b.urlPool.put(ue) }() + // is at function exit. Only pool it on success: ue is reassigned to a fresh + // URL on every retry, so on failure it holds the URL from the last failed + // attempt, and pooling that could hand a broken upload URL to the next, + // unrelated Writer on this bucket. + success := false + defer func() { + if success { + w.o.b.urlPool.put(ue) + } + }() sha1 := w.w.Hash() ctype := w.contentType if ctype == "" { @@ -355,6 +363,7 @@ func (w *Writer) simpleWriteFile() error { return err } + success = true return nil } diff --git a/base/base.go b/base/base.go index bfef4ae..2e7038f 100644 --- a/base/base.go +++ b/base/base.go @@ -210,6 +210,12 @@ func Backoff(err error) time.Duration { if !ok { return 0 } + // Cap at the same 30s ceiling used by internal/retry.Backoff, so a + // large or malformed server-supplied Retry-After value can't produce + // an unbounded single sleep in the retry loop. + if e.retry > 30 { + return 30 * time.Second + } return time.Duration(e.retry) * time.Second } diff --git a/base/base_test.go b/base/base_test.go new file mode 100644 index 0000000..69b9bd9 --- /dev/null +++ b/base/base_test.go @@ -0,0 +1,51 @@ +// Copyright 2026, the Blazer authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package base + +import ( + "testing" + "time" +) + +func TestBackoff(t *testing.T) { + table := []struct { + name string + retry int + want time.Duration + }{ + { + name: "small retry value passes through unscaled", + retry: 5, + want: 5 * time.Second, + }, + { + name: "retry value at the cap passes through unscaled", + retry: 30, + want: 30 * time.Second, + }, + { + name: "large retry value is clamped to the cap", + retry: 5000, + want: 30 * time.Second, + }, + } + + for _, e := range table { + got := Backoff(b2err{retry: e.retry}) + if got != e.want { + t.Errorf("%s: Backoff(b2err{retry: %d}): got %v, want %v", e.name, e.retry, got, e.want) + } + } +}