Cache corruption protection: Atomic replacement of manifests, statistics and object files

[siu]

In this PR the writing of manifests, statistics and object files is done following this pattern:

• Write to temporary file
• Call atomic os.replace(tmp, dst)

According to the documentation and what I read on the internet os.replace(...) is an atomic operation, meaning that it can be used to prevent cache corruption if the process is stopped in the middle of a write operation. This should help with the root cause of #263.

In addition, if writing files is atomic, reading those files can be done without locking. The last 3 commits in the PR exploit that fact to avoid locking the manifests or statistics for reading. This may need some more testing, so I am fine taking them to another PR. Theoretically this would make processDirect faster with a cold cache because it doesn't need to lock the manifest to decide that it is a miss. My measurements show a 3% faster compilation times in that case but I am not sure if it is relevant do not show any relevant change in performance. Another benefit of not locking is that clcache -s does not hang until it can lock the statistics file.

I tested these changes locally and I didn't notice any problem.

[frerich]

This looks interesting! Working on concurrency is always tricky business, so I'll need to think a bit about these patches to see whether I can understand what's going on. My first thought was that I recalled a comment explaining that os.replace is subject to some limitations (I only now noticed it was you who wrote that comment :-) ) but I see that the issue related to renaming files across drives should not kick in here since you always store the temporary file on the same drive.

[frerich]

Just a quick heads up: alas, I still didn't find a quiet moment to review your changes. This kind of patch can be quite subtle so I'd rather do a careful review instead of just a superficial look. ;-)

[siu]

Sure, there is no rush. I was considering making the writes to the cache dir atomic following the same pattern: writing to temporary directory + atomic replace but I didn't have time to implement anything useful yet.

[frerich]

I'm not sure this function pulls its own weight; atomicRename is even longer than os.replace and chances are that the latter is already known to readers of the code.

[siu]

ok, replaced by the context manager and os.replace(...)

[frerich]

If writing the statistics happens atomically now, is it really necessary to lock?

[siu]

Yes, I think so. In general writes need to be protected to avoid other processes updating the information at the same time (based on old information). It is a bit different in this specific part of the code because the resulting statistics is all zeroes but we need to lock anyway because the atomic write always writes first to outputPath + ".new" and we need to prevent two processes doing so at the same time.

We could avoid the lock here by making atomicWrite write to new temporary unique files but I don't think it is worth it at the moment.

[siu]

Thinking a bit more, even if the temporary file did not always have the same name we would need to lock. We don't want another process to write old information right after we reset the statistics.

[frerich]

It's true that the way the temporary file name is currently constructed requires locking. However, I'm not sure I appreciate with your second comment yet. Can you give an example of how two concurrent clcache processes (one of which is merely resetting the statistics) might interact which yields broken statistics in case no locking is performed when resetting the statistics? The only potential issue I could think of was:

1. clcache instance A reads the statistics in order to bump a counter (e.g. increase the number of cache hits)
2. clcache instance B resets the statistics
3. clcache instance A writes out the new statistics, with an increased counter

If this were to happen, the number of cache hits at the end would not be 1 plus whatever the value was before the stats were reset.

However, from looking at the code, all the updates to the cache stats seem to follow the theme

   with cache.statistics.lock, cache.statistics as stats:
            stats.registerCacheHit()

I.e. reading, updating and writing the cache is always an atomic operation.

[siu]

Based on your example, instance A is going to register a cache hit and instance B is going to reset the statistics (without the lock):

1. A: acquires the lock
2. A: reads current statistics, number of hits = 123
3. B: resets statistics without acquiring the lock, number of hits on disk = 0
4. A: writes updated statistics, number of hits on disk = 123 + 1

Resulting in incorrect data. If resetStatistics() acquires the lock this situation is not possible as either A or B would have to wait for the other process to finish the write.

1. A: acquires the lock
2. A: reads current statistics, number of hits = 123
3. B: about to reset statistics, blocks trying to acquire the lock
4. A: writes updated statistics, number of hits on disk = 123 + 1
5. A: releases the lock
6. B: succeeds acquiring the lock, writes all zeros in statistics
7. B: releases the lock

or

1. B: acquires the lock
2. A: about to bump cache hits, blocks trying to acquire the lock
3. B: writes all zeros in statistics file
4. B: releases the lock
5. A: succeeds acquiring the lock
6. A: reads current statistics, number of hits = 0
7. A: writes updated number of hits = 0 + 1
8. A: releases the lock

[frerich]

D'oh, of course you're right. My own example was giving an argument in favor of locking here.

[frerich]

I think you could shorten this to

entryIndex = next((i, e in enumerate(self.entries()) if e.objectHash == objectHash), 0)

[siu]

Indeed.

[frerich]

Seeing this repetition makes me wonder whether it might be nice to have a little context manager like

@contextlib.contextmanager
def atomicallyWritten(fn):
    tempFileName = fn + ".new"
    with open(tempFileName, 'w') as f:
        yield f
    os.replace(tempFileName, fn)

...so you could just replace open(self._fileName, 'w') with atomicallyWritten(self._fileName). Maybe not worth it, given that it's just two occurrences. I'm mostly thinking out loud.

[siu]

It makes sense, let's unify it for the moment, it can be unfolded later on if it is not needed anymore.

[frerich]

I think I understand it all now and it seems plausible to me. Just one thing I was wondering:

There are still two places where files are written were you do not use atomicWrite yet. Is this intentional? I found CompilerArtifactsSection._setCachedCompilerConsoleOutput and CacheFileStrategy.deserializeCacheEntry.

[siu]

I left them out only partly on purpose as I feel that those are less important and require more discussion.

The cache artifacts are 3 files (object, stdout, stderr), if we want to never corrupt the cache if the process is killed in the middle of a write we would:

• Write the 3 files to a temporary folder
• Replace the old folder by the new one

Which would guarantee that the 3 files are always in sync. Currently we have the atomic replacement of object file (which is the biggest of the 3) but no protection for the other 2 and no protection for the cache entry as a whole. I think that replacing the whole cache entry is more interesting than adding individual file protection now but I would do that in another PR if you agree.

After the cache entry is replaced atomically we can remove the signal handlers ;)

[frerich]

Makes sense!

[frerich]

Merged this now, thanks a lot for your work!