From 8e09c89e241a2e1cc448a4ad87aba8e0878cb12d Mon Sep 17 00:00:00 2001 From: s1d3sw1ped_bot <12+s1d3sw1ped_bot@git.s1d3sw1ped.com> Date: Tue, 1 Sep 2026 20:30:40 +0000 Subject: [PATCH] vfs/cache: Fix promoteToFast race with WriteCloser Close promoteToFast read FileInfo.Size from Stat after the VFS lock was released, while WriteCloser.Close updated Size on the same live object. Concurrent Create/Open on overlapping keys tripped the race detector. Stat now returns a FileInfo snapshot, and promotion uses the ReadAll length for the fast-tier Create size. Promotion stays best-effort. https://git.s1d3sw1ped.com/s1d3sw1ped/steamcache2/issues/21 --- vfs/cache/cache.go | 6 ++++-- vfs/disk/disk.go | 13 +++++++++---- vfs/memory/memory.go | 5 +++-- vfs/memory/memory_test.go | 39 +++++++++++++++++++++++++++++++++++++++ vfs/types/types.go | 11 +++++++++++ vfs/types/types_test.go | 28 ++++++++++++++++++++++++++++ 6 files changed, 94 insertions(+), 8 deletions(-) diff --git a/vfs/cache/cache.go b/vfs/cache/cache.go index 931b752..6c07617 100644 --- a/vfs/cache/cache.go +++ b/vfs/cache/cache.go @@ -186,7 +186,7 @@ func (tc *TieredCache) Capacity() int64 { func (tc *TieredCache) promoteToFast(key string, reader io.ReadCloser) { defer func() { _ = reader.Close() }() // best-effort close; error secondary to promotion attempt (async best-effort path) - // Get file info from slow tier to determine size + // Size for the space/ReadAll guards comes from a Stat snapshot, not the live in-map FileInfo. var size int64 if slow := tc.slow.Load(); slow != nil { if vfs, ok := slow.(vfs.VFS); ok { @@ -210,7 +210,7 @@ func (tc *TieredCache) promoteToFast(key string, reader io.ReadCloser) { } // Guard promotion ReadAll using already-fetched size (in addition to space check above) - if size > 0 && size > (1<<30) { // conservative 1GB hard limit on promotion reads (aligns with typical max_object_size) + if size > (1 << 30) { // conservative 1GB hard limit on promotion reads (aligns with typical max_object_size) return } // Read the entire file content @@ -218,6 +218,8 @@ func (tc *TieredCache) promoteToFast(key string, reader io.ReadCloser) { if err != nil { return // Skip promotion if read fails } + // Create with the bytes we actually hold so we never reuse a live FileInfo.Size. + size = int64(len(content)) // Create the file in fast tier if fast := tc.fast.Load(); fast != nil { diff --git a/vfs/disk/disk.go b/vfs/disk/disk.go index 35e4cba..3614142 100644 --- a/vfs/disk/disk.go +++ b/vfs/disk/disk.go @@ -612,7 +612,9 @@ func (d *DiskFS) Delete(key string) error { return nil } -// Stat returns file information with lazy discovery +// Stat returns a snapshot of file information with lazy discovery. +// The returned *FileInfo is not the live cache entry; Close may update Size +// on the in-map object under d.mu. func (d *DiskFS) Stat(key string) (*vfs.FileInfo, error) { if key == "" { return nil, vfserror.ErrInvalidKey @@ -627,9 +629,10 @@ func (d *DiskFS) Stat(key string) (*vfs.FileInfo, error) { keyMu.RLock() d.mu.RLock() if fi, ok := d.info[key]; ok { + snap := fi.Clone() d.mu.RUnlock() keyMu.RUnlock() - return fi, nil + return snap, nil } d.mu.RUnlock() keyMu.RUnlock() @@ -649,8 +652,9 @@ func (d *DiskFS) Stat(key string) (*vfs.FileInfo, error) { // Double-check after acquiring write lock d.mu.Lock() if fi, ok := d.info[key]; ok { + snap := fi.Clone() d.mu.Unlock() - return fi, nil + return snap, nil } // Re-verify the file still exists on disk under the lock before inserting. @@ -669,9 +673,10 @@ func (d *DiskFS) Stat(key string) (*vfs.FileInfo, error) { fi.UpdateAccessBatched(d.timeUpdater) // Note: size not updated on lazy discovery (preserves prior behavior; initial on-disk accounted via bg populate at New time, // subsequent files come via Create which accounts size). + snap := fi.Clone() d.mu.Unlock() - return fi, nil + return snap, nil } // EvictLRU evicts the least recently used files to free up space diff --git a/vfs/memory/memory.go b/vfs/memory/memory.go index 3117349..0ce6ac7 100644 --- a/vfs/memory/memory.go +++ b/vfs/memory/memory.go @@ -289,7 +289,8 @@ func (m *MemoryFS) Delete(key string) error { return nil } -// Stat returns file information +// Stat returns a snapshot of file information. The returned *FileInfo is not +// the live cache entry; Close may update Size on the in-map object under m.mu. func (m *MemoryFS) Stat(key string) (*types.FileInfo, error) { if key == "" { return nil, vfserror.ErrInvalidKey @@ -310,7 +311,7 @@ func (m *MemoryFS) Stat(key string) (*types.FileInfo, error) { defer m.mu.RUnlock() if fi, ok := m.info[key]; ok { - return fi, nil + return fi.Clone(), nil } return nil, vfserror.ErrNotFound diff --git a/vfs/memory/memory_test.go b/vfs/memory/memory_test.go index 410ea4d..0c3d730 100644 --- a/vfs/memory/memory_test.go +++ b/vfs/memory/memory_test.go @@ -346,6 +346,45 @@ func TestMemoryFS_ConcurrentCloseAndEvict_RaceFree(t *testing.T) { _ = m.LRU.Len() } +func TestMemoryFS_StatReturnsSnapshot(t *testing.T) { + t.Parallel() + m, err := New(1024) + if err != nil { + t.Fatal(err) + } + w, err := m.Create("k", 10) + if err != nil { + t.Fatal(err) + } + if _, err := w.Write([]byte("hello")); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + + fi, err := m.Stat("k") + if err != nil { + t.Fatal(err) + } + if fi.Size != 5 { + t.Fatalf("size %d want 5", fi.Size) + } + fi.Size = 999 + fi.AccessCount = 0 + + fi2, err := m.Stat("k") + if err != nil { + t.Fatal(err) + } + if fi2.Size != 5 { + t.Errorf("Stat returned live FileInfo; store size became %d", fi2.Size) + } + if fi2.AccessCount == 0 { + t.Error("Stat returned live FileInfo; AccessCount mutation leaked") + } +} + func TestMemoryFS_EvictVariantsAndErrors(t *testing.T) { t.Parallel() m, err := New(800) diff --git a/vfs/types/types.go b/vfs/types/types.go index 339a4e7..76b3af0 100644 --- a/vfs/types/types.go +++ b/vfs/types/types.go @@ -27,6 +27,17 @@ func NewFileInfo(key string, size int64) *FileInfo { } } +// Clone returns a snapshot copy of fi. Stat returns Clone() so callers can +// read Size and other fields without racing Close/Open mutations of the +// in-map FileInfo. +func (fi *FileInfo) Clone() *FileInfo { + if fi == nil { + return nil + } + cp := *fi + return &cp +} + // NewFileInfoFromOS creates a FileInfo from os.FileInfo func NewFileInfoFromOS(info os.FileInfo, key string) *FileInfo { return &FileInfo{ diff --git a/vfs/types/types_test.go b/vfs/types/types_test.go index ead7824..b444060 100644 --- a/vfs/types/types_test.go +++ b/vfs/types/types_test.go @@ -16,6 +16,34 @@ func TestNewFileInfo(t *testing.T) { } } +func TestFileInfoClone(t *testing.T) { + t.Parallel() + fi := NewFileInfo("k", 42) + fi.AccessCount = 7 + cp := fi.Clone() + if cp == fi { + t.Fatal("Clone returned the same pointer") + } + if cp.Key != fi.Key || cp.Size != fi.Size || cp.AccessCount != fi.AccessCount { + t.Errorf("Clone mismatch: %+v vs %+v", cp, fi) + } + if !cp.ATime.Equal(fi.ATime) || !cp.CTime.Equal(fi.CTime) { + t.Error("Clone timestamps mismatch") + } + cp.Size = 99 + cp.AccessCount = 1 + if fi.Size != 42 || fi.AccessCount != 7 { + t.Error("mutating Clone affected original") + } + if NewFileInfo("x", 1).Clone() == nil { + t.Error("Clone of non-nil was nil") + } + var none *FileInfo + if none.Clone() != nil { + t.Error("Clone of nil was non-nil") + } +} + func TestUpdateAccess(t *testing.T) { t.Parallel() fi := NewFileInfo("k", 1)