[WIP] Partial cache read fix

CHANGES.txt

@@ -36,6 +36,10 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Fix occasional test failures caused by not being able to find a file or directory fixture
       when running multiple tests with multiple jobs.
 
+  From Raven Kopelman (@ravenAtSafe on GitHub):
+    - Prevent files from being partially read from the cache. If there's an error while copying the file
+      is now removed.
+
   From Joachim Kuebart:
     - Suppress missing SConscript deprecation warning if `must_exist=False`
       is used.
@@ -48,6 +52,7 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
   From Daniel Moody:
     - Fix issue where java parsed a class incorrectly from lambdas used after a new.
 
+
   From Mats Wichmann:
     - Complete tests for Dictionary, env.keys() and env.values() for
       OverrideEnvironment. Enable env.setdefault() method, add tests.

SCons/CacheDir.py

@@ -29,6 +29,7 @@
 import os
 import stat
 import sys
+import shutil
 
 import SCons.Action
 import SCons.Errors
@@ -50,18 +51,28 @@ def CacheRetrieveFunc(target, source, env):
         cd.CacheDebug('CacheRetrieve(%s):  %s not in cache\n', t, cachefile)
         return 1
     cd.hits += 1
-    cd.CacheDebug('CacheRetrieve(%s):  retrieving from %s\n', t, cachefile)
     if SCons.Action.execute_actions:
         if fs.islink(cachefile):
             fs.symlink(fs.readlink(cachefile), t.get_internal_path())
         else:
-            env.copy_from_cache(cachefile, t.get_internal_path())
+            try:
+                env.copy_from_cache(cachefile, t.get_internal_path())
+            except (shutil.SameFileError, IOError) as e:
+                    # In case file was partially retrieved (and now corrupt)
+                    # delete it to avoid poisoning commands like 'ar' that
+                    # read from the initial state of the file they are writing
+                    # to.
+                    t.fs.unlink(t.get_internal_path())
+                    cd.CacheDebug('CacheRetrieve(%s):  Error while retrieving from %s deleting %s\n', t, cachefile)
+                    raise e
+
             try:
                 os.utime(cachefile, None)
             except OSError:
                 pass
         st = fs.stat(cachefile)
         fs.chmod(t.get_internal_path(), stat.S_IMODE(st[stat.ST_MODE]) | stat.S_IWRITE)
+    cd.CacheDebug('CacheRetrieve(%s):  retrieved from %s\n', t, cachefile)
     return 0
 
 def CacheRetrieveString(target, source, env):

SCons/Taskmaster.py

@@ -223,10 +223,13 @@ def execute(self):
                 cached_targets.append(t)
             if len(cached_targets) < len(self.targets):
                 # Remove targets before building. It's possible that we
-                # partially retrieved targets from the cache, leaving
-                # them in read-only mode. That might cause the command
+                # retrieved a subset of targets from the cache, leaving
+                # them in an inconsistent state. That might cause the command
                 # to fail.
                 #
+                # Note that retrieve_from_cache() ensures no single target can
+                # be partially retrieved (file left in corrupt state).
+                #
                 for t in cached_targets:
                     try:
                         t.fs.unlink(t.get_internal_path())

test/CacheDir/debug.py

@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# __COPYRIGHT__
+# Copyright The SCons Foundation
 #
 # Permission is hereby granted, free of charge, to any person obtaining
 # a copy of this software and associated documentation files (the
@@ -22,7 +22,6 @@
 # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 #
 
-__revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
 """
 Test the --cache-debug option to see if it prints the expected messages.
@@ -145,16 +144,16 @@ def cat(env, source, target):
 
 expect = \
 r"""Retrieved `aaa.out' from cache
-CacheRetrieve\(aaa.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(aaa.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 Retrieved `bbb.out' from cache
-CacheRetrieve\(bbb.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(bbb.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 Retrieved `ccc.out' from cache
-CacheRetrieve\(ccc.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(ccc.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 Retrieved `all' from cache
-CacheRetrieve\(all\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(all\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 """
 
@@ -179,13 +178,13 @@ def cat(env, source, target):
          stdout=expect)
 
 expect = \
-r"""CacheRetrieve\(aaa.out\):  retrieving from [0-9a-fA-F]+
+r"""CacheRetrieve\(aaa.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
-CacheRetrieve\(bbb.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(bbb.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
-CacheRetrieve\(ccc.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(ccc.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
-CacheRetrieve\(all\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(all\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 """