recover from corrupt repos #596

marwan-at-work · 2017-05-17T13:44:18Z

This PR ensures local caches in $GOPATH/pkg/dep/sources are removed and recloned in case they are corrupt. As @sdboyer suggested, the next step is to create a larger subsystem for error handling.

I'm also thinking of adding maybe_source_test.go as well as the corrupt cache dependency that prompted this PR. This way, the test suite can copy the corrupt dependency to $GOPATH/pkg/dep/sources then tries to call newGitRepo on it and assert that it got properly removed and no errors returned.

googlebot · 2017-05-17T13:44:22Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

marwan-at-work · 2017-05-17T13:45:41Z

I signed it!

googlebot · 2017-05-17T13:45:44Z

CLAs look good, thanks!

ascandella · 2017-05-21T19:12:59Z

internal/gps/vcs_repo.go

+			// if vcs could not initialize the repo due to a local error
+			// then the local repo is in an incorrect state. Remove and
+			// treat it as a new not-yet-cloned repo.
+			os.RemoveAll(path)


Check for error on this?

@sectioneight i don't think there's a need to check for this particular error, since os.RemoveAll is an attempt to recover from the parent error.

If the folder could not be removed, then it might be better to return the parent error than this one.

Not a strong opinion though, let me know if there's a better reason to handle this error.

We should at least log something IMHO. If somebody runs into this case they're already in a very strange state, and running dep with -v should output as much as possible for why things aren't going "right". Not sure how easy it would be to plumb the logger in, so maybe not worth it in this PR, just generally get scared of widely-used tools that silently drop errors.

No direct logging from within gps! 📏😄

This is a case where we really want warnings, per #534 (I've already added a bullet item for it). Let's put a TODO in indicating that this should generate a warning, and call it enough for now.

ascandella · 2017-05-21T19:22:20Z

internal/gps/vcs_repo.go

+
+	return &hgRepo{r}, nil
+}
+


Lots of duplication here... the functions basically vary on vcs.New*Repo and their return type. Since the return type is just shoved into a baseVCSSource which defines repo as a ctxRepo interface you can significantly simplify this by making a single function that takes in a ctor, ustr, and path, and returns a ctxRepo (I think).

Rough sketch, not tested at all:

