Auto merge of #1675 - sgrif:sg-resilient-crate-download, r=jtgeibel

Allow download counts to fail to be updated

This ensures that the download endpoint still works even if counting the
download fails. The main case that we expect failure to occur is when
the application is in read only mode. However, even if an unexpected
failure occurs, we still want `cargo build` to succeed. Counting
downloads is always considered optional -- it's much more important that
people's builds succeed than having accurate download stats.

Note that we still require a database connection from the pool. In
theory, we could allow getting the version ID to fail as well, and just
blindly redirect to S3 no matter what, and rely on a 404 happening
there. However, this could result in successful builds in the event that
the index is out of sync with our DB, since we don't routinely clean up
the S3 bucket. Our current test behavior says that we 404 instead of
redirecting, so I've left things as is.

Once sfackler/r2d2#73 is released, my plan is to
try to get a DB connection with a shorter timeout, and if we were able
to get a DB connection, do our current behavior, but if that fails just
blindly redirect to S3. We don't expect getting a connection to fail
unless we're being DDOS'd, nor do we expect requests to this endpoint
for crates that don't exist (since they should only come for things in
the index).

Fixes #1387

Author: bors

src/controllers/version/downloads.rs

@@ -6,8 +6,6 @@ use crate::controllers::prelude::*;
 
 use chrono::{Duration, NaiveDate, Utc};
 
-use crate::Replica;
-
 use crate::models::{Crate, VersionDownload};
 use crate::schema::*;
 use crate::views::EncodableVersionDownload;
@@ -20,15 +18,7 @@ pub fn download(req: &mut dyn Request) -> CargoResult<Response> {
     let crate_name = &req.params()["crate_id"];
     let version = &req.params()["version"];
 
-    // If we are a mirror, ignore failure to update download counts.
-    // API-only mirrors won't have any crates in their database, and
-    // incrementing the download count will look up the crate in the
-    // database. Mirrors just want to pass along a redirect URL.
-    if req.app().config.mirror == Replica::ReadOnlyMirror {
-        let _ = increment_download_counts(req, crate_name, version);
-    } else {
-        increment_download_counts(req, crate_name, version)?;
-    }
+    increment_download_counts(req, crate_name, version)?;
 
     let redirect_url = req
         .app()
@@ -48,6 +38,14 @@ pub fn download(req: &mut dyn Request) -> CargoResult<Response> {
     }
 }
 
+/// Increment the download counts for a given crate version.
+///
+/// Returns an error if we could not load the version ID from the database.
+///
+/// This ignores any errors that occur updating the download count. Failure is
+/// expected if the application is in read only mode, or for API-only mirrors.
+/// Even if failure occurs for unexpected reasons, we would rather have `cargo
+/// build` succeed and not count the download than break people's builds.
 fn increment_download_counts(
     req: &dyn Request,
     crate_name: &str,
@@ -62,7 +60,9 @@ fn increment_download_counts(
         .filter(num.eq(version))
         .first(&*conn)?;
 
-    VersionDownload::create_or_increment(version_id, &conn)?;
+    // Wrap in a transaction so we don't poison the outer transaction if this
+    // fails
+    let _ = conn.transaction(|| VersionDownload::create_or_increment(version_id, &conn));
     Ok(())
 }
 

src/tests/krate.rs

@@ -1262,7 +1262,7 @@ fn download() {
 
 #[test]
 fn download_nonexistent_version_of_existing_crate_404s() {
-    let (app, anon, user) = TestApp::init().with_user();
+    let (app, anon, user) = TestApp::with_proxy().with_user();
     let user = user.as_model();
 
     app.db(|conn| {

src/tests/read_only_mode.rs

@@ -24,7 +24,6 @@ fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() {
 }
 
 #[test]
-#[ignore] // Will be implicitly fixed by #1387, no need to special case here
 fn can_download_crate_in_read_only_mode() {
     let (app, anon, user) = TestApp::with_proxy().with_user();