diff --git a/weed/s3api/s3api_object_handlers_delete.go b/weed/s3api/s3api_object_handlers_delete.go index 99664b871..5f42bc9f5 100644 --- a/weed/s3api/s3api_object_handlers_delete.go +++ b/weed/s3api/s3api_object_handlers_delete.go @@ -447,6 +447,10 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h identity, _ = id.(*Identity) } + // The keys below each drive their own bounded filer retries, and the client + // picks how many keys there are, so the whole batch shares one allowance. + r = r.WithContext(withFilerRetryBudget(r.Context(), filerRetryRequestBudget)) + err = s3a.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { // delete file entries for _, object := range deleteObjects.Objects { diff --git a/weed/s3api/s3api_object_versioning.go b/weed/s3api/s3api_object_versioning.go index 5e1028b1d..77840f387 100644 --- a/weed/s3api/s3api_object_versioning.go +++ b/weed/s3api/s3api_object_versioning.go @@ -15,6 +15,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" "github.com/seaweedfs/seaweedfs/weed/filer" @@ -1375,14 +1376,64 @@ func (s3a *S3ApiServer) repointLatestBeforeDeletion(ctx context.Context, bucket, // retryAttempts and retryStep tune the bounded retries used when the // load-bearing filer ops in updateLatestVersionAfterDeletion fail with -// transient errors. Doubled per attempt, capped at retryCap. Total -// worst-case wall time ≈ 6.3s before propagating. +// transient errors. Doubled per attempt, capped at retryCap, for a +// worst case of ~3.1s of backoff per op before propagating. const ( updateLatestRetryAttempts = 6 updateLatestRetryStep = 100 * time.Millisecond updateLatestRetryCap = 2 * time.Second ) +func retryFilerBackoff(attempt int) time.Duration { + backoff := updateLatestRetryStep << (attempt - 1) + if backoff <= 0 || backoff > updateLatestRetryCap { + return updateLatestRetryCap + } + return backoff +} + +// filerRetryRequestBudget is the total backoff one request may spend across +// every retryFilerOp it drives: the worst case of a single op, so a batch +// whose length the client chooses waits about as long as one key would. +var filerRetryRequestBudget = func() (total time.Duration) { + for attempt := 1; attempt < updateLatestRetryAttempts; attempt++ { + total += retryFilerBackoff(attempt) + } + return total +}() + +type filerRetryBudgetKey struct{} + +type filerRetryBudget struct { + mu sync.Mutex + remaining time.Duration +} + +// withFilerRetryBudget hands every retryFilerOp reached through ctx one shared +// allowance, so per-key backoff no longer multiplies by the number of keys. +func withFilerRetryBudget(ctx context.Context, total time.Duration) context.Context { + return context.WithValue(ctx, filerRetryBudgetKey{}, &filerRetryBudget{remaining: total}) +} + +func filerRetryBudgetFrom(ctx context.Context) *filerRetryBudget { + budget, _ := ctx.Value(filerRetryBudgetKey{}).(*filerRetryBudget) + return budget +} + +// take reserves up to d of what is left, reporting false once nothing is. +func (b *filerRetryBudget) take(d time.Duration) (time.Duration, bool) { + b.mu.Lock() + defer b.mu.Unlock() + if b.remaining <= 0 { + return 0, false + } + if d > b.remaining { + d = b.remaining + } + b.remaining -= d + return d, true +} + // isRetryableFilerErr reports whether err is worth retrying through // retryFilerOp. Terminal conditions return false so the caller surfaces // them immediately without the backoff delay or the retry-budget @@ -1412,7 +1463,7 @@ func isRetryableFilerErr(err error) bool { func retryFilerOp(ctx context.Context, name string, fn func() error) error { var lastErr error - backoff := updateLatestRetryStep + budget := filerRetryBudgetFrom(ctx) for attempt := 1; attempt <= updateLatestRetryAttempts; attempt++ { err := fn() if err == nil { @@ -1431,9 +1482,17 @@ func retryFilerOp(ctx context.Context, name string, fn func() error) error { if attempt == updateLatestRetryAttempts { break } + backoff := retryFilerBackoff(attempt) + if budget != nil { + granted, ok := budget.take(backoff) + if !ok { + return fmt.Errorf("%s stopped after %d attempts, request retry allowance spent: %w", name, attempt, lastErr) + } + backoff = granted + } // Context-aware backoff so a server shutdown / client - // disconnect cancels the worst-case ~6.3s retry budget - // immediately instead of blocking the goroutine. + // disconnect cancels the pending retries immediately + // instead of blocking the goroutine. timer := time.NewTimer(backoff) select { case <-ctx.Done(): @@ -1441,10 +1500,6 @@ func retryFilerOp(ctx context.Context, name string, fn func() error) error { return ctx.Err() case <-timer.C: } - backoff *= 2 - if backoff > updateLatestRetryCap { - backoff = updateLatestRetryCap - } } return fmt.Errorf("%s exhausted %d retries: %w", name, updateLatestRetryAttempts, lastErr) } diff --git a/weed/s3api/s3api_versioning_reconciler_test.go b/weed/s3api/s3api_versioning_reconciler_test.go index 34fc6cbd4..83adb69a7 100644 --- a/weed/s3api/s3api_versioning_reconciler_test.go +++ b/weed/s3api/s3api_versioning_reconciler_test.go @@ -168,3 +168,78 @@ func TestRetryFilerOp_TerminalErrorsShortCircuit(t *testing.T) { }) } } + +// TestFilerRetryBudget_ClampsToWhatIsLeft covers the allowance arithmetic: +// a reservation larger than the remainder is trimmed, and once nothing is +// left the caller is told to stop rather than handed a zero wait. +func TestFilerRetryBudget_ClampsToWhatIsLeft(t *testing.T) { + b := &filerRetryBudget{remaining: 150 * time.Millisecond} + + granted, ok := b.take(100 * time.Millisecond) + require.True(t, ok) + assert.Equal(t, 100*time.Millisecond, granted) + + granted, ok = b.take(200 * time.Millisecond) + require.True(t, ok) + assert.Equal(t, 50*time.Millisecond, granted, "trimmed to the remainder") + + _, ok = b.take(time.Millisecond) + assert.False(t, ok, "allowance spent") +} + +// TestFilerRetryRequestBudget_MatchesOneOpWorstCase pins the request-wide +// allowance to what a single retryFilerOp can sleep for, so a batch delete +// adds the wait of one key rather than one per key. +func TestFilerRetryRequestBudget_MatchesOneOpWorstCase(t *testing.T) { + var singleOp time.Duration + for attempt := 1; attempt < updateLatestRetryAttempts; attempt++ { + singleOp += retryFilerBackoff(attempt) + } + assert.Equal(t, singleOp, filerRetryRequestBudget) + assert.Equal(t, 3100*time.Millisecond, filerRetryRequestBudget) +} + +// TestRetryFilerOp_SharedBudgetAcrossBatch is the batch-delete shape: many +// keys, each driving its own retryFilerOp against a filer that always fails +// retryably. Without a shared allowance every key pays the full per-op +// backoff, so the wait scales with a key count the client picks. The +// allowance here is deliberately small so the test never sleeps for the +// production budget. +func TestRetryFilerOp_SharedBudgetAcrossBatch(t *testing.T) { + const keys = 200 + const allowance = 250 * time.Millisecond + + ctx := withFilerRetryBudget(context.Background(), allowance) + calls := 0 + start := time.Now() + for i := 0; i < keys; i++ { + err := retryFilerOp(ctx, "test", func() error { + calls++ + return errors.New("transient") + }) + require.Error(t, err) + } + elapsed := time.Since(start) + + assert.LessOrEqual(t, calls, keys+updateLatestRetryAttempts, "only the keys that fit the allowance retry") + assert.Less(t, elapsed, 2*allowance, "the whole batch stays inside one allowance") +} + +// TestRetryFilerOp_SpentBudgetStopsAfterFirstAttempt confirms a key that +// arrives after the allowance is gone fails immediately, reporting why, and +// still gets its one real attempt at the filer. +func TestRetryFilerOp_SpentBudgetStopsAfterFirstAttempt(t *testing.T) { + ctx := withFilerRetryBudget(context.Background(), 0) + calls := 0 + start := time.Now() + err := retryFilerOp(ctx, "test", func() error { + calls++ + return errors.New("transient") + }) + + require.Error(t, err) + assert.Equal(t, 1, calls, "the op still runs once") + assert.Less(t, time.Since(start), 50*time.Millisecond, "no backoff once the allowance is spent") + assert.Contains(t, err.Error(), "retry allowance spent") + assert.Contains(t, err.Error(), "transient", "underlying error preserved") +}