Crates.io incident report for 2020-02-20
blog.rust-lang.org
blog.rust-lang.org
In this case, the git2 crate (https://docs.rs/git2/) is a thin wrapping over libgit2 (https://libgit2.org). To ensure a push went through properly you have to watch what happens inside a callback. The docs state the following:
Note that you'll likely want to use RemoteCallbacks and set push_update_reference to test whether all the references were pushed successfully.
It looks like the only sensible way to utilize this is by closing over mutable state and then checking that imperatively, which is far from idiomatic (this is what they do in the patch). If you were pushing multiple refs it would be even worse!
Usually, when you use some rust code, it's pretty easy to tell what can fail simply by looking at the types - this is a huge win. Thin wrappers can subtly violate this intuition: sometimes errors are handled idiomatically and sometimes they aren't, leading to a sort of false sense of security.
The takeaway here is to use an abundance of caution when utilizing a thin wrapper and to understand just how thick it is, especially with errors.
Unfortunately common. That said, I think the best route is usually a thin wrapper module, and then a module that attempts to map that to something more idiomatic. At least then there's a clear place where changes/additions in the underlying library should be handled. I've lived the problem of multiple modules that each try to deal with mapping the underlying library itself to an idiomatic interface. They're good for a while, but they always seem much more susceptible to drift in the underlying library causing problems over time.
I think there are two alternatives of increasing difficulty: 1. Write a thicker wrapping around git2 (possibly in a different crate) that achieves an idiomatic API. 2. Write an equivalent to libgit2 in rust.
It’s true that 2 has some significant downsides and probably isn’t worth the effort but it certainly would’ve prevented this bug.
The root cause was likely Github returning 200 when they should've returned a 5xx error.
A better solution in this case is to verify the push worked by looking at the remote for a ref in the push before returning success. I would never just depend on git2 to libgit2 callbacks succeeding as "proof" that it worked, because there's too much to go wrong.
Also, the crates.io patch should be upstreamed into git2 by wrapping the block. And, the ultimate fix is to address libgit2 to make sure it's handling errors correctly. It's not a very good fix, but it's better than what exists now.
Furthermore, it seems like a pure Rust git would be better than problematic wrapper layers stacked one on of top of the other.
I agree though that a pure rust implementation of libgit2 would be ideal.
I didn’t realize how complicated the backend tooling is to achieve the same portability as C - it seems so much simpler as a consumer of the toolchain. IIRC the bootstrapping problem is still something distros are fighting with today, but hopefully one day we can have rustc bundled with all the other standard build tools.
Simplest solution at the scale and level of reliability that I've had experience with is to effectively say that master is what's live. In other words, a merge to master triggers a deploy under normal circumstances. It'll save you a lot of headaches. That plus one command/click deploy last known good build and feature flagging all major updates pretty much removes the whole deploy process as a significant failure risk.
> those sort of complex deploys block other people's work
I think this is the root cause of your concerns, the database migrations seem to always be backwards incompatible and therefore deployments and migrations need to be synced, blocking other changes.
Typically CI/CD pipelines are never blocked.
Making sure the database migration / code change is backwards compatible is minimally more tedious (i.e. multiple incremental changes across multiple days), but overall speeds up development.
The decision to always be backwards compatible is just that, a decision. It comes with some really nice benefits around CI/CD, but also adds overhead for certain kinds of development, especially if data models are evolving rapidly. I've been in situations where we made the explicit decision not be backwards compatible due to business context that prioritized these evolutions.
Likewise, deploying to production continuously is not always an option. I've been in situations where customer relationships (contractual or otherwise) require that changes not be made continuously. Accidental outages or regressions are simply not an acceptable risk in some business contexts.
I've seen staged releases work quite well at multiple companies and fail miserably at some others.
That said, continuous deployment is the right choice in many/most contexts. I'd just prefer that people see it as a choice instead of assuming that it's the only way that can work.
I'd love to see an action item to address this. The CI/CD system should be pushing all changes in master to live immediately. Or does a CI/CD system need to be setup for this?
> Deploying the change took way longer than expected
what does this mean? how much longer is "way longer"? what was your RTO goal, how did you fail to meet it, and how will you meet it in the future?
> We should also strive to reduce the amount of time PRs sit in master without being live.
not a plan. everyones striving at whatever their doing. were you not striving before? how will your accomplish your goal of minimizing non-live time?
> It took 1 hour and 31 minutes from the start of the incident to the deploy of the fix.
"deploy of the fix" is an unclear statement. does that mean when code was pushed, or when you verified your clients no longer had problems?
Yes, i saw and it did not clarify my question to which you are responding which is obvious because...
> all ill effects of the bug were addressed and service was fully restored.
...this is not true. The timeline says a customer problem was submitted the next day, and additional action was required to ensure the "service was fully restored"