file_cache.py: idempotent remove without races (#31477)

There's a race condition in `remove()` as the lockfile is removed after
releasing the lock, which is a problem when another process acquires a
write lock during deletion.

Also simplify life a bit in multiprocessing when a file is possibly
removed multiple times, which currently is an error on the second
deletion, so the proposed fix is to make remove(...) idempotent and not
error when deleting non-existing cache entries.

Don't tests for existence of lockfile, cause windows/linux behavior is different

2 files changed, 19 insertions, 4 deletions

diff --git a/lib/spack/spack/test/util/file_cache.py b/lib/spack/spack/test/util/file_cache.py
index 106a411b69..815b21fb38 100644
--- a/lib/spack/spack/test/util/file_cache.py
+++ b/lib/spack/spack/test/util/file_cache.py
@@ -55,9 +55,12 @@ def test_write_and_remove_cache_file(file_cache):
 
     file_cache.remove('test.yaml')
 
-    # After removal both the file and the lock file should not exist
+    # After removal the file should not exist
     assert not os.path.exists(file_cache.cache_path('test.yaml'))
-    assert not os.path.exists(file_cache._lock_path('test.yaml'))
+
+    # Whether the lock file exists is more of an implementation detail, on Linux they
+    # continue to exist, on Windows they don't.
+    # assert os.path.exists(file_cache._lock_path('test.yaml'))
 
 
 @pytest.mark.skipif(sys.platform == 'win32',
@@ -94,3 +97,10 @@ def test_cache_write_readonly_cache_fails(file_cache):
 
     with pytest.raises(CacheError, match='Insufficient permissions to write'):
         file_cache.write_transaction(filename)
+
+
+@pytest.mark.regression('31475')
+def test_delete_is_idempotent(file_cache):
+    """Deleting a non-existent key should be idempotent, to simplify life when
+    running delete with multiple processes"""
+    file_cache.remove('test.yaml')
diff --git a/lib/spack/spack/util/file_cache.py b/lib/spack/spack/util/file_cache.py
index cf5ed8310e..611f428881 100644
--- a/lib/spack/spack/util/file_cache.py
+++ b/lib/spack/spack/util/file_cache.py
@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import errno
 import os
 import shutil
 
@@ -170,13 +171,17 @@ class FileCache(object):
             return sinfo.st_mtime
 
     def remove(self, key):
+        file = self.cache_path(key)
         lock = self._get_lock(key)
         try:
             lock.acquire_write()
-            os.unlink(self.cache_path(key))
+            os.unlink(file)
+        except OSError as e:
+            # File not found is OK, so remove is idempotent.
+            if e.errno != errno.ENOENT:
+                raise
         finally:
             lock.release_write()
-            lock.cleanup()
 
 
 class CacheError(SpackError):