From 87c1c6b3dc189799912f748572364e319676a7fc Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sat, 17 Oct 2026 11:18:08 +0800 Subject: [PATCH] fix(listing): confirm current groups and bound recovery lock waits Distinguish all-drive native absence from read uncertainty and confirm current pool targets before omitting a recovered name. Cover both existing single-pool writer lock domains, release before encoding or output, and preserve cache terminal errors without blocking the HTTP error response on remote publication. Keep ordinary entries free of recovery I/O and locks. Bound the selective short lock-conflict wait, preserve existing retry intervals and quorum, and make retries cancelable with late-grant cleanup coverage. Restore the original raw listing callback and unchanged scanner/healing/migration callers now that recovery errors originate in the merger. Add deterministic deletion, pool ordering, cache, lock-routing and cancellation regressions to race CI. Focused cmd/dsync/lsync race suites and verifiers pass; complete current-main integration and bare-binary performance validation are recorded in PR #239. Refs: https://github.com/pgsty/silo/issues/218 Signed-off-by: Feng Ruohang --- .github/workflows/go.yml | 5 +- cmd/data-scanner.go | 9 +- cmd/erasure-healing.go | 6 +- cmd/erasure-object.go | 51 +++ cmd/erasure-server-pool-decom.go | 3 +- cmd/erasure-server-pool-rebalance.go | 3 +- cmd/erasure-server-pool.go | 14 +- cmd/global-heal.go | 3 +- cmd/globals.go | 2 + cmd/local-locker-listing-reverse_test.go | 412 ++++++++++++++++++++ cmd/local-locker-listing_test.go | 432 +++++++++++++++++++++ cmd/local-locker.go | 93 ++++- cmd/metacache-entries.go | 111 +++++- cmd/metacache-listing-cache_test.go | 77 ++++ cmd/metacache-listing-delete_test.go | 2 +- cmd/metacache-listing-lock-routes_test.go | 59 ++- cmd/metacache-listing-locks_test.go | 2 +- cmd/metacache-listing-recovery_test.go | 391 +++++++++++++++++++ cmd/metacache-null-quorum_test.go | 56 ++- cmd/metacache-parts-retry_test.go | 444 ++++++++++++++++++++++ cmd/metacache-server-pool.go | 200 +++++++++- cmd/metacache-set.go | 233 +++++------- cmd/metacache-stream.go | 6 + internal/dsync/drwmutex-retry_test.go | 218 +++++++++++ internal/dsync/drwmutex.go | 12 +- internal/lsync/lrwmutex-retry_test.go | 72 ++++ internal/lsync/lrwmutex.go | 8 +- 27 files changed, 2716 insertions(+), 208 deletions(-) create mode 100644 cmd/local-locker-listing-reverse_test.go create mode 100644 cmd/local-locker-listing_test.go create mode 100644 cmd/metacache-listing-recovery_test.go create mode 100644 cmd/metacache-parts-retry_test.go create mode 100644 internal/dsync/drwmutex-retry_test.go create mode 100644 internal/lsync/lrwmutex-retry_test.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 813bc3995..47e889986 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -97,7 +97,10 @@ jobs: run: go test -race ./cmd -run '^Test(MultipartListing|MultipartAbort|PaginateMultipartUploads|ListMultipartUploads)' -count=1 -timeout=5m - name: Run listing metadata recovery, pagination and cancellation tests under race detector - run: go test -race ./cmd -run '^Test(ListFallback|ListingCache|ListingMerge|MergeXLV2LatestNullCrossParity|CountXLV2MixedECGroups|ListCrossParity)' -count=1 -timeout=5m + run: go test -race ./cmd -run '^Test(ListFallback|ListingCache|ListingMerge|ListingRecovery|ListingPending|ListingEntry|ListingSecondLock|StreamMetadataParts|LocalLockerListing|DistributedListing|MergeXLV2LatestNullCrossParity|CountXLV2MixedECGroups|ListCrossParity)' -count=1 -timeout=5m + + - name: Run lock cancellation and late-grant cleanup tests under race detector + run: go test -race ./internal/dsync ./internal/lsync -run '^Test(DRWMutex|LRWMutex)' -count=1 -timeout=5m - name: Run CPU metrics tests under race detector run: go test -race ./cmd -run '^TestLoadCPUMetrics' -count=1 -timeout=5m diff --git a/cmd/data-scanner.go b/cmd/data-scanner.go index 8bbfd8f31..0475c4a53 100644 --- a/cmd/data-scanner.go +++ b/cmd/data-scanner.go @@ -744,10 +744,10 @@ func (f *folderScanner) scanFolder(ctx context.Context, folder cachedFolder, int } }, // Some disks have data for this. - partial: func(entries metaCacheEntries, errs []error) error { + partial: func(entries metaCacheEntries, errs []error) { if !f.shouldHeal() { cancel() - return nil + return } entry, ok := entries.resolve(&resolver) if !ok { @@ -772,7 +772,7 @@ func (f *folderScanner) scanFolder(ctx context.Context, folder cachedFolder, int } if entry.isDir() { - return nil + return } // We got an entry which we should be able to heal. @@ -787,7 +787,7 @@ func (f *folderScanner) scanFolder(ctx context.Context, folder cachedFolder, int scannerLogIf(ctx, err) } foundObjs = foundObjs || err == nil - return nil + return } custom["versions"] = fmt.Sprint(len(fiv.Versions)) @@ -814,7 +814,6 @@ func (f *folderScanner) scanFolder(ctx context.Context, folder cachedFolder, int } custom["success_versions"] = fmt.Sprint(successVersions) custom["failed_versions"] = fmt.Sprint(failVersions) - return nil }, // Too many disks failed. finished: func(errs []error) { diff --git a/cmd/erasure-healing.go b/cmd/erasure-healing.go index e02a075f1..8fb371270 100644 --- a/cmd/erasure-healing.go +++ b/cmd/erasure-healing.go @@ -86,7 +86,7 @@ func (er erasureObjects) listAndHeal(ctx context.Context, bucket, prefix string, cancel() } }, - partial: func(entries metaCacheEntries, _ []error) error { + partial: func(entries metaCacheEntries, _ []error) { entry, ok := entries.resolve(&resolver) if !ok { // check if we can get one entry at least @@ -94,12 +94,12 @@ func (er erasureObjects) listAndHeal(ctx context.Context, bucket, prefix string, entry, _ = entries.firstFound() } if !recursive && prefix != entry.name { - return nil + return } if err := healEntry(bucket, *entry, scanMode); err != nil { cancel() + return } - return nil }, finished: nil, } diff --git a/cmd/erasure-object.go b/cmd/erasure-object.go index b617a1b9a..e5d12967f 100644 --- a/cmd/erasure-object.go +++ b/cmd/erasure-object.go @@ -729,6 +729,57 @@ func readAllXL(ctx context.Context, disks []StorageAPI, bucket, object string, r return pickLatestQuorumFilesInfo(ctx, rawFileInfos, errs, bucket, object, readData, inclFreeVers) } +// readListingFileInfo is the read-only, latest-version metadata path for listing +// recovery. Ordinary GET keeps its fast-read, healing and dangling semantics. +// Absence is proved before decoding can replace a native storage error: every +// configured slot must have returned FileNotFound in this read. +func (er erasureObjects) readListingFileInfo(ctx context.Context, bucket, object string) (FileInfo, bool, error) { + disks := er.getDisks() + if len(disks) == 0 || len(disks) != er.setDriveCount { + return FileInfo{}, false, listingReadQuorumError(bucket, object) + } + // An offline slot is not evidence of absence, even if a test or transport + // implementation could return a cached not-found response for it. + online := append([]StorageAPI(nil), disks...) + for i, disk := range online { + if disk != nil && !disk.IsOnline() { + online[i] = nil + } + } + raw, errs := readAllRawFileInfo(ctx, online, bucket, object, false) + if err := ctx.Err(); err != nil { + return FileInfo{}, false, err + } + absent := true + for _, err := range errs { + absent = absent && err == errFileNotFound + } + if absent { + return FileInfo{}, true, nil + } + + // Keep the same latest-version and actual object parity quorum as GET, + // without its opportunistic dangling deletion or generic NotFound mapping. + meta, errs := pickLatestQuorumFilesInfo(ctx, raw, errs, bucket, object, false, false) + quorum, _, err := objectQuorumFromMeta(ctx, meta, errs, er.defaultParityCount) + if err == nil { + err = reduceReadQuorumErrs(ctx, errs, objectOpIgnoredErrs, quorum) + } + if err != nil { + return FileInfo{}, false, listingReadQuorumError(bucket, object) + } + _, modTime, etag := listOnlineDisks(disks, meta, errs, quorum) + fi, err := pickValidFileInfo(ctx, meta, modTime, etag, quorum) + if err != nil || (!fi.Deleted && len(fi.Erasure.Distribution) != len(disks)) { + return FileInfo{}, false, listingReadQuorumError(bucket, object) + } + return fi, false, nil +} + +func listingReadQuorumError(bucket, object string) error { + return InsufficientReadQuorum{Bucket: bucket, Object: decodeDirObject(object), Err: errErasureReadQuorum, Type: RQInconsistentMeta} +} + func (er erasureObjects) getObjectFileInfo(ctx context.Context, bucket, object string, opts ObjectOptions, readData bool) (FileInfo, []FileInfo, []StorageAPI, error) { rawArr := make([]RawFileInfo, er.setDriveCount) metaArr := make([]FileInfo, er.setDriveCount) diff --git a/cmd/erasure-server-pool-decom.go b/cmd/erasure-server-pool-decom.go index abae13c0d..0e422c3ae 100644 --- a/cmd/erasure-server-pool-decom.go +++ b/cmd/erasure-server-pool-decom.go @@ -730,12 +730,11 @@ func (set *erasureObjects) listObjectsToDecommission(ctx context.Context, bi dec minDisks: listingQuorum, reportNotFound: false, agreed: fn, - partial: func(entries metaCacheEntries, _ []error) error { + partial: func(entries metaCacheEntries, _ []error) { entry, ok := entries.resolve(&resolver) if ok { fn(*entry) } - return nil }, finished: nil, }) diff --git a/cmd/erasure-server-pool-rebalance.go b/cmd/erasure-server-pool-rebalance.go index f352c5f93..b60c9c904 100644 --- a/cmd/erasure-server-pool-rebalance.go +++ b/cmd/erasure-server-pool-rebalance.go @@ -525,12 +525,11 @@ func (set *erasureObjects) listObjectsToRebalance(ctx context.Context, bucketNam minDisks: listingQuorum, reportNotFound: false, agreed: fn, - partial: func(entries metaCacheEntries, _ []error) error { + partial: func(entries metaCacheEntries, _ []error) { entry, ok := entries.resolve(&resolver) if ok { fn(*entry) } - return nil }, finished: nil, }) diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go index 0b5fa9e85..c2cda0896 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -2587,15 +2587,11 @@ func (z *erasureServerPools) Walk(ctx context.Context, bucket, prefix string, re minDisks: listingQuorum, reportNotFound: false, agreed: send, - partial: func(entries metaCacheEntries, errs []error) error { - entry, err := set.resolveListEntry(ctx, bucket, entries, errs, &resolver, z.listingNSLock(set)) - if err != nil { - return err - } + partial: func(entries metaCacheEntries, errs []error) { + entry := set.resolveListEntry(entries, errs, &resolver) if entry != nil { send(*entry) } - return nil }, finished: nil, } @@ -2695,7 +2691,11 @@ func (z *erasureServerPools) Walk(ctx context.Context, bucket, prefix string, re // Merge all entries from all disks. // We leave quorum at 1, since entries are already resolved to have the desired quorum. // mergeEntryChannels will close 'merged' channel upon completion or cancellation. - errCh <- mergeEntryChannels(ctx, entries, merged, 1) + errCh <- mergeEntryChannelsWithResolver(ctx, entries, merged, 1, func(int) error { + return context.Cause(ctx) + }, func(group []*metaCacheEntry) (*metaCacheEntry, error) { + return z.resolveListingGroup(ctx, bucket, group) + }) }() return nil diff --git a/cmd/global-heal.go b/cmd/global-heal.go index e153418c1..ca78df818 100644 --- a/cmd/global-heal.go +++ b/cmd/global-heal.go @@ -529,7 +529,7 @@ func (er *erasureObjects) healErasureSet(ctx context.Context, buckets []string, jt.Take() go healEntry(bucket, entry) }, - partial: func(entries metaCacheEntries, _ []error) error { + partial: func(entries metaCacheEntries, _ []error) { entry, ok := entries.resolve(&resolver) if !ok { // check if we can get one entry at least @@ -538,7 +538,6 @@ func (er *erasureObjects) healErasureSet(ctx context.Context, buckets []string, } jt.Take() go healEntry(bucket, *entry) - return nil }, finished: func(errs []error) { success := countErrs(errs, nil) diff --git a/cmd/globals.go b/cmd/globals.go index ebec422ae..e71294508 100644 --- a/cmd/globals.go +++ b/cmd/globals.go @@ -319,6 +319,8 @@ var ( globalOperationTimeout = newDynamicTimeout(10*time.Minute, 5*time.Minute) // default timeout for general ops globalDeleteOperationTimeout = newDynamicTimeout(5*time.Minute, 1*time.Minute) // default time for delete ops + // Listing recovery has its own failure history and a five-second caller budget. + globalListingOperationTimeout = newDynamicTimeout(5*time.Second, 5*time.Second) globalBucketObjectLockSys *BucketObjectLockSys globalBucketQuotaSys *BucketQuotaSys diff --git a/cmd/local-locker-listing-reverse_test.go b/cmd/local-locker-listing-reverse_test.go new file mode 100644 index 000000000..8894feb2f --- /dev/null +++ b/cmd/local-locker-listing-reverse_test.go @@ -0,0 +1,412 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "context" + "fmt" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + "github.com/minio/minio/internal/dsync" +) + +func listingLockerTestWrite(t *testing.T, l *localLocker, args dsync.LockArgs) { + t.Helper() + if ok, err := l.Lock(t.Context(), args); !ok || err != nil { + t.Fatalf("Lock = %v, %v", ok, err) + } +} + +func listingLockerTestUnlock(t *testing.T, l *localLocker, args dsync.LockArgs) { + t.Helper() + if ok, err := l.Unlock(t.Context(), args); !ok || err != nil { + t.Fatalf("Unlock = %v, %v", ok, err) + } +} + +func TestLocalLockerListingReverseWaitRechecks(t *testing.T) { + for _, tc := range []struct { + name string + change func(*testing.T, *localLocker, dsync.LockArgs) + want bool + }{ + {"writer-released", func(t *testing.T, l *localLocker, w dsync.LockArgs) { listingLockerTestUnlock(t, l, w) }, true}, + {"writer-still-held", func(*testing.T, *localLocker, dsync.LockArgs) {}, false}, + {"replacement-writer", func(t *testing.T, l *localLocker, w dsync.LockArgs) { + listingLockerTestUnlock(t, l, w) + listingLockerTestWrite(t, l, listingLockerTestArgs("new-writer", "key", "writer")) + }, false}, + {"ordinary-reader-arrives", func(t *testing.T, l *localLocker, w dsync.LockArgs) { + listingLockerTestUnlock(t, l, w) + listingLockerTestRead(t, l, listingLockerTestArgs("ordinary-reader", "key", "ordinary-reader")) + }, true}, + {"listing-reader-arrives", func(t *testing.T, l *localLocker, w dsync.LockArgs) { + listingLockerTestUnlock(t, l, w) + listingLockerTestRead(t, l, listingLockerTestArgs("other-listing", "key", listingLockerTestSource)) + }, true}, + {"force-unlock", func(t *testing.T, l *localLocker, w dsync.LockArgs) { + if ok, err := l.ForceUnlock(t.Context(), w); !ok || err != nil { + t.Fatalf("ForceUnlock = %v, %v", ok, err) + } + }, true}, + {"expiry", func(_ *testing.T, l *localLocker, _ dsync.LockArgs) { l.expireOldLocks(-time.Minute) }, true}, + } { + t.Run(tc.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + writer := listingLockerTestArgs("writer", "key", "writer") + reader := listingLockerTestArgs("listing", "key", listingLockerTestSource) + listingLockerTestWrite(t, l, writer) + var got bool + var err error + start := time.Now() + go func() { got, err = l.RLock(t.Context(), reader) }() + synctest.Wait() + if state := listingLockerTestSnapshot(l); state.waiting != 1 { + t.Fatalf("waiting = %d, want 1", state.waiting) + } + listingLockerTestWrite(t, l, listingLockerTestArgs("unrelated", "other-key", "writer")) + if elapsed := time.Since(start); elapsed != 0 { + t.Fatalf("unrelated key waited %v", elapsed) + } + time.Sleep(time.Millisecond) + tc.change(t, l, writer) + time.Sleep(time.Millisecond) + synctest.Wait() + if state := listingLockerTestSnapshot(l); got != tc.want || err != nil || state.waiting != 0 { + t.Fatalf("RLock = %v, %v, waiting = %d; want %v, nil, 0", got, err, state.waiting, tc.want) + } + if elapsed := time.Since(start); elapsed != 2*time.Millisecond { + t.Fatalf("one attempt elapsed %v, want 2ms", elapsed) + } + found := false + for _, holder := range l.DupLockMap()["key"] { + if holder.UID == reader.UID { + found = true + if holder.Writer || holder.Timestamp != UTCNow().UnixNano() || holder.TimeLastRefresh != holder.Timestamp { + t.Fatalf("invalid granted reader: %+v", holder) + } + } + } + if found != got { + t.Fatalf("reader present = %v, granted = %v", found, got) + } + }) + }) + } +} + +func TestLocalLockerListingReverseWaitCancellation(t *testing.T) { + for _, when := range []string{"before-call", "during-wait", "deadline"} { + t.Run(when, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + writer := listingLockerTestArgs("writer", "key", "writer") + listingLockerTestWrite(t, l, writer) + ctx, cancel := context.WithCancel(t.Context()) + if when == "deadline" { + cancel() + ctx, cancel = context.WithTimeout(t.Context(), time.Millisecond) + } + defer cancel() + if when == "before-call" { + cancel() + } + var got bool + var err error + start := time.Now() + go func() { got, err = l.RLock(ctx, listingLockerTestArgs("listing", "key", listingLockerTestSource)) }() + synctest.Wait() + if when != "before-call" { + if state := listingLockerTestSnapshot(l); state.waiting != 1 { + t.Fatalf("waiting = %d, want 1", state.waiting) + } + time.Sleep(time.Millisecond) + // Removal must not turn the canceled request into a grant. + listingLockerTestUnlock(t, l, writer) + cancel() + } else { + listingLockerTestUnlock(t, l, writer) + } + synctest.Wait() + state := listingLockerTestSnapshot(l) + if got || err == nil || err != ctx.Err() || state != (listingLockerTestState{}) { + t.Fatalf("canceled RLock = %v, %v; context %v; state %+v", got, err, ctx.Err(), state) + } + if elapsed := time.Since(start); elapsed >= 2*time.Millisecond { + t.Fatalf("canceled request waited for full timer: %v", elapsed) + } + }) + }) + } +} + +func TestLocalLockerListingReverseWaitExcluded(t *testing.T) { + for _, source := range []string{"ordinary-reader", "", "[:0:()]", "listingRLock", "[metacache-server-pool.go:0:(*erasureServerPools).listingRLock()]"} { + t.Run(source, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + listingLockerTestWrite(t, l, listingLockerTestArgs("writer", "key", "writer")) + start := time.Now() + if ok, err := l.RLock(t.Context(), listingLockerTestArgs("reader", "key", source)); ok || err != nil { + t.Fatalf("excluded RLock = %v, %v", ok, err) + } + if state := listingLockerTestSnapshot(l); time.Since(start) != 0 || state.waiting != 0 || state.uids != 1 { + t.Fatalf("excluded RLock changed timing or state: %+v", state) + } + }) + }) + } +} + +func TestLocalLockerListingReverseWaitFastPaths(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + start := time.Now() + for i, source := range []string{listingLockerTestSource, "ordinary-reader", ""} { + listingLockerTestRead(t, l, listingLockerTestArgs(fmt.Sprintf("shared-%d", i), "shared", source)) + listingLockerTestRead(t, l, listingLockerTestArgs(fmt.Sprintf("free-%d", i), fmt.Sprintf("free-%d", i), source)) + } + if state := listingLockerTestSnapshot(l); time.Since(start) != 0 || state.waiting != 0 || state.uids != 6 { + t.Fatalf("uncontended readers changed timing or state: %+v", state) + } + }) +} + +func TestLocalLockerListingReverseWaitOverloaded(t *testing.T) { + for _, when := range []string{"before-call", "during-wait"} { + t.Run(when, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + writer := listingLockerTestArgs("writer", "key", "writer") + listingLockerTestWrite(t, l, writer) + defer l.waitMutex.Store(0) + if when == "before-call" { + l.waitMutex.Store(lockMutexWaitLimit + 1) + } + var got bool + var err error + go func() { + got, err = l.RLock(t.Context(), listingLockerTestArgs("listing", "key", listingLockerTestSource)) + }() + synctest.Wait() + if when == "during-wait" { + if state := listingLockerTestSnapshot(l); state.waiting != 1 { + t.Fatalf("waiting = %d, want 1", state.waiting) + } + listingLockerTestUnlock(t, l, writer) + l.waitMutex.Store(lockMutexWaitLimit + 1) + time.Sleep(2 * time.Millisecond) + synctest.Wait() + } else { + listingLockerTestUnlock(t, l, writer) + } + if state := listingLockerTestSnapshot(l); got || err != nil || state != (listingLockerTestState{}) || l.locksOverloaded.Load() != 1 { + t.Fatalf("overloaded RLock = %v, %v; state %+v; overloads %d", got, err, state, l.locksOverloaded.Load()) + } + }) + }) + } +} + +func TestLocalLockerListingWaitSharedAdmission(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + listingLockerTestRead(t, l, listingLockerTestArgs("reader", "read-key", listingLockerTestSource)) + listingLockerTestWrite(t, l, listingLockerTestArgs("writer", "write-key", "writer")) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + results := make(chan error, listingLockWaitLimit) + for i := range listingLockWaitLimit { + go func() { + var ok bool + var err error + if i%2 == 0 { + ok, err = l.Lock(ctx, listingLockerTestArgs(fmt.Sprintf("writer-%d", i), "read-key", "writer")) + } else { + ok, err = l.RLock(ctx, listingLockerTestArgs(fmt.Sprintf("listing-%d", i), "write-key", listingLockerTestSource)) + } + if ok { + err = fmt.Errorf("granted over a conflicting holder") + } + results <- err + }() + } + synctest.Wait() + if state := listingLockerTestSnapshot(l); state.waiting != listingLockWaitLimit { + t.Fatalf("combined waiting = %d, want %d", state.waiting, listingLockWaitLimit) + } + start := time.Now() + if ok, err := l.Lock(ctx, listingLockerTestArgs("excess-writer", "read-key", "writer")); ok || err != nil { + t.Fatalf("excess Lock = %v, %v", ok, err) + } + if ok, err := l.RLock(ctx, listingLockerTestArgs("excess-listing", "write-key", listingLockerTestSource)); ok || err != nil { + t.Fatalf("excess RLock = %v, %v", ok, err) + } + listingLockerTestWrite(t, l, listingLockerTestArgs("unrelated-writer", "unrelated-write", "writer")) + listingLockerTestRead(t, l, listingLockerTestArgs("unrelated-listing", "unrelated-read", listingLockerTestSource)) + if state := listingLockerTestSnapshot(l); time.Since(start) != 0 || state.waiting != listingLockWaitLimit || state.uids != 4 { + t.Fatalf("combined cap changed immediate paths: %+v", state) + } + cancel() + synctest.Wait() + for range listingLockWaitLimit { + if err := <-results; err != context.Canceled { + t.Fatalf("canceled waiter: %v", err) + } + } + if state := listingLockerTestSnapshot(l); state.waiting != 0 || state.uids != 4 { + t.Fatalf("combined wait leaked: %+v", state) + } + }) +} + +type listingReverseCountLocker struct { + *localLocker + calls atomic.Int32 + grants atomic.Int32 + reject bool +} + +func (l *listingReverseCountLocker) RLock(ctx context.Context, args dsync.LockArgs) (bool, error) { + l.calls.Add(1) + if l.reject { + return false, nil + } + ok, err := l.localLocker.RLock(ctx, args) + if ok { + l.grants.Add(1) + } + return ok, err +} + +func TestDistributedListingReverseWait(t *testing.T) { + for _, tc := range []struct { + name string + free, held int + immediateRejects int + cancel bool + want bool + }{ + {name: "all-writers-release", want: true}, + {name: "two-writers-remain", held: 2, want: true}, + {name: "three-writers-remain", held: 3}, + {name: "partial-grant-then-quorum", free: 1, want: true}, + {name: "partial-grant-without-quorum", free: 1, held: 3}, + {name: "late-grant-after-rejection", immediateRejects: 3}, + {name: "partial-and-late-grants-after-cancel", free: 1, cancel: true}, + } { + t.Run(tc.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + clients := make([]dsync.NetLocker, 4) + writer := listingLockerTestArgs("writer", "key", "writer") + for i := range clients { + l := &listingReverseCountLocker{localLocker: newLocker(), reject: i < tc.immediateRejects} + clients[i] = l + if i >= tc.free { + listingLockerTestWrite(t, l.localLocker, writer) + } + } + ds := &dsync.Dsync{GetLockers: func() ([]dsync.NetLocker, string) { return clients, writer.Owner }, Timeouts: dsync.DefaultTimeouts} + dm := dsync.NewDRWMutex(ds, "key") + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + result := make(chan bool, 1) + go func() { + result <- dm.GetRLock(ctx, nil, "listing", listingLockerTestSource, dsync.Options{Timeout: time.Second, RetryInterval: -1}) + }() + synctest.Wait() + for i, c := range clients { + l := c.(*listingReverseCountLocker) + if i < tc.free && l.grants.Load() != 1 { + t.Fatal("uncontended partial grant did not arrive immediately") + } + if i >= tc.free && !l.reject && listingLockerTestSnapshot(l.localLocker).waiting != 1 { + t.Fatal("conflicting reader did not wait in its first RPC") + } + } + earlyResult := tc.immediateRejects == 3 || tc.cancel + if tc.cancel { + cancel() + synctest.Wait() + } + if earlyResult { + select { + case got := <-result: + if got { + t.Fatal("attempt succeeded before delayed replies") + } + default: + t.Fatal("attempt did not return before delayed replies") + } + for i := range tc.free { + if state := listingLockerTestSnapshot(clients[i].(*listingReverseCountLocker).localLocker); state.uids != 0 { + t.Fatal("early partial grant was not released") + } + } + } + time.Sleep(time.Millisecond) + for i, c := range clients { + if i >= tc.free+tc.held { + listingLockerTestUnlock(t, c.(*listingReverseCountLocker).localLocker, writer) + } + } + time.Sleep(time.Millisecond) + synctest.Wait() + if !earlyResult { + if got := <-result; got != tc.want { + t.Fatalf("GetRLock = %v, want %v", got, tc.want) + } + } + for i, c := range clients { + l := c.(*listingReverseCountLocker) + if l.calls.Load() != 1 || listingLockerTestSnapshot(l.localLocker).waiting != 0 { + t.Fatalf("peer %d calls = %d or leaked admission", i, l.calls.Load()) + } + wantGrant := !l.reject && (i < tc.free || i >= tc.free+tc.held) + if (l.grants.Load() == 1) != wantGrant { + t.Fatalf("peer %d grants = %d, want granted = %v", i, l.grants.Load(), wantGrant) + } + if !tc.want { + for _, holder := range l.DupLockMap()["key"] { + if holder.UID == "listing" { + t.Fatalf("peer %d leaked partial or late read grant", i) + } + } + } + } + if tc.want { + dm.RUnlock(t.Context()) + synctest.Wait() + } + for i, c := range clients { + l := c.(*listingReverseCountLocker) + if i >= tc.free && i < tc.free+tc.held { + listingLockerTestUnlock(t, l.localLocker, writer) + } + if state := listingLockerTestSnapshot(l.localLocker); state != (listingLockerTestState{}) { + t.Fatalf("peer %d final state is not empty: %+v", i, state) + } + } + }) + }) + } +} diff --git a/cmd/local-locker-listing_test.go b/cmd/local-locker-listing_test.go new file mode 100644 index 000000000..f6f51d115 --- /dev/null +++ b/cmd/local-locker-listing_test.go @@ -0,0 +1,432 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "context" + "fmt" + "strings" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + "github.com/minio/minio/internal/dsync" +) + +const listingLockerTestSource = "[metacache-server-pool.go:274:(*erasureServerPools).listingRLock()]" + +func listingLockerTestArgs(uid, resource, source string) dsync.LockArgs { + quorum := 3 + return dsync.LockArgs{UID: uid, Resources: []string{resource}, Source: source, Owner: "listing-wait-test", Quorum: &quorum} +} + +func listingLockerTestRead(t *testing.T, l *localLocker, args dsync.LockArgs) { + t.Helper() + if ok, err := l.RLock(t.Context(), args); !ok || err != nil { + t.Fatalf("RLock = %v, %v", ok, err) + } +} + +func listingLockerTestRUnlock(t *testing.T, l *localLocker, args dsync.LockArgs) { + t.Helper() + if ok, err := l.RUnlock(t.Context(), args); !ok || err != nil { + t.Fatalf("RUnlock = %v, %v", ok, err) + } +} + +type listingLockerTestState struct { + waiting, resources, uids int +} + +func listingLockerTestSnapshot(l *localLocker) listingLockerTestState { + defer l.getMutex()() + return listingLockerTestState{waiting: l.listingWaiters, resources: len(l.lockMap), uids: len(l.lockUID)} +} + +func TestLocalLockerListingSource(t *testing.T) { + for _, source := range []string{ + "", "listingRLock", "[:0:()]", + strings.Replace(listingLockerTestSource, ":274:", ":", 1), + strings.Replace(listingLockerTestSource, "274", "", 1), + strings.Replace(listingLockerTestSource, "274", "0", 1), + strings.Replace(listingLockerTestSource, "274", "1x", 1), + strings.Replace(listingLockerTestSource, "274", "-1", 1), + strings.Replace(listingLockerTestSource, "listingRLock", "GetObjectNInfo", 1), + strings.Replace(listingLockerTestSource, "listingRLock", "listingRLock.func1", 1), + strings.Replace(listingLockerTestSource, "metacache-server-pool.go", "other.go", 1), + listingLockerTestSource + "trailing", + } { + if isListingReadLock([]lockRequesterInfo{{Source: source}}) { + t.Errorf("unrecognized source accepted: %q", source) + } + } + if isListingReadLock(nil) || isListingReadLock([]lockRequesterInfo{{Source: listingLockerTestSource, Writer: true}}) { + t.Fatal("empty or write holders accepted") + } + if isListingReadLock([]lockRequesterInfo{{Source: listingLockerTestSource}, {Source: "ordinary-reader"}}) { + t.Fatal("mixed readers accepted") + } + + // Check the real runtime-generated source, not only a hand-written label. + synctest.Test(t, func(t *testing.T) { + lockers := []dsync.NetLocker{newLocker(), newLocker(), newLocker(), newLocker()} + set := &erasureObjects{nsMutex: newNSLock(true), getLockers: func() ([]dsync.NetLocker, string) { return lockers, "listing-source-test" }} + z := &erasureServerPools{serverPools: []*erasureSets{{sets: []*erasureObjects{set}}, {sets: []*erasureObjects{set}}}} + _, unlock, err := z.listingRLock(t.Context(), "bucket", "object", set) + if err != nil { + t.Fatal(err) + } + for _, c := range lockers { + l := c.(*localLocker) + for _, holders := range l.DupLockMap() { + if !isListingReadLock(holders) { + t.Errorf("actual listingRLock source was not recognized: %#v", holders) + } + } + } + unlock() + synctest.Wait() + }) +} + +func FuzzListingLockSource(f *testing.F) { + f.Add(listingLockerTestSource) + f.Add("[metacache-server-pool.go:(*erasureServerPools).listingRLock()]") + f.Add("") + f.Fuzz(func(t *testing.T, source string) { + // Source is diagnostic text from a peer, including older versions. No + // malformed string may panic or make a writer eligible for this wait. + isListingReadLock([]lockRequesterInfo{{Source: source}}) + if isListingReadLock([]lockRequesterInfo{{Source: source, Writer: true}}) { + t.Fatal("writer was accepted as a listing reader") + } + }) +} + +func TestLocalLockerListingWaitRechecks(t *testing.T) { + for _, tc := range []struct { + name string + change func(*testing.T, *localLocker, dsync.LockArgs) + want bool + }{ + {"reader-released", func(t *testing.T, l *localLocker, r dsync.LockArgs) { listingLockerTestRUnlock(t, l, r) }, true}, + {"reader-still-held", func(*testing.T, *localLocker, dsync.LockArgs) {}, false}, + {"one-of-two-readers-remains", func(t *testing.T, l *localLocker, r dsync.LockArgs) { + other := r + other.UID = "second-reader" + listingLockerTestRead(t, l, other) + listingLockerTestRUnlock(t, l, r) + }, false}, + {"ordinary-reader-arrives", func(t *testing.T, l *localLocker, r dsync.LockArgs) { + listingLockerTestRUnlock(t, l, r) + listingLockerTestRead(t, l, listingLockerTestArgs("new-reader", "key", "ordinary-reader")) + }, false}, + {"other-writer-wins", func(t *testing.T, l *localLocker, r dsync.LockArgs) { + listingLockerTestRUnlock(t, l, r) + if ok, err := l.Lock(t.Context(), listingLockerTestArgs("other-writer", "key", "writer")); !ok || err != nil { + t.Fatalf("competing Lock = %v, %v", ok, err) + } + }, false}, + {"force-unlock", func(t *testing.T, l *localLocker, r dsync.LockArgs) { + if ok, err := l.ForceUnlock(t.Context(), r); !ok || err != nil { + t.Fatalf("ForceUnlock = %v, %v", ok, err) + } + }, true}, + {"expiry", func(_ *testing.T, l *localLocker, _ dsync.LockArgs) { l.expireOldLocks(-time.Minute) }, true}, + } { + t.Run(tc.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + reader := listingLockerTestArgs("reader", "key", listingLockerTestSource) + listingLockerTestRead(t, l, reader) + var got bool + var err error + start := time.Now() + go func() { got, err = l.Lock(t.Context(), listingLockerTestArgs("writer", "key", "writer")) }() + synctest.Wait() + if state := listingLockerTestSnapshot(l); state.waiting != 1 { + t.Fatalf("waiting = %d, want 1", state.waiting) + } + // The global mutex must be released throughout the timer wait. + if ok, err := l.Lock(t.Context(), listingLockerTestArgs("unrelated", "other-key", "writer")); !ok || err != nil { + t.Fatalf("unrelated key blocked: %v, %v", ok, err) + } + if elapsed := time.Since(start); elapsed != 0 { + t.Fatalf("unrelated key waited %v", elapsed) + } + time.Sleep(time.Millisecond) + tc.change(t, l, reader) + time.Sleep(time.Millisecond) + synctest.Wait() + if state := listingLockerTestSnapshot(l); got != tc.want || err != nil || state.waiting != 0 { + t.Fatalf("Lock = %v, %v, waiting = %d; want %v, nil, 0", got, err, state.waiting, tc.want) + } + if elapsed := time.Since(start); elapsed != 2*time.Millisecond { + t.Fatalf("one attempt elapsed %v, want 2ms", elapsed) + } + }) + }) + } +} + +func TestLocalLockerListingWaitCancellation(t *testing.T) { + for _, when := range []string{"before-call", "during-wait", "deadline"} { + t.Run(when, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + reader := listingLockerTestArgs("reader", "key", listingLockerTestSource) + listingLockerTestRead(t, l, reader) + ctx, cancel := context.WithCancel(t.Context()) + if when == "deadline" { + cancel() + ctx, cancel = context.WithTimeout(t.Context(), time.Millisecond) + } + defer cancel() + if when == "before-call" { + cancel() + } + var got bool + var err error + go func() { got, err = l.Lock(ctx, listingLockerTestArgs("writer", "key", "writer")) }() + synctest.Wait() + if when != "before-call" { + if state := listingLockerTestSnapshot(l); state.waiting != 1 { + t.Fatalf("waiting = %d", state.waiting) + } + time.Sleep(time.Millisecond) + cancel() + } + synctest.Wait() + if state := listingLockerTestSnapshot(l); got || err != ctx.Err() || err == nil || state.waiting != 0 { + t.Fatalf("canceled Lock = %v, %v; context %v; waiting = %d", got, err, ctx.Err(), state.waiting) + } + listingLockerTestRUnlock(t, l, reader) + if state := listingLockerTestSnapshot(l); state.resources != 0 || state.uids != 0 { + t.Fatal("canceled wait left a lock") + } + }) + }) + } +} + +func TestLocalLockerListingWaitExcluded(t *testing.T) { + for _, kind := range []string{"ordinary-reader", "unknown-reader", "mixed-readers", "writer", "bulk", "overloaded"} { + t.Run(kind, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + reader := listingLockerTestArgs("holder", "key", listingLockerTestSource) + if kind == "ordinary-reader" { + reader.Source = "[erasure-object.go:100:(*erasureObjects).GetObjectNInfo()]" + } + if kind == "unknown-reader" { + reader.Source = "" + } + if kind == "writer" { + if ok, err := l.Lock(t.Context(), reader); !ok || err != nil { + t.Fatalf("initial write = %v, %v", ok, err) + } + } else { + listingLockerTestRead(t, l, reader) + } + if kind == "mixed-readers" { + listingLockerTestRead(t, l, listingLockerTestArgs("ordinary", "key", "ordinary-reader")) + } + writer := listingLockerTestArgs("writer", "key", "writer") + if kind == "bulk" { + writer.Resources = []string{"key", "other-key"} + } + if kind == "overloaded" { + l.waitMutex.Store(lockMutexWaitLimit + 1) + } + start := time.Now() + if ok, err := l.Lock(t.Context(), writer); ok || err != nil { + t.Fatalf("conflicting Lock = %v, %v", ok, err) + } + state := listingLockerTestSnapshot(l) + if elapsed := time.Since(start); elapsed != 0 || state.waiting != 0 { + t.Fatalf("excluded path waited %v; waiters = %d", elapsed, state.waiting) + } + if _, ok := l.DupLockMap()["other-key"]; ok { + t.Fatal("bulk request partially acquired an unrelated key") + } + }) + }) + } +} + +func TestLocalLockerListingWaitAdmission(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + reader := listingLockerTestArgs("reader", "key", listingLockerTestSource) + listingLockerTestRead(t, l, reader) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + results := make(chan error, listingLockWaitLimit) + for i := range listingLockWaitLimit { + go func() { + ok, err := l.Lock(ctx, listingLockerTestArgs(fmt.Sprintf("writer-%d", i), "key", "writer")) + if ok { + err = fmt.Errorf("granted over a live reader") + } + results <- err + }() + } + synctest.Wait() + if state := listingLockerTestSnapshot(l); state.waiting != listingLockWaitLimit { + t.Fatalf("waiting = %d, want %d", state.waiting, listingLockWaitLimit) + } + start := time.Now() + if ok, err := l.Lock(ctx, listingLockerTestArgs("excess", "key", "writer")); ok || err != nil { + t.Fatalf("excess waiter = %v, %v", ok, err) + } + if state := listingLockerTestSnapshot(l); time.Since(start) != 0 || state.waiting != listingLockWaitLimit { + t.Fatal("excess waiter consumed time or admission") + } + if ok, err := l.Lock(ctx, listingLockerTestArgs("unrelated", "other-key", "writer")); !ok || err != nil { + t.Fatalf("admission limit rejected an uncontended key: %v, %v", ok, err) + } + cancel() + synctest.Wait() + for range listingLockWaitLimit { + if err := <-results; err != context.Canceled { + t.Fatalf("canceled waiter: %v", err) + } + } + if state := listingLockerTestSnapshot(l); state.waiting != 0 { + t.Fatalf("admission leaked: %d", state.waiting) + } + }) +} + +func TestLocalLockerListingWaitContended(t *testing.T) { + for _, n := range []int{2, 8, 32} { + t.Run(fmt.Sprintf("%d-writers", n), func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + l := newLocker() + reader := listingLockerTestArgs("reader", "key", listingLockerTestSource) + listingLockerTestRead(t, l, reader) + results := make(chan bool, n) + for i := range n { + go func() { + ok, err := l.Lock(t.Context(), listingLockerTestArgs(fmt.Sprintf("writer-%d", i), "key", "writer")) + if err != nil { + t.Errorf("Lock: %v", err) + } + results <- ok + }() + } + synctest.Wait() + if state := listingLockerTestSnapshot(l); state.waiting != n { + t.Fatalf("waiting = %d, want %d", state.waiting, n) + } + time.Sleep(time.Millisecond) + listingLockerTestRUnlock(t, l, reader) + time.Sleep(time.Millisecond) + synctest.Wait() + granted := 0 + for range n { + if <-results { + granted++ + } + } + if state := listingLockerTestSnapshot(l); granted != 1 || state.uids != 1 || state.waiting != 0 { + t.Fatalf("grants = %d, indexed = %d, waiting = %d", granted, state.uids, state.waiting) + } + }) + }) + } +} + +type listingCountLocker struct { + *localLocker + calls atomic.Int32 +} + +func (l *listingCountLocker) Lock(ctx context.Context, args dsync.LockArgs) (bool, error) { + l.calls.Add(1) + return l.localLocker.Lock(ctx, args) +} + +// Exercise real DRWMutex quorum and late-response cleanup. The mixed case with +// two immediate rejections fails before the upgraded lockers finish waiting. +func TestDistributedListingWait(t *testing.T) { + for _, ordinaryReaders := range []int{0, 1, 2} { + t.Run(fmt.Sprintf("%d-immediate-rejections", ordinaryReaders), func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + clients := make([]dsync.NetLocker, 4) + reader := listingLockerTestArgs("reader", "key", listingLockerTestSource) + for i := range clients { + l := &listingCountLocker{localLocker: newLocker()} + clients[i] = l + r := reader + if i < ordinaryReaders { + r.Source = "old-node-unknown-source" + } + listingLockerTestRead(t, l.localLocker, r) + } + ds := &dsync.Dsync{GetLockers: func() ([]dsync.NetLocker, string) { return clients, reader.Owner }, Timeouts: dsync.DefaultTimeouts} + dm := dsync.NewDRWMutex(ds, "key") + var got bool + go func() { + got = dm.GetLock(t.Context(), nil, "writer", "writer", dsync.Options{Timeout: time.Second, RetryInterval: -1}) + }() + synctest.Wait() + time.Sleep(time.Millisecond) + for _, c := range clients { + listingLockerTestRUnlock(t, c.(*listingCountLocker).localLocker, reader) + } + time.Sleep(time.Millisecond) + synctest.Wait() + if got != (ordinaryReaders < 2) { + t.Fatalf("GetLock = %v", got) + } + granted := 0 + for _, c := range clients { + l := c.(*listingCountLocker) + if l.calls.Load() != 1 { + t.Fatalf("lock calls = %d, want one round", l.calls.Load()) + } + state := listingLockerTestSnapshot(l.localLocker) + if state.resources != 0 { + granted++ + } + if state.waiting != 0 { + t.Fatal("wait admission leaked") + } + } + if got && granted < 3 { + t.Fatalf("writer accepted without quorum: %d", granted) + } + if !got && granted != 0 { + t.Fatalf("late grants leaked after rejected attempt: %d", granted) + } + if got { + dm.Unlock(t.Context()) + synctest.Wait() + } + for _, c := range clients { + l := c.(*listingCountLocker) + if state := listingLockerTestSnapshot(l.localLocker); state.resources != 0 || state.uids != 0 { + t.Fatal("final lock state not empty") + } + } + }) + }) + } +} diff --git a/cmd/local-locker.go b/cmd/local-locker.go index 3fd630b9c..7c441b43a 100644 --- a/cmd/local-locker.go +++ b/cmd/local-locker.go @@ -23,6 +23,7 @@ import ( "context" "fmt" "strconv" + "strings" "sync" "sync/atomic" "time" @@ -38,6 +39,14 @@ import ( // Rejected clients are expected to retry. const lockMutexWaitLimit = 1000 +// A short listing recovery read or conflicting writer may finish before the +// rejected request retries. Recheck once in the same request, sharing a bound +// for timers outside the mutex. This is not a queue and gives neither priority. +const ( + listingLockWait = 2 * time.Millisecond + listingLockWaitLimit = 64 +) + // lockRequesterInfo stores various info from the client for each lock that is requested. type lockRequesterInfo struct { Name string // name of the resource lock was requested for @@ -65,6 +74,8 @@ type localLocker struct { waitMutex atomic.Int32 lockMap map[string][]lockRequesterInfo lockUID map[string]string // UUID -> resource map. + // Protected by mutex. These waiters are not included in waitMutex. + listingWaiters int // the following are updated on every cleanup defined in lockValidityDuration readers atomic.Int32 @@ -96,6 +107,32 @@ func (l *localLocker) canTakeLock(resources ...string) bool { return true } +// isListingReadLock recognizes only the existing diagnostic source emitted by +// listingRLock. Unknown or changed source formats retain immediate rejection; +// Source is not a new wire contract or proof that the read will finish quickly. +func isListingReadLock(holders []lockRequesterInfo) bool { + if len(holders) == 0 { + return false + } + const prefix = "[metacache-server-pool.go:" + const suffix = ":(*erasureServerPools).listingRLock()]" + for _, holder := range holders { + if holder.Writer || len(holder.Source) <= len(prefix)+len(suffix) || !strings.HasPrefix(holder.Source, prefix) || !strings.HasSuffix(holder.Source, suffix) { + return false + } + line := holder.Source[len(prefix) : len(holder.Source)-len(suffix)] + if len(line) == 0 || line[0] == '0' { + return false + } + for _, c := range line { + if c < '0' || c > '9' { + return false + } + } + } + return true +} + func (l *localLocker) Lock(ctx context.Context, args dsync.LockArgs) (reply bool, err error) { if len(args.Resources) > maxDeleteList { return false, fmt.Errorf("internal error: localLocker.Lock called with more than %d resources", maxDeleteList) @@ -112,9 +149,33 @@ func (l *localLocker) Lock(ctx context.Context, args dsync.LockArgs) (reply bool return false, ctx.Err() } if !l.canTakeLock(args.Resources...) { - // Not all locks can be taken on resources, - // reject it completely. - return false, nil + // Only a single-resource write blocked exclusively by listing recovery + // readers gets a short, bounded opportunity to succeed in this attempt. + if len(args.Resources) != 1 || l.listingWaiters >= listingLockWaitLimit || !isListingReadLock(l.lockMap[args.Resources[0]]) { + return false, nil + } + l.listingWaiters++ + l.mutex.Unlock() + timer := time.NewTimer(listingLockWait) + select { + case <-ctx.Done(): + case <-timer.C: + } + timer.Stop() + // Restore the mutex before returning to the existing deferred unlock. + // Readers or another writer may have entered while it was released. + l.getMutex() + l.listingWaiters-- + if ctx.Err() != nil { + return false, ctx.Err() + } + if l.waitMutex.Load() > lockMutexWaitLimit { + l.locksOverloaded.Add(1) + return false, nil + } + if !l.canTakeLock(args.Resources...) { + return false, nil + } } // No locks held on the all resources, so claim write @@ -206,6 +267,31 @@ func (l *localLocker) RLock(ctx context.Context, args dsync.LockArgs) (reply boo return false, ctx.Err() } resource := args.Resources[0] + lri, ok := l.lockMap[resource] + if ok && isWriteLock(lri) && l.listingWaiters < listingLockWaitLimit && isListingReadLock([]lockRequesterInfo{{Source: args.Source}}) { + // Only a positively identified listing read blocked by a writer may + // wait once. A long or replacement writer still rejects normally. + l.listingWaiters++ + l.mutex.Unlock() + timer := time.NewTimer(listingLockWait) + select { + case <-ctx.Done(): + case <-timer.C: + } + timer.Stop() + // Restore the mutex before the existing deferred unlock, then use + // the current holders in the normal read-lock grant path below. + l.getMutex() + l.listingWaiters-- + if ctx.Err() != nil { + return false, ctx.Err() + } + if l.waitMutex.Load() > lockMutexWaitLimit { + l.locksOverloaded.Add(1) + return false, nil + } + lri, ok = l.lockMap[resource] + } now := UTCNow() lrInfo := lockRequesterInfo{ Name: resource, @@ -217,7 +303,6 @@ func (l *localLocker) RLock(ctx context.Context, args dsync.LockArgs) (reply boo TimeLastRefresh: now.UnixNano(), Quorum: *args.Quorum, } - lri, ok := l.lockMap[resource] if ok { if reply = !isWriteLock(lri); reply { // Unless there is a write lock diff --git a/cmd/metacache-entries.go b/cmd/metacache-entries.go index b94de924c..3a0600bee 100644 --- a/cmd/metacache-entries.go +++ b/cmd/metacache-entries.go @@ -37,6 +37,10 @@ type metaCacheEntry struct { // Entries without metadata will only be present in non-recursive scans. metadata []byte + // listingSet marks an eligible unresolved walk entry. It is consumed by the + // pool merger before any cache serialization or downstream filtering. + listingSet *erasureObjects + // cached contains the metadata if decoded. cached *xlMetaV2 @@ -46,17 +50,17 @@ type metaCacheEntry struct { // isDir returns if the entry is representing a prefix directory. func (e metaCacheEntry) isDir() bool { - return len(e.metadata) == 0 && strings.HasSuffix(e.name, slashSeparator) + return e.listingSet == nil && len(e.metadata) == 0 && strings.HasSuffix(e.name, slashSeparator) } // isObject returns if the entry is representing an object. func (e metaCacheEntry) isObject() bool { - return len(e.metadata) > 0 + return e.listingSet != nil || len(e.metadata) > 0 } // isObjectDir returns if the entry is representing an object/ func (e metaCacheEntry) isObjectDir() bool { - return len(e.metadata) > 0 && strings.HasSuffix(e.name, slashSeparator) + return (e.listingSet != nil || len(e.metadata) > 0) && strings.HasSuffix(e.name, slashSeparator) } // hasPrefix returns whether an entry has a specific prefix @@ -676,29 +680,76 @@ func mergeEntryChannels(ctx context.Context, in []chan metaCacheEntry, out chan< // terminalErr is inspected after each input closes and before the merger // advances beyond it. Callers must publish the error before closing the input. func mergeEntryChannelsWithErrors(ctx context.Context, in []chan metaCacheEntry, out chan<- metaCacheEntry, readQuorum int, terminalErr func(int) error) error { + return mergeEntryChannelsWithResolver(ctx, in, out, readQuorum, terminalErr, nil) +} + +// resolve is called only for groups containing a pending listing entry. All +// selected inputs are advanced (and their terminal errors checked) before it is +// called, so recovery never holds an object lock while waiting for input. +func mergeEntryChannelsWithResolver(ctx context.Context, in []chan metaCacheEntry, out chan<- metaCacheEntry, readQuorum int, terminalErr func(int) error, resolve func([]*metaCacheEntry) (*metaCacheEntry, error)) error { defer xioutil.SafeClose(out) top := make([]*metaCacheEntry, len(in)) nDone := 0 ctxDone := ctx.Done() - // Use simpler forwarder. + // Keep the single-input normal path allocation-free. Only a pending entry + // requires looking ahead to observe its stream's terminal error first. if len(in) == 1 { + var buffered metaCacheEntry + haveBuffered, exhausted := false, false for { - select { - case <-ctxDone: - return ctx.Err() - case v, ok := <-in[0]: - if !ok { - if terminalErr != nil { - return terminalErr(0) + var v metaCacheEntry + if haveBuffered { + v, haveBuffered = buffered, false + } else { + select { + case <-ctxDone: + return ctx.Err() + case entry, ok := <-in[0]: + if !ok { + if terminalErr != nil { + return terminalErr(0) + } + return nil } - return nil + v = entry + } + } + if v.listingSet != nil { + if resolve == nil { + return errInvalidArgument } select { case <-ctxDone: return ctx.Err() - case out <- v: + case entry, ok := <-in[0]: + buffered, haveBuffered, exhausted = entry, ok, !ok + if !ok && terminalErr != nil { + if err := terminalErr(0); err != nil { + return err + } + } } + pending := v + resolved, err := resolve([]*metaCacheEntry{&pending}) + if err != nil { + return err + } + if resolved == nil { + if exhausted { + return nil + } + continue + } + v = *resolved + } + select { + case <-ctxDone: + return ctx.Err() + case out <- v: + } + if exhausted { + return nil } } } @@ -792,6 +843,40 @@ func mergeEntryChannelsWithErrors(ctx context.Context, in []chan metaCacheEntry, } } + pending := best.listingSet != nil + for _, idx := range toMerge { + pending = pending || top[idx].listingSet != nil + } + if pending { + if resolve == nil { + return errInvalidArgument + } + group := make([]*metaCacheEntry, 1, len(toMerge)+1) + group[0] = best + for _, idx := range toMerge { + group = append(group, top[idx]) + if err := selectFrom(idx); err != nil { + return err + } + } + if err := selectFrom(bestIdx); err != nil { + return err + } + resolved, err := resolve(group) + if err != nil { + return err + } + if resolved != nil && resolved.name > last { + select { + case <-ctxDone: + return ctx.Err() + case out <- *resolved: + last = resolved.name + } + } + continue + } + // Merge any unmerged if len(toMerge) > 0 { versions := make([][]xlMetaV2ShallowVersion, 0, len(toMerge)+1) diff --git a/cmd/metacache-listing-cache_test.go b/cmd/metacache-listing-cache_test.go index 68140fa8c..55cfcbb1f 100644 --- a/cmd/metacache-listing-cache_test.go +++ b/cmd/metacache-listing-cache_test.go @@ -21,6 +21,7 @@ import ( "bytes" "context" "encoding/xml" + "errors" "net/http" "net/http/httptest" "net/url" @@ -324,3 +325,79 @@ func TestListingCacheFullPageDoesNotWaitForTail(t *testing.T) { t.Fatalf("cached remainder lost keys: %v", keys) } } + +// Keep the error update pending at the actual peer RPC boundary. Other cache +// operations, including readers and abandoned-cache cleanup, remain live. +func listingCacheErrorPeer(t *testing.T, fail bool) (*peerRESTClient, <-chan struct{}, func()) { + t.Helper() + tg, err := grid.SetupTestGrid(2) + if err != nil { + t.Fatal(err) + } + ready, resume := make(chan struct{}), make(chan struct{}) + release := sync.OnceFunc(func() { close(resume) }) + t.Cleanup(func() { + release() + tg.Cleanup() + }) + var entered sync.Once + server := &peerRESTServer{} + if err := getMetacacheListingRPC.Register(tg.Managers[1], server.GetMetacacheListingHandler); err != nil { + t.Fatal(err) + } + if err := updateMetacacheListingRPC.Register(tg.Managers[1], func(m *metacache) (*metacache, *grid.RemoteErr) { + if m.error != "" && m.error != "no longer used" { + entered.Do(func() { close(ready) }) + select { + case <-resume: + case <-t.Context().Done(): + return nil, grid.NewRemoteErr(t.Context().Err()) + } + if fail { + return nil, grid.NewRemoteErr(errors.New("cache error update failed")) + } + } + return server.UpdateMetacacheListingHandler(m) + }); err != nil { + t.Fatal(err) + } + return &peerRESTClient{gridConn: func() *grid.Connection { return tg.Managers[0].Connection(tg.Hosts[1]) }}, ready, release +} + +func TestListingCacheRetainsErrorAfterPeerFailure(t *testing.T) { + client, ready, release := listingCacheErrorPeer(t, true) + o := listPathOptions{ID: mustGetUUID(), Bucket: "bucket"} + m := o.newMetacache() + mc := metaCacheRPC{meta: &m, rpc: client, o: o} + done := make(chan struct{}) + go func() { + defer close(done) + mc.setErr("terminal listing failure") + }() + t.Cleanup(func() { + release() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Error("setErr worker did not finish before grid cleanup") + } + }) + select { + case <-ready: + case <-time.After(5 * time.Second): + t.Fatal("error update did not reach peer") + } + release() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("failed peer update did not return") + } + if m.id != o.ID || m.bucket != o.Bucket || m.status != scanStateError || m.error != "terminal listing failure" || mc.getErr() == nil { + t.Fatalf("peer failure erased local failure: %+v", m) + } + mc.setErr("later failure") + if m.error != "terminal listing failure" { + t.Fatalf("later failure replaced the terminal error: %+v", m) + } +} diff --git a/cmd/metacache-listing-delete_test.go b/cmd/metacache-listing-delete_test.go index 74cfa2b5e..c59e64844 100644 --- a/cmd/metacache-listing-delete_test.go +++ b/cmd/metacache-listing-delete_test.go @@ -126,7 +126,7 @@ func TestListFallbackRejectsSplitGenerations(t *testing.T) { } } resolver := metadataResolutionParams{dirQuorum: 8, objQuorum: 8, bucket: bucket, requestedVersions: 1} - entry, err := set.resolveListEntry(t.Context(), bucket, entries, nil, &resolver, z.listingNSLock(set)) + entry, err := z.resolveListingGroup(t.Context(), bucket, []*metaCacheEntry{set.resolveListEntry(entries, nil, &resolver)}) if entry != nil || !isErrReadQuorum(err) { t.Fatalf("split live metadata became absence: entry=%v err=%v", entry, err) } diff --git a/cmd/metacache-listing-lock-routes_test.go b/cmd/metacache-listing-lock-routes_test.go index 040fc5139..32c5f94e5 100644 --- a/cmd/metacache-listing-lock-routes_test.go +++ b/cmd/metacache-listing-lock-routes_test.go @@ -35,6 +35,8 @@ func TestListFallbackDistributedLockRoute(t *testing.T) { {"multi-pool-shared-lockers", true, true}, {"multi-pool-disjoint-lockers", true, false}, {"single-pool-target-set-lockers", false, false}, + {"single-pool-first-set-lockers", false, false}, + {"single-pool-shared-ns-distinct-lockers", false, false}, } { t.Run(tc.name, func(t *testing.T) { a := []dsync.NetLocker{newLocker(), newLocker()} @@ -44,12 +46,15 @@ func TestListFallbackDistributedLockRoute(t *testing.T) { } set0 := &erasureObjects{nsMutex: newNSLock(true), getLockers: func() ([]dsync.NetLocker, string) { return a, "listing-test" }} target := &erasureObjects{nsMutex: newNSLock(true), getLockers: func() ([]dsync.NetLocker, string) { return b, "listing-test" }} + if tc.name == "single-pool-shared-ns-distinct-lockers" { + target.nsMutex = set0.nsMutex + } z := &erasureServerPools{serverPools: []*erasureSets{{sets: []*erasureObjects{set0, target}}}} if tc.multiPool { z.serverPools = []*erasureSets{{sets: []*erasureObjects{set0}}, {sets: []*erasureObjects{target}}} } writer := target.NewNSLock("bucket", "object") - if tc.multiPool { + if tc.multiPool || tc.name == "single-pool-first-set-lockers" { writer = z.NewNSLock("bucket", "object") } wctx, err := writer.GetLock(t.Context(), globalOperationTimeout) @@ -62,23 +67,65 @@ func TestListFallbackDistributedLockRoute(t *testing.T) { writer.Unlock(wctx) } }() - reader := z.listingNSLock(target)("bucket", "object") ctx, cancel := context.WithTimeout(t.Context(), 80*time.Millisecond) defer cancel() - rctx, err := reader.GetRLock(ctx, globalOperationTimeout) + _, unlock, err := z.listingRLock(ctx, "bucket", "object", target) if err == nil { - reader.RUnlock(rctx) + unlock() t.Fatal("fallback read lock acquired while ordinary writer lock is held") } writer.Unlock(wctx) locked = false ctx, cancel = context.WithTimeout(t.Context(), 5*time.Second) defer cancel() - rctx, err = reader.GetRLock(ctx, globalOperationTimeout) + _, unlock, err = z.listingRLock(ctx, "bucket", "object", target) if err != nil { t.Fatalf("fallback read lock failed after writer unlock: %v", err) } - reader.RUnlock(rctx) + unlock() }) } } + +// Cancellation while waiting for the second domain must release the first. +func TestListingSecondLockCancellationReleasesFirst(t *testing.T) { + first := &erasureObjects{nsMutex: newNSLock(false)} + target := &erasureObjects{nsMutex: newNSLock(false)} + z := &erasureServerPools{serverPools: []*erasureSets{{sets: []*erasureObjects{first, target}}}} + writer := target.NewNSLock("bucket", "object") + wctx, err := writer.GetLock(t.Context(), globalOperationTimeout) + if err != nil { + t.Fatal(err) + } + defer writer.Unlock(wctx) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan struct{}) + var lockErr error + go func() { + defer close(done) + _, unlock, err := z.listingRLock(ctx, "bucket", "object", target) + lockErr = err + if err == nil { + unlock() + } + }() + waitForListingReadLock(t, target.nsMutex, "bucket", "object", done) + cancel() + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("second lock did not cancel") + } + if lockErr == nil { + t.Fatal("canceled second lock succeeded") + } + poolWriter := z.NewNSLock("bucket", "object") + ctx, cancel = context.WithTimeout(t.Context(), time.Second) + defer cancel() + pctx, err := poolWriter.GetLock(ctx, globalOperationTimeout) + if err != nil { + t.Fatalf("first lock leaked after second failed: %v", err) + } + poolWriter.Unlock(pctx) +} diff --git a/cmd/metacache-listing-locks_test.go b/cmd/metacache-listing-locks_test.go index bd5998467..2ba18d51d 100644 --- a/cmd/metacache-listing-locks_test.go +++ b/cmd/metacache-listing-locks_test.go @@ -75,7 +75,7 @@ func TestListFallbackMultiPoolReadLock(t *testing.T) { var readErr error go func() { defer close(done) - entry, readErr = set.resolveListEntry(ctx, bucket, entries, errs, &resolver, z.listingNSLock(set)) + entry, readErr = z.resolveListingGroup(ctx, bucket, []*metaCacheEntry{set.resolveListEntry(entries, errs, &resolver)}) }() waitForListingReadLock(t, z.serverPools[0].sets[0].nsMutex, bucket, object, done) lock.Unlock(lkctx) diff --git a/cmd/metacache-listing-recovery_test.go b/cmd/metacache-listing-recovery_test.go new file mode 100644 index 000000000..57daef0bf --- /dev/null +++ b/cmd/metacache-listing-recovery_test.go @@ -0,0 +1,391 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "slices" + "sync/atomic" + "testing" + "time" + "unsafe" +) + +type listingRecoveryDisk struct { + StorageAPI + reads *atomic.Int64 + err error + emptySuccess bool +} + +func (d *listingRecoveryDisk) ReadXL(ctx context.Context, bucket, object string, data bool) (RawFileInfo, error) { + if d.reads != nil { + d.reads.Add(1) + } + if d.emptySuccess { + return RawFileInfo{}, nil + } + if d.err != nil { + return RawFileInfo{}, d.err + } + return d.StorageAPI.ReadXL(ctx, bucket, object, data) +} + +func TestListingRecoveryStrictAbsence(t *testing.T) { + z, bucket, _ := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + disks, switchDisks := installListingDiskSwitch(set) + for _, tc := range []struct { + name string + err error + offline, empty, absent bool + }{ + {name: "all-native-not-found", absent: true}, + {name: "offline", offline: true}, + {name: "volume-not-found", err: errVolumeNotFound}, + {name: "version-not-found", err: errFileVersionNotFound}, + {name: "corrupt", err: errFileCorrupt}, + {name: "decode-generated-not-found", empty: true}, + } { + t.Run(tc.name, func(t *testing.T) { + next := slices.Clone(disks) + next[0] = &listingRecoveryDisk{StorageAPI: disks[0], err: tc.err, emptySuccess: tc.empty} + if tc.offline { + next[0] = &offlineNullQuorumDisk{StorageAPI: disks[0]} + } + switchDisks(next) + _, absent, err := set.readListingFileInfo(t.Context(), bucket, "missing") + if absent != tc.absent || (tc.absent && err != nil) || (!tc.absent && !isErrReadQuorum(err)) { + t.Fatalf("absent=%v err=%v, want absent=%v", absent, err, tc.absent) + } + }) + } + switchDisks(disks) +} + +// The target walk can predate the copy. Only current cross-pool reads can +// distinguish this copy-before-delete schedule from a genuine object deletion. +func TestListingRecoveryCurrentPools(t *testing.T) { + for _, tc := range []struct { + name string + target, marker, unknown, stale bool + }{ + {name: "copied-before-source-delete", target: true}, + {name: "deleted-with-stale-other-walk", stale: true}, + {name: "current-marker-over-stale-live", target: true, marker: true, stale: true}, + {name: "missing-source-unknown-target", unknown: true}, + } { + t.Run(tc.name, func(t *testing.T) { + z, bucket, _ := listingPoolsBackend(t) + target, source := z.serverPools[0].sets[0], z.serverPools[1].sets[0] + targetDisks, switchTarget := installListingDiskSwitch(target) + sourceDisks, switchSource := installListingDiskSwitch(source) + const object = "moved" + body := bytes.Repeat([]byte{'a'}, 8192) + mtime := time.Now().UTC().Add(-time.Hour) + put := func(set *erasureObjects) { + _, err := set.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(body), int64(len(body)), "", ""), ObjectOptions{MTime: mtime, VersionSuspended: true}) + if err != nil { + t.Fatal(err) + } + } + put(source) + var stale *metaCacheEntry + if tc.stale || tc.target { + put(target) + stale = &metaCacheEntry{name: object, metadata: mustReadNullQuorumMeta(t, targetDisks[0], bucket, object)} + } + if tc.stale && !tc.target { + if _, err := target.DeleteObject(t.Context(), bucket, object, ObjectOptions{}); err != nil { + t.Fatal(err) + } + } + if tc.marker { + if _, err := target.DeleteObject(t.Context(), bucket, object, ObjectOptions{VersionSuspended: true, DeleteMarker: true, MTime: mtime.Add(time.Minute)}); err != nil { + t.Fatal(err) + } + } + // Actual copy is committed before actual source cleanup. + if _, err := source.DeleteObject(t.Context(), bucket, object, ObjectOptions{}); err != nil { + t.Fatal(err) + } + var sourceReads, targetReads atomic.Int64 + nextSource, nextTarget := slices.Clone(sourceDisks), slices.Clone(targetDisks) + for i := range nextSource { + nextSource[i] = &listingRecoveryDisk{StorageAPI: sourceDisks[i], reads: &sourceReads} + } + for i := range nextTarget { + nextTarget[i] = &listingRecoveryDisk{StorageAPI: targetDisks[i], reads: &targetReads} + } + if tc.unknown { + nextTarget[0] = &listingRecoveryDisk{StorageAPI: targetDisks[0], reads: &targetReads, err: errDiskNotFound} + } + switchSource(nextSource) + switchTarget(nextTarget) + group := []*metaCacheEntry{{name: object, listingSet: source}} + if stale != nil && tc.stale { + group = append(group, stale) + } + entry, err := z.resolveListingGroup(t.Context(), bucket, group) + if sourceReads.Load() != int64(len(sourceDisks)) || targetReads.Load() != int64(len(targetDisks)) { + t.Fatalf("reads source=%d target=%d", sourceReads.Load(), targetReads.Load()) + } + if tc.unknown { + if entry != nil || !isErrReadQuorum(err) { + t.Fatalf("unknown became success: entry=%v err=%v", entry, err) + } + return + } + if err != nil { + t.Fatal(err) + } + if !tc.target { + if entry != nil { + t.Fatalf("stale walk survived current absence: %+v", entry) + } + return + } + if entry == nil || entry.listingSet != nil { + t.Fatalf("missing current result: %+v", entry) + } + fi, err := entry.fileInfo(bucket) + if err != nil || fi.Deleted != tc.marker { + t.Fatalf("current result deleted=%v err=%v, want marker=%v", fi.Deleted, err, tc.marker) + } + }) + } +} + +func TestListingRecoveryNormalPathDoesNotReadOrLock(t *testing.T) { + z, bucket, _ := listingPoolsBackend(t) + const object = "normal" + set := z.serverPools[1].sets[0] + body := []byte("normal data") + if _, err := set.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(body), int64(len(body)), "", ""), ObjectOptions{}); err != nil { + t.Fatal(err) + } + var reads atomic.Int64 + for _, pool := range z.serverPools { + for _, set := range pool.sets { + disks, switchDisks := installListingDiskSwitch(set) + wrapped := slices.Clone(disks) + for i := range wrapped { + wrapped[i] = &listingRecoveryDisk{StorageAPI: disks[i], reads: &reads} + } + switchDisks(wrapped) + } + } + writer := z.NewNSLock(bucket, object) + lkctx, err := writer.GetLock(t.Context(), globalOperationTimeout) + if err != nil { + t.Fatal(err) + } + defer writer.Unlock(lkctx) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + out := make(chan metaCacheEntry, 10) + err = z.listMerged(ctx, listPathOptions{Bucket: bucket, Recursive: true, Limit: 10}, out) + if err != nil && err != io.EOF { + t.Fatalf("normal listing waited on writer or failed: %v", err) + } + var names []string + for entry := range out { + names = append(names, entry.name) + if entry.listingSet != nil { + t.Fatal("pending escaped") + } + } + if !slices.Equal(names, []string{object}) || reads.Load() != 0 { + t.Fatalf("normal names=%v additional ReadXL=%d", names, reads.Load()) + } +} + +func TestListingPendingDirectoryCollision(t *testing.T) { + for _, objectFirst := range []bool{false, true} { + t.Run(fmt.Sprint(objectFirst), func(t *testing.T) { + in := []chan metaCacheEntry{make(chan metaCacheEntry, 1), make(chan metaCacheEntry, 1)} + objectIndex := 1 + if objectFirst { + objectIndex = 0 + } + in[objectIndex] <- metaCacheEntry{name: "dir/", listingSet: &erasureObjects{}} + in[1-objectIndex] <- metaCacheEntry{name: "dir/"} + for _, c := range in { + close(c) + } + out := make(chan metaCacheEntry, 2) + calls := 0 + err := mergeEntryChannelsWithResolver(t.Context(), in, out, 1, nil, func(group []*metaCacheEntry) (*metaCacheEntry, error) { + calls++ + if len(group) != 1 || group[0].listingSet == nil { + t.Fatalf("wrong pending group: %+v", group) + } + return &metaCacheEntry{name: "dir/", metadata: []byte{1}}, nil + }) + if err != nil || calls != 1 || len(out) != 1 { + t.Fatalf("err=%v recoveries=%d outputs=%d", err, calls, len(out)) + } + if entry := <-out; entry.isDir() || !entry.isObjectDir() { + t.Fatalf("object became directory: %+v", entry) + } + }) + } +} + +func TestListingRecoveryRejectsUnexpectedSinglePoolSet(t *testing.T) { + sets := []*erasureObjects{{}, {}} + pool := &erasureSets{sets: sets, distributionAlgo: formatErasureVersionV3DistributionAlgoV3} + z := &erasureServerPools{serverPools: []*erasureSets{pool}} + for _, object := range []string{"key", "directory/"} { + target := pool.getHashedSet(encodeDirObject(object)) + wrong := sets[0] + if wrong == target { + wrong = sets[1] + } + entry, err := z.resolveListingGroup(t.Context(), "bucket", []*metaCacheEntry{{name: object, listingSet: wrong}}) + if entry != nil || !isErrReadQuorum(err) { + t.Fatalf("unexpected set became success: %v %v", entry, err) + } + } +} + +func TestListingRecoveryReleasesBeforeOutput(t *testing.T) { + z, bucket, _ := nullQuorumBackend(t) + const object = "object/" + nullQuorumPut(t, z, bucket, object, []byte("data"), ObjectOptions{}) + set := z.serverPools[0].sets[0] + input := make(chan metaCacheEntry, 1) + input <- metaCacheEntry{name: object, listingSet: set} + close(input) + out := make(chan metaCacheEntry) + resolved := make(chan struct{}) + done := make(chan error, 1) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + go func() { + done <- mergeEntryChannelsWithResolver(ctx, []chan metaCacheEntry{input}, out, 1, nil, func(group []*metaCacheEntry) (*metaCacheEntry, error) { + entry, err := z.resolveListingGroup(ctx, bucket, group) + close(resolved) + return entry, err + }) + }() + select { + case <-resolved: + case <-ctx.Done(): + t.Fatal(ctx.Err()) + } + lock := set.NewNSLock(bucket, encodeDirObject(object)) + lkctx, err := lock.GetLock(ctx, globalOperationTimeout) + if err != nil { + t.Fatalf("writer blocked by output backpressure: %v", err) + } + lock.Unlock(lkctx) + entry := <-out + if entry.name != object || !entry.isObjectDir() || entry.isDir() || entry.listingSet != nil { + t.Fatalf("directory object identity lost: %+v", entry) + } + if err := <-done; err != nil { + t.Fatal(err) + } +} + +func TestListingRecoveryTerminalErrorPrecedesLock(t *testing.T) { + for _, n := range []int{1, 2} { + t.Run(fmt.Sprint(n), func(t *testing.T) { + inputs := make([]chan metaCacheEntry, n) + for i := range inputs { + inputs[i] = make(chan metaCacheEntry, 1) + inputs[i] <- metaCacheEntry{name: "key", listingSet: &erasureObjects{}} + close(inputs[i]) + } + out := make(chan metaCacheEntry, 1) + want := errors.New("walk failed") + called := false + err := mergeEntryChannelsWithResolver(t.Context(), inputs, out, 1, func(int) error { return want }, func([]*metaCacheEntry) (*metaCacheEntry, error) { called = true; return nil, nil }) + if !errors.Is(err, want) || called || len(out) != 0 { + t.Fatalf("err=%v recovery=%v output=%d", err, called, len(out)) + } + }) + } +} + +func TestListingPendingCannotBeSerialized(t *testing.T) { + entry := metaCacheEntry{name: "directory/", listingSet: &erasureObjects{}} + if entry.isDir() || !entry.isObjectDir() { + t.Fatal("pending object became a directory") + } + var buf bytes.Buffer + w := newMetacacheWriter(&buf, 0) + if err := w.write(entry); err == nil { + t.Fatal("pending entry serialized") + } + _ = w.Close() + buf.Reset() + w = newMetacacheWriter(&buf, 0) + stream, err := w.stream() + if err != nil { + t.Fatal(err) + } + stream <- entry + close(stream) + if err = w.Close(); err == nil { + t.Fatal("stream serialized pending entry") + } +} + +func TestListingEntryRepresentationCost(t *testing.T) { + type before struct { + _ string // name + _ []byte // metadata + _ *xlMetaV2 // cached + _ bool // reusable + } + t.Logf("metaCacheEntry before=%d after=%d bytes", unsafe.Sizeof(before{}), unsafe.Sizeof(metaCacheEntry{})) +} + +func BenchmarkListingMergeNormal(b *testing.B) { + for _, n := range []int{1, 2} { + for _, resolver := range []bool{false, true} { + b.Run(fmt.Sprintf("inputs=%d/resolver=%v", n, resolver), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + inputs := make([]chan metaCacheEntry, n) + for i := range inputs { + inputs[i] = make(chan metaCacheEntry, 100) + for j := range 100 { + inputs[i] <- metaCacheEntry{name: fmt.Sprintf("key-%03d-%d/", j, i)} + } + close(inputs[i]) + } + out := make(chan metaCacheEntry, n*100) + var resolveGroup func([]*metaCacheEntry) (*metaCacheEntry, error) + if resolver { + resolveGroup = func([]*metaCacheEntry) (*metaCacheEntry, error) { return nil, io.ErrUnexpectedEOF } + } + if err := mergeEntryChannelsWithResolver(context.Background(), inputs, out, 1, nil, resolveGroup); err != nil { + b.Fatal(err) + } + } + }) + } + } +} diff --git a/cmd/metacache-null-quorum_test.go b/cmd/metacache-null-quorum_test.go index fb0b18e5f..d4cf74351 100644 --- a/cmd/metacache-null-quorum_test.go +++ b/cmd/metacache-null-quorum_test.go @@ -29,6 +29,7 @@ import ( "path/filepath" "slices" "sync" + "sync/atomic" "testing" "time" @@ -285,7 +286,7 @@ func TestListObjectsSingleNullQuorumHTTP(t *testing.T) { func TestListObjectsFallsBackToReadableMetadata(t *testing.T) { z, bucket, router := nullQuorumBackend(t) set := z.serverPools[0].sets[0] - disks := set.getDisks() + disks, switchDisks := installListingDiskSwitch(set) const object = "object" oldBody := bytes.Repeat([]byte{'a'}, 8192) newBody := bytes.Repeat([]byte{'b'}, 8192) @@ -317,7 +318,7 @@ func TestListObjectsFallsBackToReadableMetadata(t *testing.T) { wrapped[i] = &failedMetadataWalkDisk{StorageAPI: disk} } } - set.getDisks = func() []StorageAPI { return wrapped } + switchDisks(wrapped) rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1000", "", "", "")) var list ListObjectsV2Response @@ -334,6 +335,49 @@ func TestListObjectsFallsBackToReadableMetadata(t *testing.T) { if get.Code != http.StatusOK || !bytes.Equal(get.Body.Bytes(), newBody) { t.Fatalf("GET lost the readable generation: %d %q", get.Code, get.Body.String()) } + // Walk uses a separate producer but must share the same latest-only pool + // recovery. Versioned walks retain their existing no-fallback behavior. + for _, latestOnly := range []bool{false, true} { + out := make(chan itemOrErr[ObjectInfo], 10) + if err := z.Walk(t.Context(), bucket, "", out, WalkOptions{LatestOnly: latestOnly, AskDisks: "strict"}); err != nil { + t.Fatal(err) + } + var objects []ObjectInfo + for result := range out { + if result.Err != nil { + t.Fatal(result.Err) + } + objects = append(objects, result.Item) + } + if latestOnly { + if len(objects) != 1 || objects[0].Name != object || !objects[0].ModTime.Equal(oldTime.Add(time.Minute)) { + t.Fatalf("Walk latest recovery: %+v", objects) + } + } else if len(objects) != 0 { + t.Fatalf("versioned Walk unexpectedly enabled recovery: %+v", objects) + } + } + failedDisks := slices.Clone(wrapped) + for i, disk := range failedDisks { + failedDisks[i] = &listingReadFailDisk{StorageAPI: disk, object: object, reads: &atomic.Int64{}} + } + switchDisks(failedDisks) + out := make(chan itemOrErr[ObjectInfo], 10) + if err := z.Walk(t.Context(), bucket, "", out, WalkOptions{LatestOnly: true, AskDisks: "strict"}); err != nil { + t.Fatal(err) + } + failed := false + for result := range out { + if result.Err == nil { + t.Fatalf("failed Walk yielded object: %+v", result.Item) + } + if isErrReadQuorum(result.Err) { + failed = true + } + } + if !failed { + t.Fatal("failed Walk recovery did not report quorum error") + } } // The fallback must read under the object read lock. A lock-free read of an @@ -548,7 +592,7 @@ func TestListSplitDirectoryObject(t *testing.T) { } } -func TestListFallbackDoesNotTrustNotFound(t *testing.T) { +func TestListFallbackConfirmsCurrentAbsence(t *testing.T) { z, bucket, _ := nullQuorumBackend(t) set := z.serverPools[0].sets[0] disks := set.getDisks() @@ -582,9 +626,9 @@ func TestListFallbackDoesNotTrustNotFound(t *testing.T) { errs[i] = errDiskNotFound } resolver := metadataResolutionParams{dirQuorum: 8, objQuorum: 8, bucket: bucket, requestedVersions: 1} - entry, err := set.resolveListEntry(t.Context(), bucket, entries, errs, &resolver, z.listingNSLock(set)) - if entry != nil || !isErrReadQuorum(err) { - t.Fatalf("ambiguous not-found: got entry=%v err=%v; want read quorum error", entry, err) + entry, err := z.resolveListingGroup(t.Context(), bucket, []*metaCacheEntry{set.resolveListEntry(entries, errs, &resolver)}) + if entry != nil || err != nil { + t.Fatalf("strict current absence: got entry=%v err=%v; want no entry or error", entry, err) } } diff --git a/cmd/metacache-parts-retry_test.go b/cmd/metacache-parts-retry_test.go new file mode 100644 index 000000000..845c0852f --- /dev/null +++ b/cmd/metacache-parts-retry_test.go @@ -0,0 +1,444 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "slices" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/minio/minio/internal/grid" +) + +// Read real cache objects and use an actual peer RPC, counting only the block +// under test. Gates control publication, never replace quorum or EOF decisions. +type listingCachePartsProbe struct { + state atomic.Pointer[metacache] + rpcFails atomic.Bool + reads atomic.Int64 + mu sync.Mutex + probes []int + checks []time.Time + onProbe func(int) bool + onCheck func(int) + target string + readErr error +} + +func (p *listingCachePartsProbe) setState(o listPathOptions, failed bool) { + m := o.newMetacache() + m.lastUpdate = time.Now() + if failed { + m.status, m.error = scanStateError, "cache producer failed" + } else { + m.status = scanStateStarted + } + p.state.Store(&m) +} + +func (p *listingCachePartsProbe) snapshot() ([]int, []time.Time) { + p.mu.Lock() + defer p.mu.Unlock() + return slices.Clone(p.probes), slices.Clone(p.checks) +} + +type listingCachePartsDisk struct { + StorageAPI + index int + probe *listingCachePartsProbe +} + +func (d *listingCachePartsDisk) ReadXL(ctx context.Context, bucket, object string, readData bool) (RawFileInfo, error) { + if bucket == minioMetaBucket && object == d.probe.target { + d.probe.reads.Add(1) + if d.probe.readErr != nil { + return RawFileInfo{}, d.probe.readErr + } + } + return d.StorageAPI.ReadXL(ctx, bucket, object, readData) +} + +func (d *listingCachePartsDisk) ReadVersion(ctx context.Context, origVolume, volume, path, versionID string, opts ReadOptions) (FileInfo, error) { + if volume == minioMetaBucket && path == d.probe.target { + d.probe.mu.Lock() + d.probe.probes = append(d.probe.probes, d.index) + d.probe.mu.Unlock() + if d.probe.onProbe != nil && d.probe.onProbe(d.index) { + return FileInfo{}, errFileNotFound + } + } + return d.StorageAPI.ReadVersion(ctx, origVolume, volume, path, versionID, opts) +} + +func listingCachePartsBackend(t *testing.T, part int) (*erasureObjects, listPathOptions, *listingCachePartsProbe, func([]StorageAPI)) { + t.Helper() + z, bucket, _ := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + o := listPathOptions{ID: mustGetUUID(), Bucket: bucket, Limit: 100, Recursive: true, IncludeDirectories: true} + p := &listingCachePartsProbe{target: o.objectPath(part)} + p.setState(o, false) + disks, switchDisks := installListingDiskSwitch(set) + wrapped := slices.Clone(disks) + for i, disk := range disks { + wrapped[i] = &listingCachePartsDisk{StorageAPI: disk, index: i, probe: p} + } + switchDisks(wrapped) + + tg, err := grid.SetupTestGrid(2) + if err != nil { + t.Fatal(err) + } + previous := globalNotificationSys + t.Cleanup(func() { + globalNotificationSys = previous + tg.Cleanup() + }) + if err := getMetacacheListingRPC.Register(tg.Managers[1], func(got *listPathOptions) (*metacache, *grid.RemoteErr) { + if got.ID != o.ID || got.Create { + return nil, grid.NewRemoteErr(errors.New("invalid cache state lookup")) + } + p.mu.Lock() + p.checks = append(p.checks, time.Now()) + n := len(p.checks) + p.mu.Unlock() + if p.onCheck != nil { + p.onCheck(n) + } + if p.rpcFails.Load() { + return nil, grid.NewRemoteErr(errors.New("cache state RPC failed")) + } + m := *p.state.Load() + return &m, nil + }); err != nil { + t.Fatal(err) + } + client := &peerRESTClient{gridConn: func() *grid.Connection { return tg.Managers[0].Connection(tg.Hosts[1]) }} + globalNotificationSys = &NotificationSys{peerClients: []*peerRESTClient{client}, allPeerClients: []*peerRESTClient{client}} + return set, o, p, switchDisks +} + +func putListingCachePart(t *testing.T, set *erasureObjects, o listPathOptions, part int, name string, eos bool) { + t.Helper() + var buf bytes.Buffer + w := newMetacacheWriter(&buf, 0) + if err := w.write(metaCacheEntry{name: name}); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + block := metacacheBlock{n: part, First: name, Last: name, EOS: eos} + _, err := set.putMetacacheObject(t.Context(), o.objectPath(part), mustGetPutObjReader(t, bytes.NewReader(buf.Bytes()), int64(buf.Len()), "", ""), ObjectOptions{UserDefined: block.headerKV()}) + if err != nil { + t.Fatal(err) + } +} + +func TestStreamMetadataPartsHealthyNoStateRPC(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 1) + putListingCachePart(t, set, o, 0, "a/", false) + putListingCachePart(t, set, o, 1, "b/", true) + entries, err := set.streamMetadataParts(t.Context(), o) + if !errors.Is(err, io.EOF) || entries.len() != 2 || entries.o[0].name != "a/" || entries.o[1].name != "b/" { + t.Fatalf("healthy two-block read: entries=%v err=%v", entries.o, err) + } + probes, checks := p.snapshot() + if len(probes) != 0 || len(checks) != 0 { + t.Fatalf("healthy read added probes=%v state RPCs=%d", probes, len(checks)) + } + t.Logf("healthy persisted blocks: state RPCs=0 retry probes=0 target ReadXL=%d", p.reads.Load()) +} + +func TestStreamMetadataPartsTerminalError(t *testing.T) { + for _, part := range []int{0, 1} { + t.Run(fmt.Sprintf("part=%d", part), func(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, part) + if part > 0 { + putListingCachePart(t, set, o, 0, "a/", false) + } + p.setState(o, true) + start := time.Now() + entries, err := set.streamMetadataParts(t.Context(), o) + elapsed := time.Since(start) + if err == nil || errors.Is(err, io.EOF) || !strings.Contains(err.Error(), "cache producer failed") || entries.len() != part { + t.Fatalf("failed cache became successful: entries=%v err=%v", entries.o, err) + } + probes, checks := p.snapshot() + if len(checks) != 1 || len(probes) != 0 || elapsed > time.Second { + t.Fatalf("terminal error waited for disk retries: elapsed=%v state RPCs=%d probes=%v", elapsed, len(checks), probes) + } + t.Logf("terminal part=%d elapsed=%v state RPCs=%d probes=%d target ReadXL=%d", part, elapsed, len(checks), len(probes), p.reads.Load()) + }) + } +} + +func TestStreamMetadataPartsLateTerminalError(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 1) + putListingCachePart(t, set, o, 0, "a/", false) + p.onProbe = func(int) bool { + p.setState(o, true) + return false + } + start := time.Now() + entries, err := set.streamMetadataParts(t.Context(), o) + elapsed := time.Since(start) + probes, checks := p.snapshot() + if err == nil || errors.Is(err, io.EOF) || !strings.Contains(err.Error(), "cache producer failed") || entries.len() != 1 { + t.Fatalf("late producer error lost: entries=%v err=%v", entries.o, err) + } + if elapsed > time.Second || len(probes) != 2 || !slices.Equal(probes, []int{0, 1}) || len(checks) != 2 { + t.Fatalf("late error retry work: elapsed=%v probes=%v state RPCs=%d", elapsed, probes, len(checks)) + } + if checks[1].Sub(checks[0]) < 250*time.Millisecond || p.reads.Load() > 16 { + t.Fatalf("unbounded retry: state interval=%v target ReadXL=%d", checks[1].Sub(checks[0]), p.reads.Load()) + } + t.Logf("late terminal elapsed=%v state RPCs=%d probes=%v target ReadXL=%d", elapsed, len(checks), probes, p.reads.Load()) +} + +type listingCachePartsResult struct { + entries metaCacheEntriesSorted + err error +} + +func startListingCacheParts(t *testing.T, set *erasureObjects, o listPathOptions) (<-chan listingCachePartsResult, context.CancelFunc) { + t.Helper() + ctx, cancel := context.WithCancel(t.Context()) + done := make(chan listingCachePartsResult, 1) + joined := make(chan struct{}) + go func() { + defer close(joined) + entries, err := set.streamMetadataParts(ctx, o) + done <- listingCachePartsResult{entries: entries, err: err} + }() + t.Cleanup(func() { + cancel() + select { + case <-joined: + case <-time.After(8 * time.Second): + t.Error("cache continuation worker did not finish") + } + }) + return done, cancel +} + +func receiveListingCacheParts(t *testing.T, done <-chan listingCachePartsResult) listingCachePartsResult { + t.Helper() + select { + case result := <-done: + return result + case <-time.After(8 * time.Second): + t.Fatal("cache continuation did not finish") + return listingCachePartsResult{} + } +} + +func TestStreamMetadataPartsGeneratingThenPublished(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 1) + putListingCachePart(t, set, o, 0, "a/", false) + checked := make(chan struct{}) + p.onCheck = func(n int) { + if n == 1 { + close(checked) + } + } + // The first selected replica is absent even after the producer publishes + // the block elsewhere. Rotation must reach a readable replica. + p.onProbe = func(index int) bool { return index == 0 } + done, _ := startListingCacheParts(t, set, o) + select { + case <-checked: + case <-time.After(5 * time.Second): + t.Fatal("missing block did not check producer state") + } + select { + case result := <-done: + t.Fatalf("still-generating cache finished early: %+v", result) + case <-time.After(100 * time.Millisecond): + } + putListingCachePart(t, set, o, 1, "b/", true) + result := receiveListingCacheParts(t, done) + if !errors.Is(result.err, io.EOF) || result.entries.len() != 2 || result.entries.o[0].name != "a/" || result.entries.o[1].name != "b/" { + t.Fatalf("published second block lost: entries=%v err=%v", result.entries.o, result.err) + } + probes, checks := p.snapshot() + if len(probes) < 2 || probes[0] != 0 || probes[1] != 1 { + t.Fatalf("retry did not rotate: probes=%v checks=%d", probes, len(checks)) + } + t.Logf("later block recovered: state RPCs=%d probes=%v target ReadXL=%d", len(checks), probes, p.reads.Load()) +} + +func TestStreamMetadataPartsRPCFailure(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 1) + putListingCachePart(t, set, o, 0, "a/", false) + p.rpcFails.Store(true) + entries, err := set.streamMetadataParts(t.Context(), o) + if err == nil || errors.Is(err, io.EOF) || !strings.Contains(err.Error(), "cache state RPC failed") || entries.len() != 1 { + t.Fatalf("RPC failure became successful EOF: entries=%v err=%v", entries.o, err) + } + probes, checks := p.snapshot() + if len(checks) != 1 || len(probes) != 0 { + t.Fatalf("state RPC failure continued retrying: probes=%v checks=%d", probes, len(checks)) + } +} + +func TestStreamMetadataPartsMissingIndex(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 0) + putListingCachePart(t, set, o, 0, "a/", false) + o.Marker = "b/" + p.setState(o, true) + entries, err := set.streamMetadataParts(t.Context(), o) + if err == nil || errors.Is(err, io.EOF) || !strings.Contains(err.Error(), "cache producer failed") || entries.len() != 0 { + t.Fatalf("unpublished index became successful EOF: entries=%v err=%v", entries.o, err) + } + probes, checks := p.snapshot() + if len(checks) != 1 || len(probes) != 0 { + t.Fatalf("missing index delayed terminal state: probes=%v checks=%d", probes, len(checks)) + } +} + +func TestStreamMetadataPartsOtherErrorRetriesOnce(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 0) + p.readErr = errVolumeNotFound + entries, err := set.streamMetadataParts(t.Context(), o) + if err == nil || errors.Is(err, io.EOF) || !strings.Contains(err.Error(), "reading first part metadata:") || entries.len() != 0 { + t.Fatalf("unexpected metadata error result: entries=%v err=%v", entries.o, err) + } + probes, checks := p.snapshot() + if p.reads.Load() != 32 || len(probes) != 0 || len(checks) != 1 { + t.Fatalf("other metadata error did not get exactly one full retry: ReadXL=%d probes=%v checks=%d", p.reads.Load(), probes, len(checks)) + } +} + +func TestStreamMetadataPartsMissingBlockWorkBound(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 0) + checked := make(chan struct{}) + p.onCheck = func(n int) { + if n == 3 { + close(checked) + } + } + start := time.Now() + done, cancel := startListingCacheParts(t, set, o) + select { + case <-checked: + case <-time.After(5 * time.Second): + t.Fatal("missing cache stopped checking producer state") + } + cancel() + result := receiveListingCacheParts(t, done) + probes, checks := p.snapshot() + if !errors.Is(result.err, context.Canceled) || result.entries.len() != 0 { + t.Fatalf("generating cache became successful EOF: %+v", result) + } + if len(probes) != 3 || !slices.Equal(probes, []int{0, 1, 2}) || p.reads.Load() != 16 || len(checks) != 3 { + t.Fatalf("missing block exceeded work bound: probes=%v target ReadXL=%d checks=%d", probes, p.reads.Load(), len(checks)) + } + for i := 1; i < len(checks); i++ { + // Receive timestamps allow a small amount of peer scheduling jitter. + if checks[i].Sub(checks[i-1]) < 240*time.Millisecond { + t.Fatalf("cache state queried too often: %v", checks) + } + } + t.Logf("still missing for %v: state RPCs=%d probes=%v target ReadXL=%d", time.Since(start), len(checks), probes, p.reads.Load()) +} + +func TestStreamMetadataPartsCancelRetryWait(t *testing.T) { + set, o, p, _ := listingCachePartsBackend(t, 1) + putListingCachePart(t, set, o, 0, "a/", false) + probed := make(chan struct{}) + p.onProbe = func(index int) bool { + if index == 1 { + close(probed) + } + return false + } + done, cancel := startListingCacheParts(t, set, o) + select { + case <-probed: + case <-time.After(5 * time.Second): + t.Fatal("retry probe did not run") + } + // Let the probe return and enter the regular 250 ms wait before canceling. + time.Sleep(20 * time.Millisecond) + start := time.Now() + cancel() + result := receiveListingCacheParts(t, done) + elapsed := time.Since(start) + if !errors.Is(result.err, context.Canceled) || elapsed > 150*time.Millisecond { + t.Fatalf("retry wait ignored cancellation: elapsed=%v err=%v", elapsed, result.err) + } + t.Logf("cancel during retry wait: elapsed=%v", elapsed) +} + +func TestStreamMetadataPartsChangedDisks(t *testing.T) { + for _, empty := range []bool{false, true} { + t.Run(fmt.Sprintf("empty=%v", empty), func(t *testing.T) { + testStreamMetadataPartsChangedDisks(t, empty) + }) + } +} + +func testStreamMetadataPartsChangedDisks(t *testing.T, empty bool) { + set, o, p, switchDisks := listingCachePartsBackend(t, 1) + putListingCachePart(t, set, o, 0, "a/", false) + disks := set.getDisks() + probeDone, restore := make(chan struct{}), make(chan struct{}) + p.onProbe = func(index int) bool { + if index == 0 { + // A later probe must safely skip an empty/offline disk snapshot. + if empty { + switchDisks([]StorageAPI{}) + } else { + switchDisks([]StorageAPI{nil}) + } + close(probeDone) + return true + } + return false + } + p.onCheck = func(n int) { + if n == 2 { + close(restore) + } + } + done, _ := startListingCacheParts(t, set, o) + select { + case <-probeDone: + case <-time.After(5 * time.Second): + t.Fatal("initial retry probe did not run") + } + select { + case <-restore: + case <-time.After(5 * time.Second): + t.Fatal("empty disk snapshot prevented cache state checks") + } + switchDisks(disks) + putListingCachePart(t, set, o, 1, "b/", true) + result := receiveListingCacheParts(t, done) + if !errors.Is(result.err, io.EOF) || result.entries.len() != 2 { + t.Fatalf("changed disks did not recover: entries=%v err=%v", result.entries.o, result.err) + } +} diff --git a/cmd/metacache-server-pool.go b/cmd/metacache-server-pool.go index 6510aaeba..2889e9a96 100644 --- a/cmd/metacache-server-pool.go +++ b/cmd/metacache-server-pool.go @@ -264,14 +264,174 @@ func (z *erasureServerPools) listPathShortcutEOF(ctx context.Context, bucket str return io.EOF } -// listingNSLock chooses the namespace used by regular object writes. -// Multiple pools serialize at the pools layer; single-pool PUT and GET use -// the destination set. This keeps the existing single-pool lock routing. -func (z *erasureServerPools) listingNSLock(er *erasureObjects) func(string, ...string) RWLocker { - if z.SinglePool() { - return er.NewNSLock +// listingRLock covers the lock domains used by normal same-key mutations. +// Multi-pool writes use the pools lock. In a single pool, DELETE/COPY use the +// first set while PUT and metadata updates use the destination set, so recovery +// takes both in that order. No writer routing or wire protocol changes here. +func (z *erasureServerPools) listingRLock(ctx context.Context, bucket, object string, source *erasureObjects) (LockContext, func(), error) { + firstSet := z.serverPools[0].sets[0] + first := z.NewNSLock(bucket, object) + firstCtx, err := first.GetRLock(ctx, globalListingOperationTimeout) + if err != nil { + return LockContext{}, nil, err } - return z.NewNSLock + if !z.SinglePool() || source == firstSet || (!source.nsMutex.isDistErasure && source.nsMutex == firstSet.nsMutex) { + return firstCtx, func() { first.RUnlock(firstCtx) }, nil + } + // Shared nsMutex pointers do not prove shared distributed locker sets. + second := source.NewNSLock(bucket, object) + secondCtx, err := second.GetRLock(firstCtx.Context(), globalListingOperationTimeout) + if err != nil { + first.RUnlock(firstCtx) + return LockContext{}, nil, err + } + return secondCtx, func() { + second.RUnlock(secondCtx) + first.RUnlock(firstCtx) + }, nil +} + +// resolveListingGroup recovers a complete same-name group, never a private +// per-set contribution. A strictly absent source requires current reads from +// every pool, since another pool's earlier walk may predate a migration copy. +func (z *erasureServerPools) resolveListingGroup(ctx context.Context, bucket string, group []*metaCacheEntry) (*metaCacheEntry, error) { + var source *erasureObjects + for _, entry := range group { + if entry.listingSet != nil { + source = entry.listingSet + break + } + } + if source == nil { + return nil, errInvalidArgument + } + // Recovery consumes this group. Return reusable walk buffers only after + // their contents have been copied into the newly encoded result. + defer func() { + for _, entry := range group { + if entry.reusable { + metaDataPoolPut(entry.metadata) + } + } + }() + name := group[0].name + object := encodeDirObject(name) + if z.SinglePool() { + target := z.serverPools[0].getHashedSet(object) + for _, entry := range group { + if entry.listingSet != nil && entry.listingSet != target { + // Unexpected placement is not evidence of absence at the + // current write target. Do not extend recovery to arbitrary sets. + return nil, listingReadQuorumError(bucket, object) + } + } + } + type readResult struct { + fi FileInfo + absent bool + } + reads := make(map[*erasureObjects]readResult) + currentOnly := false + var current *FileInfo + err := func() error { + lockCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + lkctx, unlock, err := z.listingRLock(lockCtx, bucket, object, source) + if err != nil { + return err + } + defer unlock() + read := func(set *erasureObjects) (readResult, error) { + if result, ok := reads[set]; ok { + return result, nil + } + fi, absent, err := set.readListingFileInfo(lkctx.Context(), bucket, object) + result := readResult{fi: fi, absent: absent} + if err == nil { + reads[set] = result + } + return result, err + } + for _, entry := range group { + if entry.listingSet == nil { + continue + } + result, err := read(entry.listingSet) + if err != nil { + return err + } + currentOnly = currentOnly || result.absent + } + if !currentOnly { + return lkctx.Context().Err() + } + // Migration publishes the replacement through the pooled write lock + // before source cleanup. While this read lock is held, new publication + // cannot pass these reads; any concurrent cleanup already has a copy. + // Unknown pools still fail rather than being counted as absent. + for _, pool := range z.serverPools { + result, err := read(pool.getHashedSet(object)) + if err != nil { + return err + } + if result.absent { + continue + } + // Match latest GET: newest ModTime, then lowest pool index. + if current == nil || result.fi.ModTime.After(current.ModTime) { + fi := result.fi + current = &fi + } + } + return lkctx.Context().Err() + }() + if err != nil { + return nil, err + } + // All FileInfo values above are owned decoded results. Encoding and merge + // allocation happen after RUnlock, as do all stream sends and cache RPCs. + encode := func(fi FileInfo) (*metaCacheEntry, error) { + xl := &xlMetaV2{} + if err := xl.AddVersion(fi); err != nil { + return nil, listingReadQuorumError(bucket, object) + } + entry := &metaCacheEntry{name: name, cached: xl, reusable: true} + var err error + entry.metadata, err = xl.AppendTo(metaDataPoolGet()) + return entry, err + } + if currentOnly { + if current == nil { + return nil, nil + } + // Replace the whole group: no stale live copy or marker can override + // this current global decision. + return encode(*current) + } + versions := make([][]xlMetaV2ShallowVersion, 0, len(group)) + for _, entry := range group { + if entry.listingSet != nil { + fi := reads[entry.listingSet].fi + xl := &xlMetaV2{} + if err := xl.AddVersion(fi); err != nil { + return nil, listingReadQuorumError(bucket, object) + } + versions = append(versions, xl.versions) + continue + } + xl, err := entry.xlmeta() + if err != nil { + return nil, listingReadQuorumError(bucket, object) + } + versions = append(versions, xl.versions) + } + // Preserve the existing cross-pool merge rule for recovered live metadata + // and delete markers when no source was strictly absent. + xl := &xlMetaV2{versions: mergeXLV2Versions(1, true, 0, versions...)} + // Do not retain shallow versions referencing reusable input buffers. + resolved := &metaCacheEntry{name: name, reusable: true} + resolved.metadata, err = xl.AppendTo(metaDataPoolGet()) + return resolved, err } // listMerged will list across all sets and return a merged results stream. @@ -293,7 +453,7 @@ func (z *erasureServerPools) listMerged(ctx context.Context, o listPathOptions, inputs = append(inputs, innerResults) go func(i int, set *erasureObjects) { defer wg.Done() - err := set.listPath(listCtx, o, innerResults, z.listingNSLock(set)) + err := set.listPath(listCtx, o, innerResults) mu.Lock() if err == nil { allAtEOF = false @@ -311,7 +471,7 @@ func (z *erasureServerPools) listMerged(ctx context.Context, o listPathOptions, // Gather results to a single channel. // Quorum is one since we are merging across sets. - err := mergeEntryChannelsWithErrors(ctx, inputs, results, 1, func(i int) error { + err := mergeEntryChannelsWithResolver(ctx, inputs, results, 1, func(i int) error { mu.Lock() err := errs[i] mu.Unlock() @@ -320,6 +480,8 @@ func (z *erasureServerPools) listMerged(ctx context.Context, o listPathOptions, return nil } return err + }, func(group []*metaCacheEntry) (*metaCacheEntry, error) { + return z.resolveListingGroup(ctx, o.Bucket, group) }) cancelList() @@ -440,14 +602,20 @@ func (z *erasureServerPools) listAndSave(ctx context.Context, o *listPathOptions // Publish the typed terminal result before either consumer observes EOF. // mergeEntryChannels closes inCh before listMerged has inspected set errors. listingDone := make(chan struct{}) + var cacheErrorDone chan struct{} var listingErr error go func(o listPathOptions) { listingErr = z.listMerged(listCtx, o, inCh) - if listingErr != nil && !errors.Is(listingErr, io.EOF) { - meta.setErr(listingErr.Error()) - cancel() + failed := listingErr != nil && !errors.Is(listingErr, io.EOF) + if failed { + cacheErrorDone = make(chan struct{}) } close(listingDone) + if failed { + meta.setErr(listingErr.Error()) + cancel() + close(cacheErrorDone) + } o.debugln("listAndSave: listing", o.ID, "finished with ", listingErr) }(*o) @@ -478,12 +646,16 @@ func (z *erasureServerPools) listAndSave(ctx context.Context, o *listPathOptions entry.reusable = returned saveCh <- entry } - // Do not publish an end-of-stream cache block or a final response - // before the scan has recorded its terminal error. + // The typed scan result is enough to finish the HTTP response. <-listingDone if !returned { xioutil.SafeClose(outCh) } + // Closing saveCh writes an end-of-stream cache block. On failure, + // wait until the error is recorded and the writer is canceled. + if cacheErrorDone != nil { + <-cacheErrorDone + } xioutil.SafeClose(saveCh) }() diff --git a/cmd/metacache-set.go b/cmd/metacache-set.go index b7fa58580..50af060ea 100644 --- a/cmd/metacache-set.go +++ b/cmd/metacache-set.go @@ -422,63 +422,89 @@ func (er *erasureObjects) streamMetadataParts(ctx context.Context, o listPathOpt retryDelay = 50 * time.Millisecond retryDelay250 = 250 * time.Millisecond ) + var lastStateCheck time.Time + retryWait := func() error { + if contextCanceled(ctx) { + return ctx.Err() + } + // A failed producer cannot publish another block. Check on the first + // failure, then at most once per retry interval while it is still running. + if time.Since(lastStateCheck) >= retryDelay250 { + lastStateCheck = time.Now() + if err := o.checkMetacacheState(ctx, rpc); err != nil { + return fmt.Errorf("remote listing canceled: %w", err) + } + } + retries++ + delay := retryDelay250 + if retries == 1 { + delay = retryDelay + } + timer := time.NewTimer(delay) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } + } + // Probe one online disk per retry, rotating so a missing replica cannot + // pin the reader to one disk. A miss only delays the next probe: it never + // establishes absence or replaces the quorum read after a probe succeeds. + // This bounds missing-block I/O without sleeping once for every disk. + nextProbeDisk := 0 + probePart := func(part int) bool { + disks := er.getDisks() + for range disks { + if contextCanceled(ctx) { + return false + } + i := nextProbeDisk % len(disks) + nextProbeDisk = i + 1 + disk := disks[i] + if disk == nil || !disk.IsOnline() { + continue + } + _, err := disk.ReadVersion(ctx, "", minioMetaBucket, o.objectPath(part), "", ReadOptions{}) + return err == nil + } + return false + } + retryOtherError := false for { if contextCanceled(ctx) { return entries, ctx.Err() } - // If many failures, check the cache state. - if retries > 10 { - err := o.checkMetacacheState(ctx, rpc) - if err != nil { - return entries, fmt.Errorf("remote listing canceled: %w", err) - } - retries = 1 - } - // All operations are performed without locks, so we must be careful and allow for failures. - // Read metadata associated with the object from a disk. - if retries > 0 { - for _, disk := range er.getDisks() { - if disk == nil { - continue - } - if !disk.IsOnline() { - continue - } - _, err := disk.ReadVersion(ctx, "", minioMetaBucket, - o.objectPath(0), "", ReadOptions{}) - if err != nil { - time.Sleep(retryDelay250) - retries++ - continue - } - break - } - } - retryWait := func() { - retries++ - if retries == 1 { - time.Sleep(retryDelay) - } else { - time.Sleep(retryDelay250) + if retries > 0 && !retryOtherError && !probePart(0) { + if err := retryWait(); err != nil { + return entries, err } + continue } // Load first part metadata... // Read metadata associated with the object from all disks. + retryOtherError = false fi, metaArr, onlineDisks, err := er.getObjectFileInfo(ctx, minioMetaBucket, o.objectPath(0), ObjectOptions{}, true) if err != nil { switch toObjectErr(err, minioMetaBucket, o.objectPath(0)).(type) { case ObjectNotFound, InsufficientReadQuorum: - retryWait() + if err := retryWait(); err != nil { + return entries, err + } continue } // Allow one fast retry for other errors. if retries > 0 { return entries, fmt.Errorf("reading first part metadata: %v", err) } - retryWait() + retryOtherError = true + if err := retryWait(); err != nil { + return entries, err + } continue } @@ -486,14 +512,9 @@ func (er *erasureObjects) streamMetadataParts(ctx context.Context, o listPathOpt switch { case err == nil: case errors.Is(err, io.ErrUnexpectedEOF): - if retries == 10 { - err := o.checkMetacacheState(ctx, rpc) - if err != nil { - return entries, fmt.Errorf("remote listing canceled: %w", err) - } - retries = -1 + if err := retryWait(); err != nil { + return entries, err } - retryWait() continue case errors.Is(err, io.EOF): return entries, io.EOF @@ -507,39 +528,19 @@ func (er *erasureObjects) streamMetadataParts(ctx context.Context, o listPathOpt } if partN != loadedPart { - if retries > 10 { - err := o.checkMetacacheState(ctx, rpc) - if err != nil { - return entries, fmt.Errorf("waiting for next part %d: %w", partN, err) - } - retries = 1 - } - - if retries > 0 { - // Load from one disk only - for _, disk := range er.getDisks() { - if disk == nil { - continue - } - if !disk.IsOnline() { - continue - } - _, err := disk.ReadVersion(ctx, "", minioMetaBucket, - o.objectPath(partN), "", ReadOptions{}) - if err != nil { - time.Sleep(retryDelay250) - retries++ - continue - } - break + if retries > 0 && !probePart(partN) { + if err := retryWait(); err != nil { + return entries, err } + continue } // Load partN metadata... fi, metaArr, onlineDisks, err = er.getObjectFileInfo(ctx, minioMetaBucket, o.objectPath(partN), ObjectOptions{}, true) if err != nil { - time.Sleep(retryDelay250) - retries++ + if err := retryWait(); err != nil { + return entries, err + } continue } loadedPart = partN @@ -575,13 +576,15 @@ func (er *erasureObjects) streamMetadataParts(ctx context.Context, o listPathOpt if err != nil && !errors.Is(err, io.EOF) { switch toObjectErr(err, minioMetaBucket, o.objectPath(partN)).(type) { case ObjectNotFound: - retries++ - time.Sleep(retryDelay250) + if err := retryWait(); err != nil { + return entries, err + } continue case InsufficientReadQuorum: - retries++ loadedPart = -1 - time.Sleep(retryDelay250) + if err := retryWait(); err != nil { + return entries, err + } continue default: internalLogIf(ctx, err) @@ -699,18 +702,16 @@ func getQuorumDisks(disks []StorageAPI, infos []DiskInfo, readQuorum int) (newDi return newDisks } -// resolveListEntry resolves walk snapshots and, for latest-only listings, -// falls back to the regular object read path when those snapshots disagree. -// -// Only unresolved entries pay for the additional namespace read lock and -// metadata read. The caller supplies the lock domain used by object writes. -func (er *erasureObjects) resolveListEntry(ctx context.Context, bucket string, entries metaCacheEntries, errs []error, resolver *metadataResolutionParams, newNSLock func(string, ...string) RWLocker) (*metaCacheEntry, error) { +// resolveListEntry keeps the existing walk resolution and fallback eligibility. +// Eligible latest-only disagreements are resolved at the pool merger, where a +// current absence can replace the entire same-name group, including stale walks. +func (er *erasureObjects) resolveListEntry(entries metaCacheEntries, errs []error, resolver *metadataResolutionParams) *metaCacheEntry { entry, ok := entries.resolve(resolver) if ok { - return entry, nil + return entry } if resolver.requestedVersions != 1 { - return nil, nil + return nil } valid := 0 for i := range entries { @@ -731,55 +732,15 @@ func (er *erasureObjects) resolveListEntry(ctx context.Context, bucket string, e } } if entry == nil || valid < resolver.objQuorum { - return nil, nil + return nil } - readQuorumErr := func() error { - return InsufficientReadQuorum{ - Bucket: bucket, - Object: entry.name, - Err: errErasureReadQuorum, - Type: RQInconsistentMeta, - } - } - // Read under the object read lock, exactly as the GET path does. A - // lock-free read of an object being overwritten stably reports a live - // object as absent, so an unlocked fallback only trades one silent - // omission for another. - object := encodeDirObject(entry.name) - lock := newNSLock(bucket, object) - lockCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - lkctx, err := lock.GetRLock(lockCtx, globalOperationTimeout) - if err != nil { - return nil, err - } - defer lock.RUnlock(lkctx) - - fi, _, _, err := er.getObjectFileInfo(lkctx.Context(), bucket, object, ObjectOptions{}, false) - if err != nil { - // getObjectFileInfo maps inconsistent metadata to not-found, so the - // walk evidence means not-found is still undecidable here. - if isErrObjectNotFound(err) || isErrVersionNotFound(err) { - return nil, readQuorumErr() - } - return nil, err - } - // Preserve delete markers until the cross-pool merge has discarded any - // older live copy. The normal listing filter removes the merged marker. - xl := &xlMetaV2{} - if err = xl.AddVersion(fi); err != nil { - return nil, readQuorumErr() - } - resolved := &metaCacheEntry{name: entry.name, cached: xl, reusable: true} - resolved.metadata, err = xl.AppendTo(metaDataPoolGet()) - if err != nil { - return nil, err - } - return resolved, nil + // This entry exists only in the internal set-to-pool stream. Do not retain + // an arbitrary walk snapshot or encode the pending state as metadata. + return &metaCacheEntry{name: entry.name, listingSet: er} } // Will return io.EOF if continuing would not yield more results. -func (er *erasureObjects) listPath(ctx context.Context, o listPathOptions, results chan<- metaCacheEntry, newNSLock func(string, ...string) RWLocker) (err error) { +func (er *erasureObjects) listPath(ctx context.Context, o listPathOptions, results chan<- metaCacheEntry) (err error) { // The caller records the terminal error before closing results. o.debugf(color.Green("listPath:")+" with options: %#v", o) @@ -854,19 +815,15 @@ func (er *erasureObjects) listPath(ctx context.Context, o listPathOptions, resul case results <- entry: } }, - partial: func(entries metaCacheEntries, errs []error) error { + partial: func(entries metaCacheEntries, errs []error) { // Results Disagree :-( - entry, err := er.resolveListEntry(ctx, o.Bucket, entries, errs, &resolver, newNSLock) - if err != nil { - return err - } + entry := er.resolveListEntry(entries, errs, &resolver) if entry != nil { select { case <-ctxDone: case results <- *entry: } } - return nil }, }) } @@ -891,7 +848,9 @@ func (m *metaCacheRPC) setErr(err string) { // An error is already set. return } - meta, _ = m.o.updateMetacacheListing(meta, m.rpc) + if updated, err := m.o.updateMetacacheListing(meta, m.rpc); err == nil { + meta = updated + } *m.meta = meta } @@ -1062,7 +1021,7 @@ type listPathRawOptions struct { // partial will be called when there is disagreement between disks. // if disk did not return any result, but also haven't errored // the entry will be empty and errs will - partial func(entries metaCacheEntries, errs []error) error + partial func(entries metaCacheEntries, errs []error) // finished will be called when all streams have finished and // more than one disk returned an error. @@ -1273,9 +1232,7 @@ func listPathRaw(ctx context.Context, opts listPathRawOptions) (err error) { continue } if opts.partial != nil { - if err := opts.partial(topEntries, errs); err != nil { - return err - } + opts.partial(topEntries, errs) } // Skip the inputs we used. for i, r := range readers { diff --git a/cmd/metacache-stream.go b/cmd/metacache-stream.go index cf61895fe..52ae6de22 100644 --- a/cmd/metacache-stream.go +++ b/cmd/metacache-stream.go @@ -126,6 +126,9 @@ func (w *metacacheWriter) write(objs ...metaCacheEntry) error { } } for _, o := range objs { + if o.listingSet != nil { + return errors.New("metacacheWriter: unresolved listing entry") + } if len(o.name) == 0 { return errors.New("metacacheWriter: no name provided") } @@ -170,6 +173,9 @@ func (w *metacacheWriter) stream() (chan<- metaCacheEntry, error) { go func() { defer w.streamWg.Done() for o := range objs { + if o.listingSet != nil && w.streamErr == nil { + w.streamErr = errors.New("metacacheWriter: unresolved listing entry") + } if len(o.name) == 0 || w.streamErr != nil { continue } diff --git a/internal/dsync/drwmutex-retry_test.go b/internal/dsync/drwmutex-retry_test.go new file mode 100644 index 000000000..806197d73 --- /dev/null +++ b/internal/dsync/drwmutex-retry_test.go @@ -0,0 +1,218 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package dsync + +import ( + "context" + "math/rand" + "sync/atomic" + "testing" + "testing/synctest" + "time" +) + +// retryTestLocker deliberately leaves unexpected methods unimplemented. +// These tests exercise the real DRWMutex retry and cleanup code without HTTP. +type retryTestLocker struct { + NetLocker + acquire func(context.Context, LockArgs) (bool, error) + release func(context.Context, LockArgs) (bool, error) +} + +func (l retryTestLocker) Lock(ctx context.Context, args LockArgs) (bool, error) { + return l.acquire(ctx, args) +} + +func (l retryTestLocker) RLock(ctx context.Context, args LockArgs) (bool, error) { + return l.acquire(ctx, args) +} + +func (l retryTestLocker) Unlock(ctx context.Context, args LockArgs) (bool, error) { + return l.release(ctx, args) +} + +func (l retryTestLocker) RUnlock(ctx context.Context, args LockArgs) (bool, error) { + return l.release(ctx, args) +} + +func (l retryTestLocker) String() string { return "retry-test-locker" } + +func newRetryTestMutex(lockers []NetLocker) *DRWMutex { + dm := NewDRWMutex(&Dsync{ + GetLockers: func() ([]NetLocker, string) { return lockers, "retry-test-owner" }, + Timeouts: DefaultTimeouts, + }, "retry-test-resource") + dm.rng = rand.New(&lockedRandSource{src: rand.NewSource(1)}) + return dm +} + +func retryTestAcquire(ctx context.Context, dm *DRWMutex, read bool, opts Options) bool { + if read { + return dm.GetRLock(ctx, nil, "retry-test-uid", "retry-test", opts) + } + return dm.GetLock(ctx, nil, "retry-test-uid", "retry-test", opts) +} + +func TestDRWMutexRetryCancellation(t *testing.T) { + for _, read := range []bool{false, true} { + for _, opts := range []Options{ + {Timeout: time.Hour}, + {Timeout: time.Hour, RetryInterval: time.Hour}, + } { + synctest.Test(t, func(t *testing.T) { + var calls atomic.Int64 + lockers := make([]NetLocker, 4) + for i := range lockers { + lockers[i] = retryTestLocker{acquire: func(context.Context, LockArgs) (bool, error) { + calls.Add(1) + return false, nil + }} + } + dm := newRetryTestMutex(lockers) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan bool, 1) + go func() { + done <- retryTestAcquire(ctx, dm, read, opts) + }() + synctest.Wait() + if got := calls.Load(); got != 4 { + t.Fatalf("first attempt made %d RPCs, want 4", got) + } + cancel() + synctest.Wait() + select { + case locked := <-done: + if locked { + t.Fatal("canceled waiter acquired lock") + } + default: + t.Errorf("read=%v opts=%+v: cancellation did not interrupt retry wait", read, opts) + <-done + } + time.Sleep(time.Second) + synctest.Wait() + if got := calls.Load(); got != 4 { + t.Fatalf("canceled waiter issued a new RPC attempt: %d calls", got) + } + }) + } + } +} + +func TestDRWMutexRetryDeadline(t *testing.T) { + for _, interval := range []time.Duration{0, time.Hour} { + synctest.Test(t, func(t *testing.T) { + locker := retryTestLocker{acquire: func(context.Context, LockArgs) (bool, error) { + return false, nil + }} + dm := newRetryTestMutex([]NetLocker{locker}) + start := time.Now() + if retryTestAcquire(t.Context(), dm, false, Options{Timeout: 10 * time.Millisecond, RetryInterval: interval}) { + t.Fatal("unavailable lock was acquired") + } + if elapsed := time.Since(start); elapsed != 10*time.Millisecond { + t.Fatalf("retry wait overran deadline: %v", elapsed) + } + }) + } +} + +func TestDRWMutexCanceledPartialAndLateGrants(t *testing.T) { + for _, read := range []bool{false, true} { + for _, slowRelease := range []bool{false, true} { + synctest.Test(t, func(t *testing.T) { + lateGrant := make(chan struct{}) + releaseGate := make(chan struct{}) + if !slowRelease { + close(releaseGate) + } + var calls, releases atomic.Int64 + var held [4]atomic.Bool + lockers := make([]NetLocker, len(held)) + for i := range lockers { + lockers[i] = retryTestLocker{ + acquire: func(ctx context.Context, _ LockArgs) (bool, error) { + calls.Add(1) + if i > 0 { + <-lateGrant + } + if ctx.Err() != nil { + t.Error("NetLocker acquisition unexpectedly inherits caller cancellation") + } + held[i].Store(true) + return true, nil + }, + release: func(context.Context, LockArgs) (bool, error) { + if i == 0 { + <-releaseGate + } + held[i].Store(false) + releases.Add(1) + return true, nil + }, + } + } + dm := newRetryTestMutex(lockers) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan bool, 1) + go func() { done <- retryTestAcquire(ctx, dm, read, Options{Timeout: time.Hour}) }() + synctest.Wait() + if !held[0].Load() || calls.Load() != 4 { + t.Fatal("partial acquisition was not established") + } + cancel() + synctest.Wait() + if slowRelease { + // Cancellation of retry sleep is not a whole-call bound: + // synchronous cleanup of a partial grant can still block. + select { + case <-done: + t.Fatal("acquisition returned before partial-grant cleanup completed") + default: + } + close(releaseGate) + synctest.Wait() + } + select { + case locked := <-done: + if locked { + t.Fatal("canceled partial acquisition returned success") + } + default: + t.Error("acquisition did not return after partial-grant cleanup") + <-done + } + if held[0].Load() || releases.Load() != 1 { + t.Fatal("partial grant was not released") + } + close(lateGrant) + synctest.Wait() + for i := range held { + if held[i].Load() { + t.Errorf("late grant on node %d was not released", i) + } + } + if calls.Load() != 4 || releases.Load() != 4 { + t.Fatalf("calls=%d releases=%d, want 4/4", calls.Load(), releases.Load()) + } + }) + } + } +} diff --git a/internal/dsync/drwmutex.go b/internal/dsync/drwmutex.go index f8e4f95fd..eaa6f87bd 100644 --- a/internal/dsync/drwmutex.go +++ b/internal/dsync/drwmutex.go @@ -260,14 +260,22 @@ func (dm *DRWMutex) lockBlocking(ctx context.Context, lockLossCallback func(), i return locked } + var retryInterval time.Duration switch { case opts.RetryInterval < 0: return false case opts.RetryInterval > 0: - time.Sleep(opts.RetryInterval) + retryInterval = opts.RetryInterval default: attempt++ - time.Sleep(lockRetryBackOff(dm.rng, attempt)) + retryInterval = lockRetryBackOff(dm.rng, attempt) + } + timer := time.NewTimer(retryInterval) + select { + case <-ctx.Done(): + timer.Stop() + return false + case <-timer.C: } } } diff --git a/internal/lsync/lrwmutex-retry_test.go b/internal/lsync/lrwmutex-retry_test.go new file mode 100644 index 000000000..f4aa31e36 --- /dev/null +++ b/internal/lsync/lrwmutex-retry_test.go @@ -0,0 +1,72 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package lsync + +import ( + "context" + "testing" + "testing/synctest" + "time" +) + +func TestLRWMutexRetryCancellation(t *testing.T) { + for _, read := range []bool{false, true} { + synctest.Test(t, func(t *testing.T) { + lm := NewLRWMutex() + lm.Lock() + defer lm.Unlock() + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan bool, 1) + go func() { + if read { + done <- lm.GetRLock(ctx, "waiter", "retry-test", time.Hour) + } else { + done <- lm.GetLock(ctx, "waiter", "retry-test", time.Hour) + } + }() + synctest.Wait() + cancel() + synctest.Wait() + select { + case locked := <-done: + if locked { + t.Fatal("canceled waiter acquired lock") + } + default: + t.Error("cancellation did not interrupt local retry wait") + <-done + } + }) + } +} + +func TestLRWMutexRetryDeadline(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + lm := NewLRWMutex() + lm.Lock() + defer lm.Unlock() + start := time.Now() + if lm.GetLock(t.Context(), "waiter", "retry-test", time.Millisecond) { + t.Fatal("unavailable lock was acquired") + } + if elapsed := time.Since(start); elapsed != time.Millisecond { + t.Fatalf("local retry wait overran deadline: %v", elapsed) + } + }) +} diff --git a/internal/lsync/lrwmutex.go b/internal/lsync/lrwmutex.go index 30442a9a0..96669df14 100644 --- a/internal/lsync/lrwmutex.go +++ b/internal/lsync/lrwmutex.go @@ -115,7 +115,13 @@ func (lm *LRWMutex) lockLoop(ctx context.Context, id, source string, timeout tim if lm.lock(id, source, isWriteLock) { return true } - time.Sleep(time.Duration(r.Float64() * float64(lockRetryInterval))) + timer := time.NewTimer(time.Duration(r.Float64() * float64(lockRetryInterval))) + select { + case <-retryCtx.Done(): + timer.Stop() + return false + case <-timer.C: + } } } }