Auto merge of #3564 - pietroalbini:yolo, r=jtgeibel

Do unconditional redirects for downloads when the db is broken

We had a few database-related outages in the recent weeks, which impacted people downloading crates with Cargo. Download requests are the majority of the traffic we serve, and they're also the most critical ones. Even if the rest of crates.io stops working downloads *must* continue to be operational to reduce the impact on our users.

As a preface, the downloads endpoint used the database for two reasons: counting the downloads (which can be skipped during outages) and ensuring the crate name is canonicalized. For example, if someone tries to  `Foo-bar 1.0.0` by calling `/api/v1/crates/foo_Bar/1.0.0/download` the crates.io application canonicalizes the name and redirects the user to `https://static.crates.io/crates/Foo-bar/Foo-bar-1.0.0.crate`. If we were not to perform canonicalization the user would be redirected to `https://static.crates.io/crates/foo_Bar/foo_Bar-1.0.0.crate`, which would result in a 404.

While analyzing the outages and after investigating the traffic patterns of multiple weeks, we identified that the vast majority of downloads (including *all* downloads from Cargo) already send the canonical name to crates.io, and only a couple of third-party tools send crate names that are not canonicalized.

This PR changes the downloads endpoint to do unconditional redirects without canonicalizing the crate names **during a full database outage**. During normal operations the canonicalization will still be performed. The change will allow Cargo builds to continue functioning even without a database, and the really small percentage of requests will get a 404 instead of a 500, which in my view is an acceptable tradeoff.

This PR also adds two metrics:

* `cratesio_instance_downloads_non_canonical_crate_name_total`: how many download requests we received with a non-canonical crate name. This will allow us to revisit whether the tradeoff is still worth, and it's way easier to query than the previous method we used to extract the data.
* `cratesio_instance_downloads_unconditional_redirects_total`: how many unconditional redirects we performed. We'll want to setup an alert that pages us as soon as the metric is `> 0`.

Finally, this PR implements full tests for the changes introduced here (tests are actually the bulk of the diff). The test uses the "real" database pool instead of the dummy one used for the rest of the tests, acting on a fresh schema. Creating the fresh schema takes a second or two, so we're only doing it in this test to avoid slowing down the test suite. Also, instead of connecting directly to PostgreSQL the database the pool connects through `ChaosProxy`, a simple proxy implemented in our codebase that allows to break or restore the connection at will.

Part of #3541
r? `@jtgeibel`

Author: bors

Cargo.lock

@@ -81,7 +81,7 @@ sha2 = "0.9"
 swirl = { git = "https://github.com/sgrif/swirl.git", rev = "e87cf37" }
 tar = "0.4.16"
 tempfile = "3"
-tokio = { version = "1", features = ["net", "signal", "io-std", "io-util", "rt-multi-thread"]}
+tokio = { version = "1.5.0", features = ["net", "signal", "io-std", "io-util", "rt-multi-thread", "macros"]}
 toml = "0.5"
 tracing = "0.1"
 tracing-subscriber = "0.2"
@@ -92,7 +92,7 @@ claim = "0.5"
 conduit-test = "0.9.0-alpha.4"
 hyper-tls = "0.5"
 lazy_static = "1.0"
-tokio = "1"
+tokio = "1.5.0"
 tower-service = "0.3.0"
 
 [build-dependencies]

Cargo.toml

@@ -1,6 +1,7 @@
 //! Application-wide components in a struct accessible from each request
 
-use crate::{db, Config, Env};
+use crate::db::{ConnectionConfig, DieselPool};
+use crate::{Config, Env};
 use std::{sync::Arc, time::Duration};
 
 use crate::downloads_counter::DownloadsCounter;