type repoConstructor func(ustr, path string) (ctxRepo, error) func newRepo(ustr, path string, fn repoConstructor, typeFn repoCreator) (ctxRepo, error) { r, err := fn(ustr, path) if err != nil { // as before ... } // as before return typeFn(r), nil }

type typeFn func(*vcs.Repo) ctxRepo func newHgRepo(r *vcs.Repo) ctxRepo { return &hgRepo{r} } // etc

@sectioneight that was my thinking as well. But I was torn between DRY and readability lol. Makes sense tho, I'll go ahead and update the pr.

I'm 👍 to @sectioneight's suggestion. There's a lot in this part of the codebase that we more or less have to duplicate, so when we come across something that we can add a little reasonable indirection to be DRY with, it's generally a good idea.

ascandella · 2017-05-21T22:12:19Z

It's up to you and Sam, just thought I'd point it out in case you hadn't seen that pattern. Agreed it will harm readability.

…

On Sun, May 21, 2017 at 2:44 PM Marwan Sulaiman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In internal/gps/vcs_repo.go <#596 (comment)>: > + // if vcs could not initialize the repo due to a local error + // then the local repo is in an incorrect state. Remove and + // treat it as a new not-yet-cloned repo. + if _, ok := err.(*vcs.LocalError); ok { + os.RemoveAll(path) + r, err = vcs.NewHgRepo(ustr, path) + } + } + + if err != nil { + return nil, err + } + + return &hgRepo{r}, nil +} + @sectioneight <https://github.com/sectioneight> that was my thinking as well. But I was torn between DRY and readability lol. Makes sense tho, I'll go ahead and update the pr. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#596 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7PyRPHmftb1yCYnKS5TtwtMygcvUNks5r8LAkgaJpZM4Nd4pe> .

marwan-at-work · 2017-05-21T22:14:59Z

@sectioneight no probs, thanks for taking a look! Will wait to hear from Sam/others.

ascandella · 2017-05-21T22:31:54Z

Ya, I'm very new to this project as well, and I know Sam is a very busy guy... hopefully soon enough I will have enough expertise that I can properly approve/request changes on diffs. He is very friendly and should have some feedback to you within the next day or so, you can always try pinging on Slack if you want others to look...today is Sunday so I wouldn't expect much

sdboyer · 2017-05-23T03:24:13Z

internal/gps/vcs_repo.go

+			// if vcs could not initialize the repo due to a local error
+			// then the local repo is in an incorrect state. Remove and
+			// treat it as a new not-yet-cloned repo.
+			os.RemoveAll(path)


No direct logging from within gps! 📏😄

This is a case where we really want warnings, per #534 (I've already added a bullet item for it). Let's put a TODO in indicating that this should generate a warning, and call it enough for now.

sdboyer · 2017-05-23T03:27:08Z

internal/gps/vcs_repo.go

+
+	return &hgRepo{r}, nil
+}
+


I'm 👍 to @sectioneight's suggestion. There's a lot in this part of the codebase that we more or less have to duplicate, so when we come across something that we can add a little reasonable indirection to be DRY with, it's generally a good idea.

sdboyer · 2017-05-23T17:57:37Z

whoops - added coverage file 😄

marwan-at-work · 2017-05-23T18:14:04Z

@sdboyer lmao, i removed the coverage file. should the tests clean up after themselves? i could take that on as the next issue lol

sdboyer · 2017-05-23T18:38:46Z

@marwan-at-work hmm, the coverage file should only be generated by the commands run by travis - standard way for just running tests locally is go test -race $(go list ./... | grep -v vendor), which shouldn't make any such files.

marwan-at-work · 2017-05-23T18:59:46Z

@sdboyer gotcha. sorry i was blindly copying the test commands from travis hahah.

sdboyer · 2017-05-23T19:27:24Z

no worries...probably something we should better document :D

marwan-at-work · 2017-05-23T19:30:07Z

@sdboyer sounds good (i can push a readme pr hahah). lemme know if this pr needs any updates. thanks!

sdboyer · 2017-05-23T19:36:06Z

I think tests are probably all we're missing. It's especially important for things like this, where we want to be sure that we have at least some idea about the cases that will trigger the automated recovery.

We needn't cover every possibility, but we need to cover at least some. All the better if they're realistic possibilities 😄

marwan-at-work · 2017-05-23T19:38:30Z

@sdboyer cool. I was thinking of adding the corrupt repo as an artifact to the testdata folder.
And run the newCtxRepo function in a test case, then assert the folder got removed and fixed. How does that sound?

sdboyer · 2017-05-23T23:52:42Z

@marwan-at-work ah yes! that should work well. best to add it as an archive (e.g. archive/tar), then extract it into place as part of the test setup.

sdboyer · 2017-05-24T17:51:00Z

Is a follow-up commit coming that wraps those files up into a .tar archive?

marwan-at-work · 2017-05-24T17:59:33Z

@sdboyer ah nice catch. However, the entire _testdata folder looks very similar to the folder i uploaded (aka no tar, just straight up folders/files). Are we going to slowly turn those also into tars?

ps. looking into why fs cannot be imported in the test suite. worked fine locally :(

sdboyer · 2017-05-24T18:50:50Z

Are we going to slowly turn those also into tars?

Nope - my goal with doing it here is really to avoid any situation that even appears to nest repositories inside repositories. Now, we can avoid it by dropping the leading . (as you've done), but the point here is more semantic than functional: when we tar up a whole repo, it's in keeping with the basic idea that the logic in dep itself isn't really concerned with the internals of vcs repository structure. What's in there is a bit of a black box to us. Archiving it up sends this signal much better than suddenly and seamlessly entering a file layout that resembles a git repository.

marwan-at-work · 2017-05-24T21:20:45Z

@sdboyer sounds good to me, i just pushed a change. let me know if the code/file structure makes sense. thanks!

sdboyer

couple nits and some file organization stuff

sdboyer · 2017-05-25T00:30:09Z

internal/gps/vcs_repo_test.go

+		t.Fatal(err)
+	}
+
+	src := filepath.Join(cwd, "_testdata", "src", "corrupt", "corrupt_dot_git_directory.tar")


Let's not put it in the src directory, actually - I realize this is totally not documented, but all the dirs in there are basically fodder for ListPackages(). Instead, let's put it in internal/gps/_testdata/badrepo/corrupt_dot_git_directory.tar

Also - for our future selves' sake, let's add a internal/gps/_testdata/badrepo/README.md that outlines the purpose of the directory (to hold various forms of malformed/corrupted repo archives), and for each archive file (so for now, just one) gives a summary of the way in which each archived repo is broken.

sdboyer · 2017-05-25T00:31:19Z

internal/gps/vcs_repo_test.go

@@ -46,6 +52,53 @@ func TestErrs(t *testing.T) {
 	}
 }

+func TestNewCtxRepo(t *testing.T) {


Let's split this into two tests, one for the simple happy path, and the other for the failure + recovery path.

just fyi, this particular test has no happy path. It's just the failure+recovery part. So it won't be a split per se. more like adding a new test that is a happy path one. Adding one : )

sdboyer · 2017-05-25T00:32:44Z

internal/gps/vcs_repo.go

+		// then the local repo is in an incorrect state. Remove and
+		// treat it as a new not-yet-cloned repo.
+
+		// TODO(marwan-at-work): pass a logger here to warn/give progress of the above comment.


Let's be a little more vague about the solution in the TODO - passing a logger is not the preferred design goal, per my other comments.

marwan-at-work · 2017-05-25T03:20:18Z

@sdboyer done 🎈

sdboyer · 2017-05-31T03:28:27Z

OK, I think this is in good enough shape.

I feel like I'm missing a path while reviewing, but nothing's jumping out at me, and I don't want to wait to merge until it does. If something goes wrong, we can figure it out and fix it.

sdboyer · 2017-05-31T03:28:43Z

oh, right - 🎉 🎉 thank you!

recover from corrupt repos

25fcbfc

googlebot added the cla: no label May 17, 2017

googlebot added cla: yes and removed cla: no labels May 17, 2017

copy comments

df5c648

ascandella reviewed May 21, 2017

View reviewed changes

sdboyer mentioned this pull request May 23, 2017

dep ensure: Unable to update checked out version #618

Closed

fabulous-gopher mentioned this pull request May 23, 2017

Systematic support for "warnings" #534

Closed

17 tasks

sdboyer suggested changes May 23, 2017

View reviewed changes

pr fixes

7854ebf

marwan-at-work force-pushed the local-recovery branch from 92e5f21 to 7854ebf Compare May 23, 2017 18:11

marwan-at-work force-pushed the local-recovery branch from 616dc66 to 25c26cd Compare May 24, 2017 20:42

add newCtxRepo test cases

ff64da3

marwan-at-work force-pushed the local-recovery branch from 25c26cd to ff64da3 Compare May 24, 2017 20:48

sdboyer suggested changes May 25, 2017

View reviewed changes

add happy path test + readme

a0db261

marwan-at-work force-pushed the local-recovery branch from b26ae81 to a0db261 Compare May 25, 2017 02:44

change tmepdir name for TestNewCtxRepoHappyPath

ce46ed0

sdboyer approved these changes May 31, 2017

View reviewed changes

sdboyer merged commit 94e95d2 into golang:master May 31, 2017

carolynvs mentioned this pull request May 31, 2017

detect failed directories in /pkg/dep/resources #583

Closed

sdboyer mentioned this pull request Aug 1, 2017

dep ensure --update is not repeatable #927

Closed

recover from corrupt repos #596

recover from corrupt repos #596

Conversation

marwan-at-work commented May 17, 2017

googlebot commented May 17, 2017

marwan-at-work commented May 17, 2017

googlebot commented May 17, 2017

Choose a reason for hiding this comment

marwan-at-work May 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascandella commented May 21, 2017 via email

marwan-at-work commented May 21, 2017

ascandella commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented May 23, 2017

marwan-at-work commented May 23, 2017

sdboyer commented May 23, 2017

marwan-at-work commented May 23, 2017

sdboyer commented May 23, 2017

marwan-at-work commented May 23, 2017 • edited Loading

sdboyer commented May 23, 2017

marwan-at-work commented May 23, 2017

sdboyer commented May 23, 2017

sdboyer commented May 24, 2017

marwan-at-work commented May 24, 2017 • edited Loading

sdboyer commented May 24, 2017

marwan-at-work commented May 24, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marwan-at-work commented May 25, 2017

sdboyer commented May 31, 2017

sdboyer commented May 31, 2017

marwan-at-work May 21, 2017 •

edited

Loading

marwan-at-work commented May 23, 2017 •

edited

Loading

marwan-at-work commented May 24, 2017 •

edited

Loading