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. #21
This commit is contained in:
Vendored
+4
-2
@@ -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 {
|
||||
|
||||
+9
-4
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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{
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user