swarming: delete named cache symlinks before workdir

client/named_cache.py

Index: client/named_cache.py
diff --git a/client/named_cache.py b/client/named_cache.py
index 52e9b17768bf21149f3d267fd8d8676df98dde4d..50a4e1c04fb9005f5d4a83150f3ee405f0044e63 100644
--- a/client/named_cache.py
+++ b/client/named_cache.py
@@ -141,8 +141,9 @@ class CacheManager(object):
     assert isinstance(name, basestring), name
     return self._lru.get_timestamp(name)
 
+  @contextlib.contextmanager
   def create_symlinks(self, root, named_caches):
-    """Creates symlinks in |root| for specified named_caches.
+    """Creates symlinks in |root| for the specified named_caches.
 
     named_caches must be a list of (name, path) tuples.
 
@@ -154,10 +155,7 @@ class CacheManager(object):
     for name, path in named_caches:
       logging.info('Named cache %r -> %r', name, path)
       try:
-        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 ".."')
+        _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)
@@ -168,6 +166,22 @@ class CacheManager(object):
             '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.
+    """
+    for name, path in named_caches:
+      logging.info('Unlinking named cache "%s"', name)
+      try:
+        _validate_named_cache_path(path)

[M-A Ruel, 2017/02/01 20:58:27]

You think it's necessary?

[nodir, 2017/02/01 21:01:06]

the idea is that I don't want to delete files/dirs outside of the root

[M-A Ruel, 2017/02/02 01:11:10]

Oh sure but I would think they were validated in create_symlinks. It's fine.

[M-A Ruel, 2017/02/02 05:11:57]

Sure, still lgtm

+        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.
 
@@ -262,3 +276,10 @@ def process_named_cache_options(parser, options):
   if options.named_cache_root:
     return CacheManager(os.path.abspath(options.named_cache_root))
   return None
+
+
+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 ".."')