From d1b1338558d00c0124568d0db19b674949fbb558 Mon Sep 17 00:00:00 2001 From: os-pradipbabar Date: Thu, 25 Jun 2026 05:01:32 +0530 Subject: [PATCH] Fix stale cache fallback for empty volume locations in wdclient (#10081) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix(wdclient): prevent stale cache fallback for empty volume locations ## Problem During Kubernetes pod restarts, volume servers temporarily disconnect and their locations are removed from vidMap. The deleteLocation function leaves an empty array [] in vid2Locations map instead of removing the key entirely. GetLocations() was checking 'if found && len(locations) > 0', which would fail for empty arrays and fall back to the cache chain, returning STALE locations from before the restart. This caused S3 gateway to try connecting to old pod IPs that no longer exist, resulting in connection timeouts and hanging registry sync jobs. Example timeline: 1. Volume pod at 10.131.1.28:8081 registers volumes 10,12 2. S3 gateway caches: vid2Locations[10] = [10.131.1.28:8081] 3. Pod restarts, gets new IP 10.131.1.65:8081 4. Master sends delete → vid2Locations[10] = [] (empty, but key exists) 5. BUG: GetLocations(10) sees found=true, len=0 → falls back to cache 6. Returns stale 10.131.1.28:8081 instead of waiting for new location 7. S3 requests timeout trying to reach unreachable old IP ## Solution Distinguish between two cases: - found=true, locations=[] : Volume explicitly has no locations (e.g. restart) → Return nil, false (no fallback to cache) - found=false : Volume never seen in current map → Check cache (preserve cache benefits for unknown volumes) An empty array explicitly means 'this volume currently has no locations', which is semantically different from 'volume unknown'. Don't fall back to stale cache for explicitly empty volumes. ## Testing Added comprehensive tests: - TestGetLocationsEmptyArrayNoFallback: Verifies empty arrays don't use cache - TestGetLocationsUnknownVolumeUsesCache: Verifies unknown volumes still use cache - All existing tests pass ## Impact Fixes registry sync job hangs during SeaweedFS upgrades/restarts. S3 gateway will now correctly wait for updated volume locations instead of using stale cached IPs. Related: OutSystems.SeaWeedfs Helm chart, vega cluster incident 2026-06-24 --- weed/wdclient/vid_map.go | 13 ++++- weed/wdclient/vidmap_invalidation_test.go | 71 +++++++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/weed/wdclient/vid_map.go b/weed/wdclient/vid_map.go index de5b09de1..648cb614a 100644 --- a/weed/wdclient/vid_map.go +++ b/weed/wdclient/vid_map.go @@ -130,10 +130,19 @@ func (vc *vidMap) GetVidLocations(vid string) (locations []Location, err error) func (vc *vidMap) GetLocations(vid uint32) (locations []Location, found bool) { // glog.V(4).Infof("~ lookup volume id %d: %+v ec:%+v", vid, vc.vid2Locations, vc.ecVid2Locations) locations, found = vc.getLocations(vid) - if found && len(locations) > 0 { - return locations, found + if found { + // If volume is explicitly tracked (found=true), return its locations even if empty. + // An empty array means "volume has no locations" (e.g., during pod restart), + // which is different from "volume never existed" (found=false). + // Don't fall back to stale cache for explicitly empty volumes. + if len(locations) > 0 { + return locations, found + } + // Volume exists but has no locations - return empty, don't check cache + return nil, false } + // Volume not found in current map - check cache for unknown volumes if cachedMap := vc.cache.Load(); cachedMap != nil { return cachedMap.GetLocations(vid) } diff --git a/weed/wdclient/vidmap_invalidation_test.go b/weed/wdclient/vidmap_invalidation_test.go index e2a544d41..0d25de975 100644 --- a/weed/wdclient/vidmap_invalidation_test.go +++ b/weed/wdclient/vidmap_invalidation_test.go @@ -205,3 +205,74 @@ func TestDeleteVidRecursion(t *testing.T) { t.Error("Expected vid to be removed from vm1 (cascaded)") } } + +// TestGetLocationsEmptyArrayNoFallback tests that empty location arrays don't fall back to cache +// This tests the fix for the bug where volume pods restart and vidMap has empty array [], +// but GetLocations would fall back to stale cached locations from before restart. +func TestGetLocationsEmptyArrayNoFallback(t *testing.T) { + // Setup: Create vidMap with cache + currentMap := newVidMap("") + vid := uint32(10) + oldLocation := Location{Url: "10.131.1.28:8081"} + newLocation := Location{Url: "10.131.1.65:8081"} + + // Scenario: Volume initially has old location + currentMap.addLocation(vid, oldLocation) + locs, found := currentMap.GetLocations(vid) + if !found || len(locs) != 1 || locs[0].Url != oldLocation.Url { + t.Fatalf("Expected to find old location, got found=%v locs=%v", found, locs) + } + + // Create cache chain with old location + cachedMap := newVidMap("") + cachedMap.addLocation(vid, oldLocation) + currentMap.cache.Store(cachedMap) + + // Simulate: Volume server restarts, old location is deleted + currentMap.deleteLocation(vid, oldLocation) + + // BUG: At this point vid2Locations[vid] = [] (empty array, key exists) + // OLD BEHAVIOR: GetLocations would see found=true, len=0 and fall back to cache + // returning stale oldLocation + // NEW BEHAVIOR: GetLocations should return nil, false (no locations available) + + locs, found = currentMap.GetLocations(vid) + if found { + t.Errorf("Expected found=false for empty location array, got found=true with locs=%v", locs) + } + if locs != nil { + t.Errorf("Expected nil locations for empty array, got %v (should not fall back to stale cache!)", locs) + } + + // Verify: When new location is added, it should be returned (not stale cache) + currentMap.addLocation(vid, newLocation) + locs, found = currentMap.GetLocations(vid) + if !found || len(locs) != 1 { + t.Fatalf("Expected to find new location, got found=%v locs=%v", found, locs) + } + if locs[0].Url != newLocation.Url { + t.Errorf("Expected new location %s, got %s (got stale cache!)", newLocation.Url, locs[0].Url) + } +} + +// TestGetLocationsUnknownVolumeUsesCache tests that truly unknown volumes still use cache +func TestGetLocationsUnknownVolumeUsesCache(t *testing.T) { + // Setup: Current map doesn't know about volume, but cache does + currentMap := newVidMap("") + cachedMap := newVidMap("") + vid := uint32(99) + cachedLocation := Location{Url: "cache-server:8081"} + + cachedMap.addLocation(vid, cachedLocation) + currentMap.cache.Store(cachedMap) + + // Volume 99 is completely unknown to currentMap (not in vid2Locations) + // This should fall back to cache + locs, found := currentMap.GetLocations(vid) + if !found || len(locs) != 1 { + t.Fatalf("Expected to find cached location for unknown volume, got found=%v locs=%v", found, locs) + } + if locs[0].Url != cachedLocation.Url { + t.Errorf("Expected cached location %s, got %s", cachedLocation.Url, locs[0].Url) + } +}