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

Crates that are fetched but not added to the index because of a crash will be lost #628

Closed
jyn514 opened this issue Mar 1, 2020 · 10 comments · Fixed by #746
Closed

Crates that are fetched but not added to the index because of a crash will be lost #628

jyn514 opened this issue Mar 1, 2020 · 10 comments · Fixed by #746
Labels
C-bug Category: This is a bug E-easy Effort: Should be easy to implement and would make a good first PR mentor This has instructions for getting started

Comments

@jyn514
Copy link
Member

jyn514 commented Mar 1, 2020

Secondary problem: apparently crates that are fetched but not added to the index will be lost and not build. Relevant code: https://github.com/rust-lang/docs.rs/blob/master/src/docbuilder/queue.rs#L13

Originally posted by @jyn514 in #626 (comment)

crates-index-diff just released a change in 5.1.0 that will let us peek without consuming the changes 🎉 🎉 . This will give us much better durability in the event of a crash, I just need to implement using the new API.

@jyn514
Copy link
Member Author

jyn514 commented Mar 1, 2020

Mentoring instructions:

@jyn514 jyn514 added C-bug Category: This is a bug E-easy Effort: Should be easy to implement and would make a good first PR mentor This has instructions for getting started labels Mar 1, 2020
@harrisonbrock
Copy link

If this is still open, I would like to work on it.

@jyn514
Copy link
Member Author

jyn514 commented Mar 1, 2020

@harrisonbrock go for it! If you have questions, feel free to reach out here or on Discord :)

@jyn514
Copy link
Member Author

jyn514 commented Mar 6, 2020

There was a new API added recently that will make this a little easier: https://docs.rs/crates-index-diff/6.2.0/crates_index_diff/struct.Index.html#method.set_last_seen_reference

Also note that we currently use crates-index-diff 4.0, so it will need to be upgraded to at least 5 (preferably 6) to use this new API.

@harrisonbrock
Copy link

Thanks. I have been a little sick and have not had a chance to start yet.

@jyn514
Copy link
Member Author

jyn514 commented Mar 6, 2020

Get well soon! Your health is more important than code :)

@Byron
Copy link
Member

Byron commented Mar 20, 2020

I hope you are doing better, @harrisonbrock!
In case you need additional support related to how to use crates-index-diff, I am happy to help (I have authored crates-index-diff).

jyn514 added a commit to jyn514/docs.rs that referenced this issue Mar 29, 2020
This removes the dependency on libssh2-sys. It also makes it possible to
implement rust-lang#628.
@jyn514
Copy link
Member Author

jyn514 commented Jul 19, 2020

#746 only logged the failure, it will still not be added to the index.

@jyn514 jyn514 reopened this Jul 19, 2020
@jyn514
Copy link
Member Author

jyn514 commented Jul 19, 2020

Ok, this is a slightly different issue than originally - we record the failure internally, it won't be lost, we just don't display it anywhere but in the logs. So it seems from the outside as if the build was never there, but we can still rerun it with UPDATE queue SET attempts = 0;

@jyn514
Copy link
Member Author

jyn514 commented Jul 19, 2020

Closing in favor of #903, the original bug this was for has been fixed.

@jyn514 jyn514 closed this as completed Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug E-easy Effort: Should be easy to implement and would make a good first PR mentor This has instructions for getting started
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants