Add executable permissions to directories when attempting to clean up build directory

[staticfloat]

Problem

If a build does something silly like chmod 0444 ./subdir, attempts to clean up the build directory will fail with messages like:

# Removing /Users/julia/buildkite-agent/build/julia-master
🚨 Error: Failed to remove "/Users/julia/buildkite-agent/build/julia-master" (fstatat /Users/julia/buildkite-agent/build/julia-master/subdir/Manifest.toml: permission denied)

Then, subsequent git checkouts will fail (because that build directory is not empty) and the cleanup process will fail all over again. In our case, this happened due to a misconfigured find . -name \*.jl -exec chmod 0444 {} \; which found directories with names that ended in .jl instead of source code files like we intended. Unfortunately, this then required manual intervention on the build agent machines to clear out those bad permissions to allow the build to successfully go through.

Potential solution

I propose that when buildkite is attempting to clear out the build directory, that it should add execute (and write) permissions to all subdirectories, e.g. the equivalent of:

find . -type d -exec chmod u+wx {} \;

This will allow normal directory removal code to then process the repository. I don't think there's any valid usecase for allowing user code to create things that cannot be deleted like this, as it badly breaks the ability of buildkite to deal with the build directory.

[CerealBoy]

Hi @staticfloat, this sounds like a reasonable idea to an interesting problem, though we would be cautious to add this type of change to the agent as it could easily have a negative impact on other customers unique setups.

Rather, this could be resolved using a hook to make the permission changes required for this specific scenario. It could be part of the pre-checkout or post-command hook to make the permission changes required. Would this be suitable for your situation?

[staticfloat]

though we would be cautious to add this type of change to the agent as it could easily have a negative impact on other customers unique setups.

Can you explain how this could have a negative impact? I don't quite see how this could have a negative impact, because as far as I can tell, if buildkite attempts to remove the build directory (due to a failed checkout or any other reason) and fails, the agent then enters a state that it cannot recover from. If buildkite is already attempting to delete the entire build directory, I can't see why it would be harmful to modify permissions of the files about to be deleted.

It could be part of the pre-checkout or post-command hook to make the permission changes required.

While yes I can work around this with hooks, in my mind this feels like incomplete behavior from buildkite's side. Yes, it's an unusual situation to have directories without executable or writable permissions in the build directory, but in a similar vein to how buildkite kills the entire process group at the end of the build to ensure there are no sneaky processes persisting past the end of the build, if the build directory needs to be cleaned out, it seems reasonable to expect buildkite to try as best it can to remove those files.