Improve handling of inconsistent immutable workspaces #28503
Conversation
|
@bot-gradle test QF please. |
|
I've triggered the following builds for you. Click here to see all build failures. |
| boolean moveSuccessful = workspace.withTemporaryWorkspace(temporaryWorkspace -> moveInconsistentImmutableWorkspaceToTemporaryLocation(immutableLocation, temporaryWorkspace, outputSnapshots)); | ||
| if (moveSuccessful) { | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
💭 This is the heart of the change: if we find an inconsistent workspace, we don't throw, but instead try to move it out of the way. If we managed to move it out of the way, we act as if no existing workspace was found (Optional.empty()). If we fail to move, we fall back to trying to reuse what we found.
| "These workspace directories are not supposed to be modified once they are created. " + | ||
| "Deleting the directory in question can allow the content to be recreated."); | ||
| boolean moveSuccessful = workspace.withTemporaryWorkspace(temporaryWorkspace -> moveInconsistentImmutableWorkspaceToTemporaryLocation(immutableLocation, temporaryWorkspace, outputSnapshots)); | ||
| if (moveSuccessful) { |
There was a problem hiding this comment.
❓ Is it ok we continue if move was not successful or should we fail in that case?
There was a problem hiding this comment.
Yes, I added a clarifying comment.
There was a problem hiding this comment.
Thinking about this a bit more, I agree that this is weird. It's also a corner case, and I can't even imagine how it could possibly be triggered. Let's just fall back to failing the build in that case.
There was a problem hiding this comment.
Clarification: I can only imagine catastrophic ways this could fail, i.e. disk full, permissions on cache directory have changed etc. I can't come up with a scenario we would be expected to recover from.
|
The merge queue build has started. Click here to see all failures if any. |
|
The merge queue build has started. Click here to see all failures if any. |
Improves #28475
Previously when we detected an inconsistent immutable workspace, we'd fail the build. Now instead we attempt to move the offending workspace to a temporary location, and re-try re-creating the content. When this happens, the following deprecation warning is produced:
This makes the build more resilient against consistency problems caused by disk corruption, or external processes making changes to the workspaces.
In a separate PR we still need to fix cache cleanup causing problems, see details at #28475 (comment).
Reviewing cheatsheet
Before merging the PR, comments starting with