Revert "named caches: move instead of symlinking"

client/named_cache.py

Index: client/named_cache.py
diff --git a/client/named_cache.py b/client/named_cache.py
index 2f8f8cf659e40326f38a9cb86f9252886fa05435..e3bf5e81c865e62c1235b0d4cb1f13917159c237 100644
--- a/client/named_cache.py
+++ b/client/named_cache.py
@@ -28,15 +28,15 @@ class Error(Exception):
 
 
 class CacheManager(object):
-  """Manages cache directories exposed to a task.
+  """Manages cache directories exposed to a task as symlinks.
 
   A task can specify that caches should be present on a bot. A cache is
   tuple (name, path), where
     name is a short identifier that describes the contents of the cache, e.g.
       "git_v8" could be all git repositories required by v8 builds, or
       "build_chromium" could be build artefacts of the Chromium.
-    path is a directory path relative to the task run dir. Cache installation
-      puts the requested cache directory at the path.
+    path is a directory path relative to the task run dir. It will be mapped
+      to the cache directory persisted on the bot.
   """
 
   def __init__(self, root_dir):
@@ -44,9 +44,8 @@ class CacheManager(object):
 
     |root_dir| is a directory for persistent cache storage.
     """
-    assert isinstance(root_dir, unicode), root_dir
     assert file_path.isabs(root_dir), root_dir
-    self.root_dir = root_dir
+    self.root_dir = unicode(root_dir)
     self._lock = threading_utils.LockWithAssert()
     # LRU {cache_name -> cache_location}
     # It is saved to |root_dir|/state.json.
@@ -54,9 +53,9 @@ class CacheManager(object):
 
   @contextlib.contextmanager
   def open(self, time_fn=None):
-    """Opens NamedCaches for mutation operations, such as install.
+    """Opens NamedCaches for mutation operations, such as request or trim.
 
-    Only one caller can open the cache manager at a time. If the same thread
+    Only on caller can open the cache manager at a time. If the same thread
     calls this function after opening it earlier, the call will deadlock.
 
     time_fn is a function that returns timestamp (float) and used to take
@@ -87,14 +86,48 @@ class CacheManager(object):
   def __len__(self):
     """Returns number of items in the cache.
 
-    NamedCache must be open.
+    Requires NamedCache to be open.
     """
     return len(self._lru)
 
+  def request(self, name):
+    """Returns an absolute path to the directory of the named cache.
+
+    Creates a cache directory if it does not exist yet.
+
+    Requires NamedCache to be open.
+    """
+    self._lock.assert_locked()
+    assert isinstance(name, basestring), name
+    path = self._lru.get(name)
+    create_named_link = False
+    if path is None:
+      path = self._allocate_dir()
+      create_named_link = True
+      logging.info('Created %r for %r', path, name)
+    abs_path = os.path.join(self.root_dir, path)
+
+    # TODO(maruel): That's weird, it should exist already.
+    file_path.ensure_tree(abs_path)
+    self._lru.add(name, path)
+
+    if create_named_link:
+      # Create symlink <root_dir>/<named>/<name> -> <root_dir>/<short name>
+      # for user convenience.
+      named_path = self._get_named_path(name)
+      if os.path.exists(named_path):
+        file_path.remove(named_path)
+      else:
+        file_path.ensure_tree(os.path.dirname(named_path))
+      logging.info('Symlink %r to %r', named_path, abs_path)
+      fs.symlink(abs_path, named_path)
+
+    return abs_path
+
   def get_oldest(self):
     """Returns name of the LRU cache or None.
 
-    NamedCache must be open.
+    Requires NamedCache to be open.
     """
     self._lock.assert_locked()
     try:
@@ -105,7 +138,7 @@ class CacheManager(object):
   def get_timestamp(self, name):
     """Returns timestamp of last use of an item.
 
-    NamedCache must be open.
+    Requires NamedCache to be open.
 
     Raises KeyError if cache is not found.
     """
@@ -113,96 +146,46 @@ class CacheManager(object):
     assert isinstance(name, basestring), name
     return self._lru.get_timestamp(name)
 
