s3api: stop retrying a definitive NotFound in getLatestObjectVersion (#11067)

The .versions lookup retried every error through the full backoff
ladder, so a missing key spent 12.7s (8 attempts, 100ms..6.4s) before
the pre-versioning fallback could answer. NotFound is an answer, not a
transient failure: gate the retries on isRetryableFilerErr, the same
classifier retryFilerOp already uses, which also stops retrying for
callers whose context is canceled or past its deadline.

GetObject already treats NotFound on .versions/ as definitive; this
brings the retention/tagging/ACL/attributes/delete/copy paths that go
through getLatestObjectVersion in line with it.

Claude-Session: https://claude.ai/code/session_01FquvGtTD2zA3uMZGQHAuV4
This commit is contained in:
Chris Lu
2026-09-01 10:11:09 -07:00
committed by GitHub
parent 2ef0e60aeb
commit 4c9cbf72bc
2 changed files with 86 additions and 14 deletions
+24 -14
View File
@@ -1824,6 +1824,27 @@ func (s3a *S3ApiServer) getLatestObjectVersion(bucket, object string) (*filer_pb
return s3a.doGetLatestObjectVersion(bucket, object, 8)
}
// lookupVersionsEntryWithRetry retries a .versions directory lookup, but only
// for errors that can change on retry (isRetryableFilerErr). A definitive
// NotFound is an answer, not a transient failure: retrying it walks the whole
// backoff ladder (12.7s at 8 attempts) before the caller's pre-versioning
// fallback gets to run, stalling every missing-key retention/tagging/ACL call.
func lookupVersionsEntryWithRetry(lookup func() (*filer_pb.Entry, error), maxRetries int) (entry *filer_pb.Entry, err error) {
for attempt := 1; attempt <= maxRetries; attempt++ {
entry, err = lookup()
if err == nil || !isRetryableFilerErr(err) {
return entry, err
}
if attempt < maxRetries {
// Exponential backoff with higher base: 100ms, 200ms, 400ms, 800ms, 1600ms, 3200ms, 6400ms
delay := time.Millisecond * time.Duration(100*(1<<(attempt-1)))
time.Sleep(delay)
}
}
return entry, err
}
func (s3a *S3ApiServer) doGetLatestObjectVersion(bucket, object string, maxRetries int) (*filer_pb.Entry, error) {
// Normalize object path to ensure consistency with toFilerPath behavior
normalizedObject := s3_constants.NormalizeObjectKey(object)
@@ -1834,20 +1855,9 @@ func (s3a *S3ApiServer) doGetLatestObjectVersion(bucket, object string, maxRetri
glog.V(1).Infof("doGetLatestObjectVersion: looking for latest version of %s/%s (normalized: %s, retries: %d)", bucket, object, normalizedObject, maxRetries)
// Get the .versions directory entry to read latest version metadata with retry logic for filer consistency
var versionsEntry *filer_pb.Entry
var err error
for attempt := 1; attempt <= maxRetries; attempt++ {
versionsEntry, err = s3a.getEntry(bucketDir, versionsObjectPath)
if err == nil {
break
}
if attempt < maxRetries {
// Exponential backoff with higher base: 100ms, 200ms, 400ms, 800ms, 1600ms, 3200ms, 6400ms
delay := time.Millisecond * time.Duration(100*(1<<(attempt-1)))
time.Sleep(delay)
}
}
versionsEntry, err := lookupVersionsEntryWithRetry(func() (*filer_pb.Entry, error) {
return s3a.getEntry(bucketDir, versionsObjectPath)
}, maxRetries)
if err != nil {
// .versions directory doesn't exist - this can happen for objects that existed
@@ -0,0 +1,62 @@
package s3api
import (
"context"
"fmt"
"testing"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
"github.com/stretchr/testify/assert"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
// A definitive NotFound (or an aborted request) is an answer, not a transient
// failure. Walking the full backoff ladder before the pre-versioning fallback
// turned every missing-key GetObjectRetention into a 12.7s stall in the field.
func TestLookupVersionsEntryTerminalErrorsSkipTheLadder(t *testing.T) {
for _, terminal := range []error{
filer_pb.ErrNotFound,
fmt.Errorf("wrapped: %w", filer_pb.ErrNotFound),
status.Error(codes.NotFound, "gone"),
context.Canceled,
status.Error(codes.DeadlineExceeded, "context deadline exceeded"),
} {
calls := 0
entry, err := lookupVersionsEntryWithRetry(func() (*filer_pb.Entry, error) {
calls++
return nil, terminal
}, 8)
assert.Nil(t, entry)
assert.Equal(t, terminal, err)
assert.Equal(t, 1, calls, "%v is definitive, no retry", terminal)
}
}
// Transient filer errors keep the original retry-with-backoff behavior.
func TestLookupVersionsEntryTransientErrorsStillRetry(t *testing.T) {
want := &filer_pb.Entry{Name: "obj" + ".versions"}
calls := 0
entry, err := lookupVersionsEntryWithRetry(func() (*filer_pb.Entry, error) {
calls++
if calls < 3 {
return nil, status.Error(codes.Unavailable, "transport is closing")
}
return want, nil
}, 8)
assert.NoError(t, err)
assert.Same(t, want, entry)
assert.Equal(t, 3, calls)
}
// Exhausting the attempts surfaces the last transient error to the caller.
func TestLookupVersionsEntryTransientExhaustionReturnsLastErr(t *testing.T) {
calls := 0
entry, err := lookupVersionsEntryWithRetry(func() (*filer_pb.Entry, error) {
calls++
return nil, status.Error(codes.Unavailable, "still down")
}, 2)
assert.Nil(t, entry)
assert.Error(t, err)
assert.Equal(t, 2, calls)
}