vfs/disk: Fix EvictDiskVisibilityAndRecreateSafety flake

New() launches background calculateSizeAndPopulateIndex which scans disk and calls insertBatch. Create does not wait on initDone, so a file can be written and indexed, discovered by the scan, then removed from d.info and disk by EvictLRU/EvictBySize. insertBatch then re-inserted the stale discoveredFile without checking the path still existed, so Stat succeeded from the index while os.Stat failed. Re-stat under the lock and skip gone files so evicted keys are not resurrected. Fixes #18.
2026-09-01 20:19:14 +00:00
parent 0198e8990b
commit 30a695458e
2 changed files with 89 additions and 46 deletions
@@ -248,15 +248,25 @@ func (d *DiskFS) calculateSizeAndPopulateIndex() {

 // insertBatch populates info/LRU under lock for a bounded batch (follows maxEvictBatch pattern for short critical sections).
 // Size is incremented here only for files actually added (prevents double-count vs. concurrent Create during window).
+// Fail-closed: re-stat each path under d.mu and skip if the file is gone. Create does not wait on
+// initDone, so a file the scanner observed can be Evict/Delete'd (info + os.Remove) before this
+// insert runs. Inserting without a live-file check would resurrect the key in d.info and make
+// Stat succeed while os.Stat fails.
 func (d *DiskFS) insertBatch(batch []discoveredFile) {
 	d.mu.Lock()
 	for _, df := range batch {
-		if _, exists := d.info[df.key]; !exists {
-			fi := vfs.NewFileInfoFromOS(df.osInfo, df.key)
-			d.info[df.key] = fi
-			d.LRU.Add(df.key, fi)
-			d.size += df.size
+		if _, exists := d.info[df.key]; exists {
+			continue
 		}
+		path := d.pathForKey(df.key)
+		st, err := os.Stat(path)
+		if err != nil {
+			continue
+		}
+		fi := vfs.NewFileInfoFromOS(st, df.key)
+		d.info[df.key] = fi
+		d.LRU.Add(df.key, fi)
+		d.size += st.Size()
 	}
 	d.mu.Unlock()
 }
@@ -371,7 +371,8 @@ func testKey(i int) string {
 // artifacts for victims are immediately gone (no resurrection via lazy discovery in Stat/Open),
 // and that recreating the same key produces independent content that is not subject to any
 // stale eviction unlinks. This exercises the coordinated WLock remove path for DiskFS.
-// Uses tolerant checks suitable for raw DiskFS lazy discovery + bg size.
+// Create does not wait on initDone, so this also covers insertBatch racing with eviction:
+// gone files must not be re-indexed (Stat present / disk missing).
 func TestDiskFS_EvictDiskVisibilityAndRecreateSafety(t *testing.T) {
 	t.Parallel()
 	td := t.TempDir()
@@ -400,47 +401,21 @@ func TestDiskFS_EvictDiskVisibilityAndRecreateSafety(t *testing.T) {
 		_ = d.EvictBySize(1024*1024, true)
 	}

-	// Consistency check: never have a key absent from Stat but with a file on disk (would indicate
-	// either resurrection risk or orphan). If Stat succeeds, file should exist.
-	// A few retries tolerate the documented lazy discovery + eviction coordination windows under
-	// artificial "force massive eviction then immediate audit" load (especially visible under -race).
-	for attempt := 0; attempt < 3; attempt++ {
-		bad := false
-		for _, k := range created {
-			p := d.pathForKey(k)
-			_, statErr := d.Stat(k)
-			_, diskErr := os.Stat(p)
-			if statErr != nil {
-				if !os.IsNotExist(diskErr) {
-					bad = true
-				}
-			} else {
-				if diskErr != nil {
-					bad = true
-				}
-			}
-		}
-		if !bad {
-			break
-		}
-		if attempt < 2 {
-			time.Sleep(10 * time.Millisecond)
-		} else {
-			// On final attempt, report the last observed state for the keys
-			for _, k := range created {
-				p := d.pathForKey(k)
-				_, statErr := d.Stat(k)
-				_, diskErr := os.Stat(p)
-				if statErr != nil {
-					if !os.IsNotExist(diskErr) {
-						t.Errorf("key %s absent via Stat but file lingers on disk at %s (resurrection risk)", k, p)
-					}
-				} else {
-					if diskErr != nil {
-						t.Errorf("key %s present via Stat but missing on disk: %v", k, diskErr)
-					}
-				}
+	// Drain bg population so insertBatch cannot still be in flight when we audit.
+	_ = d.Size()
+
+	// Consistency: Stat success iff the file exists on disk. insertBatch must not resurrect
+	// keys whose backing files were already evicted.
+	for _, k := range created {
+		p := d.pathForKey(k)
+		_, statErr := d.Stat(k)
+		_, diskErr := os.Stat(p)
+		if statErr != nil {
+			if !os.IsNotExist(diskErr) {
+				t.Errorf("key %s absent via Stat but file lingers on disk at %s (resurrection risk)", k, p)
 			}
+		} else if diskErr != nil {
+			t.Errorf("key %s present via Stat but missing on disk: %v", k, diskErr)
 		}
 	}

@@ -464,6 +439,64 @@ func TestDiskFS_EvictDiskVisibilityAndRecreateSafety(t *testing.T) {
 	}
 }

+// TestDiskFS_InsertBatchSkipsGoneFiles is the fail-closed contract for bg/lazy index
+// insert: a discoveredFile whose path was removed (evicted) must not be re-inserted
+// into d.info. That resurrection is what made Stat succeed while os.Stat failed.
+func TestDiskFS_InsertBatchSkipsGoneFiles(t *testing.T) {
+	t.Parallel()
+	td := t.TempDir()
+	d, err := New(td, 10*1024*1024, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_ = d.Size() // finish constructor scan so it cannot also index these keys
+
+	liveKey := "live"
+	goneKey := "gone"
+	writeKey := func(key, body string) os.FileInfo {
+		t.Helper()
+		p := d.pathForKey(key)
+		if err := os.MkdirAll(filepath.Dir(p), 0700); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(p, []byte(body), 0600); err != nil {
+			t.Fatal(err)
+		}
+		st, err := os.Stat(p)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return st
+	}
+	liveInfo := writeKey(liveKey, "still-here")
+	goneInfo := writeKey(goneKey, "about-to-vanish")
+	if err := os.Remove(d.pathForKey(goneKey)); err != nil {
+		t.Fatal(err)
+	}
+
+	d.insertBatch([]discoveredFile{
+		{key: liveKey, size: liveInfo.Size(), osInfo: liveInfo},
+		{key: goneKey, size: goneInfo.Size(), osInfo: goneInfo},
+	})
+
+	d.mu.RLock()
+	_, liveExists := d.info[liveKey]
+	_, goneExists := d.info[goneKey]
+	d.mu.RUnlock()
+	if !liveExists {
+		t.Errorf("insertBatch skipped live key %s", liveKey)
+	}
+	if goneExists {
+		t.Errorf("insertBatch resurrected gone key %s", goneKey)
+	}
+	if _, err := d.Stat(goneKey); err == nil {
+		t.Errorf("Stat succeeded for gone key %s", goneKey)
+	}
+	if _, err := os.Stat(d.pathForKey(liveKey)); err != nil {
+		t.Errorf("live key %s missing on disk: %v", liveKey, err)
+	}
+}
+
 // TestDiskFS_EvictBoundedLargeN exercises the maxEvictBatch early-break logic (Idea #2)
 // under a map size >> batch limit. Forces repeated eviction rounds via GC-style pressure
 // and asserts progress + consistency (no resurrection/orphans). Covers bounded collection