[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,}%
 """