Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix filecache scan deleting entries added by concurrent actions #7924

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bduffany
Copy link
Member

During the initial filecache scan, we repeatedly call AddFile() which calls lru.Remove().

The current implementation of AddFile() is:

  1. Evict any existing entry. If an entry does exist, unlink it from the destination path, via the OnEvict callback.
  2. Hardlink the file from the source path to its dest path.

For the filecache scan specifically, this logic is not ideal for two reasons:

  • At best, it's unnecessary. We're effectively doing a bunch of hardlinks that just link each path to itself, which should be a NOP.
  • At worst, an action can race with the filecache scan to update a cache key. If we call AddFile() during the scan, but an action was running concurrently and already called AddFile() for the same key, then step (1) above will invoke the OnEvict callback, which deletes the cache entry. Because we're linking the path from itself, the filecache scan sees a NotFound error when it tries to link this file. This is silly - if we hadn't deleted the entry, then it would be found.

Comment on lines +300 to 310
if fp != existingFilePath {
c.l.Remove(k)
if err := disk.EnsureDirectoryExists(filepath.Dir(fp)); err != nil {
return err
}
if err := cloneOrLink(groupID, existingFilePath, fp); err != nil {
return err
}
} else if c.l.Contains(k) {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is a little confusing to me -- would it be clearer if we just enumerated the special case and unindented the rest?

if fp == existingFilePath && c.l.Contains(k) {
  return nil // skip adding the same file twice which could cause an eviction
}
c.l.Remove(k)
if err := disk.EnsureDirectoryExists(filepath.Dir(fp)); err != nil {
	return err
}
if err := cloneOrLink(groupID, existingFilePath, fp); err != nil {
	return err
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants