Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reinitialize index on "Object not found" error. #8735

Merged
merged 1 commit into from
Sep 27, 2020

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 26, 2020

Fixes #4007

Users have occasionally been reporting "Object not found" errors when updating the index. This PR changes cargo to detect this error, and delete the index and attempt one more time to update it. Our best theory is that the git repo is getting corrupted or out-of-sync somehow.

Other options
We talked about having cargo generate a ZIP file of the corrupt repo and ask the user to upload it to the issue tracker, but I feel like that isn't going to be too useful (there will be an object missing in the repo, which is unlikely to tell us how to got lost).

We could also implement some tricks to make the fetch process more atomic (by renaming the git directory before starting the fetch), but I'm uncertain if the added complexity is justified.

Another option (which I personally like) is to use net.git-fetch-with-cli by default if git is found in PATH. It is faster and more reliable and handles authentication better, and I suspect the vast majority of developer and CI systems have git installed. This kinda sweeps the problems of libgit2 under the rug, and would mean a huge amount of code would no longer be exercised by most users, leaving the few without git to be more likely to suffer obscure bugs.

Testing
Note that I was unable to create a local test to reproduce this, but I was able to reproduce against GitHub. I took my index repo and removed the most recent pack file, and then ran cargo fetch. This resulted in the exact same error users are reporting. I believe I cannot repro locally because the network update code is significantly different from the file:// update code (there's all sorts of negotiation that happens over the protocol). Unless anyone has ideas on how to repro this in an automated fashion, I'm out of ideas.

Other corruption
In testing, I noticed there are a few other ways the "Object not found" error can happen, but this does not address them. Both cases involved deleting pack files:

  1. After deleting the oldest pack file in an index, running an update, the index fetch succeeds. But then, when the RemoteRegistry attempts to load the config.json file, it can't find it (it fails here).

  2. After deleting the newest pack file of a git dependency in the db directory, the fetch succeeds, but then the call to reset of the checkout fails (around here).

Fixing these I think will be require a bit of work, since retry loops will need to be added. I'm not too eager to do that since nobody has reported an error with either of these cases (the error stack is slightly different).

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2020
@ehuss
Copy link
Contributor Author

ehuss commented Sep 26, 2020

As an aside, I did a little performance testing of libgit2 vs git-cli. Some sample times (I tried all tests twice, and got reproducible times):

Fresh fetch
    with-cli: 32s
    libgit2: 37s

Fetch with very old index
    with-cli: 32s
    libgit2: 39s

incremental (about 1 day old)
    with-cli: 2s
    libgit2: 4s

@ehuss
Copy link
Contributor Author

ehuss commented Sep 26, 2020

Oh, I forgot to mention, for my repro against GitHub, running with net.git-fetch-with-cli works correctly. The CLI appears to re-download the entire index when it detects the corruption.

@alexcrichton
Copy link
Member

Thanks for the investigation here! This was actually a much smaller change than I thought that it was going to be. I also think it's a good find that the CLI is basically redownloading the whole index like we are, and I also think it's nice that you were able to deterministically reproduce. I agree that getting zip/tarballs isn't too useful since I mainly just wanted to confirm that the CLI fixed it and/or poke around libgit2 a bit more.

I'm still hesitant to use the CLI by default for the reasons you pointed out. It may still come to that eventually but if we can I'd like to hold out for as long as possible.

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 27, 2020

📌 Commit 3bbb44c has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2020
@bors
Copy link
Contributor

bors commented Sep 27, 2020

⌛ Testing commit 3bbb44c with merge 76c0161...

@bors
Copy link
Contributor

bors commented Sep 27, 2020

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 76c0161 to master...

@bors bors merged commit 76c0161 into rust-lang:master Sep 27, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 30, 2020
Update cargo

8 commits in 05c611ae3c4255b7a2bcf4fcfa65b20286a07839..75615f8e69f748d7ef0df7bc0b064a9b1f5c78b2
2020-09-23 23:10:38 +0000 to 2020-09-29 18:42:19 +0000
- Correct mistake about supporting sub-makes and document CARGO_MAKEFLAGS (rust-lang/cargo#8741)
- Properly set for_host for proc-macro tests. (rust-lang/cargo#8742)
- Add Zsh completion for target triples (rust-lang/cargo#8740)
- Reinitialize index on "Object not found" error. (rust-lang/cargo#8735)
- Normalize raw string indentation. (rust-lang/cargo#8739)
- Update links to rustup docs. (rust-lang/cargo#8738)
- Add contributor guide. (rust-lang/cargo#8715)
- Fix minor error in `cargo update` docs. (rust-lang/cargo#8737)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 1, 2020
Update cargo

8 commits in 05c611ae3c4255b7a2bcf4fcfa65b20286a07839..75615f8e69f748d7ef0df7bc0b064a9b1f5c78b2
2020-09-23 23:10:38 +0000 to 2020-09-29 18:42:19 +0000
- Correct mistake about supporting sub-makes and document CARGO_MAKEFLAGS (rust-lang/cargo#8741)
- Properly set for_host for proc-macro tests. (rust-lang/cargo#8742)
- Add Zsh completion for target triples (rust-lang/cargo#8740)
- Reinitialize index on "Object not found" error. (rust-lang/cargo#8735)
- Normalize raw string indentation. (rust-lang/cargo#8739)
- Update links to rustup docs. (rust-lang/cargo#8738)
- Add contributor guide. (rust-lang/cargo#8715)
- Fix minor error in `cargo update` docs. (rust-lang/cargo#8737)
@ehuss ehuss added this to the 1.48.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Object not found" updating registry
4 participants