@@ -18,10 +19,10 @@ use scheduled_thread_pool::ScheduledThreadPool;
 #[allow(missing_debug_implementations)]
 pub struct App {
     /// The primary database connection pool
-    pub primary_database: db::DieselPool,
+    pub primary_database: DieselPool,
 
     /// The read-only replica database connection pool
-    pub read_only_replica_database: Option<db::DieselPool>,
+    pub read_only_replica_database: Option<DieselPool>,
 
     /// GitHub API client
     pub github: GitHubClient,
@@ -103,38 +104,45 @@ impl App {
             _ => 30,
         };
 
-        let primary_db_connection_config = db::ConnectionConfig {
-            statement_timeout: db_connection_timeout,
-            read_only: config.db_primary_config.read_only_mode,
-        };
-
         let thread_pool = Arc::new(ScheduledThreadPool::new(db_helper_threads));
 
-        let primary_db_config = r2d2::Pool::builder()
-            .max_size(db_pool_size)
-            .min_idle(db_min_idle)
-            .connection_timeout(Duration::from_secs(db_connection_timeout))
-            .connection_customizer(Box::new(primary_db_connection_config))
-            .thread_pool(thread_pool.clone());
-
-        let primary_database =
-            db::diesel_pool(&config.db_primary_config.url, config.env, primary_db_config);
-
-        let replica_database = if let Some(url) = config.db_replica_config.as_ref().map(|c| &c.url)
-        {
-            let replica_db_connection_config = db::ConnectionConfig {
+        let primary_database = if config.use_test_database_pool {
+            DieselPool::new_test(&config.db_primary_config.url)
+        } else {
+            let primary_db_connection_config = ConnectionConfig {
                 statement_timeout: db_connection_timeout,
-                read_only: true,
+                read_only: config.db_primary_config.read_only_mode,
             };
 
-            let replica_db_config = r2d2::Pool::builder()
+            let primary_db_config = r2d2::Pool::builder()
                 .max_size(db_pool_size)
                 .min_idle(db_min_idle)
                 .connection_timeout(Duration::from_secs(db_connection_timeout))
-                .connection_customizer(Box::new(replica_db_connection_config))
-                .thread_pool(thread_pool);
+                .connection_customizer(Box::new(primary_db_connection_config))
+                .thread_pool(thread_pool.clone());
+
+            DieselPool::new(&config.db_primary_config.url, primary_db_config)
+        };
 
-            Some(db::diesel_pool(&url, config.env, replica_db_config))
+        let replica_database = if let Some(url) = config.db_replica_config.as_ref().map(|c| &c.url)
+        {
+            if config.use_test_database_pool {
+                Some(DieselPool::new_test(url))
+            } else {
+                let replica_db_connection_config = ConnectionConfig {
+                    statement_timeout: db_connection_timeout,
+                    read_only: true,
+                };
+
+                let replica_db_config = r2d2::Pool::builder()
+                    .max_size(db_pool_size)
+                    .min_idle(db_min_idle)
+                    .connection_timeout(Duration::from_secs(db_connection_timeout))
+                    .connection_customizer(Box::new(replica_db_connection_config))
+                    .thread_pool(thread_pool);
+
+                Some(DieselPool::new(&url, replica_db_config))
+            }
         } else {
             None
         };

src/app.rs

@@ -2,10 +2,9 @@ use reqwest::blocking::Client;
 use std::panic::AssertUnwindSafe;
 use std::sync::{Arc, Mutex, MutexGuard, PoisonError};
 
-use diesel::r2d2::PoolError;
 use swirl::PerformError;
 
-use crate::db::{DieselPool, DieselPooledConn};
+use crate::db::{DieselPool, DieselPooledConn, PoolError};
 use crate::git::Repository;
 use crate::uploaders::Uploader;
 

src/background_jobs.rs

@@ -22,6 +22,7 @@ pub struct Config {
     pub downloads_persist_interval_ms: usize,
     pub ownership_invitations_expiration_days: u64,
     pub metrics_authorization_token: Option<String>,
+    pub use_test_database_pool: bool,
 }
 
 #[derive(Debug)]
@@ -211,6 +212,7 @@ impl Default for Config {
                 .unwrap_or(60_000), // 1 minute
             ownership_invitations_expiration_days: 30,
             metrics_authorization_token: dotenv::var("METRICS_AUTHORIZATION_TOKEN").ok(),
+            use_test_database_pool: false,
         }
     }
 }

src/config.rs

@@ -2,53 +2,84 @@
 //!
 //! Crate level functionality is located in `krate::downloads`.
 
+use super::{extract_crate_name_and_semver, version_and_crate};
 use crate::controllers::prelude::*;
-
-use chrono::{Duration, NaiveDate, Utc};
-
+use crate::db::PoolError;
 use crate::models::{Crate, VersionDownload};
 use crate::schema::*;
 use crate::views::EncodableVersionDownload;
-
-use super::{extract_crate_name_and_semver, version_and_crate};
+use chrono::{Duration, NaiveDate, Utc};
 
 /// Handles the `GET /crates/:crate_id/:version/download` route.
 /// This returns a URL to the location where the crate is stored.
 pub fn download(req: &mut dyn RequestExt) -> EndpointResult {
+    let app = req.app().clone();
     let recorder = req.timing_recorder();
 
-    let crate_name = &req.params()["crate_id"];
-    let version = &req.params()["version"];
-
-    let (version_id, canonical_crate_name): (_, String) = {
-        use self::versions::dsl::*;
-
-        let conn = recorder.record("get_conn", || req.db_conn())?;
-
-        // Returns the crate name as stored in the database, or an error if we could
-        // not load the version ID from the database.
-        recorder.record("get_version", || {
-            versions
-                .inner_join(crates::table)
-                .select((id, crates::name))
-                .filter(Crate::with_name(crate_name))
-                .filter(num.eq(version))
-                .first(&*conn)
-        })?
-    };
-
-    // The increment does not happen instantly, but it's deferred to be executed in a batch
-    // along with other downloads. See crate::downloads_counter for the implementation.
-    req.app().downloads_counter.increment(version_id);
+    let mut crate_name = req.params()["crate_id"].clone();
+    let version = req.params()["version"].as_str();
+
+    let mut log_metadata = None;
+    match recorder.record("get_conn", || req.db_conn()) {
+        Ok(conn) => {
+            use self::versions::dsl::*;
+
+            // Returns the crate name as stored in the database, or an error if we could
+            // not load the version ID from the database.
+            let (version_id, canonical_crate_name) = recorder.record("get_version", || {
+                versions
+                    .inner_join(crates::table)
+                    .select((id, crates::name))
+                    .filter(Crate::with_name(&crate_name))
+                    .filter(num.eq(version))
+                    .first::<(i32, String)>(&*conn)
+            })?;
+
+            if canonical_crate_name != crate_name {
+                app.instance_metrics
+                    .downloads_non_canonical_crate_name_total
+                    .inc();
+                log_metadata = Some(("bot", "dl"));
+            }
+            crate_name = canonical_crate_name;
+
+            // The increment does not happen instantly, but it's deferred to be executed in a batch
+            // along with other downloads. See crate::downloads_counter for the implementation.
+            app.downloads_counter.increment(version_id);
+        }
+        Err(PoolError::UnhealthyPool) => {
+            // The download endpoint is the most critical route in the whole crates.io application,
+            // as it's relied upon by users and automations to download crates. Keeping it working
+            // is the most important thing for us.
+            //
+            // The endpoint relies on the database to fetch the canonical crate name (with the
+            // right capitalization and hyphenation), but that's only needed to serve clients who
+            // don't call the endpoint with the crate's canonical name.
+            //
+            // Thankfully Cargo always uses the right name when calling the endpoint, and we can
+            // keep it working during a full database outage by unconditionally redirecting without
+            // checking whether the crate exists or the rigth name is used. Non-Cargo clients might
+            // get a 404 response instead of a 500, but that's worth it.
+            //
+            // Without a working database we also can't count downloads, but that's also less
+            // critical than keeping Cargo downloads operational.
+
+            app.instance_metrics
+                .downloads_unconditional_redirects_total
+                .inc();
+            log_metadata = Some(("unconditional_redirect", "true"));
+        }
+        Err(err) => return Err(err.into()),
+    }
 
     let redirect_url = req
         .app()
         .config
         .uploader
-        .crate_location(&canonical_crate_name, version);
+        .crate_location(&crate_name, &*version);
 
-    if &canonical_crate_name != crate_name {
-        req.log_metadata("bot", "dl");
+    if let Some((key, value)) = log_metadata {
+        req.log_metadata(key, value);
     }
 
     if req.wants_json() {