From 9115a84e1b2b0b3582caa4d2deca40e9c807ff34 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 8 May 2026 03:04:48 -0600 Subject: [PATCH 1/4] Remove no-cache-dirs (Copilot) --- component/attr_cache/attr_cache.go | 176 +++++----------- component/attr_cache/attr_cache_test.go | 269 +----------------------- setup/advancedConfig.yaml | 1 - setup/baseConfig.yaml | 1 - 4 files changed, 52 insertions(+), 395 deletions(-) diff --git a/component/attr_cache/attr_cache.go b/component/attr_cache/attr_cache.go index afa5e1a06..31f1e8322 100644 --- a/component/attr_cache/attr_cache.go +++ b/component/attr_cache/attr_cache.go @@ -51,7 +51,6 @@ type AttrCache struct { cacheTimeout uint32 cacheOnList bool enableSymlinks bool - cacheDirs bool maxFiles int cache *cacheTreeMap cacheLock sync.RWMutex @@ -65,7 +64,6 @@ type AttrCacheOptions struct { Timeout uint32 `config:"timeout-sec" yaml:"timeout-sec,omitempty"` NoCacheOnList bool `config:"no-cache-on-list" yaml:"no-cache-on-list,omitempty"` EnableSymlinks bool `config:"enable-symlinks" yaml:"enable-symlinks,omitempty"` - NoCacheDirs bool `config:"no-cache-dirs" yaml:"no-cache-dirs,omitempty"` // hidden option for backward compatibility NoSymlinks bool `config:"no-symlinks" yaml:"no-symlinks,omitempty"` @@ -180,8 +178,6 @@ func (ac *AttrCache) Configure(_ bool) error { ac.enableSymlinks = conf.EnableSymlinks } - ac.cacheDirs = !conf.NoCacheDirs - log.Crit( "AttrCache::Configure : cache-timeout %d, enable-symlinks %t, cache-on-list %t, max-files %d", ac.cacheTimeout, @@ -208,33 +204,14 @@ func (ac *AttrCache) deleteDirectory(path string, deletedAt time.Time) error { // handle errors and unexpected behavior dirExists := found && item.exists() if !dirExists { - if ac.cacheDirs { - // when cacheDirs is true, deleting a non-existent directory should return ENOENT - log.Err("AttrCache::deleteDirectory : %s does not exist", path) - return syscall.ENOENT - } else { - // when cacheDirs is false, attr_cache is not responsible for returning ENOENT - // just log a warning for this unexpected behavior - log.Warn("AttrCache::deleteDirectory : %s directory does not exist", path) - // if not already done, record the fact that the directory has been deleted - if !found { - log.Info("AttrCache::deleteDirectory : %s recording directory as deleted", path) - ac.cache.insert(insertOptions{ - attr: internal.CreateObjAttrDir(path), - exists: false, - cachedAt: deletedAt, - }) - } - return nil - } + log.Err("AttrCache::deleteDirectory : %s does not exist", path) + return syscall.ENOENT } // record that the entry and all its children have been deleted item.markDeleted(deletedAt) - if ac.cacheDirs { - // update whether cloud storage has any record of the parent directory's existence - ac.updateAncestorsInCloud(getParentDir(path), deletedAt) - } + // update whether cloud storage has any record of the parent directory's existence + ac.updateAncestorsInCloud(getParentDir(path), deletedAt) return nil } @@ -257,8 +234,7 @@ func getParentDir(childPath string) string { return parentDir } -// mark the directory and all its contents invalid -// only use when cacheDirs=false +// mark file entries under a directory invalid func (ac *AttrCache) invalidateDirectory(path string) { item, found := ac.cache.get(path) if !found || !item.valid() { @@ -266,24 +242,13 @@ func (ac *AttrCache) invalidateDirectory(path string) { return } - // only invalidate directories when cacheDirs is false - if ac.cacheDirs { - // invalidating anything when cacheDirs=true is risky - // TODO: should we do nothing here? - // let's compromise: recursively invalidate only file items - for _, childItem := range item.children { - if !childItem.attr.IsDir() { - childItem.invalidate() - } else { - ac.invalidateDirectory(childItem.attr.Path) - } + for _, childItem := range item.children { + if !childItem.attr.IsDir() { + childItem.invalidate() + } else { + ac.invalidateDirectory(childItem.attr.Path) } - } else { - // invalidate the whole directory, recursively - item.invalidate() - return } - } // move an item to a new location, and return the destination item @@ -361,14 +326,10 @@ func (ac *AttrCache) markAncestorsInCloud(dirPath string, time time.Time) { func (ac *AttrCache) touchParentDirTimes( childPath string, touchedAt time.Time, - createIfMissing bool, ) { parentPath := getParentDir(childPath) parentItem, found := ac.cache.get(parentPath) if !found || !parentItem.exists() { - if !createIfMissing { - return - } parentAttr := internal.CreateObjAttrDir(parentPath) parentAttr.Ctime = touchedAt parentAttr.Mtime = touchedAt @@ -456,8 +417,7 @@ func (ac *AttrCache) cleanupExpiredEntries() { } // ------------------------- Methods implemented by this component ------------------------------------------- -// CreateDir: Mark the directory invalid, or -// insert the dir item into cache when cacheDirs is true. +// CreateDir: Insert the directory item into cache. func (ac *AttrCache) CreateDir(options internal.CreateDirOptions) error { log.Trace("AttrCache::CreateDir : %s", options.Name) err := ac.NextComponent().CreateDir(options) @@ -468,9 +428,7 @@ func (ac *AttrCache) CreateDir(options internal.CreateDirOptions) error { // does the directory already exist? oldDirAttrCacheItem, found := ac.cache.get(options.Name) directoryAlreadyExists := found && oldDirAttrCacheItem.exists() - // if the attribute cache tracks directory existence - // then prevent redundant directory creation - if ac.cacheDirs && directoryAlreadyExists { + if directoryAlreadyExists { return os.ErrExist } // invalidate existing directory entry (this is redundant but readable) @@ -488,11 +446,11 @@ func (ac *AttrCache) CreateDir(options internal.CreateDirOptions) error { newDirAttrCacheItem.setMode(options.Mode) } // update flags for tracking directory existence - if ac.cacheDirs && newDirAttrCacheItem != nil { + if newDirAttrCacheItem != nil { newDirAttrCacheItem.markInCloud(false) } if err == nil && !directoryAlreadyExists { - ac.touchParentDirTimes(options.Name, currentTime, ac.cacheDirs) + ac.touchParentDirTimes(options.Name, currentTime) } } return err @@ -512,7 +470,7 @@ func (ac *AttrCache) DeleteDir(options internal.DeleteDirOptions) error { defer ac.cacheLock.Unlock() err = ac.deleteDirectory(options.Name, deletionTime) if err == nil { - ac.touchParentDirTimes(options.Name, deletionTime, ac.cacheDirs) + ac.touchParentDirTimes(options.Name, deletionTime) } } @@ -562,21 +520,18 @@ func (ac *AttrCache) StreamDir( options.Name, len(pathList), nextToken) // cache returned list ac.cacheAttributes(pathList, options.Name) - // - if ac.cacheDirs { - // remember that this directory is in cloud storage - if len(pathList) > 0 { - ac.cacheLock.Lock() - ac.markAncestorsInCloud(options.Name, time.Now()) - ac.cacheLock.Unlock() - } - // merge missing directory cache into the last page of results - if ac.cacheDirs && nextToken == "" { - var numAdded int // prevent shadowing pathList in following line - pathList, numAdded = ac.addDirsNotInCloudToListing(options.Name, pathList) - log.Info("AttrCache::StreamDir : %s +%d from cache = %d", - options.Name, numAdded, len(pathList)) - } + // remember that this directory is in cloud storage + if len(pathList) > 0 { + ac.cacheLock.Lock() + ac.markAncestorsInCloud(options.Name, time.Now()) + ac.cacheLock.Unlock() + } + // merge missing directory cache into the last page of results + if nextToken == "" { + var numAdded int // prevent shadowing pathList in following line + pathList, numAdded = ac.addDirsNotInCloudToListing(options.Name, pathList) + log.Info("AttrCache::StreamDir : %s +%d from cache = %d", + options.Name, numAdded, len(pathList)) } } // add cached items in @@ -731,14 +686,6 @@ func (ac *AttrCache) cacheListSegment( func (ac *AttrCache) IsDirEmpty(options internal.IsDirEmptyOptions) bool { log.Trace("AttrCache::IsDirEmpty : %s", options.Name) - // This function only has a use if we're caching directories - if !ac.cacheDirs { - log.Debug( - "AttrCache::IsDirEmpty : %s Dir cache is disabled. Checking with container", - options.Name, - ) - return ac.NextComponent().IsDirEmpty(options) - } // Is the directory in our cache? ac.cacheLock.RLock() pathInCache := ac.pathExistsInCache(options.Name) @@ -793,35 +740,24 @@ func (ac *AttrCache) RenameDir(options internal.RenameDirOptions) error { defer ac.cacheLock.Unlock() // check if destination already exists in cache - if ac.cacheDirs { - // if attr_cache is tracking directories, validate this rename - // First, check if the destination directory already exists - if ac.pathExistsInCache(options.Dst) { - return os.ErrExist - } - } else { - // TLDR: Dst is guaranteed to be non-existent or empty. - // Note: We do not need to invalidate children of Dst due to the logic in our FUSE connector, see comments there, - // but it is always safer to double check than not. - ac.invalidateDirectory(options.Dst) + if ac.pathExistsInCache(options.Dst) { + return os.ErrExist } // get the source directory srcItem, found := ac.cache.get(options.Src) if !found || !srcItem.exists() { log.Err("AttrCache::RenameDir : %s source not found", options.Src) - if ac.cacheDirs { - return syscall.ENOENT - } + return syscall.ENOENT } else { // move everything over srcDir := internal.TruncateDirName(options.Src) dstDir := internal.TruncateDirName(options.Dst) ac.moveCachedItem(srcItem, srcDir, dstDir, currentTime) } - ac.touchParentDirTimes(options.Src, currentTime, ac.cacheDirs) + ac.touchParentDirTimes(options.Src, currentTime) if getParentDir(options.Src) != getParentDir(options.Dst) { - ac.touchParentDirTimes(options.Dst, currentTime, ac.cacheDirs) + ac.touchParentDirTimes(options.Dst, currentTime) } } @@ -839,10 +775,8 @@ func (ac *AttrCache) CreateFile(options internal.CreateFileOptions) (*handlemap. // They routinely lock the cache for reading, but then write to it ac.cacheLock.Lock() defer ac.cacheLock.Unlock() - if ac.cacheDirs { - // record that the parent directory tree contains at least one object - ac.markAncestorsInCloud(getParentDir(options.Name), currentTime) - } + // record that the parent directory tree contains at least one object + ac.markAncestorsInCloud(getParentDir(options.Name), currentTime) // add new entry newFileAttr := internal.CreateObjAttr(options.Name, 0, currentTime) newFileEntry := ac.cache.insert(insertOptions{ @@ -853,7 +787,7 @@ func (ac *AttrCache) CreateFile(options internal.CreateFileOptions) (*handlemap. if newFileEntry != nil { newFileEntry.setMode(options.Mode) } - ac.touchParentDirTimes(options.Name, currentTime, ac.cacheDirs) + ac.touchParentDirTimes(options.Name, currentTime) } return h, err @@ -874,9 +808,7 @@ func (ac *AttrCache) OpenFile(options internal.OpenFileOptions) (*handlemap.Hand if found && cacheItem.exists() { cacheItem.markDeleted(currentTime) } - if ac.cacheDirs { - ac.updateAncestorsInCloud(getParentDir(options.Name), currentTime) - } + ac.updateAncestorsInCloud(getParentDir(options.Name), currentTime) } return h, err @@ -906,10 +838,8 @@ func (ac *AttrCache) DeleteFile(options internal.DeleteFileOptions) error { }) } toBeDeleted.markDeleted(deletionTime) - if ac.cacheDirs { - ac.updateAncestorsInCloud(getParentDir(options.Name), deletionTime) - } - ac.touchParentDirTimes(options.Name, deletionTime, ac.cacheDirs) + ac.updateAncestorsInCloud(getParentDir(options.Name), deletionTime) + ac.touchParentDirTimes(options.Name, deletionTime) } return err @@ -968,14 +898,12 @@ func (ac *AttrCache) RenameFile(options internal.RenameFileOptions) error { // move source item to destination ac.moveCachedItem(sourceItem, options.Src, options.Dst, renameTime) - if ac.cacheDirs { - ac.updateAncestorsInCloud(getParentDir(options.Src), renameTime) - // mark the destination parent directory tree as containing objects - ac.markAncestorsInCloud(getParentDir(options.Dst), renameTime) - } - ac.touchParentDirTimes(options.Src, renameTime, ac.cacheDirs) + ac.updateAncestorsInCloud(getParentDir(options.Src), renameTime) + // mark the destination parent directory tree as containing objects + ac.markAncestorsInCloud(getParentDir(options.Dst), renameTime) + ac.touchParentDirTimes(options.Src, renameTime) if getParentDir(options.Src) != getParentDir(options.Dst) { - ac.touchParentDirTimes(options.Dst, renameTime, ac.cacheDirs) + ac.touchParentDirTimes(options.Dst, renameTime) } } return err @@ -1095,11 +1023,9 @@ func (ac *AttrCache) CopyFromFile(options internal.CopyFromFileOptions) error { ac.cacheLock.Lock() defer ac.cacheLock.Unlock() - if ac.cacheDirs { - // This call needs to be treated like it's creating a new file - // Mark ancestors as existing in cloud storage now - ac.markAncestorsInCloud(getParentDir(options.Name), uploadTime) - } + // This call needs to be treated like it's creating a new file + // Mark ancestors as existing in cloud storage now + ac.markAncestorsInCloud(getParentDir(options.Name), uploadTime) // use local file to update the attribute cache entry fileStat, statErr := options.File.Stat() @@ -1185,9 +1111,7 @@ func (ac *AttrCache) GetAttr(options internal.GetAttrOptions) (*internal.ObjAttr exists: true, cachedAt: time.Now(), }) - if ac.cacheDirs { - ac.markAncestorsInCloud(getParentDir(options.Name), time.Now()) - } + ac.markAncestorsInCloud(getParentDir(options.Name), time.Now()) case syscall.ENOENT: // cache this entity not existing ac.cache.insert(insertOptions{ @@ -1220,10 +1144,8 @@ func (ac *AttrCache) CreateLink(options internal.CreateLinkOptions) error { exists: true, cachedAt: currentTime, }) - if ac.cacheDirs { - ac.markAncestorsInCloud(getParentDir(options.Name), currentTime) - } - ac.touchParentDirTimes(options.Name, currentTime, ac.cacheDirs) + ac.markAncestorsInCloud(getParentDir(options.Name), currentTime) + ac.touchParentDirTimes(options.Name, currentTime) } return err diff --git a/component/attr_cache/attr_cache_test.go b/component/attr_cache/attr_cache_test.go index 7e876b60f..122f2eb0c 100644 --- a/component/attr_cache/attr_cache_test.go +++ b/component/attr_cache/attr_cache_test.go @@ -81,7 +81,7 @@ func getDirPathAttr(path string) *internal.ObjAttr { return objAttr } -func getPathAttr(path string, size int64, mode os.FileMode, metadata bool) *internal.ObjAttr { +func getPathAttr(path string, size int64, mode os.FileMode, _ bool) *internal.ObjAttr { flags := internal.NewFileBitMap() return &internal.ObjAttr{ Path: path, @@ -312,14 +312,13 @@ func (suite *attrCacheTestSuite) TestDefault() { suite.assert.EqualValues(120, suite.attrCache.cacheTimeout) suite.assert.True(suite.attrCache.cacheOnList) suite.assert.False(suite.attrCache.enableSymlinks) - suite.assert.True(suite.attrCache.cacheDirs) } // Tests configuration func (suite *attrCacheTestSuite) TestConfig() { defer suite.cleanupTest() suite.cleanupTest() // clean up the default attr cache generated - config := "attr_cache:\n timeout-sec: 60\n no-cache-on-list: true\n enable-symlinks: true\n no-cache-dirs: true" + config := "attr_cache:\n timeout-sec: 60\n no-cache-on-list: true\n enable-symlinks: true" suite.setupTestHelper( config, ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) @@ -328,7 +327,6 @@ func (suite *attrCacheTestSuite) TestConfig() { suite.assert.EqualValues(60, suite.attrCache.cacheTimeout) suite.assert.False(suite.attrCache.cacheOnList) suite.assert.True(suite.attrCache.enableSymlinks) - suite.assert.False(suite.attrCache.cacheDirs) } // Tests backward compatibility @@ -360,7 +358,7 @@ func (suite *attrCacheTestSuite) TestConfigMaxFiles() { func (suite *attrCacheTestSuite) TestConfigZero() { defer suite.cleanupTest() suite.cleanupTest() // clean up the default attr cache generated - config := "attr_cache:\n timeout-sec: 0\n no-cache-on-list: true\n enable-symlinks: true\n no-cache-dirs: true" + config := "attr_cache:\n timeout-sec: 0\n no-cache-on-list: true\n enable-symlinks: true" suite.setupTestHelper( config, ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) @@ -369,7 +367,6 @@ func (suite *attrCacheTestSuite) TestConfigZero() { suite.assert.EqualValues(0, suite.attrCache.cacheTimeout) suite.assert.False(suite.attrCache.cacheOnList) suite.assert.True(suite.attrCache.enableSymlinks) - suite.assert.False(suite.attrCache.cacheDirs) } // Tests Create Directory @@ -480,53 +477,6 @@ func (suite *attrCacheTestSuite) TestCreateDirExistingDoesNotUpdateParentTimes() suite.Equal(staleTime, updatedParent.attr.Mtime) } -// Tests Create Directory Without Caching Empty Directories -func (suite *attrCacheTestSuite) TestCreateDirNoCacheDirs() { - defer suite.cleanupTest() - var paths = []string{"a", "a/"} - - noCacheDirs := true - config := fmt.Sprintf("attr_cache:\n no-cache-dirs: %t", noCacheDirs) - - for _, path := range paths { - log.Debug("%s", path) - // This is a little janky but required since testify suite does not support running setup or clean up for subtests. - suite.cleanupTest() - suite.setupTestHelper( - config, - ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) - suite.assert.Equal(!noCacheDirs, suite.attrCache.cacheDirs) - suite.Run(path, func() { - truncatedPath := internal.TruncateDirName(path) - extendedPath := internal.ExtendDirName(path) - options := internal.CreateDirOptions{Name: path} - - // Error - suite.mock.EXPECT().CreateDir(options).Return(errors.New("Failed")) - - err := suite.attrCache.CreateDir(options) - suite.assert.Error(err) - suite.assertNotInCache(truncatedPath) - - // Success - // Entry Does Not Already Exist - suite.mock.EXPECT().CreateDir(options).Return(nil) - - err = suite.attrCache.CreateDir(options) - suite.assert.NoError(err) - suite.assertExists(truncatedPath) - - // Entry Already Exists - suite.addPathToCache(extendedPath, false) - suite.mock.EXPECT().CreateDir(options).Return(nil) - - err = suite.attrCache.CreateDir(options) - suite.assert.NoError(err) - suite.assertExists(truncatedPath) - }) - } -} - // Tests Delete Directory func (suite *attrCacheTestSuite) TestDeleteDir() { defer suite.cleanupTest() @@ -575,61 +525,6 @@ func (suite *attrCacheTestSuite) TestDeleteDir() { } } -// Tests Delete Directory Without Caching Empty Directories -func (suite *attrCacheTestSuite) TestDeleteDirNoCacheDirs() { - defer suite.cleanupTest() - var paths = []string{"a", "a/"} - - noCacheDirs := true - config := fmt.Sprintf("attr_cache:\n no-cache-dirs: %t", noCacheDirs) - - for _, path := range paths { - // This is a little janky but required since testify suite does not support running setup or clean up for subtests. - suite.cleanupTest() - suite.setupTestHelper( - config, - ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) - suite.assert.Equal(!noCacheDirs, suite.attrCache.cacheDirs) - suite.Run(path, func() { - truncatedPath := internal.TruncateDirName(path) - options := internal.DeleteDirOptions{Name: path} - - // Error - suite.mock.EXPECT().DeleteDir(options).Return(errors.New("Failed")) - - err := suite.attrCache.DeleteDir(options) - suite.assert.Error(err) - suite.assertNotInCache(truncatedPath) - - // Success - // Entry Does Not Already Exist - suite.mock.EXPECT().DeleteDir(options).Return(nil) - - err = suite.attrCache.DeleteDir(options) - suite.assert.NoError(err) - suite.assertDeleted(truncatedPath) - - // Entry Already Exists - a, ab, ac := suite.addDirectoryToCache(path, false) - - suite.mock.EXPECT().DeleteDir(options).Return(nil) - - err = suite.attrCache.DeleteDir(options) - suite.assert.NoError(err) - // a paths should be deleted - for p := a.Front(); p != nil; p = p.Next() { - truncatedPath = internal.TruncateDirName(p.Value.(string)) - suite.assertDeleted(truncatedPath) - } - ab.PushBackList(ac) // ab and ac paths should be untouched - for p := ab.Front(); p != nil; p = p.Next() { - truncatedPath = internal.TruncateDirName(p.Value.(string)) - suite.assertUntouched(truncatedPath) - } - }) - } -} - // Tests Stream Directory func (suite *attrCacheTestSuite) TestStreamDirDoesNotExist() { defer suite.cleanupTest() @@ -829,37 +724,6 @@ func (suite *attrCacheTestSuite) TestStreamDirNoCacheOnList() { suite.assertExists(path) } -func (suite *attrCacheTestSuite) TestStreamDirNoCacheOnListNoCacheDirs() { - defer suite.cleanupTest() - suite.cleanupTest() // clean up the default attr cache generated - cacheOnList := false - cacheDirs := false - config := fmt.Sprintf( - "attr_cache:\n no-cache-on-list: %t\n no-cache-dirs: %t", - !cacheOnList, - !cacheDirs, - ) - suite.setupTestHelper( - config, - ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) - suite.assert.Equal(cacheOnList, suite.attrCache.cacheOnList) - suite.assert.Equal(cacheDirs, suite.attrCache.cacheDirs) - path := "a" - size := int64(1024) - mode := os.FileMode(0) - aAttr := generateNestedPathAttr(path, size, mode) - - options := internal.StreamDirOptions{Name: path} - suite.mock.EXPECT().StreamDir(options).Return(aAttr, "", nil) - - suite.assertCacheEmpty() // cacheMap should be empty before call - returnedAttr, _, err := suite.attrCache.StreamDir(options) - suite.assert.NoError(err) - suite.assert.Equal(aAttr, returnedAttr) - - suite.assertCacheEmpty() // cacheMap should be empty after call -} - func (suite *attrCacheTestSuite) TestStreamDirError() { defer suite.cleanupTest() var paths = []string{"a", "a/", "ab", "ab/"} @@ -1040,79 +904,6 @@ func (suite *attrCacheTestSuite) TestRenameDir() { } } -// Tests Rename Directory Without Caching Empty Directories -func (suite *attrCacheTestSuite) TestRenameDirNoCacheDirs() { - defer suite.cleanupTest() - var inputs = []struct { - src string - dst string - }{ - {src: "a", dst: "ab"}, - {src: "a/", dst: "ab"}, - {src: "a", dst: "ab/"}, - {src: "a/", dst: "ab/"}, - } - - noCacheDirs := true - config := fmt.Sprintf("attr_cache:\n no-cache-dirs: %t", noCacheDirs) - - for _, input := range inputs { - // This is a little janky but required since testify suite does not support running setup or clean up for subtests. - suite.cleanupTest() - suite.setupTestHelper( - config, - ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) - suite.assert.Equal(!noCacheDirs, suite.attrCache.cacheDirs) - suite.Run(input.src+"->"+input.dst, func() { - truncatedSrc := internal.TruncateDirName(input.src) - truncatedDst := internal.TruncateDirName(input.dst) - options := internal.RenameDirOptions{Src: input.src, Dst: input.dst} - - // Error - suite.mock.EXPECT(). - RenameDir(options). - Return(errors.New("Failed to rename a directory")) - - err := suite.attrCache.RenameDir(options) - suite.assert.Error(err) - suite.assertNotInCache(truncatedSrc) - suite.assertNotInCache(truncatedDst) - - // Success - // Entry Does Not Already Exist - suite.mock.EXPECT().RenameDir(options).Return(nil) - - err = suite.attrCache.RenameDir(options) - suite.assert.NoError(err) - suite.assertNotInCache(truncatedSrc) - suite.assertNotInCache(truncatedDst) - - // Entry Already Exists - a, ab, ac := suite.addDirectoryToCache(input.src, false) - - suite.mock.EXPECT().RenameDir(options).Return(nil) - - err = suite.attrCache.RenameDir(options) - suite.assert.NoError(err) - // a paths should be deleted - for p := a.Front(); p != nil; p = p.Next() { - truncatedPath := internal.TruncateDirName(p.Value.(string)) - suite.assertDeleted(truncatedPath) - } - // ab paths should be invalidated - for p := ab.Front(); p != nil; p = p.Next() { - truncatedPath := internal.TruncateDirName(p.Value.(string)) - suite.assertExists(truncatedPath) - } - // ac paths should be untouched - for p := ac.Front(); p != nil; p = p.Next() { - truncatedPath := internal.TruncateDirName(p.Value.(string)) - suite.assertUntouched(truncatedPath) - } - }) - } -} - // Tests Create File func (suite *attrCacheTestSuite) TestCreateFile() { defer suite.cleanupTest() @@ -1338,60 +1129,6 @@ func (suite *attrCacheTestSuite) TestSyncDir() { } } -// Tests Sync Directory -func (suite *attrCacheTestSuite) TestSyncDirNoCacheDirs() { - defer suite.cleanupTest() - var paths = []string{"a", "a/"} - - noCacheDirs := true - config := fmt.Sprintf("attr_cache:\n no-cache-dirs: %t", noCacheDirs) - - for _, path := range paths { - suite.cleanupTest() - suite.setupTestHelper( - config, - ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) - suite.assert.Equal(!noCacheDirs, suite.attrCache.cacheDirs) - suite.Run(path, func() { - truncatedPath := internal.TruncateDirName(path) - options := internal.SyncDirOptions{Name: path} - - // Error - suite.mock.EXPECT().SyncDir(options).Return(errors.New("Failed")) - - err := suite.attrCache.SyncDir(options) - suite.assert.Error(err) - suite.assertNotInCache(truncatedPath) - - // Success - // Entry Does Not Already Exist - suite.mock.EXPECT().SyncDir(options).Return(nil) - - err = suite.attrCache.SyncDir(options) - suite.assert.NoError(err) - suite.assertNotInCache(truncatedPath) - - // Entry Already Exists - a, ab, ac := suite.addDirectoryToCache(path, false) - - suite.mock.EXPECT().SyncDir(options).Return(nil) - - err = suite.attrCache.SyncDir(options) - suite.assert.NoError(err) - // a paths should be deleted - for p := a.Front(); p != nil; p = p.Next() { - truncatedPath = internal.TruncateDirName(p.Value.(string)) - suite.assertInvalid(truncatedPath) - } - ab.PushBackList(ac) // ab and ac paths should be untouched - for p := ab.Front(); p != nil; p = p.Next() { - truncatedPath = internal.TruncateDirName(p.Value.(string)) - suite.assertUntouched(truncatedPath) - } - }) - } -} - // Tests Rename File func (suite *attrCacheTestSuite) TestRenameFile() { defer suite.cleanupTest() diff --git a/setup/advancedConfig.yaml b/setup/advancedConfig.yaml index 02c73521b..ce837f455 100644 --- a/setup/advancedConfig.yaml +++ b/setup/advancedConfig.yaml @@ -134,7 +134,6 @@ attr_cache: no-cache-on-list: true|false enable-symlinks: true|false max-files: - no-cache-dirs: true|false # Size tracker related configuration size_tracker: diff --git a/setup/baseConfig.yaml b/setup/baseConfig.yaml index 3e4ca5f72..27cf5c293 100644 --- a/setup/baseConfig.yaml +++ b/setup/baseConfig.yaml @@ -127,7 +127,6 @@ attr_cache: no-cache-on-list: true|false enable-symlinks: true|false max-files: - no-cache-dirs: true|false # Loopback configuration loopbackfs: From 1ccc78225c3f629fefe06c4187345bb5fb221c41 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 8 May 2026 03:08:15 -0600 Subject: [PATCH 2/4] Drop unused parameters --- component/attr_cache/attr_cache_test.go | 66 ++++++++++++------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/component/attr_cache/attr_cache_test.go b/component/attr_cache/attr_cache_test.go index 122f2eb0c..a785677ec 100644 --- a/component/attr_cache/attr_cache_test.go +++ b/component/attr_cache/attr_cache_test.go @@ -74,14 +74,14 @@ func newTestAttrCache(next internal.Component, configuration string) *AttrCache } func getDirPathAttr(path string) *internal.ObjAttr { - objAttr := getPathAttr(path, defaultSize, fs.FileMode(defaultMode), true) + objAttr := getPathAttr(path, defaultSize, fs.FileMode(defaultMode)) flags := internal.NewDirBitMap() objAttr.Flags = flags return objAttr } -func getPathAttr(path string, size int64, mode os.FileMode, _ bool) *internal.ObjAttr { +func getPathAttr(path string, size int64, mode os.FileMode) *internal.ObjAttr { flags := internal.NewFileBitMap() return &internal.ObjAttr{ Path: path, @@ -108,10 +108,10 @@ func (suite *attrCacheTestSuite) assertNotInCache(path string) { suite.assert.False(found) } -func (suite *attrCacheTestSuite) addPathToCache(path string, metadata bool) { +func (suite *attrCacheTestSuite) addPathToCache(path string) { isDir := strings.HasSuffix(path, "/") path = internal.TruncateDirName(path) - pathAttr := getPathAttr(path, defaultSize, fs.FileMode(defaultMode), metadata) + pathAttr := getPathAttr(path, defaultSize, fs.FileMode(defaultMode)) if isDir { pathAttr = getDirPathAttr(path) } @@ -243,7 +243,7 @@ func generateNestedPathAttr(path string, size int64, mode os.FileMode) []*intern pString := p.Value.(string) isDir := strings.HasSuffix(pString, "/") pString = internal.TruncateDirName(pString) - newPathAttr := getPathAttr(pString, size, mode, true) + newPathAttr := getPathAttr(pString, size, mode) if isDir { newPathAttr = getDirPathAttr(pString) } @@ -257,7 +257,7 @@ func generateListPathAttr(path string, numEntries int) []*internal.ObjAttr { pathAttrs := make([]*internal.ObjAttr, 0) for i := range numEntries { filename := fmt.Sprintf("%s/file%d", path, i) - newPathAttr := getPathAttr(filename, defaultSize, fs.FileMode(defaultMode), true) + newPathAttr := getPathAttr(filename, defaultSize, fs.FileMode(defaultMode)) pathAttrs = append(pathAttrs, newPathAttr) } return pathAttrs @@ -271,13 +271,13 @@ func (suite *attrCacheTestSuite) addDirectoryToCache( aPaths, abPaths, acPaths := generateDirectory(path) for p := aPaths.Front(); p != nil; p = p.Next() { - suite.addPathToCache(p.Value.(string), metadata) + suite.addPathToCache(p.Value.(string)) } for p := abPaths.Front(); p != nil; p = p.Next() { - suite.addPathToCache(p.Value.(string), metadata) + suite.addPathToCache(p.Value.(string)) } for p := acPaths.Front(); p != nil; p = p.Next() { - suite.addPathToCache(p.Value.(string), metadata) + suite.addPathToCache(p.Value.(string)) } return aPaths, abPaths, acPaths @@ -754,7 +754,7 @@ func (suite *attrCacheTestSuite) TestDirInCloud() { // build up the attribute cache suite.addDirectoryToCache("a", true) deepPath := "a/b/c/d" - suite.addPathToCache(deepPath, true) + suite.addPathToCache(deepPath) // delete file a/b/c/d and make sure a/b/ and a/b/c/ are marked not in cloud storage delOptions := internal.DeleteFileOptions{Name: deepPath} @@ -786,7 +786,7 @@ func (suite *attrCacheTestSuite) TestIsDirEmpty() { options := internal.IsDirEmptyOptions{ Name: path, } - suite.addPathToCache(path, false) + suite.addPathToCache(path) suite.mock.EXPECT().IsDirEmpty(options).Return(true) empty := suite.attrCache.IsDirEmpty(options) @@ -930,7 +930,7 @@ func (suite *attrCacheTestSuite) TestCreateFile() { suite.assert.EqualValues(0, checkItem.attr.Size) // Entry Already Exists - suite.addPathToCache(path, false) + suite.addPathToCache(path) suite.mock.EXPECT().CreateFile(options).Return(&handlemap.Handle{}, nil) _, err = suite.attrCache.CreateFile(options) @@ -972,7 +972,7 @@ func (suite *attrCacheTestSuite) TestOpenFile() { suite.assertNotInCache(path) // Attribute cache entry does exist - suite.addPathToCache(path, true) + suite.addPathToCache(path) // OpenFile fails suite.mock.EXPECT().OpenFile(options).Return(nil, syscall.ENOENT) @@ -1006,7 +1006,7 @@ func (suite *attrCacheTestSuite) TestDeleteFile() { suite.assertDeleted(path) // Entry Already Exists - suite.addPathToCache(path, false) + suite.addPathToCache(path) suite.mock.EXPECT().DeleteFile(options).Return(nil) err = suite.attrCache.DeleteFile(options) @@ -1067,7 +1067,7 @@ func (suite *attrCacheTestSuite) TestSyncFile() { suite.assertNotInCache(path) // Entry Already Exists - suite.addPathToCache(path, false) + suite.addPathToCache(path) suite.mock.EXPECT().SyncFile(options).Return(nil) err = suite.attrCache.SyncFile(options) @@ -1155,8 +1155,8 @@ func (suite *attrCacheTestSuite) TestRenameFile() { suite.assertNotInCache(dst) // Entry Already Exists - suite.addPathToCache(src, false) - suite.addPathToCache(dst, false) + suite.addPathToCache(src) + suite.addPathToCache(dst) attr, found := suite.attrCache.cache.get(src) suite.assert.True(found) @@ -1182,7 +1182,7 @@ func (suite *attrCacheTestSuite) TestRenameFile() { assertAttributesTransferred(suite, options.SrcAttr, modifiedDstAttr) // Src Entry Exist and Dst Entry Don't Exist - suite.addPathToCache(src, false) + suite.addPathToCache(src) // Add negative entry to cache for Dst suite.attrCache.cache.insert( insertOptions{attr: internal.CreateObjAttrDir(dst), exists: false, cachedAt: time.Now()}, @@ -1261,7 +1261,7 @@ func (suite *attrCacheTestSuite) TestWriteFileExists() { options := internal.WriteFileOptions{Handle: &handle, Metadata: nil} // Entry Already Exists - suite.addPathToCache(path, true) + suite.addPathToCache(path) suite.mock.EXPECT().WriteFile(&options).Return(0, nil) _, err := suite.attrCache.WriteFile(&options) @@ -1294,7 +1294,7 @@ func (suite *attrCacheTestSuite) TestTruncateFile() { suite.assert.True(found) // Entry Already Exists - suite.addPathToCache(path, false) + suite.addPathToCache(path) suite.mock.EXPECT().TruncateFile(options).Return(nil) err = suite.attrCache.TruncateFile(options) @@ -1353,7 +1353,7 @@ func (suite *attrCacheTestSuite) TestCopyFromFileExists() { options := internal.CopyFromFileOptions{Name: path, File: nil, Metadata: nil} // Entry Already Exists - suite.addPathToCache(path, true) + suite.addPathToCache(path) suite.mock.EXPECT().CopyFromFile(options).Return(nil) _, found := suite.attrCache.cache.get(options.Name) @@ -1514,7 +1514,7 @@ func (suite *attrCacheTestSuite) TestGetAttrDoesNotExist() { // attributes should not be accessible so call the mock suite.mock.EXPECT(). GetAttr(options). - Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode), false), nil) + Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode)), nil) suite.assertCacheEmpty() // cacheMap should be empty before call _, err := suite.attrCache.GetAttr(options) @@ -1588,7 +1588,7 @@ func (suite *attrCacheTestSuite) TestCacheTimeout() { // attributes should not be accessible so call the mock suite.mock.EXPECT(). GetAttr(options). - Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode), true), nil) + Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode)), nil) suite.assertCacheEmpty() // cacheMap should be empty before call _, err := suite.attrCache.GetAttr(options) @@ -1605,7 +1605,7 @@ func (suite *attrCacheTestSuite) TestCacheTimeout() { // After cache timeout elapses, subsequent get attr should need to call next component suite.mock.EXPECT(). GetAttr(options). - Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode), true), nil) + Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode)), nil) _, err = suite.attrCache.GetAttr(options) suite.assert.NoError(err) } @@ -1632,10 +1632,10 @@ func (suite *attrCacheTestSuite) TestCacheCleanupExpiredEntries() { // Add two files to cache suite.mock.EXPECT(). GetAttr(options1). - Return(getPathAttr(path1, defaultSize, fs.FileMode(defaultMode), true), nil) + Return(getPathAttr(path1, defaultSize, fs.FileMode(defaultMode)), nil) suite.mock.EXPECT(). GetAttr(options2). - Return(getPathAttr(path2, defaultSize, fs.FileMode(defaultMode), true), nil) + Return(getPathAttr(path2, defaultSize, fs.FileMode(defaultMode)), nil) _, err := suite.attrCache.GetAttr(options1) suite.assert.NoError(err) @@ -1652,7 +1652,7 @@ func (suite *attrCacheTestSuite) TestCacheCleanupExpiredEntries() { suite.mock.EXPECT().GetAttr(parentOptions).Return(getDirPathAttr(parentDir), nil) suite.mock.EXPECT(). GetAttr(childOptions). - Return(getPathAttr(childFile, defaultSize, fs.FileMode(defaultMode), true), nil) + Return(getPathAttr(childFile, defaultSize, fs.FileMode(defaultMode)), nil) _, err = suite.attrCache.GetAttr(parentOptions) suite.assert.NoError(err) @@ -1712,12 +1712,12 @@ func (suite *attrCacheTestSuite) TestCacheCleanupDuringBulkCaching() { path2 := "oldfile2" oldTime := time.Now().Add(-time.Second * time.Duration(cacheTimeout+1)) suite.attrCache.cache.cacheMap[path1] = newAttrCacheItem( - getPathAttr(path1, defaultSize, fs.FileMode(defaultMode), true), + getPathAttr(path1, defaultSize, fs.FileMode(defaultMode)), true, oldTime, ) suite.attrCache.cache.cacheMap[path2] = newAttrCacheItem( - getPathAttr(path2, defaultSize, fs.FileMode(defaultMode), true), + getPathAttr(path2, defaultSize, fs.FileMode(defaultMode)), true, oldTime, ) @@ -1785,8 +1785,8 @@ func (suite *attrCacheTestSuite) TestCreateLink() { suite.assertExists(link) // Entry Already Exists - suite.addPathToCache(link, false) - suite.addPathToCache(path, false) + suite.addPathToCache(link) + suite.addPathToCache(path) suite.mock.EXPECT().CreateLink(options).Return(nil) err = suite.attrCache.CreateLink(options) @@ -1825,7 +1825,7 @@ func (suite *attrCacheTestSuite) TestChmod() { suite.assertNotInCache(truncatedPath) // Entry Already Exists - suite.addPathToCache(path, false) + suite.addPathToCache(path) suite.mock.EXPECT().Chmod(options).Return(nil) err = suite.attrCache.Chmod(options) @@ -1874,7 +1874,7 @@ func (suite *attrCacheTestSuite) TestChown() { suite.assertNotInCache(truncatedPath) // Entry Already Exists - suite.addPathToCache(path, false) + suite.addPathToCache(path) suite.mock.EXPECT().Chown(options).Return(nil) err = suite.attrCache.Chown(options) From 2a97326d34684e5083677e8e8b8a3d8c1cbb149c Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 8 May 2026 03:10:23 -0600 Subject: [PATCH 3/4] TODO --- component/attr_cache/attr_cache.go | 1 + 1 file changed, 1 insertion(+) diff --git a/component/attr_cache/attr_cache.go b/component/attr_cache/attr_cache.go index 31f1e8322..680b952a4 100644 --- a/component/attr_cache/attr_cache.go +++ b/component/attr_cache/attr_cache.go @@ -202,6 +202,7 @@ func (ac *AttrCache) deleteDirectory(path string, deletedAt time.Time) error { // get the entry to be marked deleted item, found := ac.cache.get(path) // handle errors and unexpected behavior + // TODO: should we avoid throwing a fit when there is no entry? dirExists := found && item.exists() if !dirExists { log.Err("AttrCache::deleteDirectory : %s does not exist", path) From 9267d63da9df1e41c8465eb05ae7a47333a24b3c Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Sat, 16 May 2026 13:31:48 -0600 Subject: [PATCH 4/4] Drop unused parameters --- component/attr_cache/attr_cache_test.go | 43 +++++++++---------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/component/attr_cache/attr_cache_test.go b/component/attr_cache/attr_cache_test.go index 20a974e46..b39b347e6 100644 --- a/component/attr_cache/attr_cache_test.go +++ b/component/attr_cache/attr_cache_test.go @@ -265,7 +265,6 @@ func generateListPathAttr(path string, numEntries int) []*internal.ObjAttr { func (suite *attrCacheTestSuite) addDirectoryToCache( path string, - metadata bool, ) (*list.List, *list.List, *list.List) { // TODO: flag directories as such, or else recursion based on IsDir() won't work... aPaths, abPaths, acPaths := generateDirectory(path) @@ -505,7 +504,7 @@ func (suite *attrCacheTestSuite) TestDeleteDir() { suite.assertNotInCache(truncatedPath) // Entry Exists - a, ab, ac := suite.addDirectoryToCache(path, false) + a, ab, ac := suite.addDirectoryToCache(path) suite.mock.EXPECT().DeleteDir(options).Return(nil) @@ -654,7 +653,7 @@ func (suite *attrCacheTestSuite) TestStreamDirExists() { // Success // Entries Already Exist - a, ab, ac := suite.addDirectoryToCache(path, false) + a, ab, ac := suite.addDirectoryToCache(path) // cache entries should be untouched before read dir call for _, p := range aAttr { @@ -798,7 +797,7 @@ func (suite *attrCacheTestSuite) TestStreamDirOfflineNoData() { func (suite *attrCacheTestSuite) TestDirInCloud() { defer suite.cleanupTest() // build up the attribute cache - suite.addDirectoryToCache("a", true) + suite.addDirectoryToCache("a") deepPath := "a/b/c/d" suite.addPathToCache(deepPath) @@ -859,7 +858,7 @@ func (suite *attrCacheTestSuite) TestIsDirEmptyFalseInCache() { options := internal.IsDirEmptyOptions{ Name: path, } - suite.addDirectoryToCache(path, false) + suite.addDirectoryToCache(path) // make sure the attribute cache handles the request itself suite.mock.EXPECT().IsDirEmpty(options).MaxTimes(0) @@ -977,7 +976,7 @@ func (suite *attrCacheTestSuite) TestRenameDir() { // Error // Destination Entry (ab) Already Exists - a, ab, ac := suite.addDirectoryToCache(input.src, false) + a, ab, ac := suite.addDirectoryToCache(input.src) suite.mock.EXPECT().RenameDir(options).Return(nil) @@ -1216,7 +1215,7 @@ func (suite *attrCacheTestSuite) TestSyncDir() { suite.assertNotInCache(truncatedPath) // Entry Already Exists - a, ab, ac := suite.addDirectoryToCache(path, false) + a, ab, ac := suite.addDirectoryToCache(path) suite.mock.EXPECT().SyncDir(options).Return(nil) @@ -1487,7 +1486,7 @@ func (suite *attrCacheTestSuite) TestGetAttrExistsDeleted() { suite.SetupTest() suite.Run(path, func() { - suite.addDirectoryToCache("a", false) + suite.addDirectoryToCache("a") // delete directory a and file ac suite.mock.EXPECT().DeleteDir(gomock.Any()).Return(nil) suite.mock.EXPECT().DeleteFile(gomock.Any()).Return(nil) @@ -1513,10 +1512,7 @@ func (suite *attrCacheTestSuite) TestGetAttrExistsWithMetadata() { suite.SetupTest() suite.Run(path, func() { truncatedPath := internal.TruncateDirName(path) - suite.addDirectoryToCache( - "a", - true, - ) // add the paths to the cache with IsMetadataRetrieved=true + suite.addDirectoryToCache("a") options := internal.GetAttrOptions{Name: path} // no call to mock component since attributes are accessible @@ -1543,10 +1539,7 @@ func (suite *attrCacheTestSuite) TestGetAttrExistsWithoutMetadataNoSymlinks() { ) // setup a new attr cache with a custom config (clean up will occur after the test as usual) suite.Run(path, func() { truncatedPath := internal.TruncateDirName(path) - suite.addDirectoryToCache( - "a", - true, - ) // add the paths to the cache with IsMetadataRetrived=true + suite.addDirectoryToCache("a") options := internal.GetAttrOptions{Name: path} // no call to mock component since metadata is not needed in noSymlinks mode @@ -1565,10 +1558,7 @@ func (suite *attrCacheTestSuite) TestGetAttrExistsWithoutMetadata() { for _, path := range paths { suite.Run(path, func() { truncatedPath := internal.TruncateDirName(path) - suite.addDirectoryToCache( - "a", - true, - ) // add the paths to the cache with IsMetadataRetrieved=true + suite.addDirectoryToCache("a") options := internal.GetAttrOptions{Name: path} // no call to mock component since metadata is not needed when symlinks are disabled @@ -1596,14 +1586,11 @@ func (suite *attrCacheTestSuite) TestGetAttrExistsWithoutMetadataWithSymlinks() suite.assert.Equal(enableSymlinks, suite.attrCache.enableSymlinks) suite.Run(path, func() { truncatedPath := internal.TruncateDirName(path) - suite.addDirectoryToCache( - "a", - false, - ) // add the paths to the cache with IsMetadataRetrieved=false + suite.addDirectoryToCache("a") options := internal.GetAttrOptions{Name: path} // attributes should not be accessible so call the mock - //suite.mock.EXPECT().GetAttr(options).Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode), false), nil) + //suite.mock.EXPECT().GetAttr(options).Return(getPathAttr(path, defaultSize, fs.FileMode(defaultMode)), nil) _, err := suite.attrCache.GetAttr(options) suite.assert.NoError(err) @@ -1841,8 +1828,8 @@ func (suite *attrCacheTestSuite) TestCacheCleanupExpiredEntries() { suite.assert.NoError(err) // Add a nested directory with a child (both old/expired) - parentDir := "parentdir" - childFile := parentDir + "/childfile" + parentDir := "parentDir" + childFile := parentDir + "/childFile" parentOptions := internal.GetAttrOptions{Name: parentDir} childOptions := internal.GetAttrOptions{Name: childFile} @@ -1861,7 +1848,7 @@ func (suite *attrCacheTestSuite) TestCacheCleanupExpiredEntries() { suite.assert.Len( suite.attrCache.cache.cacheMap, 5, - ) // root + file1 + file2 + parentdir + parentdir/childfile + ) // root + file1 + file2 + parentDir + parentDir/childFile suite.assertUntouched(path1) suite.assertUntouched(path2) suite.assertUntouched(childFile)