Make response stale in case of invalid expires

methanol/src/main/java/com/github/mizosoft/methanol/internal/cache/CacheStrategy.java

@@ -49,9 +49,16 @@ class CacheStrategy {
 
   private final CacheControl requestCacheControl;
   private final CacheControl responseCacheControl;
+
+  /** The age of the cached response. */
   private final Duration age;
+
+  /** How much time the response stays fresh from when this strategy has been computed. */
   private final Duration freshness;
+
+  /** How much time the response has been stale since this strategy had been computed. */
   private final Duration staleness;
+
   private final Optional<LocalDateTime> lastModified;
   private final Optional<String> etag;
   private final boolean usesHeuristicFreshness;
@@ -112,7 +119,7 @@ HttpRequest conditionalize(HttpRequest request) {
   }
 
   static CacheStrategy create(HttpRequest request, TrackedResponse<?> cacheResponse, Instant now) {
-    return new Factory(request, cacheResponse, now).create(now);
+    return new Factory(request, cacheResponse).create(now);
   }
 
   static final class Factory {
@@ -126,7 +133,7 @@ static final class Factory {
     final Optional<LocalDateTime> expires;
     final Optional<LocalDateTime> lastModified;
 
-    Factory(HttpRequest request, TrackedResponse<?> cacheResponse, Instant now) {
+    Factory(HttpRequest request, TrackedResponse<?> cacheResponse) {
       timeRequestSent = cacheResponse.timeRequestSent();
       timeResponseReceived = cacheResponse.timeResponseReceived();
       cacheResponseHeaders = cacheResponse.headers();
@@ -152,15 +159,15 @@ static final class Factory {
       //   expired").
       // ---
       //
-      // If Expires is invalid, we fall back to a second prior to 'now' to represent a time in tha
-      // past.
+      // So if Expires is invalid, we fall back to the furthest time in the past LocalDateTime can
+      // represent. This has the advantage over approaches like falling back to an arbitrary amount
+      // of time before the response's date, say a minute, in that it won't accidentally pass even
+      // if the user passes in a 'max-stale=60', still satisfying the server's assumed intentions.
       expires =
           cacheResponse
               .headers()
               .firstValue("Expires")
-              .map(
-                  expiresValues ->
-                      tryParseHttpDate(expiresValues).orElseGet(() -> date.minusSeconds(1)));
+              .map(expiresValues -> tryParseHttpDate(expiresValues).orElse(LocalDateTime.MIN));
     }
 
     /** Computes response's age relative to {@code now} as specified by rfc7324 Section 4.2.3. */

methanol/src/test/java/com/github/mizosoft/methanol/HttpCacheTest.java

@@ -719,8 +719,35 @@ void pastExpires(Store store) throws Exception {
   }
 
   @StoreParameterizedTest
-  void invalidExpires(Store store) throws Exception {
+  void invalidExpiresMakesResponseStale(Store store) throws Exception {
     setUpCache(store);
+
+    var date = toUtcDateTime(clock.instant());
+    server.enqueue(
+        new MockResponse()
+            .setHeader("Date", formatHttpDate(date))
+            .setHeader("Expires", -1)
+            .setBody("Pikachu"));
+    verifyThat(send(serverUri)).isCacheMiss().hasBody("Pikachu");
+
+    // Negative freshness lifetime caused by invalid Expires triggers revalidation.
+    server.enqueue(
+        new MockResponse()
+            .setHeader("Date", formatHttpDate(date))
+            .setHeader("Expires", -1)
+            .setBody("Psyduck"));
+    verifyThat(send(serverUri)).isConditionalMiss().hasBody("Psyduck");
+
+    clock.advanceSeconds(1);
+
+    // Staleness rules should still apply in case of invalid 'Expires'.
+    verifyThat(
+            send(
+                GET(serverUri)
+                    .cacheControl(
+                        CacheControl.newBuilder().maxStale(Duration.ofSeconds(1)).build())))
+        .isCacheHit()
+        .hasBody("Psyduck");
   }
 
   @StoreParameterizedTest