From 258b167b1c65f1a4fa4d85dbef7ae8ef9fe4f3f6 Mon Sep 17 00:00:00 2001 From: mr javad seydi Date: Sat, 19 Sep 2026 22:11:10 +0330 Subject: [PATCH 1/4] fix(listing): resolve undecidable list entries against the locked read path Refs: https://github.com/pgsty/silo/issues/218 Signed-off-by: mr javad seydi --- cmd/data-scanner.go | 9 +- cmd/erasure-healing.go | 6 +- cmd/erasure-server-pool-decom.go | 3 +- cmd/erasure-server-pool-rebalance.go | 3 +- cmd/erasure-server-pool.go | 10 +- cmd/global-heal.go | 3 +- cmd/metacache-entries.go | 10 ++ cmd/metacache-null-quorum_test.go | 164 +++++++++++++++++++++++++++ cmd/metacache-set.go | 82 +++++++++++++- 9 files changed, 272 insertions(+), 18 deletions(-) diff --git a/cmd/data-scanner.go b/cmd/data-scanner.go index 0475c4a53..8bbfd8f31 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) { + partial: func(entries metaCacheEntries, errs []error) error { if !f.shouldHeal() { cancel() - return + return nil } 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 + return nil } // 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 + return nil } custom["versions"] = fmt.Sprint(len(fiv.Versions)) @@ -814,6 +814,7 @@ 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 8fb371270..e02a075f1 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) { + partial: func(entries metaCacheEntries, _ []error) 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 + return nil } if err := healEntry(bucket, *entry, scanMode); err != nil { cancel() - return } + return nil }, finished: nil, } diff --git a/cmd/erasure-server-pool-decom.go b/cmd/erasure-server-pool-decom.go index 0e422c3ae..abae13c0d 100644 --- a/cmd/erasure-server-pool-decom.go +++ b/cmd/erasure-server-pool-decom.go @@ -730,11 +730,12 @@ func (set *erasureObjects) listObjectsToDecommission(ctx context.Context, bi dec minDisks: listingQuorum, reportNotFound: false, agreed: fn, - partial: func(entries metaCacheEntries, _ []error) { + partial: func(entries metaCacheEntries, _ []error) 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 b60c9c904..f352c5f93 100644 --- a/cmd/erasure-server-pool-rebalance.go +++ b/cmd/erasure-server-pool-rebalance.go @@ -525,11 +525,12 @@ func (set *erasureObjects) listObjectsToRebalance(ctx context.Context, bucketNam minDisks: listingQuorum, reportNotFound: false, agreed: fn, - partial: func(entries metaCacheEntries, _ []error) { + partial: func(entries metaCacheEntries, _ []error) 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 df1d2f22d..b04e57589 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -2642,11 +2642,15 @@ func (z *erasureServerPools) Walk(ctx context.Context, bucket, prefix string, re minDisks: listingQuorum, reportNotFound: false, agreed: send, - partial: func(entries metaCacheEntries, _ []error) { - entry, ok := entries.resolve(&resolver) - if ok { + partial: func(entries metaCacheEntries, _ []error) error { + entry, err := set.resolveListEntry(ctx, bucket, entries, &resolver) + if err != nil { + return err + } + if entry != nil { send(*entry) } + return nil }, finished: nil, } diff --git a/cmd/global-heal.go b/cmd/global-heal.go index ca78df818..e153418c1 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) { + partial: func(entries metaCacheEntries, _ []error) error { entry, ok := entries.resolve(&resolver) if !ok { // check if we can get one entry at least @@ -538,6 +538,7 @@ 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/metacache-entries.go b/cmd/metacache-entries.go index fceec0b43..166176596 100644 --- a/cmd/metacache-entries.go +++ b/cmd/metacache-entries.go @@ -461,6 +461,16 @@ func (m metaCacheEntries) firstFound() (first *metaCacheEntry, n int) { return first, n } +// firstObject returns the first entry that contains object metadata. +func (m metaCacheEntries) firstObject() *metaCacheEntry { + for i := range m { + if m[i].isObject() { + return &m[i] + } + } + return nil +} + // names will return all names in order. // Since this allocates it should not be used in critical functions. func (m metaCacheEntries) names() []string { diff --git a/cmd/metacache-null-quorum_test.go b/cmd/metacache-null-quorum_test.go index a0aa09308..8804f15f3 100644 --- a/cmd/metacache-null-quorum_test.go +++ b/cmd/metacache-null-quorum_test.go @@ -52,6 +52,14 @@ type nullQuorumWalkDisk struct { index int } +type failedMetadataWalkDisk struct { + StorageAPI +} + +func (d *failedMetadataWalkDisk) WalkDir(context.Context, WalkDirOptions, io.Writer) error { + return errDiskNotFound +} + func (d *nullQuorumWalkDisk) WalkDir(ctx context.Context, opts WalkDirOptions, out io.Writer) error { if opts.Bucket != d.schedule.bucket { return d.StorageAPI.WalkDir(ctx, opts, out) @@ -227,6 +235,162 @@ func TestListObjectsSingleNullQuorumHTTP(t *testing.T) { } } +func TestListObjectsFallsBackToReadableMetadata(t *testing.T) { + z, bucket, router := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + disks := set.getDisks() + const object = "object" + oldBody := bytes.Repeat([]byte{'a'}, 8192) + newBody := bytes.Repeat([]byte{'b'}, 8192) + oldTime := time.Now().UTC().Add(-time.Hour) + + if _, err := z.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(oldBody), int64(len(oldBody)), "", ""), ObjectOptions{MTime: oldTime}); err != nil { + t.Fatal(err) + } + oldMeta := make([][]byte, len(disks)) + for i, disk := range disks { + oldMeta[i] = mustReadNullQuorumMeta(t, disk, bucket, object) + } + if _, err := z.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(newBody), int64(len(newBody)), "", ""), ObjectOptions{MTime: oldTime.Add(time.Minute)}); err != nil { + t.Fatal(err) + } + + // The eight readable walk streams are split 3/5 across two generations; + // the other eight streams fail. Neither generation is proven readable by + // the listing inputs, so returning HTTP 200 would silently omit the key. + for i := range 3 { + if err := disks[i].WriteAll(t.Context(), bucket, object+"/"+xlStorageFormatFile, oldMeta[i]); err != nil { + t.Fatal(err) + } + } + wrapped := make([]StorageAPI, len(disks)) + for i, disk := range disks { + wrapped[i] = disk + if i >= 8 { + wrapped[i] = &failedMetadataWalkDisk{StorageAPI: disk} + } + } + set.getDisks = func() []StorageAPI { return wrapped } + + rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1000", "", "", "")) + var list ListObjectsV2Response + if rec.Code != http.StatusOK || xml.Unmarshal(rec.Body.Bytes(), &list) != nil { + t.Fatalf("LIST failed to resolve readable metadata: %d %s", rec.Code, rec.Body.String()) + } + if list.KeyCount != 1 || len(list.Contents) != 1 || list.Contents[0].Key != object { + t.Fatalf("LIST omitted the readable key: %+v", list) + } + + // The object itself still has a read quorum outside the failed walk + // streams, demonstrating why the LIST must report uncertainty, not absence. + get := nullQuorumRequest(t, router, http.MethodGet, getGetObjectURL("", bucket, object)) + 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()) + } +} + +// The fallback must read under the object read lock. A lock-free read of an +// object being overwritten stably reports a live object as absent, which is +// the omission this repair exists to remove. Hold the write lock and assert +// the listing waits for it rather than resolving straight through. +func TestListFallbackTakesObjectReadLock(t *testing.T) { + z, bucket, router := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + disks := set.getDisks() + const object = "object" + oldBody, newBody := bytes.Repeat([]byte{'a'}, 8192), bytes.Repeat([]byte{'b'}, 8192) + oldTime := time.Now().UTC().Add(-time.Hour) + + if _, err := z.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(oldBody), int64(len(oldBody)), "", ""), ObjectOptions{MTime: oldTime}); err != nil { + t.Fatal(err) + } + oldMeta := make([][]byte, len(disks)) + for i, disk := range disks { + oldMeta[i] = mustReadNullQuorumMeta(t, disk, bucket, object) + } + if _, err := z.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(newBody), int64(len(newBody)), "", ""), ObjectOptions{MTime: oldTime.Add(time.Minute)}); err != nil { + t.Fatal(err) + } + // Split the readable walk streams so the entry cannot be resolved from + // the snapshots and the fallback has to run. + for i := range 3 { + if err := disks[i].WriteAll(t.Context(), bucket, object+"/"+xlStorageFormatFile, oldMeta[i]); err != nil { + t.Fatal(err) + } + } + wrapped := make([]StorageAPI, len(disks)) + for i, disk := range disks { + wrapped[i] = disk + if i >= 8 { + wrapped[i] = &failedMetadataWalkDisk{StorageAPI: disk} + } + } + set.getDisks = func() []StorageAPI { return wrapped } + + lock := set.NewNSLock(bucket, object) + lkctx, err := lock.GetLock(t.Context(), globalOperationTimeout) + if err != nil { + t.Fatal(err) + } + const hold = 300 * time.Millisecond + released := make(chan struct{}) + go func() { + time.Sleep(hold) + lock.Unlock(lkctx) + close(released) + }() + + start := time.Now() + rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1000", "", "", "")) + elapsed := time.Since(start) + <-released + + if elapsed < hold { + t.Fatalf("listing fallback did not wait for the object lock: %v < %v", elapsed, hold) + } + var list ListObjectsV2Response + if rec.Code != http.StatusOK || xml.Unmarshal(rec.Body.Bytes(), &list) != nil { + t.Fatalf("LIST failed after the lock was released: %d %s", rec.Code, rec.Body.String()) + } + if list.KeyCount != 1 || len(list.Contents) != 1 || list.Contents[0].Key != object { + t.Fatalf("LIST omitted the readable key: %+v", list) + } +} + +// A truncated xl.meta is what a concurrent overwrite looks like on one drive, +// and the walker skips that entry. Skipping is safe and deliberate: the key is +// only omitted when no generation reaches quorum, which listPath now resolves +// against the read path. Pin that, so the walker is not "hardened" into +// failing a whole listing over a minority of unreadable drives. +func TestListObjectsSurvivesTruncatedMetadataMinority(t *testing.T) { + z, bucket, router := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + disks := set.getDisks() + const object = "object" + body := bytes.Repeat([]byte{'a'}, 8192) + + if _, err := z.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(body), int64(len(body)), "", ""), ObjectOptions{}); err != nil { + t.Fatal(err) + } + + // Truncate xl.meta on a minority of drives, as an overwrite in flight does. + for i := range 4 { + meta := mustReadNullQuorumMeta(t, disks[i], bucket, object) + if err := disks[i].WriteAll(t.Context(), bucket, object+"/"+xlStorageFormatFile, meta[:len(meta)/2]); err != nil { + t.Fatal(err) + } + } + + rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1000", "", "", "")) + var list ListObjectsV2Response + if rec.Code != http.StatusOK || xml.Unmarshal(rec.Body.Bytes(), &list) != nil { + t.Fatalf("a truncated minority failed the listing: %d %s", rec.Code, rec.Body.String()) + } + if list.KeyCount != 1 || len(list.Contents) != 1 || list.Contents[0].Key != object { + t.Fatalf("a truncated minority omitted the key: %+v", list) + } +} + func TestSingleNullQuorumReadAndConsumers(t *testing.T) { z, bucket, router := nullQuorumBackend(t) set := z.serverPools[0].sets[0] diff --git a/cmd/metacache-set.go b/cmd/metacache-set.go index a2ab603c6..16eb022a0 100644 --- a/cmd/metacache-set.go +++ b/cmd/metacache-set.go @@ -700,6 +700,72 @@ 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. +// +// Cost: one locked getObjectFileInfo per unresolved entry. Only disagreeing +// entries pay it -- 123 of ~162k entries in the issue #218 rolling-restart run +// -- but each is a namespace lock round trip, so a listing taken during a +// restart of a large bucket can pay it per page. Batch the fallback reads if +// listing latency during restarts becomes the complaint. +func (er *erasureObjects) resolveListEntry(ctx context.Context, bucket string, entries metaCacheEntries, resolver *metadataResolutionParams) (*metaCacheEntry, error) { + entry, ok := entries.resolve(resolver) + if ok { + return entry, nil + } + entry = entries.firstObject() + if entry == nil { + return nil, nil + } + readQuorumErr := func() error { + return InsufficientReadQuorum{ + Bucket: bucket, + Object: entry.name, + Err: errErasureReadQuorum, + Type: RQInconsistentMeta, + } + } + if resolver.requestedVersions != 1 { + return nil, readQuorumErr() + } + + // 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. + lock := er.NewNSLock(bucket, entry.name) + lkctx, err := lock.GetRLock(ctx, globalOperationTimeout) + if err != nil { + return nil, err + } + defer lock.RUnlock(lkctx) + + fi, _, _, err := er.getObjectFileInfo(lkctx.Context(), bucket, entry.name, ObjectOptions{}, false) + if err != nil { + // Absence is reported only when the locked read proves it. Anything + // else is undecidable and fails the request instead of dropping the + // key from a successful listing. + if isErrObjectNotFound(err) || isErrVersionNotFound(err) { + return nil, nil + } + return nil, err + } + if fi.Deleted { + return nil, nil + } + + 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 +} + // Will return io.EOF if continuing would not yield more results. func (er *erasureObjects) listPath(ctx context.Context, o listPathOptions, results chan<- metaCacheEntry) (err error) { defer xioutil.SafeClose(results) @@ -776,15 +842,19 @@ func (er *erasureObjects) listPath(ctx context.Context, o listPathOptions, resul case results <- entry: } }, - partial: func(entries metaCacheEntries, errs []error) { + partial: func(entries metaCacheEntries, _ []error) error { // Results Disagree :-( - entry, ok := entries.resolve(&resolver) - if ok { + entry, err := er.resolveListEntry(ctx, o.Bucket, entries, &resolver) + if err != nil { + return err + } + if entry != nil { select { case <-ctxDone: case results <- *entry: } } + return nil }, }) } @@ -980,7 +1050,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) + partial func(entries metaCacheEntries, errs []error) error // finished will be called when all streams have finished and // more than one disk returned an error. @@ -1191,7 +1261,9 @@ func listPathRaw(ctx context.Context, opts listPathRawOptions) (err error) { continue } if opts.partial != nil { - opts.partial(topEntries, errs) + if err := opts.partial(topEntries, errs); err != nil { + return err + } } // Skip the inputs we used. for i, r := range readers { From 8c784f6133ac1d857b16710f3033c5dbe780231a Mon Sep 17 00:00:00 2001 From: mr javad seydi Date: Wed, 23 Sep 2026 18:22:56 +0330 Subject: [PATCH 2/4] fix(listing): narrow the list fallback per review Signed-off-by: mr javad seydi --- cmd/erasure-server-pool.go | 4 +- cmd/metacache-entries.go | 10 -- cmd/metacache-null-quorum_test.go | 236 ++++++++++++++++++++++++++++++ cmd/metacache-set.go | 50 +++++-- 4 files changed, 272 insertions(+), 28 deletions(-) diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go index b04e57589..4b476a0b2 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -2642,8 +2642,8 @@ func (z *erasureServerPools) Walk(ctx context.Context, bucket, prefix string, re minDisks: listingQuorum, reportNotFound: false, agreed: send, - partial: func(entries metaCacheEntries, _ []error) error { - entry, err := set.resolveListEntry(ctx, bucket, entries, &resolver) + partial: func(entries metaCacheEntries, errs []error) error { + entry, err := set.resolveListEntry(ctx, bucket, entries, errs, &resolver) if err != nil { return err } diff --git a/cmd/metacache-entries.go b/cmd/metacache-entries.go index 166176596..fceec0b43 100644 --- a/cmd/metacache-entries.go +++ b/cmd/metacache-entries.go @@ -461,16 +461,6 @@ func (m metaCacheEntries) firstFound() (first *metaCacheEntry, n int) { return first, n } -// firstObject returns the first entry that contains object metadata. -func (m metaCacheEntries) firstObject() *metaCacheEntry { - for i := range m { - if m[i].isObject() { - return &m[i] - } - } - return nil -} - // names will return all names in order. // Since this allocates it should not be used in critical functions. func (m metaCacheEntries) names() []string { diff --git a/cmd/metacache-null-quorum_test.go b/cmd/metacache-null-quorum_test.go index 8804f15f3..7b93c8733 100644 --- a/cmd/metacache-null-quorum_test.go +++ b/cmd/metacache-null-quorum_test.go @@ -60,6 +60,29 @@ func (d *failedMetadataWalkDisk) WalkDir(context.Context, WalkDirOptions, io.Wri return errDiskNotFound } +// offlineNullQuorumDisk models a drive of a node that is away. +type offlineNullQuorumDisk struct { + StorageAPI +} + +func (d *offlineNullQuorumDisk) IsOnline() bool { return false } + +func (d *offlineNullQuorumDisk) DiskInfo(context.Context, DiskInfoOptions) (DiskInfo, error) { + return DiskInfo{}, errDiskNotFound +} + +func (d *offlineNullQuorumDisk) WalkDir(context.Context, WalkDirOptions, io.Writer) error { + return errDiskNotFound +} + +func (d *offlineNullQuorumDisk) ReadXL(context.Context, string, string, bool) (RawFileInfo, error) { + return RawFileInfo{}, errDiskNotFound +} + +func (d *offlineNullQuorumDisk) ReadVersion(context.Context, string, string, string, string, ReadOptions) (FileInfo, error) { + return FileInfo{}, errDiskNotFound +} + func (d *nullQuorumWalkDisk) WalkDir(ctx context.Context, opts WalkDirOptions, out io.Writer) error { if opts.Bucket != d.schedule.bucket { return d.StorageAPI.WalkDir(ctx, opts, out) @@ -127,6 +150,30 @@ func nullQuorumRequest(t *testing.T, router http.Handler, method, target string) return rec } +func nullQuorumPut(t *testing.T, z *erasureServerPools, bucket, object string, body []byte, opts ObjectOptions) { + t.Helper() + if _, err := z.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(body), int64(len(body)), "", ""), opts); err != nil { + t.Fatal(err) + } +} + +func nullQuorumListV2Keys(t *testing.T, router http.Handler, bucket string) (int, []string) { + t.Helper() + rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1000", "", "", "")) + if rec.Code != http.StatusOK { + return rec.Code, nil + } + var list ListObjectsV2Response + if err := xml.Unmarshal(rec.Body.Bytes(), &list); err != nil { + t.Fatal(err) + } + keys := make([]string, 0, len(list.Contents)) + for _, object := range list.Contents { + keys = append(keys, object.Key) + } + return rec.Code, keys +} + func TestListObjectsSingleNullQuorumHTTP(t *testing.T) { z, bucket, router := nullQuorumBackend(t) oldBody := bytes.Repeat([]byte{'a'}, 8192) @@ -391,6 +438,195 @@ func TestListObjectsSurvivesTruncatedMetadataMinority(t *testing.T) { } } +func TestListStaleMinorityIgnored(t *testing.T) { + z, bucket, router := nullQuorumBackend(t) + disks := z.serverPools[0].sets[0].getDisks() + body := bytes.Repeat([]byte{'a'}, 8192) + nullQuorumPut(t, z, bucket, "gone", body, ObjectOptions{}) + nullQuorumPut(t, z, bucket, "keep", body, ObjectOptions{}) + for _, disk := range disks[4:] { + if err := disk.Delete(t.Context(), bucket, "gone", DeleteOptions{Recursive: true, Immediate: true}); err != nil { + t.Fatal(err) + } + } + + if code, keys := nullQuorumListV2Keys(t, router, bucket); code != http.StatusOK || !slices.Equal(keys, []string{"keep"}) { + t.Errorf("ListObjectsV2: got %d %v; want 200 [keep]", code, keys) + } + results := make(chan itemOrErr[ObjectInfo], 16) + if err := z.Walk(t.Context(), bucket, "", results, WalkOptions{}); err != nil { + t.Fatal(err) + } + var names []string + for result := range results { + if result.Err != nil { + t.Fatal(result.Err) + } + names = append(names, result.Item.Name) + } + if !slices.Equal(names, []string{"keep"}) { + t.Errorf("Walk: got %v; want [keep]", names) + } + + rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectVersionsURL("", bucket, "", "1000", "")) + var list struct { + Versions []struct { + Key string `xml:"Key"` + } `xml:"Version"` + } + if rec.Code != http.StatusOK { + t.Errorf("ListObjectVersions: got %d; want 200", rec.Code) + } else if err := xml.Unmarshal(rec.Body.Bytes(), &list); err != nil { + t.Fatal(err) + } else if len(list.Versions) != 1 || list.Versions[0].Key != "keep" { + t.Errorf("ListObjectVersions: got %+v; want [keep]", list.Versions) + } + + if _, err := z.DeleteObject(t.Context(), bucket, "keep", ObjectOptions{}); err != nil { + t.Fatal(err) + } + if err := z.DeleteBucket(t.Context(), bucket, DeleteBucketOptions{}); err != nil { + t.Errorf("DeleteBucket with only a stale minority: %v", err) + } +} + +func TestIAMListingIgnoresStaleMinority(t *testing.T) { + z, _, _ := nullQuorumBackend(t) + body := []byte(`{"version":1}`) + alive := iamConfigUsersPrefix + "alive/identity.json" + ghost := iamConfigUsersPrefix + "ghost/identity.json" + nullQuorumPut(t, z, minioMetaBucket, alive, body, ObjectOptions{}) + nullQuorumPut(t, z, minioMetaBucket, ghost, body, ObjectOptions{}) + for _, disk := range z.serverPools[0].getHashedSet(ghost).getDisks()[4:] { + if err := disk.Delete(t.Context(), minioMetaBucket, ghost, DeleteOptions{Recursive: true, Immediate: true}); err != nil { + t.Fatal(err) + } + } + + items, err := newIAMObjectStore(z, MinIOUsersSysType).listAllIAMConfigItems(t.Context()) + if err != nil { + t.Fatalf("IAM listing failed on a stale minority (retriable=%v): %v", configRetriableErrors(err), err) + } + if got := items["users/"]; !slices.Equal(got, []string{"alive/identity.json"}) { + t.Errorf("IAM users: got %v; want [alive/identity.json]", got) + } +} + +func TestListSplitDirectoryObject(t *testing.T) { + z, bucket, router := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + disks := set.getDisks() + const object = "dir/" + encoded := encodeDirObject(object) + mtime := time.Now().UTC().Add(-time.Hour) + + nullQuorumPut(t, z, bucket, object, nil, ObjectOptions{MTime: mtime}) + old := make([][]byte, 3) + for i := range old { + old[i] = mustReadNullQuorumMeta(t, disks[i], bucket, encoded) + } + nullQuorumPut(t, z, bucket, object, nil, ObjectOptions{MTime: mtime.Add(time.Minute)}) + for i := range old { + if err := disks[i].WriteAll(t.Context(), bucket, encoded+"/"+xlStorageFormatFile, old[i]); err != nil { + t.Fatal(err) + } + } + + walkers := slices.Clone(disks) + for i := 8; i < len(walkers); i++ { + walkers[i] = &failedMetadataWalkDisk{StorageAPI: disks[i]} + } + set.getDisks = func() []StorageAPI { return walkers } + if code, keys := nullQuorumListV2Keys(t, router, bucket); code != http.StatusOK || !slices.Equal(keys, []string{object}) { + t.Errorf("ListObjectsV2: got %d %v; want 200 [%s]", code, keys, object) + } +} + +func TestListFallbackDoesNotTrustNotFound(t *testing.T) { + z, bucket, _ := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + disks := set.getDisks() + const object = "object" + body := bytes.Repeat([]byte{'a'}, 8192) + mtime := time.Now().UTC().Add(-time.Hour) + + nullQuorumPut(t, z, bucket, object, body, ObjectOptions{MTime: mtime}) + old := make([][]byte, len(disks)) + for i := range disks { + old[i] = mustReadNullQuorumMeta(t, disks[i], bucket, object) + } + nullQuorumPut(t, z, bucket, object, body, ObjectOptions{MTime: mtime.Add(time.Minute)}) + newer := make([][]byte, len(disks)) + for i := range disks { + newer[i] = mustReadNullQuorumMeta(t, disks[i], bucket, object) + if err := disks[i].Delete(t.Context(), bucket, object, DeleteOptions{Recursive: true, Immediate: true}); err != nil { + t.Fatal(err) + } + } + + entries := make(metaCacheEntries, len(disks)) + errs := make([]error, len(disks)) + for i := range 3 { + entries[i] = metaCacheEntry{name: object, metadata: old[i]} + } + for i := 3; i < 8; i++ { + entries[i] = metaCacheEntry{name: object, metadata: newer[i]} + } + for i := 8; i < len(errs); i++ { + errs[i] = errDiskNotFound + } + resolver := metadataResolutionParams{dirQuorum: 8, objQuorum: 8, bucket: bucket, requestedVersions: 1} + entry, err := set.resolveListEntry(t.Context(), bucket, entries, errs, &resolver) + if entry != nil || !isErrReadQuorum(err) { + t.Fatalf("ambiguous not-found: got entry=%v err=%v; want read quorum error", entry, err) + } +} + +// G0 was written 8+8 while a node was away. G1 was later written 12+4 but +// missed drive 0, which still holds G0. Then a node holding four G1 drives +// goes away: eleven G1 and one G0 remain online. G1 exists but lacks one of +// its twelve data shards until the node returns, so HEAD must not report 404 +// and LIST must still show the key. +func TestListCrossParityMinorityStaysVisible(t *testing.T) { + z, bucket, router := nullQuorumBackend(t) + set := z.serverPools[0].sets[0] + disks := set.getDisks() + const object = "object" + mtime := time.Now().UTC().Add(-time.Hour) + + nullQuorumPut(t, z, bucket, object, bytes.Repeat([]byte{'a'}, 256<<10), ObjectOptions{MTime: mtime, MaxParity: true}) + g0 := mustReadNullQuorumMeta(t, disks[0], bucket, object) + var x xlMetaV2 + if err := x.LoadOrConvert(g0); err != nil { + t.Fatal(err) + } + if h := x.versions[0].header; h.EcM != 8 || h.EcN != 8 { + t.Fatalf("G0 is %d+%d, want 8+8", h.EcM, h.EcN) + } + g1Body := bytes.Repeat([]byte{'b'}, 256<<10) + nullQuorumPut(t, z, bucket, object, g1Body, ObjectOptions{MTime: mtime.Add(time.Minute)}) + if err := disks[0].WriteAll(t.Context(), bucket, object+"/"+xlStorageFormatFile, g0); err != nil { + t.Fatal(err) + } + + online := slices.Clone(disks) + for i := 12; i < len(online); i++ { + online[i] = &offlineNullQuorumDisk{StorageAPI: disks[i]} + } + set.getDisks = func() []StorageAPI { return online } + if code := nullQuorumRequest(t, router, http.MethodHead, getHeadObjectURL("", bucket, object)).Code; code == http.StatusNotFound { + t.Errorf("HEAD with a node away: 404 for an existing object") + } + if code, keys := nullQuorumListV2Keys(t, router, bucket); code != http.StatusOK || !slices.Equal(keys, []string{object}) { + t.Errorf("ListObjectsV2 with a node away: got %d %v; want 200 [%s]", code, keys, object) + } + + set.getDisks = func() []StorageAPI { return disks } + if get := nullQuorumRequest(t, router, http.MethodGet, getGetObjectURL("", bucket, object)); get.Code != http.StatusOK || !bytes.Equal(get.Body.Bytes(), g1Body) { + t.Fatalf("GET after the node returns: %d", get.Code) + } +} + func TestSingleNullQuorumReadAndConsumers(t *testing.T) { z, bucket, router := nullQuorumBackend(t) set := z.serverPools[0].sets[0] diff --git a/cmd/metacache-set.go b/cmd/metacache-set.go index 16eb022a0..160289f16 100644 --- a/cmd/metacache-set.go +++ b/cmd/metacache-set.go @@ -708,13 +708,33 @@ func getQuorumDisks(disks []StorageAPI, infos []DiskInfo, readQuorum int) (newDi // -- but each is a namespace lock round trip, so a listing taken during a // restart of a large bucket can pay it per page. Batch the fallback reads if // listing latency during restarts becomes the complaint. -func (er *erasureObjects) resolveListEntry(ctx context.Context, bucket string, entries metaCacheEntries, resolver *metadataResolutionParams) (*metaCacheEntry, error) { +func (er *erasureObjects) resolveListEntry(ctx context.Context, bucket string, entries metaCacheEntries, errs []error, resolver *metadataResolutionParams) (*metaCacheEntry, error) { entry, ok := entries.resolve(resolver) if ok { return entry, nil } - entry = entries.firstObject() - if entry == nil { + if resolver.requestedVersions != 1 { + return nil, nil + } + valid := 0 + for i := range entries { + if !entries[i].isObject() { + continue + } + if _, err := entries[i].xlmeta(); err != nil { + continue + } + valid++ + if entry == nil { + entry = &entries[i] + } + } + for _, err := range errs { + if err != nil { + valid++ + } + } + if entry == nil || valid < resolver.objQuorum { return nil, nil } readQuorumErr := func() error { @@ -725,28 +745,26 @@ func (er *erasureObjects) resolveListEntry(ctx context.Context, bucket string, e Type: RQInconsistentMeta, } } - if resolver.requestedVersions != 1 { - return nil, readQuorumErr() - } - // 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. - lock := er.NewNSLock(bucket, entry.name) - lkctx, err := lock.GetRLock(ctx, globalOperationTimeout) + object := encodeDirObject(entry.name) + lock := er.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, entry.name, ObjectOptions{}, false) + fi, _, _, err := er.getObjectFileInfo(lkctx.Context(), bucket, object, ObjectOptions{}, false) if err != nil { - // Absence is reported only when the locked read proves it. Anything - // else is undecidable and fails the request instead of dropping the - // key from a successful listing. + // 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, nil + return nil, readQuorumErr() } return nil, err } @@ -842,9 +860,9 @@ func (er *erasureObjects) listPath(ctx context.Context, o listPathOptions, resul case results <- entry: } }, - partial: func(entries metaCacheEntries, _ []error) error { + partial: func(entries metaCacheEntries, errs []error) error { // Results Disagree :-( - entry, err := er.resolveListEntry(ctx, o.Bucket, entries, &resolver) + entry, err := er.resolveListEntry(ctx, o.Bucket, entries, errs, &resolver) if err != nil { return err } From 2a4db6a4f9d0dedf7229a3b1c5866d2a1bbb22ab Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Fri, 16 Oct 2026 01:23:29 +0800 Subject: [PATCH 3/4] fix(listing): snapshot asynchronous cache cleanup inputs Capture the metacache peer before launching cleanup and pass listing options by value. The parent can clear its cache ID without changing the background RPC lookup or racing with its reads of shared request state. Publish complete immutable disk snapshots through atomic.Value in the listing fixtures instead of replacing getDisks after requests start. Add a real peer RPC regression for cleanup using the original cache ID; reverting the options copy reproduces both the race and an empty RPC ID. Verified the final 18-test listing race suite, 30 repetitions of the real peer regression, formatting checks and make lint. Prior runtime evidence retains its original e90cb206 source identity. Refs: https://github.com/pgsty/silo/issues/218 Refs: https://github.com/pgsty/silo/pull/239 Signed-off-by: Feng Ruohang --- cmd/metacache-listing-cache_test.go | 253 ++++++++++++++++++--------- cmd/metacache-listing-delete_test.go | 4 +- cmd/metacache-listing-merge_test.go | 9 +- cmd/metacache-null-quorum_test.go | 6 +- cmd/metacache-server-pool.go | 8 +- 5 files changed, 187 insertions(+), 93 deletions(-) diff --git a/cmd/metacache-listing-cache_test.go b/cmd/metacache-listing-cache_test.go index ef6b24e55..68140fa8c 100644 --- a/cmd/metacache-listing-cache_test.go +++ b/cmd/metacache-listing-cache_test.go @@ -29,6 +29,8 @@ import ( "sync/atomic" "testing" "time" + + "github.com/minio/minio/internal/grid" ) type listingReadFailDisk struct { @@ -45,6 +47,17 @@ func (d *listingReadFailDisk) ReadXL(ctx context.Context, bucket, object string, return d.StorageAPI.ReadXL(ctx, bucket, object, readData) } +// Install before issuing requests: cache writers can outlive their HTTP +// response, so disk faults must be switched without replacing getDisks. +// Each published slice must remain immutable. +func installListingDiskSwitch(set *erasureObjects) ([]StorageAPI, func([]StorageAPI)) { + disks := set.getDisks() + var current atomic.Value + current.Store(disks) + set.getDisks = func() []StorageAPI { return current.Load().([]StorageAPI) } + return disks, func(disks []StorageAPI) { current.Store(disks) } +} + func TestListingCachePreservesFallbackError(t *testing.T) { for _, continuation := range []bool{false, true} { name := "cold" @@ -52,87 +65,167 @@ func TestListingCachePreservesFallbackError(t *testing.T) { name = "continuation" } t.Run(name, func(t *testing.T) { - z, bucket, router := nullQuorumBackend(t) - set := z.serverPools[0].sets[0] - disks := set.getDisks() - body := bytes.Repeat([]byte{'a'}, 8192) - mtime := time.Now().UTC().Add(-time.Hour) - for _, object := range []string{"a", "b", "z"} { - nullQuorumPut(t, z, bucket, object, body, ObjectOptions{MTime: mtime}) - } - old := make([][]byte, 3) - for i := range old { - old[i] = mustReadNullQuorumMeta(t, disks[i], bucket, "z") - } - nullQuorumPut(t, z, bucket, "z", body, ObjectOptions{MTime: mtime.Add(time.Minute)}) - var token string - if continuation { - rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1", "", "", "")) - var list ListObjectsV2Response - if rec.Code != http.StatusOK || xml.Unmarshal(rec.Body.Bytes(), &list) != nil || !list.IsTruncated || list.NextContinuationToken == "" { - t.Fatalf("first page: %d %s", rec.Code, rec.Body.String()) - } - token = list.NextContinuationToken - } - for i := range old { - if err := disks[i].WriteAll(t.Context(), bucket, "z/"+xlStorageFormatFile, old[i]); err != nil { - t.Fatal(err) - } - } - var reads atomic.Int64 - walkers := slices.Clone(disks) - for i := range walkers { - walkers[i] = &listingReadFailDisk{StorageAPI: disks[i], object: "z", reads: &reads} - if i >= 8 { - walkers[i] = &failedMetadataWalkDisk{StorageAPI: walkers[i]} - } - } - set.getDisks = func() []StorageAPI { return walkers } - target := getListObjectsV2URL("", bucket, "", "1000", "", "", "") - if token != "" { - target += "&continuation-token=" + url.QueryEscape(token) - } - rec := nullQuorumRequest(t, router, http.MethodGet, target) - t.Logf("LIST=%d failed metadata reads=%d body=%s", rec.Code, reads.Load(), rec.Body.String()) - if reads.Load() == 0 { - t.Fatal("fallback not exercised") - } - var apiErr APIErrorResponse - if rec.Code != http.StatusServiceUnavailable || xml.Unmarshal(rec.Body.Bytes(), &apiErr) != nil || apiErr.Code != "SlowDownRead" { - t.Fatalf("fallback failure lost its read-quorum error: %d %s", rec.Code, rec.Body.String()) - } - if continuation { - caches, _ := localMetacacheMgr.getBucket(t.Context(), bucket).cloneCaches() - if len(caches) == 0 { - t.Fatal("continuation did not create a cache") - } - for _, cache := range caches { - if cache.status != scanStateError { - t.Fatalf("incomplete cache was not marked failed: %+v", cache) - } - } - } - set.getDisks = func() []StorageAPI { return disks } - retry := nullQuorumRequest(t, router, http.MethodGet, target) - var recovered ListObjectsV2Response - if retry.Code != http.StatusOK || xml.Unmarshal(retry.Body.Bytes(), &recovered) != nil { - t.Fatalf("retry after metadata recovers: %d %s", retry.Code, retry.Body.String()) - } - var keys []string - for _, entry := range recovered.Contents { - keys = append(keys, entry.Key) - } - want := []string{"a", "b", "z"} - if continuation { - want = []string{"b", "z"} - } - if !slices.Equal(keys, want) || recovered.IsTruncated { - t.Fatalf("retry returned incomplete result: %+v", recovered) - } + testListingCacheFallbackError(t, continuation, false) }) } } +func TestListingCacheCleanupKeepsOriginalID(t *testing.T) { + testListingCacheFallbackError(t, true, true) +} + +// Use actual peer RPCs to verify the abandoned cache is updated after the +// caller clears its own listing ID. The local-only path cannot observe this. +func recordListingPeerCleanup(t *testing.T) func() { + t.Helper() + tg, err := grid.SetupTestGrid(2) + if err != nil { + t.Fatal(err) + } + previous := globalNotificationSys + t.Cleanup(func() { + tg.Cleanup() + globalNotificationSys = previous + }) + getIDs := make(chan string, 8) + type cleanupResult struct { + id string + err *grid.RemoteErr + } + updated := make(chan cleanupResult, 1) + server := &peerRESTServer{} + if err := getMetacacheListingRPC.Register(tg.Managers[1], func(o *listPathOptions) (*metacache, *grid.RemoteErr) { + getIDs <- o.ID + return server.GetMetacacheListingHandler(o) + }); err != nil { + t.Fatal(err) + } + if err := updateMetacacheListingRPC.Register(tg.Managers[1], func(m *metacache) (*metacache, *grid.RemoteErr) { + result, err := server.UpdateMetacacheListingHandler(m) + if m.error == "no longer used" { + updated <- cleanupResult{id: m.id, err: err} + } + return result, err + }); 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 func() { + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + var ids []string + for range 2 { + select { + case id := <-getIDs: + ids = append(ids, id) + case <-ctx.Done(): + t.Fatal("peer did not receive the failed cache cleanup lookup") + } + } + if ids[0] == "" || ids[1] != ids[0] { + t.Fatalf("cleanup lookup lost the original cache ID: %q", ids) + } + select { + case result := <-updated: + if result.err != nil || result.id != ids[0] { + t.Fatalf("cleanup update: id=%q err=%v, want id=%q", result.id, result.err, ids[0]) + } + t.Logf("peer cleanup Get/Update retained cache ID %q", result.id) + case <-ctx.Done(): + t.Fatal("peer did not finish the failed cache cleanup update") + } + } +} + +func testListingCacheFallbackError(t *testing.T, continuation, peer bool) { + t.Helper() + z, bucket, router := nullQuorumBackend(t) + var waitPeerCleanup func() + if peer { + waitPeerCleanup = recordListingPeerCleanup(t) + } + set := z.serverPools[0].sets[0] + disks, switchDisks := installListingDiskSwitch(set) + body := bytes.Repeat([]byte{'a'}, 8192) + mtime := time.Now().UTC().Add(-time.Hour) + for _, object := range []string{"a", "b", "z"} { + nullQuorumPut(t, z, bucket, object, body, ObjectOptions{MTime: mtime}) + } + old := make([][]byte, 3) + for i := range old { + old[i] = mustReadNullQuorumMeta(t, disks[i], bucket, "z") + } + nullQuorumPut(t, z, bucket, "z", body, ObjectOptions{MTime: mtime.Add(time.Minute)}) + var token string + if continuation { + rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1", "", "", "")) + var list ListObjectsV2Response + if rec.Code != http.StatusOK || xml.Unmarshal(rec.Body.Bytes(), &list) != nil || !list.IsTruncated || list.NextContinuationToken == "" { + t.Fatalf("first page: %d %s", rec.Code, rec.Body.String()) + } + token = list.NextContinuationToken + } + for i := range old { + if err := disks[i].WriteAll(t.Context(), bucket, "z/"+xlStorageFormatFile, old[i]); err != nil { + t.Fatal(err) + } + } + var reads atomic.Int64 + walkers := slices.Clone(disks) + for i := range walkers { + walkers[i] = &listingReadFailDisk{StorageAPI: disks[i], object: "z", reads: &reads} + if i >= 8 { + walkers[i] = &failedMetadataWalkDisk{StorageAPI: walkers[i]} + } + } + switchDisks(walkers) + target := getListObjectsV2URL("", bucket, "", "1000", "", "", "") + if token != "" { + target += "&continuation-token=" + url.QueryEscape(token) + } + rec := nullQuorumRequest(t, router, http.MethodGet, target) + t.Logf("LIST=%d failed metadata reads=%d body=%s", rec.Code, reads.Load(), rec.Body.String()) + if reads.Load() == 0 { + t.Fatal("fallback not exercised") + } + var apiErr APIErrorResponse + if rec.Code != http.StatusServiceUnavailable || xml.Unmarshal(rec.Body.Bytes(), &apiErr) != nil || apiErr.Code != "SlowDownRead" { + t.Fatalf("fallback failure lost its read-quorum error: %d %s", rec.Code, rec.Body.String()) + } + if waitPeerCleanup != nil { + waitPeerCleanup() + } + if continuation { + caches, _ := localMetacacheMgr.getBucket(t.Context(), bucket).cloneCaches() + if len(caches) == 0 { + t.Fatal("continuation did not create a cache") + } + for _, cache := range caches { + if cache.status != scanStateError { + t.Fatalf("incomplete cache was not marked failed: %+v", cache) + } + } + } + switchDisks(disks) + retry := nullQuorumRequest(t, router, http.MethodGet, target) + var recovered ListObjectsV2Response + if retry.Code != http.StatusOK || xml.Unmarshal(retry.Body.Bytes(), &recovered) != nil { + t.Fatalf("retry after metadata recovers: %d %s", retry.Code, retry.Body.String()) + } + var keys []string + for _, entry := range recovered.Contents { + keys = append(keys, entry.Key) + } + want := []string{"a", "b", "z"} + if continuation { + want = []string{"b", "z"} + } + if !slices.Equal(keys, want) || recovered.IsTruncated { + t.Fatalf("retry returned incomplete result: %+v", recovered) + } +} + func TestListingCachePagination(t *testing.T) { z, bucket, router := nullQuorumBackend(t) for _, key := range []string{"a", "b", "c", "d"} { @@ -172,6 +265,7 @@ func TestListingCachePagination(t *testing.T) { // completion barrier belongs at EOF, not before every page response. func TestListingCacheFullPageDoesNotWaitForTail(t *testing.T) { z, bucket, router := nullQuorumBackend(t) + disks, switchDisks := installListingDiskSwitch(z.serverPools[0].sets[0]) for _, key := range []string{"a", "b", "c", "d", "e", "zz"} { nullQuorumPut(t, z, bucket, key, []byte(key), ObjectOptions{}) } @@ -180,16 +274,15 @@ func TestListingCacheFullPageDoesNotWaitForTail(t *testing.T) { if first.Code != http.StatusOK || xml.Unmarshal(first.Body.Bytes(), &page) != nil || !page.IsTruncated { t.Fatalf("first page: %d %s", first.Code, first.Body.String()) } - set := z.serverPools[0].sets[0] resume := make(chan struct{}) release := sync.OnceFunc(func() { close(resume) }) defer release() var blocked, active atomic.Int64 - walkers := slices.Clone(set.getDisks()) + walkers := slices.Clone(disks) for i, disk := range walkers { walkers[i] = &listingPausedTailDisk{StorageAPI: disk, bucket: bucket, tail: "zz", resume: resume, blocked: &blocked, active: &active} } - set.getDisks = func() []StorageAPI { return walkers } + switchDisks(walkers) ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) defer cancel() target := getListObjectsV2URL("", bucket, "", "1", "", "", "") + "&continuation-token=" + url.QueryEscape(page.NextContinuationToken) diff --git a/cmd/metacache-listing-delete_test.go b/cmd/metacache-listing-delete_test.go index 9afe58ef3..74cfa2b5e 100644 --- a/cmd/metacache-listing-delete_test.go +++ b/cmd/metacache-listing-delete_test.go @@ -33,6 +33,7 @@ func TestListFallbackPreservesPoolDeleteMarker(t *testing.T) { z, bucket, router := listingPoolsBackend(t) ctx := t.Context() live, deleted := z.serverPools[1-markerPool].sets[0], z.serverPools[markerPool].sets[0] + disks, switchDisks := installListingDiskSwitch(deleted) const object = "gone" body := bytes.Repeat([]byte{'a'}, 8192) oldTime := time.Now().UTC().Add(-time.Hour) @@ -42,7 +43,6 @@ func TestListFallbackPreservesPoolDeleteMarker(t *testing.T) { t.Fatal(err) } } - disks := deleted.getDisks() old := make([][]byte, 3) for i := range old { old[i] = mustReadNullQuorumMeta(t, disks[i], bucket, object) @@ -69,7 +69,7 @@ func TestListFallbackPreservesPoolDeleteMarker(t *testing.T) { for i := 8; i < len(walkers); i++ { walkers[i] = &failedMetadataWalkDisk{StorageAPI: disks[i]} } - deleted.getDisks = func() []StorageAPI { return walkers } + switchDisks(walkers) fi, _, _, err := deleted.getObjectFileInfo(ctx, bucket, object, ObjectOptions{}, false) if err != nil || !fi.Deleted { t.Fatalf("read fallback marker: %+v %v", fi, err) diff --git a/cmd/metacache-listing-merge_test.go b/cmd/metacache-listing-merge_test.go index da71e8fb1..9fe4f349a 100644 --- a/cmd/metacache-listing-merge_test.go +++ b/cmd/metacache-listing-merge_test.go @@ -116,6 +116,8 @@ func TestListingMergeDoesNotSkipFailedPool(t *testing.T) { defer cancel() z, bucket, router := listingPoolsBackend(t) broken, healthy := z.serverPools[0].sets[0], z.serverPools[1].sets[0] + brokenDisks, switchBrokenDisks := installListingDiskSwitch(broken) + healthyDisks, switchHealthyDisks := installListingDiskSwitch(healthy) body := bytes.Repeat([]byte{'a'}, 8192) mtime := time.Now().UTC().Add(-time.Hour) put := func(set *erasureObjects, name string, tm time.Time) { @@ -135,7 +137,6 @@ func TestListingMergeDoesNotSkipFailedPool(t *testing.T) { t.Fatalf("first page HTTP=%d body=%s", first.Code, first.Body.String()) } firstToken := page.NextContinuationToken - brokenDisks := broken.getDisks() old := make([][]byte, 3) for i := range old { old[i] = mustReadNullQuorumMeta(t, brokenDisks[i], bucket, "e") @@ -154,14 +155,14 @@ func TestListingMergeDoesNotSkipFailedPool(t *testing.T) { brokenWalkers[i] = &failedMetadataWalkDisk{StorageAPI: brokenWalkers[i]} } } - broken.getDisks = func() []StorageAPI { return brokenWalkers } + switchBrokenDisks(brokenWalkers) resume := make(chan struct{}) defer close(resume) - healthyWalkers := slices.Clone(healthy.getDisks()) + healthyWalkers := slices.Clone(healthyDisks) for i, disk := range healthyWalkers { healthyWalkers[i] = &listingPausedTailDisk{StorageAPI: disk, bucket: bucket, tail: "zz", resume: resume, blocked: &blocked, active: &active} } - healthy.getDisks = func() []StorageAPI { return healthyWalkers } + switchHealthyDisks(healthyWalkers) target := getListObjectsV2URL("", bucket, "", "2", "", "", "") + "&continuation-token=" + url.QueryEscape(firstToken) req, err := newTestSignedRequestV4(http.MethodGet, target, 0, nil, globalActiveCred.AccessKey, globalActiveCred.SecretKey, nil) diff --git a/cmd/metacache-null-quorum_test.go b/cmd/metacache-null-quorum_test.go index 8f1cc002c..fb0b18e5f 100644 --- a/cmd/metacache-null-quorum_test.go +++ b/cmd/metacache-null-quorum_test.go @@ -596,7 +596,7 @@ func TestListFallbackDoesNotTrustNotFound(t *testing.T) { func TestListCrossParityMinorityStaysVisible(t *testing.T) { z, bucket, router := nullQuorumBackend(t) set := z.serverPools[0].sets[0] - disks := set.getDisks() + disks, switchDisks := installListingDiskSwitch(set) const object = "object" mtime := time.Now().UTC().Add(-time.Hour) @@ -619,7 +619,7 @@ func TestListCrossParityMinorityStaysVisible(t *testing.T) { for i := 12; i < len(online); i++ { online[i] = &offlineNullQuorumDisk{StorageAPI: disks[i]} } - set.getDisks = func() []StorageAPI { return online } + switchDisks(online) if code := nullQuorumRequest(t, router, http.MethodHead, getHeadObjectURL("", bucket, object)).Code; code == http.StatusNotFound { t.Errorf("HEAD with a node away: 404 for an existing object") } @@ -627,7 +627,7 @@ func TestListCrossParityMinorityStaysVisible(t *testing.T) { t.Errorf("ListObjectsV2 with a node away: got %d %v; want 200 [%s]", code, keys, object) } - set.getDisks = func() []StorageAPI { return disks } + switchDisks(disks) if get := nullQuorumRequest(t, router, http.MethodGet, getGetObjectURL("", bucket, object)); get.Code != http.StatusOK || !bytes.Equal(get.Body.Bytes(), g1Body) { t.Fatalf("GET after the node returns: %d", get.Code) } diff --git a/cmd/metacache-server-pool.go b/cmd/metacache-server-pool.go index 11bbb80a8..6510aaeba 100644 --- a/cmd/metacache-server-pool.go +++ b/cmd/metacache-server-pool.go @@ -191,19 +191,19 @@ func (z *erasureServerPools) listPath(ctx context.Context, o *listPathOptions) ( return entries, err } entries.truncate(0) - go func() { - rpc := globalNotificationSys.restClientFromHash(pathJoin(o.Bucket, o.Prefix)) + rpc := globalNotificationSys.restClientFromHash(pathJoin(o.Bucket, o.Prefix)) + go func(o listPathOptions) { if rpc != nil { ctx, cancel := context.WithTimeout(GlobalContext, 5*time.Second) defer cancel() - c, err := rpc.GetMetacacheListing(ctx, *o) + c, err := rpc.GetMetacacheListing(ctx, o) if err == nil { c.error = "no longer used" c.status = scanStateError rpc.UpdateMetacacheListing(ctx, *c) } } - }() + }(*o) o.ID = "" } From 87c1c6b3dc189799912f748572364e319676a7fc Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sat, 17 Oct 2026 11:18:08 +0800 Subject: [PATCH 4/4] 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: + } } } }