Added error message when failure to delete partial cache retrieval occurs

[dmoody256]

Mongodb was running into cases where the partial cache file still existed in a bad state after a bad retrieval. The lack of any error message around the related code in this PR made it difficult to understand what was happening. This error message should give some more obvious insight into what took place and why the failed retrieval file still exists after a failed retrieval.

Contributor Checklist:

• [ ] I have created a new test or updated the unit tests to cover the new/changed functionality.
• [x] I have updated CHANGES.txt (and read the README.rst)
• [ ] I have updated the appropriate documentation

[bdbaddog]

@dmoody256 Should this be an error instead of a warning?

[mwichmann]

Can you recover? That is, any "bad cache retrieve" operation could just mean "forget about the cache entry, build instead", but is that possible at this point, or is there no way back once you've already committed to getting the object from cache? If it's recoverable, it should only be a warning

[bdbaddog]

@mwichmann good point. I think the answer is "it depends", if the file can be overwritten when the actual builder runs, then all's fine, but if it can't then it should error out at that point, which should be right after the warning in this PR..?

[dmoody256]

@mwichmann good point. I think the answer is "it depends", if the file can be overwritten when the actual builder runs, then all's fine, but if it can't then it should error out at that point, which should be right after the warning in this PR..?

Yeah, it is "it depends". Most cases the builder will be able to overwrite the file and all's good, there are cases, like AR, where its not an overwrite, or even more rare, filesystem issues or something. I usually always prefer hard fails, so am for switching to an error.

[bdbaddog]

@dmoody256 - I agree. my motto for tools is almost always "Fail Fast and immediately" when there's an error.

Only quandry is how to test this.. thoughts?

[bdbaddog]

I think this is a way to create a unit test. TaskmasterTests.py has TaskmasterTestCase.test_cached_execute() test, the new code will only be tested by having 2 targets, one of which is cached, and the for the node which is cached, when trying to unlink() it, a mocked node.fs.unlink() will have to raise an IOError or OSError, and then check for the warning in question to be issued.