From e37900e36e84e870b4bbcff853d2047003c8c8ce Mon Sep 17 00:00:00 2001 From: "Jared Scott (agent)" Date: Wed, 29 Jul 2026 19:06:45 +0000 Subject: [PATCH 1/2] b2: don't pool upload URL after a hard upload failure Writer.simpleWriteFile unconditionally returned its upload URL/token to the bucket's urlPool on exit via a bare defer, even when retry.Do had exhausted retries and returned a hard error. Since urlPool.get()/put() do no liveness check on stored entries, an unrelated subsequent Writer on the same bucket could pull that broken upload URL back out of the pool and immediately fail the same way (e.g. a small lock-file write right after a large upload's non-retryable failure on the same bucket). Guard the pooling with a boolean success flag set only right before simpleWriteFile's final nil return, so a failed upload's last-used URL (reassigned to a fresh one on each retry.OnRetry) is discarded instead of pooled. Adds TestFailedUploadDoesNotPoolURL, which forces a hard failure via the existing testError{reupload:true, maxReuploads:0} fake and asserts urlPool.get() returns nil afterward, and TestSuccessfulUploadPoolsURL, which confirms the happy path still pools the URL as before. Co-Authored-By: Claude Sonnet 5 --- b2/b2_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++ b2/writer.go | 13 ++++++-- 2 files changed, 97 insertions(+), 2 deletions(-) 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 } From 0e34d623930811ae85a4ae4d8b20a0dd5e9a0740 Mon Sep 17 00:00:00 2001 From: "Jared Scott (agent)" Date: Wed, 29 Jul 2026 19:11:18 +0000 Subject: [PATCH 2/2] Cap base.Backoff() at 30s to bound server-supplied Retry-After delays PR #24 (internal/retry package) bounded the retry loop's iteration count, but base.Backoff() still converts the B2 server's Retry-After header straight into a time.Duration with no upper limit. A large or malformed Retry-After value (from B2 or a misbehaving proxy) could still cause a single very long sleep, undermining that bounded-retry-count fix. Clamp the result to 30 seconds, matching the ceiling internal/retry.Backoff already uses for its own exponential-backoff fallback. Co-Authored-By: Claude Sonnet 5 --- base/base.go | 6 ++++++ base/base_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 base/base_test.go 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) + } + } +}