Never cache errors on disk

Update Cache::store and Cache::load to skip CacheStatus::Error. As a result, failures are always rechecked on subsequent runs. We completely sidestep issues with caching transient network errors or missing HTTP status codes.

Fixes #1734.
Fixes #1783.

Author: mre

lychee-bin/src/cache.rs

@@ -46,6 +46,10 @@ impl StoreExt for Cache {
             .has_headers(false)
             .from_path(path)?;
         for result in self {
+            // Do not serialize errors to disk. We always want to recheck failing links.
+            if matches!(result.value().status, CacheStatus::Error(_)) {
+                continue;
+            }
             wtr.serialize((result.key(), result.value()))?;
         }
         Ok(())
@@ -60,7 +64,7 @@ impl StoreExt for Cache {
             .has_headers(false)
             .from_path(path)?;
 
-        let map = DashMap::new();
+        let map = Cache::new();
         let current_ts = timestamp();
         for result in rdr.deserialize() {
             let (uri, value): (Uri, CacheValue) = result?;
@@ -70,6 +74,14 @@ impl StoreExt for Cache {
                 continue;
             }
 
+            // Discard errors. Caching errors goes against typical CI workflows.
+            // If a link fails due to a network issue, a server outage, or if a previously
+            // failing link has been fixed, reading an error from the cache prevents lychee
+            // from realizing the link is now working.
+            if matches!(value.status, CacheStatus::Error(_)) {
+                continue;
+            }
+
             // Discard entries for status codes which have been excluded.
             // Without this check, an entry might be cached, then its status code is configured as
             // excluded, and in subsequent runs the cached value is still reused.
@@ -85,7 +97,6 @@ impl StoreExt for Cache {
 
 #[cfg(test)]
 mod tests {
-    use dashmap::DashMap;
     use http::StatusCode;
     use lychee_lib::{CacheStatus, StatusCodeSelector, StatusRange, Uri};
 
@@ -98,7 +109,7 @@ mod tests {
     fn test_excluded_status_not_reused_from_cache() {
         let uri: Uri = "https://example.com".try_into().unwrap();
 
-        let cache: Cache = DashMap::<Uri, CacheValue>::new();
+        let cache = Cache::new();
         cache.insert(
             uri.clone(),
             CacheValue {
@@ -116,4 +127,34 @@ mod tests {
         let cache = Cache::load(tmp.path(), u64::MAX, &excluder).unwrap();
         assert!(cache.get(&uri).is_none());
     }
+
+    #[test]
+    fn test_errors_not_stored_in_cache() {
+        let uri: Uri = "https://example.com/error".try_into().unwrap();
+
+        let cache = Cache::new();
+        cache.insert(
+            uri.clone(),
+            CacheValue {
+                status: CacheStatus::Error(Some(StatusCode::INTERNAL_SERVER_ERROR)),
+                timestamp: timestamp(),
+            },
+        );
+        let uri_none: Uri = "https://example.com/none".try_into().unwrap();
+        cache.insert(
+            uri_none.clone(),
+            CacheValue {
+                status: CacheStatus::Error(None),
+                timestamp: timestamp(),
+            },
+        );
+
+        let tmp = tempfile::NamedTempFile::new().unwrap();
+        cache.store(tmp.path()).unwrap();
+
+        let excluder = StatusCodeSelector::empty();
+        let loaded_cache = Cache::load(tmp.path(), u64::MAX, &excluder).unwrap();
+        assert!(loaded_cache.get(&uri).is_none());
+        assert!(loaded_cache.get(&uri_none).is_none());
+    }
 }

lychee-bin/tests/cli.rs

@@ -1316,8 +1316,8 @@ The config file should contain every possible key for documentation purposes."
             "Missing OK entry in cache"
         );
         assert!(
-            data.contains(&format!("{}/,404", mock_server_err.uri())),
-            "Missing error entry in cache"
+            !data.contains(&format!("{}/,404", mock_server_err.uri())),
+            "Error entry should not be cached"
         );
 
         // Run again to verify cache behavior
@@ -1327,7 +1327,7 @@ The config file should contain every possible key for documentation purposes."
                 mock_server_ok.uri()
             )))
             .stderr(contains(format!(
-                "[404] {}/ (at 2:1) | Error (cached)\n",
+                "[404] {}/ (at 2:1) | Rejected status code: 404 Not Found (configurable with \"accept\" option)\n",
                 mock_server_err.uri()
             )));
 
@@ -1454,8 +1454,10 @@ The config file should contain every possible key for documentation purposes."
         // check content of cache file
         let data = fs::read_to_string(&cache_file)?;
         assert!(data.contains(&format!("{}/,200", mock_server_ok.uri())));
-        assert!(data.contains(&format!("{}/,418", mock_server_teapot.uri())));
-        assert!(data.contains(&format!("{}/,500", mock_server_server_error.uri())));
+        // Because the first run DID NOT use `--accept`, 418 and 500 were both treated as errors.
+        // With our new error dropping logic, NEITHER gets cached in the first run.
+        assert!(!data.contains(&format!("{}/,418", mock_server_teapot.uri())));
+        assert!(!data.contains(&format!("{}/,500", mock_server_server_error.uri())));
 
         // run again to verify cache behavior
         // this time accept 418 and 500 as valid status codes
@@ -1466,11 +1468,11 @@ The config file should contain every possible key for documentation purposes."
             .assert()
             .success()
             .stderr(contains(format!(
-                "[418] {}/ (at 2:1) | OK (cached)",
+                "[418] {}/ (at 2:1) | 418 I'm a teapot: I'm a teapot",
                 mock_server_teapot.uri()
             )))
             .stderr(contains(format!(
-                "[500] {}/ (at 3:1) | OK (cached)",
+                "[500] {}/ (at 3:1) | 500 Internal Server Error: Internal Server Error",
                 mock_server_server_error.uri()
             )));
 
@@ -1601,8 +1603,11 @@ The config file should contain every possible key for documentation purposes."
         // If the status code was 999, the cache file should be empty
         // because we do not want to cache unknown status codes
         let buf = fs::read(&cache_file).unwrap();
-        let data = String::from_utf8(buf)?;
-        assert!(data.contains(",999,"));
+        assert!(
+            buf.is_empty(),
+            "cache file should be empty, but was {}",
+            String::from_utf8_lossy(&buf)
+        );
 
         Ok(())
     }
@@ -3753,4 +3758,42 @@ https://lychee.cli.rs/guides/cli/#fragments-ignored
             .assert()
             .success();
     }
+
+    /// Verifies that loading an older, legacy `.lycheecache` file containing a cached error
+    /// correctly drops the error and successfully retries the link.
+    /// This ensures we don't break existing user CI workflows that have older cache files
+    /// stored before we stopped caching errors.
+    #[tokio::test]
+    async fn test_legacy_cache_file_ignores_errors() -> Result<()> {
+        let ts = std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .unwrap()
+            .as_secs();
+        let dir = tempfile::tempdir()?;
+        let base_path = dir.path();
+        let cache_file = base_path.join(LYCHEE_CACHE_FILE);
+
+        // A server that is currently returning OK
+        let mock_server_ok = mock_server!(StatusCode::OK);
+
+        // Simulate an older `.lycheecache` where this exact URL previously failed with 404
+        // We use a future timestamp ({ts}) so it doesn't expire
+        fs::write(&cache_file, format!("{},404,{ts}\n", mock_server_ok.uri()))?;
+
+        let mut file = File::create(dir.path().join("input.txt"))?;
+        writeln!(file, "{}", mock_server_ok.uri())?;
+
+        // Run lychee. It should ignore the 404 from the cache, actually check the mock server (which returns 200), and succeed.
+        cargo_bin_cmd!()
+            .current_dir(base_path)
+            .arg("input.txt")
+            .arg("--cache")
+            .arg("--verbose")
+            .assert()
+            .success()
+            .stdout(contains("1 OK"))
+            .stdout(contains("0 Errors"));
+
+        Ok(())
+    }
 }