-  @property
-  def available(self):
-    """Returns a set of names of available caches.
-
-    NamedCache must be open.
-    """
-    self._lock.assert_locked()
-    return self._lru.keys_set()
+  @contextlib.contextmanager
+  def create_symlinks(self, root, named_caches):
+    """Creates symlinks in |root| for the specified named_caches.
 
-  def install(self, path, name):
-    """Moves the directory for the specified named cache to |path|.
+    named_caches must be a list of (name, path) tuples.
 
-    NamedCache must be open. path must be absolute, unicode and must not exist.
+    Requires NamedCache to be open.
 
-    Raises Error if cannot install the cache.
+    Raises Error if cannot create a symlink.
     """
     self._lock.assert_locked()
-    logging.info('Installing named cache %r to %r', name, path)
-    try:
-      _check_abs(path)
-      if os.path.isdir(path):
-        raise Error('installation directory %r already exists' % path)
-
-      rel_cache = self._lru.get(name)
-      if rel_cache:
-        abs_cache = os.path.join(self.root_dir, rel_cache)
-        if os.path.isdir(abs_cache):
-          logging.info('Moving %r to %r', abs_cache, path)
-          file_path.ensure_tree(os.path.dirname(path))
-          fs.rename(abs_cache, path)
-          self._remove(name)
-          return
-
-        logging.warning('directory for named cache %r does not exist', name)
-        self._remove(name)
-
-      # The named cache does not exist, create an empty directory.
-      # When uninstalling, we will move it back to the cache and create an
-      # an entry.
-      file_path.ensure_tree(path)
-    except (OSError, Error) as ex:
-      raise Error(
-          'cannot install cache named %r at %r: %s' % (
-            name, path, ex))
-
-  def uninstall(self, path, name):
-    """Moves the cache directory back. Opposite to install().
-
-    NamedCache must be open. path must be absolute and unicode.
-
-    Raises Error if cannot uninstall the cache.
+    for name, path in named_caches:
+      logging.info('Named cache %r -> %r', name, path)
+      try:
+        _validate_named_cache_path(path)
+        symlink_path = os.path.abspath(os.path.join(root, path))
+        file_path.ensure_tree(os.path.dirname(symlink_path))
+        requested = self.request(name)
+        logging.info('Symlink %r to %r', symlink_path, requested)
+        fs.symlink(requested, symlink_path)
+      except (OSError, Error) as ex:
+        raise Error(
+            'cannot create a symlink for cache named "%s" at "%s": %s' % (
+              name, symlink_path, ex))
+
+  def delete_symlinks(self, root, named_caches):
+    """Deletes symlinks from |root| for the specified named_caches.
+
+    named_caches must be a list of (name, path) tuples.
     """
