Delete workload-related things if the rest of the operation was unsuccessful #47110
Conversation
ghost
added
| FileAccessRetrier.RetryOnMoveAccessFailure(() => DirectoryPath.MoveDirectory(tempBackupDir, targetFolder)); | ||
| } | ||
| else if (!directoryExists) | ||
| { | ||
| Directory.Delete(targetFolder, recursive: true); |
There was a problem hiding this comment.
In the cleanup rollback logic, the code deletes the target directory without checking if it exists. To prevent a potential DirectoryNotFoundException, consider adding a conditional check using Directory.Exists(targetFolder) before calling Directory.Delete.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is checked via directoryExists
There was a problem hiding this comment.
Theoretically, directoryExists could be false if the directory existed but was empty if I'm reading the call right. That isn't related to what copilot is chatting about but just something I noticed.
There was a problem hiding this comment.
But if the directory did not exist, we'd still try to delete it. That would throw.
There was a problem hiding this comment.
There are five cases that cover all possibilities:
Directory exists and is not empty, crash after line 332: directoryExists is true, but this is overwritten via the first 'if', so this doesn't really change anything (rollback is already correct)
Directory exists and is not empty, crash before line 332: directoryExists is true, and neither this new rollback code nor the old rollback code run. Rollback is already correct.
Directory exists and is empty: directoryExists is false. We delete whatever we find. The directory should not exist if it's empty, so though this is a minor change, I think it's slightly beneficial
Directory does not exist, crash after line 332: directoryExists is false, and this deletes the new folder copied to this location. This is currently broken, fixed by this PR
Directory does not exist, crash before line 332: directoryExists is false, and this throws erroneously.
I actually did test that last scenario, and it didn't cause any problems. I looked into why that doesn't cause actually matter in this case. This is the last part of the rollback code, so it doesn't affect this particular rollback action. Each rollback action is executed separately in a try/catch by CliTransaction, so it catches this exception. It then executes RollbackFailed, which in our case just prints out a message and continues, so the exception is essentially ignored, and this bug is roughly irrelevant until we change any part of this just slightly 🙂
I think that's still worth changing, though it's interesting that we have enough try/catches around this that it actually doesn't matter until we change the code again.
|
/azp run sdk-unified-build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Using a file-based installer, we assume that certain components will update in tandem. If a workload set is successfully installed, for instance, that means all the manifests were successfully installed. To effect this, we have a CliTransaction. If some part of the CLI transaction fails, the whole transaction executes rollback logic.
The rollback logic for installing some kinds of workload-related things—I'm trying not to say "packages" because that sounds like workload packs, which actually are not affected by this issue—involves overwriting the current version with what was there before. If there was nothing there before, however, we just leave it. That means, as an example, that if:
Then the workload set is not properly uninstalled, and users end up in a broken state where the only solution I've found at this point is to delete a specific folder in your SDK installation, which is not a workaround I'd feel particularly comfortable with giving users.
After this PR, I intend to look at previous versions of the SDK to see where this was introduced to help decide if this is worth backporting. I have not proven this, but I have reason to suspect that this is only an issue when using workload sets—not because workload sets themselves are broken but because they go through the same installation procedure as workload manifests except workload manifests don't rely on each other as much, and the packs are rolled back properly. For that reason, I don't think this should be needed in previous versions of the SDK, or rather it shouldn't be needed at high priority, but I do want to validate that.