Allow download counts to fail to be updated

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();