-    logging.info('Uninstalling named cache %r from %r', name, path)
-    try:
-      _check_abs(path)
-      if not os.path.isdir(path):
-        logging.warning(
-            'Directory %r does not exist anymore. Cache lost.', path)
-        return
-
-      rel_cache = self._lru.get(name)
-      if rel_cache:
-        # Do not crash because cache already exists.
-        logging.warning('overwriting an existing named cache %r', name)
-        create_named_link = False
-      else:
-        rel_cache = self._allocate_dir()
-        create_named_link = True
-
-      # Move the dir and create an entry for the named cache.
-      abs_cache = os.path.join(self.root_dir, rel_cache)
-      logging.info('Moving %r to %r', path, abs_cache)
-      file_path.ensure_tree(os.path.dirname(abs_cache))
-      fs.rename(path, abs_cache)
-      self._lru.add(name, rel_cache)
-
-      if create_named_link:
-        # Create symlink <root_dir>/<named>/<name> -> <root_dir>/<short name>
-        # for user convenience.
-        named_path = self._get_named_path(name)
-        if os.path.exists(named_path):
-          file_path.remove(named_path)
-        else:
-          file_path.ensure_tree(os.path.dirname(named_path))
-        fs.symlink(abs_cache, named_path)
-        logging.info('Created symlink %r to %r', named_path, abs_cache)
-    except (OSError, Error) as ex:
-      raise Error(
-          'cannot uninstall cache named %r at %r: %s' % (
-            name, path, ex))
+    for name, path in named_caches:
+      logging.info('Unlinking named cache "%s"', name)
+      try:
+        _validate_named_cache_path(path)
+        symlink_path = os.path.abspath(os.path.join(root, path))
+        fs.unlink(symlink_path)
+      except (OSError, Error) as ex:
+        raise Error(
+            'cannot unlink cache named "%s" at "%s": %s' % (
+              name, symlink_path, ex))
 
   def trim(self, min_free_space):
     """Purges cache.
@@ -212,7 +195,7 @@ class CacheManager(object):
 
     If min_free_space is None, disk free space is not checked.
 
-    NamedCache must be open.
+    Requires NamedCache to be open.
 
     Returns:
       Number of caches deleted.
@@ -228,16 +211,22 @@ class CacheManager(object):
     while ((min_free_space and free_space < min_free_space)
            or len(self._lru) > MAX_CACHE_SIZE):
       logging.info(
-          'Making space for named cache %d > %d or %d > %d',
+          'Making space for named cache %s > %s or %s > %s',
           free_space, min_free_space, len(self._lru), MAX_CACHE_SIZE)
       try:
-        name, _ = self._lru.get_oldest()
+        name, (path, _) = self._lru.get_oldest()
       except KeyError:
         return total
-      logging.info('Removing named cache %r', name)
-      self._remove(name)
+      named_dir = self._get_named_path(name)
+      if fs.islink(named_dir):
+        fs.unlink(named_dir)
+      path_abs = os.path.join(self.root_dir, path)
+      if os.path.isdir(path_abs):
+        logging.info('Removing named cache %s', path_abs)
+        file_path.rmtree(path_abs)
       if min_free_space:
         free_space = file_path.get_free_space(self.root_dir)
+      self._lru.pop(name)
       total += 1
     return total
 
@@ -262,28 +251,6 @@ class CacheManager(object):
       tried.add(rel_path)
     raise Error('could not allocate a new cache dir, too many cache dirs')
 
-  def _remove(self, name):
-    """Removes a cache directory and entry.
-
-    NamedCache must be open.
-
-    Returns:
-      Number of caches deleted.
-    """
-    self._lock.assert_locked()
-    rel_path = self._lru.get(name)
-    if not rel_path:
-      return
-
-    named_dir = self._get_named_path(name)
-    if fs.islink(named_dir):
-      fs.unlink(named_dir)
-
-    abs_path = os.path.join(self.root_dir, rel_path)
-    if os.path.isdir(abs_path):
-      file_path.rmtree(abs_path)
-    self._lru.pop(name)
-
   def _get_named_path(self, name):
     return os.path.join(self.root_dir, 'named', name)
 
@@ -299,7 +266,7 @@ def add_named_cache_options(parser):
       help='A named cache to request. Accepts two arguments, name and path. '
            'name identifies the cache, must match regex [a-z0-9_]{1,4096}. '
            'path is a path relative to the run dir where the cache directory '
-           'must be put to. '
+           'must be symlinked to. '
            'This option can be specified more than once.')
   group.add_option(
       '--named-cache-root',
@@ -314,16 +281,16 @@ def process_named_cache_options(parser, options):
   for name, path in options.named_caches:
     if not CACHE_NAME_RE.match(name):
       parser.error(
-          'cache name %r does not match %r' % (name, CACHE_NAME_RE.pattern))
+          'cache name "%s" does not match %s' % (name, CACHE_NAME_RE.pattern))
     if not path:
       parser.error('cache path cannot be empty')
   if options.named_cache_root:
-    return CacheManager(unicode(os.path.abspath(options.named_cache_root)))
+    return CacheManager(os.path.abspath(options.named_cache_root))
   return None
 
 
-def _check_abs(path):
-  if not isinstance(path, unicode):
-    raise Error('named cache installation path must be unicode')
-  if not os.path.isabs(path):
-    raise Error('named cache installation path must be absolute')
+def _validate_named_cache_path(path):
+  if os.path.isabs(path):
+    raise Error('named cache path must not be absolute')
+  if '..' in path.split(os.path.sep):
+    raise Error('named cache path must not contain ".."')