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/2] 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/2] 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 }