Fix by by-hash index cleanup + root dir data "loss" #706
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
previously it'd return an absolute path which makes the path absolutely useless as all other functions of PublishedStorage need relative input and will prepend them with the rootPath, so getting an absolute ReadLink and then trying to remove that'd would ultimately try to remove the absolute path `$root/AbsoluteRoot/LinkTarget` instead of `$root/LinkTarget` add a unit test to actually verify readlink
by using the power of variables!
the logic here was wrong.
if we managed to find the link target (the physical index file) pointed to
by our old symlink we want to remove it (this is basically "cleaning up old
index" logic).
previously we'd try to only delete it when the ReadLink came back with
error. which had two serious issues with it:
a) linkTarget was empty, so we basically called Remove("") which would
delete the storage -> root <- directory if the root is a symlink!
b) we'd leak old indexes as the cleanup logic only ran if there was en
error which would ordinarily never be
new code correctly cleans up unless there was an error.
this relates to a previous bugfix of readLink which incorrectly returned
absolute paths ultimately rendering the Remove call also broken.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
All new code should be covered with tests, documentation should be updated. CI should pass.
Description of the Change
the logic here was wrong.
if we managed to find the link target (the physical index file) pointed to
by our old symlink we want to remove it (this is basically "cleaning up old
index" logic).
previously we'd try to only delete it when the ReadLink came back with
error. which had two serious issues with it:
a) linkTarget was empty, so we basically called Remove("") which would
delete the storage -> root <- directory if the root is a symlink!
b) we'd leak old indexes as the cleanup logic only ran if there was en
error which would ordinarily never be
new code correctly cleans up unless there was an error.
readlink was also entirely wrong and adjusted to work correctly within the expectations of a publishedstorage
(I presently don't have time to poke around to make the functional test assert that the cleanup works, not sure we really need to care all that much either... I'll probably not get to it this week but this bug is fairly serious as it basically blew up our aptly earlier today)
This also relates to #705 which introduces a guard against part of the underlying problem here (Remove being content with using an empty path)
Checklist