fix(cache): never cache errors on disk (#1734)

After thinking about this for a while, I think caching errors goes against typical CI workflows.

That's because 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. Right now it will load the broken link from cache, notice that it has no status code associated with it, and fail immediately. It just caches the error again until it expires and gets re-checked. That's quite unintuitive.

This commit updates 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.

It updates CLI tests to assert that errors are no longer cached (correcting prior tests that had their assertions flipped to expect the buggy behavior), and explicitly adds a new test to verify that loading an older, legacy `.lycheecache` file which contains a cached error correctly drops it and successfully retries the link.

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,11 @@ 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.
+        println!("DATA: {}", data);
+        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 +1469,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 +1604,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(())
     }
@@ -3697,4 +3703,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(())
+    }
 }