Add FORCE_UNCONDITIONAL_REDIRECTS

src/config.rs

@@ -27,6 +27,7 @@ pub struct Server {
     pub metrics_authorization_token: Option<String>,
     pub use_test_database_pool: bool,
     pub instance_metrics_log_every_seconds: Option<u64>,
+    pub force_unconditional_redirects: bool,
 }
 
 impl Default for Server {
@@ -55,6 +56,8 @@ impl Default for Server {
     ///   will occur.
     /// - `INSTANCE_METRICS_LOG_EVERY_SECONDS`: How frequently should instance metrics be logged.
     ///   If the environment variable is not present instance metrics are not logged.
+    /// - `FORCE_UNCONDITIONAL_REDIRECTS`: Whether to force unconditional redirects in the download
+    ///   endpoint even with a healthy database pool.
     ///
     /// # Panics
     ///
@@ -96,6 +99,7 @@ impl Default for Server {
             metrics_authorization_token: dotenv::var("METRICS_AUTHORIZATION_TOKEN").ok(),
             use_test_database_pool: false,
             instance_metrics_log_every_seconds: env_optional("INSTANCE_METRICS_LOG_EVERY_SECONDS"),
+            force_unconditional_redirects: dotenv::var("FORCE_UNCONDITIONAL_REDIRECTS").is_ok(),
         }
     }
 }

src/controllers/version/downloads.rs

@@ -18,9 +18,22 @@ pub fn download(req: &mut dyn RequestExt) -> EndpointResult {
     let mut crate_name = req.params()["crate_id"].clone();
     let version = req.params()["version"].as_str();
 
-    let mut log_metadata = None;
+    // When no database connection is ready unconditional redirects will be performed. This could
+    // happen if the pool is not healthy or if an operator manually configured the application to
+    // always perform unconditional redirects (for example as part of the mitigations for an
+    // outage). See the comments below for a description of what unconditional redirects do.
+    let conn = if app.config.force_unconditional_redirects {
+        None
+    } else {
         match req.db_conn() {
-        Ok(conn) => {
+            Ok(conn) => Some(conn),
+            Err(PoolError::UnhealthyPool) => None,
+            Err(err) => return Err(err.into()),
+        }
+    };
+
+    let mut log_metadata = None;
+    if let Some(conn) = &conn {
         use self::versions::dsl::*;
 
         // Returns the crate name as stored in the database, or an error if we could
@@ -34,7 +47,7 @@ pub fn download(req: &mut dyn RequestExt) -> EndpointResult {
                     .select((id, crates::name))
                     .filter(Crate::with_name(&crate_name))
                     .filter(num.eq(version))
-                        .first::<(i32, String)>(&*conn)
+                    .first::<(i32, String)>(&**conn)
             })?;
 
         if canonical_crate_name != crate_name {
@@ -48,8 +61,7 @@ pub fn download(req: &mut dyn RequestExt) -> EndpointResult {
         // 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) => {
+    } else {
         // 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.
@@ -71,8 +83,10 @@ pub fn download(req: &mut dyn RequestExt) -> EndpointResult {
             .inc();
         log_metadata = Some(("unconditional_redirect", "true"));
     }
-        Err(err) => return Err(err.into()),
-    }
+
+    // Ensure the connection is released to the pool as soon as possible, as the download endpoint
+    // covers the majority of our traffic and we don't want it to starve other requests.
+    drop(conn);
 
     let redirect_url = req
         .app()

src/tests/krate/downloads.rs

@@ -105,3 +105,34 @@ fn download_noncanonical_crate_name() {
     anon.get::<()>("/api/v1/crates/foo-download/1.0.0/download")
         .assert_redirect_ends_with("/crates/foo_download/foo_download-1.0.0.crate");
 }
+
+#[test]
+fn force_unconditional_redirect() {
+    let (app, anon, user) = TestApp::init()
+        .with_config(|config| {
+            config.force_unconditional_redirects = true;
+        })
+        .with_user();
+
+    app.db(|conn| {
+        CrateBuilder::new("foo-download", user.as_model().id)
+            .version(VersionBuilder::new("1.0.0"))
+            .expect_build(conn);
+    });
+
+    // Any redirect to an existing crate and version works correctly.
+    anon.get::<()>("/api/v1/crates/foo-download/1.0.0/download")
+        .assert_redirect_ends_with("/crates/foo-download/foo-download-1.0.0.crate");
+
+    // Redirects to crates with wrong capitalization are performed unconditionally.
+    anon.get::<()>("/api/v1/crates/Foo_downloaD/1.0.0/download")
+        .assert_redirect_ends_with("/crates/Foo_downloaD/Foo_downloaD-1.0.0.crate");
+
+    // Redirects to missing versions are performed unconditionally.
+    anon.get::<()>("/api/v1/crates/foo-download/2.0.0/download")
+        .assert_redirect_ends_with("/crates/foo-download/foo-download-2.0.0.crate");
+
+    // Redirects to missing crates are performed unconditionally.
+    anon.get::<()>("/api/v1/crates/bar-download/1.0.0/download")
+        .assert_redirect_ends_with("/crates/bar-download/bar-download-1.0.0.crate");
+}

src/tests/util/test_app.rs

@@ -334,6 +334,7 @@ fn simple_config() -> config::Server {
         metrics_authorization_token: None,
         use_test_database_pool: true,
         instance_metrics_log_every_seconds: None,
+        force_unconditional_redirects: false,
     }
 }