fix(download): Tune HostDenyList (#1775)

* Increased the failure threshold default value to 60
* Decreased the block time default to 1h
* Added the error to download failure and block logs
* Short-circuit `register_download_failure` for already blocked hosts

Author: loewenheim

crates/symbolicator-service/src/config.rs

@@ -498,18 +498,26 @@ pub struct Config {
     ///
     /// Hosts will be put on the deny list if a certain number of downloads
     /// fail within this time window.
+    ///
+    /// Defaults to 60s.
     #[serde(with = "humantime_serde")]
     pub deny_list_time_window: Duration,
 
     /// The granularity at which download failures are tracked in the host deny list.
+    ///
+    /// Defaults to 5s.
     #[serde(with = "humantime_serde")]
     pub deny_list_bucket_size: Duration,
 
     /// The number of failures that must occur in the configured time window for a
     /// server to be put on the deny list.
+    ///
+    /// Defaults to 60.
     pub deny_list_threshold: usize,
 
     /// The duration for which a host will remain on the deny list.
+    ///
+    /// Defaults to 1h.
     #[serde(with = "humantime_serde")]
     pub deny_list_block_time: Duration,
 
@@ -616,8 +624,8 @@ impl Default for Config {
             deny_list_enabled: true,
             deny_list_time_window: Duration::from_secs(60),
             deny_list_bucket_size: Duration::from_secs(5),
-            deny_list_threshold: 20,
-            deny_list_block_time: Duration::from_secs(24 * 60 * 60),
+            deny_list_threshold: 60,
+            deny_list_block_time: Duration::from_secs(60 * 60),
             deny_list_never_block_hosts: Vec::new(),
             timeouts: DownloadTimeouts::default(),
             // This value is tuned according to Symbolicator's observed real-world performance.

crates/symbolicator-service/src/download/mod.rs

@@ -135,6 +135,8 @@ impl HostDenyList {
             block_time: config.deny_list_block_time,
             never_block: config.deny_list_never_block_hosts.clone(),
             failures: moka::sync::Cache::builder()
+                // If a host hasn't had download failures for an entire `deny_list_time_window`
+                // we can remove it from the `failures` cache.
                 .time_to_idle(config.deny_list_time_window)
                 .build(),
             blocked_hosts: moka::sync::Cache::builder()
@@ -161,12 +163,19 @@ impl HostDenyList {
     ///
     /// If that puts the host over the threshold, it is added
     /// to the blocked servers.
-    fn register_failure(&self, source_name: &str, host: String) {
+    fn register_failure(&self, source_name: &str, host: String, error: &CacheError) {
         let current_ts = SystemTime::now();
 
+        // Sanity check: we don't need to count failures for hosts which are currently blocked.
+        // This can happen if multiple download requests fail around the same time.
+        if self.is_blocked(&host) {
+            return;
+        }
+
         tracing::trace!(
             host = %host,
             time = %humantime::format_rfc3339(current_ts),
+            %error,
             "Registering download failure"
         );
 
@@ -205,6 +214,7 @@ impl HostDenyList {
             tracing::info!(
                 %host,
                 block_time = %humantime::format_duration(self.block_time),
+                %error,
                 "Blocking host due to too many download failures"
             );
 
@@ -386,7 +396,7 @@ impl DownloadService {
             if let Some(ref deny_list) = self.host_deny_list
                 && source_can_be_blocked
             {
-                deny_list.register_failure(&source_metric_key, host);
+                deny_list.register_failure(&source_metric_key, host, e);
             }
         }
 
@@ -1211,12 +1221,20 @@ mod tests {
         let deny_list = HostDenyList::from_config(&config);
         let host = String::from("test");
 
-        deny_list.register_failure("test", host.clone());
+        deny_list.register_failure(
+            "test",
+            host.clone(),
+            &CacheError::DownloadError("Test error".to_owned()),
+        );
 
         // shouldn't be blocked after one failure
         assert!(!deny_list.is_blocked(&host));
 
-        deny_list.register_failure("test", host.clone());
+        deny_list.register_failure(
+            "test",
+            host.clone(),
+            &CacheError::DownloadError("Test error".to_owned()),
+        );
 
         // should be blocked after two failures
         assert!(deny_list.is_blocked(&host));
@@ -1240,8 +1258,16 @@ mod tests {
         let deny_list = HostDenyList::from_config(&config);
         let host = String::from("test");
 
-        deny_list.register_failure("test", host.clone());
-        deny_list.register_failure("test", host.clone());
+        deny_list.register_failure(
+            "test",
+            host.clone(),
+            &CacheError::DownloadError("Test error".to_owned()),
+        );
+        deny_list.register_failure(
+            "test",
+            host.clone(),
+            &CacheError::DownloadError("Test error".to_owned()),
+        );
 
         assert!(!deny_list.is_blocked(&host));
     }